onedrive: about the Sites.Read.All permission request #5883
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cqjjjzr
force-pushed
the
fix-onedrive-sites
branch
from
5d76ca0
to
6ccc558
Compare
|
I added (partial) docs about the Sites.Read.All permission. |
5 tasks
Cnly
requested changes
Jan 9, 2022
backend/onedrive/onedrive.go
Outdated
| @@ -374,6 +388,12 @@ func Config(ctx context.Context, name string, m configmap.Mapper, config fs.Conf | |||
| region, graphURL := getRegionURL(m) | |||
|
|
|||
| if config.State == "" { | |||
| disableSitePermission, ok := m.Get("disable_site_permission") | |||
| if disableSitePermission == "true" && ok { | |||
There was a problem hiding this comment.
Suggested change
| if disableSitePermission == "true" && ok { | |
| if ok && disableSitePermission == "true" { |
Usually we put ok before the value predicate. Or we can just remove it here.
There was a problem hiding this comment.
After some more thinking I think we should just remove the check on ok to allow possible changes to the default value. If at some later point we decided true will be better as the default, the ok check here will break that as it assumes false should be the default.
docs/content/onedrive.md
Outdated
| @@ -132,7 +132,7 @@ Client ID and Key by following the steps below: | |||
| 2. Enter a name for your app, choose account type `Accounts in any organizational directory (Any Azure AD directory - Multitenant) and personal Microsoft accounts (e.g. Skype, Xbox)`, select `Web` in `Redirect URI`, then type (do not copy and paste) `http://localhost:53682/` and click Register. Copy and keep the `Application (client) ID` under the app name for later use. | |||
| 3. Under `manage` select `Certificates & secrets`, click `New client secret`. Enter a description (can be anything) and set `Expires` to 24 months. Copy and keep that secret _Value_ for later use (you _won't_ be able to see this value afterwards). | |||
| 4. Under `manage` select `API permissions`, click `Add a permission` and select `Microsoft Graph` then select `delegated permissions`. | |||
| 5. Search and select the following permissions: `Files.Read`, `Files.ReadWrite`, `Files.Read.All`, `Files.ReadWrite.All`, `offline_access`, `User.Read`. Once selected click `Add permissions` at the bottom. | |||
| 5. Search and select the following permissions: `Files.Read`, `Files.ReadWrite`, `Files.Read.All`, `Files.ReadWrite.All`, `offline_access`, `User.Read`, `Sites.Read.All` (optional). Once selected click `Add permissions` at the bottom. | |||
There was a problem hiding this comment.
Suggested change
| 5. Search and select the following permissions: `Files.Read`, `Files.ReadWrite`, `Files.Read.All`, `Files.ReadWrite.All`, `offline_access`, `User.Read`, `Sites.Read.All` (optional). Once selected click `Add permissions` at the bottom. | |
| 5. Search and select the following permissions: `Files.Read`, `Files.ReadWrite`, `Files.Read.All`, `Files.ReadWrite.All`, `offline_access`, `User.Read`, and optionally `Sites.Read.All` (see below). Once selected click `Add permissions` at the bottom. |
Perhaps this and an extra sentence below explaining the option & linking to the issue #1770 would be better.
|
Should I push multiple commits or squash them using rebase and do a force push? |
|
You don't need to force push :) Force pushing will probably mess up with the review history. Just push them normally and everything will be squashed before merging into master. |
Cnly
approved these changes
Jan 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the purpose of this change?
In fix of #1770 , a new permission request was added in the OneDrive backend support:
Sites.Read.All. This is intended to solve the error when using a custom SharePoint site. However, such changes are undocumented.On a domain with an excessively strict authorization policy (e.g. my university), the domain disallows users to consent permissions on their own and the
Sites.Read.Allpermission is undocumented thus not configured, the extraSites.Read.Allscope when requesting an access token will trigger "Application needs permission to access resources in your organization that only an admin can grant. Please ask an admin to grant permission to this app before you can use it." message.Therefore, this PR involves a new advanced config entry
disable_site_permission(default to false). When turned on, this config will prevent the OAuth client to requestSites.Read.Allpermission. The "Search for a SharePoint site" may be unusable because of this, but for those under a strict policy who don't need custom site configuration, this should be enough.For the documentation, since I'm unfamiliar with the documentation structure, I didn't attempt to include all documentation for this. Instructions on writing docs will be much appreciated
Was the change discussed in an issue or in the forum before?
See #1770
Checklist