Skip to content

Commit

Permalink
minor #36520 [Security] Apply left-over review comments from #33558 (…
Browse files Browse the repository at this point in the history
…wouterj)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[Security] Apply left-over review comments from #33558

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | n/a

This applies the review comments of @noniagriconomie in #33558. It's mostly doc fixes and one extra security-safeguard by resetting the plaintext password early (similair to what is done in `PasswordCredentials`).

Commits
-------

be3a9a9 Applied left-over review comments from #33558
  • Loading branch information
chalasr committed Apr 21, 2020
2 parents 6b682bf + be3a9a9 commit 80444e8
Show file tree
Hide file tree
Showing 6 changed files with 26 additions and 18 deletions.
Expand Up @@ -77,7 +77,7 @@ public function authenticateUser(UserInterface $user, AuthenticatorInterface $au
public function supports(Request $request): ?bool
{
if (null !== $this->logger) {
$context = ['firewall_key' => $this->firewallName];
$context = ['firewall_name' => $this->firewallName];

if ($this->authenticators instanceof \Countable || \is_array($this->authenticators)) {
$context['authenticators'] = \count($this->authenticators);
Expand All @@ -90,14 +90,14 @@ public function supports(Request $request): ?bool
$lazy = true;
foreach ($this->authenticators as $authenticator) {
if (null !== $this->logger) {
$this->logger->debug('Checking support on authenticator.', ['firewall_key' => $this->firewallName, 'authenticator' => \get_class($authenticator)]);
$this->logger->debug('Checking support on authenticator.', ['firewall_name' => $this->firewallName, 'authenticator' => \get_class($authenticator)]);
}

if (false !== $supports = $authenticator->supports($request)) {
$authenticators[] = $authenticator;
$lazy = $lazy && null === $supports;
} elseif (null !== $this->logger) {
$this->logger->debug('Authenticator does not support the request.', ['firewall_key' => $this->firewallName, 'authenticator' => \get_class($authenticator)]);
$this->logger->debug('Authenticator does not support the request.', ['firewall_name' => $this->firewallName, 'authenticator' => \get_class($authenticator)]);
}
}

Expand Down
Expand Up @@ -19,7 +19,7 @@
*
* This is used to not break AuthenticationChecker and ContextListener when
* using the authenticator system. Once the authenticator system is no longer
* experimental, this class can be used trigger deprecation notices.
* experimental, this class can be used to trigger deprecation notices.
*
* @internal
*
Expand Down
Expand Up @@ -24,7 +24,7 @@
interface UserAuthenticatorInterface
{
/**
* Convenience method to manually login a user and return a
* Convenience method to programmatically login a user and return a
* Response *if any* for success.
*/
public function authenticateUser(UserInterface $user, AuthenticatorInterface $authenticator, Request $request): ?Response;
Expand Down
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\Security\Http\Authenticator\Passport\Badge;

use Symfony\Component\Security\Core\Exception\LogicException;
use Symfony\Component\Security\Core\User\PasswordUpgraderInterface;

/**
Expand Down Expand Up @@ -38,24 +39,23 @@ public function __construct(string $plaintextPassword, PasswordUpgraderInterface
$this->passwordUpgrader = $passwordUpgrader;
}

public function getPlaintextPassword(): string
public function getAndErasePlaintextPassword(): string
{
return $this->plaintextPassword;
$password = $this->plaintextPassword;
if (null === $password) {
throw new LogicException('The password is erased as another listener already used this badge.');
}

$this->plaintextPassword = null;

return $password;
}

public function getPasswordUpgrader(): PasswordUpgraderInterface
{
return $this->passwordUpgrader;
}

/**
* @internal
*/
public function eraseCredentials()
{
$this->plaintextPassword = null;
}

public function isResolved(): bool
{
return true;
Expand Down
@@ -1,5 +1,14 @@
<?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\EventListener;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
Expand Down Expand Up @@ -32,8 +41,7 @@ public function onLoginSuccess(LoginSuccessEvent $event): void

/** @var PasswordUpgradeBadge $badge */
$badge = $passport->getBadge(PasswordUpgradeBadge::class);
$plaintextPassword = $badge->getPlaintextPassword();
$badge->eraseCredentials();
$plaintextPassword = $badge->getAndErasePlaintextPassword();

if ('' === $plaintextPassword) {
return;
Expand Down
Expand Up @@ -17,7 +17,7 @@
use Symfony\Component\Security\Http\Session\SessionAuthenticationStrategyInterface;

/**
* Migrates/invalidate the session after successful login.
* Migrates/invalidates the session after successful login.
*
* This should be registered as subscriber to any "stateful" firewalls.
*
Expand Down

0 comments on commit 80444e8

Please sign in to comment.