Skip to content

Components auth: basic services and components #10227

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 25 commits into from
May 16, 2019

Conversation

SteveSandersonMS
Copy link
Member

The first step in implementing the auth spec, https://gist.github.com/SteveSandersonMS/60ca3a5f70a7f42fba14981add7e7f79

This PR contains:

  • The underlying DI service, IAuthenticationStateProvider, and its server-side implementation that simply passes through the state from HttpContext.User
  • The AuthenticationStateProvider component that adapts IAuthenticationStateProvider into a cascading value, so components can easily observe changes to the authentication state and optionally use it in procedural logic
  • (Partial) The AuthorizeView component that consumes the auth state cascading value and lets developers easily show different UI states depending on authorization conditions purely using declarative code.

More auth PRs will follow.

@SteveSandersonMS SteveSandersonMS added the area-blazor Includes: Blazor, Razor Components label May 14, 2019
@SteveSandersonMS SteveSandersonMS added this to the 3.0.0-preview6 milestone May 14, 2019
@SteveSandersonMS
Copy link
Member Author

Review note: I'll be adding E2E tests later in process of doing the whole auth thing, once the feature set is more complete. So not in this PR.

Copy link
Member

@rynowak rynowak left a comment

Choose a reason for hiding this comment

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

Looks good. Looks like what I expected to see 👍

@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/components-auth-services branch from 8f3b2c3 to dcc7e11 Compare May 15, 2019 12:46
@SteveSandersonMS SteveSandersonMS requested a review from rynowak May 15, 2019 14:54
/// <summary>
/// Gets a <see cref="ClaimsPrincipal"/> that describes the current user.
/// </summary>
public ClaimsPrincipal User { get; }
Copy link
Member

Choose a reason for hiding this comment

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

nit: fields -> constructors -> properties

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. Rather than go through another multi-hour CI process to make this tweak, I'm going to include it in my next auth PR.

/// Gets an <see cref="AuthenticationState"/> instance that describes
/// the current user.
/// </summary>
/// <returns>An <see cref="AuthenticationState"/> instance that describes the current user.</returns>
Copy link
Member

Choose a reason for hiding this comment

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

technically not correct, it returns "a task that blah blah blah"

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. Rather than go through another multi-hour CI process to make this tweak, I'm going to include it in my next auth PR.

/// </summary>
#pragma warning disable 0067 // "Never used" (it's only raised by subclasses)
public event AuthenticationStateChangedHandler AuthenticationStateChanged;
#pragma warning restore 0067
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually needed now that NotifyAuthenticationStateChanged is there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well spotted. Rather than go through another multi-hour CI process to make this tweak, I'm going to include it in my next auth PR.


AuthenticationStateChanged?.Invoke(task);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Food for thought. It appears that we want to (and support) raising this event on any thread. Do we want to document that as supported? Or do we want to try and make it enforced that you raise this on the sync context.

Copy link
Member

Choose a reason for hiding this comment

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

I don’t think we want that. The callback should be responsible for calling Invoke or InvokeAsync when handling the event.

I’m wondering how good our support story is when a component calls Invoke

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS May 16, 2019

Choose a reason for hiding this comment

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

It's an interesting question. We could make the AuthenticationStateProvider aware of the sync context (it's already a scoped service), and we could use an async-event pattern (Task-returning delegates) so that all the handlers would be async functions running on the sync context, whether or not they need to be.

However, the AuthenticationStateProvider is intended as a low-level service that developers will commonly only be the producer of (if implementing a custom authentication system), not the consumer. To consume the authentication state, developers will typically use either the cascaded value or the <AuthorizeView>, both of which already marshal the flow onto the sync context for you.

So, since in common usage patterns this won't be an issue anyway, I'd prefer to keep the AuthenticationStateProvider simple and independent of this, letting any developers who choose to use this low-level service deal with their own Invoke/InvokeAsync as Javier mentions, like they would when subscribing to any other service that's separate from the rendering system.

@SteveSandersonMS SteveSandersonMS merged commit 1dbc203 into master May 16, 2019
@SteveSandersonMS SteveSandersonMS deleted the stevesa/components-auth-services branch May 16, 2019 08:59
SteveSandersonMS added a commit that referenced this pull request May 16, 2019
public partial class AuthorizeView : Microsoft.AspNetCore.Components.ComponentBase
{
public AuthorizeView() { }
[Microsoft.AspNetCore.Components.ParameterAttribute]
Copy link
Member

Choose a reason for hiding this comment

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

@SteveSandersonMS - something else I thought of late (sorry).

These need to go into the manually maintained section of the ref assembly. The reason why is that ref assemblies don't preserve the setter if it's non-public. You can use cc1b294 as an example.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great reminder - thanks. This should be covered in today's PR in this commit: ad9ac55

SteveSandersonMS added a commit that referenced this pull request May 20, 2019
SteveSandersonMS added a commit that referenced this pull request May 20, 2019
SteveSandersonMS added a commit that referenced this pull request May 20, 2019
* CR feedback left over from #10227

* Begin adding E2E test case

* Add cookie auth and test login page

* Make E2E auth component work client-side too

* Restructure auth E2E tests around a router so there can easily be multiple such test components

* Add E2E test case for AuthorizeView

* Prepare for E2E test implementations

* Fix ToBaseRelativePath handling of hashes

... otherwise E2E test will fail, because we're using the hash to control server-or-client execution

* Decouple E2E execution mode from hosting mode

* Actual E2E tests for cascading authentication state

* Actual E2E tests for AuthorizeView (in "no authentication rule" mode)

* Fix inconsistent namespace

* CR: Manual ref assembly definitions for AuthorizeView/CascadingAuthenticationState
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
Development

Successfully merging this pull request may close these issues.

3 participants