From d4922f992611971dce54cdab803f8e900430e3ca Mon Sep 17 00:00:00 2001 From: Rahmat Hidayat Date: Tue, 7 Feb 2023 14:10:57 +0700 Subject: [PATCH] fix(grant): fix bug when grouping imported access (#356) * fix(grant): upsert child resources separately * fix(grant): use grant at headIndex as sample grant * fix: fix grouping imported access by permissions configured in role id * refactor: fill grant.Role value via AccessEntry.ToGrant method * fix: sort roles to prioritize role with most permissions --- core/grant/service.go | 30 ++--- core/grant/service_test.go | 117 +++++++++++++++++++- domain/grant.go | 1 + domain/provider.go | 24 ++-- internal/store/postgres/grant_repository.go | 39 +++++++ internal/store/postgres/model/resource.go | 1 + 6 files changed, 180 insertions(+), 32 deletions(-) diff --git a/core/grant/service.go b/core/grant/service.go index a35d92f0f..90183856f 100644 --- a/core/grant/service.go +++ b/core/grant/service.go @@ -363,12 +363,12 @@ func (s *Service) ImportFromProvider(ctx context.Context, criteria ImportFromPro // group grants for the same account (accountGrants) by provider role rc := resourceConfigs[resource.Type] - grants = reduceGrantsByProviderRole(rc, grants) - for _, g := range grants { + grants = reduceGrantsByProviderRole(*rc, grants) + for i, g := range grants { key := g.PermissionsKey() if existingGrant, ok := activeGrantsMap[rURN][accountSignature][key]; ok { // replace imported grant values with existing grant - *g = *existingGrant + *grants[i] = *existingGrant // remove updated grant from active grants map delete(activeGrantsMap[rURN][accountSignature], key) @@ -419,10 +419,7 @@ func groupAccessEntriesByAccount(accessEntries []domain.AccessEntry) map[string] } // reduceGrantsByProviderRole reduces grants based on configured roles in the provider's resource config and returns reduced grants containing the Role according to the resource config -func reduceGrantsByProviderRole(rc *domain.ResourceConfig, grants []*domain.Grant) (reducedGrants []*domain.Grant) { - rolePermissionsMap := rc.GetRolePermissionsMap() - // TODO: sort rolePermissionsMap based on permissions length descending - +func reduceGrantsByProviderRole(rc domain.ResourceConfig, grants []*domain.Grant) (reducedGrants []*domain.Grant) { grantsGroupedByPermission := map[string]*domain.Grant{} var allGrantPermissions []string for _, g := range grants { @@ -432,18 +429,22 @@ func reduceGrantsByProviderRole(rc *domain.ResourceConfig, grants []*domain.Gran } sort.Strings(allGrantPermissions) - for roleID, permissionsByRole := range rolePermissionsMap { - if containing, headIndex := utils.SubsliceExists(allGrantPermissions, permissionsByRole); containing { - sampleGrant := grantsGroupedByPermission[allGrantPermissions[0]] - sampleGrant.Role = roleID - sampleGrant.Permissions = permissionsByRole + // prioritize roles with more permissions + sort.Slice(rc.Roles, func(i, j int) bool { + return len(rc.Roles[i].Permissions) > len(rc.Roles[j].Permissions) + }) + for _, role := range rc.Roles { + rolePermissions := role.GetOrderedPermissions() + if containing, headIndex := utils.SubsliceExists(allGrantPermissions, rolePermissions); containing { + sampleGrant := grantsGroupedByPermission[rolePermissions[0]] + sampleGrant.Role = role.ID + sampleGrant.Permissions = rolePermissions reducedGrants = append(reducedGrants, sampleGrant) - for _, p := range allGrantPermissions { + for _, p := range rolePermissions { // delete combined grants delete(grantsGroupedByPermission, p) } - allGrantPermissions = append(allGrantPermissions[:headIndex], allGrantPermissions[headIndex+1:]...) } } @@ -451,7 +452,6 @@ func reduceGrantsByProviderRole(rc *domain.ResourceConfig, grants []*domain.Gran if len(grantsGroupedByPermission) > 0 { // add remaining grants with non-registered provider role for _, g := range grantsGroupedByPermission { - g.Role = g.Permissions[0] reducedGrants = append(reducedGrants, g) } } diff --git a/core/grant/service_test.go b/core/grant/service_test.go index bbf30557a..06c0f3edf 100644 --- a/core/grant/service_test.go +++ b/core/grant/service_test.go @@ -630,6 +630,112 @@ func (s *ServiceTestSuite) TestImportFromProvider() { }, }, }, + { + name: "imported access contain multiple permissions configured in one role id", + provider: domain.Provider{ + ID: "test-provider-id", + Type: "test-provider-type", + URN: "test-provider-urn", + Config: &domain.ProviderConfig{ + Type: "test-provider-type", + URN: "test-provider-urn", + Resources: []*domain.ResourceConfig{ + { + Type: "test-resource-type", + Roles: []*domain.Role{ + { + ID: "test-role-id", + Permissions: []interface{}{ + "test-permission", "test-permission-2", + }, + }, + }, + }, + }, + }, + }, + importedGrants: domain.MapResourceAccess{ + "test-resource-urn": []domain.AccessEntry{ + { + AccountID: "test-account-id", // existing + AccountType: "user", + Permission: "test-permission", + }, + { + AccountID: "test-account-id", // existing + AccountType: "user", + Permission: "test-permission-2", + }, + { + AccountID: "test-account-id", // new + AccountType: "user", + Permission: "test-permission-3", + }, + { + AccountID: "test-account-id-2", // new + AccountType: "user", + Permission: "test-permission", + }, + { + AccountID: "test-account-id-2", // new + AccountType: "user", + Permission: "test-permission-2", + }, + }, + }, + existingGrants: []domain.Grant{ + { + ID: "test-grant-id", + Status: domain.GrantStatusActive, + StatusInProvider: domain.GrantStatusActive, + ResourceID: "test-resource-id", + AccountID: "test-account-id", + AccountType: "user", + Role: "test-role-id", + Permissions: []string{"test-permission", "test-permission-2"}, + Resource: dummyResources[0], + Owner: "test-account-id", + }, + }, + expectedNewAndUpdatedGrants: []*domain.Grant{ + { // existing + ID: "test-grant-id", + Status: domain.GrantStatusActive, + StatusInProvider: domain.GrantStatusActive, + ResourceID: "test-resource-id", + AccountID: "test-account-id", + AccountType: "user", + Role: "test-role-id", + Permissions: []string{"test-permission", "test-permission-2"}, + Resource: dummyResources[0], + Owner: "test-account-id", + }, + { // new + Status: domain.GrantStatusActive, + StatusInProvider: domain.GrantStatusActive, + ResourceID: "test-resource-id", + AccountID: "test-account-id", + AccountType: "user", + Role: "test-permission-3", + Permissions: []string{"test-permission-3"}, + Owner: "test-account-id", + Source: "import", + IsPermanent: true, + }, + { // new + Status: domain.GrantStatusActive, + StatusInProvider: domain.GrantStatusActive, + ResourceID: "test-resource-id", + AccountID: "test-account-id-2", + AccountType: "user", + Role: "test-role-id", + Permissions: []string{"test-permission", "test-permission-2"}, + Owner: "test-account-id-2", + Source: "import", + IsPermanent: true, + }, + }, + }, } for _, tc := range testCases { @@ -638,7 +744,7 @@ func (s *ServiceTestSuite) TestImportFromProvider() { s.mockProviderService.EXPECT(). GetByID(mock.AnythingOfType("*context.emptyCtx"), "test-provider-id"). - Return(dummyProvider, nil).Once() + Return(&tc.provider, nil).Once() expectedListResourcesFilter := domain.ListResourcesFilter{ ProviderType: "test-provider-type", ProviderURN: "test-provider-urn", @@ -647,7 +753,7 @@ func (s *ServiceTestSuite) TestImportFromProvider() { Find(mock.AnythingOfType("*context.emptyCtx"), expectedListResourcesFilter). Return(dummyResources, nil).Once() s.mockProviderService.EXPECT(). - ListAccess(mock.AnythingOfType("*context.emptyCtx"), *dummyProvider, dummyResources). + ListAccess(mock.AnythingOfType("*context.emptyCtx"), tc.provider, dummyResources). Return(tc.importedGrants, nil).Once() expectedListGrantsFilter := domain.ListGrantsFilter{ ProviderTypes: []string{"test-provider-type"}, @@ -671,7 +777,12 @@ func (s *ServiceTestSuite) TestImportFromProvider() { }) s.NoError(err) - s.Equal(tc.expectedNewAndUpdatedGrants, newGrants) + s.Empty(cmp.Diff(tc.expectedNewAndUpdatedGrants, newGrants, cmpopts.SortSlices(func(a, b *domain.Grant) bool { + if a.AccountID != b.AccountID { + return a.AccountID < b.AccountID + } + return a.Role < b.Role + }))) }) } }) diff --git a/domain/grant.go b/domain/grant.go index fa5835f3b..0f61ccec7 100644 --- a/domain/grant.go +++ b/domain/grant.go @@ -125,6 +125,7 @@ func (ae AccessEntry) ToGrant(resource Resource) Grant { AccountID: ae.AccountID, AccountType: ae.AccountType, Owner: SystemActorName, + Role: ae.Permission, Permissions: []string{ae.Permission}, Source: GrantSourceImport, IsPermanent: true, diff --git a/domain/provider.go b/domain/provider.go index f2b3424a5..1890800e4 100644 --- a/domain/provider.go +++ b/domain/provider.go @@ -35,6 +35,16 @@ type Role struct { Permissions []interface{} `json:"permissions" yaml:"permissions" validate:"required"` } +// GetOrderedPermissions returns the permissions as a string slice +func (r Role) GetOrderedPermissions() []string { + permissions := []string{} + for _, p := range r.Permissions { + permissions = append(permissions, fmt.Sprintf("%s", p)) + } + sort.Strings(permissions) + return permissions +} + // PolicyConfig is the configuration that defines which policy is being used in the provider type PolicyConfig struct { ID string `json:"id" yaml:"id" validate:"required"` @@ -49,20 +59,6 @@ type ResourceConfig struct { Roles []*Role `json:"roles" yaml:"roles" validate:"required"` } -// GetRolePermissionsMap returns map[Role.ID][]PermissionStr -func (rc ResourceConfig) GetRolePermissionsMap() map[string][]string { - rolePermissions := map[string][]string{} - for _, r := range rc.Roles { - var permissionsStr []string - for _, v := range r.Permissions { - permissionsStr = append(permissionsStr, fmt.Sprintf("%s", v)) - } - sort.Strings(permissionsStr) - rolePermissions[r.ID] = permissionsStr - } - return rolePermissions -} - // AppealConfig is the policy configuration of the appeal type AppealConfig struct { AllowPermanentAccess bool `json:"allow_permanent_access" yaml:"allow_permanent_access"` diff --git a/internal/store/postgres/grant_repository.go b/internal/store/postgres/grant_repository.go index 4c88fc1a3..52f4fab6d 100644 --- a/internal/store/postgres/grant_repository.go +++ b/internal/store/postgres/grant_repository.go @@ -180,6 +180,12 @@ func (r *GrantRepository) BulkUpsert(ctx context.Context, grants []*domain.Grant } return r.db.Transaction(func(tx *gorm.DB) error { + // upsert resources separately to avoid resource upsertion duplicate issue + if err := upsertResources(tx, models); err != nil { + return fmt.Errorf("upserting resources: %w", err) + } + tx = tx.Omit("Resource") + if err := tx. Clauses(clause.OnConflict{UpdateAll: true}). Create(models). @@ -198,3 +204,36 @@ func (r *GrantRepository) BulkUpsert(ctx context.Context, grants []*domain.Grant return nil }) } + +func upsertResources(tx *gorm.DB, models []*model.Grant) error { + uniqueResourcesMap := map[string]*model.Resource{} + + for _, m := range models { + if r := m.Resource; r != nil { + key := getResourceUniqueURN(*r) + if _, exists := uniqueResourcesMap[key]; !exists { + uniqueResourcesMap[key] = r + } else { + m.Resource = uniqueResourcesMap[key] + } + } + } + + var resources []*model.Resource + for _, r := range uniqueResourcesMap { + resources = append(resources, r) + } + if len(resources) > 0 { + if err := tx.Create(resources).Error; err != nil { + return fmt.Errorf("failed to upsert resources: %w", err) + } + } + for _, g := range models { + // set resource id after upsertion + if g.Resource != nil { + g.ResourceID = g.Resource.ID.String() + } + } + + return nil +} diff --git a/internal/store/postgres/model/resource.go b/internal/store/postgres/model/resource.go index 8e43ade5e..a7be5ed7e 100644 --- a/internal/store/postgres/model/resource.go +++ b/internal/store/postgres/model/resource.go @@ -48,6 +48,7 @@ func (r *Resource) BeforeCreate(tx *gorm.DB) error { }, DoUpdates: clause.AssignmentColumns([]string{"name", "details", "updated_at", "is_deleted", "parent_id"}), }) + return nil }