[Security][SecurityBundle] User authorization checker #48142
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
...ony/Component/Security/Core/Tests/Authentication/Token/UserAuthorizationCheckerTokenTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authentication/Token/UserAuthorizationCheckerToken.php
Outdated
Show resolved
Hide resolved
8000
src/Symfony/Component/Security/Core/Authentication/Token/UserAuthorizationCheckerToken.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/UserAuthorizationChecker.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Tests/Authorization/UserAuthorizationCheckerTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Tests/Authorization/UserAuthorizationCheckerTest.php
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Tests/Authorization/UserAuthorizationCheckerTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Tests/Authorization/UserAuthorizationCheckerTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Tests/Authorization/UserAuthorizationCheckerTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Tests/Authorization/UserAuthorizationCheckerTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/UserAuthorizationChecker.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/SecurityBundle/Resources/config/security.php
Outdated
Show resolved
Hide resolved
| || 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)); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I'm not sure abstaining is always the right answer either though (the final decision won't ever be an abstain)
There was a problem hiding this comment.
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.
| @@ -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. | |||
There was a problem hiding this comment.
this should explicitly document the differences with isGranted() IMO, to make its limitation clear
There was a problem hiding this comment.
Updated, is this what you were looking for?
src/Symfony/Component/Security/Core/Authentication/Token/UserAuthorizationCheckerToken.php
Outdated
Show resolved
Hide resolved
8c0fc43 to
60d5d92
Compare
src/Symfony/Component/Security/Core/Authorization/UserAuthorizationChecker.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/SecurityBundle/Resources/config/security.php
Outdated
Show resolved
Hide resolved
c36163a to
d28b35e
Compare
|
This would be a great feature. What is missing to this PR @natewiebe13 @derrabus @stof ? |
Another round of reviews, I suppose. I'd very much like to see this feature shipped as well. |
8742756 to
260927f
Compare
|
Rebased for 7.3 🤞 |
260927f to
3979093
Compare
3979093 to
096bfaa
Compare
|
It took time, but here we go, this is in now. Thank you very much @natewiebe13. |
| * | ||
| * 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 |
There was a problem hiding this comment.
👍 discussed internally, either this or isGrantedForUser()
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.