Skip to content

Conversation

tonyhallett
Copy link

@tonyhallett tonyhallett commented May 17, 2024

Remove all references to the MinimumAgePolicyProvider that is not used when using IAuthorizationRequirementData.

The middleware constructs a policy adding the requirements from IAuthorizationRequirementData.GetRequirements

https://github.com/dotnet/aspnetcore/blob/82b0fc9f43ae2bd50fb95f427116efc2f6f094df/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs#L130


Internal previews

📄 File 🔗 Preview link
aspnetcore/security/authorization/iard.md aspnetcore/security/authorization/iard

Remove all references to the MinimumAgePolicyProvider that is not used when using IAuthorizationRequirementData.

The middleware constructs a policy adding the requirements from IAuthorizationRequirementData.GetRequirements

https://github.com/dotnet/aspnetcore/blob/82b0fc9f43ae2bd50fb95f427116efc2f6f094df/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs#L130
@Rick-Anderson Rick-Anderson self-assigned this May 22, 2024
@Rick-Anderson
Copy link
Contributor

@adityamandaleeka can you assign someone to review this?

@guardrex
Copy link
Collaborator

guardrex commented Mar 25, 2025

@Rick-Anderson ... I don't understand why we'd remove this because this is how to set up a custom IAuthorizationPolicyProvider.

Note in passing that the docs could just point to the PU's demo at ...

https://github.com/dotnet/aspnetcore/tree/main/src/Security/samples/CustomPolicyProvider

... and shed the sample that the docs maintain (in the samples repo). The PU's sample is more likely to be kept up to date because they have it wired to their tests. I recommend at least closing this and asking for an issue.

@Rick-Anderson
Copy link
Contributor

@Rick-Anderson ... I don't understand why we'd remove this because this is how to set up a custom IAuthorizationPolicyProvider.

Note in passing that the docs could just point to the PU's demo at ...

https://github.com/dotnet/aspnetcore/tree/main/src/Security/samples/CustomPolicyProvider

... and shed the sample that the docs maintain (in the samples repo). The PU's sample is more likely to be kept up to date because they have it wired to their tests. I recommend at least closing this and asking for an issue.

#35045

@Rick-Anderson
Copy link
Contributor

@tonyhallett any chance you could do #35045
?

@Rick-Anderson Rick-Anderson reopened this Apr 21, 2025
@guardrex
Copy link
Collaborator

@wadepickett @tdykstra ... Just passing through the PRs to see what's going on with some of the lingering ones.

I still think that this isn't the best approach here. I think either of the following make sense ...

  • Cross-link the sample with remarks at ...

    https://github.com/dotnet/aspnetcore/tree/main/src/Security/samples/CustomPolicyProvider

    ... because ...

    • It's maintained release-to-release and perfectly demos the use case.
    • Is relevant for the reasons stated by the sample's README, which is why this content/example was placed into the article in the first place.
    • Makes it easy to shed the example in the doc text, which is one less thing to maintain.
    • The sample app isn't just for testing. It's also an example that the PU wants to put out there to demo the use case.

    I can resolve this if everyone is busy and wants this to go that way.

  • Or, close this PR. This isn't optional content for the stated use case, namely (from the README and touched on by the text) ...

    ... way to allow 'parameterized' authorization policies. Since authorization policies are identified by name strings, the custom MinimumAgeAuthorizeAttribute in the sample allows users to specify an age parameter and then embeds it into its underlying AuthorizationAttribute's policy name string. The MinimumAgePolicyProvider dynamically generates the policies needed for use with these attributes by pulling the age from the policy name and creating necessary authorization requirements.

    Other uses of IAuthorizationPolicyProvider might be loading policy information from some external data source (like a database, for example).

@wadepickett
Copy link
Contributor

@guardrex, I agree with your suggestion to cross link to the example with remarks, since you were able to verify the PU wants to use that example to demo the use case. Great approach. My vote is that if you want to take that on since you already have your brain in it, that's great! However, if it sets you back for other priority work feel free to queue it up as an issue in the backlog indicating priority and we will see if one of us can pick it up next sprint.

@tonyhallett, your work to help improve this topic is greatly appreciated and has been the catalyst we needed for finding a solution to make it more helpful. Thank you!!

@guardrex
Copy link
Collaborator

Yes, thanks @tonyhallett. I'll take care of this on #35467.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants