Skip to content

Components auth step 2 #10293

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 13 commits into from
May 20, 2019
Merged

Components auth step 2 #10293

merged 13 commits into from
May 20, 2019

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented May 16, 2019

This PR is mainly about E2E tests for the previous auth work (#10227). It also contains a few commits that address remaining CR feedback from #10227.

Things to note:

  • I had to do a bit of refactoring on how E2E tests decide whether to run client-side or server-side. Previously, the "hosting environment" (dev server, or the TestServer app) and "execution environment" (client-side or server-side) were coupled together as concepts. But the new AuthTests have to do both client-side and server-side execution while running on TestServer, since that's what supplies the auth state. The net result is that the E2E test code is now just more explicit about execution environment in many places, so this is good anyway.
  • From looking at the ServerAuthenticationStateProvider, ClientSideAuthenticationStateData, and UserController classes, you can now get a sense of how this feels when doing client-side Blazor. It's more involved than for server-side Blazor, because we have to fetch the authentication state from the server via an API request.

@SteveSandersonMS SteveSandersonMS added the area-blazor Includes: Blazor, Razor Components label May 16, 2019
@SteveSandersonMS SteveSandersonMS added this to the 3.0.0-preview6 milestone May 16, 2019
@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner May 16, 2019 16:11
@@ -29,6 +29,9 @@ public void ComputesCorrectBaseUri(string baseUri, string expectedResult)
[InlineData("scheme://host/path/", "scheme://host/path/", "")]
[InlineData("scheme://host/path/", "scheme://host/path/more", "more")]
[InlineData("scheme://host/path/", "scheme://host/path", "")]
[InlineData("scheme://host/path/", "scheme://host/path#hash", "#hash")]
[InlineData("scheme://host/path/", "scheme://host/path/#hash", "#hash")]
[InlineData("scheme://host/path/", "scheme://host/path/more#hash", "more#hash")]
Copy link
Member Author

Choose a reason for hiding this comment

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

Support for these cases was needed for the E2E tests. It's because we use a #server suffix to trigger server-side execution (and always have done - that's not new), and it turns out the ToBaseRelativePath logic didn't understand the case that's now on line 32 here.

@natemcmaster natemcmaster removed the request for review from a team May 16, 2019 18:42
};

await HttpContext.SignInAsync(
new ClaimsPrincipal(new ClaimsIdentity(claims, "FakeAuthenticationType")));
Copy link
Member

Choose a reason for hiding this comment

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

Should we use the same scheme here? CookieAuthenticationDefaults.AuthenticationScheme - not sure that it matters.

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS May 20, 2019

Choose a reason for hiding this comment

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

I slightly prefer specifying FakeAuthenticationType here because it clarifies that we're indifferent to the scheme name. But overall I agree it doesn't matter much!

It exists so that (1) we can easily have multiple test cases that depend on the
CascadingAuthenticationState, and (2) we can test the integration between the router
and @page authorization rules.
*@
Copy link
Member

Choose a reason for hiding this comment

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

👍

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.

Seems good.

I'm starting to wonder about the scalability of using a single app for all of these tests, but that's a conversation for another day. I realize this test has some new requirements we haven't had in the past.

namespace BasicTestApp.AuthTest
{
// This is intended to be similar to the authentication stateprovider included by default
// with the client-side Blazor "Hosted in ASP.NET Core" template
Copy link
Member Author

Choose a reason for hiding this comment

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

Now I've written out this code, it makes me think we should consider making this more built-in, because it doesn't feel very friendly right now. We could ship a standard ServerAuthenticationStateProvider which developers explicitly add to their client-side Blazor DI config, along with some option about what URL it hits to get the state data. We'd also ship a standard ClientSideAuthenticationStateData DTO type (probably with a better name). Then the amount of boilerplate code in apps/templates would be pretty minimal (just some DI config plus UserController on the server), plus it's still possible to make your own entirely custom AuthenticationStateProvider if you don't like ours. Thoughts on this?

@SteveSandersonMS SteveSandersonMS merged commit 147880f into master May 20, 2019
@SteveSandersonMS SteveSandersonMS deleted the stevesa/auth-step-2 branch May 20, 2019 14:41
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.

6 participants