dotnet / AspNetCore.Docs

Documentation for ASP.NET Core
https://docs.microsoft.com/aspnet/core
Creative Commons Attribution 4.0 International
12.51k stars 25.31k forks source link

AdditionalScopesForConsent are missing from the consent prompt in Blazor WASM w/ MSAL #28859

Closed danieltharris closed 4 months ago

danieltharris commented 1 year ago

Are the samples on this page, particularly the GraphClientExtensions.cs still valid?

The following lines configure the MsalProviderOptions by adding AdditionalScopesToConsent, however this doesn't appear to work in a fresh Blazor WASM application based on .NET 7. I am using Blazor WASM and an AAD app registration configured for both personal MSA and Org accounts, but both account types have the same consent issue.

This is the code from GraphClientExtensions in the article that adds the Graph scopes to AdditionalScopesToConsent

services. Configure<RemoteAuthenticationOptions<MsalProviderOptions>>( options => { scopes?.ForEach((scope) => { options.ProviderOptions.AdditionalScopesToConsent.Add(scope); }); });

The rest is the default MSAL code you get when authenticating against the Microsoft Identity Platform, and as this is for both MSA and Org accounts the Authority is https://login.microsoftonline.com/common

The issue seems to be that the consent prompt doesn't include any of the "AdditionalScopesToConsent" - So when you try to use the Graph Client and "AuthenticateAsync" is called there are errors indicating that the user has not consented to the graph scopes.

The consent issue can be worked around during dev; if I grant admin consent for the org in Azure then the consent isn't required - Likewise with a personal MSA I can "trick it" into working by separately requesting the Graph consent in a separate app first, but this won't work in production.

The app is designed to be used by both personal MSA and Org accounts, so the consent flow needs to be smooth and the above workarounds aren't viable in production.

Am I wrong to expect the "AdditonalScopesToConsent" to be appearing when the user first authenticates? As the default MSAL implementation is mostly taken care of for us I can't override the URL used in the auth pop-up to force in the Graph Scopes.


Document Details

⚠ Do not edit this section. It is required for learn.microsoft.com ➟ GitHub issue linking.

guardrex commented 1 year ago

Hello @danieltharris ... I just worked this topic over to include Graph v5 on https://github.com/dotnet/AspNetCore.Docs/pull/28706, but the paint might still be wet ... that round of updates was only tested/applied two weeks ago and may not have addressed a use case scenario that we would like to cover, specifically the organizational account registration without admin consent, as I only tested with admin consent when working up the coverage.

As we look at this, keep in mind that the guidance here isn't meant to replace the MS Identity Platform docs or Azure docs. We only seek to provide an introductory scenario for further configuration and additional use cases ... but ... Yes! ... We do need to find out what's going wrong with the scope/admin consent scenario to see if the article has bad guidance or even just if the article can receive some new guidance for a common scenario that isn't currently addressed.

I have time today to take a look at this. I'll get back to you in a bit ... no later than tomorrow morning.

guardrex commented 1 year ago

CORRECTION ... I just checked my app registration for my test app, and admin consent was NOT granted. I'll test with a new user account and see if when they seek first access if the prompt is asking them for permission to access graph.

guardrex commented 1 year ago

Ok ... very interesting. Yes, I just hit the same problem you hit. The reason that I didn't see this in prior testing with prior accounts is that those users had already granted access for prior (different) app testing (but probably using the same app registration in Azure), so they're accounts already had permission set for Graph. When I tried a new user, it πŸ’₯ ... so I changed that line to supply the scopes as "default" token scopes ...

- options.ProviderOptions.AdditionalScopesToConsent.Add(scope);
+ options.ProviderOptions.DefaultAccessTokenScopes.Add(scope);

... and that worked. My test user, George, had to grant permission again on the next sign in, and then he was able to get user.read access.

@danieltharris ... Can you test your app with DefaultAccessTokenScopes and confirm if it works?

danieltharris commented 1 year ago

@guardrex thanks for taking a look so quickly.

If I add the graph scopes using DefaultAccessTokenScopes then I'm correctly prompted to grant permissions to Graph API - In my case I am requesting access to User.Read and Tasks.Read.

However, in most cases developers will want to request consent for Graph but also other APIs from the same Blazor application - In theory I believe we should be able to do this using the sample code; But that is based off the assumption that AdditionalScopesToConsent should lead to those being requested on the consent screen.

It's possibly an issue with MSAL ignoring the AdditionalScopesToConsent but that would probably need somebody from that team to confirm the intended behaviour/use of AdditonalScopesToConsent - Do you know who can confirm that?

guardrex commented 1 year ago

Yes, I agree with you and planned to ask even if it at least unblocked the scenario. Let's ask if @javiercn can help.

Javier ... TL;DR ... I think the basic question is if there's any reason why AdditionalScopesToConsent would fail to add scopes to the initial sign-in request because we can't seem to get Graph API scopes passed with it. It only seems to work if we add them via DefaultAccessTokenScopes. The scenario is explained here ...

https://learn.microsoft.com/en-us/aspnet/core/blazor/security/webassembly/graph-api?view=aspnetcore-7.0&pivots=graph-sdk-5

danieltharris commented 1 year ago

Just to add another link from the Blazor docs that suggest the approach in the sample is valid:

The linked article below very closely matches the approach used in the sample code - Using AdditionalScopesToConsent and TryGetToken:

https://learn.microsoft.com/en-us/aspnet/core/blazor/security/webassembly/additional-scenarios?view=aspnetcore-7.0#request-additional-access-tokens

guardrex commented 1 year ago

While we wait for Javier to get free and take a look, I'm going to go ahead with a quick patch for what we know works, DefaultAccessTokenScopes. The PR won't close this issue, and there will probably be a follow-up PR later after we find out what's going on with this.

guardrex commented 1 year ago

@danieltharris ... I haven't forgotten about this. I reached out to Javier to get some help on this.

My hunch is still that DefaultAccessTokenScopes is the best choice because users in most of these scenarios need to approve access on initial signin to the app. Thereafter on subsequent signins, it looks like DefaultAccessTokenScopes continues to pass the scopes, the user isn't asked again because they've already granted permission, and they can go on with Graph API calls succeeding on their behalf.

Our docs kind'a mirror the API docs and aren't completely helpful at this time. Here are the API definitions:

My plan is to flesh that out with more detail in the docs. Idk if Javier will feel that the API docs should get more info. The product unit controls those API comments directly through the reference source. I just work the written docs side of things.

guardrex commented 1 year ago

I don't have an answer on this yet. I'll see if Artak thinks this should wait until later this year. For now, the topic covers the problem by recommending DefaultAccessTokenScopes. I'll mark the issue back to triage for visibility in the tracking project.

guardrex commented 1 year ago

I haven't forgotten about this, @danieltharris. The team is busy with the .NET 8 work, so this will need to wait until after .NET 8 releases later this year. For now, this is covered in the topic by recommending the use of DefaultAccessTokenScopes, but I would like to see the difference between these API props clearly explained and confirm that the topic is doing the right thing. I'll also suggest to Javier that the API itself (the reference source that generates the API docs) be updated.

danieltharris commented 1 year ago

Thanks for the update @guardrex - I can develop my application using some workarounds for now and hopefully this will be resolved in an MSAL update after .NET 8 drops, as the workarounds won't really be viable in production.

My hunch right now is that the MSAL library used with Blazor needs a change to include any additional scopes in the redirect to Azure AD - So far I can't find a way to override this behaviour - I'll keep digging but also keep an eye on this issue.

guardrex commented 1 year ago

I'm planning on asking Javier to look at this, but it won't be for quite some time ... closer to EOY around when .NET 8 releases. If you can't apply a decent workaround and this is holding you up, open an issue on the product unit's repo. He'll take a look if it's that serious ...

https://github.com/dotnet/aspnetcore/issues

Add the following to your opening comment ...

cc: @guardrex https://github.com/dotnet/AspNetCore.Docs/issues/28859

... to tie the issues together.

You'll need to re-explain the full scenario and problem there because they don't look at doc issues for background info.

If you want to mention it in passing, you can show how the API comments and our docs don't really distinguish passing scopes with these. They kind'a say the same thing ...

  • DefaultAccessTokenScopes: Gets or set the list of default access tokens scopes to provision during the sign-in flow.
  • AdditionalScopesToConsent: Gets or sets a list of additional scopes to consent during the initial sign-in flow.

The difference in my test apps was that DefaultAccessTokenScopes was capable of spawning the Azure consent UI for permissions AND able to apply scopes (so that's what the topic recommends now), whereas AdditionalScopesToConsent was only able to apply the scopes ... the user would already need permissions to access the resource. That's why my test app at first wasn't having a problem granting access. My test users already had access. When I created a new user and logged in with their account, AdditionalScopesToConsent failed ... switching to DefaultAccessTokenScopes the account received the Azure consent UI and thereafter the account could access Graph. Your situation might be different. Whatever the case, explain in your product issue exactly what the behavior is and why you need AdditionalScopesToConsent to work, not DefaultAccessTokenScopes.

guardrex commented 5 months ago

@danieltharris ... I've finally made it back here after an outrageously long delay. .NET 8's release for Blazor was the largest set of Blazor doc updates we've ever had. I'm wrapping up the last few items for the release now tracked by https://github.com/dotnet/AspNetCore.Docs/issues/28161. This is the first chance that I've been free to get back to a number of languishing (non-.NET 8) issues.

I reviewed Javier's comments on AdditionalScopesToConsent not working prior to .NET 7 at ...

https://github.com/dotnet/aspnetcore/issues/29384#issuecomment-1139018626

... and there was a PR to address it in some way. However, that predates this issue ... this issue being for .NET 7 (or later).

It doesn't look like whatever was addressed for that problem that it deals with what I discovered earlier here ...

Based on comments from devs in PU issues and SO, I think that if the dev takes the approach of using a separate named client for Graph per our guidance (and separate named clients for any other server APIs) that the problem is avoided. However, I think that if a user hasn't granted access to Graph that the app will throw them to ME-ID to get consent the first time that a Graph call is made with the named client in a component ... I think πŸ€”. It's been several years since this guidance was worked out here, so I don't remember what happens.

Because so much time as gone by and because I do have a little time to work on this now, I think the best thing for me to do is work back through the scenarios here from scratch. I'll work on this now ... well ... starting tomorrow (Friday).

guardrex commented 5 months ago

I've just re-evaluated this, and the behavior is as I found earlier ...

The main problem is that AdditionalScopesToConsent won't spawn the consent UI in such a way that Graph API access is permitted after sign-in ... it never works AFAICT ... the user must have the Graph scopes go through DefaultAccessTokenScopes at least once before AdditionalScopesToConsent can be used. I don't think we can post the Graph API guidance with AdditionalScopesToConsent under these circumstances; however, I can call out the behavior in the articles.

I don't understand the reasoning for the behavior on this ... YET ... I'll research a bit more.