[Security] Allow AuthorizationChecker::isGrantedForUser to check for guest permissions #61938
Conversation
This comment has been minimized.
This comment has been minimized.
|
Indeed, I see no BC path to add the nullable type to the interface. |
|
We could introduce a class that decorates the existing one with a different signature and creates such an empty user when forwarding the call. Or maybe another helper in |
|
Such a method could also be placed in the |
|
@aschempp there is one more possible option, to make Guest user a separate class, which implements Userinterface too. But it may require a lot of changes in legacy systems. |
|
Given the purpose of this method and the fact that one aim of the now well-established authenticator system was to remove the concept of unauthenticated tokens (see e.g. #42050 and #42510), I don't think we should try to address this here, passing some kind of null user on the caller's side seems good enough. |
That would be perfectly fine with me. But shouldn't the security system then return such a null user in e.g. As an example of our current system (Contao CMS), we have permissions on objects that can be given to certain user groups, or to guests. Our voters then decides whether there is a user in the current system and which groups it belongs to, or if the object is allowed for guests. If there's a new Btw. would you consider this a bug (to be fixed in 7.3 possibly) or a feature to check for guest users? |
|
The most backward and forward compatible solution might be to create a new Let me know if you like that idea, I can then update the PR (we can still decide on final naming later). |
There was a problem hiding this comment.
how critical is it to have this in 7.4 vs 8.1?
src/Symfony/Component/Security/Core/Authorization/UserAndGuestAuthorizationCheckerInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/UserAuthorizationCheckerInterface.php
Outdated
Show resolved
Hide resolved
It would help me to have it in 7.4, but I can also make it work without. I don't see how waiting until 8.1 would solve any issue though, because we can't trigger a deprecation that we want to add an argument that isn't possible 😅 I think I found a neater solution. The implementation class ( interface UserAuthorizationCheckerInterface
{
public function isGrantedForUser(UserInterface $user, mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool;
}
interface UserGuestAuthorizationCheckerInterface
{
public function isGrantedForUser(?UserInterface $user, mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool;
}
class AuthorizationChecker implements UserAuthorizationCheckerInterface, UserGuestAuthorizationCheckerInterface
{
final public function isGrantedForUser(?UserInterface $user, mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
{
// ...
}
} |
There was a problem hiding this comment.
don't forget adding autowiring aliases
| * | ||
| * @author Nate Wiebe <nate@northern.co> | ||
| * | ||
| * @deprecated since Symfony 7.4, implement the UserGuestAuthorizationCheckerInterface instead. |
There was a problem hiding this comment.
do we have to deprecate? it might be better not to?
There was a problem hiding this comment.
you mean someone in Symfony 8.x might want to implement only checking for users but not for guests? I would think of the new interface as replacing the current one (because it allows both), but I'm fine with either way. Maybe we could also come up with something more generic than UserGuest... that would work as a better replacement in the future 😅
There was a problem hiding this comment.
Autowiring alias added.
What do you think of OfflineAuthorizationCheckerInterface since we have an OfflineToken as well?
There was a problem hiding this comment.
Dunno, others might have an opinion :) (please mind fabbot, it still has spotted a few things)
1f73361 to
08fcd51
Compare
There was a problem hiding this comment.
This approach makes sense to me.
08fcd51 to
4b2d659
Compare
|
The failing 8.4 (high deps) test seems "normal", since the interface added in this PR is not available in the dependency when using 8.x-dev. Everything else seems fixed after a rebase. Let me know if someone has better ideas for the interface name, otherwise this is RTM for me 😊 |
Unfortunately, the new
User authorization checkeradded in #48142 (Symfony 7.3) does not allow to check for a guest (a token without a user).This PR should be seen as draft because the code is not backwards compatible. It feels wrong to add anotherisGrantedForGuestmethod – any idea about a BC way? The "usual"func_get_argswouldn't work here I think?/cc @natewiebe13