Skip to content
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.

Sign up for GitHub

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

Merged
merged 5 commits into from
May 25, 2021
Merged

EZP-32333: Validate access for 'change password' menu item #92

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
55f6a7d
EZP-32333: Validate access for 'change password' menu item
barw4 May 21, 2021
bc879a4
EZP-32333: Skipped unnecessary trycatch
barw4 May 24, 2021
7ffeb70
EZP-32333: CS
barw4 May 24, 2021
0363c6f
EZP-32333: Removed unnecessary token storage check
barw4 May 24, 2021
df1b5a8
EZP-32333: CS
barw4 May 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions src/lib/EventListener/UserMenuListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@

namespace EzSystems\EzPlatformUser\EventListener;

use eZ\Publish\API\Repository\Exceptions\NotFoundException;
use eZ\Publish\API\Repository\PermissionResolver;
use eZ\Publish\API\Repository\UserService;
use EzSystems\EzPlatformAdminUi\Menu\Event\ConfigureMenuEvent;
use JMS\TranslationBundle\Model\Message;
use JMS\TranslationBundle\Translation\TranslationContainerInterface;
Expand All @@ -21,12 +24,25 @@ class UserMenuListener implements EventSubscriberInterface, TranslationContainer
/** @var \Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface */
private $tokenStorage;

/** @var \eZ\Publish\API\Repository\PermissionResolver */
private $permissionResolver;

/** @var \eZ\Publish\API\Repository\UserService */
private $userService;

/**
* @param \Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface $tokenStorage
* @param \eZ\Publish\API\Repository\PermissionResolver $permissionResolver
* @param \eZ\Publish\API\Repository\UserService $userService
barw4 marked this conversation as resolved.
Show resolved Hide resolved
*/
public function __construct(TokenStorageInterface $tokenStorage)
{
public function __construct(
TokenStorageInterface $tokenStorage,
PermissionResolver $permissionResolver,
UserService $userService
) {
$this->tokenStorage = $tokenStorage;
$this->permissionResolver = $permissionResolver;
$this->userService = $userService;
}

/**
Expand All @@ -45,7 +61,17 @@ public function onUserMenuConfigure(ConfigureMenuEvent $event): void
$menu = $event->getMenu();
$token = $this->tokenStorage->getToken();

if (null !== $token && is_object($token->getUser())) {
$currentUserId = $this->permissionResolver->getCurrentUserReference()->getUserId();
try {
$currentUser = $this->userService->loadUser($currentUserId);
} catch (NotFoundException $e) {
barw4 marked this conversation as resolved.
Show resolved Hide resolved
return;
}

if (null !== $token &&
is_object($token->getUser()) &&
Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Contributor

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?

Copy link
Member Author

@barw4 barw4 May 24, 2021 •
edited
Loading

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.

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

@ViniTou ViniTou May 24, 2021 •
edited
Loading

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

0363c6f

$this->permissionResolver->canUser('user', 'password', $currentUser, [$currentUser])
) {
$menu->addChild(
self::ITEM_CHANGE_PASSWORD,
[
Expand Down