Skip to content

[Blazor] More auth fixes #20192

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

Merged
merged 5 commits into from
Apr 4, 2020
Merged

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented Mar 26, 2020

Follow up from #19966

Being able to customize the user during the authentication process as well as to control the flow of the authentication once the core authentication flow has a great importance when building applications using remote authentication protocols. It enables developers to:

  • Enrich the user claims with data outside of the scope of the identity provider.
    • To add additional claims that might drive the UI.
    • To define roles for the user that are outside of those of the IdP.
  • Collect additional information specific to the application that might be required for it to work.
  • Prevent the authenticated user from accessing parts of the UI he's not allowed to.
    • (This is just an optimization of the UX, security must always be done server-side)

Goals/Decission drivers

  • We want the authentication decissions to be centralized in one place within the UI, as this makes it easier to enforce the full authentication flow.
  • We want the process of customizing the authenticated user simple, ideally it is done in one place.
  • We want to separate the process of generating the authenticated user from the process of making authorization decissions.

Candidate solutions

  • Do nothing and tell developers to override GetUserAsync on RemoteAuthenticationService.
  • Include a callback in RemoteAuthenticatorViewCore to customize the user.
  • Include a callback in RemoteAuthenticationOptions to customize the user.
  • Define a new abstraction for generating the final authenticated user.

Define a new abstraction for generating the final authenticated user

I decided to implement a new abstraction to make it easier to customize the user being authenticated. This abstraction, UserFactory<RemoteUserAccount> handles mapping the user from the definition that the RemoteAuthenticationService provides into the ClaimsPrincipal that it is later on, used by the application.

Trade-offs

Adding a new primitive for mapping the user offers a single place where the mapping process can take place and allows developers to use normal extensibility mechanisms like dependency injection and so on in order to provision any service that they need to use to produce the final user.

It allows developers to focus on the mapping process without interfering with how or when the user is updated, which is a complicated process and can be handled by the framework.

It offers developers a natural place to implement caching strategies and other optimizations that might be required to implement the mapping process efficiently.

It introduces a new concept that users must understand, although this is not necessary until the user needs to be customized, given that a reasonable default implementation is provided.

It makes it complicated to inject some dependencies that make use of the IAccessTokenProvider since it is part of the RemoteAuthenticationService and injecting it would produce a cyclical dependency. This is mitigated in the default implementation by offering the IAccessTokenProvider as a property, which is supplied through an IAccessTokenProviderAccessor that delays the resolution until the property is first accessed. This makes sure that any custom implementation is encouraged to offer a suitable construction for dependency injection.

Finally, this option completely separates the mapping of the user from any other mechanic relate to the authentication process and enables decissions about the authentication flow to be taken at a later stage based on claims defined in the user, which gives developers the option to make those flows context sensitive.

Other options

Do nothing and tell developers to override GetUserAsync on RemoteAuthenticationService

In this option we would ask the users to override and customize RemoteAuthenticationService to produce the actual user and register their implementation in the dependency injection container.

Trade-offs

This option is cheap in terms of cost (free) but it offers a poor user experience and sets customers up for failure. The main reasons for this are:

  • It is not easy to wire-up a RemoteAuthenticationService into the DI container as the same instance needs to be shared in different contexts. (AuthenticationStateProvider, IAccessTokenProvider, etc.).
  • It forces users to re-implement all the caching logic that the RemoteAuthenticationService offers to cache the user, limiting the amount of JS interop calls that happen to check the user status.
  • It establishes a tight contract between the RemoteAuthenticationService and the underlying AuthenticationService, which at the moment is just an implementation detail of the RemoteAuthenticationService.

Include a callback in RemoteAuthenticatorViewCore to customize the user

In this option we would define an event callback inside RemoteAuthenticatorViewCore that gets called when the user is authenticated.

Trade-offs

It would allow developers to write code in their Authentication.razor page that can access DI services in a way they are used to.

The main drawbacks for this approach are that RemoteAuthenticatorViewCore already handles many concerns and tacking an additional one to it just increases the complexity and makes it harder for users to understand it.

It forces that callback to be passed down through all the layers down to the RemoteAuthenticationService.

It can't be made to work as the mapping must be able to produce users even when a RemoteAuthenticatorViewCore is not part of the page.

Include a callback in RemoteAuthenticationOptions to customize the user

In this option we would include a callback in RemoteAuthenticationOptions that developers can set to customize the user.

Trade-offs

It is the best option for simple cases, since it makes it trivial to perform easy modifications. However, when more complex scenarios present themselves it sets up users for failure since it doesn't compose well, doesn't have a mechanism to resolve dependencies nor to hold on to other state for caching purposes and so on.

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Mar 26, 2020
@javiercn javiercn force-pushed the javiercn/blazor-wasm-auth-fixes-part2 branch from d9e9734 to 5b4ae2d Compare March 26, 2020 16:19
@javiercn javiercn changed the base branch from javiercn/blazor-wasm-auth-fixes to blazor-wasm March 26, 2020 16:19
@javiercn javiercn force-pushed the javiercn/blazor-wasm-auth-fixes-part2 branch 2 times, most recently from 887dec6 to 372a18c Compare March 27, 2020 22:27
@javiercn javiercn force-pushed the javiercn/blazor-wasm-auth-fixes-part2 branch from 6932eb8 to 5742583 Compare March 28, 2020 14:58

namespace Microsoft.AspNetCore.Components.WebAssembly.Authentication.Internal
{
internal class AccessTokenProviderAccessor : IAccessTokenProviderAccessor
Copy link
Member

Choose a reason for hiding this comment

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

As with the comment about IAccessTokenProviderAccessor, I haven't spotted what this is used for. If you could clarify what consumes this feature that would be great.

Copy link
Member Author

Choose a reason for hiding this comment

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

I answered here. The main goal is to make it easy to consume the IAccessTokenProvider from the UserFactory since that's a common thing and it's the lesser of evils.

Copy link
Member

Choose a reason for hiding this comment

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

Understood now - thanks

@javiercn
Copy link
Member Author

javiercn commented Apr 3, 2020

🆙 📅

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Excellent!

@javiercn javiercn merged commit fd9c786 into blazor-wasm Apr 4, 2020
@javiercn javiercn deleted the javiercn/blazor-wasm-auth-fixes-part2 branch April 4, 2020 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
4 participants