Skip to content

Commit

Permalink
Change logic in handler,db,events and align rhsso_test to new functio…
Browse files Browse the repository at this point in the history 
…n headers
  • Loading branch information
@dudyas6
dudyas6 committed Nov 17, 2024
1 parent 1d3a1cf commit b5e60d1
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 28 deletions.
2 changes: 1 addition & 1 deletion internal/common/db.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ func GetInfraEnvHostsFromDB(db *gorm.DB, infraEnvID strfmt.UUID) ([]*Host, error
func GetInfraEnvsFromDBWhere(db *gorm.DB, where ...interface{}) ([]*InfraEnv, error) {
var infraEnvs []*InfraEnv

err := db.Find(&infraEnvs, where...).Error
err := db.Joins("LEFT JOIN clusters on infra_envs.cluster_id = clusters.id").Find(&infraEnvs, where...).Error
if err != nil {
return nil, err
}
Expand Down
16 changes: 9 additions & 7 deletions internal/events/event.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -351,16 +351,19 @@ func (e Events) prepareEventsTable(ctx context.Context, tx *gorm.DB, clusterID *
return tx
}

//When using rhsso auth, required to have clusters fields and infra_envs fields
tx = tx.Joins("LEFT JOIN infra_envs ON infra_envs.id = events.infra_env_id").
Joins("LEFT JOIN clusters ON events.cluster_id = clusters.id")

//for bound events that are searched with cluster id (whether on clusters, bound infra-env ,
//host bound to a cluster or registered to a bound infra-env) check the access permission
//relative to the cluster ownership
if clusterBoundEvents() {
tx = tx.Model(&common.Event{}).Select("events.*, clusters.user_name, clusters.org_id").
Joins("INNER JOIN clusters ON clusters.id = events.cluster_id")
tx = tx.Model(&common.Event{}).Select("events.*, clusters.openshift_cluster_id, clusters.user_name, clusters.org_id")

// if deleted hosts flag is true, we need to add 'deleted_at' to know whether events are related to a deleted host
if swag.BoolValue(deletedHosts) {
tx = tx.Select("events.*, clusters.user_name, clusters.org_id, hosts.deleted_at").
tx = tx.Select("events.*, clusters.openshift_cluster_id, clusters.user_name, clusters.org_id, hosts.deleted_at").
Joins("LEFT JOIN hosts ON hosts.id = events.host_id")
}
return tx
Expand All @@ -369,15 +372,14 @@ func (e Events) prepareEventsTable(ctx context.Context, tx *gorm.DB, clusterID *
//for unbound events that are searched with infra-env id (whether events on hosts or the
//infra-env level itself) check the access permission relative to the infra-env ownership
if nonBoundEvents() {
return tx.Model(&common.Event{}).Select("events.*, infra_envs.user_name, infra_envs.org_id").
Joins("INNER JOIN infra_envs ON infra_envs.id = events.infra_env_id")
return tx.Model(&common.Event{}).
Select("events.*, infra_envs.user_name, infra_envs.org_id, clusters.openshift_cluster_id")
}

// Events must be linked to the infra_envs table and then to the hosts table
// The hosts table does not hold an org_id, so permissions related fields must be supplied by the infra_env
if hostOnlyEvents() {
return tx.Model(&common.Event{}).Select("events.*, infra_envs.user_name, infra_envs.org_id").
Joins("INNER JOIN infra_envs ON infra_envs.id = events.infra_env_id").
return tx.Model(&common.Event{}).Select("events.*, infra_envs.user_name, infra_envs.org_id, clusters.openshift_cluster_id").
Joins("INNER JOIN hosts ON hosts.id = events.host_id"). // This join is here to ensure that only events for a host that exists are fetched
Where("hosts.deleted_at IS NULL") // Only interested in active hosts
}
Expand Down
20 changes: 17 additions & 3 deletions pkg/auth/rhsso_authz_handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,25 @@ func (a *AuthzHandler) OwnedBy(ctx context.Context, db *gorm.DB, resource Resour
if err != nil {
return nil, err
}
if resource != ClusterResource {
return db.Where("cluster_id IN ?", allowedClusterID), nil

query := ""
switch resource {
case InfraEnvResource:
query = "infra_envs.user_name = ? OR cluster_id IN (?) OR openshift_cluster_id in (?)"
case EventsResource:
query = "infra_envs.user_name = ? OR events.cluster_id IN (?) OR openshift_cluster_id in (?)"
default:
query = "user_name = ? OR id IN (?) OR openshift_cluster_id in (?)"
}

return db.Where("id IN ? OR openshift_cluster_id IN ?", allowedClusterID, allowedClusterUuids), nil
// querys := map[Resource]string{
// ClusterResource: "user_name = ? OR id IN (?) OR openshift_cluster_id in (?)",
// InfraEnvResource: "infra_envs.user_name = ? OR cluster_id IN (?) OR openshift_cluster_id in (?)",
// EventsResource: "infra_envs.user_name = ? OR events.cluster_id IN (?) OR openshift_cluster_id in (?)",
// }

// return db.Where(querys[resource], ocm.UserNameFromContext(ctx), allowedClusterID, allowedClusterUuids), nil
return db.Where(query, ocm.UserNameFromContext(ctx), allowedClusterID, allowedClusterUuids), nil
}
if a.isTenancyEnabled() {
return db.Where("org_id = ?", ocm.OrgIDFromContext(ctx)), nil
Expand Down
39 changes: 26 additions & 13 deletions pkg/auth/rhsso_authz_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,8 @@ var _ = Describe("OwnedBy", func() {
ctx = context.WithValue(ctx, restapi.AuthKey, payload)

var records []common.Cluster
results := handler.OwnedBy(ctx, db).Find(&records)
results, _ := handler.OwnedBy(ctx, db, "")
results.Find(&records)
Expect(results.RowsAffected, 4)
})
It("admin user - non-empty query", func() {
Expand All @@ -166,7 +167,8 @@ var _ = Describe("OwnedBy", func() {
ctx = context.WithValue(ctx, restapi.AuthKey, payload)

var records []common.Cluster
results := handler.OwnedBy(ctx, db).Where("name = ?", "A").Find(&records)
results, _ := handler.OwnedBy(ctx, db, "")
results.Where("name = ?", "A").Find(&records)
Expect(results.RowsAffected, 3)
Expect(AllRecordsHasName(records, "A")).To(BeTrue())
})
Expand All @@ -176,7 +178,8 @@ var _ = Describe("OwnedBy", func() {
ctx = context.WithValue(ctx, restapi.AuthKey, payload)

var records []common.Cluster
results := handler.OwnedByUser(ctx, db, "user1").Find(&records)
results, _ := handler.OwnedByUser(ctx, db, "", "user1")
results.Find(&records)
Expect(results.RowsAffected, 2)
Expect(AllRecordsHasUserName(records, "user1")).To(BeTrue())
})
Expand All @@ -187,7 +190,8 @@ var _ = Describe("OwnedBy", func() {
ctx = context.WithValue(ctx, restapi.AuthKey, payload)

var records []common.Cluster
results := handler.OwnedBy(ctx, db).Find(&records)
results, _ := handler.OwnedBy(ctx, db, "")
results.Find(&records)
Expect(results.RowsAffected, 2)
Expect(AllRecordsHasUserName(records, "user1")).To(BeTrue())
})
Expand All @@ -197,7 +201,8 @@ var _ = Describe("OwnedBy", func() {
ctx = context.WithValue(ctx, restapi.AuthKey, payload)

var records []common.Cluster
results := handler.OwnedBy(ctx, db).Where("name = ?", "A").Find(&records)
results, _ := handler.OwnedBy(ctx, db, "")
results.Where("name = ?", "A").Find(&records)
Expect(results.RowsAffected, 2)
Expect(AllRecordsHasName(records, "A")).To(BeTrue())
Expect(AllRecordsHasUserName(records, "user1")).To(BeTrue())
Expand All @@ -208,7 +213,8 @@ var _ = Describe("OwnedBy", func() {
ctx = context.WithValue(ctx, restapi.AuthKey, payload)

var records []common.Cluster
results := handler.OwnedByUser(ctx, db, "user1").Find(&records)
results, _ := handler.OwnedByUser(ctx, db, "", "user1")
results.Find(&records)
Expect(results.RowsAffected, 2)
Expect(AllRecordsHasUserName(records, "user1")).To(BeTrue())
})
Expand All @@ -218,7 +224,8 @@ var _ = Describe("OwnedBy", func() {
ctx = context.WithValue(ctx, restapi.AuthKey, payload)

var records []common.Cluster
results := handler.OwnedByUser(ctx, db, "user2").Find(&records)
results, _ := handler.OwnedByUser(ctx, db, "", "user2")
results.Find(&records)
Expect(results.RowsAffected, 0)
})
})
Expand All @@ -233,7 +240,8 @@ var _ = Describe("OwnedBy", func() {
ctx = context.WithValue(ctx, restapi.AuthKey, payload)

var records []common.Cluster
results := handler.OwnedBy(ctx, db).Find(&records)
results, _ := handler.OwnedBy(ctx, db, "")
results.Find(&records)
Expect(results.RowsAffected, 4)
})
It("admin user - non-empty query", func() {
Expand All @@ -242,7 +250,8 @@ var _ = Describe("OwnedBy", func() {
ctx = context.WithValue(ctx, restapi.AuthKey, payload)

var records []common.Cluster
results := handler.OwnedBy(ctx, db).Where("name = ?", "A").Find(&records)
results, _ := handler.OwnedBy(ctx, db, "")
results.Where("name = ?", "A").Find(&records)
Expect(results.RowsAffected, 3)
Expect(AllRecordsHasName(records, "A")).To(BeTrue())
})
Expand All @@ -252,7 +261,8 @@ var _ = Describe("OwnedBy", func() {
ctx = context.WithValue(ctx, restapi.AuthKey, payload)

var records []common.Cluster
results := handler.OwnedBy(ctx, db).Find(&records)
results, _ := handler.OwnedBy(ctx, db, "")
results.Find(&records)
Expect(results.RowsAffected, 2)
Expect(AllRecordsHasOrgId(records, "org1")).To(BeTrue())
})
Expand All @@ -262,7 +272,8 @@ var _ = Describe("OwnedBy", func() {
ctx = context.WithValue(ctx, restapi.AuthKey, payload)

var records []common.Cluster
results := handler.OwnedBy(ctx, db).Where("name = ?", "A").Find(&records)
results, _ := handler.OwnedBy(ctx, db, "")
results.Find(&records).Where("name = ?", "A").Find(&records)
Expect(results.RowsAffected, 1)
Expect(AllRecordsHasName(records, "A")).To(BeTrue())
Expect(AllRecordsHasOrgId(records, "org1")).To(BeTrue())
Expand All @@ -274,7 +285,8 @@ var _ = Describe("OwnedBy", func() {
ctx = context.WithValue(ctx, restapi.AuthKey, payload)

var records []common.Cluster
results := handler.OwnedByUser(ctx, db, "user1").Find(&records)
results, _ := handler.OwnedByUser(ctx, db, "", "user1")
results.Find(&records)
Expect(results.RowsAffected, 1)
Expect(AllRecordsHasUserName(records, "user1")).To(BeTrue())
})
Expand All @@ -285,7 +297,8 @@ var _ = Describe("OwnedBy", func() {
ctx = context.WithValue(ctx, restapi.AuthKey, payload)

var records []common.Cluster
results := handler.OwnedByUser(ctx, db, "user2").Find(&records)
results, _ := handler.OwnedByUser(ctx, db, "", "user2")
results.Find(&records)
Expect(results.RowsAffected, 0)
})
})
Expand Down
9 changes: 5 additions & 4 deletions pkg/ocm/mock_authorization.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit b5e60d1

Please sign in to comment.