Update permissions diff computation (#3117)

Previously we were using permission ids to check if two permissions
are equal. While we were aware the permission ids can repeat across
different folder hierarchies we were not aware that this was also the
case within a single folder tree if we delete and break the
inheritance chain.

Assume the following scenario:

1. Create folder `a` and `a/b`
2. Assign user1:read permission to `a`
3. Remove the inherited permission from `a/b`
4. Assign user1:write permission to `a/b`

In this case both the user1:read and user1:write will get the same permissions id.<!-- PR description-->

---

#### Does this PR need a docs update or release note?

- [ ] ✅ Yes, it's included
- [ ] 🕐 Yes, but in a later PR
- [x] ⛔ No

#### Type of change

<!--- Please check the type of change your PR introduces: --->
- [ ] 🌻 Feature
- [x] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [ ] 🧹 Tech Debt/Cleanup

#### Issue(s)

<!-- Can reference multiple issues. Use one of the following "magic words" - "closes, fixes" to auto-close the Github issue. -->
* fixes https://github.com/alcionai/corso/issues/3116

#### Test Plan

<!-- How will this be tested prior to merging.-->
- [ ] 💪 Manual
- [x] ⚡ Unit test
- [ ] 💚 E2E

src/internal/connector/graph_connector_onedrive_test.go

@@ -358,6 +358,7 @@ type suiteInfo interface {
 	// permissions.
 	PrimaryUser() (string, string)
 	SecondaryUser() (string, string)
+	TertiaryUser() (string, string)
 	// BackupResourceOwner returns the resource owner to run the backup/restore
 	// with. This can be different from the values used for permissions and it can
 	// also be a site.
@@ -378,6 +379,8 @@ type suiteInfoImpl struct {
 	userID          string
 	secondaryUser   string
 	secondaryUserID string
+	tertiaryUser    string
+	tertiaryUserID  string
 	acct            account.Account
 	service         path.ServiceType
 	resourceType    resource
@@ -403,6 +406,10 @@ func (si suiteInfoImpl) SecondaryUser() (string, string) {
 	return si.secondaryUser, si.secondaryUserID
 }
 
+func (si suiteInfoImpl) TertiaryUser() (string, string) {
+	return si.tertiaryUser, si.tertiaryUserID
+}
+
 func (si suiteInfoImpl) BackupResourceOwner() string {
 	return si.resourceOwner
 }
@@ -449,6 +456,7 @@ func (suite *GraphConnectorSharePointIntegrationSuite) SetupSuite() {
 		connector:     loadConnector(ctx, suite.T(), Sites),
 		user:          tester.M365UserID(suite.T()),
 		secondaryUser: tester.SecondaryM365UserID(suite.T()),
+		tertiaryUser:  tester.TertiaryM365UserID(suite.T()),
 		acct:          tester.NewM365Account(suite.T()),
 		service:       path.SharePointService,
 		resourceType:  Sites,
@@ -464,6 +472,10 @@ func (suite *GraphConnectorSharePointIntegrationSuite) SetupSuite() {
 	require.NoError(suite.T(), err, "fetching user", si.secondaryUser, clues.ToCore(err))
 	si.secondaryUserID = ptr.Val(secondaryUser.GetId())
 
+	tertiaryUser, err := si.connector.Discovery.Users().GetByID(ctx, si.tertiaryUser)
+	require.NoError(suite.T(), err, "fetching user", si.tertiaryUser, clues.ToCore(err))
+	si.tertiaryUserID = ptr.Val(tertiaryUser.GetId())
+
 	suite.suiteInfo = si
 }
 
@@ -771,13 +783,14 @@ func testPermissionsRestoreAndBackup(suite oneDriveSuite, startVersion int) {
 		"root:",
 		folderBName,
 	}
-	subfolderAPath := []string{
+	// For skipped test
-		"drives",
+	// subfolderAPath := []string{
-		driveID,
+	// 	"drives",
-		"root:",
+	// 	driveID,
-		folderBName,
+	// 	"root:",
-		folderAName,
+	// 	folderBName,
-	}
+	// 	folderAName,
+	// }
 	folderCPath := []string{
 		"drives",
 		driveID,
@@ -839,7 +852,7 @@ func testPermissionsRestoreAndBackup(suite oneDriveSuite, startVersion int) {
 					perms: permData{
 						user:     secondaryUserName,
 						entityID: secondaryUserID,
-						roles:    readPerm,
+						roles:    writePerm,
 					},
 				},
 			},
@@ -854,27 +867,29 @@ func testPermissionsRestoreAndBackup(suite oneDriveSuite, startVersion int) {
 				},
 			},
 		},
-		{
+		// TODO: We can't currently support having custom permissions
-			// Tests a folder that has permissions with an item in the folder with
+		// with the same set of permissions internally
-			// the same permissions.
+		// {
-			pathElements: subfolderAPath,
+		// 	// Tests a folder that has permissions with an item in the folder with
-			files: []itemData{
+		// 	// the same permissions.
-				{
+		// 	pathElements: subfolderAPath,
-					name: fileName,
+		// 	files: []itemData{
-					data: fileDData,
+		// 		{
-					perms: permData{
+		// 			name: fileName,
-						user:     secondaryUserName,
+		// 			data: fileDData,
-						entityID: secondaryUserID,
+		// 			perms: permData{
-						roles:    readPerm,
+		// 				user:     secondaryUserName,
-					},
+		// 				entityID: secondaryUserID,
-				},
+		// 				roles:    readPerm,
-			},
+		// 			},
-			perms: permData{
+		// 		},
-				user:     secondaryUserName,
+		// 	},
-				entityID: secondaryUserID,
+		// 	perms: permData{
-				roles:    readPerm,
+		// 		user:     secondaryUserName,
-			},
+		// 		entityID: secondaryUserID,
-		},
+		// 		roles:    readPerm,
+		// 	},
+		// },
 		{
 			// Tests a folder that has permissions with an item in the folder with
 			// the different permissions.
@@ -1037,6 +1052,7 @@ func testPermissionsInheritanceRestoreAndBackup(suite oneDriveSuite, startVersio
 	defer flush()
 
 	secondaryUserName, secondaryUserID := suite.SecondaryUser()
+	tertiaryUserName, tertiaryUserID := suite.TertiaryUser()
 
 	// Get the default drive ID for the test user.
 	driveID := mustGetDefaultDriveID(
@@ -1049,6 +1065,7 @@ func testPermissionsInheritanceRestoreAndBackup(suite oneDriveSuite, startVersio
 
 	folderAName := "custom"
 	folderBName := "inherited"
+	folderCName := "empty"
 
 	rootPath := []string{
 		"drives",
@@ -1061,20 +1078,27 @@ func testPermissionsInheritanceRestoreAndBackup(suite oneDriveSuite, startVersio
 		"root:",
 		folderAName,
 	}
-	subfolderAPath := []string{
+	subfolderAAPath := []string{
 		"drives",
 		driveID,
 		"root:",
 		folderAName,
 		folderAName,
 	}
-	subfolderBPath := []string{
+	subfolderABPath := []string{
 		"drives",
 		driveID,
 		"root:",
 		folderAName,
 		folderBName,
 	}
+	subfolderACPath := []string{
+		"drives",
+		driveID,
+		"root:",
+		folderAName,
+		folderCName,
+	}
 
 	fileSet := []itemData{
 		{
@@ -1094,27 +1118,39 @@ func testPermissionsInheritanceRestoreAndBackup(suite oneDriveSuite, startVersio
 				sharingMode: onedrive.SharingModeInherited,
 			},
 		},
+		{
+			name: "file-empty",
+			data: fileAData,
+			perms: permData{
+				sharingMode: onedrive.SharingModeCustom,
+			},
+		},
 	}
 
 	// Here is what this test is testing
 	// - custom-permission-folder
 	//   - custom-permission-file
 	//   - inherted-permission-file
+	//   - empty-permission-file
 	//   - custom-permission-folder
 	// 	   - custom-permission-file
 	// 	   - inherted-permission-file
+	//     - empty-permission-file
 	//   - inherted-permission-folder
 	// 	   - custom-permission-file
 	// 	   - inherted-permission-file
+	//     - empty-permission-file
+	//   - empty-permission-folder
+	// 	   - custom-permission-file
+	// 	   - inherted-permission-file
+	//     - empty-permission-file (empty/empty might have interesting behavior)
 
 	cols := []onedriveColInfo{
 		{
 			pathElements: rootPath,
 			files:        []itemData{},
 			folders: []itemData{
-				{
+				{name: folderAName},
-					name: folderAName,
-				},
 			},
 		},
 		{
@@ -1123,30 +1159,38 @@ func testPermissionsInheritanceRestoreAndBackup(suite oneDriveSuite, startVersio
 			folders: []itemData{
 				{name: folderAName},
 				{name: folderBName},
+				{name: folderCName},
 			},
 			perms: permData{
-				user:     secondaryUserName,
+				user:     tertiaryUserName,
-				entityID: secondaryUserID,
+				entityID: tertiaryUserID,
 				roles:    readPerm,
 			},
 		},
 		{
-			pathElements: subfolderAPath,
+			pathElements: subfolderAAPath,
 			files:        fileSet,
 			perms: permData{
-				user:        secondaryUserName,
+				user:        tertiaryUserName,
-				entityID:    secondaryUserID,
+				entityID:    tertiaryUserID,
 				roles:       writePerm,
 				sharingMode: onedrive.SharingModeCustom,
 			},
 		},
 		{
-			pathElements: subfolderBPath,
+			pathElements: subfolderABPath,
 			files:        fileSet,
 			perms: permData{
 				sharingMode: onedrive.SharingModeInherited,
 			},
 		},
+		{
+			pathElements: subfolderACPath,
+			files:        fileSet,
+			perms: permData{
+				sharingMode: onedrive.SharingModeCustom,
+			},
+		},
 	}
 
 	expected := testDataForInfo(suite.T(), suite.BackupService(), cols, version.Backup)

src/internal/connector/onedrive/permission.go

@@ -6,6 +6,7 @@ import (
 	"github.com/alcionai/clues"
 	msdrive "github.com/microsoftgraph/msgraph-sdk-go/drive"
 	"github.com/microsoftgraph/msgraph-sdk-go/models"
+	"golang.org/x/exp/slices"
 
 	"github.com/alcionai/corso/src/internal/common/ptr"
 	"github.com/alcionai/corso/src/internal/connector/graph"
@@ -128,6 +129,30 @@ func createRestoreFoldersWithPermissions(
 	return id, err
 }
 
+// isSamePermission checks equality of two UserPermission objects
+func isSamePermission(p1, p2 UserPermission) bool {
+	// EntityID can be empty for older backups and Email can be empty
+	// for newer ones. It is not possible for both to be empty.  Also,
+	// if EntityID/Email for one is not empty then the other will also
+	// have EntityID/Email as we backup permissions for all the
+	// parents and children when we have a change in permissions.
+	if p1.EntityID != "" && p1.EntityID != p2.EntityID {
+		return false
+	}
+
+	if p1.Email != "" && p1.Email != p2.Email {
+		return false
+	}
+
+	p1r := p1.Roles
+	p2r := p2.Roles
+
+	slices.Sort(p1r)
+	slices.Sort(p2r)
+
+	return slices.Equal(p1r, p2r)
+}
+
 func diffPermissions(before, after []UserPermission) ([]UserPermission, []UserPermission) {
 	var (
 		added   = []UserPermission{}
@@ -138,7 +163,7 @@ func diffPermissions(before, after []UserPermission) ([]UserPermission, []UserPe
 		found := false
 
 		for _, pp := range before {
-			if pp.ID == cp.ID {
+			if isSamePermission(cp, pp) {
 				found = true
 				break
 			}
@@ -153,7 +178,7 @@ func diffPermissions(before, after []UserPermission) ([]UserPermission, []UserPe
 		found := false
 
 		for _, cp := range after {
-			if pp.ID == cp.ID {
+			if isSamePermission(cp, pp) {
 				found = true
 				break
 			}
@@ -219,6 +244,8 @@ func UpdatePermissions(
 	permAdded, permRemoved []UserPermission,
 	permissionIDMappings map[string]string,
 ) error {
+	// The ordering of the operations is important here. We first
+	// remove all the removed permissions and then add the added ones.
 	for _, p := range permRemoved {
 		// deletes require unique http clients
 		// https://github.com/alcionai/corso/issues/2707

src/internal/connector/onedrive/permission_test.go

@@ -151,3 +151,134 @@ func (suite *PermissionsUnitTestSuite) TestComputeParentPermissions() {
 		})
 	}
 }
+
+func (suite *PermissionsUnitTestSuite) TestDiffPermissions() {
+	perm1 := UserPermission{
+		ID:       "id1",
+		Roles:    []string{"read"},
+		EntityID: "user-id1",
+	}
+
+	perm2 := UserPermission{
+		ID:       "id2",
+		Roles:    []string{"write"},
+		EntityID: "user-id2",
+	}
+
+	perm3 := UserPermission{
+		ID:       "id3",
+		Roles:    []string{"write"},
+		EntityID: "user-id3",
+	}
+
+	// The following two permissions have same id and user but
+	// different roles, this is a valid scenario for permissions.
+	sameidperm1 := UserPermission{
+		ID:       "id0",
+		Roles:    []string{"write"},
+		EntityID: "user-id0",
+	}
+	sameidperm2 := UserPermission{
+		ID:       "id0",
+		Roles:    []string{"read"},
+		EntityID: "user-id0",
+	}
+
+	emailperm1 := UserPermission{
+		ID:    "id1",
+		Roles: []string{"read"},
+		Email: "email1@provider.com",
+	}
+
+	emailperm2 := UserPermission{
+		ID:    "id1",
+		Roles: []string{"read"},
+		Email: "email2@provider.com",
+	}
+
+	table := []struct {
+		name    string
+		before  []UserPermission
+		after   []UserPermission
+		added   []UserPermission
+		removed []UserPermission
+	}{
+		{
+			name:    "single permission added",
+			before:  []UserPermission{},
+			after:   []UserPermission{perm1},
+			added:   []UserPermission{perm1},
+			removed: []UserPermission{},
+		},
+		{
+			name:    "single permission removed",
+			before:  []UserPermission{perm1},
+			after:   []UserPermission{},
+			added:   []UserPermission{},
+			removed: []UserPermission{perm1},
+		},
+		{
+			name:    "multiple permission added",
+			before:  []UserPermission{},
+			after:   []UserPermission{perm1, perm2},
+			added:   []UserPermission{perm1, perm2},
+			removed: []UserPermission{},
+		},
+		{
+			name:    "single permission removed",
+			before:  []UserPermission{perm1, perm2},
+			after:   []UserPermission{},
+			added:   []UserPermission{},
+			removed: []UserPermission{perm1, perm2},
+		},
+		{
+			name:    "extra permissions",
+			before:  []UserPermission{perm1, perm2},
+			after:   []UserPermission{perm1, perm2, perm3},
+			added:   []UserPermission{perm3},
+			removed: []UserPermission{},
+		},
+		{
+			name:    "less permissions",
+			before:  []UserPermission{perm1, perm2, perm3},
+			after:   []UserPermission{perm1, perm2},
+			added:   []UserPermission{},
+			removed: []UserPermission{perm3},
+		},
+		{
+			name:    "same id different role",
+			before:  []UserPermission{sameidperm1},
+			after:   []UserPermission{sameidperm2},
+			added:   []UserPermission{sameidperm2},
+			removed: []UserPermission{sameidperm1},
+		},
+		{
+			name:    "email based extra permissions",
+			before:  []UserPermission{emailperm1},
+			after:   []UserPermission{emailperm1, emailperm2},
+			added:   []UserPermission{emailperm2},
+			removed: []UserPermission{},
+		},
+		{
+			name:    "email based less permissions",
+			before:  []UserPermission{emailperm1, emailperm2},
+			after:   []UserPermission{emailperm1},
+			added:   []UserPermission{},
+			removed: []UserPermission{emailperm2},
+		},
+	}
+
+	for _, test := range table {
+		suite.Run(test.name, func() {
+			_, flush := tester.NewContext()
+			defer flush()
+
+			t := suite.T()
+
+			added, removed := diffPermissions(test.before, test.after)
+
+			assert.Equal(t, added, test.added, "added permissions")
+			assert.Equal(t, removed, test.removed, "removed permissions")
+		})
+	}
+}

src/internal/tester/config.go

@@ -27,6 +27,7 @@ const (
 	TestCfgSiteURL          = "m365siteurl"
 	TestCfgUserID           = "m365userid"
 	TestCfgSecondaryUserID  = "secondarym365userid"
+	TestCfgTertiaryUserID   = "tertiarym365userid"
 	TestCfgLoadTestUserID   = "loadtestm365userid"
 	TestCfgLoadTestOrgUsers = "loadtestm365orgusers"
 	TestCfgAccountProvider  = "account_provider"
@@ -38,6 +39,7 @@ const (
 	EnvCorsoM365TestSiteURL         = "CORSO_M365_TEST_SITE_URL"
 	EnvCorsoM365TestUserID          = "CORSO_M365_TEST_USER_ID"
 	EnvCorsoSecondaryM365TestUserID = "CORSO_SECONDARY_M365_TEST_USER_ID"
+	EnvCorsoTertiaryM365TestUserID  = "CORSO_TERTIARY_M365_TEST_USER_ID"
 	EnvCorsoM365LoadTestUserID      = "CORSO_M365_LOAD_TEST_USER_ID"
 	EnvCorsoM365LoadTestOrgUsers    = "CORSO_M365_LOAD_TEST_ORG_USERS"
 	EnvCorsoTestConfigFilePath      = "CORSO_TEST_CONFIG_FILE"
@@ -120,6 +122,12 @@ func readTestConfig() (map[string]string, error) {
 		os.Getenv(EnvCorsoSecondaryM365TestUserID),
 		vpr.GetString(TestCfgSecondaryUserID),
 		"AdeleV@10rqc2.onmicrosoft.com")
+	fallbackTo(
+		testEnv,
+		TestCfgTertiaryUserID,
+		os.Getenv(EnvCorsoTertiaryM365TestUserID),
+		vpr.GetString(TestCfgTertiaryUserID),
+		"PradeepG@10rqc2.onmicrosoft.com")
 	fallbackTo(
 		testEnv,
 		TestCfgLoadTestUserID,

src/internal/tester/resource_owners.go

@@ -72,6 +72,18 @@ func SecondaryM365UserID(t *testing.T) string {
 	return strings.ToLower(cfg[TestCfgSecondaryUserID])
 }
 
+// TertiaryM365UserID returns an userID string representing the m365UserID
+// described by either the env var CORSO_TERTIARY_M365_TEST_USER_ID, the
+// corso_test.toml config file or the default value (in that order of priority).
+// The default is a last-attempt fallback that will only work on alcion's
+// testing org.
+func TertiaryM365UserID(t *testing.T) string {
+	cfg, err := readTestConfig()
+	require.NoError(t, err, "retrieving tertiary m365 user id from test configuration", clues.ToCore(err))
+
+	return strings.ToLower(cfg[TestCfgTertiaryUserID])
+}
+
 // LoadTestM365SiteID returns a siteID string representing the m365SiteID
 // described by either the env var CORSO_M365_LOAD_TEST_SITE_ID, the
 // corso_test.toml config file or the default value (in that order of priority).