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 IAuthorizationRequirementData #44551

Closed
HaoK opened this issue Oct 14, 2022 · 4 comments · Fixed by #44342
Closed

AuthZ: Add IAuthorizationRequirementData #44551

HaoK opened this issue Oct 14, 2022 · 4 comments · Fixed by #44342
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer blog-candidate Consider mentioning this in the release blog post Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in!
Milestone

Comments

@HaoK
Copy link
Member

HaoK commented Oct 14, 2022

Background and Motivation

We want to make it easier to attribute endpoints to add additional requirements for authorization. This interface will help by letting metadata or custom attributes specify requirements which will automatically get combined with other existing authorization mechanisms. Note the presence of IAuthorizationRequirementData metadata will be treated similar to IAuthorizeData, meaning its considered opting in for authorization (but it does not automatically bring in the default policy, see examples)

Proposed API

namespace Microsoft.AspNetCore.Authorization;

+ public interface IAuthorizationRequirementData
{
+    IEnumerable<IAuthorizationRequirement> GetRequirements();
}

Usage Examples

Example 1: Derive from Authorize attribute without specifying a policy to customize the default policy by adding arbitrary requirements, i.e.

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

    public int Age { get; }

    IEnumerable<IAuthorizationRequirement> GetRequirements() => new[] { 
        new MinimumAgeRequirement(Age)
    };
}
    
    [MinimumAgeAuthorize(21)]
    public void NoKidsControllerMethod { ... }

Example 2: Fully specify the requirements without interacting with default policy (no authorize attribute), i.e. only allow authorization on Tuesdays

internal class OnlyTuesdayAttribute : Attribute, IAuthorizationRequirementData
{
    IEnumerable<IAuthorizationRequirement> GetRequirements() => new[] { 
        new OnlyTuesdayRequirement()
    };
}
    
    [OnlyTuesday]
    public void TacoTuesdayMethod { ... }

Alternative Designs

Decided against allowing specification of a fully policy to minimize the complexity of how it combines with existing methods of producing policies, as its trivial to combine additional requirements (always safe to combine)

Risks

New way to opt into authorization where before only [Authorize]/IAuthorizeData was used to require authorization.

@HaoK HaoK added api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Oct 14, 2022
@HaoK HaoK self-assigned this Oct 14, 2022
@ghost
Copy link

ghost commented Oct 14, 2022

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@HaoK HaoK added this to the 8.0-preview1 milestone Oct 14, 2022
@HaoK HaoK removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 14, 2022
@HaoK
Copy link
Member Author

HaoK commented Oct 14, 2022

PR: #44342

@HaoK HaoK linked a pull request Oct 14, 2022 that will close this issue
@HaoK HaoK added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Oct 14, 2022
@HaoK HaoK changed the title AuthZ: Add IRequirementData AuthZ: Add IAuthorizationRequirementData Oct 20, 2022
@HaoK
Copy link
Member Author

HaoK commented Oct 24, 2022

@dotnet/aspnet-api-review this is ready for review

@halter73
Copy link
Member

halter73 commented Oct 31, 2022

API Review Notes:

  1. The motivation is that writing code to add new auth requirements to an endpoint (e.g. min age) requires writing a lot of code today in many classes.
  2. Discussed how the IAuthorizationRequirement marker interface works. All existing logic.
  3. Are we okay with logic in attributes? Yes. It's super convenient.
  4. Method vs property. We fee that a method better matches expectations. We don't know much about what the implementation could be. It's probably usually cached by our built-in policy providers, so we don't really require this is inexpensive.

API Approved!

namespace Microsoft.AspNetCore.Authorization;

+ public interface IAuthorizationRequirementData
+ {
+    IEnumerable<IAuthorizationRequirement> GetRequirements();
+ }

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Oct 31, 2022
@HaoK HaoK added Done This issue has been fixed ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in! blog-candidate Consider mentioning this in the release blog post labels Nov 1, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 1, 2022
@danroth27 danroth27 added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Mar 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer blog-candidate Consider mentioning this in the release blog post Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one ✔️ Resolution: Fixed The bug or enhancement requested in this issue has been checked-in!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants