What about doing it inLogoutUrlGenerator directly?

@chalasr : I'd like to. But theFirewallConfig is available in theSecurityBundle only (whereas as theLogoutUrlGenerator is in theSecurity component). :/

(we can provide an implementation in theSecurityBundle, but is worth it ?)

Note that providing an implementation in theSecurityBundle will automatically allow theSymfony\Bridge\Twig\Extension\LogoutUrlExtension andSymfony\Bundle\SecurityBundle\Templating\Helper\LogoutUrlHelper to benefit from this. WDYT?

Let's get feedbacks about an implementation in theSecurityBundle thanks toafdc33b.

Imho it is worth it 👍

Shouldnt we technically still checkToken::getProviderKey() first, if$key is null?

What aboutsetDefaultProviderKey on the component side? And set it using theFirewallConfig?

What about setDefaultProviderKey on the component side? And set it using the FirewallConfig?

That would made theSecurity component'sLogourUrlGenerator stateful (you'll have to set it withsetDefaultProviderKey for every request, and it'll not be a default at all, just the value retrieved from the firewall config for current request).

Shouldnt we technically still check Token::getProviderKey() first, if $key is null?

Why? 😕
Token::getProviderKey() does return the firewall name. Using the firewall config is simply the safest way to get it but cannot replaceToken::getProviderKey() in the component because it's only available in the bundle.

Shouldnt we technically still check Token::getProviderKey() first, if $key is null?

Since the parent methods are called, the token storagewill be used if$key is null, right?

About thesetProviderKey, Imho it's good to have the bundle as an extension of the component, rather than changing the component.

Yeah.. but in case of null we passextractKeyFromFirewallConfig(). Ie.FirewallConfig::getName is preferred overToken::getProviderKey(). Im not sure that's correct.

Opposed to

try {returnparent::getLogoutPath($key);}catch (\InvalidArgumentException$e) {if (null ===$key &&null !==$defaultKey =$this->extractKeyFromFirewallConfig()) {returnparent::getLogoutPath($defaultKey);   }throw$e;}

FirewallConfig::getName is preferred over Token::getProviderKey(). Im not sure that's correct.

Why?FirewallConfig::getName is strictly equivalent toToken::getProviderKey(). Unless you get a token without the current firewall name as provider key (which mean it has failed to implementgetProviderKey or there is an issue in the application/security listener). Or am I missing something?

Again not sure :) just imagining one could actually be authenticated against a different firewall (ie. identified by its current token) other then the current firewall (ie. identified by request?)

Correction:FirewallConfig::getName andToken::getProviderKey() are not strictly equivalent in case two firewalls share the same context. The firewall which has created the token will be set in the token provider key.

Then I'm not confident about this PR anymore.
With the current code, the logout listener might have been defined on only one firewall of many sharing the same context. Thus, the current firewall may not allow to generate the logout url.

Even by changing the code by:

try {returnparent::getLogoutPath($key);}catch (\InvalidArgumentException$e) {if (null ===$key &&null !==$defaultKey =$this->extractKeyFromFirewallConfig()) {returnparent::getLogoutPath($defaultKey);   }throw$e;}

as you've suggested@ro0NL. The issue is that it's probably wrong to suggest to logout using the wrong logout url, even in last resort. 😕 (and you may still not find the logout listener)

You should return a key only ifin_array('logout', $firewallConfig->getListeners(), true) :)

You should return a key only if in_array('logout', $firewallConfig->getListeners(), true) :)

The originalLogoutUrlGenerator will already throw an exception if the logout listener does not exists for this firewall.

That's not what I meant by:

you may still not find the logout listener

I meant you may still be behind a firewall on which is not defined thelogout listener, so anyyway you won't get the link in the profiler.

Anyway, I think I'm closing this one, because it brings more issues than it solves. It's not even an issue I encountered, but I thought it could be a good usage of the newFirewallConfig (apparently it was not 😄 ). Let's implementsgetProviderKey in your security tokens guys. :)

Thank you for your time. :)

@ogizanagi im really considering this a DX improvement.. and we were almost there, right?

What's wrong with the exception in case you're passing the key from firewall config and it has no listeners? In user-land if someone calls this, and there is absolutely no available url it already crashes anyway.

The only difference would be (in case of null) we get

No LogoutListener found for firewall key "<key-from-firewall-config>".

Instead of

Unable to find the current firewall LogoutListener, please provide the provider key manually.

If we add aprotected getDefaultProviderKey() to the base logout url generator we can avoid the try/catch approach.

I'm really considering this a DX improvement.. and we were almost there, right?
What's wrong with the exception in case you're passing the key from firewall config and it has no listeners?

Nothing wrong with the exception. What would appear to be "wrong" to me is to generate the logout url with the wrong firewall and logout listener (not the one which generated the token). I mean...it's not fundamentally wrong: AFAIK, given two firewalls sharing a same context, with only one defining the logout listener, the logout url is actually available for both firewalls and will invalidate the token, doesn't matter which firewall created it.

But is this PR really worth it, considering it won't give you 100% chances to get the logout url (as the logout listener might be on another firewall than the current one)? (you don't have 100% chances currently neither of course, but the implementation is sufficient to me)

I'm reopening this in order to get more feedbacks about it, but I don't expect much actually 😕

If we add a protected getDefaultProviderKey() to the base logout url generator we can avoid the try/catch approach.

Still not convinced about this because it'll make theLogoutUrlGenerator state tied to the request (theTranslator for instance is also tied to the request, though. So a request listener setting the current firewall key in the originalLogoutUrlGenerator could be considered actually).

is this PR really worth it, considering it won't give you 100% chances to get the logout url

Sure, but still more chances than if kept as is so, for what it costs, I would still say yes.

Since the token's provider key would be the better alternative if provided (right?), shouldn't it be indeed checked at first?

Since the token's provider key would be the better alternative if provided (right?), shouldn't it be indeed checked at first?

It won't give more chances, but is still more accurate yes. I agree with that and@ro0NL's suggestion about checking it first, and then fallback to the current firewall key if no logout listener is found.

Status: needs work

:)

I've updated the PR by adding a new commit as a POC for using firewall contexts and a listener setting the current firewall name + context per request (no moreFirewallConfig usage, so no moreLogoutUrlGenerator implementation in the bundle).

When not providing the firewall key:

1. Try to find the key from the token (unless it's an anonymous token)
2. If found, try to get the listener from the key. If the listener is found, stop there.
3. Try from the injected firewall key. If the listener is found, stop there.
4. Try from the injected firewall context. If the listener is found, stop there.

The behavior remains unchanged when providing explicitly the firewall key. No fallback.

diff --git a/app/Resources/views/base.html.twig b/app/Resources/views/base.html.twigindex c20f749..2e0c5a8 100644--- a/app/Resources/views/base.html.twig+++ b/app/Resources/views/base.html.twig@@ -61,7 +61,7 @@                                 {% if app.user %}                                     <li>-                                        <a href="{{ path('security_logout') }}">+                                        <a href="{{ logout_url() }}">                                             <i aria-hidden="true"></i> {{ 'menu.logout'|trans }}                                         </a>                                     </li>diff --git a/app/config/security.yml b/app/config/security.ymlindex cc31cfb..cfda7ee 100644--- a/app/config/security.yml+++ b/app/config/security.yml@@ -13,33 +13,27 @@ security:     # http://symfony.com/doc/current/book/security.html#firewalls-authentication     firewalls:-        secured_area:-            # this firewall applies to all URLs-            pattern: ^/--            # but the firewall does not require login on every page-            # denying access is done in access_control or in your controllers+        admin:+            pattern: ^/(%app_locales%)/admin             anonymous: true--            # This allows the user to login by submitting a username and password-            # Reference: http://symfony.com/doc/current/cookbook/security/form_login_setup.html             form_login:-                # The route name that the login form submits to                 check_path: security_login-                # The name of the route where the login form lives-                # When the user tries to access a protected page, they are redirected here                 login_path: security_login-                # Secure the login form against CSRF-                # Reference: http://symfony.com/doc/current/cookbook/security/csrf_in_login_form.html                 csrf_token_generator: security.csrf.token_manager-             logout:-                # The route name the user can go to in order to logout+                path: security_logout_admin+                target: security_login+            context: foo+        secured_area:+            pattern: ^/+            anonymous: true+            logout:                 path: security_logout-                # The name of the route to redirect to after logging out                 target: homepage+            context: foo     access_control:         # this is a catch-all for the admin area         # additional security lives in the controllers+        - { path: '^/(%app_locales%)/admin/login', roles: IS_AUTHENTICATED_ANONYMOUSLY }         - { path: '^/(%app_locales%)/admin', roles: ROLE_ADMIN }diff --git a/src/AppBundle/Controller/SecurityController.php b/src/AppBundle/Controller/SecurityController.phpindex fa99392..3b79931 100644--- a/src/AppBundle/Controller/SecurityController.php+++ b/src/AppBundle/Controller/SecurityController.php@@ -24,7 +24,7 @@ use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route; class SecurityController extends Controller {     /**-     * @Route("/login", name="security_login")+     * @Route("/admin/login", name="security_login")      */     public function loginAction()     {@@ -45,6 +45,7 @@ class SecurityController extends Controller      * and handle the logout automatically. See logout in app/config/security.yml      *      * @Route("/logout", name="security_logout")+     * @Route("/admin_logout", name="security_logout_admin")      */     public function logoutAction()     {

Status: Needs Review

If you still think it makes sense, let me know what could possibly be improved and what should be reconsidered regarding our previous suggested solutions.

@ro0NL : I thinkb29b004?w=1 should cover your expectations for your last review (However it's hard to follow small comments for such changes while ensuring it does not change any behavior (tests will of course be required if we want to merge this). Could you provide a full diff next time instead?).

Anyway, thank you for your review. ;)

Ship it.

@ogizanagi What's the status of this PR?

@fabpot: Squashed, rebased on master and tested back on the symfony demo using the patch mentioned in#20516 (comment). Tests are green on my side.Waiting for Travis. Failures unrelated.

No more changes planned on my side. It actually enhances the situation for cases mentioned in the PR description and should always find and use the proper logout listener across contexts (as soon as there is one of course).

So I think it's ready for the final review and validating the strategy used. :)