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

Compute parent permissions from available metadata (#3022)

Browse Source 
Previously we were querying Graph to get the current permissions and then using that to compute the permission diff. This changes that to use the metadata that we have available locally to do the computation avoiding a graph call to get permissions.

<!-- 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: --->
- [x] 🌻 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. -->
* fixes https://github.com/alcionai/corso/issues/2976

#### Test Plan

<!-- How will this be tested prior to merging.-->
- [ ] 💪 Manual
- [x] ⚡ Unit test
- [x] 💚 E2E
This commit is contained in:
Abin Simon 2023-04-12 17:52:35 +05:30 committed by GitHub
parent dd30c0e0a2
commit b1606466c2
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 340 additions and 99 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
src/internal/connector/graph_connector_onedrive_test.go
View File

@ -2,13 +2,13 @@ package connector
import (
"context"
"encoding/base64"
"encoding/json"
"fmt"
"strings"
"testing"
"github.com/alcionai/clues"
"github.com/google/uuid"
"github.com/microsoftgraph/msgraph-sdk-go/models"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@ -37,7 +37,10 @@ func getMetadata(fileName string, perm permData, permUseID bool) onedrive.Metada
}
}
id := base64.StdEncoding.EncodeToString([]byte(perm.user + strings.Join(perm.roles, "+")))
// In case of permissions, the id will usually be same for same
// user/role combo unless deleted and readded, but we have to do
// this as we only have two users of which one is already taken.
id := uuid.NewString()
uperm := onedrive.UserPermission{ID: id, Roles: perm.roles}
if permUseID {

140
src/internal/connector/onedrive/permission.go
View File

@ -6,8 +6,8 @@ import (
"github.com/alcionai/clues"
msdrive "github.com/microsoftgraph/msgraph-sdk-go/drive"
"github.com/microsoftgraph/msgraph-sdk-go/models"
"golang.org/x/exp/slices"
"github.com/alcionai/corso/src/internal/common/ptr"
"github.com/alcionai/corso/src/internal/connector/graph"
"github.com/alcionai/corso/src/internal/data"
"github.com/alcionai/corso/src/internal/version"
@ -93,7 +93,11 @@ func createRestoreFoldersWithPermissions(
service graph.Servicer,
drivePath *path.DrivePath,
restoreFolders []string,
folderPath path.Path,
folderMetadata Metadata,
folderMetas map[string]Metadata,
permissionIDMappings map[string]string,
restorePerms bool,
) (string, error) {
id, err := CreateRestoreFolders(ctx, service, drivePath.DriveID, restoreFolders)
if err != nil {
@ -105,28 +109,25 @@ func createRestoreFoldersWithPermissions(
return id, nil
}
if !restorePerms {
return id, nil
}
err = RestorePermissions(
ctx,
creds,
service,
drivePath.DriveID,
id,
folderMetadata)
folderPath,
folderMetadata,
folderMetas,
permissionIDMappings)
return id, err
}
// isSame checks equality of two string slices
func isSame(first, second []string) bool {
slices.Sort(first)
slices.Sort(second)
return slices.Equal(first, second)
}
func diffPermissions(
before, after []UserPermission,
) ([]UserPermission, []UserPermission) {
func diffPermissions(before, after []UserPermission) ([]UserPermission, []UserPermission) {
var (
added = []UserPermission{}
removed = []UserPermission{}
@ -136,8 +137,7 @@ func diffPermissions(
found := false
for _, pp := range before {
if isSame(cp.Roles, pp.Roles) &&
cp.EntityID == pp.EntityID {
if pp.ID == cp.ID {
found = true
break
}
@ -152,8 +152,7 @@ func diffPermissions(
found := false
for _, cp := range after {
if isSame(cp.Roles, pp.Roles) &&
cp.EntityID == pp.EntityID {
if pp.ID == cp.ID {
found = true
break
}
@ -167,31 +166,58 @@ func diffPermissions(
return added, removed
}
// RestorePermissions takes in the permissions that were added and the
// removed(ones present in parent but not in child) and adds/removes
// the necessary permissions on onedrive objects.
func RestorePermissions(
// computeParentPermissions computes the parent permissions by
// traversing folderMetas and finding the first item with custom
// permissions. folderMetas is expected to have all the parent
// directory metas for this to work.
func computeParentPermissions(itemPath path.Path, folderMetas map[string]Metadata) (Metadata, error) {
var (
parent path.Path
meta Metadata
err error
ok bool
)
parent = itemPath
for {
parent, err = parent.Dir()
if err != nil {
return Metadata{}, clues.New("getting parent")
}
onedrivePath, err := path.ToOneDrivePath(parent)
if err != nil {
return Metadata{}, clues.New("get parent path")
}
if len(onedrivePath.Folders) == 0 {
return Metadata{}, nil
}
meta, ok = folderMetas[parent.String()]
if !ok {
return Metadata{}, clues.New("no parent meta")
}
if meta.SharingMode == SharingModeCustom {
return meta, nil
}
}
}
// UpdatePermissions takes in the set of permission to be added and
// removed from an item to bring it to the desired state.
func UpdatePermissions(
ctx context.Context,
creds account.M365Config,
service graph.Servicer,
driveID string,
itemID string,
meta Metadata,
permAdded, permRemoved []UserPermission,
permissionIDMappings map[string]string,
) error {
if meta.SharingMode == SharingModeInherited {
return nil
}
ctx = clues.Add(ctx, "permission_item_id", itemID)
// TODO(meain): Compute this from the data that we have instead of fetching from graph
currentPermissions, err := driveItemPermissionInfo(ctx, service, driveID, itemID)
if err != nil {
return graph.Wrap(ctx, err, "fetching current permissions")
}
permAdded, permRemoved := diffPermissions(currentPermissions, meta.Permissions)
for _, p := range permRemoved {
// deletes require unique http clients
// https://github.com/alcionai/corso/issues/2707
@ -202,11 +228,16 @@ func RestorePermissions(
return graph.Wrap(ctx, err, "creating delete client")
}
pid, ok := permissionIDMappings[p.ID]
if !ok {
return clues.New("no new permission id").WithClues(ctx)
}
err = graph.NewService(a).
Client().
DrivesById(driveID).
ItemsById(itemID).
PermissionsById(p.ID).
PermissionsById(pid).
Delete(ctx, nil)
if err != nil {
return graph.Wrap(ctx, err, "removing permissions")
@ -253,11 +284,44 @@ func RestorePermissions(
pbody.SetRecipients([]models.DriveRecipientable{rec})
_, err := service.Client().DrivesById(driveID).ItemsById(itemID).Invite().Post(ctx, pbody, nil)
np, err := service.Client().DrivesById(driveID).ItemsById(itemID).Invite().Post(ctx, pbody, nil)
if err != nil {
return graph.Wrap(ctx, err, "setting permissions")
}
permissionIDMappings[p.ID] = ptr.Val(np.GetValue()[0].GetId())
}
return nil
}
// RestorePermissions takes in the permissions of an item, computes
// what permissions need to added and removed based on the parent
// folder metas and uses that to add/remove the necessary permissions
// on onedrive items.
func RestorePermissions(
ctx context.Context,
creds account.M365Config,
service graph.Servicer,
driveID string,
itemID string,
itemPath path.Path,
meta Metadata,
folderMetas map[string]Metadata,
permissionIDMappings map[string]string,
) error {
if meta.SharingMode == SharingModeInherited {
return nil
}
ctx = clues.Add(ctx, "permission_item_id", itemID)
parentPermissions, err := computeParentPermissions(itemPath, folderMetas)
if err != nil {
return clues.Wrap(err, "parent permissions").WithClues(ctx)
}
permAdded, permRemoved := diffPermissions(parentPermissions.Permissions, meta.Permissions)
return UpdatePermissions(ctx, creds, service, driveID, itemID, permAdded, permRemoved, permissionIDMappings)
}

153
src/internal/connector/onedrive/permission_test.go Normal file
View File

@ -0,0 +1,153 @@
package onedrive
import (
"fmt"
"strings"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
"github.com/alcionai/corso/src/internal/tester"
"github.com/alcionai/corso/src/pkg/path"
)
type PermissionsUnitTestSuite struct {
tester.Suite
}
func TestPermissionsUnitTestSuite(t *testing.T) {
suite.Run(t, &PermissionsUnitTestSuite{Suite: tester.NewUnitSuite(t)})
}
func (suite *PermissionsUnitTestSuite) TestComputeParentPermissions() {
entryPath := fmt.Sprintf(rootDrivePattern, "drive-id") + "/level0/level1/level2/entry"
rootEntryPath := fmt.Sprintf(rootDrivePattern, "drive-id") + "/entry"
entry, err := path.Build(
"tenant",
"user",
path.OneDriveService,
path.FilesCategory,
false,
strings.Split(entryPath, "/")...,
)
require.NoError(suite.T(), err, "creating path")
rootEntry, err := path.Build(
"tenant",
"user",
path.OneDriveService,
path.FilesCategory,
false,
strings.Split(rootEntryPath, "/")...,
)
require.NoError(suite.T(), err, "creating path")
level2, err := entry.Dir()
require.NoError(suite.T(), err, "level2 path")
level1, err := level2.Dir()
require.NoError(suite.T(), err, "level1 path")
level0, err := level1.Dir()
require.NoError(suite.T(), err, "level0 path")
metadata := Metadata{
SharingMode: SharingModeCustom,
Permissions: []UserPermission{
{
Roles: []string{"write"},
EntityID: "user-id",
},
},
}
metadata2 := Metadata{
SharingMode: SharingModeCustom,
Permissions: []UserPermission{
{
Roles: []string{"read"},
EntityID: "user-id",
},
},
}
inherited := Metadata{
SharingMode: SharingModeInherited,
Permissions: []UserPermission{},
}
table := []struct {
name string
item path.Path
meta Metadata
parentPerms map[string]Metadata
}{
{
name: "root level entry",
item: rootEntry,
meta: Metadata{},
parentPerms: map[string]Metadata{},
},
{
name: "root level directory",
item: level0,
meta: Metadata{},
parentPerms: map[string]Metadata{},
},
{
name: "direct parent perms",
item: entry,
meta: metadata,
parentPerms: map[string]Metadata{
level2.String(): metadata,
},
},
{
name: "top level parent perms",
item: entry,
meta: metadata,
parentPerms: map[string]Metadata{
level2.String(): inherited,
level1.String(): inherited,
level0.String(): metadata,
},
},
{
name: "all inherited",
item: entry,
meta: Metadata{},
parentPerms: map[string]Metadata{
level2.String(): inherited,
level1.String(): inherited,
level0.String(): inherited,
},
},
{
name: "multiple custom permission",
item: entry,
meta: metadata,
parentPerms: map[string]Metadata{
level2.String(): inherited,
level1.String(): metadata,
level0.String(): metadata2,
},
},
}
for _, test := range table {
suite.Run(test.name, func() {
_, flush := tester.NewContext()
defer flush()
t := suite.T()
m, err := computeParentPermissions(test.item, test.parentPerms)
require.NoError(t, err, "compute permissions")
assert.Equal(t, m, test.meta)
})
}
}

79
src/internal/connector/onedrive/restore.go
View File

@ -46,7 +46,11 @@ func RestoreCollections(
var (
restoreMetrics support.CollectionMetrics
metrics support.CollectionMetrics
folderMetas map[string]Metadata
folderMetas = map[string]Metadata{}
// permissionIDMappings is used to map between old and new id
// of permissions as we restore them
permissionIDMappings = map[string]string{}
)
ctx = clues.Add(
@ -60,10 +64,7 @@ func RestoreCollections(
return dcs[i].FullPath().String() < dcs[j].FullPath().String()
})
var (
el = errs.Local()
parentMetas = map[string]Metadata{}
)
el := errs.Local()
// Iterate through the data collections and restore the contents of each
for _, dc := range dcs {
@ -80,13 +81,14 @@ func RestoreCollections(
"path", dc.FullPath())
)
metrics, folderMetas, err = RestoreCollection(
metrics, err = RestoreCollection(
ictx,
creds,
backupVersion,
service,
dc,
parentMetas,
folderMetas,
permissionIDMappings,
OneDriveSource,
dest.ContainerName,
deets,
@ -96,10 +98,6 @@ func RestoreCollections(
el.AddRecoverable(err)
}
for k, v := range folderMetas {
parentMetas[k] = v
}
restoreMetrics = support.CombineMetrics(restoreMetrics, metrics)
if errors.Is(err, context.Canceled) {
@ -128,19 +126,19 @@ func RestoreCollection(
backupVersion int,
service graph.Servicer,
dc data.RestoreCollection,
parentMetas map[string]Metadata,
folderMetas map[string]Metadata,
permissionIDMappings map[string]string,
source driveSource,
restoreContainerName string,
deets *details.Builder,
restorePerms bool,
errs *fault.Bus,
) (support.CollectionMetrics, map[string]Metadata, error) {
) (support.CollectionMetrics, error) {
var (
metrics = support.CollectionMetrics{}
copyBuffer = make([]byte, copyBufferSize)
directory = dc.FullPath()
folderMetas = map[string]Metadata{}
el = errs.Local()
metrics = support.CollectionMetrics{}
copyBuffer = make([]byte, copyBufferSize)
directory = dc.FullPath()
el = errs.Local()
)
ctx, end := diagnostics.Span(ctx, "gc:oneDrive:restoreCollection", diagnostics.Label("path", directory))
@ -148,7 +146,7 @@ func RestoreCollection(
drivePath, err := path.ToOneDrivePath(directory)
if err != nil {
return metrics, folderMetas, clues.Wrap(err, "creating drive path").WithClues(ctx)
return metrics, clues.Wrap(err, "creating drive path").WithClues(ctx)
}
// Assemble folder hierarchy we're going to restore into (we recreate the folder hierarchy
@ -171,11 +169,11 @@ func RestoreCollection(
ctx,
drivePath,
dc,
parentMetas,
folderMetas,
backupVersion,
restorePerms)
if err != nil {
return metrics, folderMetas, clues.Wrap(err, "getting permissions").WithClues(ctx)
return metrics, clues.Wrap(err, "getting permissions").WithClues(ctx)
}
// Create restore folders and get the folder ID of the folder the data stream will be restored in
@ -185,11 +183,16 @@ func RestoreCollection(
service,
drivePath,
restoreFolderElements,
colMeta)
dc.FullPath(),
colMeta,
folderMetas,
permissionIDMappings,
restorePerms)
if err != nil {
return metrics, folderMetas, clues.Wrap(err, "creating folders for restore")
return metrics, clues.Wrap(err, "creating folders for restore")
}
folderMetas[dc.FullPath().String()] = colMeta
items := dc.Items(ctx, errs)
for {
@ -199,11 +202,11 @@ func RestoreCollection(
select {
case <-ctx.Done():
return metrics, folderMetas, err
return metrics, err
case itemData, ok := <-items:
if !ok {
return metrics, folderMetas, nil
return metrics, nil
}
itemPath, err := dc.FullPath().Append(itemData.UUID(), true)
@ -223,6 +226,7 @@ func RestoreCollection(
restoreFolderID,
copyBuffer,
folderMetas,
permissionIDMappings,
restorePerms,
itemData,
itemPath)
@ -259,7 +263,7 @@ func RestoreCollection(
}
}
return metrics, folderMetas, el.Failure()
return metrics, el.Failure()
}
// restores an item, according to correct backup version behavior.
@ -275,6 +279,7 @@ func restoreItem(
restoreFolderID string,
copyBuffer []byte,
folderMetas map[string]Metadata,
permissionIDMappings map[string]string,
restorePerms bool,
itemData data.Stream,
itemPath path.Path,
@ -341,6 +346,9 @@ func restoreItem(
restoreFolderID,
copyBuffer,
restorePerms,
folderMetas,
permissionIDMappings,
itemPath,
itemData)
if err != nil {
return details.ItemInfo{}, false, clues.Wrap(err, "v1 restore")
@ -361,6 +369,9 @@ func restoreItem(
restoreFolderID,
copyBuffer,
restorePerms,
folderMetas,
permissionIDMappings,
itemPath,
itemData)
if err != nil {
return details.ItemInfo{}, false, clues.Wrap(err, "v6 restore")
@ -408,6 +419,9 @@ func restoreV1File(
restoreFolderID string,
copyBuffer []byte,
restorePerms bool,
folderMetas map[string]Metadata,
permissionIDMappings map[string]string,
itemPath path.Path,
itemData data.Stream,
) (details.ItemInfo, error) {
trimmedName := strings.TrimSuffix(itemData.UUID(), DataFileSuffix)
@ -445,7 +459,10 @@ func restoreV1File(
service,
drivePath.DriveID,
itemID,
meta)
itemPath,
meta,
folderMetas,
permissionIDMappings)
if err != nil {
return details.ItemInfo{}, clues.Wrap(err, "restoring item permissions")
}
@ -463,6 +480,9 @@ func restoreV6File(
restoreFolderID string,
copyBuffer []byte,
restorePerms bool,
folderMetas map[string]Metadata,
permissionIDMappings map[string]string,
itemPath path.Path,
itemData data.Stream,
) (details.ItemInfo, error) {
trimmedName := strings.TrimSuffix(itemData.UUID(), DataFileSuffix)
@ -511,7 +531,10 @@ func restoreV6File(
service,
drivePath.DriveID,
itemID,
meta)
itemPath,
meta,
folderMetas,
permissionIDMappings)
if err != nil {
return details.ItemInfo{}, clues.Wrap(err, "restoring item permissions")
}

3
src/internal/connector/sharepoint/restore.go
View File

@ -67,13 +67,14 @@ func RestoreCollections(
switch dc.FullPath().Category() {
case path.LibrariesCategory:
metrics, _, err = onedrive.RestoreCollection(
metrics, err = onedrive.RestoreCollection(
ictx,
creds,
backupVersion,
service,
dc,
map[string]onedrive.Metadata{}, // Currently permission data is not stored for sharepoint
map[string]string{},
onedrive.SharePointSource,
dest.ContainerName,
deets,

57
src/internal/operations/backup_integration_test.go
View File

@ -1264,6 +1264,13 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() {
var (
newFile models.DriveItemable
newFileName = "new_file.txt"
permissionIDMappings = map[string]string{}
writePerm = onedrive.UserPermission{
ID: "perm-id",
Roles: []string{"write"},
EntityID: suite.user,
}
)
// Although established as a table, these tests are not isolated from each other.
@ -1307,21 +1314,16 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() {
driveItem := models.NewDriveItem()
driveItem.SetName(&newFileName)
driveItem.SetFile(models.NewFile())
err = onedrive.RestorePermissions(
err = onedrive.UpdatePermissions(
ctx,
creds,
gc.Service,
driveID,
*newFile.GetId(),
onedrive.Metadata{
SharingMode: onedrive.SharingModeCustom,
Permissions: []onedrive.UserPermission{
{
Roles: []string{"write"},
EntityID: suite.user,
},
},
})
[]onedrive.UserPermission{writePerm},
[]onedrive.UserPermission{},
permissionIDMappings,
)
require.NoErrorf(t, err, "add permission to file %v", clues.ToCore(err))
},
itemsRead: 1, // .data file for newitem
@ -1333,16 +1335,16 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() {
driveItem := models.NewDriveItem()
driveItem.SetName(&newFileName)
driveItem.SetFile(models.NewFile())
err = onedrive.RestorePermissions(
err = onedrive.UpdatePermissions(
ctx,
creds,
gc.Service,
driveID,
*newFile.GetId(),
onedrive.Metadata{
SharingMode: onedrive.SharingModeCustom,
Permissions: []onedrive.UserPermission{},
})
[]onedrive.UserPermission{},
[]onedrive.UserPermission{writePerm},
permissionIDMappings,
)
require.NoError(t, err, "add permission to file", clues.ToCore(err))
},
itemsRead: 1, // .data file for newitem
@ -1355,21 +1357,16 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() {
driveItem := models.NewDriveItem()
driveItem.SetName(&newFileName)
driveItem.SetFile(models.NewFile())
err = onedrive.RestorePermissions(
err = onedrive.UpdatePermissions(
ctx,
creds,
gc.Service,
driveID,
targetContainer,
onedrive.Metadata{
SharingMode: onedrive.SharingModeCustom,
Permissions: []onedrive.UserPermission{
{
Roles: []string{"write"},
EntityID: suite.user,
},
},
})
[]onedrive.UserPermission{writePerm},
[]onedrive.UserPermission{},
permissionIDMappings,
)
require.NoError(t, err, "add permission to file", clues.ToCore(err))
},
itemsRead: 0,
@ -1382,16 +1379,16 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() {
driveItem := models.NewDriveItem()
driveItem.SetName(&newFileName)
driveItem.SetFile(models.NewFile())
err = onedrive.RestorePermissions(
err = onedrive.UpdatePermissions(
ctx,
creds,
gc.Service,
driveID,
targetContainer,
onedrive.Metadata{
SharingMode: onedrive.SharingModeCustom,
Permissions: []onedrive.UserPermission{},
})
[]onedrive.UserPermission{},
[]onedrive.UserPermission{writePerm},
permissionIDMappings,
)
require.NoError(t, err, "add permission to file", clues.ToCore(err))
},
itemsRead: 0,