-
Notifications
You must be signed in to change notification settings - Fork 11
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
EZP-32333: Validate access for 'change password' menu item #92
Conversation
} | ||
|
||
if (null !== $token && | ||
is_object($token->getUser()) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we get currentUser from $token
instead of fetching it from userService?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly not as $token
is instance of Symfony\Component\Security\Core\User\UserInterface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm.
I would say token is instance of \Symfony\Component\Security\Core\Authentication\Token\TokenInterface
,
And you can get \eZ\Publish\Core\MVC\Symfony\Security\User
from getUser
, and then API user with getAPIUser
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, sorry, I mean getUser
on $token
was an instance of Symfony\Component\Security\Core\User\UserInterface
, not the token itself. I'll take a second look at this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually can't get user with getAPIUser
as this method is inside eZ\Publish\Core\MVC\Symfony\Security\UserInterface
whether $token->getUser()
gives us Symfony\Component\Security\Core\User\UserInterface
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we even bother asking TokenStorage
for the User if we store it elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was probably used as a first resort to validate the user, but if we should actually check it via permission resolver this could be skipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, yes if you look at interfaces, but our implementation returns \Symfony\Component\Security\Core\User\User
we could check for it (or more precise, checking if user is instance of \eZ\Publish\Core\MVC\Symfony\Security\ReferenceUserInterface
, and act accordingly. or remove tokenStorage alltogether and just relay on userService.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA - Approved. Tested on 2.5.18
Could you please merge up changes @barw4? |
As the title states.
Required
ezplatform-admin-ui
PR: ezsystems/ezplatform-admin-ui#1759Checklist:
$ composer fix-cs
)