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

Improve handling of permissions using sharing field (#2705)

Browse Source 
Permissions are only stored if SharingMode is custom( "sharing" field is present). This will help with delta incrementals as well.

---

#### Does this PR need a docs update or release note?

- [ ] ✅ Yes, it's included
- [x] 🕐 Yes, but in a later PR
- [ ] ⛔ No

#### Type of change

<!--- Please check the type of change your PR introduces: --->
- [x] 🌻 Feature
- [ ] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Test
- [ ] 💻 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. -->
* closes https://github.com/alcionai/corso/issues/2459

#### Test Plan

<!-- How will this be tested prior to merging.-->
- [ ] 💪 Manual
- [ ] ⚡ Unit test
- [ ] 💚 E2E
This commit is contained in:
Abin Simon 2023-03-09 12:08:13 +05:30 committed by GitHub
parent f28da24153
commit 6fd29097e3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 307 additions and 122 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -25,8 +25,12 @@ import (
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}
if len(perm.user) == 0 || len(perm.roles) == 0 ||
perm.sharingMode != onedrive.SharingModeCustom {
return onedrive.Metadata{
FileName: fileName,
SharingMode: perm.sharingMode,
}
}
id := base64.StdEncoding.EncodeToString([]byte(perm.user + strings.Join(perm.roles, "+")))
@ -262,9 +266,10 @@ func (c *onedriveCollection) withPermissions(perm permData) *onedriveCollection
}
type permData struct {
user string // user is only for older versions
entityID string
roles []string
user string // user is only for older versions
entityID string
roles []string
sharingMode onedrive.SharingMode
}
type itemData struct {
@ -877,3 +882,159 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndNo
},
)
}
// This is similar to TestPermissionsRestoreAndBackup but tests purely
// for inheritance and that too only with newer versions
func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsInheritanceRestoreAndBackup() {
ctx, flush := tester.NewContext()
defer flush()
// Get the default drive ID for the test user.
driveID := mustGetDefaultDriveID(
suite.T(),
ctx,
suite.connector.Service,
suite.user,
)
folderAName := "custom"
folderBName := "inherited"
rootPath := []string{
"drives",
driveID,
"root:",
}
folderAPath := []string{
"drives",
driveID,
"root:",
folderAName,
}
subfolderAPath := []string{
"drives",
driveID,
"root:",
folderAName,
folderAName,
}
subfolderBPath := []string{
"drives",
driveID,
"root:",
folderAName,
folderBName,
}
fileSet := []itemData{
{
name: "file-custom",
data: fileAData,
perms: permData{
user: suite.secondaryUser,
entityID: suite.secondaryUserID,
roles: writePerm,
sharingMode: onedrive.SharingModeCustom,
},
},
{
name: "file-inherited",
data: fileAData,
perms: permData{
sharingMode: onedrive.SharingModeInherited,
},
},
}
// Here is what this test is testing
// - custom-permission-folder
// - custom-permission-file
// - inherted-permission-file
// - custom-permission-folder
// - custom-permission-file
// - inherted-permission-file
// - inherted-permission-folder
// - custom-permission-file
// - inherted-permission-file
// No reason why it couldn't work with previous versions, but this
// is when it got introduced.
startVersion := version.OneDrive4DirIncludesPermissions
test := onedriveTest{
startVersion: startVersion,
cols: []onedriveColInfo{
{
pathElements: rootPath,
files: []itemData{},
folders: []itemData{
{
name: folderAName,
},
},
},
{
pathElements: folderAPath,
files: fileSet,
folders: []itemData{
{name: folderAName},
{name: folderBName},
},
perms: permData{
user: suite.secondaryUser,
entityID: suite.secondaryUserID,
roles: readPerm,
},
},
{
pathElements: subfolderAPath,
files: fileSet,
perms: permData{
user: suite.secondaryUser,
entityID: suite.secondaryUserID,
roles: writePerm,
sharingMode: onedrive.SharingModeCustom,
},
},
{
pathElements: subfolderBPath,
files: fileSet,
perms: permData{
sharingMode: onedrive.SharingModeInherited,
},
},
},
}
expected := testDataForInfo(suite.T(), test.cols, version.Backup)
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{
service: path.OneDriveService,
resource: Users,
backupVersion: vn,
collectionsPrevious: input,
collectionsLatest: expected,
}
runRestoreBackupTestVersions(
t,
suite.acct,
testData,
suite.connector.tenant,
[]string{suite.user},
control.Options{
RestorePermissions: true,
ToggleFeatures: control.Toggles{EnablePermissionsBackup: true},
},
)
})
}
}

13
src/internal/connector/onedrive/api/drive.go
View File

@ -3,8 +3,10 @@ package api
import (
"context"
"fmt"
"strings"
"github.com/alcionai/clues"
abstractions "github.com/microsoft/kiota-abstractions-go"
msdrives "github.com/microsoftgraph/msgraph-sdk-go/drives"
"github.com/microsoftgraph/msgraph-sdk-go/models"
mssites "github.com/microsoftgraph/msgraph-sdk-go/sites"
@ -40,7 +42,18 @@ func NewItemPager(
fields []string,
) *driveItemPager {
pageCount := pageSize
headers := abstractions.NewRequestHeaders()
preferHeaderItems := []string{
"deltashowremovedasdeleted",
"deltatraversepermissiongaps",
"deltashowsharingchanges",
"hierarchicalsharing",
}
headers.Add("Prefer", strings.Join(preferHeaderItems, ","))
requestConfig := &msdrives.ItemRootDeltaRequestBuilderGetRequestConfiguration{
Headers: headers,
QueryParameters: &msdrives.ItemRootDeltaRequestBuilderGetQueryParameters{
Top: &pageCount,
Select: fields,

13
src/internal/connector/onedrive/collection.go
View File

@ -53,6 +53,13 @@ var (
_ data.StreamModTime = &metadataItem{}
)
type SharingMode int
const (
SharingModeCustom = SharingMode(iota)
SharingModeInherited
)
// Collection represents a set of OneDrive objects retrieved from M365
type Collection struct {
// configured to handle large item downloads
@ -213,7 +220,11 @@ type UserPermission struct {
// ItemMeta contains metadata about the Item. It gets stored in a
// separate file in kopia
type Metadata struct {
FileName string `json:"filename,omitempty"`
FileName string `json:"filename,omitempty"`
// SharingMode denotes what the current mode of sharing is for the object.
// - inherited: permissions same as parent permissions (no "shared" in delta)
// - custom: use Permissions to set correct permissions ("shared" has value in delta)
SharingMode SharingMode `json:"permissionMode,omitempty"`
Permissions []UserPermission `json:"permissions,omitempty"`
}

8
src/internal/connector/onedrive/collections.go
View File

@ -622,6 +622,14 @@ func (c *Collections) UpdateCollections(
isFolder = item.GetFolder() != nil || item.GetPackage() != nil
)
// TODO(meain): Use `@microsoft.graph.sharedChanged` in
// item.GetAdditionalData() to optimize fetching
// permissions. When permissions change, this flags is
// present, if only data changes, it is not present. Have to
// add `oneDrive.sharedChanged` in `$select` in delta
// https://learn.microsoft.com/en-us/onedrive/developer/rest-api
// /concepts/scan-guidance#scanning-permissions-hierarchies
// Deleted file or folder.
if item.GetDeleted() != nil {
if err := c.handleDelete(

1
src/internal/connector/onedrive/drive.go
View File

@ -173,6 +173,7 @@ func defaultItemPager(
"size",
"deleted",
"malware",
"shared",
},
)
}

44
src/internal/connector/onedrive/item.go
View File

@ -73,14 +73,27 @@ func oneDriveItemMetaReader(
FileName: *item.GetName(),
}
perms, err := oneDriveItemPermissionInfo(ctx, service, driveID, item, fetchPermissions)
if err != nil {
// Keep this in an if-block because if it's not then we have a weird issue
// of having no value in error but golang thinking it's non nil because of
// the way interfaces work.
err = clues.Wrap(err, "fetching item permissions")
if item.GetShared() == nil {
meta.SharingMode = SharingModeInherited
} else {
meta.Permissions = perms
meta.SharingMode = SharingModeCustom
}
var (
perms []UserPermission
err error
)
if meta.SharingMode == SharingModeCustom && fetchPermissions {
perms, err = oneDriveItemPermissionInfo(ctx, service, driveID, ptr.Val(item.GetId()))
if err != nil {
// Keep this in an if-block because if it's not then we have a weird issue
// of having no value in error but golang thinking it's non nil because of
// the way interfaces work.
err = clues.Wrap(err, "fetching item permissions")
} else {
meta.Permissions = perms
}
}
metaJSON, serializeErr := json.Marshal(meta)
@ -219,29 +232,22 @@ func oneDriveItemInfo(di models.DriveItemable, itemSize int64) *details.OneDrive
}
}
// oneDriveItemPermissionInfo will fetch the permission information for a drive
// item.
// OneDriveItemPermissionInfo will fetch the permission information
// for a drive item given a drive and item id.
func oneDriveItemPermissionInfo(
ctx context.Context,
service graph.Servicer,
driveID string,
di models.DriveItemable,
fetchPermissions bool,
itemID string,
) ([]UserPermission, error) {
if !fetchPermissions {
return nil, nil
}
id := ptr.Val(di.GetId())
perm, err := service.
Client().
DrivesById(driveID).
ItemsById(id).
ItemsById(itemID).
Permissions().
Get(ctx, nil)
if err != nil {
return nil, graph.Wrap(ctx, err, "getting item metadata").With("item_id", id)
return nil, graph.Wrap(ctx, err, "getting item metadata").With("item_id", itemID)
}
uperms := filterUserPermissions(ctx, perm.GetValue())

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

@ -14,80 +14,70 @@ import (
"github.com/alcionai/corso/src/pkg/path"
)
func getParentPermissions(
func getParentMetadata(
parentPath path.Path,
parentPermissions map[string][]UserPermission,
) ([]UserPermission, error) {
parentPerms, ok := parentPermissions[parentPath.String()]
metas map[string]Metadata,
) (Metadata, error) {
parentMeta, ok := metas[parentPath.String()]
if !ok {
onedrivePath, err := path.ToOneDrivePath(parentPath)
if err != nil {
return nil, errors.Wrap(err, "invalid restore path")
return Metadata{}, errors.Wrap(err, "invalid restore path")
}
if len(onedrivePath.Folders) != 0 {
return nil, errors.Wrap(err, "computing item permissions")
return Metadata{}, errors.Wrap(err, "computing item permissions")
}
parentPerms = []UserPermission{}
parentMeta = Metadata{}
}
return parentPerms, nil
return parentMeta, nil
}
func getParentAndCollectionPermissions(
func getCollectionMetadata(
ctx context.Context,
drivePath *path.DrivePath,
dc data.RestoreCollection,
permissions map[string][]UserPermission,
metas map[string]Metadata,
backupVersion int,
restorePerms bool,
) ([]UserPermission, []UserPermission, error) {
) (Metadata, error) {
if !restorePerms || backupVersion < version.OneDrive1DataAndMetaFiles {
return nil, nil, nil
return Metadata{}, nil
}
var (
err error
parentPerms []UserPermission
colPerms []UserPermission
collectionPath = dc.FullPath()
)
// Only get parent permissions if we're not restoring the root.
if len(drivePath.Folders) > 0 {
parentPath, err := collectionPath.Dir()
if err != nil {
return nil, nil, clues.Wrap(err, "getting parent path")
}
parentPerms, err = getParentPermissions(parentPath, permissions)
if err != nil {
return nil, nil, clues.Wrap(err, "getting parent permissions")
}
if len(drivePath.Folders) == 0 {
// No permissions for root folder
return Metadata{}, nil
}
if backupVersion < version.OneDrive4DirIncludesPermissions {
colPerms, err = getParentPermissions(collectionPath, permissions)
colMeta, err := getParentMetadata(collectionPath, metas)
if err != nil {
return nil, nil, clues.Wrap(err, "getting collection permissions")
}
} else if len(drivePath.Folders) > 0 {
// Root folder doesn't have a metadata file associated with it.
folders := collectionPath.Folders()
meta, err := fetchAndReadMetadata(
ctx,
dc,
folders[len(folders)-1]+DirMetaFileSuffix)
if err != nil {
return nil, nil, clues.Wrap(err, "collection permissions")
return Metadata{}, clues.Wrap(err, "collection metadata")
}
colPerms = meta.Permissions
return colMeta, nil
}
return parentPerms, colPerms, nil
// Root folder doesn't have a metadata file associated with it.
folders := collectionPath.Folders()
meta, err := fetchAndReadMetadata(
ctx,
dc,
folders[len(folders)-1]+DirMetaFileSuffix)
if err != nil {
return Metadata{}, clues.Wrap(err, "collection metadata")
}
return meta, nil
}
// createRestoreFoldersWithPermissions creates the restore folder hierarchy in
@ -99,8 +89,7 @@ func createRestoreFoldersWithPermissions(
service graph.Servicer,
driveID string,
restoreFolders []string,
parentPermissions []UserPermission,
folderPermissions []UserPermission,
folderMetadata Metadata,
permissionIDMappings map[string]string,
) (string, error) {
id, err := CreateRestoreFolders(ctx, service, driveID, restoreFolders)
@ -113,58 +102,52 @@ func createRestoreFoldersWithPermissions(
service,
driveID,
id,
parentPermissions,
folderPermissions,
folderMetadata,
permissionIDMappings)
return id, err
}
// getChildPermissions is to filter out permissions present in the
// parent from the ones that are available for child. This is
// necessary as we store the nested permissions in the child. We
// cannot avoid storing the nested permissions as it is possible that
// a file in a folder can remove the nested permission that is present
// on itself.
func getChildPermissions(
childPermissions, parentPermissions []UserPermission,
func diffPermissions(
before, after []UserPermission,
permissionIDMappings map[string]string,
) ([]UserPermission, []UserPermission) {
var (
addedPermissions = []UserPermission{}
removedPermissions = []UserPermission{}
added = []UserPermission{}
removed = []UserPermission{}
)
for _, cp := range childPermissions {
for _, cp := range after {
found := false
for _, pp := range parentPermissions {
if cp.ID == pp.ID {
for _, pp := range before {
if cp.ID == permissionIDMappings[pp.ID] {
found = true
break
}
}
if !found {
addedPermissions = append(addedPermissions, cp)
added = append(added, cp)
}
}
for _, pp := range parentPermissions {
for _, pp := range before {
found := false
for _, cp := range childPermissions {
if pp.ID == cp.ID {
for _, cp := range after {
if permissionIDMappings[pp.ID] == cp.ID {
found = true
break
}
}
if !found {
removedPermissions = append(removedPermissions, pp)
removed = append(removed, pp)
}
}
return addedPermissions, removedPermissions
return added, removed
}
// restorePermissions takes in the permissions that were added and the
@ -175,19 +158,28 @@ func restorePermissions(
service graph.Servicer,
driveID string,
itemID string,
parentPerms []UserPermission,
childPerms []UserPermission,
meta Metadata,
permissionIDMappings map[string]string,
) error {
permAdded, permRemoved := getChildPermissions(childPerms, parentPerms)
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 := oneDriveItemPermissionInfo(ctx, service, driveID, itemID)
if err != nil {
return graph.Wrap(ctx, err, "fetching current permissions")
}
permAdded, permRemoved := diffPermissions(currentPermissions, meta.Permissions, permissionIDMappings)
for _, p := range permRemoved {
err := service.Client().
DrivesById(driveID).
ItemsById(itemID).
PermissionsById(permissionIDMappings[p.ID]).
PermissionsById(p.ID).
Delete(ctx, nil)
if err != nil {
return graph.Wrap(ctx, err, "removing permissions")

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

@ -44,7 +44,7 @@ func RestoreCollections(
var (
restoreMetrics support.CollectionMetrics
metrics support.CollectionMetrics
folderPerms map[string][]UserPermission
folderMetas map[string]Metadata
// permissionIDMappings is used to map between old and new id
// of permissions as we restore them
@ -63,8 +63,8 @@ func RestoreCollections(
})
var (
el = errs.Local()
parentPermissions = map[string][]UserPermission{}
el = errs.Local()
parentMetas = map[string]Metadata{}
)
// Iterate through the data collections and restore the contents of each
@ -82,12 +82,12 @@ func RestoreCollections(
"path", dc.FullPath()) // TODO: pii
)
metrics, folderPerms, permissionIDMappings, err = RestoreCollection(
metrics, folderMetas, permissionIDMappings, err = RestoreCollection(
ictx,
backupVersion,
service,
dc,
parentPermissions,
parentMetas,
OneDriveSource,
dest.ContainerName,
deets,
@ -98,8 +98,8 @@ func RestoreCollections(
el.AddRecoverable(err)
}
for k, v := range folderPerms {
parentPermissions[k] = v
for k, v := range folderMetas {
parentMetas[k] = v
}
restoreMetrics = support.CombineMetrics(restoreMetrics, metrics)
@ -128,20 +128,20 @@ func RestoreCollection(
backupVersion int,
service graph.Servicer,
dc data.RestoreCollection,
parentPermissions map[string][]UserPermission,
parentMetas map[string]Metadata,
source driveSource,
restoreContainerName string,
deets *details.Builder,
permissionIDMappings map[string]string,
restorePerms bool,
errs *fault.Bus,
) (support.CollectionMetrics, map[string][]UserPermission, map[string]string, error) {
) (support.CollectionMetrics, map[string]Metadata, map[string]string, error) {
var (
metrics = support.CollectionMetrics{}
copyBuffer = make([]byte, copyBufferSize)
directory = dc.FullPath()
itemInfo details.ItemInfo
folderPerms = map[string][]UserPermission{}
folderMetas = map[string]Metadata{}
)
ctx, end := D.Span(ctx, "gc:oneDrive:restoreCollection", D.Label("path", directory))
@ -149,7 +149,7 @@ func RestoreCollection(
drivePath, err := path.ToOneDrivePath(directory)
if err != nil {
return metrics, folderPerms, permissionIDMappings, clues.Wrap(err, "creating drive path").WithClues(ctx)
return metrics, folderMetas, permissionIDMappings, clues.Wrap(err, "creating drive path").WithClues(ctx)
}
// Assemble folder hierarchy we're going to restore into (we recreate the folder hierarchy
@ -168,15 +168,15 @@ func RestoreCollection(
trace.Log(ctx, "gc:oneDrive:restoreCollection", directory.String())
logger.Ctx(ctx).Info("restoring onedrive collection")
parentPerms, colPerms, err := getParentAndCollectionPermissions(
colMeta, err := getCollectionMetadata(
ctx,
drivePath,
dc,
parentPermissions,
parentMetas,
backupVersion,
restorePerms)
if err != nil {
return metrics, folderPerms, permissionIDMappings, clues.Wrap(err, "getting permissions").WithClues(ctx)
return metrics, folderMetas, permissionIDMappings, 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,12 +185,11 @@ func RestoreCollection(
service,
drivePath.DriveID,
restoreFolderElements,
parentPerms,
colPerms,
colMeta,
permissionIDMappings,
)
if err != nil {
return metrics, folderPerms, permissionIDMappings, clues.Wrap(err, "creating folders for restore")
return metrics, folderMetas, permissionIDMappings, clues.Wrap(err, "creating folders for restore")
}
var (
@ -205,11 +204,11 @@ func RestoreCollection(
select {
case <-ctx.Done():
return metrics, folderPerms, permissionIDMappings, err
return metrics, folderMetas, permissionIDMappings, err
case itemData, ok := <-items:
if !ok {
return metrics, folderPerms, permissionIDMappings, nil
return metrics, folderMetas, permissionIDMappings, nil
}
itemPath, err := dc.FullPath().Append(itemData.UUID(), true)
@ -239,7 +238,6 @@ func RestoreCollection(
dc,
restoreFolderID,
copyBuffer,
colPerms,
permissionIDMappings,
restorePerms,
itemData,
@ -253,7 +251,6 @@ func RestoreCollection(
dc,
restoreFolderID,
copyBuffer,
colPerms,
permissionIDMappings,
restorePerms,
itemData,
@ -296,7 +293,7 @@ func RestoreCollection(
}
trimmedPath := strings.TrimSuffix(itemPath.String(), DirMetaFileSuffix)
folderPerms[trimmedPath] = meta.Permissions
folderMetas[trimmedPath] = meta
}
} else {
@ -330,7 +327,7 @@ func RestoreCollection(
}
}
return metrics, folderPerms, permissionIDMappings, el.Failure()
return metrics, folderMetas, permissionIDMappings, el.Failure()
}
type fileFetcher interface {
@ -345,7 +342,6 @@ func restoreV1File(
fetcher fileFetcher,
restoreFolderID string,
copyBuffer []byte,
parentPerms []UserPermission,
permissionIDMappings map[string]string,
restorePerms bool,
itemData data.Stream,
@ -384,8 +380,7 @@ func restoreV1File(
service,
drivePath.DriveID,
itemID,
parentPerms,
meta.Permissions,
meta,
permissionIDMappings,
)
if err != nil {
@ -403,7 +398,6 @@ func restoreV2File(
fetcher fileFetcher,
restoreFolderID string,
copyBuffer []byte,
parentPerms []UserPermission,
permissionIDMappings map[string]string,
restorePerms bool,
itemData data.Stream,
@ -453,8 +447,7 @@ func restoreV2File(
service,
drivePath.DriveID,
itemID,
parentPerms,
meta.Permissions,
meta,
permissionIDMappings,
)
if err != nil {

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

@ -71,7 +71,7 @@ func RestoreCollections(
backupVersion,
service,
dc,
map[string][]onedrive.UserPermission{}, // Currently permission data is not stored for sharepoint
map[string]onedrive.Metadata{}, // Currently permission data is not stored for sharepoint
onedrive.SharePointSource,
dest.ContainerName,
deets,