Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Offline discussion updates for 8.0 #31693

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Feb 5, 2024

Fixes #31688
Addresses #28161

Items to be worked on separate issues

Configure and use gRPC in components section

Dan ... You said you'd provide a code example, but it 🤔 seems like the current code is ok. The updates to the section are on the DIFF, so you can see the current code there. The focus of the updates thus far is just to 🔪 OUT the little bits of text that pertain to hosted WASM and updating the versioning to return the content for 8.0+. Let me know if I should 🛑 HOLD this PR for new or additional code (or open a separate issue for it). 👂👂👂

Secure a SignalR hub section

Dan asks Stephen/Jeremy ...

Should we be recommending using the auth cookie instead? Are there any CSRF concerns here that would then need to be addressed?

The updates to the section are on the DIFF, so the current code is there for inspection. Thus far ... again ... the updates on the PR just focus on 🔪 OUT the little bits pertaining to hosted WASM and updating the versioning to return the content for 8.0+.

Skipped bits

Everything else that we discussed that isn't on the PR doesn't require updates because the current coverage is good.


Internal previews

📄 File 🔗 Preview link
aspnetcore/blazor/host-and-deploy/index.md Host and deploy ASP.NET Core Blazor
aspnetcore/blazor/javascript-interoperability/call-dotnet-from-javascript.md Call .NET methods from JavaScript functions in ASP.NET Core Blazor
aspnetcore/blazor/javascript-interoperability/call-javascript-from-dotnet.md Call JavaScript functions from .NET methods in ASP.NET Core Blazor
aspnetcore/blazor/javascript-interoperability/index.md ASP.NET Core Blazor JavaScript interoperability (JS interop)
aspnetcore/blazor/security/webassembly/additional-scenarios.md aspnetcore/blazor/security/webassembly/additional-scenarios
aspnetcore/blazor/security/webassembly/index.md Secure ASP.NET Core Blazor WebAssembly
aspnetcore/blazor/tutorials/signalr-blazor.md [Visual Studio](https://review.learn.microsoft.com/en-us/aspnet/core/blazor/tutorials/signalr-blazor?branch=pr-en-us-31693)

:::moniker range=">= aspnetcore-8.0"

> [!WARNING]
> Only place a `<script>` tag in a component file (`.razor`) if the component is guaranteed to adopt [static server-side rendering (static SSR)](xref:blazor/fundamentals/index#client-and-server-rendering-concepts) because the `<script>` tag can't be updated dynamically.
Copy link
Member

Choose a reason for hiding this comment

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

@javiercn Is this warning worded strong enough for you? I remember you complaining about people trying to render <script> tags in triage.

Copy link
Member

Choose a reason for hiding this comment

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

Seems fine to me. The reason for not using script tags in interactive components is clearly stated.

@guardrex
Copy link
Collaborator Author

guardrex commented Feb 12, 2024

Thanks, @halter73 ... but you didn't answer Dan's question in the opening remarks ☝️ (unless by your approval you're saying 'don't sweat CSRF here 😎').

@halter73
Copy link
Member

I think we should recommend using an auth cookie because that's the only way to prerender content that requires authentication.

As for the CSRF concerns, I think we should just link to https://learn.microsoft.com/en-us/aspnet/core/security/anti-request-forgery?view=aspnetcore-8.0. All Blazor endpoints that accept form posts that might be susceptible to CSRF attacks already require an antiforgery token as mentioned in https://learn.microsoft.com/en-us/aspnet/core/blazor/security/?view=aspnetcore-8.0#antiforgery-support, so I don't think there's much to add there. Pinging @javiercn @blowdart in case they disagree

Other types of endpoints that accept form posts either require antiforgery tokens by default (like razor pages, controllers with views, and minimal APIs) or have been long documented not to (like AddControllers without views). And the need for antiforgery checks for these other types of endpoints is unrelated to whether or not they are hosted along with a Blazor app.

@guardrex
Copy link
Collaborator Author

guardrex commented Feb 12, 2024

I think we should recommend using an auth cookie because that's the only way to prerender content that requires authentication.

It will be a bit out-of-sync with our SignalR guidance ...

Cookies are specific to browsers. Sending them from other kinds of clients adds complexity compared to sending bearer tokens. Cookie authentication isn't recommended unless the app only needs to authenticate users from the browser client. Bearer token authentication is the recommended approach when using clients other than the browser client.

Cross-ref: https://learn.microsoft.com/en-us/aspnet/core/signalr/authn-and-authz?view=aspnetcore-8.0#cookies-vs-bearer-tokens

We seem to have some cookie bits at ...

https://learn.microsoft.com/en-us/aspnet/signalr/overview/security/hub-authorization#cookie

@halter73 ... Do you want me to try to 🦖 RexHack™ 🙈 some kind of approach based on that, or do we want a real programmer to build out what we'll show for the cookie approach in the SignalR doc?

BTW ... This is the current code that we show is, which I suppose we can maintain for standalone WASM apps ...

@using Microsoft.AspNetCore.Components.WebAssembly.Authentication
@inject IAccessTokenProvider TokenProvider
@inject NavigationManager Navigation

...

var tokenResult = await TokenProvider.RequestAccessToken();

if (tokenResult.TryGetToken(out var token))
{
    hubConnection = new HubConnectionBuilder()
        .WithUrl(Navigation.ToAbsoluteUri("/chathub"), 
            options => { options.AccessTokenProvider = () => Task.FromResult(token?.Value); })
        .Build();

    ...
}

@guardrex
Copy link
Collaborator Author

guardrex commented Feb 12, 2024

🦖 NOTE TO SELF

After we finish the work for securing a SignalR hub with a cookie, update the cross-links of the SignalR-Blazor tutorial for 8.0+, which currently only cross-links to the bearer token approach.

Resolved! ...

We already had the link to the Client-side SignalR cross-origin negotiation for authentication section in the Blazor-SignalR tutorial.

@halter73
Copy link
Member

I think it's fine to continue recommend tokens for cases where there isn't prerendering like standalone WASM or non-browser apps. In those kind of apps, you don't have a browser window to login using Razor pages/components.

You can have a .NET HttpClient login using cookies with MapIdentityApi endpoints, but I think it's fair to say if you don't have a browser to do an OIDC flow or sign in using Razor pages/components, then bearer tokens are the better option.

If you're up for it, I'm more than happy to review 🦖 RexHack™ version of cookie based SignalR auth using a .NET client. I expect instead of options.AccessTokenProvider you'd want to use HttpMessageHandlerFactory and configure it with request.SetBrowserRequestCredentials(BrowserRequestCredentials.Include) as demonstrated in https://learn.microsoft.com/en-us/aspnet/core/blazor/fundamentals/signalr?view=aspnetcore-8.0#client-side-signalr-cross-origin-negotiation-for-authentication

@guardrex
Copy link
Collaborator Author

@halter73 ... Indeed! I forgot that section was there. It seems to me that we can just cross-link that from the security article, as I just set up on ...

fd91e23

WRT ...

Is this warning worded strong enough for you? I remember you complaining about people trying to render <script> tags in triage.

I already conversed with @javiercn on that point, and that's about what he told me to write into the docs.

WRT anything else ... @danroth27 / @mkArtakMSFT ... are you OK with going ahead and getting this merged?

@danroth27
Copy link
Member

Dan ... You said you'd provide a code example, but it 🤔 seems like the current code is ok.

It looks like the gRPC sample code isn't used to initialize any component state, so yeah, I think it's fine as is. No need to persist and use prerendered state.

Copy link
Member

@danroth27 danroth27 left a comment

Choose a reason for hiding this comment

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

LGTM

@guardrex guardrex merged commit 3d86df3 into main Feb 14, 2024
3 checks passed
@guardrex guardrex deleted the guardrex/blazor-80-offline-discussion-updates branch February 14, 2024 10:58
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 8.0 offline discussion updates
3 participants