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

[AuthZ] Add IRequirementData #44342

Merged
merged 9 commits into from
Nov 1, 2022
Merged

[AuthZ] Add IRequirementData #44342

merged 9 commits into from
Nov 1, 2022

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Oct 3, 2022

Explore allowing an attribute to add requirements

@ghost ghost added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Oct 3, 2022
@HaoK
Copy link
Member Author

HaoK commented Oct 3, 2022

@davidfowl

I went back to just enabling adding requirements via an attribute since that makes it a little simpler to combine as there already are existing ways to specify what authentication, i.e. [Authorize].

@davidfowl
Copy link
Member

davidfowl commented Oct 4, 2022

Maybe we just need IRequirementData to exist. We don't need RequiresAttribute. I'm thinking the typical use case for this is what we have documented here today. After this change, it would look like this:

internal class MinimumAgeAuthorizeAttribute : AuthorizeAttribute, IRequirementData
{
    public MinimumAgeAuthorizeAttribute(int age) => Age = age;

    public int Age { get; }

    IEnumerable<IAuthorizationRequirement> GetRequirements() => new[] { 
        new MinimumAgeRequirement(Age)
    };
}

@HaoK
Copy link
Member Author

HaoK commented Oct 4, 2022

Sounds good, but in your example, GetRequirements would be even simpler right, since we don't need to build the policy anymore, it'd just be: new[] { new MinimumAgeRequirement(Age) } without the policy builder

@davidfowl
Copy link
Member

Sounds good, but in your example, GetRequirements would be even simpler right, since we don't need to build the policy anymore, it'd just be: new[] { new MinimumAgeRequirement(Age) } without the policy builder

Oops, fixed.

@HaoK HaoK changed the title Prototype Requirement attribute [AuthZ] Add IRequirementData Oct 4, 2022
@HaoK
Copy link
Member Author

HaoK commented Oct 4, 2022

Okay updated, pretty tiny change that actually enables a lot

@HaoK
Copy link
Member Author

HaoK commented Oct 4, 2022

Note: IRequirementData does trigger authorization by itself, so it doesn't require an [Authorize] attribute or IAuthorizeData to have an effect

@@ -374,6 +376,55 @@ public async Task CanApplyPolicyDirectlyToEndpoint()
Assert.True(calledPolicy);
}

public class ReqAttribute : Attribute, IRequirementData
Copy link
Member

Choose a reason for hiding this comment

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

Add an example that does:

public class ReqAttribute2 : AuthorizeAttribute, IRequirementData

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated and added the CanApplyAuthorizeRequirementAttributeDirectlyToEndpoint test below for this

@HaoK HaoK marked this pull request as ready for review October 12, 2022 17:41
@HaoK HaoK requested a review from Tratcher as a code owner October 12, 2022 17:41
@davidfowl
Copy link
Member

Lets do API review, file an issue and lets get this checked in and marked for blogging 😄 cc @adityamandaleeka

@HaoK HaoK added the blog-candidate Consider mentioning this in the release blog post label Oct 14, 2022
@ghost
Copy link

ghost commented Oct 14, 2022

@HaoK, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work!

Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers.

This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement.

Thanks!

@HaoK HaoK linked an issue Oct 14, 2022 that may be closed by this pull request
@kevinchalet
Copy link
Contributor

May I suggest naming it IAuthorizationRequirementData in case you'd come with a similar design for other areas like CORS in the future?

@HaoK
Copy link
Member Author

HaoK commented Oct 20, 2022

Sounds reasonable I'll update the design issue with the suggested new name, but I won't update this PR until after the api review has been done in case of other feedback

@HaoK HaoK deleted the haok/req branch November 1, 2022 18:30
@ghost ghost added this to the 8.0-preview1 milestone Nov 1, 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 blog-candidate Consider mentioning this in the release blog post
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AuthZ: Add IAuthorizationRequirementData
4 participants