[Security] Refactor logout listener to dispatch an event instead #36243

wouterj · 2020-03-27T18:28:08Z

Q	A
Branch?	master
Bug fix?	yes (sort of...)
New feature?	yes
Deprecations?	yes
Tickets	Fix #25212, Fix #22473
License	MIT
Doc PR	tbd

The current LogoutListener has some extension points, but they are not really DX-friendly (ref #25212). It requires hacking a addMethodCall('addHandler') in the container builder to register a custom logout handler.
Also, it is impossible to overwrite the default logout functionality from a bundle (ref #22473).

This PR introduces a LogoutEvent that replaces both the LogoutSuccessHandlerInterface and LogoutHandlerInterface. This provides a DX-friendly extension point and also cleans up the authentication factories (no more addMethodCall()'s).

In order to allow different logout handlers for different firewalls, I created a specific event dispatcher for each firewall (as also shortly discussed in #33558). The dispatcher tag attribute allows you to specify which dispatcher it should be registered to (defaulting to the global dispatcher). The EventBubblingLogoutListener also dispatches logout events on the global dispatcher, to be used for listeners that should run on all firewalls.

@weaverryan and I discussed this feature while working on #33558, but figured it was unrelated and could be done while preservering BC. So that's why a separate PR is created.

wouterj · 2020-03-27T18:43:56Z

Please note that the fabbot exception message error is a false-positive

nicolas-grekas

Here is a superficial review.

src/Symfony/Component/Security/Http/Firewall/LogoutListener.php

UPGRADE-5.1.md

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/RememberMeFactory.php

src/Symfony/Bundle/SecurityBundle/Security/LegacyLogoutHandlerListener.php

src/Symfony/Component/Security/Http/Firewall/LogoutListener.php

src/Symfony/Component/Security/Http/Logout/LogoutSuccessHandlerInterface.php

src/Symfony/Component/Security/Http/Tests/Firewall/LogoutListenerTest.php

linaori

I think this is a really nice change, and I hope that adding this to the event listener documentation will also make this behavior more accessible to developers 😄

linaori · 2020-03-28T20:16:00Z

src/Symfony/Component/Security/Http/Firewall/LogoutListener.php

-            }
+        $response = $logoutEvent->getResponse();
+        if (!$response instanceof Response) {
+            throw new \RuntimeException('No logout listener set the Response, make sure at least the DefaultLogoutListener is registered.');


I feel like the second part of the exception doesn't really belong to a run-time exception: "make sure at least the DefaultLogoutListener is registered."

What if we reword it to something in form of "Register the DefaultLogoutListener as LogoutEvent listener when no other listener is supposed to give a response"? Probably not the best either, though it does give less of a "you should've registered X" feeling.

trsteel88 · 2020-04-02T01:50:25Z

+1

I've never understood why the security component uses Handlers instead of events. I'd love to see it using events for everything. For example:

LogoutEvent -> Replaces the need to override the logout handler
AuthenticationFailureEvent -> Replaces the need to override the authentication failure handler
AuthenticationSuccessEvent -> Replaces the need to override the authentication success handler

fabpot · 2020-04-04T09:30:31Z

src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php

+                $dispatcherDefinitions[] = $container->getDefinition($attributes['dispatcher']);
+            }
+
+            if ([] === $dispatcherDefinitions) {


if (!$dispatcherDefinitions) {

fabpot · 2020-04-04T11:02:38Z

Thank you @wouterj.

src/Symfony/Component/Security/Http/Logout/DefaultLogoutSuccessHandler.php

src/Symfony/Component/Security/Http/Logout/LogoutSuccessHandlerInterface.php

…s" first-class security (wouterj) This PR was squashed before being merged into the 5.1-dev branch. Discussion ---------- [Security] AuthenticatorManager to make "authenticators" first-class security | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | - | License | MIT | Doc PR | tbd The tl;dr --- The old authentication listener + authentication provider system was replaced by a new "authenticator" system (similar to Guard authentication). All existing "auth systems" (e.g. `form_login` are now written as an "authenticator" in core). Instead of each "authentication system" registering its own listener in the `Firewall`, there is now only one listener: `AuthenticatorManagerListener` * `Firewall` -> executes `AuthenticatorManagerListener` * `AuthenticatorManagerListener` -> calls `AuthenticatorManager` * `AuthenticatorManager` -> calls each authenticator This PR contains *no deprecations* and the "new system" is *marked as experimental*. This allows to continue to develop the new Security system during the 5.x release cycle without disturbing Symfony users. In 5.4, we can deprecate "old" Security and remove it completely in 6.0. Important Decisions --- * A) **The new authentication manager - `AuthenticatorManager` - now dispatches 3 important "hook" events**: * `VerifyAuthenticatorCredentialsEvent`: occurs at the point when a "password" needs to be checked. Allows us to centralize password checking, CSRF validation, password upgrading and the "user checker" logic. * `LoginSuccessEvent`: Dispatched after a successful authentication. E.g. used by remember me listener. * `LoginFailedEvent`: Dispatched after an unsuccessful authentication. Also used by remember me (and in theory could be used for login throttling). * B) **`getCredentials()`, `getUser()` and `checkCredentials()` methods from old Guard are gone: their logic is centralized**. Authenticators now have an `authenticate(Request $request): PassportInterface` method. A passport contains the user object, the credentials and any other add-in Security badges (e.g. CSRF): ```php public function authenticate(Request $request): PassportInterface { return new Passport( $user, new PasswordCredentials($request->get('_password')), [ new CsrfBadge($request->get('_token')) ] ); } ``` All badges (including the credentials) need to be resolved by listeners to `VerifyAuthenticatorCredentialsEvent`. There is build-in core support for the following badges/credentials: * `PasswordCredentials`: validated using the password encoder factory * `CustomCredentials`: allows a closure to do credentials checking * `CsrfTokenBadge`: automatic CSRF token verification * `PasswordUpgradeBadge`: enables password migration * `RememberMeBadge`: enables remember-me support for this authenticator * C) **`AuthenticatorManager` contains all logic to authenticate** As authenticators always relate to HTTP, the `AuthenticatorManager` contains all logic to authenticate. It has three methods, the most important two are: * `authenticateRequest(Request $request): TokenInterface`: Doing what is previously done by a listener and an authentication provider; * `authenticateUser(UserInterface $user, AuthenticatorInterface $authenticator, Request $request, array $badges = [])` for manual login in e.g. a controller. * D) **One AuthenticatorManager per firewall** In the old system, there was 1 authentication manager containing all providers and each firewall had a specific firewall listener. In the new system, each firewall has a specific authentication manager. * E) **Pre-authentication tokens are dropped.** As everything is now handled inside `AuthenticatorManager` and everything is stored in the Security `Passport`, there was no need for a token anymore (removing lots of confusion about what information is inside the token). This change deprecates 2 authentication calls: one in `AuthorizationChecker#isGranted()` and one in `AccessListener`. These seem now to be mis-used to reload users (e.g. re-authenticate the user after you change their roles). This (some "way" to change a user's roles *without* logging them out) needs to be "fixed"/added in another PR. * F) **The remember me service now uses *all* user providers** Previously, only user providers of authentication providers listening on that firewall were used. This change is due to practical reasons and we don't think it is common to have 2 user providers supporting the same user instance. In any case, you can always explicitly configure the user provider under `remember_me`. * G) **Auth Providers No Longer Clear the Token on Auth Failure** Previously, authentication providers did `$this->tokenStorage->setToken(null)` upon authentication failure. This is not yet implemented: our reasoning is that if you've authenticated successfully using e.g. the login form, why should you be logged out if you visit the same login form and enter wrong credentials? The pre-authenticated authenticators are an exception here, they do reset the token upon authentication failure, just like the old system. * H) **CSRF Generator Service ID No Longer Configurable** The old Form login authentication provider allowed you to configure the CSRF generator service ID. This is no longer possible with the automated CSRF listener. This feature was introduced in the first CSRF commit and didn't get any updates ever since, so we don't think this feature is required. This could also be accomplished by checking CSRF manually in your authenticator, instead of using the automated check. Future Considerations --- * Remove Security sub-components: Move CSRF to `Symfony\Component\Csrf` (just like mime); Deprecated Guard; Put HTTP + Core as `symfony/security`. This means moving the new classes to `Symfony\Component\Security` * Convert LDAP to the new system * This is fixed (and merged) by #36243 <s>There is a need for some listeners to listen for events on one firewall, but not another (e.g. `RememberMeListener`). This is now fixed by checking the `$providerKey`. We thought it might be nice to introduce a feature to the event dispatcher:</s> * <s>Create one event dispatcher per firewall;</s> * <s>Extend the `kernel.event_subscriber` tag, so that you can optionally specify the dispatcher service ID (to allow listening on events for a specific dispatcher);</s> * <s>Add a listener that always also triggers the events on the main event dispatcher, in case you want a listener that is listening on all firewalls.</s> * Drop the `AnonymousToken` and `AnonymousAuthenticator`: Anonymous authentication has never made much sense and complicates things (e.g. the user can be a string). For access control, an anonymous user has the same meaning as an un-authenticated one (`null`). This require changes in the `AccessListener` and `AuthorizationChecker` and probably also a new Security attribute (to replace `IS_AUTHENTICATED_ANONYMOUSLY`). Related issues: #34909, #30609 > **How to test** > 1. Install the Symfony demo application (or any Symfony application) > 2. Clone my Symfony fork (`git clone git@github.com:wouterj/symfony`) and checkout my branch (`git checkout security/deprecate-providers-listeners`) > 3. Use the link utility to link my fork to the Symfony application: `/path/to/symfony-fork/link /path/to/project` > 4. Enable the new system by setting `security.enable_authenticator_manager` to `true` Commits ------- b1e040f Rename providerKey to firewallName for more consistent naming 50224aa Introduce Passport & Badges to extend authenticators 9ea32c4 Also use authentication failure/success handlers in FormLoginAuthenticator 0fe5083 Added JSON login authenticator 7ef6a7a Use the firewall event dispatcher 95edc80 Added pre-authenticated authenticators (X.509 & REMOTE_USER) f5e11e5 Reverted changes to the Guard component ba3754a Differentiate between interactive and non-interactive authenticators 6b9d78d Added tests 59f49b2 Rename AuthenticatingListener 60d396f Added automatically CSRF protected authenticators bf1a452 Merge AuthenticatorManager and AuthenticatorHandler 44cc76f Use one AuthenticatorManager per firewall 09bed16 Only load old manager if new system is disabled ddf430f Added remember me functionality 1c810d5 Added support for lazy firewalls 7859977 Removed all mentions of 'guard' in the new system 999ec27 Refactor to an event based authentication approach b14a5e8 Moved new authenticator to the HTTP namespace b923e4c Enabled remember me for the GuardManagerListener 873b949 Mark new core authenticators as experimental 4c06236 Fixes after testing in Demo application fa4b3ec Implemented password migration for the new authenticators 5efa892 Create a new core AuthenticatorInterface 5013258 Add provider key in PreAuthenticationGuardToken 526f756 Added GuardManagerListener a172bac Added FormLogin and Anonymous authenticators 9b7fddd Integrated GuardAuthenticationManager in the SecurityBundle a6890db Created HttpBasicAuthenticator and some Guard traits c321f4d Created GuardAuthenticationManager to make Guard first-class Security

Aenos85 · 2020-06-10T17:02:10Z

Hello, I have two small problems with the new functionality.

i have used the success_handler to get a json response and not a redirect (because of an API call). How can I realize this with the event listener?
when i implement the LogoutEvent i get an error in the UnitTest that "Symfony\Component\HttpFoundation\Request" would not be a service (autowiring via extended LogoutEvent)

stof · 2020-06-10T17:13:29Z

1. i have used the success_handler to get a json response and not a redirect (because of an API call).  How can I realize this with the event listener?

The LogoutEvent accepts any Response, not only RedirectResponse. You could set your own response

2\. when i implement the LogoutEvent i get an error in the UnitTest that "Symfony\Component\HttpFoundation\Request" would not be a service (autowiring via extended LogoutEvent)

What do you mean exactly ?
Btw, this looks like you are registering LogoutEvent as a service that gets autowired.That does not make sense. The event object is a value object, not a service.

Aenos85 · 2020-06-10T17:46:55Z

I implemented all like it was written in "https://symfony.com/blog/new-in-symfony-5-1-simpler-logout-customization"

Reply 1.
I use the following code but the response is a 304 with a redirect

public function onSymfonyComponentSecurityHttpEventLogoutEvent()
{
    $this->setResponse(new JsonResponse(["success" => true]));
}

Reply 2.
In the UnitTest I get the following Message:

Symfony\Component\DependencyInjection\Exception\RuntimeException: Cannot autowire service "App\EventListener\LogoutListener": argument "$request" of method "Symfony\Component\Security\Http\Event\LogoutEvent::__construct()" references class "Symfony\Component\HttpFoundation\Request" but no such service exists.

I made a workaround for me

public function __construct(RequestStack $requestStack, ?TokenInterface $token)
{
    parent::__construct($requestStack->getCurrentRequest(), $token);
}

stof · 2020-06-11T09:42:41Z

without seeing your App\EventListener\LogoutListener, we cannot help you there.

Also, using $requestStack->getCurrentRequest() in the constructor of a service is the wrong way to use the RequestStack. It is totally possible that the constructor is called at the time where the stack is empty (and it will always be the case when using the CLI as there is no processed request there). And the current request will change over time.
$requestStack->getCurrentRequest() is meant to be called at the time you need to use the request. Introducing the RequestStack was done precisely to solve problems linked to defining the Request as a service in Symfony 2.x. Getting the request in the constructor reintroduces all these problems (and is even worse, as it does not provide any of the solutions which were implemented in Symfony 2.x for them).

Aenos85 · 2020-06-11T11:53:38Z

I agree with you, and in my case I do not need the request. But the class "Symfony\Component\Security\Http\Event\LogoutEvent" is the problem here the constructor is made and it gets the request service. The RequestStack is just a workaround for the listener in the CLI to work at all.

I have already shown my "App\EventListener\LogoutListener" completely, but here again.

And unfortunately the setResponse does not work either.

<?php


namespace App\EventListener;

use Symfony\Component\HttpFoundation\JsonResponse;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Http\Event\LogoutEvent;


class LogoutListener extends LogoutEvent
{
    public function __construct(RequestStack $requestStack, ?TokenInterface $token)
    {
        parent::__construct($requestStack->getCurrentRequest(), $token);
    }

    public function onSymfonyComponentSecurityHttpEventLogoutEvent()
    {
        ...
        $this->setResponse(new JsonResponse(["success" => true]));
    }
}

The LogoutEvent class have the Problem, withot the following code I would delete me constructor.

class LogoutEvent extends Event
{
    private $request;
    private $response;
    private $token;

    public function __construct(Request $request, ?TokenInterface $token)
    {
        $this->request = $request;
        $this->token = $token;
    }
...

stof · 2020-06-11T13:31:27Z

LogoutListener should not extend LogoutEvent. the listener and the event are 2 different concepts.

LogoutEvent should be the type of the argument of the method in your listener.

wouterj · 2020-06-11T14:06:52Z

Please read the documentation of the event dispatcher for more information: https://symfony.com/doc/current/event_dispatcher.html

Aenos85 · 2020-06-11T14:51:27Z

Okay, I got it. Sorry for the confusion, my bad.

ndantonio · 2020-08-03T07:46:36Z

Hi, I'm new with Symfony. I would like to ask for your help if you can give me an example of using this Logout Event on a listener or a subscriber? What I want to do is if my session expires then it will automatically logout and redirect to login page.

Thank you in advance!

alexander-schranz · 2020-08-04T14:16:45Z

src/Symfony/Component/Security/Http/Firewall/LogoutListener.php

-        // handle multiple logout attempts gracefully
-        if ($token = $this->tokenStorage->getToken()) {
-            foreach ($this->handlers as $handler) {
-                $handler->logout($request, $response, $token);


@wouterj

I'm currently working on the FosHttpCacheBundle which has a LogoutHandler but since Symfony 5.1 this seems not longer be called in the test:
https://travis-ci.org/github/FriendsOfSymfony/FOSHttpCacheBundle/jobs/714724517

For me it looks like it has todo with this change, but I'm not 100% sure. For the Bundle its not important as that service is deprecated and the thing handled by another listener but maybe it could be important for other project when the LogoutHandler is not called.

That's because it's registered in a weird way: (please note that there was no non-hacky way to add logout handlers)

$container->setAlias('security.logout.handler.session', 'fos_http_cache.user_context.session_logout_handler');

All internal handlers are now logout listeners, so "overriding" a core handler service ID no longer works. I'm not sure if internal core service IDs are covered by the BC promise (cc @nicolas-grekas please confirm).

TomasVotruba · 2021-01-28T18:48:22Z

@ndantonio Hi, you can use it like this: https://github.com/wouterj/symfony/blob/a9f096eb1f1fe687596cf62834d6c5efc003ec2a/src/Symfony/Component/Security/Http/EventListener/CsrfTokenClearingLogoutListener.php

…e-case (nicolas-grekas) This PR was merged into the 5.2 branch. Discussion ---------- [EventDispatcher] fix registering subscribers twice on edge-case | Q | A | ------------- | --- | Branch? | 5.2 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Introduced on 5.1 in #36243 Commits ------- ad60072 [EventDispatcher] fix registering subscribers twice on edge-case

This PR was merged into the 5.3 branch. Discussion ---------- [Security/Http] Fix cookie clearing on logout | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - I think this was forgotten or a merge issue when the component was refactored : - Original PR : #36252 - PR that added this file : #36243 (comment) Commits ------- d1aa32a [Security/Http] Fix cookie clearing on logout

carsonbot added Status: Needs Review Security Bug Feature Deprecation labels Mar 27, 2020

wouterj force-pushed the security/logout-listener branch from 54f0031 to 086533e Compare March 27, 2020 18:41

nicolas-grekas added this to the next milestone Mar 28, 2020

nicolas-grekas reviewed Mar 28, 2020

View reviewed changes

wouterj force-pushed the security/logout-listener branch 2 times, most recently from fdf8bd4 to 504bece Compare March 28, 2020 15:46

wouterj mentioned this pull request Mar 28, 2020

[Security/Http] Allow setting cookie security settings for delete_cookies #36252

Merged

linaori reviewed Mar 28, 2020

View reviewed changes

nicolas-grekas closed this in b1d21af Mar 30, 2020

nicolas-grekas reopened this Mar 30, 2020

fabpot approved these changes Apr 4, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 4, 2020

[Security] Refactor logout listener to dispatch an event instead

a9f096e

fabpot force-pushed the security/logout-listener branch from 504bece to a9f096e Compare April 4, 2020 11:02

fabpot mentioned this pull request Apr 4, 2020

[Security] Refactor logout listener to dispatch an event instead symfony/symfony-docs#13484

Closed

fabpot merged commit 1fc7b86 into symfony:master Apr 4, 2020

wouterj deleted the security/logout-listener branch April 4, 2020 16:07

wouterj mentioned this pull request Apr 4, 2020

[Security] AuthenticatorManager to make "authenticators" first-class security #33558

Merged

chalasr reviewed Apr 4, 2020

View reviewed changes

src/Symfony/Component/Security/Http/Logout/DefaultLogoutSuccessHandler.php Show resolved Hide resolved

src/Symfony/Component/Security/Http/Logout/LogoutSuccessHandlerInterface.php Show resolved Hide resolved

chalasr mentioned this pull request Apr 13, 2020

[Security] Logout success_handler is called **before** the actual logout happens #36227

Closed

nicolas-grekas modified the milestones: next, 5.1 May 4, 2020

fabpot mentioned this pull request May 5, 2020

Release v5.1.0-BETA1 #36713

Merged

wouterj mentioned this pull request May 13, 2020

RememberMeLogoutListener should depend on LogoutHandlerInterface #36806

Merged

alexander-schranz reviewed Aug 4, 2020

View reviewed changes

nicolas-grekas mentioned this pull request Feb 18, 2021

[EventDispatcher] fix registering subscribers twice on edge-case #40246

Merged

maxhelias mentioned this pull request Dec 21, 2021

[Security/Http] Fix cookie clearing on logout #44752

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Refactor logout listener to dispatch an event instead #36243

[Security] Refactor logout listener to dispatch an event instead #36243

wouterj commented Mar 27, 2020 •

edited

Loading

wouterj commented Mar 27, 2020 •

edited

Loading

nicolas-grekas left a comment

linaori left a comment

linaori Mar 28, 2020

trsteel88 commented Apr 2, 2020

fabpot Apr 4, 2020

fabpot commented Apr 4, 2020

Aenos85 commented Jun 10, 2020

stof commented Jun 10, 2020

Aenos85 commented Jun 10, 2020 •

edited

Loading

stof commented Jun 11, 2020

Aenos85 commented Jun 11, 2020

stof commented Jun 11, 2020

wouterj commented Jun 11, 2020

Aenos85 commented Jun 11, 2020

ndantonio commented Aug 3, 2020 •

edited

Loading

alexander-schranz Aug 4, 2020

wouterj Aug 5, 2020 •

edited

Loading

TomasVotruba commented Jan 28, 2021

[Security] Refactor logout listener to dispatch an event instead #36243

[Security] Refactor logout listener to dispatch an event instead #36243

Conversation

wouterj commented Mar 27, 2020 • edited Loading

wouterj commented Mar 27, 2020 • edited Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

linaori left a comment

Choose a reason for hiding this comment

linaori Mar 28, 2020

Choose a reason for hiding this comment

trsteel88 commented Apr 2, 2020

fabpot Apr 4, 2020

Choose a reason for hiding this comment

fabpot commented Apr 4, 2020

Aenos85 commented Jun 10, 2020

stof commented Jun 10, 2020

Aenos85 commented Jun 10, 2020 • edited Loading

stof commented Jun 11, 2020

Aenos85 commented Jun 11, 2020

stof commented Jun 11, 2020

wouterj commented Jun 11, 2020

Aenos85 commented Jun 11, 2020

ndantonio commented Aug 3, 2020 • edited Loading

alexander-schranz Aug 4, 2020

Choose a reason for hiding this comment

wouterj Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

TomasVotruba commented Jan 28, 2021

wouterj commented Mar 27, 2020 •

edited

Loading

wouterj commented Mar 27, 2020 •

edited

Loading

Aenos85 commented Jun 10, 2020 •

edited

Loading

ndantonio commented Aug 3, 2020 •

edited

Loading

wouterj Aug 5, 2020 •

edited

Loading