From 6b00548a0bc2c7be850c94b7e611ea4d2d3d9dbf Mon Sep 17 00:00:00 2001 From: Abin Simon Date: Wed, 29 Mar 2023 10:15:25 +0530 Subject: [PATCH] Integration test for permissions changes (#2965) This adds basic integration tests for backing up permissions in OneDrive. *https://github.com/alcionai/corso/issues/2790 could be a good follow up to this.* --- #### 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 - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [x] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup #### Issue(s) * fixes https://github.com/alcionai/corso/issues/2772 #### Test Plan - [ ] :muscle: Manual - [ ] :zap: Unit test - [x] :green_heart: E2E --- src/internal/connector/onedrive/permission.go | 19 +-- src/internal/connector/onedrive/restore.go | 42 ++--- src/internal/connector/sharepoint/restore.go | 3 +- .../operations/backup_integration_test.go | 144 +++++++++++++++--- 4 files changed, 140 insertions(+), 68 deletions(-) diff --git a/src/internal/connector/onedrive/permission.go b/src/internal/connector/onedrive/permission.go index b0c198302..c1d734f40 100644 --- a/src/internal/connector/onedrive/permission.go +++ b/src/internal/connector/onedrive/permission.go @@ -8,7 +8,6 @@ import ( "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" "github.com/alcionai/corso/src/internal/data" "github.com/alcionai/corso/src/internal/version" @@ -93,7 +92,6 @@ func createRestoreFoldersWithPermissions( drivePath *path.DrivePath, restoreFolders []string, folderMetadata Metadata, - permissionIDMappings map[string]string, ) (string, error) { id, err := CreateRestoreFolders(ctx, service, drivePath.DriveID, restoreFolders) if err != nil { @@ -105,13 +103,12 @@ func createRestoreFoldersWithPermissions( return id, nil } - err = restorePermissions( + err = RestorePermissions( ctx, service, drivePath.DriveID, id, - folderMetadata, - permissionIDMappings) + folderMetadata) return id, err } @@ -126,7 +123,6 @@ func isSame(first, second []string) bool { func diffPermissions( before, after []UserPermission, - permissionIDMappings map[string]string, ) ([]UserPermission, []UserPermission) { var ( added = []UserPermission{} @@ -168,16 +164,15 @@ func diffPermissions( return added, removed } -// restorePermissions takes in the permissions that were added and the +// RestorePermissions takes in the permissions that were added and the // removed(ones present in parent but not in child) and adds/removes // the necessary permissions on onedrive objects. -func restorePermissions( +func RestorePermissions( ctx context.Context, service graph.Servicer, driveID string, itemID string, meta Metadata, - permissionIDMappings map[string]string, ) error { if meta.SharingMode == SharingModeInherited { return nil @@ -191,7 +186,7 @@ func restorePermissions( return graph.Wrap(ctx, err, "fetching current permissions") } - permAdded, permRemoved := diffPermissions(currentPermissions, meta.Permissions, permissionIDMappings) + permAdded, permRemoved := diffPermissions(currentPermissions, meta.Permissions) for _, p := range permRemoved { err := service.Client(). @@ -244,12 +239,10 @@ func restorePermissions( pbody.SetRecipients([]models.DriveRecipientable{rec}) - np, err := service.Client().DrivesById(driveID).ItemsById(itemID).Invite().Post(ctx, pbody, nil) + _, err := service.Client().DrivesById(driveID).ItemsById(itemID).Invite().Post(ctx, pbody, nil) if err != nil { return graph.Wrap(ctx, err, "setting permissions") } - - permissionIDMappings[p.ID] = ptr.Val(np.GetValue()[0].GetId()) } return nil diff --git a/src/internal/connector/onedrive/restore.go b/src/internal/connector/onedrive/restore.go index 1b9da660d..24a9e6f8e 100644 --- a/src/internal/connector/onedrive/restore.go +++ b/src/internal/connector/onedrive/restore.go @@ -45,10 +45,6 @@ func RestoreCollections( restoreMetrics support.CollectionMetrics metrics support.CollectionMetrics folderMetas map[string]Metadata - - // permissionIDMappings is used to map between old and new id - // of permissions as we restore them - permissionIDMappings = map[string]string{} ) ctx = clues.Add( @@ -82,7 +78,7 @@ func RestoreCollections( "path", dc.FullPath()) // TODO: pii ) - metrics, folderMetas, permissionIDMappings, err = RestoreCollection( + metrics, folderMetas, err = RestoreCollection( ictx, backupVersion, service, @@ -91,7 +87,6 @@ func RestoreCollections( OneDriveSource, dest.ContainerName, deets, - permissionIDMappings, opts.RestorePermissions, errs) if err != nil { @@ -122,7 +117,8 @@ func RestoreCollections( // RestoreCollection handles restoration of an individual collection. // returns: // - the collection's item and byte count metrics -// - the context cancellation state (true if the context is canceled) +// - the updated metadata map that include metadata for folders in this collection +// - error, if any besides recoverable func RestoreCollection( ctx context.Context, backupVersion int, @@ -132,10 +128,9 @@ func RestoreCollection( source driveSource, restoreContainerName string, deets *details.Builder, - permissionIDMappings map[string]string, restorePerms bool, errs *fault.Bus, -) (support.CollectionMetrics, map[string]Metadata, map[string]string, error) { +) (support.CollectionMetrics, map[string]Metadata, error) { var ( metrics = support.CollectionMetrics{} copyBuffer = make([]byte, copyBufferSize) @@ -149,7 +144,7 @@ func RestoreCollection( drivePath, err := path.ToOneDrivePath(directory) if err != nil { - return metrics, folderMetas, permissionIDMappings, clues.Wrap(err, "creating drive path").WithClues(ctx) + return metrics, folderMetas, clues.Wrap(err, "creating drive path").WithClues(ctx) } // Assemble folder hierarchy we're going to restore into (we recreate the folder hierarchy @@ -176,7 +171,7 @@ func RestoreCollection( backupVersion, restorePerms) if err != nil { - return metrics, folderMetas, permissionIDMappings, clues.Wrap(err, "getting permissions").WithClues(ctx) + return metrics, folderMetas, clues.Wrap(err, "getting permissions").WithClues(ctx) } // Create restore folders and get the folder ID of the folder the data stream will be restored in @@ -185,10 +180,9 @@ func RestoreCollection( service, drivePath, restoreFolderElements, - colMeta, - permissionIDMappings) + colMeta) if err != nil { - return metrics, folderMetas, permissionIDMappings, clues.Wrap(err, "creating folders for restore") + return metrics, folderMetas, clues.Wrap(err, "creating folders for restore") } items := dc.Items(ctx, errs) @@ -200,11 +194,11 @@ func RestoreCollection( select { case <-ctx.Done(): - return metrics, folderMetas, permissionIDMappings, err + return metrics, folderMetas, err case itemData, ok := <-items: if !ok { - return metrics, folderMetas, permissionIDMappings, nil + return metrics, folderMetas, nil } itemPath, err := dc.FullPath().Append(itemData.UUID(), true) @@ -223,7 +217,6 @@ func RestoreCollection( restoreFolderID, copyBuffer, folderMetas, - permissionIDMappings, restorePerms, itemData, itemPath) @@ -260,7 +253,7 @@ func RestoreCollection( } } - return metrics, folderMetas, permissionIDMappings, el.Failure() + return metrics, folderMetas, el.Failure() } // restores an item, according to correct backup version behavior. @@ -275,7 +268,6 @@ func restoreItem( restoreFolderID string, copyBuffer []byte, folderMetas map[string]Metadata, - permissionIDMappings map[string]string, restorePerms bool, itemData data.Stream, itemPath path.Path, @@ -340,7 +332,6 @@ func restoreItem( dc, restoreFolderID, copyBuffer, - permissionIDMappings, restorePerms, itemData) if err != nil { @@ -360,7 +351,6 @@ func restoreItem( dc, restoreFolderID, copyBuffer, - permissionIDMappings, restorePerms, itemData) if err != nil { @@ -407,7 +397,6 @@ func restoreV1File( fetcher fileFetcher, restoreFolderID string, copyBuffer []byte, - permissionIDMappings map[string]string, restorePerms bool, itemData data.Stream, ) (details.ItemInfo, error) { @@ -440,13 +429,12 @@ func restoreV1File( return details.ItemInfo{}, clues.Wrap(err, "restoring file") } - err = restorePermissions( + err = RestorePermissions( ctx, service, drivePath.DriveID, itemID, - meta, - permissionIDMappings) + meta) if err != nil { return details.ItemInfo{}, clues.Wrap(err, "restoring item permissions") } @@ -462,7 +450,6 @@ func restoreV6File( fetcher fileFetcher, restoreFolderID string, copyBuffer []byte, - permissionIDMappings map[string]string, restorePerms bool, itemData data.Stream, ) (details.ItemInfo, error) { @@ -506,13 +493,12 @@ func restoreV6File( return itemInfo, nil } - err = restorePermissions( + err = RestorePermissions( ctx, service, drivePath.DriveID, itemID, meta, - permissionIDMappings, ) if err != nil { return details.ItemInfo{}, clues.Wrap(err, "restoring item permissions") diff --git a/src/internal/connector/sharepoint/restore.go b/src/internal/connector/sharepoint/restore.go index 23abcb217..8bb1a4a32 100644 --- a/src/internal/connector/sharepoint/restore.go +++ b/src/internal/connector/sharepoint/restore.go @@ -67,7 +67,7 @@ func RestoreCollections( switch dc.FullPath().Category() { case path.LibrariesCategory: - metrics, _, _, err = onedrive.RestoreCollection( + metrics, _, err = onedrive.RestoreCollection( ictx, backupVersion, service, @@ -76,7 +76,6 @@ func RestoreCollections( onedrive.SharePointSource, dest.ContainerName, deets, - map[string]string{}, false, errs) case path.ListsCategory: diff --git a/src/internal/operations/backup_integration_test.go b/src/internal/operations/backup_integration_test.go index d26f80233..4a92ccf3c 100644 --- a/src/internal/operations/backup_integration_test.go +++ b/src/internal/operations/backup_integration_test.go @@ -455,6 +455,29 @@ func toDataLayerPath( return p } +func mustGetDefaultDriveID( + t *testing.T, + ctx context.Context, //revive:disable-line:context-as-argument + service graph.Servicer, + userID string, +) string { + d, err := service.Client().UsersById(userID).Drive().Get(ctx, nil) + if err != nil { + err = graph.Wrap( + ctx, + err, + "retrieving default user drive"). + With("user", userID) + } + + require.Nil(t, clues.ToCore(err)) + + id := ptr.Val(d.GetId()) + require.NotEmpty(t, id, "drive ID not set") + + return id +} + // --------------------------------------------------------------------------- // integration tests // --------------------------------------------------------------------------- @@ -1132,31 +1155,6 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() { fault.New(true)) require.NoError(t, err, clues.ToCore(err)) - // TODO: whomever can figure out a way to declare this outside of this func - // and not have the linter complain about unused is welcome to do so. - mustGetDefaultDriveID := func( - t *testing.T, - ctx context.Context, //revive:disable-line:context-as-argument - service graph.Servicer, - userID string, - ) string { - d, err := service.Client().UsersById(userID).Drive().Get(ctx, nil) - if err != nil { - err = graph.Wrap( - ctx, - err, - "retrieving default user drive"). - With("user", userID) - } - - require.NoError(t, err) - - id := ptr.Val(d.GetId()) - require.NotEmpty(t, id, "drive ID not set") - - return id - } - driveID := mustGetDefaultDriveID(t, ctx, gc.Service, suite.user) fileDBF := func(id, timeStamp, subject, body string) []byte { @@ -1254,6 +1252,102 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() { itemsRead: 1, // .data file for newitem itemsWritten: 3, // .data and .meta for newitem, .dirmeta for parent }, + { + name: "add permission to new file", + updateUserData: func(t *testing.T) { + driveItem := models.NewDriveItem() + driveItem.SetName(&newFileName) + driveItem.SetFile(models.NewFile()) + err = onedrive.RestorePermissions( + ctx, + gc.Service, + driveID, + *newFile.GetId(), + onedrive.Metadata{ + SharingMode: onedrive.SharingModeCustom, + Permissions: []onedrive.UserPermission{ + { + Roles: []string{"write"}, + EntityID: suite.user, + }, + }, + }, + ) + require.NoErrorf(t, err, "add permission to file %v", clues.ToCore(err)) + }, + itemsRead: 1, // .data file for newitem + itemsWritten: 2, // .meta for newitem, .dirmeta for parent (.data is not written as it is not updated) + }, + { + name: "remove permission from new file", + updateUserData: func(t *testing.T) { + driveItem := models.NewDriveItem() + driveItem.SetName(&newFileName) + driveItem.SetFile(models.NewFile()) + err = onedrive.RestorePermissions( + ctx, + gc.Service, + driveID, + *newFile.GetId(), + onedrive.Metadata{ + SharingMode: onedrive.SharingModeCustom, + Permissions: []onedrive.UserPermission{}, + }, + ) + require.NoError(t, err, "add permission to file", clues.ToCore(err)) + }, + itemsRead: 1, // .data file for newitem + itemsWritten: 2, // .meta for newitem, .dirmeta for parent (.data is not written as it is not updated) + }, + { + name: "add permission to container", + updateUserData: func(t *testing.T) { + targetContainer := containerIDs[container1] + driveItem := models.NewDriveItem() + driveItem.SetName(&newFileName) + driveItem.SetFile(models.NewFile()) + err = onedrive.RestorePermissions( + ctx, + gc.Service, + driveID, + targetContainer, + onedrive.Metadata{ + SharingMode: onedrive.SharingModeCustom, + Permissions: []onedrive.UserPermission{ + { + Roles: []string{"write"}, + EntityID: suite.user, + }, + }, + }, + ) + require.NoError(t, err, "add permission to file", clues.ToCore(err)) + }, + itemsRead: 0, + itemsWritten: 1, // .dirmeta for collection + }, + { + name: "remove permission from container", + updateUserData: func(t *testing.T) { + targetContainer := containerIDs[container1] + driveItem := models.NewDriveItem() + driveItem.SetName(&newFileName) + driveItem.SetFile(models.NewFile()) + err = onedrive.RestorePermissions( + ctx, + gc.Service, + driveID, + targetContainer, + onedrive.Metadata{ + SharingMode: onedrive.SharingModeCustom, + Permissions: []onedrive.UserPermission{}, + }, + ) + require.NoError(t, err, "add permission to file", clues.ToCore(err)) + }, + itemsRead: 0, + itemsWritten: 1, // .dirmeta for collection + }, { name: "update contents of a file", updateUserData: func(t *testing.T) {