larryh/corso
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Ignore permissions restore failure with a log (#4220)

Browse Source 
<!-- PR description-->

---

#### 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
- [ ] 🐛 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.-->
- [x] 💪 Manual
- [ ] ⚡ Unit test
- [ ] 💚 E2E
This commit is contained in:
Abin Simon 2023-09-12 03:30:24 +05:30 committed by GitHub
parent f335f6de6f
commit b9c8bcbd30
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 154 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

44
src/internal/m365/collection/drive/permission.go
View File

@ -12,7 +12,9 @@ import (
"github.com/alcionai/corso/src/internal/common/ptr"
"github.com/alcionai/corso/src/internal/data"
"github.com/alcionai/corso/src/internal/m365/collection/drive/metadata"
"github.com/alcionai/corso/src/internal/m365/graph"
"github.com/alcionai/corso/src/internal/version"
"github.com/alcionai/corso/src/pkg/fault"
"github.com/alcionai/corso/src/pkg/logger"
"github.com/alcionai/corso/src/pkg/path"
)
@ -193,7 +195,10 @@ func UpdatePermissions(
itemID string,
permAdded, permRemoved []metadata.Permission,
oldPermIDToNewID *xsync.MapOf[string, string],
errs *fault.Bus,
) error {
el := errs.Local()
// 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 {
@ -223,6 +228,10 @@ func UpdatePermissions(
}
for _, p := range permAdded {
if el.Failure() != nil {
break
}
ictx := clues.Add(
ctx,
"permission_entity_type", p.EntityType,
@ -267,14 +276,20 @@ func UpdatePermissions(
pbody.SetRecipients([]models.DriveRecipientable{rec})
newPerm, err := udip.PostItemPermissionUpdate(ictx, driveID, itemID, pbody)
if graph.IsErrUsersCannotBeResolved(err) {
logger.CtxErr(ictx, err).Info("Unable to restore link share")
continue
}
if err != nil {
return clues.Stack(err)
el.AddRecoverable(ictx, clues.Stack(err))
continue
}
oldPermIDToNewID.Store(p.ID, ptr.Val(newPerm.GetValue()[0].GetId()))
}
return nil
return el.Failure()
}
type updateDeleteItemLinkSharer interface {
@ -289,14 +304,20 @@ func UpdateLinkShares(
itemID string,
lsAdded, lsRemoved []metadata.LinkShare,
oldLinkShareIDToNewID *xsync.MapOf[string, string],
errs *fault.Bus,
) (bool, error) {
// You can only delete inherited sharing links the first time you
// create a sharing link which is done using
// `retainInheritedPermissions`. We cannot separately delete any
// inherited link shares via DELETE API call like for permissions.
alreadyDeleted := false
el := errs.Local()
for _, ls := range lsAdded {
if el.Failure() != nil {
break
}
ictx := clues.Add(ctx, "link_share_id", ls.ID)
// Links with password are not shared with a specific user
@ -363,13 +384,23 @@ func UpdateLinkShares(
}
newLS, err := upils.PostItemLinkShareUpdate(ictx, driveID, itemID, lsbody)
if graph.IsErrUsersCannotBeResolved(err) {
logger.CtxErr(ictx, err).Info("Unable to restore link share")
continue
}
if err != nil {
return alreadyDeleted, clues.Stack(err)
el.AddRecoverable(ctx, clues.Stack(err))
continue
}
oldLinkShareIDToNewID.Store(ls.ID, ptr.Val(newLS.GetId()))
}
if el.Failure() != nil {
return alreadyDeleted, el.Failure()
}
// It is possible to have empty link shares even though we should
// have inherited one if the user creates a link using
// `retainInheritedPermissions` as false, but then deleted it. We
@ -411,6 +442,7 @@ func RestorePermissions(
itemPath path.Path,
current metadata.Metadata,
caches *restoreCaches,
errs *fault.Bus,
) error {
if current.SharingMode == metadata.SharingModeInherited {
return nil
@ -435,7 +467,8 @@ func RestorePermissions(
itemID,
lsAdded,
lsRemoved,
caches.OldLinkShareIDToNewID)
caches.OldLinkShareIDToNewID,
errs)
if err != nil {
return clues.Wrap(err, "updating link shares")
}
@ -464,7 +497,8 @@ func RestorePermissions(
itemID,
permAdded,
permRemoved,
caches.OldPermIDToNewID)
caches.OldPermIDToNewID,
errs)
if err != nil {
return clues.Wrap(err, "updating permissions")
}

25
src/internal/m365/collection/drive/restore.go
View File

@ -130,7 +130,8 @@ func RestoreCollection(
dc.FullPath(),
colMeta,
caches,
rcc.RestoreConfig.IncludePermissions)
rcc.RestoreConfig.IncludePermissions,
errs)
if err != nil {
return metrics, clues.Wrap(err, "creating folders for restore")
}
@ -211,7 +212,8 @@ func RestoreCollection(
caches,
itemData,
itemPath,
ctr)
ctr,
errs)
// skipped items don't get counted, but they can error
if !skipped {
@ -260,6 +262,7 @@ func restoreItem(
itemData data.Item,
itemPath path.Path,
ctr *count.Bus,
errs *fault.Bus,
) (details.ItemInfo, bool, error) {
itemUUID := itemData.ID()
ctx = clues.Add(ctx, "item_id", itemUUID)
@ -332,7 +335,8 @@ func restoreItem(
caches,
itemPath,
itemData,
ctr)
ctr,
errs)
if err != nil {
if errors.Is(err, graph.ErrItemAlreadyExistsConflict) && rcc.RestoreConfig.OnCollision == control.Skip {
return details.ItemInfo{}, true, nil
@ -357,7 +361,8 @@ func restoreItem(
caches,
itemPath,
itemData,
ctr)
ctr,
errs)
if err != nil {
if errors.Is(err, graph.ErrItemAlreadyExistsConflict) && rcc.RestoreConfig.OnCollision == control.Skip {
return details.ItemInfo{}, true, nil
@ -412,6 +417,7 @@ func restoreV1File(
itemPath path.Path,
itemData data.Item,
ctr *count.Bus,
errs *fault.Bus,
) (details.ItemInfo, error) {
trimmedName := strings.TrimSuffix(itemData.ID(), metadata.DataFileSuffix)
@ -452,7 +458,8 @@ func restoreV1File(
itemID,
itemPath,
meta,
caches)
caches,
errs)
if err != nil {
return details.ItemInfo{}, clues.Wrap(err, "restoring item permissions")
}
@ -472,6 +479,7 @@ func restoreV6File(
itemPath path.Path,
itemData data.Item,
ctr *count.Bus,
errs *fault.Bus,
) (details.ItemInfo, error) {
trimmedName := strings.TrimSuffix(itemData.ID(), metadata.DataFileSuffix)
@ -528,7 +536,8 @@ func restoreV6File(
itemID,
itemPath,
meta,
caches)
caches,
errs)
if err != nil {
return details.ItemInfo{}, clues.Wrap(err, "restoring item permissions")
}
@ -550,6 +559,7 @@ func CreateRestoreFolders(
folderMetadata metadata.Metadata,
caches *restoreCaches,
restorePerms bool,
errs *fault.Bus,
) (string, error) {
id, err := createRestoreFolders(
ctx,
@ -577,7 +587,8 @@ func CreateRestoreFolders(
id,
folderPath,
folderMetadata,
caches)
caches,
errs)
return id, err
}

4
src/internal/m365/collection/drive/restore_test.go
View File

@ -23,6 +23,7 @@ import (
"github.com/alcionai/corso/src/internal/version"
"github.com/alcionai/corso/src/pkg/control"
"github.com/alcionai/corso/src/pkg/count"
"github.com/alcionai/corso/src/pkg/fault"
"github.com/alcionai/corso/src/pkg/path"
"github.com/alcionai/corso/src/pkg/services/m365/api"
apiMock "github.com/alcionai/corso/src/pkg/services/m365/api/mock"
@ -246,7 +247,8 @@ func (suite *RestoreUnitSuite) TestRestoreItem_collisionHandling() {
ItemInfo: odStub.DriveItemInfo(),
},
nil,
ctr)
ctr,
fault.New(true))
require.NoError(t, err, clues.ToCore(err))
test.expectSkipped(t, skip)

30
src/internal/m365/graph/errors.go
View File

@ -49,6 +49,7 @@ const (
// nameAlreadyExists occurs when a request with
// @microsoft.graph.conflictBehavior=fail finds a conflicting file.
nameAlreadyExists errorCode = "nameAlreadyExists"
noResolvedUsers errorCode = "noResolvedUsers"
QuotaExceeded errorCode = "ErrorQuotaExceeded"
RequestResourceNotFound errorCode = "Request_ResourceNotFound"
// Returned when we try to get the inbox of a user that doesn't exist.
@ -66,6 +67,7 @@ const (
MysiteURLNotFound errorMessage = "unable to retrieve user's mysite url"
MysiteNotFound errorMessage = "user's mysite not found"
NoSPLicense errorMessage = "Tenant does not have a SPO license"
usersCannotBeResolved errorMessage = "One or more users could not be resolved"
)
const (
@ -225,6 +227,10 @@ func IsErrFolderExists(err error) bool {
return hasErrorCode(err, folderExists)
}
func IsErrUsersCannotBeResolved(err error) bool {
return hasErrorCode(err, noResolvedUsers) || hasErrorMessage(err, usersCannotBeResolved)
}
// ---------------------------------------------------------------------------
// error parsers
// ---------------------------------------------------------------------------
@ -252,6 +258,30 @@ func hasErrorCode(err error, codes ...errorCode) bool {
return filters.Equal(cs).Compare(code)
}
// only use this as a last resort. Prefer the code or statuscode if possible.
func hasErrorMessage(err error, msgs ...errorMessage) bool {
if err == nil {
return false
}
var oDataError odataerrors.ODataErrorable
if !errors.As(err, &oDataError) {
return false
}
msg, ok := ptr.ValOK(oDataError.GetErrorEscaped().GetMessage())
if !ok {
return false
}
cs := make([]string, len(msgs))
for i, c := range msgs {
cs[i] = string(c)
}
return filters.Equal(cs).Compare(msg)
}
// Wrap is a helper function that extracts ODataError metadata from
// the error. If the error is not an ODataError type, returns the error.
func Wrap(ctx context.Context, e error, msg string) *clues.Err {

53
src/internal/m365/graph/errors_test.go
View File

@ -4,6 +4,7 @@ import (
"context"
"encoding/json"
"net/http"
"strings"
"syscall"
"testing"
@ -466,7 +467,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrFolderExists() {
expect: assert.False,
},
{
name: "matching oDataErr",
name: "matching oDataErr msg",
err: odErr(string(folderExists)),
expect: assert.True,
},
@ -489,6 +490,56 @@ func (suite *GraphErrorsUnitSuite) TestIsErrFolderExists() {
}
}
func (suite *GraphErrorsUnitSuite) TestIsErrUsersCannotBeResolved() {
table := []struct {
name string
err error
expect assert.BoolAssertionFunc
}{
{
name: "nil",
err: nil,
expect: assert.False,
},
{
name: "non-matching",
err: assert.AnError,
expect: assert.False,
},
{
name: "non-matching oDataErr",
err: odErrMsg("InvalidRequest", "cant resolve users"),
expect: assert.False,
},
{
name: "matching oDataErr code",
err: odErrMsg(string(noResolvedUsers), "usersCannotBeResolved"),
expect: assert.True,
},
{
name: "matching oDataErr msg",
err: odErrMsg("InvalidRequest", string(usersCannotBeResolved)),
expect: assert.True,
},
// next two tests are to make sure the checks are case insensitive
{
name: "oDataErr uppercase",
err: odErrMsg("InvalidRequest", strings.ToUpper(string(usersCannotBeResolved))),
expect: assert.True,
},
{
name: "oDataErr lowercase",
err: odErrMsg("InvalidRequest", strings.ToLower(string(usersCannotBeResolved))),
expect: assert.True,
},
}
for _, test := range table {
suite.Run(test.name, func() {
test.expect(suite.T(), IsErrUsersCannotBeResolved(test.err))
})
}
}
func (suite *GraphErrorsUnitSuite) TestIsErrCannotOpenFileAttachment() {
table := []struct {
name string

12
src/internal/operations/test/onedrive_test.go
View File

@ -393,7 +393,8 @@ func runDriveIncrementalTest(
ptr.Val(newFile.GetId()),
[]metadata.Permission{writePerm},
[]metadata.Permission{},
permissionIDMappings)
permissionIDMappings,
fault.New(true))
require.NoErrorf(t, err, "adding permission to file %v", clues.ToCore(err))
// no expectedDeets: metadata isn't tracked
},
@ -411,7 +412,8 @@ func runDriveIncrementalTest(
*newFile.GetId(),
[]metadata.Permission{},
[]metadata.Permission{writePerm},
permissionIDMappings)
permissionIDMappings,
fault.New(true))
require.NoErrorf(t, err, "removing permission from file %v", clues.ToCore(err))
// no expectedDeets: metadata isn't tracked
},
@ -430,7 +432,8 @@ func runDriveIncrementalTest(
targetContainer,
[]metadata.Permission{writePerm},
[]metadata.Permission{},
permissionIDMappings)
permissionIDMappings,
fault.New(true))
require.NoErrorf(t, err, "adding permission to container %v", clues.ToCore(err))
// no expectedDeets: metadata isn't tracked
},
@ -449,7 +452,8 @@ func runDriveIncrementalTest(
targetContainer,
[]metadata.Permission{},
[]metadata.Permission{writePerm},
permissionIDMappings)
permissionIDMappings,
fault.New(true))
require.NoErrorf(t, err, "removing permission from container %v", clues.ToCore(err))
// no expectedDeets: metadata isn't tracked
},