Skip to content

Move the Blazor WebAssembly with Graph API content to a new doc#20192

Merged
guardrex merged 11 commits intomasterfrom
guardrex/blazor-wasm-security
Oct 27, 2020
Merged

Move the Blazor WebAssembly with Graph API content to a new doc#20192
guardrex merged 11 commits intomasterfrom
guardrex/blazor-wasm-security

Conversation

@guardrex
Copy link
Copy Markdown
Collaborator

@guardrex guardrex commented Oct 14, 2020

@guardrex
Copy link
Copy Markdown
Collaborator Author

guardrex commented Oct 23, 2020

@jmprieur I placed the code directly into the comment above ☝️ to make it easy for you to run an eye over it.

However ...

WRT the new approaches on AzureAD/microsoft-identity-web#705 ... I'm working with Microsoft.Graph 3.18.0 and Microsoft.Identity.Client 4.18.0, but I see that Microsoft.Identity.Client 4.21.1 is out. Does that package contain the new API for the latest daemon approach from the PR? If so, then I need to update my process here and then ping u back next week. If I can't use the new daemon approach yet, then I'll open a new issue, and we should proceed with what I'm working on here because I need to get these updates in for 5.0 GA.

Tell me one thing tho if I'm going to update to the latest daemon app approach now: Do I need to consider if the Graph access token ever expires in a daemon app? I think that I do not need to worry about that, but I just want to check with you on it.

@jmprieur
Copy link
Copy Markdown
Contributor

@guardrex : we released Microsoft.Identity.Web 1.2.0 today.
It does have the new daemon approach, which is explained here: https://github.com/AzureAD/microsoft-identity-web/wiki/1.2.0#you-can-now-specify-scopes-and-app-permissions-for-graphserviceclient

If the token expires, Microsoft.Identity.Web (via MSAL.NET) will renew it. So no worries about that

@guardrex
Copy link
Copy Markdown
Collaborator Author

guardrex commented Oct 23, 2020

@jmprieur ... Awesome! ... Thanks ... ok ... I'll rework that bit then and ping u back next week (by Tuesday morning). I'll put the approach into a comment here to make it quick for you to inspect.

cc: @captainsafia ... TL;DR ... The final bit for 5.0 WASM security updates here should be done Monday/Tuesday. @jmprieur will 👁️ a piece of this first to make sure that I'm doing good things with the hosted server API AAD group policies+Graph SDK/API piece ... the very last bit that needs work here 😅. After I have that resolved (and make a few other nit updates), I'll ping u. This PR has all of the remaining 5.0 WASM security work AFAIK. In fact AFAIK, this PR will wrap up ALL of the major Blazor 5.0 updates† 🎉🎈🍾.

†But we will have to wrap up the component integration PR (dotnet/AspNetCore.Docs #19887), which is stuck until I get PU assistance with a couple of nits on it.

@jmprieur
Copy link
Copy Markdown
Contributor

@guardrex : what I don't fully understand is why you don't start from a project generated with dotnet new blazorserver --auth SingleOrg --calls-graph ? (these are your templates)

Then for the groups, did you manage the overflow?
https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2/blob/8189f7ccf02b59de67e5d1ffe8404000830de510/5-WebApp-AuthZ/5-2-Groups/Startup.cs#L51

@guardrex
Copy link
Copy Markdown
Collaborator Author

guardrex commented Oct 23, 2020

@jmprieur ... This is a special case scenario for WASM hosted-generated apps (blazorwasm --hosted). Also, we don't want an on-behalf-of the user call to Graph API where the user/client app has Directory.Read.All permission (scope) for the server API app call to Graph API. We only want the user/client to have a permission/scope for the server API. The server API has the permission/scope for Graph API and Directory.Read.All permission. This is a special case scenario and not in the main Blazor WASM security docs. It only appears in the AAD groups and roles topic and only for apps that need to set up auth policies for controller endpoints. It's an edge case for sure, but I wanted to cover it for completeness here with the rest of our AAD groups and roles coverage. We had a reader ask for it, and I thought it was a good idea to have it.

Yes! on handling the presence of a hasgroups claim ... overflow is under control here and demonstrated in our code examples.

... and btw --- Javier and Safia ... and perhaps more PU folks ... will put an 👁️ on everything before we hit 5.0 GA. This won't be the last review of this, so we might make further changes. I'm just trying to get the draft coverage in place, and then we can take it from there.

@guardrex guardrex marked this pull request as ready for review October 26, 2020 14:10
@guardrex
Copy link
Copy Markdown
Collaborator Author

@captainsafia ... I think we're ready here. I've looked over the Graph API/SDK docs again this morning. I reverted the code on this PR back to making the AcquireTokenForClient request in Startup.ConfigureServices because the token granted to a daemon app does expire and must be refreshed. According to Azure docs, an application (not user) token cache works behind-the-scenes obtaining a new token if needed and storing them for future use. It all comes pretty much straight out of the Azure docs for the daemon app. I reiterate that this is only for the AAD groups and roles topic and only for a server API (web API) that needs to independently set up auth policies for controller endpoints without on-behalf-of user requests to Graph API and without additional permission/scopes for users granted to the client app. The user only ever has a permission/scope for the server API in this scenario.

WRT Javier's Graph code, that's on the PR. That code has been placed into a new Graph API topic. That code is also used in the AAD groups and roles for the Graph SDK approach in the client-side app.

I've added the VS NOTE that DR recommends for 5.0, which we can remove later when VS releases (I'll open a tracking issue so that I don't forget). The wording of this NOTE probably needs a bit of help. You'll see these NOTEs at the tops of the Standalone and Hosted AAD topics. He said 'organization' account, so I assumed that he means multi-tenant apps created by VS are ok (i.e., B2C). If not tho, let me know 👂, and I'll add the NOTE to the Standalone and Hosted AAD B2C topics.

AFAIK, we're sticking to the plan to have a final Javier (+probably others) review everything prior to GA from the live preview topics.

Copy link
Copy Markdown
Contributor

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not done with the review but figured I'd post what I have at the moment. Will circle back with more feedback later.

Comment thread aspnetcore/blazor/security/webassembly/azure-active-directory-groups-and-roles.md Outdated
Comment thread aspnetcore/blazor/security/webassembly/azure-active-directory-groups-and-roles.md Outdated
Comment thread aspnetcore/blazor/security/webassembly/azure-active-directory-groups-and-roles.md Outdated
Comment thread aspnetcore/blazor/security/webassembly/azure-active-directory-groups-and-roles.md Outdated
Comment thread aspnetcore/blazor/security/webassembly/azure-active-directory-groups-and-roles.md Outdated
Comment thread aspnetcore/blazor/security/webassembly/azure-active-directory-groups-and-roles.md Outdated
Comment thread aspnetcore/blazor/security/webassembly/azure-active-directory-groups-and-roles.md Outdated
Comment thread aspnetcore/blazor/security/webassembly/azure-active-directory-groups-and-roles.md Outdated
Comment thread aspnetcore/blazor/security/webassembly/azure-active-directory-groups-and-roles.md Outdated
@guardrex
Copy link
Copy Markdown
Collaborator Author

Thanks for your help @jmprieur ... I think we're ready to move forward here. This content and these examples will be reviewed again by Javier (and others) later but prior to GA, so we'll fix them up further from here.

btw --- I did notice one thing in passing when I was looking at the Groups sample that you cross linked. The example has app settings for the group memberships that say ...

Enter the objectID for GroupAdmin group copied from Azure Portal
Enter the objectID for GroupMember group copied from Azure Portal

https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2/blob/8189f7ccf02b59de67e5d1ffe8404000830de510/5-WebApp-AuthZ/5-2-Groups/appsettings.json

Focusing on the remark "copied from Azure Portal" from the file, I don't think those are in the portal, nor are they published anywhere in Azure docs that I can find. I've tried a few times to track them down, including speaking with Kyle Marsh on #18924. I never found them, so we published them over here at ...

https://docs.microsoft.com/aspnet/core/blazor/security/webassembly/azure-active-directory-groups-and-roles#aad-administrator-role-object-ids

I don't think this is the best spot for these. I think they should be in the Azure docs, but I'm not sure how to address it further. Let me know if you think I should take some additional action on it.

@guardrex
Copy link
Copy Markdown
Collaborator Author

Will circle back with more feedback later.

I'll wait until ur done and then resolve everything at once.

Comment thread aspnetcore/blazor/security/webassembly/graph-api.md Outdated
@guardrex
Copy link
Copy Markdown
Collaborator Author

@captainsafia ... Looks good thus far. I'm going to go ahead and merge now and take live to get us beyond half-baked 5.0 preview topics. I intend to go over it all again by the end of this week and probably make some further nit updates.

FYIs on the latest updates:

  • I addressed your feedback, but I couldn't always do it in the way that you suggested. For example, we can't use directional terms in topics (e.g., "before," "after," etc.), and we mostly avoid future tense. I made changes in most cases along the lines of what u suggested.
  • Some of your suggestions just led to merely killing off 🔪 non-essential language, which worked out well to focus the text.
  • I caught myself writing "permission (scope)" or "permissions (scopes)" too much. This happened because the Azure portal overloads the concept with both terms. I refactored the PR a bit and mostly just go with "scope" and "scopes." It streamlines the text and is a bit more technically specific. I keep the word "permission" in a few important Azure portal-focused text.

I have one ❓ on the code that bugs me ...

In the new Graph API topic, AddGraphClient conveniently takes an array of scopes ...

public static IServiceCollection AddGraphClient(
    this IServiceCollection services, params string[] scopes)
builder.Services.AddGraphClient("https://graph.microsoft.com/User.Read");

However, AuthenticateRequestAsync has a hard-coded scope ...

new AccessTokenRequestOptions()
{
    Scopes = new[] { "https://graph.microsoft.com/User.Read" }
});

I wish it could be refactored so that AccessTokenRequestOptions receives the scope array passed to AddGraphClient. I'm not confident that it's possible/reasonable given that Javier didn't just set it up that way in the first place. Is it possible/reasonable? If so, what would that look like?

@guardrex guardrex merged commit f0763c2 into master Oct 27, 2020
@guardrex guardrex deleted the guardrex/blazor-wasm-security branch October 27, 2020 12:25
@scottaddie scottaddie changed the title Blazor with Graph API Move the Blazor WebAssembly with Graph API content to a new doc Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Blazor] Call graph API using graph client SDK [RC1] Microsoft Identity Platform 2.0 for Blazor

3 participants