From 33ecbd18208618c7447038f01018b6d77f7ab021 Mon Sep 17 00:00:00 2001 From: Abin Simon Date: Tue, 7 Mar 2023 09:37:37 +0530 Subject: [PATCH] Switch to using ids for storing permissions (#2674) This also enable backing up and restoring permissions for groups and applications. --- #### Does this PR need a docs update or release note? - [x] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [ ] :no_entry: No #### Type of change - [x] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Test - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup #### Issue(s) * fixes https://github.com/alcionai/corso/issues/2621 * fixes https://github.com/alcionai/corso/issues/2673 #### Test Plan - [ ] :muscle: Manual - [ ] :zap: Unit test - [x] :green_heart: E2E --- CHANGELOG.md | 1 + .../graph_connector_onedrive_test.go | 183 ++++++++++-------- src/internal/connector/onedrive/collection.go | 3 +- src/internal/connector/onedrive/item.go | 32 ++- src/internal/connector/onedrive/item_test.go | 80 ++++++-- src/internal/connector/onedrive/permission.go | 9 +- 6 files changed, 208 insertions(+), 100 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 72e55c84c..2519e666a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Show owner information when doing backup list in json format - Onedrive files that are flagged as malware get skipped during backup. +- Permissions for groups can now be backed up and restored ### Fixed - Corso-generated .meta files and permissions no longer appear in the backup details. diff --git a/src/internal/connector/graph_connector_onedrive_test.go b/src/internal/connector/graph_connector_onedrive_test.go index 7d8891555..0d3e3db6e 100644 --- a/src/internal/connector/graph_connector_onedrive_test.go +++ b/src/internal/connector/graph_connector_onedrive_test.go @@ -20,17 +20,27 @@ import ( "github.com/alcionai/corso/src/pkg/path" ) -func getMetadata(fileName, user string, roles []string) onedrive.Metadata { - if len(user) == 0 || len(roles) == 0 { +// For any version post this(inclusive), we expect to be using IDs for +// permission instead of email +const versionPermissionSwitchedToID = version.OneDrive4DirIncludesPermissions + +func getMetadata(fileName string, perm permData, permUseID bool) onedrive.Metadata { + if len(perm.user) == 0 || len(perm.roles) == 0 { return onedrive.Metadata{FileName: fileName} } - id := base64.StdEncoding.EncodeToString([]byte(user + strings.Join(roles, "+"))) + id := base64.StdEncoding.EncodeToString([]byte(perm.user + strings.Join(perm.roles, "+"))) + uperm := onedrive.UserPermission{ID: id, Roles: perm.roles} + + if permUseID { + uperm.EntityID = perm.entityID + } else { + uperm.Email = perm.user + } + testMeta := onedrive.Metadata{ - FileName: fileName, - Permissions: []onedrive.UserPermission{ - {ID: id, Roles: roles, Email: user}, - }, + FileName: fileName, + Permissions: []onedrive.UserPermission{uperm}, } return testMeta @@ -66,12 +76,12 @@ func onedriveItemWithData( func onedriveMetadata( t *testing.T, fileName, itemID string, - user string, - roles []string, + perm permData, + permUseID bool, ) itemInfo { t.Helper() - testMeta := getMetadata(fileName, user, roles) + testMeta := getMetadata(fileName, perm, permUseID) testMetaJSON, err := json.Marshal(testMeta) require.NoError(t, err, "marshalling metadata") @@ -85,10 +95,12 @@ func onedriveMetadata( type GraphConnectorOneDriveIntegrationSuite struct { tester.Suite - connector *GraphConnector - user string - secondaryUser string - acct account.Account + connector *GraphConnector + user string + userID string + secondaryUser string + secondaryUserID string + acct account.Account } func TestGraphConnectorOneDriveIntegrationSuite(t *testing.T) { @@ -110,6 +122,14 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) SetupSuite() { suite.secondaryUser = tester.SecondaryM365UserID(suite.T()) suite.acct = tester.NewM365Account(suite.T()) + user, err := suite.connector.Owners.Users().GetByID(ctx, suite.user) + require.NoErrorf(suite.T(), err, "fetching user %s", suite.user) + suite.userID = *user.GetId() + + secondaryUser, err := suite.connector.Owners.Users().GetByID(ctx, suite.secondaryUser) + require.NoErrorf(suite.T(), err, "fetching user %s", suite.secondaryUser) + suite.secondaryUserID = *secondaryUser.GetId() + tester.LogTimeOfTest(suite.T()) } @@ -157,12 +177,7 @@ func (c onedriveCollection) collection() colInfo { } } -func (c *onedriveCollection) withFile( - name string, - fileData []byte, - user string, - roles []string, -) *onedriveCollection { +func (c *onedriveCollection) withFile(name string, fileData []byte, perm permData) *onedriveCollection { switch c.backupVersion { case 0: // Lookups will occur using the most recent version of things so we need @@ -184,8 +199,8 @@ func (c *onedriveCollection) withFile( c.t, "", name+onedrive.MetaFileSuffix, - user, - roles) + perm, + c.backupVersion >= versionPermissionSwitchedToID) c.items = append(c.items, metadata) c.aux = append(c.aux, metadata) @@ -196,11 +211,7 @@ func (c *onedriveCollection) withFile( return c } -func (c *onedriveCollection) withFolder( - name string, - user string, - roles []string, -) *onedriveCollection { +func (c *onedriveCollection) withFolder(name string, perm permData) *onedriveCollection { switch c.backupVersion { case 0, version.OneDrive4DirIncludesPermissions: return c @@ -212,8 +223,8 @@ func (c *onedriveCollection) withFolder( c.t, "", name+onedrive.DirMetaFileSuffix, - user, - roles)) + perm, + c.backupVersion >= versionPermissionSwitchedToID)) default: assert.FailNowf(c.t, "bad backup version", "version %d", c.backupVersion) @@ -224,10 +235,7 @@ func (c *onedriveCollection) withFolder( // withPermissions adds permissions to the folder represented by this // onedriveCollection. -func (c *onedriveCollection) withPermissions( - user string, - roles []string, -) *onedriveCollection { +func (c *onedriveCollection) withPermissions(perm permData) *onedriveCollection { // These versions didn't store permissions for the folder or didn't store them // in the folder's collection. if c.backupVersion < version.OneDrive4DirIncludesPermissions { @@ -244,8 +252,8 @@ func (c *onedriveCollection) withPermissions( c.t, name, name+onedrive.DirMetaFileSuffix, - user, - roles) + perm, + c.backupVersion >= versionPermissionSwitchedToID) c.items = append(c.items, metadata) c.aux = append(c.aux, metadata) @@ -254,8 +262,9 @@ func (c *onedriveCollection) withPermissions( } type permData struct { - user string - roles []string + user string // user is only for older versions + entityID string + roles []string } type itemData struct { @@ -285,14 +294,14 @@ func testDataForInfo(t *testing.T, cols []onedriveColInfo, backupVersion int) [] onedriveCol := newOneDriveCollection(t, c.pathElements, backupVersion) for _, f := range c.files { - onedriveCol.withFile(f.name, f.data, f.perms.user, f.perms.roles) + onedriveCol.withFile(f.name, f.data, f.perms) } for _, d := range c.folders { - onedriveCol.withFolder(d.name, d.perms.user, d.perms.roles) + onedriveCol.withFolder(d.name, d.perms) } - onedriveCol.withPermissions(c.perms.user, c.perms.roles) + onedriveCol.withPermissions(c.perms) res = append(res, onedriveCol.collection()) } @@ -503,8 +512,9 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndBa name: fileName, data: fileAData, perms: permData{ - user: suite.secondaryUser, - roles: writePerm, + user: suite.secondaryUser, + entityID: suite.secondaryUserID, + roles: writePerm, }, }, { @@ -521,15 +531,17 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndBa { name: folderAName, perms: permData{ - user: suite.secondaryUser, - roles: readPerm, + user: suite.secondaryUser, + entityID: suite.secondaryUserID, + roles: readPerm, }, }, { name: folderCName, perms: permData{ - user: suite.secondaryUser, - roles: readPerm, + user: suite.secondaryUser, + entityID: suite.secondaryUserID, + roles: readPerm, }, }, }, @@ -543,8 +555,9 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndBa name: fileName, data: fileBData, perms: permData{ - user: suite.secondaryUser, - roles: readPerm, + user: suite.secondaryUser, + entityID: suite.secondaryUserID, + roles: readPerm, }, }, }, @@ -552,8 +565,9 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndBa { name: folderAName, perms: permData{ - user: suite.secondaryUser, - roles: readPerm, + user: suite.secondaryUser, + entityID: suite.secondaryUserID, + roles: readPerm, }, }, }, @@ -567,14 +581,16 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndBa name: fileName, data: fileDData, perms: permData{ - user: suite.secondaryUser, - roles: readPerm, + user: suite.secondaryUser, + entityID: suite.secondaryUserID, + roles: readPerm, }, }, }, perms: permData{ - user: suite.secondaryUser, - roles: readPerm, + user: suite.secondaryUser, + entityID: suite.secondaryUserID, + roles: readPerm, }, }, { @@ -586,14 +602,16 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndBa name: fileName, data: fileEData, perms: permData{ - user: suite.secondaryUser, - roles: writePerm, + user: suite.secondaryUser, + entityID: suite.secondaryUserID, + roles: writePerm, }, }, }, perms: permData{ - user: suite.secondaryUser, - roles: readPerm, + user: suite.secondaryUser, + entityID: suite.secondaryUserID, + roles: readPerm, }, }, { @@ -607,8 +625,9 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndBa }, }, perms: permData{ - user: suite.secondaryUser, - roles: readPerm, + user: suite.secondaryUser, + entityID: suite.secondaryUserID, + roles: readPerm, }, }, }, @@ -619,6 +638,9 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndBa for vn := test.startVersion; vn <= version.Backup; vn++ { suite.Run(fmt.Sprintf("Version%d", vn), func() { t := suite.T() + // Ideally this can always be true or false and still + // work, but limiting older versions to use emails so as + // to validate that flow as well. input := testDataForInfo(t, test.cols, vn) testData := restoreBackupInfoMultiVersion{ @@ -670,8 +692,9 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsBackupAndNoR name: fileName, data: fileAData, perms: permData{ - user: suite.secondaryUser, - roles: writePerm, + user: suite.secondaryUser, + entityID: suite.secondaryUserID, + roles: writePerm, }, }, }, @@ -742,6 +765,18 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndNo suite.user, ) + secondaryUserRead := permData{ + user: suite.secondaryUser, + entityID: suite.secondaryUserID, + roles: readPerm, + } + + secondaryUserWrite := permData{ + user: suite.secondaryUser, + entityID: suite.secondaryUserID, + roles: writePerm, + } + test := restoreBackupInfoMultiVersion{ service: path.OneDriveService, resource: Users, @@ -759,13 +794,11 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndNo withFile( fileName, fileAData, - suite.secondaryUser, - writePerm, + secondaryUserWrite, ). withFolder( folderBName, - suite.secondaryUser, - readPerm, + secondaryUserRead, ). collection(), newOneDriveCollection( @@ -781,12 +814,10 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndNo withFile( fileName, fileEData, - suite.secondaryUser, - readPerm, + secondaryUserRead, ). withPermissions( - suite.secondaryUser, - readPerm, + secondaryUserRead, ). collection(), }, @@ -803,13 +834,11 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndNo withFile( fileName, fileAData, - "", - nil, + permData{}, ). withFolder( folderBName, - "", - nil, + permData{}, ). collection(), newOneDriveCollection( @@ -825,14 +854,12 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndNo withFile( fileName, fileEData, - "", - nil, + permData{}, ). // Call this to generate a meta file with the folder name that we can // check. withPermissions( - "", - nil, + permData{}, ). collection(), }, diff --git a/src/internal/connector/onedrive/collection.go b/src/internal/connector/onedrive/collection.go index 16d442182..bf50e3f3f 100644 --- a/src/internal/connector/onedrive/collection.go +++ b/src/internal/connector/onedrive/collection.go @@ -204,7 +204,8 @@ func (oc Collection) DoNotMergeItems() bool { type UserPermission struct { ID string `json:"id,omitempty"` Roles []string `json:"role,omitempty"` - Email string `json:"email,omitempty"` + Email string `json:"email,omitempty"` // DEPRECATED: Replaced with UserID in newer backups + EntityID string `json:"entityId,omitempty"` Expiration *time.Time `json:"expiration,omitempty"` } diff --git a/src/internal/connector/onedrive/item.go b/src/internal/connector/onedrive/item.go index b0a98f511..0c33c0111 100644 --- a/src/internal/connector/onedrive/item.go +++ b/src/internal/connector/onedrive/item.go @@ -237,12 +237,12 @@ func oneDriveItemPermissionInfo( return nil, err } - uperms := filterUserPermissions(perm.GetValue()) + uperms := filterUserPermissions(ctx, perm.GetValue()) return uperms, nil } -func filterUserPermissions(perms []models.Permissionable) []UserPermission { +func filterUserPermissions(ctx context.Context, perms []models.Permissionable) []UserPermission { up := []UserPermission{} for _, p := range perms { @@ -252,6 +252,7 @@ func filterUserPermissions(perms []models.Permissionable) []UserPermission { continue } + gv2 := p.GetGrantedToV2() roles := []string{} for _, r := range p.GetRoles() { @@ -265,10 +266,35 @@ func filterUserPermissions(perms []models.Permissionable) []UserPermission { continue } + entityID := "" + if gv2.GetUser() != nil { + entityID = *gv2.GetUser().GetId() + } else if gv2.GetGroup() != nil { + entityID = *gv2.GetGroup().GetId() + } else { + // TODO Add appliction permissions when adding permissions for SharePoint + // https://devblogs.microsoft.com/microsoft365dev/controlling-app-access-on-specific-sharepoint-site-collections/ + logm := logger.Ctx(ctx) + if gv2.GetApplication() != nil { + logm.With("application_id", *gv2.GetApplication().GetId()) + } + if gv2.GetDevice() != nil { + logm.With("application_id", *gv2.GetDevice().GetId()) + } + logm.Warn("untracked permission") + } + + // Technically GrantedToV2 can also contain devices, but the + // documentation does not mention about devices in permissions + if entityID == "" { + // This should ideally not be hit + continue + } + up = append(up, UserPermission{ ID: ptr.Val(p.GetId()), Roles: roles, - Email: *p.GetGrantedToV2().GetUser().GetAdditionalData()["email"].(*string), + EntityID: entityID, Expiration: p.GetExpirationDateTime(), }) } diff --git a/src/internal/connector/onedrive/item_test.go b/src/internal/connector/onedrive/item_test.go index fca6a5614..ef6954c40 100644 --- a/src/internal/connector/onedrive/item_test.go +++ b/src/internal/connector/onedrive/item_test.go @@ -235,12 +235,23 @@ func (suite *ItemIntegrationSuite) TestDriveGetFolder() { } } -func getPermsUperms(permID, userID string, scopes []string) (models.Permissionable, UserPermission) { +func getPermsUperms(permID, userID, entity string, scopes []string) (models.Permissionable, UserPermission) { identity := models.NewIdentity() + identity.SetId(&userID) identity.SetAdditionalData(map[string]any{"email": &userID}) sharepointIdentity := models.NewSharePointIdentitySet() - sharepointIdentity.SetUser(identity) + + switch entity { + case "user": + sharepointIdentity.SetUser(identity) + case "group": + sharepointIdentity.SetGroup(identity) + case "application": + sharepointIdentity.SetApplication(identity) + case "device": + sharepointIdentity.SetDevice(identity) + } perm := models.NewPermission() perm.SetId(&permID) @@ -248,23 +259,34 @@ func getPermsUperms(permID, userID string, scopes []string) (models.Permissionab perm.SetGrantedToV2(sharepointIdentity) uperm := UserPermission{ - ID: permID, - Roles: []string{"read"}, - Email: userID, + ID: permID, + Roles: []string{"read"}, + EntityID: userID, } return perm, uperm } -func TestOneDrivePermissionsFilter(t *testing.T) { +type ItemUnitTestSuite struct { + tester.Suite +} + +func TestItemUnitTestSuite(t *testing.T) { + suite.Run(t, &ItemUnitTestSuite{Suite: tester.NewUnitSuite(t)}) +} + +func (suite *ItemUnitTestSuite) TestOneDrivePermissionsFilter() { 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"}) + userReadPerm, userReadUperm := getPermsUperms(permID, userID, "user", []string{"read"}) + userReadWritePerm, userReadWriteUperm := getPermsUperms(permID, userID2, "user", []string{"read", "write"}) - noPerm, _ := getPermsUperms(permID, userID, []string{"read"}) + groupReadPerm, groupReadUperm := getPermsUperms(permID, userID, "group", []string{"read"}) + groupReadWritePerm, groupReadWriteUperm := getPermsUperms(permID, userID2, "group", []string{"read", "write"}) + + noPerm, _ := getPermsUperms(permID, userID, "user", []string{"read"}) noPerm.SetGrantedToV2(nil) // eg: link shares cases := []struct { @@ -282,24 +304,48 @@ func TestOneDrivePermissionsFilter(t *testing.T) { graphPermissions: []models.Permissionable{noPerm}, parsedPermissions: []UserPermission{}, }, + + // user { name: "user with read permissions", - graphPermissions: []models.Permissionable{readPerm}, - parsedPermissions: []UserPermission{readUperm}, + graphPermissions: []models.Permissionable{userReadPerm}, + parsedPermissions: []UserPermission{userReadUperm}, }, { name: "user with read and write permissions", - graphPermissions: []models.Permissionable{readWritePerm}, - parsedPermissions: []UserPermission{readWriteUperm}, + graphPermissions: []models.Permissionable{userReadWritePerm}, + parsedPermissions: []UserPermission{userReadWriteUperm}, }, { name: "multiple users with separate permissions", - graphPermissions: []models.Permissionable{readPerm, readWritePerm}, - parsedPermissions: []UserPermission{readUperm, readWriteUperm}, + graphPermissions: []models.Permissionable{userReadPerm, userReadWritePerm}, + parsedPermissions: []UserPermission{userReadUperm, userReadWriteUperm}, + }, + + // group + { + name: "group with read permissions", + graphPermissions: []models.Permissionable{groupReadPerm}, + parsedPermissions: []UserPermission{groupReadUperm}, + }, + { + name: "group with read and write permissions", + graphPermissions: []models.Permissionable{groupReadWritePerm}, + parsedPermissions: []UserPermission{groupReadWriteUperm}, + }, + { + name: "multiple groups with separate permissions", + graphPermissions: []models.Permissionable{groupReadPerm, groupReadWritePerm}, + parsedPermissions: []UserPermission{groupReadUperm, groupReadWriteUperm}, }, } for _, tc := range cases { - actual := filterUserPermissions(tc.graphPermissions) - assert.ElementsMatch(t, tc.parsedPermissions, actual) + suite.Run(tc.name, func() { + ctx, flush := tester.NewContext() + defer flush() + + actual := filterUserPermissions(ctx, tc.graphPermissions) + assert.ElementsMatch(suite.T(), tc.parsedPermissions, actual) + }) } } diff --git a/src/internal/connector/onedrive/permission.go b/src/internal/connector/onedrive/permission.go index c69d19f78..78c15df58 100644 --- a/src/internal/connector/onedrive/permission.go +++ b/src/internal/connector/onedrive/permission.go @@ -210,7 +210,14 @@ func restorePermissions( pbody.SetRequireSignIn(&rs) rec := models.NewDriveRecipient() - rec.SetEmail(&p.Email) + if p.EntityID != "" { + rec.SetObjectId(&p.EntityID) + } else { + // Previous versions used to only store email for a + // permissions. Use that if id is not found. + rec.SetEmail(&p.Email) + } + pbody.SetRecipients([]models.DriveRecipientable{rec}) np, err := service.Client().DrivesById(driveID).ItemsById(itemID).Invite().Post(ctx, pbody, nil)