From a4ff93bd47a92ba4759b98a528fad29cd8fbef0f Mon Sep 17 00:00:00 2001 From: Abin Simon Date: Wed, 19 Apr 2023 22:36:14 +0530 Subject: [PATCH] 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. --- #### Does this PR need a docs update or release note? - [ ] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [x] :no_entry: No #### Type of change - [ ] :sunflower: Feature - [x] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup #### Issue(s) * fixes https://github.com/alcionai/corso/issues/3116 #### Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- .../graph_connector_onedrive_test.go | 124 +++++++++++------ src/internal/connector/onedrive/permission.go | 31 ++++- .../connector/onedrive/permission_test.go | 131 ++++++++++++++++++ src/internal/tester/config.go | 8 ++ src/internal/tester/resource_owners.go | 12 ++ 5 files changed, 264 insertions(+), 42 deletions(-) diff --git a/src/internal/connector/graph_connector_onedrive_test.go b/src/internal/connector/graph_connector_onedrive_test.go index d6447ae3f..495ddedc4 100644 --- a/src/internal/connector/graph_connector_onedrive_test.go +++ b/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{ - "drives", - driveID, - "root:", - folderBName, - folderAName, - } + // For skipped test + // subfolderAPath := []string{ + // "drives", + // driveID, + // "root:", + // 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) { }, }, }, - { - // Tests a folder that has permissions with an item in the folder with - // the same permissions. - pathElements: subfolderAPath, - files: []itemData{ - { - name: fileName, - data: fileDData, - perms: permData{ - user: secondaryUserName, - entityID: secondaryUserID, - roles: readPerm, - }, - }, - }, - perms: permData{ - user: secondaryUserName, - entityID: secondaryUserID, - roles: readPerm, - }, - }, + // TODO: We can't currently support having custom permissions + // with the same set of permissions internally + // { + // // Tests a folder that has permissions with an item in the folder with + // // the same permissions. + // pathElements: subfolderAPath, + // files: []itemData{ + // { + // name: fileName, + // data: fileDData, + // perms: permData{ + // user: secondaryUserName, + // entityID: secondaryUserID, + // roles: readPerm, + // }, + // }, + // }, + // perms: permData{ + // 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, - entityID: secondaryUserID, + user: tertiaryUserName, + entityID: tertiaryUserID, roles: readPerm, }, }, { - pathElements: subfolderAPath, + pathElements: subfolderAAPath, files: fileSet, perms: permData{ - user: secondaryUserName, - entityID: secondaryUserID, + user: tertiaryUserName, + 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) diff --git a/src/internal/connector/onedrive/permission.go b/src/internal/connector/onedrive/permission.go index 44c4ae5f7..d59461a00 100644 --- a/src/internal/connector/onedrive/permission.go +++ b/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 diff --git a/src/internal/connector/onedrive/permission_test.go b/src/internal/connector/onedrive/permission_test.go index a729ca24a..0483462b1 100644 --- a/src/internal/connector/onedrive/permission_test.go +++ b/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") + }) + } +} diff --git a/src/internal/tester/config.go b/src/internal/tester/config.go index ec08c239a..8a002fd2c 100644 --- a/src/internal/tester/config.go +++ b/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, diff --git a/src/internal/tester/resource_owners.go b/src/internal/tester/resource_owners.go index a4afb97ff..c39b39151 100644 --- a/src/internal/tester/resource_owners.go +++ b/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).