-
Notifications
You must be signed in to change notification settings - Fork 10.3k
[Blazor][Wasm] Adds a library for performing autentication in blazor webassembly applications using OIDC #18851
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
Conversation
...Components/Blazor/testassets/Wasm.Authentication.Client/wwwroot/css/open-iconic/FONT-LICENSE
Outdated
Show resolved
Hide resolved
e2df786
to
88e365f
Compare
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
...Blazor/WebAssembly.Authentication/src/Options/RemoteAuthenticationApplicationPathsOptions.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this PR is epic!
Even though I have posted literally one million comments, I want to emphasise that I am totally in favour of this happening, and in general am really happy with the overall approach you're taking. I strongly support this work being done and am glad you're doing it!
Also in case it got lost in the flood of comments above, I want to ask again for any further work on this PR to be added as regular extra commits (non-squashed, non-force-pushed) so that follow-up review can just cover the subsequent diffs. It's taken about 4 hours to read through the first time 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed things in individual commits. There are still things to address here that are listed here:
- Design
- AccessTokenResultStatus
- SignOutSessionStateManager
- Refactor
- Enums for remote authentication actions
- Enums for RemoteAuthenticationStatus
- Get rid of GetCurrentUser
- Cleanup GetCurrentUser
- Cache GetCurrentUser results
- Get the application paths in an agnostic way (Through a service?)
- Look into removing GetStaticWebAssetManifest
- Bug fixes
- Redirect to return url when user is not logged in.
I'm refactoring the area, will handle that at part of that. - Cleanup navigate to register
https://github.com/dotnet/aspnetcore/pull/18851/files#r376996189
Deferred for now, I believe I hit some issues with Identity and decided to do it this wait. But I will re-evaluate it. - Caching on GetAuthenticationState
- Redirect to return url when user is not logged in.
- Other
- File issue for
https://github.com/dotnet/aspnetcore/pull/18851/files#diff-b45fe29798979440f8a2fcda493077f6R7 - File issue for -> Provide RedirectToLogin component in the app template.
[Blazor] Provide redirect to login component in Microsoft.AspNetCore.Authorization #18979 - EnableTypeScriptNuGetTarget -> Remove to avoid typescript SDK import errors.
[Blazor][Wasm] Disable built-in typescript target #18980
- File issue for
I will go through the comments later today/tomorrow and resolve/answer comments as appropriate.
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/AuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/DefaultAuthenticationManager.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/testassets/Wasm.Authentication.Server/Wasm.Authentication.Server.csproj
Outdated
Show resolved
Hide resolved
src/Components/test/E2ETest/Microsoft.AspNetCore.Components.E2ETests.csproj
Outdated
Show resolved
Hide resolved
src/Components/Blazor/testassets/HostedInAspNet.Server/HostedInAspNet.Server.csproj
Show resolved
Hide resolved
0853f13
to
2e4dabc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the PR to address all the feedback, the only thing I haven't been able to do is the Enum switch as it breaks for some reason I can't tell (with a massive call stack), so I leave that as a follow-up.
...Blazor/WebAssembly.Authentication/src/Options/RemoteAuthenticationApplicationPathsOptions.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/Services/IAccessTokenProvider.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/Services/IAccessTokenProvider.cs
Outdated
Show resolved
Hide resolved
/// </summary> | ||
[Parameter] public RenderFragment LoginFragment { get; set; } = DefaultLoginFragment; | ||
[Parameter] public RenderFragment LogingIn { get; set; } = DefaultLogInFragment; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is also fixed in a later commit, but just in case it isn't:
[Parameter] public RenderFragment LogingIn { get; set; } = DefaultLogInFragment; | |
[Parameter] public RenderFragment LoggingIn { get; set; } = DefaultLogInFragment; |
/// </summary> | ||
[Parameter] public RenderFragment LoginCallbackFragment { get; set; } = DefaultLoginCallbackFragment; | ||
[Parameter] public RenderFragment CompletingLogingIn { get; set; } = DefaultLogInCallbackFragment; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Parameter] public RenderFragment CompletingLogingIn { get; set; } = DefaultLogInCallbackFragment; | |
[Parameter] public RenderFragment CompletingLoggingIn { get; set; } = DefaultLogInCallbackFragment; |
/// </summary> | ||
[Parameter] public RenderFragment LoggedOutFragment { get; set; } = DefaultLoggedOutFragment; | ||
[Parameter] public RenderFragment LogOutSucceded { get; set; } = DefaultLoggedOutFragment; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Parameter] public RenderFragment LogOutSucceded { get; set; } = DefaultLoggedOutFragment; | |
[Parameter] public RenderFragment LogOutSucceeded { get; set; } = DefaultLoggedOutFragment; |
/// An <see cref="RemoteAuthenticatorViewCore{TAuthenticationState}"/> that uses <see cref="RemoteAuthenticationState"/> as the | ||
/// state to be persisted across authentication operations. | ||
/// </summary> | ||
public class RemoteAuthenticatorView : RemoteAuthenticatorViewCore<RemoteAuthenticationState> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good now.
One thing I'd like to check to complete my understanding: do we already have use cases for needing to inherit directly from RemoteAuthenticatorViewCore<T>
elsewhere (e.g., for msal or something)? If we do then great, this all makes sense. If we don't yet have reasons to do that and the base class only exists for future extensibility, we should consider making the base class internal for now.
src/Components/Blazor/WebAssembly.Authentication/src/RemoteAuthenticatorViewCore.cs
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/Services/RemoteAuthenticationService.cs
Outdated
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/Services/RemoteAuthenticationService.cs
Show resolved
Hide resolved
@@ -119,8 +125,14 @@ public virtual async ValueTask<AccessTokenResult> GetAccessToken(AccessTokenRequ | |||
} | |||
|
|||
/// <inheritdoc /> | |||
public virtual async Task<ClaimsPrincipal> GetCurrentUser() | |||
protected virtual async ValueTask<ClaimsPrincipal> GetCurrentUser() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the 1-minute caching you've put in here. That feels like a good balance of efficiency and freshness.
Just so I can explain this to other people, can you summarise what it is that happens each time the 1-minute cache expires? What will we say in docs? Something like: The next time we evaluate authentication status, the system will make an HTTP request to the remote authentication service to verify that the user is still authenticated, and to update the list of claims associated with their ClaimsPrincipal
. Is that accurate?
src/Components/Blazor/WebAssembly.Authentication/src/Services/AccessTokenResult.cs
Show resolved
Hide resolved
src/Components/Blazor/WebAssembly.Authentication/src/Services/RemoteAuthenticationService.cs
Show resolved
Hide resolved
@@ -120,8 +127,24 @@ public virtual async ValueTask<AccessTokenResult> GetAccessToken() | |||
/// <inheritdoc /> | |||
public virtual async ValueTask<AccessTokenResult> GetAccessToken(AccessTokenRequestOptions options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have some naming issues here now. If I'm reading this correctly, the usage pattern would look like:
var accessTokenResult = await service.GetAccessToken(options);
if (accessTokenResult.TryGetAccessToken(out var accessToken))
{
// ...
}
Issues:
GetAccessToken
should have anAsync
suffix on its name, right?- It's confusing that both of the methods are basically called "get access token". If you got it the first time, why do you have to get it again?
I wish C# had a richer type system for this. The ideal result type would be AccessToken | AccessTokenRedirection
. But since that's not an option, I'm tending to prefer the low-ceremony option you originally had, e.g.,
var result = await service.GetAccessTokenAsync(options);
if (result.Success)
{
// ... use result.Token
}
else
{
// ... use result.RedirectionUrl
}
If we're feeling fancy, we might even consider defining a deconstructor on AccessTokenResult
so that in all the docs, we'd demonstrate its consumption like this:
var (token, redirectionUrl) = await service.GetAccessTokenAsync(options);
if (token != null)
{
// ... use token
}
else
{
// ... use redirectionUrl
}
However this relies on the idea that there will never be any other status besides "got token" and "needs redirection".
Let's discuss this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could even rely on C#8 nullables to communicate the intent more clearly. If accessTokenResult.Token
and accessTokenResult.RedirectionUrl
were both marked as nullable, the compiler will force people to at least think about the possibility that the token isn't provided.
src/Components/Blazor/testassets/Wasm.Authentication.Client/Shared/LoginDisplay.razor
Outdated
Show resolved
Hide resolved
be96bfd
to
7855aef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Adds a Microsoft.AspNetCore.Components.WebAssembly.Authentication library for performing authentication in Blazor webassembly. * Includes a default implementation that supports OIDC capable IdPs using oidc-client.js * Includes multiple primitives to deal with authentication flows and supports acquiring access tokens to call APIs. * RemoteAuthenticatorView is responsible for handling authentication operations at the user interface level. * RemoteAuthenticatorService is responsible for handling the lower level authentication details by using JavaScript interop to interact with the underlying javascript library implementing the auth protocol. * SignOutSessionStateManager handles CSRF protection for the logout path. * IAccessTokenProvider handles provisioning access tokens to call APIs.
30d3143
to
28c3bc1
Compare
This library includes the following components:
The general expected flow to follow by the templates is as follows:
'/authentication/{action}'.
The authentication manager accepts multiple parameters to configure the UI that is displayed at each stage of the authentication process. Those steps are:
Authenticated users can request access tokens by injecting the IAccessTokenProvider service into their app and calling GetAccessToken().
By default, a token with the default scopes is provisioned, but additional tokens might be provisioned by passing options to the GetAccessToken method.
This method returns an AccessTokenResult object that indicates whether a token is available or a redirect operation is needed to acquire the token.
In that case, the user can preserve the state locally and navigate to the url in the result to start the token provisioning process, after which, it will be sent back to the current point.
There are a few more tests that I'm adding and low level implementation details that I'm handling, but it is ready for review.