onedrive: fix --metadata-mapper called twice if writing permissions

Before this change, the --metadata-mapper was called twice if an object was
uploaded via multipart upload with --metadata and --onedrive-metadata-permissions
"write" or "read,write". This change fixes the issue.
This commit is contained in:
nielash 2024-03-28 13:00:43 -04:00 committed by Nick Craig-Wood
parent 93c960df59
commit 998df26ceb
5 changed files with 68 additions and 61 deletions

View File

@ -613,7 +613,7 @@ func (o *Object) tryGetBtime(modTime time.Time) time.Time {
} }
// adds metadata (except permissions) if --metadata is in use // adds metadata (except permissions) if --metadata is in use
func (o *Object) fetchMetadataForCreate(ctx context.Context, src fs.ObjectInfo, options []fs.OpenOption, modTime time.Time) (createRequest api.CreateUploadRequest, err error) { func (o *Object) fetchMetadataForCreate(ctx context.Context, src fs.ObjectInfo, options []fs.OpenOption, modTime time.Time) (createRequest api.CreateUploadRequest, metadata fs.Metadata, err error) {
createRequest = api.CreateUploadRequest{ // we set mtime no matter what createRequest = api.CreateUploadRequest{ // we set mtime no matter what
Item: api.Metadata{ Item: api.Metadata{
FileSystemInfo: &api.FileSystemInfoFacet{ FileSystemInfo: &api.FileSystemInfoFacet{
@ -625,10 +625,10 @@ func (o *Object) fetchMetadataForCreate(ctx context.Context, src fs.ObjectInfo,
meta, err := fs.GetMetadataOptions(ctx, o.fs, src, options) meta, err := fs.GetMetadataOptions(ctx, o.fs, src, options)
if err != nil { if err != nil {
return createRequest, fmt.Errorf("failed to read metadata from source object: %w", err) return createRequest, nil, fmt.Errorf("failed to read metadata from source object: %w", err)
} }
if meta == nil { if meta == nil {
return createRequest, nil // no metadata or --metadata not in use, so just return mtime return createRequest, nil, nil // no metadata or --metadata not in use, so just return mtime
} }
if o.meta == nil { if o.meta == nil {
o.meta = o.fs.newMetadata(o.Remote()) o.meta = o.fs.newMetadata(o.Remote())
@ -636,13 +636,13 @@ func (o *Object) fetchMetadataForCreate(ctx context.Context, src fs.ObjectInfo,
o.meta.mtime = modTime o.meta.mtime = modTime
numSet, err := o.meta.Set(ctx, meta) numSet, err := o.meta.Set(ctx, meta)
if err != nil { if err != nil {
return createRequest, err return createRequest, meta, err
} }
if numSet == 0 { if numSet == 0 {
return createRequest, nil return createRequest, meta, nil
} }
createRequest.Item = o.meta.toAPIMetadata() createRequest.Item = o.meta.toAPIMetadata()
return createRequest, nil return createRequest, meta, nil
} }
// Fetch metadata and update updateInfo if --metadata is in use // Fetch metadata and update updateInfo if --metadata is in use
@ -665,27 +665,6 @@ func (f *Fs) fetchAndUpdateMetadata(ctx context.Context, src fs.ObjectInfo, opti
return newInfo, err return newInfo, err
} }
// Fetch and update permissions if --metadata is in use
// This is similar to fetchAndUpdateMetadata, except it does NOT set modtime or other metadata if there are no permissions to set.
// This is intended for cases where metadata may already have been set during upload and an extra step is needed only for permissions.
func (f *Fs) fetchAndUpdatePermissions(ctx context.Context, src fs.ObjectInfo, options []fs.OpenOption, updateInfo *Object) (info *api.Item, err error) {
meta, err := fs.GetMetadataOptions(ctx, f, src, options)
if err != nil {
return nil, fmt.Errorf("failed to read metadata from source object: %w", err)
}
if meta == nil || !f.needsUpdatePermissions(meta) {
return nil, nil // no metadata, --metadata not in use, or wrong flags
}
if updateInfo.meta == nil {
updateInfo.meta = f.newMetadata(updateInfo.Remote())
}
newInfo, err := updateInfo.updateMetadata(ctx, meta)
if newInfo == nil {
return info, err
}
return newInfo, err
}
// updateMetadata calls Get, Set, and Write // updateMetadata calls Get, Set, and Write
func (o *Object) updateMetadata(ctx context.Context, meta fs.Metadata) (info *api.Item, err error) { func (o *Object) updateMetadata(ctx context.Context, meta fs.Metadata) (info *api.Item, err error) {
_, err = o.meta.Get(ctx) // refresh permissions _, err = o.meta.Get(ctx) // refresh permissions

View File

@ -4,11 +4,11 @@ differences between OneDrive Personal and Business (see table below for
details). details).
Permissions are also supported, if `--onedrive-metadata-permissions` is set. The Permissions are also supported, if `--onedrive-metadata-permissions` is set. The
accepted values for `--onedrive-metadata-permissions` are `read`, `write`, accepted values for `--onedrive-metadata-permissions` are "`read`", "`write`",
`read,write`, and `off` (the default). `write` supports adding new permissions, "`read,write`", and "`off`" (the default). "`write`" supports adding new permissions,
updating the "role" of existing permissions, and removing permissions. Updating updating the "role" of existing permissions, and removing permissions. Updating
and removing require the Permission ID to be known, so it is recommended to use and removing require the Permission ID to be known, so it is recommended to use
`read,write` instead of `write` if you wish to update/remove permissions. "`read,write`" instead of "`write`" if you wish to update/remove permissions.
Permissions are read/written in JSON format using the same schema as the Permissions are read/written in JSON format using the same schema as the
[OneDrive API](https://learn.microsoft.com/en-us/onedrive/developer/rest-api/resources/permission?view=odsp-graph-online), [OneDrive API](https://learn.microsoft.com/en-us/onedrive/developer/rest-api/resources/permission?view=odsp-graph-online),
@ -92,31 +92,14 @@ an ObjectID can be provided in `User.ID`. At least one valid recipient must be
provided in order to add a permission for a user. Creating a Public Link is also provided in order to add a permission for a user. Creating a Public Link is also
supported, if `Link.Scope` is set to `"anonymous"`. supported, if `Link.Scope` is set to `"anonymous"`.
Example request to add a "read" permission: Example request to add a "read" permission with `--metadata-mapper`:
```json ```json
[ {
{ "Metadata": {
"id": "", "permissions": "[{\"grantedToIdentities\":[{\"user\":{\"id\":\"ryan@contoso.com\"}}],\"roles\":[\"read\"]}]"
"grantedTo": { }
"user": {}, }
"application": {},
"device": {}
},
"grantedToIdentities": [
{
"user": {
"id": "ryan@contoso.com"
},
"application": {},
"device": {}
}
],
"roles": [
"read"
]
}
]
``` ```
Note that adding a permission can fail if a conflicting permission already Note that adding a permission can fail if a conflicting permission already

View File

@ -2304,11 +2304,11 @@ func (o *Object) Open(ctx context.Context, options ...fs.OpenOption) (in io.Read
} }
// createUploadSession creates an upload session for the object // createUploadSession creates an upload session for the object
func (o *Object) createUploadSession(ctx context.Context, src fs.ObjectInfo, modTime time.Time) (response *api.CreateUploadResponse, err error) { func (o *Object) createUploadSession(ctx context.Context, src fs.ObjectInfo, modTime time.Time) (response *api.CreateUploadResponse, metadata fs.Metadata, err error) {
opts := o.fs.newOptsCallWithPath(ctx, o.remote, "POST", "/createUploadSession") opts := o.fs.newOptsCallWithPath(ctx, o.remote, "POST", "/createUploadSession")
createRequest, err := o.fetchMetadataForCreate(ctx, src, opts.Options, modTime) createRequest, metadata, err := o.fetchMetadataForCreate(ctx, src, opts.Options, modTime)
if err != nil { if err != nil {
return nil, err return nil, metadata, err
} }
var resp *http.Response var resp *http.Response
err = o.fs.pacer.Call(func() (bool, error) { err = o.fs.pacer.Call(func() (bool, error) {
@ -2321,7 +2321,7 @@ func (o *Object) createUploadSession(ctx context.Context, src fs.ObjectInfo, mod
} }
return shouldRetry(ctx, resp, err) return shouldRetry(ctx, resp, err)
}) })
return response, err return response, metadata, err
} }
// getPosition gets the current position in a multipart upload // getPosition gets the current position in a multipart upload
@ -2437,7 +2437,7 @@ func (o *Object) uploadMultipart(ctx context.Context, in io.Reader, src fs.Objec
// Create upload session // Create upload session
fs.Debugf(o, "Starting multipart upload") fs.Debugf(o, "Starting multipart upload")
session, err := o.createUploadSession(ctx, src, modTime) session, metadata, err := o.createUploadSession(ctx, src, modTime)
if err != nil { if err != nil {
return nil, err return nil, err
} }
@ -2474,10 +2474,10 @@ func (o *Object) uploadMultipart(ctx context.Context, in io.Reader, src fs.Objec
if err != nil { if err != nil {
return info, err return info, err
} }
if !o.fs.opt.MetadataPermissions.IsSet(rwWrite) { if metadata == nil || !o.fs.needsUpdatePermissions(metadata) {
return info, err return info, err
} }
info, err = o.fs.fetchAndUpdatePermissions(ctx, src, options, o) // for permissions, which can't be set during original upload info, err = o.updateMetadata(ctx, metadata) // for permissions, which can't be set during original upload
if info == nil { if info == nil {
return nil, err return nil, err
} }

View File

@ -323,6 +323,49 @@ func (f *Fs) TestServerSideCopyMove(t *testing.T, r *fstest.Run) {
f.compareMeta(t, expectedMeta, actualMeta, true) f.compareMeta(t, expectedMeta, actualMeta, true)
} }
// TestMetadataMapper tests adding permissions with the --metadata-mapper
func (f *Fs) TestMetadataMapper(t *testing.T, r *fstest.Run) {
// setup
ctx, ci := fs.AddConfig(ctx)
ci.Metadata = true
_ = f.opt.MetadataPermissions.Set("read,write")
file1 := r.WriteFile(randomFilename(), content, t2)
const blob = `{"Metadata":{"permissions":"[{\"grantedToIdentities\":[{\"user\":{\"id\":\"ryan@contoso.com\"}}],\"roles\":[\"read\"]}]"}}`
// Copy
ci.MetadataMapper = []string{"echo", blob}
require.NoError(t, ci.Dump.Set("mapper"))
obj1, err := r.Flocal.NewObject(ctx, file1.Path)
assert.NoError(t, err)
obj2, err := operations.Copy(ctx, f, nil, randomFilename(), obj1)
assert.NoError(t, err)
actualMeta, err := fs.GetMetadata(ctx, obj2)
assert.NoError(t, err)
actualP := unmarshalPerms(t, actualMeta["permissions"])
found := false
foundCount := 0
for _, p := range actualP {
for _, identity := range p.GrantedToIdentities {
if identity.User.DisplayName == testUserID {
assert.Equal(t, []api.Role{api.ReadRole}, p.Roles)
found = true
foundCount++
}
}
if f.driveType == driveTypePersonal {
if p.GrantedTo != nil && p.GrantedTo.User != (api.Identity{}) && p.GrantedTo.User.ID == testUserID { // shows up in a different place on biz vs. personal
assert.Equal(t, []api.Role{api.ReadRole}, p.Roles)
found = true
foundCount++
}
}
}
assert.True(t, found, fmt.Sprintf("no permission found with expected role (want: \n\n%v \n\ngot: \n\n%v\n\n)", blob, actualMeta))
assert.Equal(t, 1, foundCount, "expected to find exactly 1 match")
}
// helper function to put an object with metadata and permissions // helper function to put an object with metadata and permissions
func (f *Fs) putWithMeta(ctx context.Context, t *testing.T, file *fstest.Item, perms []*api.PermissionsType) (expectedMeta, actualMeta fs.Metadata) { func (f *Fs) putWithMeta(ctx context.Context, t *testing.T, file *fstest.Item, perms []*api.PermissionsType) (expectedMeta, actualMeta fs.Metadata) {
t.Helper() t.Helper()
@ -459,6 +502,8 @@ func (f *Fs) InternalTest(t *testing.T) {
testF, r = newTestF() testF, r = newTestF()
t.Run("TestServerSideCopyMove", func(t *testing.T) { testF.TestServerSideCopyMove(t, r) }) t.Run("TestServerSideCopyMove", func(t *testing.T) { testF.TestServerSideCopyMove(t, r) })
testF.resetTestDefaults(r) testF.resetTestDefaults(r)
t.Run("TestMetadataMapper", func(t *testing.T) { testF.TestMetadataMapper(t, r) })
testF.resetTestDefaults(r)
} }
var _ fstests.InternalTester = (*Fs)(nil) var _ fstests.InternalTester = (*Fs)(nil)

View File

@ -203,7 +203,7 @@ func PrettyPrint(in any, label string, level LogLevel) {
return return
} }
inBytes, err := json.MarshalIndent(in, "", "\t") inBytes, err := json.MarshalIndent(in, "", "\t")
if err != nil { if err != nil || string(inBytes) == "{}" || string(inBytes) == "[]" {
LogPrintf(level, label, "\n%+v\n", in) LogPrintf(level, label, "\n%+v\n", in)
return return
} }