[Security][SecurityBundle] User authorization checker #48142

natewiebe13 · 2022-11-07T17:53:41Z

Q	A
Branch?	7.3
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #43372
License	MIT
Doc PR	symfony/symfony-docs#18033

isGranted() assumes that it's checking against the currently logged in user. This provides the same functionality to check against a user during times when there isn't a session (cronjobs/commands, message queue, etc.) or for a different user than the one logged in.

Having this functionality allows for removing the dependency on sessions entirely for services reducing the number of issues that come up during a project because some underlying function was session dependent.

...ony/Component/Security/Core/Tests/Authentication/Token/UserAuthorizationCheckerTokenTest.php

derrabus

Thank you for your PR.

src/Symfony/Component/Security/Core/Authentication/Token/UserAuthorizationCheckerToken.php

src/Symfony/Component/Security/Core/Authorization/UserAuthorizationChecker.php

src/Symfony/Component/Security/Core/Tests/Authorization/UserAuthorizationCheckerTest.php

src/Symfony/Component/Security/Core/Authorization/UserAuthorizationChecker.php

src/Symfony/Bundle/SecurityBundle/Resources/config/security.php

stof · 2022-11-08T11:59:03Z

src/Symfony/Component/Security/Core/Authorization/UserAuthorizationChecker.php

+            || AuthenticatedVoter::IS_AUTHENTICATED === $attribute
+            || AuthenticatedVoter::IS_IMPERSONATOR === $attribute
+            || AuthenticatedVoter::IS_REMEMBERED === $attribute) {
+            throw new InvalidArgumentException(sprintf('"%s" cannot decide on attribute "%s" as it requires checking the session. Use "%s" instead.', self::class, $attribute, AuthorizationCheckerInterface::class));


This check should probably be done in the AuthenticatedVoter when passing it a UserAuthorizationCheckerToken instead of making a special case here (which would not work if a custom voter uses a delegated check on the authentication level as part of their logic, as this would pass the UserAuthorizationCheckerToken to the AuthenticatedVoter without going through this UserAuthorizationChecker)

Next push will include the voter abstaining if the token is marked with the interface. I feel like it's probably still useful to know if you're using the checker improperly, rather than getting unexpected results. Any concerns with leaving this check in as well?

I'm not sure abstaining is always the right answer either though (the final decision won't ever be an abstain)

True, I guess technically since the token is internal, it really shouldn't get this far. I'm good with moving the exception into the voter.

stof · 2022-11-08T11:59:50Z

src/Symfony/Component/Security/Core/Security.php

@@ -79,6 +79,15 @@ public function isGranted(mixed $attributes, mixed $subject = null): bool
            ->isGranted($attributes, $subject);
    }

+    /**
+     * Checks if the attributes are granted against the user and optionally supplied subject.


this should explicitly document the differences with isGranted() IMO, to make its limitation clear

Updated, is this what you were looking for?

src/Symfony/Component/Security/Core/Authentication/Token/UserAuthorizationCheckerToken.php

src/Symfony/Component/Security/Core/Authorization/UserAuthorizationChecker.php

HeahDude

Nice feature, thanks!

src/Symfony/Bundle/SecurityBundle/Resources/config/security.php

VincentLanglet · 2023-01-31T10:49:05Z

This would be a great feature. What is missing to this PR @natewiebe13 @derrabus @stof ?

derrabus · 2023-01-31T11:17:55Z

What is missing to this PR

Another round of reviews, I suppose. I'd very much like to see this feature shipped as well.

94noni

Would love to see this land on 7.2 :)

natewiebe13 · 2024-12-03T20:52:28Z

Rebased for 7.3 🤞

derrabus · 2024-12-07T13:46:42Z

It took time, but here we go, this is in now. Thank you very much @natewiebe13.

…rvice façade (derrabus) This PR was merged into the 7.3 branch. Discussion ---------- Remove ServiceSubscriberInterface implementation from Service façade | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | Follow-up to #48142 | License | MIT #48142 added a `ServiceSubscriberInterface` implementation to our `Security` façade, but that implementation is never consumed. If we did that, we would fail because the `security.firewall.event_dispatcher_locator` is hinted as a generic `ServiceLocator` which we cannot autowire. This is why I'd like to remove that implementation. Commits ------- a185129 Remove ServiceSubscriberInterface implementation

smnandre · 2024-12-14T20:15:50Z

src/Symfony/Bundle/SecurityBundle/Security.php

+     *
+     * This should be used over isGranted() when checking permissions against a user that is not currently logged in or while in a CLI context.
+     */
+    public function userIsGranted(UserInterface $user, mixed $attribute, mixed $subject = null): bool


isUserGranted no ?

👍 discussed internally, either this or isGrantedForUser()

…rantedForUser() (xabbuh) This PR was merged into the 7.3 branch. Discussion ---------- [Security][SecurityBundle] rename userIsGranted() to isGrantedForUser() | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | see #48142 (comment) | License | MIT Commits ------- 278d62c rename userIsGranted() to isGrantedForUser()

…)` function (natewiebe13) This PR was merged into the 7.3 branch. Discussion ---------- [SecurityBundle][TwigBridge] Add `is_granted_for_user()` function | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | N/A | License | MIT Twig function to accompany #48142 Commits ------- 82ff3a5 Add is_granted_for_user() function to twig

…)` function (natewiebe13) This PR was merged into the 7.3 branch. Discussion ---------- [SecurityBundle][TwigBridge] Add `is_granted_for_user()` function | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | N/A | License | MIT Twig function to accompany symfony/symfony#48142 Commits ------- 82ff3a5e64f Add is_granted_for_user() function to twig

…tion (natewiebe13) This PR was merged into the 7.3 branch. Discussion ---------- [Security] Add tip for using new isGrantedForUser() function Adds tip for using new `isGrantedForUser()` function from symfony/symfony#48142 Commits ------- c196b44 Add tip for using new isGrantedForUser() function

…kas) This PR was merged into the 7.3 branch. Discussion ---------- [Security] Improve DX of recent additions | 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. Commits ------- aa38eb8 [Security] Improve DX of recent additions

natewiebe13 requested review from wouterj and chalasr as code owners November 7, 2022 17:53

carsonbot added Status: Needs Review Feature labels Nov 7, 2022

carsonbot added this to the 6.2 milestone Nov 7, 2022

This comment was marked as outdated.

Sign in to view

OskarStark reviewed Nov 8, 2022

View reviewed changes

...ony/Component/Security/Core/Tests/Authentication/Token/UserAuthorizationCheckerTokenTest.php Outdated Show resolved Hide resolved

derrabus requested changes Nov 8, 2022

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Nov 8, 2022

derrabus added Security SecurityBundle and removed Status: Needs Work labels Nov 8, 2022

carsonbot changed the title ~~User authorization checker~~ [Security][SecurityBundle] User authorization checker Nov 8, 2022

derrabus reviewed Nov 8, 2022

View reviewed changes

src/Symfony/Bundle/SecurityBundle/Resources/config/security.php Outdated Show resolved Hide resolved

derrabus modified the milestones: 6.2, 6.3 Nov 8, 2022

stof reviewed Nov 8, 2022

View reviewed changes

natewiebe13 requested review from derrabus and removed request for wouterj and chalasr November 8, 2022 15:56

carsonbot added the Status: Needs Review label Nov 8, 2022

natewiebe13 force-pushed the user-authorization-checker branch from 8c0fc43 to 60d5d92 Compare November 8, 2022 18:03

derrabus reviewed Nov 8, 2022

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/UserAuthorizationChecker.php Outdated Show resolved Hide resolved

natewiebe13 requested a review from derrabus November 8, 2022 20:34

HeahDude reviewed Nov 12, 2022

View reviewed changes

src/Symfony/Bundle/SecurityBundle/Resources/config/security.php Outdated Show resolved Hide resolved

natewiebe13 force-pushed the user-authorization-checker branch from c36163a to d28b35e Compare November 28, 2022 15:48

natewiebe13 mentioned this pull request Dec 7, 2022

Using isGranted() without a Session #43372

Closed

94noni approved these changes Jul 23, 2024

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jul 23, 2024

jeremyjaouen approved these changes Aug 12, 2024

View reviewed changes

fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024

natewiebe13 force-pushed the user-authorization-checker branch 2 times, most recently from 8742756 to 260927f Compare December 3, 2024 20:51

stof approved these changes Dec 7, 2024

View reviewed changes

derrabus approved these changes Dec 7, 2024

View reviewed changes

derrabus force-pushed the user-authorization-checker branch from 260927f to 3979093 Compare December 7, 2024 13:34

[Security][SecurityBundle] User authorization checker

096bfaa

derrabus force-pushed the user-authorization-checker branch from 3979093 to 096bfaa Compare December 7, 2024 13:41

derrabus merged commit 4612ff2 into symfony:7.3 Dec 7, 2024
8 of 11 checks passed

natewiebe13 mentioned this pull request Dec 7, 2024

[SecurityBundle][TwigBridge] Add is_granted_for_user() function #59129

Merged

natewiebe13 deleted the user-authorization-checker branch December 7, 2024 14:12

derrabus mentioned this pull request Dec 7, 2024

Remove ServiceSubscriberInterface implementation from Service façade #59133

Merged

smnandre reviewed Dec 14, 2024

View reviewed changes

xabbuh mentioned this pull request Dec 15, 2024

[Security][SecurityBundle] rename userIsGranted() to isGrantedForUser() #59214

Merged

natewiebe13 mentioned this pull request Jan 13, 2025

[AccessControl] Add Access Control component with strategies and voters #59439

Open

This was referenced Feb 17, 2025

[Security] Merge UserAuthorizationCheckerInterface into AuthorizationCheckerInterface #59793

Closed

[Security] Improve DX of recent additions #59805

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security][SecurityBundle] User authorization checker #48142

[Security][SecurityBundle] User authorization checker #48142

natewiebe13 commented Nov 7, 2022 •

edited

Loading

This comment was marked as outdated.

derrabus left a comment

stof Nov 8, 2022

natewiebe13 Nov 8, 2022

stof Nov 8, 2022

natewiebe13 Nov 8, 2022

natewiebe13 Nov 8, 2022

stof Nov 8, 2022

natewiebe13 Nov 8, 2022

HeahDude left a comment

VincentLanglet commented Jan 31, 2023

derrabus commented Jan 31, 2023

94noni left a comment

natewiebe13 commented Dec 3, 2024

derrabus commented Dec 7, 2024

smnandre Dec 14, 2024

chalasr Dec 14, 2024

xabbuh Dec 15, 2024

[Security][SecurityBundle] User authorization checker #48142

[Security][SecurityBundle] User authorization checker #48142

Conversation

natewiebe13 commented Nov 7, 2022 • edited Loading

This comment was marked as outdated.

derrabus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude left a comment

Choose a reason for hiding this comment

VincentLanglet commented Jan 31, 2023

derrabus commented Jan 31, 2023

94noni left a comment

Choose a reason for hiding this comment

natewiebe13 commented Dec 3, 2024

derrabus commented Dec 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natewiebe13 commented Nov 7, 2022 •

edited

Loading