GetPermissionRequestsAsync does not return request for the app where another app with the same scope was previously approved.

[larry-lau]

Category

• Bug

GetPermissionRequestsAsync does not return request for the app where another app with the same scope was previously approved.

Steps to reproduce

1. Deploy a SPFx app with the following config

"webApiPermissionRequests": [
      {
        "resource": "App1",
        "scope": "access_as_user"
      }
    ]

2. Approve the access request in SharePoint Admin center
3. Deploy another app with the following config but don't approve it and just leave it pending request

"webApiPermissionRequests": [
      {
        "resource": "App2",
        "scope": "access_as_user"
      }
    ]

Note: App1 and App2 has the same scope name "access_as_user" which is commonly used in OAuth application.
4. Call GetPermissionRequestsAsync()

var appManager = context.GetTenantAppManager();
List<IPermissionRequest> permissionRequests = await appManager.ServicePrincipal.GetPermissionRequestsAsync();

5. Observe that the result value permissionRequests is empty array.

Expected behavior

The expected return value should contain an request object for App2 / access_as_user

Environment details (development & target environment)

• SDK version: [1.11.0]
• OS: [Windows 11]
• SDK used in: [Console App | ASP.Net Web app]
• Framework: [.NET 8]
• Browser(s): [Chrome latest]
• Tooling: [Visual Studio 2022]

Additional context

Investigating further, I found that the filter logic in GetPermissionRequestsAsync in ServicePrincipal.cs is filtering out previoysly approved permission by looking at the scope only. It does not account for different resource Id with the same scope name.

Line 102 in ServicePrincipal.cs

return permissionRequests
    .Where(permissionRequest => !alreadyGranted.Contains(permissionRequest.Scope))
    .ToList();

The filter logic should check both resource id and scope.

Thanks for your contribution! Sharing is caring.

[jansenbe]

@larry-lau : thanks for reporting this.

@mloitzl : is this something you can have a look at?

[mloitzl]

@jansenbe Yep, I think I will be able to fix this issue.

@larry-lau Thanks for the thorough analysis 🤜🤛

[jansenbe]

@mloitzl : did you manage to have some time for this?

[mloitzl]

Hi @jansenbe,
Yes, I have implemented a half baked solution, but it's a surprisingly complicated problem.

There's an edge case left I want to cover:
It can occur, that there is more than one App with the same name and the a same scope in the Directory. But they still have different ids.
I want to introduce an optional parameter (app, or object id, not sure yet) that can be used to denote the specific app that should be used in such a case.

I think I will be able to finish on one of the upcoming weekends.

[mloitzl]

Dear @jansenbe,

Some updates on that:
It seems that the internal CSOM way of approving Permission Requests does not work anymore.
It returns an error like this:
The service principal for permssion request <PackageName> could not be found.

I did some research and discovered that also the "API Access" page in the SharePoint Admin Portal now uses the Graph API instead of the CSOM API to grant OAuth2Permissions on the SharePoint Client Extensibility Principal.

During that I found some other GitHub issues, e.g. SharePoint/sp-dev-docs#9633, or microsoftgraph/msgraph-sample-spfx#14 which may also have the same reason.

I put together a PoC in this draft PR #1479 that mimics the behaviour of the API Access page (work in progress).

It extends the IApp with an ApprovePermissionRequestsAsync method.

Some questions on that:

• Is the IApp interface the right place for that?
• Should the internal CSOM methods ApprovePermissionRequest, DenyPermissionRequest and GetPermissionRequests be removed?
• Should the Enable, Disable, ListGrants, AddGrant and RevokeGrant methods also be moved to the Graph API (if possible)?

Thanks for the feedback, Martin

[jansenbe]

@mloitzl : interesting find on the move to Graph, let me check with the engineering side and come back to you. Thanks for helping with this one!

[mloitzl]

Yep, tested it on 2 different tenants, loitzl1 and loitzl2, same behavior… also with the m365 cli, btw. Then I tried the API Access in the Admin Center and found the new way using browser developer tools. The new way works good 👍 Am 10.06.2024 um 14:10 schrieb Bert Jansen ***@***.***>:  @mloitzl <https://github.com/mloitzl> : can you also check if the "SharePoint Online Client Extensibility Web Application Principal" still showing up in your tenant? — Reply to this email directly, view it on GitHub <#1406 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADARU52OBBHXVRM27O2FYC3ZGWJUDAVCNFSM6AAAAABDVPK23SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJYGE3TKNZRGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jansenbe]

@mloitzl : moving to Graph is indeed the recommended approach, we can mark the CSOM based methods as obsolete once done and then drop them in a next major release.

The fact that CSOM fails is however not expected, I could repro this and am following up.

[mloitzl]

Hi @jansenbe

• I updated GetPermissionRequestsAsync so that it should fix this issue GetPermissionRequestsAsync does not return request for the app where another app with the same scope was previously approved.  #1406.
• It uses a new graph based implementation for getting existing grants "ListGrants2"
• Since it makes sense to also implement the other "Permission" operations, I added "AddGrants2", "RevokeGrant2", "DeleteGrant2"
• The test "AddListRevokeDeleteGrant2Test" puts it all together
• Out of this it is now possible to approve all permission requests of a freshly deployed IApp with "ApprovePermissionRequests" using the new IServicePricipal API

Some issues with that:
I have no clue how to version the API (ListGrants -> ListGrants2, AddGrant -> AddGrant2) ... I just added the postfix '2' for easier search/replace later on.
The same applies for IPermissionRequest and IOAuth2PermissionGrant

What do you think?

I am open for hints/feedback/advise/help on the changes in #1479

[jansenbe]

@mloitzl : thanks for the great work on this! We're still investigating the issue with the existing CSOM API and will fix that, but being able to use the "modern" approach via Graph is the future.

Let's settle with a "2" suffix whenever that's needed for methods and types. When there's already an "Async" suffix then the let's go for "...2Async". Once this is in place we can mark the old classes/methods as Obsolete. In a next major release then obsolete classes/methods then will be removed from the code base. This way we give folks time to switch over without breaking their code.