search

dotnet / blazor-samples

Creative Commons Attribution 4.0 International
711 stars 608 forks source link

.NET 8 Blazor OIDC examples still using .NET 7 code base #194

Closed mnj closed 8 months ago

mnj commented 8 months ago

Describe the issue The examples of using Blazor with .NET 8, still seems to be using the old fashioned .NET 7 way, which isn't recommended by: https://learn.microsoft.com/en-us/aspnet/core/migration/70-80?view=aspnetcore-8.0&tabs=visual-studio#migrate-the-cascadingauthenticationstate-component-to-cascading-authentication-state-services

For example: https://github.com/dotnet/blazor-samples/blob/main/8.0/BlazorWebAppOidc/BlazorWebAppOidc.Client/Routes.razor https://github.com/dotnet/blazor-samples/blob/main/8.0/BlazorWebAppOidcBff/BlazorWebAppOidc.Client/Routes.razor

It makes it really hard to figure out how we are supposed to be able to use Entra ID with the new way of using Blazor in .NET 8.

It boils down this this orignal issue: https://github.com/dotnet/aspnetcore/issues/51202

None of the suggested solutions in that issue really work properly, with who knows what security risks, when everyone has to reinvent some way of managing authentication/token, because there has been zero guidance from MS for anything but "individual accounts" in Blazor Web Apps for .NET 8.

Issue Details

⚠ Do not edit this section. It is required for issue processing.

guardrex commented 8 months ago

Hello @mnj ... BTW ... Not that it will help here, but the article that goes with the sample apps just went live a few minutes ago at ...

https://learn.microsoft.com/aspnet/core/blazor/security/server/blazor-web-app-with-oidc

I just wanted to call that out in passing. It doesn't address your question directly.

WRT that migration guidance, we perhaps were too strong 💪 there and made a mistake in the way that we phrased that guidance. Client-side, I think that's fine. IIRC, it was server-side in a BWA that it doesn't work properly 💥. Let me check on that and get back to you. If I need to make an update, I'll open a new issue on the docs repo from that article, and I'll ping you on the PR when it goes up.

WRT your other remarks on Entra and BWA with auth via a project template experience, the changes that had to be made for BWA ... for the whole paradigm of BWA ... took a monster effort to even get to this point (.NET 8 GA). My armchair quarterback view is that they did everything that they possibly could for the .NET 8 release. Like most teams in most organizations, they have a limit on staffing, time, and 💰. As you can see, they intend to improve the BWA project template with auth experience, and that will be for .NET 9 (November of this year).

What we have right now is only the OIDC sample apps, which you can refactor for MS Identity Web if you wish. Damien Bowden has a sample up here that you can look at https://github.com/damienbod/Hostedblazor8MeID/tree/main/BlazorWebMeID, which isn't supported by Microsoft, but it's probably a safe example to study because he's a trusted security expert and has been interacting with the MS engineers for years. I just heard this morning that the Azure team is going to create one or more BWA + Microsoft Identity Platform (Web) + Azure hosting sample apps, possibly with tutorial guidance. That's in the planning stages as far as I know, so it would probably be several weeks or months before it appears.

For now, you can use the OIDC sample with or without the BFF pattern. These samples are supported by the ASP.NET Core team. If you run into a problem, you can file a support request on their repo at ...

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

You might want to cc: @guardrex in the OP of any issues that you open to let me know about it for possible docs work.

For now ... leave this issue open for a bit ... I'm wrapping up for the day, but I'd like to take a closer look at that migration guidance and see if we went too far there. I think for client-side scenarios, that wrapping the Router is fine. I'll check on that and get back to you tomorrow (Thursday).

guardrex commented 8 months ago

UPDATE: I decided it best to email the PU engineers about the migration guidance. They'll let me know sometime today. I'll post back here what they say.

mnj commented 8 months ago

Hey,

Thank you for looking into it.

Yep, but I don't think most of us really need the templates as such, but any kind of documentation would be helpful for how to actually implement it in the "modern" way with Blazor/.NET 8, since I could imagine quite a few developers of enterprise-related apps will need Entra ID (or one of the other IdP solutions).

I looked at Damien Bowden's repo (https://github.com/damienbod/Hostedblazor8MeID/blob/main/HostedBlazorMeID/Client/App.razor), but not the one you linked, the Hosted BWA feels like it's mostly a legacy .NET 7 codebase, made to run on .NET 8, than using the listed best practices for Blazor Web Apps on .NET 8. The one you linked looks better ill take a look at that thanks. (What I'm personally after is a (.NET Core) Hosted BWA with Interactive WASM, not interactive Server/Auto, but where the WASM part can call external APIs with bearer tokens from Entra ID, which might be a bit specific I know :)).

guardrex commented 8 months ago

These OIDC apps work fine with Entra as they are; but in theory, you can update either of the OIDC sample apps to use Microsoft Identity Web, and it should be fairly close to what Damien has.

At this point, the product unit isn't going to provide additional samples for this repo. The next batch of samples would be in the Azure docs, which are handled by a different team. I'm not sure exactly what their plans are other than they do plan to provide BWA with MS Identity Platform/Entra sample(s), perhaps with a tutorial article. It will be for this area of their docs ...

https://learn.microsoft.com/entra/identity-platform/tutorial-blazor-server

... and that's the Blazor Server tutorial, which of course is last year's model 😄.

You could try searching their doc repo for issues related to BWAs to see if they filed one or more issues for the work that they plan to do. If you don't see any issues there, you can open an issue there to ask them their plans and what their timeline looks like ...

https://github.com/MicrosoftDocs/entra-docs/issues

If you open an issue to ask them, please cc: @guardrex in your OP so that I can see what they say. I've only heard a rough sketch of their plans via an internal email convo.

WRT ...

BWA with Interactive WASM, not interactive Server/Auto, but where the WASM part can call external APIs with bearer tokens from Entra ID

WRT my next round of work here ...

I'm still waiting to hear back on the question about the migration guidance. I think we were too strong in that language. I think it should be limited to upgrading from Blazor Server to BWA, specifically only in the server project of the BWA. AFAIK, that's the scope of that migration guidance ... nothing else than that. However, we'll have to get a ruling on that from one of the PU engineers, probably Stephen Halter.

guardrex commented 8 months ago

Stephen answered ... and HERE it is .... 🥁🥁🥁🥁 roll plz! ..........

IIRC, the motivation for recommending calling AddCascadingAuthenticationState over the CascadingAuthenticationState component in the templates is because the cascading authentication state needs to be available in both the server and client (i.e. everywhere) projects, but the CascadingAuthenticationState component needs to wrap the Router component which is only available on the server when there’s per-page interactivity.

In the new BWA+OIDC sample I worked on, I used global interactivity which puts the Router component in the client project meaning I could wrap it with the CascadingAuthenticationState component in one place and make the Task<AuthenticationState> available on both the server in client. I thought this was a little cleaner than calling AddCascadingAuthenticationState in Program.cs of both projects.

When there’s per-page interactivity, you must call AddCascadingAuthenticationState in the client project since there’s no Router component to wrap. In the server project, you could use the CascadingAuthenticationState component instead of calling AddCascadingAuthenticationState, but you must do one or the other. I went with CascadingAuthenticationState in the template to be consistent with the client project.

This distinction between projects with global vs. per-page interactivity is very nuanced, and not very easy to explain. I think the guidance to always call AddCascadingAuthenticationState in both projects is fine even if it’s a little extra code in some cases. We can update the BWA+OIDC samples to follow this guidance so as not to confuse people.

Soooooo ..... my conclusions thus far are ....

In the meantime, we'll see what Dan says and take this forward from there. I think we'll end up using this issue for changes to the sample apps, so leave this issue open for now.

guardrex commented 8 months ago

Dan has RULED! 👨‍⚖️

... and the ruling is that we'll leave the migration guidance alone [also note that it's documented in the server security overview article] and the OIDC sample apps will call AddCascadingAuthenticationState in Program.cs of both projects.

He didn't elaborate, but the idea here is just that it's simpler that way 😄 than trying to remember these factoids about the shape of the app's render mode adoption.

I'll take care of the update tomorrow morning.

WRT looking into CSR, I'll get to that ASAP, but I have a lot of stuff going on right now, so it might not be this week.

guardrex commented 8 months ago

~I might need another day on this one. I'll try to square it away tomorrow morning. This morning (Thursday) was burned on other high priority issues. I'll get back to this ASAP ⛰⛏😅.~

No ... wait ... I do have a few minutes left on the clock here. I'm going to try and address the cascading auth state aspect now.

I'll circle back to this for CSR testing shortly.

guardrex commented 8 months ago

Good news ... I was able to apply Interactive WebAssembly in my test app (to the UserClaims component in my test) following the updates to addess this issue. All good 👍 ... No further action required AFAICT. Thanks again for the issue!