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

Dev authorization fakes #151

Merged
merged 94 commits into from
Jun 25, 2020
Merged

Dev authorization fakes #151

merged 94 commits into from
Jun 25, 2020

Conversation

DarthPedro
Copy link
Contributor

@DarthPedro DarthPedro commented Jun 16, 2020

Pull request description

For components that use the authentication and authorization, there is a lot of boilerplate code required to write to create a test. This code can be complex and needs to be copied into user's test projects each time.

Provide a set of "fake" authorization services that allow the user to choose and change the authentication/authorization state and user identification for a test. (closes #146)

PR meta checklist

  • Pull request is targeting at DEV branch.
  • Pull request is linked to all related issues, if any.
  • I have read the CONTRIBUTING.md document.

Content checklist

  • My code follows the code style of this project and AspNetCore coding guidelines.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have updated the appropriate sub section in the CHANGELOG.md.
  • I have added, updated or removed tests to according to my changes.
    • All tests passed.

Todo checklist

  • Does this cover all auth scenarios, e.g. both declarative in razor syntax and in c# code?
  • Are there other types of claims that should be supported besides just basic/username? Is it relevant for testing? [DarthPedro] other claims can be really mocked based on what's required for a component or test. It shouldn't need fakes for that at this point. Blazor components like AuthorizeView and AuthorizeRouteView don't make use of claims.
  • Test that we can correctly switch between authenticated and unauthenticated states.
  • Create unit tests that covers the FakeAuthenticationStateProvider and FakeAuthorizationService. This can most likely be a regular unit test, maybe with a single component test in the mix to verify that e.g. AuthorizeView is compatible.
  • Create a document page that describe how to use the FakeAuthenticationStateProvider
  • Create a stub for FakeAuthenticationStateProvider (Register stubs of all default services Blazor provides at runtime with useful helper message #115) that is registered by default and provides the user with a good error message that tells how to register FakeAuthenticationStateProvider and link to docs. See example from JSMock at:
    • PlaceholderAuthenticationStateProvider.cs and PlaceholderAuthorizationService.cs.
    • TestServiceProviderExtensions.cs register the stubs in the AddDefaultTestContextServices.
  • Code should be placed in src/bunit.web/TestDoubles/Authorization or something similar.
  • Test code placement should mirror source code in src/bunit.web.test/TestDoubles/Authorization.
  • Create extension methods for TestServiceProvider which supports registering with or without a username, e.g. Services.AddAuthorization() and Service.AddAuthorization("UserName", isAuthorized = true);

…ion/authorization. Added unit tests for them.
…on test service, in the right state, to the TestServiceProvider. Added unit tests.
@egil egil added this to the beta-8 milestone Jun 16, 2020
@egil egil self-requested a review June 16, 2020 16:26
DarthPedro and others added 13 commits June 16, 2020 09:36
…Service to give user friendly reminder to call AddAuthorization to TestContext.Services.
Getting latest changes for docs from egil/bUnit dev branch.
…ion/authorization. Added unit tests for them.
…on test service, in the right state, to the TestServiceProvider. Added unit tests.
…Service to give user friendly reminder to call AddAuthorization to TestContext.Services.
@egil egil linked an issue Jun 17, 2020 that may be closed by this pull request
8 tasks
Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Very solid start. Here are a few general comments:

  • I haven't read through the docs and changelog text yet, since that might change once the API settles. Ill do proper review of those later.
  • Well structured tests, good work on those.
  • I sometimes to suggest a code change in one place, but that change should also be made in multiple other places, where the same thing happens.
  • I focused mostly on reviewing the tests, as they are using the API you are building. I will return to the implementation in bunit.web when the API is settled.

@egil egil linked an issue Jun 17, 2020 that may be closed by this pull request
@egil egil added the enhancement New feature or request label Jun 17, 2020
@egil
Copy link
Member

egil commented Jun 17, 2020

FYI: I have updated the dev branch with test-doubles as the name for the documentation section.

DarthPedro and others added 7 commits June 17, 2020 08:49
…eProvider.cs


Change to method comment.

Co-authored-by: Egil Hansen <[email protected]>
changing mocking to faking

Co-authored-by: Egil Hansen <[email protected]>
…nPolicyProviderTest.cs


Removing ConfigureAwait call.

Co-authored-by: Egil Hansen <[email protected]>
removed navigation manager from component.

Co-authored-by: Egil Hansen <[email protected]>
removed navigation manager from component.

Co-authored-by: Egil Hansen <[email protected]>
@egil
Copy link
Member

egil commented Jun 23, 2020

I pushed some changes to the PR including refactored docs to fit the general style.

While documenting policies and roles i stumples onto something I think is a bug. I added a bunch of extra tests to the project, but in essens, the problem is content limited by roles and policies are always shown, even when inside a <AuthorizeView Roles="superuser"> component.

@DarthPedro
Copy link
Contributor Author

I don't see any new pushed commits from you to the PR. Did you forget to push them? ;)

@egil
Copy link
Member

egil commented Jun 23, 2020

Weird, I did not. But maybe I don't have the right remote set. Let me check and get back to you.

@egil
Copy link
Member

egil commented Jun 23, 2020

Ahh, I managed to push a new branch to this repo with the changes: https://www.github.com/egil/bUnit/tree/dev-authorization-fakes/

Not sure how I add these to your PR without pushing directly to your local repo. Suggestions?

@DarthPedro
Copy link
Contributor Author

That's weird. You pushed a commit into this PR earlier: 3e4b04d. Did you do that in my forked repo?

@egil
Copy link
Member

egil commented Jun 23, 2020

Hmm nop. But I did pull the PR down using the gh pr pull command... Maybe that set something up. I'll have to check tonight after the kids are down for the night.

@egil
Copy link
Member

egil commented Jun 24, 2020

OK, I managed to push it to this PR instead. It seems that when you use gh pr checkout 151 to review locally, you cannot change to another branch and come back again for a later review.

@DarthPedro
Copy link
Contributor Author

Ok, I have your changes now. I will look at the failing tests.

@DarthPedro
Copy link
Contributor Author

Fixed the AuthorizationService.AuthorizeAsync to validate authorization state based on requirements and either policies, roles, or authorization state. This covers all of the states we have, I believe.

I'll look at a SetClaims method as well though I'm not sure we'll be able to fake of the behavior that ClaimsPrincipal.Claims does.

…View to behave as expected. Updated sample code too.
Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Just a quick review here on gh, see review comments on code below.

Regarding SetClaims, let's leave it for now and see if somebody asks for the feature.

What about a way to test asynchronous authentication, e.g. the state (https://docs.microsoft.com/en-us/aspnet/core/blazor/security/?view=aspnetcore-3.1#content-displayed-during-asynchronous-authentication), is it a hard thing to get working?

@DarthPedro
Copy link
Contributor Author

I got both SetClaims and the AuthorizeView.Authorizing state working. Authorizing was a little tricky because how to get into that state is not documented. But I figured it out looking through the AuthorizeViewCore code.

Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Awesome getting both claims and authorizing working. Ill update the docs tomorrow (I am on CET timezone) and play with the bits.

Found a few small things.

@egil
Copy link
Member

egil commented Jun 25, 2020

@DarthPedro I changed the code a bit to make it possible to call SetAuthorizing() before RenderComponent. It was necessary as the code otherwise through a null pointer exception, since you were passing a null! in as the authentication state.

Please give my changes a review. If it looks alright, I'd say we are done!

@DarthPedro
Copy link
Contributor Author

Ok, that looks good. Thanks for all of the help.

@egil
Copy link
Member

egil commented Jun 25, 2020

Ok, that looks good. Thanks for all of the help.

No thank you. You almost all the work here!

@egil egil merged commit 41540d5 into bUnit-dev:dev Jun 25, 2020
@DarthPedro DarthPedro deleted the dev-authorization-fakes branch June 25, 2020 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants