Clarify behavior when both FSC.Manage.All and FSC.Selected are granted for admin and nonadmin#10763
Clarify behavior when both FSC.Manage.All and FSC.Selected are granted for admin and nonadmin#10763lilealdai wants to merge 8 commits intoSharePoint:mainfrom
Conversation
|
Learn Build status updates of commit 3664737: ✅ Validation status: passed
For more details, please refer to the build report. |
PoliCheck Scan ReportThe following report lists PoliCheck issues in PR files. Before you merge the PR, you must fix all severity-1 and severity-2 issues. The AI Review Details column lists suggestions for either removing or replacing the terms. If you find a false positive result, mention it in a PR comment and include this text: #policheck-false-positive. This feedback helps reduce false positives in future scans. ✅ No issues foundMore information about PoliCheckInformation: PoliCheck | Severity Guidance | Term |
PoliCheck Scan ReportThe following report lists PoliCheck issues in PR files. Before you merge the PR, you must fix all severity-1 and severity-2 issues. The AI Review Details column lists suggestions for either removing or replacing the terms. If you find a false positive result, mention it in a PR comment and include this text: #policheck-false-positive. This feedback helps reduce false positives in future scans. ✅ No issues foundMore information about PoliCheckInformation: PoliCheck | Severity Guidance | Term |
|
Learn Build status updates of commit 9a2227e: ✅ Validation status: passed
For more details, please refer to the build report. |
PoliCheck Scan ReportThe following report lists PoliCheck issues in PR files. Before you merge the PR, you must fix all severity-1 and severity-2 issues. The AI Review Details column lists suggestions for either removing or replacing the terms. If you find a false positive result, mention it in a PR comment and include this text: #policheck-false-positive. This feedback helps reduce false positives in future scans. ✅ No issues foundMore information about PoliCheckInformation: PoliCheck | Severity Guidance | Term |
|
Learn Build status updates of commit 640bb6e: ✅ Validation status: passed
For more details, please refer to the build report. |
PoliCheck Scan ReportThe following report lists PoliCheck issues in PR files. Before you merge the PR, you must fix all severity-1 and severity-2 issues. The AI Review Details column lists suggestions for either removing or replacing the terms. If you find a false positive result, mention it in a PR comment and include this text: #policheck-false-positive. This feedback helps reduce false positives in future scans. ✅ No issues foundMore information about PoliCheckInformation: PoliCheck | Severity Guidance | Term |
|
Learn Build status updates of commit 6438db9: ✅ Validation status: passed
For more details, please refer to the build report. |
PoliCheck Scan ReportThe following report lists PoliCheck issues in PR files. Before you merge the PR, you must fix all severity-1 and severity-2 issues. The AI Review Details column lists suggestions for either removing or replacing the terms. If you find a false positive result, mention it in a PR comment and include this text: #policheck-false-positive. This feedback helps reduce false positives in future scans. ✅ No issues foundMore information about PoliCheckInformation: PoliCheck | Severity Guidance | Term |
|
Learn Build status updates of commit 0af6d96: ✅ Validation status: passed
For more details, please refer to the build report. |
PoliCheck Scan ReportThe following report lists PoliCheck issues in PR files. Before you merge the PR, you must fix all severity-1 and severity-2 issues. The AI Review Details column lists suggestions for either removing or replacing the terms. If you find a false positive result, mention it in a PR comment and include this text: #policheck-false-positive. This feedback helps reduce false positives in future scans. ✅ No issues foundMore information about PoliCheckInformation: PoliCheck | Severity Guidance | Term |
|
Learn Build status updates of commit dc76527: ✅ Validation status: passed
For more details, please refer to the build report. |
| | :--------------- | :------------------------------------------------------------- | :------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | Administrator | FileStorageContainer.Selected | The application can access containers on behalf of the user as a non-administrator. Container instance-level [user permissions](#user-permissions) will apply. | | ||
| | Administrator | FileStorageContainer.Manage.All | The application utilizes file storage container administration capabilities on behalf of an administrator. | | ||
| | Administrator | FileStorageContainer.Selected, FileStorageContainer.Manage.All | The application utilizes file storage container administration capabilities on behalf of an administrator. FileStorageContainer.Selected will be ignored. | |
There was a problem hiding this comment.
This bit documented here would mean it is a product direction decision. I’m not confident we want this from a product perspective. It also requires a security review as it’s a deviation from the auth standard (scopes are additive and not restrictive, i.e. more scopes = more permissions).
I think this is something we may want to call out specifically for the container APIs in the docs for those endpoints. But not here. In here, we can mention the container APIs under “exceptional access patters” and explain this off behaviour.
There was a problem hiding this comment.
@dluces neither of these scopes restrict any application permissions, but they serve as gates to different AuthZ requirements:
- FSC.Selected doesn't grant any permissions but it lets the app though the door towards the ContainerType-based AuthZ checks (the mask is defined by the CT-based app perms).
- FSC.Manage.All allows the app to act on behalf of an administrator user. If the user is not an administrator, this scope doesn't allow anything (it's an empty mask).
Now, when an API endpoint supports both scopes, and both scopes a present, and the user is an admin, then the permission mask will always be >= of what FSC.Selected would have granted.
Example: Create Container Permission
IsAdmin: true
Scopes: FSC.Selected, FSC.Manage.All
App's CT perms: SPContainerPermissions.ManagePermissions
Result: access granted
If the user is a non-admin, then FSC.Manage.All will grant an EmptyMask and the app will have CT-based perms.
There was a problem hiding this comment.
This is an interesting interpretation that we should discuss during a security review. Access is calculated like this for *.Selected scopes: https://learn.microsoft.com/en-us/graph/permissions-selected-overview?tabs=http#how-access-is-calculated
There is room for interpretation. We should discuss with the sec review team.
There was a problem hiding this comment.
*.Selected scopes are meant to trigger the resource specific permission lookup. You’re changing that flow.
And also, the part about making the application permissions mean one thing based on certain circumstances (user role) and another thing based on some other circumstances (no user role), can be an interesting interpretation as well. An application getting different privileges by a same permission depending on the user context, let’s discuss in the sec review.
There was a problem hiding this comment.
@dluces we're not changing how the FSC.Selected scope works. The presence of this scope triggers a resource-specific permission check (a.k.a. ContainerType application permission check). If the FSC.Manage.All scope is also present and the user is an admin, then we skip the resource-specific permission check (since it's not required), but that doesn't break the principle of how multiple scopes work together.
FSC.Manage.All was introduced as a Delegated-only scope that "Allows the application to utilize the file storage container administration capabilities on behalf of an administrator user. ". This was reviewed and approved by the Graph API Council: https://msazure.visualstudio.com/One/_git/AD-AggregatorService-Workloads/pullrequest/8713742
There was a problem hiding this comment.
Scopes shouldn't be defined in terms of user permissions. Scopes are meant to be used by the application only. I believe this is the original problem. We made a mistake when we reviewed / approved that definition.
There was a problem hiding this comment.
The "app roles vs scope" distinction is a mechanism on top of "OAuth scopes" that we (Microsoft / Entra ID) use to quickly differentiate access on behalf of a user vs access without a user. In OAuth those are just token scopes. They are an artifact exclusively for the application interaction.
There was a problem hiding this comment.
Fun fact, we're actually removing those mentions to user roles for FSCT and FSCTR permissions because they had the same issue: https://msazure.visualstudio.com/One/_git/AD-AggregatorService-Workloads/pullrequest/15206619
There was a problem hiding this comment.
@dluces could you please point me to the description of the standard you're referring to?
| | Administrator | FileStorageContainer.Manage.All | The application utilizes file storage container administration capabilities on behalf of an administrator. | | ||
| | Administrator | FileStorageContainer.Selected, FileStorageContainer.Manage.All | The application utilizes file storage container administration capabilities on behalf of an administrator. FileStorageContainer.Selected will be ignored. | | ||
| | Non-administrator | FileStorageContainer.Selected | The application can access containers on behalf of a non-administrator user. Container instance-level [user permissions](#user-permissions) will apply. | | ||
| | Non-administrator | FileStorageContainer.Manage.All | Access denied. | |
There was a problem hiding this comment.
This is also an exceptional access (denied?) pattern that doesn’t follow standard. The app has permissions for metadata of all containers, the user has some access to some containers, but the intersection is empty 🤷🏾
Again, not here to fight the fight of how that doesn’t follow industry standards. But instead to make it clear that it’s an exceptional access pattern (that hopefully we’ll fix someday)
There was a problem hiding this comment.
@dluces FSC.Manage.All allows the app to perform admin actions on behalf of an administrator user. In other words, it grants certain permissions if the calling user is an Admin, or an EmptyMask otherwise.
There was a problem hiding this comment.
Re: "the app has permissions for metadata of all containers, the user has some access to some containers, but the intersection is empty 🤷🏾" - with FSC.Manage.All the app will have "EmptyMask" permissions if the caller is a non-admin (that's the contract of this scope as well as SPE - non-Admins can only access SPE containers of different types via respective type-specific apps), hence the intersection will always be an EmptyMask regardless of the user's container-level perms.
There was a problem hiding this comment.
@dluces FSC.Manage.All allows the app to perform admin actions on behalf of an administrator user. In other words, it grants certain permissions if the calling user is an Admin, or an EmptyMask otherwise.
Everything after the "if" is what deviates from standard. OAuth scopes are for application interactions alone. Nothing to do with users or user permissions.
There was a problem hiding this comment.
@dluces, looking at https://learn.microsoft.com/en-us/graph/permissions-reference, it's a common pattern to require the signed-in user to be an administrator in some Delegated scenarios. Note the "The signed-in user must be an administrator." or "when the app is used by a privileged user (e.g. a Company Administrator)" and other similar verbiage used in multiple different cases.
Application scopes/roles support both App-only (role) and Delegated (scope) flows. FSC.Selected supports both while FSC.Manage.All only supports the Delegated flow. In the latter case, who the user is does matter.
| - **[FileStorageContainerType.Manage.All](/graph/permissions-reference#filestoragecontainertypemanageall)** to allow an application to create and manage container types on the owning tenant. This permission is only needed on the owning tenant where the container type is created. | ||
| - **[FileStorageContainerTypeReg.Selected](/graph/permissions-reference#filestoragecontainertyperegselected)** to allow an application to register the container type on consuming tenants. | ||
| - **[FileStorageContainer.Selected](/graph/permissions-reference#filestoragecontainerselected)** to allow an application to access containers of the given container type on consuming tenants. | ||
| - **[FileStorageContainer.Manage.All](/graph/permissions-reference#filestoragecontainermanageall)** to allow an application to utilize the file storage container administration capabilities against all containers of all governable container types within the consuming tenant on behalf of an administrator user. |
There was a problem hiding this comment.
Add a note that this should NOT be requested unless building an application to manage other SPE applications, like the SPAC. Should be an important note.
There was a problem hiding this comment.
My comment to Li was that we want this note in the description of the permission itself, because that's what most people will interact with.
There was a problem hiding this comment.
The more places, the better. We also don't list all SPE permissions here, only the ones that are relevant to build and app. So we could also just remove this.
| #### Access on behalf of a user | ||
|
|
||
| SharePoint Embedded operations [on behalf of a user](/graph/auth-v2-user) require applications to receive consent for Microsoft Graph **[FileStorageContainer.Selected](/graph/permissions-reference#filestoragecontainerselected)** delegated permission. | ||
| SharePoint Embedded operations [on behalf of a user](/graph/auth-v2-user) support two Microsoft Graph permissions: **[FileStorageContainer.Selected](/graph/permissions-reference#filestoragecontainerselected)**, which allows an application to access containers on behalf of the signed-in user, and **[FileStorageContainer.Manage.All](/graph/permissions-reference#filestoragecontainermanageall)**, which allows an application to utilize file storage container administration capabilities on behalf of an administrator. |
There was a problem hiding this comment.
Define “file storage container administration capabilities” does that mean create / delete containers? How about content? What are the implications of this on the SPE app that owns those containers? Some quick definition would be helpful linking to the consuming tenant admin article
There was a problem hiding this comment.
And we’d need to make sure that that article makes sense in the context of this scope and not just SPAC. Shreyas can help with that too.
There was a problem hiding this comment.
@dluces I'd rather not include the specific list of supported APIs here, otherwise we'd have to maintain / update this doc every time an API is added / updated. For every SPE API, we either list both FSC.Selected and FCS.Manage.All scopes, or one of them. If only FSC.Selected is listed, then it's not a part of the administrative API surface.
It may be fine to include a non-exhaustive list as an example, e.g.: soft-delete, hard-delete and restore soft-deleted containers, enumerate active and deleted containers, manage containers' permissions, etc.
There was a problem hiding this comment.
I understand it could mean more work for us and yet the priority is to provide clarity to the reader. We could summarize all of those as "actions on fileStorageContainer resources" and mention that administrative capabilities do not include access to content (that is driveItem resources in containers). Does that sound like a fair compromise?
|
|
||
| In addition to your application receiving consent for **[FileStorageContainer.Selected](/graph/permissions-reference#filestoragecontainerselected)** on a consuming tenant, the user that it's acting on behalf of is required to have [user permissions](#user-permissions). The effective permissions that the application has are the intersection of the application permissions and the user permissions when acting on behalf of a user. | ||
|
|
||
| For APIs that support both **FileStorageContainer.Selected** and **FileStorageContainer.Manage.All** permissions, the effective access depends on the permissions granted to the application and whether the user is an administrator: |
There was a problem hiding this comment.
If we need to explain an intersection of permissions, it likely is because it doesn’t follow standard and thus should be in the exceptional access pattern section instead.
There was a problem hiding this comment.
@dluces I assume you mean a union, not an intersection. The key thing about the FSC.Manage.All scope is that it only allows the app to act of behalf of an administrator user. In other words, if the user is an Admin, the scope allows the app to call the API (for APIs that support the FSC.Manage.All scope), otherwise it grants nothing (an EmptyMask) thus adding nothing to the union.
For any given API that supports both scopes, FSC.Manage.All + Admin >= FSC.Selected.
There was a problem hiding this comment.
Effective permissions in user-delegated mode are an intersection. I meant that if the intersection isn’t obviously clear, it likely signals that there’s a deviation from the standard.
There was a problem hiding this comment.
Right. App and User perms are always an intersection. This applies to all SPE scenarios.
Application scopes are a union. This also applies to all SPE scenarios (with the recent fix Li checked in).
There was a problem hiding this comment.
Since this would be the only off-standard cases:
Non-administrator | FSC.Manage.All | Access Denied
Non-administrator | FSC.Manage.All + FSC.Selected | FSC.Manage.All is ignored
And given our convo yesterday w/Barry and Humberto and the conclusion that this is not something that needs changing, can we simplify this whole table to say something like "If the user identity is not a SharePoint Embedded or Global Administrator, FileStorageContainer.Manage.All does NOT grant the application any permissions." And move this within the "Exceptional access patterns" instead? This section is too soon to go into the weeds of this and makes it noisy when we're just trying to describe access on behalf of a user:
Instead, we can do this:
Exceptional access patterns
Currently, there are three types of operations with exceptional access patterns:
- (exists) Operations involving searching SharePoint Embedded content
- (exists) Operations that require a user license
- (new) Operations that involve administrative actions on containers
Operations that involve administrative actions on containers
Introduce FileStorageContainer.Manage.All here.
Explain the very specific exceptional access pattern (FSC.Manage.All with non-administrators). That's the only thing that deviates from standard, so no need to explain the rest (FSC.Manage.All + admin).
There was a problem hiding this comment.
@dluces Makes sense to me. @vhontovyy-MS What do you think?
There was a problem hiding this comment.
@lilealdai @dluces
I'm okay with the "exceptional access pattern" approach as long as it helps make things clearer for our customers. That said, we don't plan to change how FSC.Manage.All works meaning that we'll continue requiring the caller to be an admin in order for the app to get actual permissions on the basis of having this scope.
Also, I think we should explain that FSC.Manage.All + Admin allows the caller to manage (list, delete, restore, purge, update, manage permissions) all containers of all container types in the tenant.
Note: looking at the screenshot in Diego's post above, FileStorageContainer.Manage.All should be listed within the "Access on behalf of a user" section since this scope is only supported in Delegated mode (so it shouldn't be listed in the "Application permissions" section). If my understanding is correct, if we take the exceptional access pattern route, this will no longer apply.
4 participants
Category
What's in this Pull Request?
the user is an administrator.