dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.38k stars 10k forks source link

MicrosoftIdentity URLs in Blazor Server for Azure AD authenticated apps #30000

Closed mayur-ekbote closed 3 years ago

mayur-ekbote commented 3 years ago

Somewhere between .net core 5.0.0 to 5.0.2 and Microsoft Microsoft.Identity.Web 1.3.0 and 1.5.1 the signout URL for Azure AD authenticated apps changed from AzureAD/Account/SignOut to MicrosoftIdentity/Account/SignOut

I could not find any documentation that describes this change. Please add it somewhere.

javiercn commented 3 years ago

@mayur-ekbote thanks for contacting us.

@jmprieur I don't think this changed in ASP.NET Core, did this change in Microsoft.Identity.Web?

javiercn commented 3 years ago

@guardrex do we own the have specific docs for Blazor Server regarding Microsoft.Identity.Web integration?

guardrex commented 3 years ago

We rely on the guidance in the main doc set's Security and Identity node and two Azure/Identity tutorials ...

WRT our main doc set's Security and Identity node coverage, our docs were updated to cover the Identity v2.0 changes.

btw- Since this is potentially one of the deltas for moving from 3.x to 5.x, the Migration topic is in play at ...

https://docs.microsoft.com/en-us/aspnet/core/migration/31-to-50?view=aspnetcore-5.0&tabs=visual-studio

... and this issue pertaining to paths is not mentioned in the migration coverage there.

mayur-ekbote commented 3 years ago

@guardrex This is not a 3.x to 5.x because I have a working instance of my application on .net 5.0.0 that works on the old sign out URL. I also can't find the original tutorial on Blazor Server authentication which used these URLs (I think it was authored by @danroth27 ). The new documentation contains only WebAPI driven approach which does not cover the blazor server scenario which uses only a combination of dependency injection and razor pages.

guardrex commented 3 years ago

I see. I couldn't tell from the opening remark if this was a change or not across versions.

The new documentation

We've always covered Blazor Server Identity the same way, so I'm not sure what you mean. We send readers into the Security and Identity node and the Azure/Identity docs for both web API and app auth because it should configure and behave like an RP/MVC app and we don't want to duplicate coverage and explode costs 💰 in doc maintenance. The things that we needed to cover are threat mitigation specific to Blazor Server and how to pass tokens to components.

I'll keep an 👂 open here for guidance on what/where to cover something.

javiercn commented 3 years ago

@guardrex that's fine, I just want to make sure that the request is routed to the right folks.

mayur-ekbote commented 3 years ago

Blazor apps don't necessarily follow MVVC pattern. In a blazor server app, authentication is adding AddXXX and UseXXX to the startup file and then adding authorization attributes wherever desired. (apart from the azure config/ app registration). There may or may not be any controllers. It would be great if the specific walkthrough around blazor server authentication is added.

Coming to the Identity platform documentation, this is only good for an overview: https://docs.microsoft.com/en-us/azure/active-directory/develop/tutorial-blazor-server

The part which talks about sign in and sign out assumes that it is a controller. https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-web-app-sign-user-sign-in?tabs=aspnetcore

But the blazor specific solution is actually found in the layout component of in Azure AD samples (that is where I found it at least): https://github.com/Azure-Samples/ms-identity-blazor-server/blob/main/WebApp-OIDC/MyOrg/blazorserver-singleOrg/Shared/LoginDisplay.razor

guardrex commented 3 years ago

Do you think that https://docs.microsoft.com/en-us/azure/active-directory/develop/quickstart-v2-aspnet-core-webapp covers it? What's missing?

blazor specific solution

For which aspect tho? ...

javiercn commented 3 years ago

@mayur-ekbote Blazor Server applications are considered traditional web applications for authentication purposes and follow the exact same pattern as an MVC/Razor application.

I believe that Microsoft.Identity.Web is implemented as an MVC area internally.

The only Blazor specific point to the solution is the login display component, all the other authentication concerns are handled in the exact same way as a classic ASP.NET Core application.

I don't think there's any specific Blazor docs that we need here, I think any doc requests fall upon Microsoft.Identity.Web to cover.

guardrex commented 3 years ago

I checked all of our docs for any link with MicrosoftIdentity/Account in them. I also checked for "asp-area="MicrosoftIdentity". No 🎲🎲. There's no coverage here for that change.

Covered here :point_right: https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-web-app-sign-user-sign-in?tabs=aspnetcore#sign-in-button

That's about it AFAICT :point_up: in either doc set outside of perhaps some floating samples for Cosmos, App Insights, and perhaps other Azure samples.

mayur-ekbote commented 3 years ago

Do you think that https://docs.microsoft.com/en-us/azure/active-directory/develop/quickstart-v2-aspnet-core-webapp covers it? What's missing?

blazor specific solution

For which aspect tho? ...

The AuthorizeView and related components come into effect only post-authentication/authorization and hence are independent of authentication providers. They do not cover the Login - Logout flow.

@mayur-ekbote Blazor Server applications are considered traditional web applications for authentication purposes and follow the exact same pattern as an MVC/Razor application.

I believe that Microsoft.Identity.Web is implemented as an MVC area internally.

Yes. When I said "controller" I meant the component that is requesting the logout. Not the one which is handling the request (i.e. Microsoft Identity package)

The only Blazor specific point to the solution is the login display component, all the other authentication concerns are handled in the exact same way as a classic ASP.NET Core application.

Actually, the login screen is provided by AzureAD (through the Identity platform's UI package I guess). And the logout flow needs some clarity.

I don't think there are any specific Blazor docs that we need here, I think any doc requests fall upon Microsoft.Identity.Web to cover.

Blazor code:

<AuthorizeView>
    <Authorized>
        Hello, @context.User.Identity.Name!
        <a href="MicrosoftIdentity/Account/SignOut">Log out</a>
    </Authorized>
    <NotAuthorized>
        <a href="MicrosoftIdentity/Account/SignIn">Log in</a>
    </NotAuthorized>
</AuthorizeView>

ASP.NET Core MVC code:

<ul class="navbar-nav">
  @if (User.Identity.IsAuthenticated)
  {
    <li class="nav-item">
        <span class="navbar-text text-dark">Hello @User.Identity.Name!</span>
    </li>
    <li class="nav-item">
        <a class="nav-link text-dark" asp-area="MicrosoftIdentity" asp-controller="Account" asp-action="SignOut">Sign out</a>
    </li>
  }
  else
  {
    <li class="nav-item">
        <a class="nav-link text-dark" asp-area="MicrosoftIdentity" asp-controller="Account" asp-action="SignIn">Sign in</a>
    </li>
  }
</ul>

You can argue that both codes are semantically equivalent (they are). But I strongly feel that it helps if the documentation does not assume prior knowledge (in this case it assumes knowledge of asp.net views).

guardrex commented 3 years ago

Thanks for explaining it out. I understand better now what your ask is.

@javiercn, I can float a PR to cover this in the existing AuthorizeView content for LoginDisplay easily enough ... it's just a few lines and the LoginDisplay component added to the bottom of that AuthorizeView section. Let me know :ear:.

In the meantime, I'll open a docs issue to take a look at scaffolding coverage for Identity v2.0 as a separate, but related, matter.

guardrex commented 3 years ago

@mayur-ekbote ... Which project template gave you "MicrosoftIdentity"? I'm not getting that in 5.x or 3.x, and I'm not trying to use an earlier Identity v2.0 package. We usually don't document patch changes anyway.

<AuthorizeView>
    <Authorized>
        <a href="Identity/Account/Manage">Hello, @context.User.Identity.Name!</a>
        <form method="post" action="Identity/Account/LogOut">
            <button type="submit" class="nav-link btn btn-link">Log out</button>
        </form>
    </Authorized>
    <NotAuthorized>
        <a href="Identity/Account/Register">Register</a>
        <a href="Identity/Account/Login">Log in</a>
    </NotAuthorized>
</AuthorizeView>

... and that's a good thing, too, because it seems like scaffolding coverage is ok (thus far 🤞) on that point. I'll still need to review all of that coverage and probably make updates to it on that issue that I opened.

... and we can still cover the LoginDisplay component as I proposed.

mayur-ekbote commented 3 years ago

This one: https://github.com/Azure-Samples/ms-identity-blazor-server

guardrex commented 3 years ago

I see .........

Microsoft.Identity.Web.ProjectTemplates

We won't be covering that. That's up to them to cover their own project templates. Basing an app on our Blazor Server template with Identity is going to use "Identity"-based URL segments and area in an app.

guardrex commented 3 years ago

@javiercn I'm going to open that separate issue to do a little something with LoginDisplay in the AuthorizeView section because it's hosting model agnostic and really just a by-convention component to manage the UI for auth state (context and links) (unless you disagree, of course). AFAICT, you can close this issue. I'll take care of this on the docs side.

mayur-ekbote commented 3 years ago

@guardrex I am unaware of how internal structures within .net core community work. So I am not sure who 'them' are (Identity platform?). But it would be great if there is a cross-link between blazor docs and Azure AD/B2C docs. That would help developers to discover solutions faster.

guardrex commented 3 years ago

We're somewhat compartmentalized here, and there isn't strong coordination between what we're doing in our docs and what they're doing AFAIK. It might be coordinated at a higher level tho ... via my supervisors.

The docs are split for this subject matter. We do cross-link to quite a bit of their docs and samples. We try not to step on each other too much (i.e., show different approaches for the same app specifications and requirements), and we try not to duplicate a lot of coverage because that's obviously expensive 💰 $$$ 💰 to maintain.

It's unusual to see Azure docs use separate project templates. We don't want to cross-link those. That's confusing imo. We cover what we'd like devs to do in ...

https://docs.microsoft.com/aspnet/core/blazor/security/server/#blazor-server-project-template

... including in those non-VS tabs for using the CLI for VS Code and working entirely from the CLI with some other editor.

I'm not sure who in the Azure product unit (or Azure docs) created those project templates. We only document our templates that are installed via the SDK. You could ask Shama-K on a new issue in that repo (Azure-Samples/ms-identity-blazor-server) about covering the difference between that project template's area links and the normal "Identity"-based area. I think they should cover the delta exclusively given that those templates are owned+maintained by them and are using an approach that doesn't match what any templates are doing here.

javiercn commented 3 years ago

@jmprieur can you handle the doc request ask here in the Microsoft.Identity.Web docs?

jmprieur commented 3 years ago

Thanks for the heads-up, @javiercn. I'll have a look

cc: @jennyf19

ghost commented 3 years ago

Thank you for contacting us. Due to a lack of activity on this discussion issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue.

This issue will be locked after 30 more days of inactivity. If you still wish to discuss this subject after then, please create a new issue!