Skip to content

[Security] Improve DX of recent additions #59805

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 1 commit into from
Feb 21, 2025

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Feb 18, 2025

Q A
Branch? 7.3
Bug fix? no
New feature? not really
Deprecations? no
Issues -
License MIT

This is a follow up of #48142 and #59150 which were merged recently into 7.3.

Summary of the changes:

  • UserAuthorizationChecker, the implementation of the corresponding interface is merged into the existing AuthorizationChecker.
  • AuthorizationChecker::isGranted() is made aware of token changed by its new isGrantedForUser() method, so that calls to is_granted() nested into is_granted_for_user() calls will check the provided user, not the logged in one.
  • Replace the many arguments passed to IsGranted's closures by 1. a new IsGrantedContext and 2. the $subject. This makes everything simpler, easier to discover, and more extensible. Thanks to the previous item, IsGrantedContext only needs the auth-checker, not the access-decision-manager anymore. Simpler again.
  • Fix to the tests, those were broken in both PRs.

@carsonbot carsonbot added this to the 7.3 milestone Feb 18, 2025
@nicolas-grekas nicolas-grekas force-pushed the is-granted-for-user branch 3 times, most recently from a07496e to 8d7316b Compare February 19, 2025 08:51
@nicolas-grekas
Copy link
Member Author

Thanks for the reviews, comments addressed.

Status: needs review

Copy link
Member

@alexandre-daubois alexandre-daubois left a comment

Choose a reason for hiding this comment

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

Great improvement to the DX, thanks!

@nicolas-grekas
Copy link
Member Author

I just removed the IsGranted::IS_* consts in favor of corresponding IsGrantedContext::is*() methods.

@chalasr
Copy link
Member

chalasr commented Feb 20, 2025

isn't fabbot's patch legit?

@nicolas-grekas
Copy link
Member Author

I think this alignment rule doesn't work when the type description is too long, so I ignored fabbot on purpose...

@chalasr
Copy link
Member

chalasr commented Feb 21, 2025

Thank you @nicolas-grekas.

@chalasr chalasr merged commit 0c7c251 into symfony:7.3 Feb 21, 2025
9 of 11 checks passed
@nicolas-grekas nicolas-grekas deleted the is-granted-for-user branch February 27, 2025 15:34
@fabpot fabpot mentioned this pull request May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants