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).