diff --git a/src/internal/m365/collection/drive/permission.go b/src/internal/m365/collection/drive/permission.go index 2eb19c054..61bdb1955 100644 --- a/src/internal/m365/collection/drive/permission.go +++ b/src/internal/m365/collection/drive/permission.go @@ -19,6 +19,9 @@ import ( "github.com/alcionai/corso/src/pkg/services/m365/api/graph" ) +// empty string is used to indicate that a permission cannot be restored +const nonRestorablePermission = "" + func getParentMetadata( parentPath path.Path, parentDirToMeta syncd.MapTo[metadata.Metadata], @@ -214,7 +217,14 @@ func UpdatePermissions( pid, ok := oldPermIDToNewID.Load(p.ID) if !ok { - return clues.NewWC(ictx, "no new permission id") + el.AddRecoverable(ictx, clues.NewWC(ictx, "no permission matches id")) + continue + } + + if pid == nonRestorablePermission { + // permission was not restored on parent and thus cannot + // be deleted + continue } err := udip.DeleteItemPermission( @@ -277,7 +287,9 @@ func UpdatePermissions( newPerm, err := udip.PostItemPermissionUpdate(ictx, driveID, itemID, pbody) if graph.IsErrUsersCannotBeResolved(err) { - logger.CtxErr(ictx, err).Info("Unable to restore link share") + oldPermIDToNewID.Store(p.ID, nonRestorablePermission) + logger.CtxErr(ictx, err).Info("unable to restore permission") + continue } @@ -385,7 +397,9 @@ func UpdateLinkShares( newLS, err := upils.PostItemLinkShareUpdate(ictx, driveID, itemID, lsbody) if graph.IsErrUsersCannotBeResolved(err) { - logger.CtxErr(ictx, err).Info("Unable to restore link share") + oldLinkShareIDToNewID.Store(ls.ID, "") // empty to signify that we could not restore + logger.CtxErr(ictx, err).Info("unable to restore link share") + continue } @@ -448,39 +462,44 @@ func RestorePermissions( current metadata.Metadata, caches *restoreCaches, errs *fault.Bus, -) error { +) { if current.SharingMode == metadata.SharingModeInherited { - return nil + return } + var didReset bool + ctx = clues.Add(ctx, "permission_item_id", itemID) previousLinkShares, err := computePreviousLinkShares(ctx, itemPath, caches.ParentDirToMeta) if err != nil { - return clues.Wrap(err, "previous link shares") + errs.AddRecoverable(ctx, clues.WrapWC(ctx, err, "previous link shares")) } - lsAdded, lsRemoved := metadata.DiffLinkShares(previousLinkShares, current.LinkShares) + if previousLinkShares != nil { + lsAdded, lsRemoved := metadata.DiffLinkShares(previousLinkShares, current.LinkShares) - // Link shares have to be updated before permissions as we have to - // use the information about if we had to reset the inheritance to - // decide if we have to restore all the permissions. - didReset, err := UpdateLinkShares( - ctx, - rh, - driveID, - itemID, - lsAdded, - lsRemoved, - caches.OldLinkShareIDToNewID, - errs) - if err != nil { - return clues.Wrap(err, "updating link shares") + // Link shares have to be updated before permissions as we have to + // use the information about if we had to reset the inheritance to + // decide if we have to restore all the permissions. + didReset, err = UpdateLinkShares( + ctx, + rh, + driveID, + itemID, + lsAdded, + lsRemoved, + caches.OldLinkShareIDToNewID, + errs) + if err != nil { + errs.AddRecoverable(ctx, clues.WrapWC(ctx, err, "updating link shares")) + } } previous, err := computePreviousMetadata(ctx, itemPath, caches.ParentDirToMeta) if err != nil { - return clues.Wrap(err, "previous metadata") + errs.AddRecoverable(ctx, clues.WrapWC(ctx, err, "previous metadata")) + return } permAdded, permRemoved := metadata.DiffPermissions(previous.Permissions, current.Permissions) @@ -506,8 +525,10 @@ func RestorePermissions( errs) if graph.IsErrSharingDisabled(err) { logger.CtxErr(ctx, err).Info("sharing disabled, not restoring permissions") - return nil + return } - return clues.Wrap(err, "updating permissions").OrNil() + if err != nil { + errs.AddRecoverable(ctx, clues.WrapWC(ctx, err, "updating permissions")) + } } diff --git a/src/internal/m365/collection/drive/permission_test.go b/src/internal/m365/collection/drive/permission_test.go index b834e837c..d787ae63a 100644 --- a/src/internal/m365/collection/drive/permission_test.go +++ b/src/internal/m365/collection/drive/permission_test.go @@ -1,18 +1,24 @@ package drive import ( + "context" "strings" "testing" + "github.com/microsoftgraph/msgraph-sdk-go/drives" + "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" + "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/common/syncd" "github.com/alcionai/corso/src/internal/m365/collection/drive/metadata" odConsts "github.com/alcionai/corso/src/internal/m365/service/onedrive/consts" "github.com/alcionai/corso/src/internal/tester" + "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/path" + graphTD "github.com/alcionai/corso/src/pkg/services/m365/api/graph/testdata" ) type PermissionsUnitTestSuite struct { @@ -169,3 +175,154 @@ func runComputeParentPermissionsTest( }) } } + +type mockUdip struct { + // cannot use []bool as this is not passed by ref + success chan bool +} + +func (mockUdip) DeleteItemPermission( + ctx context.Context, + driveID, itemID, permissionID string, +) error { + return nil +} + +func (m mockUdip) PostItemPermissionUpdate( + ctx context.Context, + driveID, itemID string, + body *drives.ItemItemsItemInvitePostRequestBody, +) (drives.ItemItemsItemInviteResponseable, error) { + if ptr.Val(body.GetRecipients()[0].GetObjectId()) == "failure" { + m.success <- false + + err := graphTD.ODataErrWithMsg("InvalidRequest", string("One or more users could not be resolved")) + + return nil, err + } + + m.success <- true + + resp := drives.NewItemItemsItemInviteResponse() + perm := models.NewPermission() + perm.SetId(ptr.To(itemID)) + resp.SetValue([]models.Permissionable{perm}) + + return resp, nil +} + +func (suite *PermissionsUnitTestSuite) TestPermissionRestoreNonExistentUser() { + t := suite.T() + + ctx, flush := tester.NewContext(t) + defer flush() + + successChan := make(chan bool, 3) + m := mockUdip{ + success: successChan, + } + + err := UpdatePermissions( + ctx, + &m, + "drive-id", + "item-id", + []metadata.Permission{ + {Roles: []string{"write"}, EntityID: "user-id1"}, + {Roles: []string{"write"}, EntityID: "failure"}, + {Roles: []string{"write"}, EntityID: "user-id2"}, + }, + []metadata.Permission{}, + syncd.NewMapTo[string](), + fault.New(true)) + + assert.NoError(t, err, "update permissions") + close(successChan) + + var successValues []bool + for success := range successChan { + successValues = append(successValues, success) + } + + expectedSuccessValues := []bool{true, false, true} + assert.Equal(t, expectedSuccessValues, successValues) +} + +type mockUpils struct { + success chan bool +} + +func (mockUpils) DeleteItemPermission( + ctx context.Context, + driveID, itemID, permissionID string, +) error { + return nil +} + +func (m mockUpils) PostItemLinkShareUpdate( + ctx context.Context, + driveID, itemID string, + body *drives.ItemItemsItemCreateLinkPostRequestBody, +) (models.Permissionable, error) { + shouldFail := false + + recip := body.GetAdditionalData()["recipients"].([]map[string]string) + for _, r := range recip { + if r["objectId"] == "failure" { + shouldFail = true + } + } + + if shouldFail { + m.success <- false + + err := graphTD.ODataErrWithMsg("InvalidRequest", string("One or more users could not be resolved")) + + return nil, err + } + + m.success <- true + + perm := models.NewPermission() + perm.SetId(ptr.To(itemID)) + + return perm, nil +} + +func (suite *PermissionsUnitTestSuite) TestLinkShareRestoreNonExistentUser() { + t := suite.T() + + ctx, flush := tester.NewContext(t) + defer flush() + + successChan := make(chan bool, 10) // 7 required + m := mockUpils{ + success: successChan, + } + + _, err := UpdateLinkShares( + ctx, + &m, + "drive-id", + "item-id", + []metadata.LinkShare{ + {Roles: []string{"write"}, Entities: []metadata.Entity{{ID: "user-id"}}}, + {Roles: []string{"write"}, Entities: []metadata.Entity{{ID: "failure"}}}, + {Roles: []string{"write"}, Entities: []metadata.Entity{{ID: "user-id"}, {ID: "failure"}}}, + {Roles: []string{"write"}, Entities: []metadata.Entity{{ID: "user-id"}, {ID: "failure"}, {ID: "user-id"}}}, + }, + []metadata.LinkShare{}, + syncd.NewMapTo[string](), + fault.New(true)) + + assert.NoError(t, err, "update permissions") + close(successChan) + + var successValues []bool + for success := range successChan { + successValues = append(successValues, success) + } + + expectedSuccessValues := []bool{true, false, false, false} + assert.Equal(t, expectedSuccessValues, successValues) +} diff --git a/src/internal/m365/collection/drive/restore.go b/src/internal/m365/collection/drive/restore.go index 79cf4aa97..f5a18a091 100644 --- a/src/internal/m365/collection/drive/restore.go +++ b/src/internal/m365/collection/drive/restore.go @@ -452,7 +452,7 @@ func restoreV1File( return details.ItemInfo{}, clues.Wrap(err, "restoring file") } - err = RestorePermissions( + RestorePermissions( ctx, rh, drivePath.DriveID, @@ -461,9 +461,6 @@ func restoreV1File( meta, caches, errs) - if err != nil { - return details.ItemInfo{}, clues.Wrap(err, "restoring item permissions") - } return itemInfo, nil } @@ -530,7 +527,7 @@ func restoreV6File( return itemInfo, nil } - err = RestorePermissions( + RestorePermissions( ctx, rh, drivePath.DriveID, @@ -539,9 +536,6 @@ func restoreV6File( meta, caches, errs) - if err != nil { - return details.ItemInfo{}, clues.Wrap(err, "restoring item permissions") - } return itemInfo, nil } @@ -581,7 +575,7 @@ func CreateRestoreFolders( return id, nil } - err = RestorePermissions( + RestorePermissions( ctx, rh, drivePath.DriveID, @@ -591,7 +585,7 @@ func CreateRestoreFolders( caches, errs) - return id, err + return id, nil } type folderRestorer interface {