dotnet / AspNetCore.Docs

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

The syntax in CustomAccountFactory is incorrect or has changed in GraphAPI 5 #33401

Closed Echostorm44 closed 2 months ago

Echostorm44 commented 2 months ago

Description

This:

var memberships = await requestMemberOf.Request().GetAsync();

if (memberships is not null)
{
    foreach (var entry in memberships)
    {
        if (entry.ODataType == "#microsoft.graph.group")
        {
            userIdentity.AddClaim(
                new Claim("directoryGroup", entry.Id));
        }
    }
}

should be

var memberships = await requestMemberOf.GetAsync();

if (memberships is not null)
{
    foreach (var entry in memberships.Value)
    {
        if (entry.OdataType == "#microsoft.graph.group")
        {
            userIdentity.AddClaim(
                new Claim("directoryGroup", entry.Id));
        }
    }
}

Page URL

https://learn.microsoft.com/en-us/aspnet/core/blazor/security/webassembly/microsoft-entra-id-groups-and-roles?view=aspnetcore-8.0

Content source URL

https://github.com/dotnet/AspNetCore.Docs/blob/main/aspnetcore/blazor/security/webassembly/microsoft-entra-id-groups-and-roles.md

Document ID

4088c5ca-39c2-9bed-ebc5-683d3441b615

Article author

@guardrex

github-actions[bot] commented 2 months ago

😎🏖️🌴 Summertime! 🏐🏄‍♀️🦩

Stand by! A green dinosaur 🦖 (guardrex) will arrive shortly to assist.

guardrex commented 2 months ago

Thanks @Echostorm44 ... I'll take a look tomorrow morning (Tuesday).

BTW ... Code here in a block is triple-backticks above and below. ~I'll edit your comment to fix it.~ UPDATE: Done! 👍

Yes, I wouldn't be surprised if it was a change in the SDK. The original code was tested before publication.

Echostorm44 commented 2 months ago

@guardrex Thanks. I was thinking about this last night. Have you guys ever thought about rigging up the code samples with some kind of syntax checker / snippet compiler thing? Obviously they couldn't compile but if it knew the framework and nuget packages it could probably raise an alert that certain classes and methods don't exist.

guardrex commented 2 months ago

First off BTW ... There's more to change than just your initial report :point_up:. There is additional new SDK API, possibly with some different Azure portal guidance, to update in the article. I'm going to work on this today and should have it resolved completely by EOD, depending on if I run into any 😈 along the way.

WRT the article/code: I've resisted carrying a sample app for some of these non-Blazor scenarios because sample apps add overhead ... costs 💰, and I'm always swamped with work here. The source of truth for Graph is really supposed to be the Microsoft Graph docs. The problem was that it was hard to see from their docs how to implement their API in a WASM app.

We don't have a syntax checker outside of using an actual sample app. I'll probably add a sample app for this today. It's just that we've now blown up the number of samples to 14 ... this will be 15. It's quite a lot of samples. However, Blazor is now Microsoft's primary recommendation for all new ASP.NET Core development, recommended over Razor Pages and MVC. Therefore, it makes sense to give devs sample apps for the key scenarios, and Azure/Graph are very important to the company.

Echostorm44 commented 2 months ago

Totally get it. I'm glad you're on the case. It would be nice if they created more robust templates for each security scenario in Blazor. I know it would be more than 6 but we've lost so much dev mindshare over the years to mobile apps then more recently to python. Ballmer said it best, Developers, Developers, Developers. It should be much easier to spin up a Blazor app and have the security working perfectly after filling in a couple blanks and a beautiful couple starter pages with navigation and theming setup and ready to expand. That way, you'd just have to spin up the templates and make sure they still build.

guardrex commented 2 months ago

I think that's especially true now that we're pushing Blazor as the premier 🥇 ASP.NET Core app development technology.

I'm in the process of placing a new sample app and updating the guidance. There's a backstory to our article versioning that I won't bore you with, but we're kind'a stuck these days with crappy article versioning. The best thing for me to do is take the current article and make it for <8.0 and create a new article for >=8.0. The new article will drop Graph SDK 4 and all of the content about hosted Blazor WebAssembly, which as you probably know was dropped as a project template at 8.0. By creating a new article, I can clean up this very messy article that I'm working with. Still tho, the existing article will remain live under the doc version selector for ASP.NET Core 5.0 to 7.0, and it will have the updated Graph 5.0 content. The new streamlined >=8.0 article will be easier for devs to consume.

Bascially, the code update for the factory will go like this for groups ...

var graphGroups = await requestMemberOf.GraphGroup.GetAsync();

if (graphGroups?.Value is not null)
{
    foreach (var entry in graphGroups.Value)
    {
        if (entry.Id is not null)
        {
            userIdentity.AddClaim(
                new Claim("directoryGroup", entry.Id));
        }
    }
}

... and I'm going to start telling readers to set the groupMembershipClaims attribute in the app's manifest to DirectoryRole because the Graph call will take care of the security groups. There's no need to send those down in a separate claim in the token.

I also noticed some NITs 😈 to fix, such as the version for the base URL of Graph. I'm going to clean that up a bit and fix it up. It will go with code like this ...

var baseUrl = string.Join("/", 
    builder.Configuration.GetSection("MicrosoftGraph")["BaseUrl"], 
    builder.Configuration.GetSection("MicrosoftGraph")["Version"]);
var scopes = builder.Configuration.GetSection("MicrosoftGraph:Scopes")
    .Get<List<string>>();

builder.Services.AddGraphClient(baseUrl, scopes);

... in Program.cs.

... and ...

private readonly string? baseUrl = string.Join("/",
    config.GetSection("MicrosoftGraph")["BaseUrl"],
    config.GetSection("MicrosoftGraph")["Version"]);

... in the factory.

wwwroot/appsettings.json:

{
  "MicrosoftGraph": {
    "BaseUrl": "https://graph.microsoft.com",
    "Version": "v1.0",
    "Scopes": [
      "user.read"
    ]
  }
}

... instead of just setting the base URL with the version (i.e., "BaseUrl": "https://graph.microsoft.com/v1.0",). That then matches what I do in the Graph API article.

I still think this work will all be done within a few hours, and I'll publish it live today as well.

Echostorm44 commented 2 months ago

Very good! I'm glad to see someone so energetic on this. I'm hoping the new templates that ship for 9 are equally both simplified and with more scenarios covered.

guardrex commented 2 months ago

I made some nice major updates this morning. I'll have them posted shortly.

hoping the new templates that ship for 9 are equally both simplified and with more scenarios covered.

I don't think for Blazor WebAssembly there will be many changes. AFAIK, Halter is working on BWA template security updates. The security aspects will be improved ... more built-in with less boilerplate code in the app IIRC.

Echostorm44 commented 2 months ago

Taking out the IdentityServer4 stuff was a great start but it still takes way too much effort to spin up a Blazor app with even a common security setup. I want Blazor to succeed and for C# to stand over Python's grave but there is still a lot that could be done to make the dev experience more appealing to new and existing devs. I'm glad you're here doing your part.

guardrex commented 2 months ago

Thanks ... and you can submit your ideas to the product unit if you think they're missing some things when 9.0 reaches GA ( https://github.com/dotnet/aspnetcore/issues).

Ok ... I've updated the sample app and the article.

The new bits are to use Graph SDK's new directory roles and administrative unit API. Bringing the built-in admin roles down that way (along with using the SDK to get group membership) means that the manifest doesn't need to be edited to return wids claims. Wids has been completely removed from the article.

I'm putting final touches on it now, and I think I'll merge it today ... with possibly a patch PR tomorrow for any NITs 😈 that I discover after publication.

guardrex commented 2 months ago

It's live now ...

https://learn.microsoft.com/en-us/aspnet/core/blazor/security/webassembly/microsoft-entra-id-groups-and-roles

If you find any bugs, please open a new issue from the bottom of the article. Thanks again, @Echostorm44! 🚀