Skip to content

Add AuthorizationBuilder #42264

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 9 commits into from
Jun 29, 2022
Merged

Add AuthorizationBuilder #42264

merged 9 commits into from
Jun 29, 2022

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Jun 17, 2022

AuthZ part of #42235

I didn't update everything, but updated enough of the tests to feel confident that this does reduce lines of code even just in our tests, and that the basics are working, will finish the rest of this assuming this is something that we want to take for real.

Before

      var authorizationService = BuildAuthorizationService(services =>
        {
            services.AddAuthorization(options =>
            {
                options.AddPolicy("Basic", policy => policy.RequireClaim("Permission", "CanViewPage"));
            });
        });

After

            services.AddAuthorization().AddPolicy("Basic", policy => policy.RequireClaim("Permission", "CanViewPage")));```

Note: this does change the return value for AddAuthorization* methods to return the new AuthorizationBuilderServiceCollection that wraps the service collection

cc @DamianEdwards @davidfowl @captainsafia @adityamandaleeka

@ghost ghost added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jun 17, 2022
@DamianEdwards
Copy link
Member

DamianEdwards commented Jun 17, 2022

Yeah I like this approach that enables us to return the builder from
builder.Authorization and services.AddAuthorization() (mostly) without a breaking change.

@HaoK
Copy link
Member Author

HaoK commented Jun 20, 2022

Updated with new proposed AddAuthorizationBuilder name

@HaoK
Copy link
Member Author

HaoK commented Jun 20, 2022

@DamianEdwards is this something you want me to get in for preview6, or can this wait for preview7? The PR Is basically ready, since I don't have to update all tests to use the new API

@DamianEdwards
Copy link
Member

It would still need an API review and we should likely do this change at the same time as introducing the new WebApplicationBuilder.Authorization property. Code complete is tomorrow and we still have quite a lot in preview.6 to land in an SDK build so might be wise to push this to preview.7

@HaoK
Copy link
Member Author

HaoK commented Jun 20, 2022

@captainsafia do you want to make the WAB.Authorization side of things in my branch or do you want me to do it (for preview 7)?

@davidfowl
Copy link
Member

Lets put this up for API review (write an API proposal based on this PR in the issue)

@HaoK HaoK marked this pull request as ready for review June 28, 2022 18:27
@HaoK HaoK requested a review from Tratcher as a code owner June 28, 2022 18:27
@@ -29,7 +29,7 @@ public AuthorizationBuilder(IServiceCollection services)
/// Defaults to true.
/// </summary>
/// <returns>The builder.</returns>
public virtual AuthorizationBuilder SetInvokeHandlersAfterFailure(bool invoke)
public virtual AuthorizationBuilder InvokeHandlersAfterFailure(bool invoke)
Copy link
Member

Choose a reason for hiding this comment

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

Set* seems better tbh

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, going back to the original SetInvokeHandlersAfterFailure

HaoK added 3 commits June 29, 2022 13:06
This reverts commit a4a0f52.
This reverts commit 8c38ad6.
@HaoK HaoK enabled auto-merge (squash) June 29, 2022 20:51
@HaoK HaoK merged commit 4942713 into main Jun 29, 2022
@HaoK HaoK deleted the haok/authZBuilder branch June 29, 2022 21:59
@ghost ghost added this to the 7.0-preview7 milestone Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants