Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[Security] Don't invalidate the user when the password was not stored in the session #59539

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

Merged
merged 1 commit into from
Jan 29, 2025
Merged

[Security] Don't invalidate the user when the password was not stored in the session #59539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
[Security] Don't invalidate the user when the password was not stored… 
… in the session
  • Loading branch information
@nicolas-grekas
nicolas-grekas committed Jan 29, 2025
commit 3d618db98f5d6184a1ca8c9042e07c3d3eb375e2
32 changes: 13 additions & 19 deletions 32 src/Symfony/Component/Security/Core/Tests/Authentication/Token/Fixtures/CustomUser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,24 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Security\Core\Tests\Authentication\Token\Fixtures;

use Symfony\Component\Security\Core\User\UserInterface;

final class CustomUser implements UserInterface
{
/** @var string */
private $username;
/** @var array */
private $roles;

public function __construct(string $username, array $roles)
{
$this->username = $username;
$this->roles = $roles;
public function __construct(
private string $username,
private array $roles,
) {
Copy link
Member Author

Choose a reason for hiding this comment

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

CPP FTW

}

public function getUserIdentifier(): string
Expand All @@ -27,16 +31,6 @@ public function getRoles(): array
return $this->roles;
}

public function getPassword(): ?string
{
return null;
}

public function getSalt(): ?string
{
return null;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

legacy stuff


public function eraseCredentials(): void
{
}
Expand Down
7 changes: 5 additions & 2 deletions 7 src/Symfony/Component/Security/Http/Firewall/ContextListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ public function onKernelResponse(ResponseEvent $event): void
*
* @throws \RuntimeException
*/
protected function refreshUser(TokenInterface $token): ?TokenInterface
private function refreshUser(TokenInterface $token): ?TokenInterface
Copy link
Member Author

Choose a reason for hiding this comment

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

the class is @final

{
$user = $token->getUser();

Expand Down Expand Up @@ -292,7 +292,10 @@ private static function hasUserChanged(UserInterface $originalUser, TokenInterfa
}

if ($originalUser instanceof PasswordAuthenticatedUserInterface || $refreshedUser instanceof PasswordAuthenticatedUserInterface) {
if (!$originalUser instanceof PasswordAuthenticatedUserInterface || !$refreshedUser instanceof PasswordAuthenticatedUserInterface || $originalUser->getPassword() !== $refreshedUser->getPassword()) {
if (!$originalUser instanceof PasswordAuthenticatedUserInterface
|| !$refreshedUser instanceof PasswordAuthenticatedUserInterface
|| $refreshedUser->getPassword() !== ($originalUser->getPassword() ?? $refreshedUser->getPassword())
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the meat of this PR: ignore null passwords from the session storage

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this also mean that you lose the ability to detect that the user changed the password?

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely. See the main thread of this PR for some thoughts on the topic.

) {
return true;
}

Expand Down
6 changes: 1 addition & 5 deletions 6 src/Symfony/Component/Security/Http/Tests/Firewall/AccessListenerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,7 @@ public function testHandleWhenTheAccessDecisionManagerDecidesToRefuseAccess()
->willReturn([['foo' => 'bar'], null])
;

$token = new class extends AbstractToken {
public function getCredentials(): mixed
Copy link
Member Author

Choose a reason for hiding this comment

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

legacy stuff also

{
}
};
$token = new class extends AbstractToken {};

$tokenStorage = $this->createMock(TokenStorageInterface::class);
$tokenStorage
Expand Down
24 changes: 20 additions & 4 deletions 24 src/Symfony/Component/Security/Http/Tests/Firewall/ContextListenerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
use Symfony\Component\Security\Core\User\UserInterface;
use Symfony\Component\Security\Core\User\UserProviderInterface;
use Symfony\Component\Security\Http\Firewall\ContextListener;
use Symfony\Component\Security\Http\Tests\Fixtures\CustomUser;
use Symfony\Component\Security\Http\Tests\Fixtures\NullUserToken;
use Symfony\Contracts\Service\ServiceLocatorTrait;

Expand Down Expand Up @@ -376,6 +377,25 @@ public function testOnKernelResponseRemoveListener()
$this->assertEmpty($dispatcher->getListeners());
}

public function testRemovingPasswordFromSessionDoesntInvalidateTheToken()
{
$user = new CustomUser('user', ['ROLE_USER'], 'pass');

$userProvider = $this->createMock(UserProviderInterface::class);
$userProvider->expects($this->once())
->method('supportsClass')
->with(CustomUser::class)
->willReturn(true);
$userProvider->expects($this->once())
->method('refreshUser')
->willReturn($user);

$tokenStorage = $this->handleEventWithPreviousSession([$userProvider], $user);

$this->assertInstanceOf(UsernamePasswordToken::class, $tokenStorage->getToken());
$this->assertSame($user, $tokenStorage->getToken()->getUser());
}

protected function runSessionOnKernelResponse($newToken, $original = null)
{
$session = new Session(new MockArraySessionStorage());
Expand Down Expand Up @@ -568,10 +588,6 @@ public function getRoleNames(): array
return $this->roles;
}

public function getCredentials()
{
}

public function getUser(): UserInterface
{
return $this->user;
Expand Down
49 changes: 49 additions & 0 deletions 49 src/Symfony/Component/Security/Http/Tests/Fixtures/CustomUser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Security\Http\Tests\Fixtures;

use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
use Symfony\Component\Security\Core\User\UserInterface;

final class CustomUser implements UserInterface, PasswordAuthenticatedUserInterface
{
public function __construct(
private string $username,
private array $roles,
private ?string $password = null,
) {
}

public function getUserIdentifier(): string
{
return $this->username;
}

public function getRoles(): array
{
return $this->roles;
}

public function getPassword(): ?string
{
return $this->password ?? null;
}

public function eraseCredentials(): void
{
}

public function __serialize(): array
{
return [\sprintf("\0%s\0username", self::class) => $this->username];
}
}
Morty Proxy This is a proxified and sanitized view of the page, visit original site.