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

@@ -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,26 +429,29 @@ 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:]...)
 		}
 	}
 
 	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)
 		}
 	}

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
+				})))
 			})
 		}
 	})

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,

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"`

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
+}

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
 }