From 76c2ac628b7b34f2b532b36ba7821f9f63cbc8b0 Mon Sep 17 00:00:00 2001 From: Abin Simon Date: Fri, 3 Feb 2023 10:38:28 +0530 Subject: [PATCH] Fix some edge cases around OneDrive permissions backup (#2370) ## Description This fixes tying to parse link shares as permissions and some error/retry handling. ## 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 - [x] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Test - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup ## Issue(s) * # ## Test Plan - [x] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- CHANGELOG.md | 1 + src/internal/connector/onedrive/collection.go | 6 +- src/internal/connector/onedrive/item.go | 18 ++++- src/internal/connector/onedrive/item_test.go | 70 +++++++++++++++++++ 4 files changed, 90 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 09d3845da..53ec01a36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - When the same user has permissions to a file and the containing folder, we only restore folder level permissions for the user and no separate file only permission is restored. +- Link shares are not restored ## [v0.2.0] (alpha) - 2023-1-29 diff --git a/src/internal/connector/onedrive/collection.go b/src/internal/connector/onedrive/collection.go index 3b8ff5cbb..2de05c073 100644 --- a/src/internal/connector/onedrive/collection.go +++ b/src/internal/connector/onedrive/collection.go @@ -292,7 +292,9 @@ func (oc *Collection) populateItems(ctx context.Context) { itemMeta, itemMetaSize, err = oc.itemMetaReader(ctx, oc.service, oc.driveID, item) // retry on Timeout type errors, break otherwise. - if err == nil || !graph.IsErrTimeout(err) { + if err == nil || + !graph.IsErrTimeout(err) || + !graph.IsInternalServerError(err) { break } @@ -302,7 +304,7 @@ func (oc *Collection) populateItems(ctx context.Context) { } if err != nil { - errUpdater(*item.GetId(), err) + errUpdater(*item.GetId(), errors.Wrap(err, "failed to get item permissions")) return } } diff --git a/src/internal/connector/onedrive/item.go b/src/internal/connector/onedrive/item.go index 1526f1401..c527ce09b 100644 --- a/src/internal/connector/onedrive/item.go +++ b/src/internal/connector/onedrive/item.go @@ -191,12 +191,24 @@ func oneDriveItemMetaInfo( perm, err := service.Client().DrivesById(driveID).ItemsById(*itemID).Permissions().Get(ctx, nil) if err != nil { - return Metadata{}, errors.Wrapf(err, "failed to get item permissions %s", *itemID) + return Metadata{}, err } + uperms := filterUserPermissions(perm.GetValue()) + + return Metadata{Permissions: uperms}, nil +} + +func filterUserPermissions(perms []models.Permissionable) []UserPermission { up := []UserPermission{} - for _, p := range perm.GetValue() { + for _, p := range perms { + if p.GetGrantedToV2() == nil { + // For link shares, we get permissions without a user + // specified + continue + } + roles := []string{} for _, r := range p.GetRoles() { @@ -218,7 +230,7 @@ func oneDriveItemMetaInfo( }) } - return Metadata{Permissions: up}, nil + return up } // sharePointItemInfo will populate a details.SharePointInfo struct diff --git a/src/internal/connector/onedrive/item_test.go b/src/internal/connector/onedrive/item_test.go index b0e42943a..a2e008ec5 100644 --- a/src/internal/connector/onedrive/item_test.go +++ b/src/internal/connector/onedrive/item_test.go @@ -8,6 +8,7 @@ import ( msgraphsdk "github.com/microsoftgraph/msgraph-sdk-go" "github.com/microsoftgraph/msgraph-sdk-go/models" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" @@ -257,3 +258,72 @@ func (suite *ItemIntegrationSuite) TestDriveGetFolder() { }) } } + +func getPermsUperms(permID, userID string, scopes []string) (models.Permissionable, UserPermission) { + identity := models.NewIdentity() + identity.SetAdditionalData(map[string]any{"email": &userID}) + + sharepointIdentity := models.NewSharePointIdentitySet() + sharepointIdentity.SetUser(identity) + + perm := models.NewPermission() + perm.SetId(&permID) + perm.SetRoles([]string{"read"}) + perm.SetGrantedToV2(sharepointIdentity) + + uperm := UserPermission{ + ID: permID, + Roles: []string{"read"}, + Email: userID, + } + + return perm, uperm +} + +func TestOneDrivePermissionsFilter(t *testing.T) { + permID := "fakePermId" + userID := "fakeuser@provider.com" + userID2 := "fakeuser2@provider.com" + + readPerm, readUperm := getPermsUperms(permID, userID, []string{"read"}) + readWritePerm, readWriteUperm := getPermsUperms(permID, userID2, []string{"read", "write"}) + + noPerm, _ := getPermsUperms(permID, userID, []string{"read"}) + noPerm.SetGrantedToV2(nil) // eg: link shares + + cases := []struct { + name string + graphPermissions []models.Permissionable + parsedPermissions []UserPermission + }{ + { + name: "no perms", + graphPermissions: []models.Permissionable{}, + parsedPermissions: []UserPermission{}, + }, + { + name: "no user bound to perms", + graphPermissions: []models.Permissionable{noPerm}, + parsedPermissions: []UserPermission{}, + }, + { + name: "user with read permissions", + graphPermissions: []models.Permissionable{readPerm}, + parsedPermissions: []UserPermission{readUperm}, + }, + { + name: "user with read and write permissions", + graphPermissions: []models.Permissionable{readWritePerm}, + parsedPermissions: []UserPermission{readWriteUperm}, + }, + { + name: "multiple users with separate permissions", + graphPermissions: []models.Permissionable{readPerm, readWritePerm}, + parsedPermissions: []UserPermission{readUperm, readWriteUperm}, + }, + } + for _, tc := range cases { + actual := filterUserPermissions(tc.graphPermissions) + assert.ElementsMatch(t, tc.parsedPermissions, actual) + } +}