Sort Graph permissions with users ahead of groups #8465
Labels
Support Contract
Issues made for customers with support contracts
Milestone
Comments
chscott
added
the
Support Contract
|
Heads up @rclone/support - the "Support Contract" label was applied to this issue. |
chscott
changed the title
|
@ncw This is what I emailed about earlier today. It appears to be a Graph issue and not a bug, and there is a workaround available. Still, I think it would be good to address this in rclone, as it could lead to loss of access in the following scenario:
Whether intentional or not, Graph is assuming group memberships are static, and that's anything but true in the real world. |
ncw
added a commit
that referenced
this issue
Mar 26, 2025
Before this change, due to a quirk in Graph, User permissions could be lost when applying permissions. Fixes #8465
|
@chscott I had a go at ordering the permission updates so anything with a user in it comes first. I think this should fix the problem. Can you give it a try? v1.70.0-beta.8641.2af1e8f46.fix-8465-onedrive-metadata on branch fix-8465-onedrive-metadata (uploaded in 15-30 mins) Thanks |
|
@ncw The fix didn't work for me. Here's are two log excerpts. The first is with the fix and no changes on my side. The second is with |
|
There is something which is puzzling me above. The POSTs have grantedTo not grantedToV2 like I would expect for a onedrive business. Is that what the metadata mapper is sending? I think it probably needs to send grantedTo for onedrive personal and grantedToV2 for business, same for grantedToIdentities |
|
The mapper is sending |
|
Yes I see the mapper is sending What is confusing me is this which appears to send or receive Can you send me what rclone sent and what it received from onedrive for this call? I want to know if rclone sent The reason I'm worried about this is that we sort the entries according to Thank you |
|
@ncw Do you want data different from what is in #8465 (comment), or is that what you're looking for? |
|
I'd like to see what rclone sent if possible - I think (but I'm not 100% sure) that the above is what onedrive sent. That will need |
|
A full log is attached. I think the bit you're looking for is here: I believe what we are seeing was raised as an issue in https://learn.microsoft.com/en-us/answers/questions/1332382/microsoft-graph-api-drives-((drive-id))-items-((it, though I suspect In my case, I deduced that it was because the user/group I was trying to assign permissions already had permissions on the item through some other means. I think this is a bug on the Microsoft side, but the workaround of adding users before groups will help mitigate it, with no real downside I can think of. Presumably, there will still be cases that can be impacted, like groups that contain other groups. |
|
Thanks for that. I think you are right the I've just been through the log you sent. It looks like the user was sent before the group even though the metadata mapper sent them the other way round. is followed by So in this case the new code appears to have done its job. Both the users are visible in the final output Final output```json { "@odata.context": "https://graph.microsoft.com/v1.0/$metadata#drives('b%21RI7jTE_7VEWvQfQFOGxQOFg6zGE89vBPg_ISUx_CLGNzm-ky8cLeQrzJ6fhJvcxd')/items('014TQUZBEF3KFB577R5NBYOXBGZ4XPB5MR')/permissions", "value": [ { "id": "aTowIy5mfG1lbWJlcnNoaXB8Y3VzZXIyQHRyYW5zZW5kdGVzdGluZy5vbm1pY3Jvc29mdC5jb20", "roles": [ "write" ], "shareId": "aTowIy5mfG1lbWJlcnNoaXB8Y3VzZXIyQHRyYW5zZW5kdGVzdGluZy5vbm1pY3Jvc29mdC5jb20", "grantedToV2": { "user": { "@odata.type": "#microsoft.graph.sharePointIdentity", "displayName": "Cloud User2", "email": "cuser2@transendtesting.onmicrosoft.com", "id": "57c26025-316f-4a71-a464-bf3adb0a6ac4" }, "siteUser": { "displayName": "Cloud User2", "email": "cuser2@transendtesting.onmicrosoft.com", "id": "7", "loginName": "i:0#.f|membership|cuser2@transendtesting.onmicrosoft.com" } }, "grantedTo": { "user": { "displayName": "Cloud User2", "email": "cuser2@transendtesting.onmicrosoft.com", "id": "57c26025-316f-4a71-a464-bf3adb0a6ac4" } } }, { "id": "Yzowby5jfGZlZGVyYXRlZGRpcmVjdG9yeWNsYWltcHJvdmlkZXJ8ODVhZDY2ZTItYzJlYS00ZTMxLWEwNjQtMmU5NThlZGVjYzNm", "roles": [ "write" ], "shareId": "Yzowby5jfGZlZGVyYXRlZGRpcmVjdG9yeWNsYWltcHJvdmlkZXJ8ODVhZDY2ZTItYzJlYS00ZTMxLWEwNjQtMmU5NThlZGVjYzNm", "grantedToV2": { "group": { "@odata.type": "#microsoft.graph.sharePointIdentity", "displayName": "Reviewers Members", "email": "reviewers@transendtesting.onmicrosoft.com", "id": "85ad66e2-c2ea-4e31-a064-2e958edecc3f" }, "siteUser": { "displayName": "Reviewers Members", "email": "reviewers@transendtesting.onmicrosoft.com", "id": "8", "loginName": "c:0o.c|federateddirectoryclaimprovider|85ad66e2-c2ea-4e31-a064-2e958edecc3f" } }, "grantedTo": { "user": { "displayName": "Reviewers Members", "email": "reviewers@transendtesting.onmicrosoft.com", "id": "85ad66e2-c2ea-4e31-a064-2e958edecc3f" } } }, { "id": "aTowIy5mfG1lbWJlcnNoaXB8Y3VzZXIxQHRyYW5zZW5kdGVzdGluZy5vbm1pY3Jvc29mdC5jb20", "roles": [ "owner" ], "shareId": "aTowIy5mfG1lbWJlcnNoaXB8Y3VzZXIxQHRyYW5zZW5kdGVzdGluZy5vbm1pY3Jvc29mdC5jb20", "grantedToV2": { "user": { "@odata.type": "#microsoft.graph.sharePointIdentity", "displayName": "Cloud User1", "email": "cuser1@transendtesting.onmicrosoft.com", "id": "e05ce5e6-bd01-4a63-981f-23719dc7e625" }, "siteUser": { "displayName": "Cloud User1", "email": "cuser1@transendtesting.onmicrosoft.com", "id": "3", "loginName": "i:0#.f|membership|cuser1@transendtesting.onmicrosoft.com" } }, "grantedTo": { "user": { "displayName": "Cloud User1", "email": "cuser1@transendtesting.onmicrosoft.com", "id": "e05ce5e6-bd01-4a63-981f-23719dc7e625" } } } ] } ```
What do you think @chscott did the new code work OK in this circumstance? If so why didn't it work above? 😕 |
|
In the most recent test, I was only trying to get the requests and responses, so I didn't back out my code change to return the permissions to Rclone in sorted (users, then groups) order. When I remove that change, the results I get are as in #8465 (comment). |
ncw
added a commit
that referenced
this issue
Apr 8, 2025
Before this change, due to a quirk in Graph, User permissions could be lost when applying permissions. Fixes #8465
|
I'm struggling to work out why this isn't working, so here is a version with some more debugging v1.70.0-beta.8658.ec2c1bab8.fix-8465-onedrive-metadata on branch fix-8465-onedrive-metadata (uploaded in 15-30 mins) It should output ERROR logs which look like and Can you have a go with that and send me the log? Hopefully that will clear up what is happening. Thank you |
|
I don't see a Windows build at that link.
Chad Scott
…________________________________
From: Nick Craig-Wood ***@***.***>
Sent: Tuesday, April 8, 2025 6:36 AM
To: rclone/rclone ***@***.***>
Cc: Chad Scott ***@***.***>; Mention ***@***.***>
Subject: Re: [rclone/rclone] Sort Graph permissions with users ahead of groups (Issue #8465)
I'm struggling to work out why this isn't working, so here is a version with some more debugging
v1.70.0-beta.8658.ec2c1bab8.fix-8465-onedrive-metadata<https://beta.rclone.org/branch/fix-8465-onedrive-metadata/v1.70.0-beta.8658.ec2c1bab8.fix-8465-onedrive-metadata/> on branch fix-8465-onedrive-metadata<https://github.com/rclone/rclone/tree/fix-8465-onedrive-metadata> (uploaded in 15-30 mins)
It should output ERROR logs which look like
2025/04/08 12:34:36 ERROR : perms before sort:
[
{
"id": "1",
"grantedTo": {
"user": {},
"application": {},
"device": {},
...
and
2025/04/08 12:34:36 ERROR : perms after sort:
[
{
"id": "2",
"grantedToIdentities": [
{
"user": {
"displayName": "Alice"
},
"application": {},
"device": {},
...
Can you have a go with that and send me the log? Hopefully that will clear up what is happening.
Thank you
—
Reply to this email directly, view it on GitHub<#8465 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACKZ34XYJGBUXR73TMGMX532YOYDJAVCNFSM6AAAAABZQ5F37KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOBWGE2DKMBRGM>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
[https://avatars.githubusercontent.com/u/536803?s=20&v=4]ncw left a comment (rclone/rclone#8465)<#8465 (comment)>
I'm struggling to work out why this isn't working, so here is a version with some more debugging
v1.70.0-beta.8658.ec2c1bab8.fix-8465-onedrive-metadata<https://beta.rclone.org/branch/fix-8465-onedrive-metadata/v1.70.0-beta.8658.ec2c1bab8.fix-8465-onedrive-metadata/> on branch fix-8465-onedrive-metadata<https://github.com/rclone/rclone/tree/fix-8465-onedrive-metadata> (uploaded in 15-30 mins)
It should output ERROR logs which look like
2025/04/08 12:34:36 ERROR : perms before sort:
[
{
"id": "1",
"grantedTo": {
"user": {},
"application": {},
"device": {},
...
and
2025/04/08 12:34:36 ERROR : perms after sort:
[
{
"id": "2",
"grantedToIdentities": [
{
"user": {
"displayName": "Alice"
},
"application": {},
"device": {},
...
Can you have a go with that and send me the log? Hopefully that will clear up what is happening.
Thank you
—
Reply to this email directly, view it on GitHub<#8465 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACKZ34XYJGBUXR73TMGMX532YOYDJAVCNFSM6AAAAABZQ5F37KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOBWGE2DKMBRGM>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Sorry committed a bad test. Try this v1.70.0-beta.8659.9d4c792da.fix-8465-onedrive-metadata on branch fix-8465-onedrive-metadata (uploaded in 15-30 mins) |
|
Now I'm confused. I don't get the ERROR logs you mentioned, but this build worked, even with my changes reverted and the group still being added first. |
ncw
added a commit
that referenced
this issue
Apr 8, 2025
Before this change, due to a quirk in Graph, User permissions could be lost when applying permissions. Fixes #8465
That would explain why it hasn't been working if the permissions ordering code isn't being called for some reason. Looking at the code I see there is a path (when there are only new permissions and no old ones) when they don't get ordered! That is an easy fix though v1.70.0-beta.8674.9e62fa8c8.fix-8465-onedrive-metadata on branch fix-8465-onedrive-metadata (uploaded in 15-30 mins)
Confusing indeed - the updates are sent in the wrong order but the result looks OK. Do you think Microsoft have fixed this? |
I looked for something in the Graph changelog, but I don't see anything that looks remotely close. Since there's a workaround of ensuring the mapper sends sorted permissions, I don't know that there's anything more we can do right now. |
I think my fix above should sort the permissions properly if you have time to give it a go. |
|
@ncw The latest fix did not work for me, as I still see the group sorted ahead of the user. Version Data from mapper Requests to add permissions Request to fetch permissions |
ncw
added a commit
that referenced
this issue
Apr 10, 2025
Before this change, due to a quirk in Graph, User permissions could be lost when applying permissions. Fixes #8465
|
Thanks for that @chscott I can see from the logs that the sorting isn't working. I figured out why - it is because I was ignoring the ID field in the permissions to detect whether they were set or not, and that is the only one you are setting! I've fixed this now. I've tested this with your data so I think it should work for you now. v1.70.0-beta.8711.96957a177.fix-8465-onedrive-metadata on branch fix-8465-onedrive-metadata (uploaded in 15-30 mins) |
|
Looks good, @ncw. I see this in the log, and the user permission is added to the file. I think we can close this as resolved. |
|
Thanks for testing @chscott - glad we got there in the end. I've merged this to master now which means it will be in the latest beta in 15-30 minutes and released in v1.70 It will be in this and subsequent betas v1.70.0-beta.8714.e0c99d620 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The associated forum post URL from
https://forum.rclone.orgDiscussed in email.
What is your current rclone version (output from
rclone version)?v1.69.1What problem are you are trying to solve?
Graph has a quirk that manifests when all of these conditions apply:
When all of the above are true, Graph indicates it has added the user permission, but it immediately drops it. For example:
How do you think rclone should be changed to solve that?
Attempt to apply user permissions before group permissions, which works around the issue. Note that you can force this to be the case today by returning a sorted permissions object from the mapper, but it's probably not obvious that this is required.
How to use GitHub
The text was updated successfully, but these errors were encountered: