onedrive: fix metadata ordering in permissions

Before this change, due to a quirk in Graph, User permissions could be
lost when applying permissions.

Fixes #8465

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
 }
 

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)
+
+}