Update handling of permissions restore failures (#4954)
- All permissions failures are now recoverable to the granularity of a single permission or link share - Failure to delete child permissions because the permission was not restored on parent will now not produce any warnings --- #### 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. --> * #<issue> #### Test Plan <!-- How will this be tested prior to merging.--> - [ ] 💪 Manual - [x] ⚡ Unit test - [ ] 💚 E2E
This commit is contained in:
Abin Simon
GitHub
parent
ffbcbe8907
commit
a52b085cea
@ -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"))
|
||||
}
|
||||
}
|
||||
|
||||
@ -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)
|
||||
}
|
||||
|
||||
@ -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 {
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user