Skip to content

Commit

Permalink
feature #40145 [Security] Rework the remember me system (wouterj)
Browse files Browse the repository at this point in the history
This PR was squashed before being merged into the 5.3-dev branch.

Discussion
----------

[Security] Rework the remember me system

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fixes part of #39308
| License       | MIT
| Doc PR        | tbd

As I said in #39308, I want to change the remember me system in Symfony 5.3. The remember me system has a couple big "problems":

1. **It's hardwired into some Security classes** like `ContextListener`. The `RememberMeFactory` adds a `setRememberMe()` method call to the DI config and the context listener calls methods on this. This is very coupled, instead of the decoupled nature of the rest of security.
2. **Conditional conditions are combined with cookie creation in one class**. This is especially hard in e.g. 2FA (where setting the cookie should be done after 2FA is completed, which is currently near impossible as it's directly bound to the conditional of being called after logging in).

The changes
---

* The first commits harden the current functional test suite of remember me, to avoid breaking it.
* I discovered a lot of similarity between remember me tokens and login links. That's why I've extracted the shared logic into a generic `SignatureHasher` in the 3rd commit.
* I then remodelled `RememberMeAuthenticator` to the login link system, which I think improves a lot and at least improves problem (2) - as the conditionals (`RememberMeAuthenticator`) is split from the cookie creation (`RememberMeHandlerInterface`).
* Finally, I added a new event (`TokenDeauthenticatedEvent`) to the `ContextListener` to avoid direct coupling - solving problem (1).

This removes any usage of remember me services, which can be deprecated along with the rest of the security system.

Usage
---

As with the authenticator manager: **Nothing changes in the configuration**

Usage of persistent token providers has been improved. First, configuration is provided (setting up services is no longer needed):
```yaml
# before
services:
    Symfony\Bridge\Doctrine\Security\RememberMe\DoctrineTokenProvider:
        autowire: true

security:
    firewalls:
        main:
            remember_me:
                # ...
                token_provider: 'Symfony\Bridge\Doctrine\Security\RememberMe\DoctrineTokenProvider'

# after
security:
    firewalls:
        main:
            remember_me:
                # ...
                token_provider:
                    doctrine: true
```

Furthermore, a schema listener is created. Whenever the doctrine token provider is used, `make:migration`/`doctrine:schema:update` will automatically create the required table.

Some advanced usage of Remember me is also improved a lot (there is no real "before" here, consider looking at scheb/2fa to get an idea of the before). A few use-cases I took into account:

* If you ever need to **programmatically create a remember me cookie**, you can autowire `RememberMeHandlerInterface` and use `createRememberMeCookie($user)`. This will make sure the remember me cookie is set on the final response (using the `ResponseListener`)
* The `RememberMeListener` previously was responsible for both determining if a cookie must be set and setting the cookie. This is now split in 2 listeners (checking is done by `RememberMeConditionsListener`). If `RememberMeBadge` is enabled, the cookie is set and otherwise it isn't. This allows e.g. SchebTwoFactorBundle to create a listener that catches whether remember me was requested, but suppress it until the 2nd factor is completed.

Todo
---

* [x] Update UPGRADE and CHANGELOG
* [x] Show before/after examples
* [x] Investigate the conditional event registering of `ContextListener`. This forces to inject both the firewall and the global event dispatcher at the moment.
* Make sure old remember me tokens still function. As remember me tokens are long lived, we may need to provide backwards compatibility for at least Symfony 6.x. **Update: it was decided to not include this for now: #40145 (comment)

cc `@scheb` `@weaverryan` as you both initiated this PR by sharing the problems with the current design.

Commits
-------

1567041 [Security] Rework the remember me system
  • Loading branch information
chalasr committed Apr 11, 2021
2 parents 1e42411 + 1567041 commit b40eac2
Show file tree
Hide file tree
Showing 68 changed files with 2,240 additions and 505 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
<?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\Bridge\Doctrine\SchemaListener;

use Doctrine\Common\EventSubscriber;
use Doctrine\ORM\Tools\Event\GenerateSchemaEventArgs;
use Doctrine\ORM\Tools\ToolEvents;
use Symfony\Bridge\Doctrine\Security\RememberMe\DoctrineTokenProvider;
use Symfony\Component\Security\Http\RememberMe\PersistentRememberMeHandler;
use Symfony\Component\Security\Http\RememberMe\RememberMeHandlerInterface;

/**
* Automatically adds the rememberme table needed for the {@see DoctrineTokenProvider}.
*
* @author Wouter de Jong <wouter@wouterj.nl>
*/
final class RememberMeTokenProviderDoctrineSchemaSubscriber implements EventSubscriber
{
private $rememberMeHandlers;

/**
* @param iterable|RememberMeHandlerInterface[] $rememberMeHandlers
*/
public function __construct(iterable $rememberMeHandlers)
{
$this->rememberMeHandlers = $rememberMeHandlers;
}

public function postGenerateSchema(GenerateSchemaEventArgs $event): void
{
$dbalConnection = $event->getEntityManager()->getConnection();

foreach ($this->rememberMeHandlers as $rememberMeHandler) {
if (
$rememberMeHandler instanceof PersistentRememberMeHandler
&& ($tokenProvider = $rememberMeHandler->getTokenProvider()) instanceof DoctrineTokenProvider
) {
$tokenProvider->configureSchema($event->getSchema(), $dbalConnection);
}
}
}

public function getSubscribedEvents(): array
{
if (!class_exists(ToolEvents::class)) {
return [];
}

return [
ToolEvents::postGenerateSchema,
];
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,15 @@
use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Driver\Result as DriverResult;
use Doctrine\DBAL\Result;
use Doctrine\DBAL\Schema\Schema;
use Doctrine\DBAL\Types\Types;
use Symfony\Component\Security\Core\Authentication\RememberMe\PersistentToken;
use Symfony\Component\Security\Core\Authentication\RememberMe\PersistentTokenInterface;
use Symfony\Component\Security\Core\Authentication\RememberMe\TokenProviderInterface;
use Symfony\Component\Security\Core\Exception\TokenNotFoundException;

/**
* This class provides storage for the tokens that is set in "remember me"
* This class provides storage for the tokens that is set in "remember-me"
* cookies. This way no password secrets will be stored in the cookies on
* the client machine, and thus the security is improved.
*
Expand Down Expand Up @@ -53,8 +54,7 @@ public function __construct(Connection $conn)
public function loadTokenBySeries(string $series)
{
// the alias for lastUsed works around case insensitivity in PostgreSQL
$sql = 'SELECT class, username, value, lastUsed AS last_used'
.' FROM rememberme_token WHERE series=:series';
$sql = 'SELECT class, username, value, lastUsed AS last_used FROM rememberme_token WHERE series=:series';
$paramValues = ['series' => $series];
$paramTypes = ['series' => \PDO::PARAM_STR];
$stmt = $this->conn->executeQuery($sql, $paramValues, $paramTypes);
Expand Down Expand Up @@ -87,8 +87,7 @@ public function deleteTokenBySeries(string $series)
*/
public function updateToken(string $series, string $tokenValue, \DateTime $lastUsed)
{
$sql = 'UPDATE rememberme_token SET value=:value, lastUsed=:lastUsed'
.' WHERE series=:series';
$sql = 'UPDATE rememberme_token SET value=:value, lastUsed=:lastUsed WHERE series=:series';
$paramValues = [
'value' => $tokenValue,
'lastUsed' => $lastUsed,
Expand All @@ -114,9 +113,7 @@ public function updateToken(string $series, string $tokenValue, \DateTime $lastU
*/
public function createNewToken(PersistentTokenInterface $token)
{
$sql = 'INSERT INTO rememberme_token'
.' (class, username, series, value, lastUsed)'
.' VALUES (:class, :username, :series, :value, :lastUsed)';
$sql = 'INSERT INTO rememberme_token (class, username, series, value, lastUsed) VALUES (:class, :username, :series, :value, :lastUsed)';
$paramValues = [
'class' => $token->getClass(),
// @deprecated since 5.3, change to $token->getUserIdentifier() in 6.0
Expand All @@ -138,4 +135,32 @@ public function createNewToken(PersistentTokenInterface $token)
$this->conn->executeUpdate($sql, $paramValues, $paramTypes);
}
}

/**
* Adds the Table to the Schema if "remember me" uses this Connection.
*/
public function configureSchema(Schema $schema, Connection $forConnection): void
{
// only update the schema for this connection
if ($forConnection !== $this->conn) {
return;
}

if ($schema->hasTable('rememberme_token')) {
return;
}

$this->addTableToSchema($schema);
}

private function addTableToSchema(Schema $schema): void
{
$table = $schema->createTable('rememberme_token');
$table->addColumn('series', Types::STRING, ['length' => 88]);
$table->addColumn('value', Types::STRING, ['length' => 88]);
$table->addColumn('lastUsed', Types::DATETIME_MUTABLE);
$table->addColumn('class', Types::STRING, ['length' => 100]);
$table->addColumn('username', Types::STRING, ['length' => 200]);
$table->setPrimaryKey(['series']);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ class UnusedTagsPass implements CompilerPassInterface
'security.authenticator.login_linker',
'security.expression_language_provider',
'security.remember_me_aware',
'security.remember_me_handler',
'security.voter',
'serializer.encoder',
'serializer.normalizer',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use Symfony\Component\Security\Http\Event\LoginFailureEvent;
use Symfony\Component\Security\Http\Event\LoginSuccessEvent;
use Symfony\Component\Security\Http\Event\LogoutEvent;
use Symfony\Component\Security\Http\Event\TokenDeauthenticatedEvent;
use Symfony\Component\Security\Http\SecurityEvents;

/**
Expand All @@ -44,6 +45,7 @@ class RegisterGlobalSecurityEventListenersPass implements CompilerPassInterface
AuthenticationTokenCreatedEvent::class,
AuthenticationSuccessEvent::class,
InteractiveLoginEvent::class,
TokenDeauthenticatedEvent::class,

// When events are registered by their name
AuthenticationEvents::AUTHENTICATION_SUCCESS,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
<?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\Bundle\SecurityBundle\DependencyInjection\Compiler;

use Symfony\Bundle\SecurityBundle\RememberMe\DecoratedRememberMeHandler;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;

/**
* Replaces the DecoratedRememberMeHandler services with the real definition.
*
* @author Wouter de Jong <wouter@wouterj.nl>
*
* @internal
*/
final class ReplaceDecoratedRememberMeHandlerPass implements CompilerPassInterface
{
private const HANDLER_TAG = 'security.remember_me_handler';

/**
* {@inheritdoc}
*/
public function process(ContainerBuilder $container): void
{
$handledFirewalls = [];
foreach ($container->findTaggedServiceIds(self::HANDLER_TAG) as $definitionId => $rememberMeHandlerTags) {
$definition = $container->findDefinition($definitionId);
if (DecoratedRememberMeHandler::class !== $definition->getClass()) {
continue;
}

// get the actual custom remember me handler definition (passed to the decorator)
$realRememberMeHandler = $container->findDefinition((string) $definition->getArgument(0));
if (null === $realRememberMeHandler) {
throw new \LogicException(sprintf('Invalid service definition for custom remember me handler; no service found with ID "%s".', (string) $definition->getArgument(0)));
}

foreach ($rememberMeHandlerTags as $rememberMeHandlerTag) {
// some custom handlers may be used on multiple firewalls in the same application
if (\in_array($rememberMeHandlerTag['firewall'], $handledFirewalls, true)) {
continue;
}

$rememberMeHandler = clone $realRememberMeHandler;
$rememberMeHandler->addTag(self::HANDLER_TAG, $rememberMeHandlerTag);
$container->setDefinition('security.authenticator.remember_me_handler.'.$rememberMeHandlerTag['firewall'], $rememberMeHandler);

$handledFirewalls[] = $rememberMeHandlerTag['firewall'];
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -113,18 +113,24 @@ public function createAuthenticator(ContainerBuilder $container, string $firewal
->replaceArgument(1, $config['lifetime']);
}

$signatureHasherId = 'security.authenticator.login_link_signature_hasher.'.$firewallName;
$container
->setDefinition($signatureHasherId, new ChildDefinition('security.authenticator.abstract_login_link_signature_hasher'))
->replaceArgument(1, $config['signature_properties'])
->replaceArgument(3, $expiredStorageId ? new Reference($expiredStorageId) : null)
->replaceArgument(4, $config['max_uses'] ?? null)
;

$linkerId = 'security.authenticator.login_link_handler.'.$firewallName;
$linkerOptions = [
'route_name' => $config['check_route'],
'lifetime' => $config['lifetime'],
'max_uses' => $config['max_uses'] ?? null,
];
$container
->setDefinition($linkerId, new ChildDefinition('security.authenticator.abstract_login_link_handler'))
->replaceArgument(1, new Reference($userProviderId))
->replaceArgument(3, $config['signature_properties'])
->replaceArgument(5, $linkerOptions)
->replaceArgument(6, $expiredStorageId ? new Reference($expiredStorageId) : null)
->replaceArgument(2, new Reference($signatureHasherId))
->replaceArgument(3, $linkerOptions)
->addTag('security.authenticator.login_linker', ['firewall' => $firewallName])
;

Expand Down

0 comments on commit b40eac2

Please sign in to comment.