diff --git a/backend/onedrive/metadata.go b/backend/onedrive/metadata.go index 12ba522f6..f62c07028 100644 --- a/backend/onedrive/metadata.go +++ b/backend/onedrive/metadata.go @@ -396,10 +396,57 @@ func (m *Metadata) WritePermissions(ctx context.Context) (err error) { return nil } +// Order the permissions so that any with users come first. +// +// This is to work around a quirk with Graph: +// +// 1. You are adding permissions for both a group and a user. +// 2. The user is a member of the group. +// 3. The permissions for the group and user are the same. +// 4. You are adding the group permission before the user permission. +// +// When all of the above are true, Graph indicates it has added the +// user permission, but it immediately drops it +// +// See: https://github.com/rclone/rclone/issues/8465 +func (m *Metadata) orderPermissions(xs []*api.PermissionsType) { + // Return true if identity has any user permissions + hasUserIdentity := func(identity *api.IdentitySet) bool { + if identity == nil { + return false + } + return identity.User.ID != "" || identity.User.DisplayName != "" || identity.User.Email != "" || identity.User.LoginName != "" + } + // Return true if p has any user permissions + hasUser := func(p *api.PermissionsType) bool { + if hasUserIdentity(p.GetGrantedTo(m.fs.driveType)) { + return true + } + for _, identity := range p.GetGrantedToIdentities(m.fs.driveType) { + if hasUserIdentity(identity) { + return true + } + } + return false + } + // Put Permissions with a user first, leaving unsorted otherwise + slices.SortStableFunc(xs, func(a, b *api.PermissionsType) int { + aHasUser := hasUser(a) + bHasUser := hasUser(b) + if aHasUser && !bHasUser { + return -1 + } else if !aHasUser && bHasUser { + return 1 + } + return 0 + }) +} + // sortPermissions sorts the permissions (to be written) into add, update, and remove queues func (m *Metadata) sortPermissions() (add, update, remove []*api.PermissionsType) { new, old := m.queuedPermissions, m.permissions if len(old) == 0 || m.permsAddOnly { + m.orderPermissions(new) return new, nil, nil // they must all be "add" } @@ -447,6 +494,9 @@ func (m *Metadata) sortPermissions() (add, update, remove []*api.PermissionsType remove = append(remove, o) } } + m.orderPermissions(add) + m.orderPermissions(update) + m.orderPermissions(remove) return add, update, remove } diff --git a/backend/onedrive/metadata_test.go b/backend/onedrive/metadata_test.go new file mode 100644 index 000000000..bc7415233 --- /dev/null +++ b/backend/onedrive/metadata_test.go @@ -0,0 +1,125 @@ +package onedrive + +import ( + "encoding/json" + "testing" + + "github.com/rclone/rclone/backend/onedrive/api" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestOrderPermissions(t *testing.T) { + tests := []struct { + name string + input []*api.PermissionsType + expected []string + }{ + { + name: "empty", + input: []*api.PermissionsType{}, + expected: []string(nil), + }, + { + name: "users first, then group, then none", + input: []*api.PermissionsType{ + {ID: "1", GrantedTo: &api.IdentitySet{Group: api.Identity{DisplayName: "Group1"}}}, + {ID: "2", GrantedToIdentities: []*api.IdentitySet{{User: api.Identity{DisplayName: "Alice"}}}}, + {ID: "3", GrantedTo: &api.IdentitySet{User: api.Identity{DisplayName: "Alice"}}}, + {ID: "4"}, + }, + expected: []string{"2", "3", "1", "4"}, + }, + { + name: "same type unsorted", + input: []*api.PermissionsType{ + {ID: "b", GrantedTo: &api.IdentitySet{Group: api.Identity{DisplayName: "Group B"}}}, + {ID: "a", GrantedTo: &api.IdentitySet{Group: api.Identity{DisplayName: "Group A"}}}, + {ID: "c", GrantedToIdentities: []*api.IdentitySet{{Group: api.Identity{DisplayName: "Group A"}}, {User: api.Identity{DisplayName: "Alice"}}}}, + }, + expected: []string{"c", "b", "a"}, + }, + { + name: "all user identities", + input: []*api.PermissionsType{ + {ID: "c", GrantedTo: &api.IdentitySet{User: api.Identity{DisplayName: "Bob"}}}, + {ID: "a", GrantedTo: &api.IdentitySet{User: api.Identity{Email: "alice@example.com"}}}, + {ID: "b", GrantedToIdentities: []*api.IdentitySet{{User: api.Identity{LoginName: "user3"}}}}, + }, + expected: []string{"c", "a", "b"}, + }, + { + name: "no user or group info", + input: []*api.PermissionsType{ + {ID: "z"}, + {ID: "x"}, + {ID: "y"}, + }, + expected: []string{"z", "x", "y"}, + }, + } + + for _, driveType := range []string{driveTypePersonal, driveTypeBusiness} { + t.Run(driveType, func(t *testing.T) { + for _, tt := range tests { + m := &Metadata{fs: &Fs{driveType: driveType}} + t.Run(tt.name, func(t *testing.T) { + if driveType == driveTypeBusiness { + for i := range tt.input { + tt.input[i].GrantedToV2 = tt.input[i].GrantedTo + tt.input[i].GrantedTo = nil + tt.input[i].GrantedToIdentitiesV2 = tt.input[i].GrantedToIdentities + tt.input[i].GrantedToIdentities = nil + } + } + m.orderPermissions(tt.input) + var gotIDs []string + for _, p := range tt.input { + gotIDs = append(gotIDs, p.ID) + } + assert.Equal(t, tt.expected, gotIDs) + }) + } + }) + } +} + +func TestOrderPermissionsJSON(t *testing.T) { + testJSON := `[ + { + "id": "1", + "grantedToV2": { + "group": { + "id": "group@example.com" + } + }, + "roles": [ + "write" + ] + }, + { + "id": "2", + "grantedToV2": { + "user": { + "id": "user@example.com" + } + }, + "roles": [ + "write" + ] + } +]` + + var testPerms []*api.PermissionsType + err := json.Unmarshal([]byte(testJSON), &testPerms) + require.NoError(t, err) + + m := &Metadata{fs: &Fs{driveType: driveTypeBusiness}} + m.orderPermissions(testPerms) + var gotIDs []string + for _, p := range testPerms { + gotIDs = append(gotIDs, p.ID) + } + assert.Equal(t, []string{"2", "1"}, gotIDs) + +}