Skip to content

Conversation

@shethaadit
Copy link
Contributor

Description

This PR updates the XML documentation for DefaultPolicy and FallbackPolicy in the AuthorizationPolicy class to provide clearer explanations of when each policy applies.

Changes:

  • FallbackPolicy:

    • Clarifies that it only applies when no authorization metadata is present (e.g., no [Authorize] attributes or RequireAuthorization() calls).
    • Explains why it is not used when an [Authorize] attribute is present without a policy name.
    • Emphasizes that it is primarily relevant for middleware-based authorization.
  • DefaultPolicy:

    • States that it applies whenever authorization is required but no specific policy is set.
    • Clarifies that an [Authorize] attribute without a policy name will always use DefaultPolicy instead of FallbackPolicy.
    • Provides guidance on explicitly defining policies to avoid confusion.

Why This Change?

  • Addresses user concerns about unexpected behavior when using [Authorize] without a policy name.
  • Ensures developers understand when FallbackPolicy is ignored and why DefaultPolicy takes precedence.
  • Prevents security risks due to misinterpretations of policy precedence.

Fixes #60452

Copilot AI review requested due to automatic review settings February 25, 2025 23:44
@shethaadit shethaadit requested a review from halter73 as a code owner February 25, 2025 23:44
@ghost ghost added the area-security label Feb 25, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 25, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/Security/Authorization/Core/src/AuthorizationOptions.cs:30

  • [nitpick] Consider using XML elements instead of backticks to mark inline code (e.g., policy names) for improved clarity in XML documentation.
/// - The `DefaultPolicy` applies whenever authorization is required, but no specific policy is set.

@halter73 halter73 self-assigned this Feb 26, 2025
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

Thanks! This looks like a big improvement.

Part of me wanted to continue to cross-reference IAuthorizeData for completeness, but most consumers of the API won't have any familiarity with that and are better served by the new mentions to [Authorize] and RequireAuthorization().

@shethaadit
Copy link
Contributor Author

Hi @halter73, Than you for all suggestions. I have committed them. Could you please re-approve and merge?

@shethaadit shethaadit requested a review from halter73 February 28, 2025 17:53
@shethaadit
Copy link
Contributor Author

Hi @halter73, Than you for all suggestions. I have committed them. Could you please re-approve and merge?

Hi @halter73, a gentle reminder on this. :)

@halter73
Copy link
Member

halter73 commented Mar 7, 2025

Thank you!

@halter73 halter73 merged commit 8ec8cbf into dotnet:main Mar 7, 2025
27 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview3 milestone Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-security community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Authorization FallbackPolicy is misleading

2 participants