[Security] Configurable execution order for firewall listeners #37337

scheb · 2020-06-18T13:58:35Z

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

Hello there, I'm the author of scheb/two-factor-bundle, which extends Symfony's security layer with two-factor authentication. I've been closely following the recent changes by @wouterj to rework the security layer with "authenticators" (great work!). While I managed to make my bundle work with authenticators, I see some limitations in the security layer that I'd like to address to make such extensions easier to implement.

In #37336 I've submitted a draft to let security factories add their own authentication listeners to the firewall. This PR is intended to address the issue of execution order. If you look at the Firewall class

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

Lines 62 to 82 in f64f59a

    
           $authenticationListeners = function () use ($authenticationListeners, $logoutListener) { 
        
               $accessListener = null; 
        
               foreach ($authenticationListeners as $listener) { 
        
                   if ($listener instanceof AccessListener) { 
        
                       $accessListener = $listener; 
        
                       continue; 
        
                   } 
        
                   yield $listener; 
        
               } 
        
               if (null !== $logoutListener) { 
        
                   yield $logoutListener; 
        
               } 
        
               if (null !== $accessListener) { 
        
                   yield $accessListener; 
        
               } 
        
           };

authentication listeners are executed in the order of their creation. Additionally, there's hardcoded logic to execute Symfony\Component\Security\Http\Firewall\AccessListener always last and the logout listener second to last. I'd like to have a more flexible approach, to remove the hardcoded order and give authentication listeners the ability to determine their execution order.

I've added an optional interface to provide a priority to sort all registered authenitication listeners. Sorting is done in a compiler pass, so no time is wasted at runtime.

This is a draft, so I'd like to hear your opinion on this :)

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

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/SortFirewallListenersPass.php

scheb · 2020-06-19T14:34:36Z

I've added the logout listener to the array of authentication listeners and removed the sorting logic from Firewall.

I have a bit of an issue with CompleteConfigurationTest. My compiler pass requires ResolveChildDefinitionsPass to be executed, but the test has disabled all optimization steps. The tests are failing because some ChildDefintions are missing the class attribute, which would normally be set by ResolveChildDefinitionsPass. If I add ResolveChildDefinitionsPass as a optimization pass, other things are starting to break, because of missing child definitions.

I've tried to resolve the class recursively within my compiler pass (basically doing what ResolveChildDefinitionsPass would normally do for me) and then the tests go green. So it's an issue with test setup somehow. I don't really understand how to fix this.

wouterj · 2020-06-19T14:56:37Z

When I make this change:

diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php
index 95e3e6e345..226eb1a440 100644
--- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php
+++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php
@@ -15,6 +15,7 @@ use PHPUnit\Framework\TestCase;
 use Symfony\Bundle\SecurityBundle\DependencyInjection\SecurityExtension;
 use Symfony\Bundle\SecurityBundle\SecurityBundle;
 use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
+use Symfony\Component\DependencyInjection\Compiler\ResolveChildDefinitionsPass;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\Security\Core\Authorization\AccessDecisionManager;
@@ -69,10 +70,9 @@ abstract class CompleteConfigurationTest extends TestCase
         $configs = [];
         foreach (array_keys($arguments[1]->getValues()) as $contextId) {
             $contextDef = $container->getDefinition($contextId);
-            $arguments = $contextDef->getArguments();
-            $listeners[] = array_map('strval', $arguments['index_0']->getValues());
+            $listeners[] = array_map('strval', $contextDef->getArgument(0)->getValues());

-            $configDef = $container->getDefinition((string) $arguments['index_3']);
+            $configDef = $container->getDefinition((string) $contextDef->getArgument(3));
             $configs[] = array_values($configDef->getArguments());
         }

@@ -637,6 +637,7 @@ abstract class CompleteConfigurationTest extends TestCase

         $container = new ContainerBuilder();
         $container->setParameter('kernel.debug', false);
+        $container->register('cache.system', \stdClass::class);

         $security = new SecurityExtension();
         $container->registerExtension($security);
@@ -645,7 +646,7 @@ abstract class CompleteConfigurationTest extends TestCase
         $bundle->build($container); // Attach all default factories
         $this->getLoader($container)->load($file);

-        $container->getCompilerPassConfig()->setOptimizationPasses([]);
+        $container->getCompilerPassConfig()->setOptimizationPasses([new ResolveChildDefinitionsPass()]);
         $container->getCompilerPassConfig()->setRemovingPasses([]);
         $container->getCompilerPassConfig()->setAfterRemovingPasses([]);
         $container->compile();

There is only one failing test in the CompleteConfigurationTest:

1) Symfony\Bundle\SecurityBundle\Tests\DependencyInjection\XmlCompleteConfigurationTest::testFirewalls
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
         1 => 'security.user_checker'
         2 => '.security.request_matcher.xmi9dcw'
         3 => false
+        4 => false
+        5 => ''
+        6 => ''
+        7 => ''
+        8 => ''
+        9 => ''
+        10 => Array ()
+        11 => null
     )
     1 => Array (...)
     2 => Array (...)
     3 => Array (...)
 )

/home/wouter/symfony/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php:86

Seems like there are many new empty security listeners.

scheb · 2020-06-19T15:04:08Z

@wouterj Nice, I'll try that and have a look at this failing test.

src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php

wouterj · 2020-06-19T16:25:13Z

Can you maybe checkout the Travis failures?

The first job can be fixed by executing php src/Symfony/Bundle/FrameworkBundle/Resources/bin/check-unused-tags-whitelist.php (it'll update the unused tags list)
deps=high seems to indicate there are some issues with the logout listener behaving differently now
deps=low indicates that we have to update the version constraint in SecurityBundle. You can set it to ^5.2 - which will automagically be transformed into the branch of this PR.

src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/SecurityExtensionTest.php

scheb · 2020-06-19T18:12:41Z

So I tried to reproduce these failing tests now for an hour or so. I don't get it. I'm unable to reproduce these failures.

And then there's this build on 1bc5c7b: https://travis-ci.org/github/symfony/symfony/builds/700163046

PHP7.2 + php_extra=7.4
PHP7.4 + deps=low

These fail in PhpCompleteConfigurationTest because I have removed the code that adds logout listener to the authentication listeners array, but didn't update the test. So I expected these tests to fail.

But interestingly PHP: 7.3 + deps=high builds fine without any errors. How's that possible? I've deliberately introduced a failing test, but it doesn't fail 🤔

It feels as if the dependencies of this PR (specifically symfony/security-http) are not using the latest code from this PR but rather some outdated version of this PR's codebase.

wouterj · 2020-06-19T22:30:53Z

Oh, looking at the composer logs I think deps=high always installs the master branch and not the PR branch. So that should be green once this PR is merged. I'm not totally sure though.

…ervice if invalid (chalasr) This PR was merged into the 4.4 branch. Discussion ---------- [SecurityBundle] Drop cache.security_expression_language service if invalid | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Definition should be removed when its parent (`cache.system`) does not exist. Spotted in #37337 Commits ------- bc96693 [SecurityBundle] Drop cache.security_expression_language definition if invalid

@wouterj

…rs (scheb) This PR was merged into the 5.2-dev branch. Discussion ---------- [Security] Let security factories add firewall listeners | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | Deprecations? | no | License | MIT | Doc PR | n/a Hello there, I'm the author of `scheb/two-factor-bundle`, which extends Symfony's security layer with two-factor authentication. I've been closely following the recent changes by @wouterj to rework the security layer with "authenticators" (great work!). While I managed to make my bundle work with authenticators, I see some limitations in the security layer that I'd like to address to make such extensions easier to implement. With the new authenticator-based security system, it is no longer possible to add a authentication listener to the firewall. The only way to do it is a dirty compiler pass, which extends the argument on the `security.firewall.map.context.[firewallName]` service (like I do in: https://github.com/scheb/2fa/blob/ed2ce9804b6a78fe539894e77038ab96a52f5c56/src/bundle/DependencyInjection/Compiler/AccessListenerCompilerPass.php). This is quite ugly and hacky, so I believe there should be an easier and clean way to add firewall-level listeners. This PR adds an interface, which may be implemented by security factories and lets them add additional listeners to the firewall. Why would you want to do that? There are certain use-cases that require extra logic to handle a request within the firewall. For example in my bundle, I need to handle the intermediate state between login and the completion of two-factor authentication. So ideally, I'm able to execute some code from the firewall right before `Symfony\Component\Security\Http\Firewall\AccessListener`. In the old security system, I could handle this in my authentication listener, which I had to implement anyways. With the new authenticator-based system this option is gone. In the ideal world, I could add a firewall listener and tell it to execute between `LogoutListener` and `AccessListener`. This is a draft, so I'd like to hear your opinion on this :) There's another issue, regarding the order of execution, which I'm addressing with #37337. Commits ------- 0a4fcea Add interface to let security factories add their own firewall listeners

scheb · 2020-06-23T21:11:50Z

src/Symfony/Component/Security/Http/composer.json

@@ -31,7 +31,8 @@
    },
    "conflict": {
        "symfony/event-dispatcher": "<4.3",
-        "symfony/security-csrf": "<4.4"
+        "symfony/security-csrf": "<4.4",
+        "symfony/security-bundle": "<5.2"


Have added this because both symfony/security and symfony/security-bundle need to be installed in the "=>5.2" version to work properly together.

security-http should not be aware of security-bundle, only security-bundle knows about security components and has version constraints for them

Hmm, I'm wondering how would you ensure compatibility then? Combining a 5.1 symfony/security-bundle with 5.2 symfony/security would lead to an odd behavior.

wouterj · 2020-06-23T21:09:48Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

@@ -285,6 +284,7 @@ private function createFirewalls(array $config, ContainerBuilder $container)
                ->replaceArgument(1, $exceptionListener)
                ->replaceArgument(2, $logoutListener)
                ->replaceArgument(3, new Reference($configId))
+                ->addTag('security.firewall_map_context')


Since #37368 has been merged, there now is a security.firewalls parameter containing all firewall names. I propose to use that in the compiler pass instead of introducing a tag (see the referenced PR for a usage example).

Ryan tried to not introduce SecurityBundle "internal" tags, as this might confuse users. E.g. PHPstorm will auto complete this, so we should try to not clutter the autocompletion list with tags the user should never use. (in the end, we should maybe introduce the concept of internal tags?)

Rebased and changed to use security.firewalls now.

src/Symfony/Bundle/SecurityBundle/SecurityBundle.php

wouterj · 2020-08-13T10:16:16Z

Hi @scheb! Is there anything missing from this PR, or can it be marked as ready-to-merge? I think it's a great feature to add to Symfony and feature freeze is only in a few weeks (so it's good to have a clear overview of what PRs can be ready before this deadline).

scheb · 2020-08-13T10:28:45Z

@wouterj Technically it's ready and I'd like to see it in 5.2. It's still a draft, because I need feedback on this issue:

Hmm, I'm wondering how would you ensure compatibility then? Combining a 5.1 symfony/security-bundle with 5.2 symfony/security would lead to an odd behavior.

It breaks the logout feature when you have such a combination in your application. And since @chalasr said

security-http should not be aware of security-bundle, only security-bundle knows about security components and has version constraints for them

I'm not sure how we could solve this.

I tend to say the change to add the LogoutListener to the list of authentication listeners shouldn't be part of this PR. I added this part as an optimization, but it's not required to make the PR work. That specific change looks like a BC break to me, since it breaks compatibility among 5.x versions of symfony/security-bundle and symfony/security. So we're probably better off considering this change (to add the LogoutListener to the list of authentication) for Symfony 6.0.

What do you think?

fabpot · 2020-08-13T11:03:36Z

What about removing the LogoutListener for now, merge this PR. Then, we create a PR for the LogoutListener and we can discuss the best option for BC. How does that sound?

scheb · 2020-09-02T10:22:23Z

Hello there! I've removed the change regarding the LogoutListener, as Fabien suggested. The failed builds have been unrelated to the changes. So anything stopping up from merging the PR?

fabpot · 2020-09-02T11:06:41Z

@scheb Thanks for the ping. Unfortunately, without a comment, there is no way I can see that something had changed here, except checking from time to time.

fabpot · 2020-09-02T11:07:01Z

Thank you @scheb.

scheb · 2020-09-02T11:40:46Z

@fabpot Thank you for merging! :)

This PR was merged into the 5.2-dev branch. Discussion ---------- [Security] Add some missing CHANGELOG entries | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Mentions #37337 in CHANGELOG files. Commits ------- 46ce480 [Security] Add some missing CHANGELOG entries

carsonbot added Status: Needs Review Security Feature labels Jun 18, 2020

scheb mentioned this pull request Jun 18, 2020

[Security] Let security factories add firewall listeners #37336

Merged

wouterj reviewed Jun 18, 2020

View reviewed changes

src/Symfony/Component/Security/Http/Firewall.php Show resolved Hide resolved

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/SortFirewallListenersPass.php Show resolved Hide resolved

nicolas-grekas added this to the next milestone Jun 18, 2020

scheb force-pushed the firewall-listener-priority branch from 7e359df to 1972786 Compare June 19, 2020 14:26

scheb commented Jun 19, 2020

View reviewed changes

src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php Outdated Show resolved Hide resolved

chalasr mentioned this pull request Jun 19, 2020

[SecurityBundle] Drop cache.security_expression_language service if invalid #37362

Merged

chalasr reviewed Jun 19, 2020

View reviewed changes

src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/SecurityExtensionTest.php Outdated Show resolved Hide resolved

scheb force-pushed the firewall-listener-priority branch 4 times, most recently from 6c9a2bb to 7f05dc3 Compare June 23, 2020 20:52

scheb commented Jun 23, 2020

View reviewed changes

wouterj reviewed Jun 23, 2020

View reviewed changes

scheb force-pushed the firewall-listener-priority branch from 458d104 to 88d92a2 Compare June 24, 2020 17:03

Add ability to prioritize firewall listeners

91388e8

scheb force-pushed the firewall-listener-priority branch from 88d92a2 to 91388e8 Compare August 13, 2020 14:52

scheb marked this pull request as ready for review August 13, 2020 15:32

fabpot approved these changes Sep 2, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 2, 2020

fabpot mentioned this pull request Sep 2, 2020

[Security] Configurable execution order for firewall listeners symfony/symfony-docs#14159

Closed

fabpot merged commit afdb97e into symfony:master Sep 2, 2020

chalasr mentioned this pull request Sep 2, 2020

[Security] Add some missing CHANGELOG entries #38038

Merged

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

fabpot mentioned this pull request Oct 5, 2020

Release v5.2.0-BETA1 #38417

Merged

scheb mentioned this pull request Oct 31, 2020

Integrate bundle with new Symfony 5.2 features scheb/2fa#29

Closed

scheb deleted the firewall-listener-priority branch December 28, 2020 11:40

This was referenced Oct 17, 2021

[Security] Remove sorting of security listeners at runtime from Firewall #43548

Closed

[Security] Deprecate LogoutListener being returned as 3rd element by FirewallMapInterface::getListeners #43674

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Configurable execution order for firewall listeners #37337

[Security] Configurable execution order for firewall listeners #37337

scheb commented Jun 18, 2020

scheb commented Jun 19, 2020

wouterj commented Jun 19, 2020

scheb commented Jun 19, 2020

wouterj commented Jun 19, 2020

scheb commented Jun 19, 2020 •

edited

Loading

wouterj commented Jun 19, 2020

scheb Jun 23, 2020

chalasr Jun 23, 2020

scheb Jun 24, 2020

wouterj Jun 23, 2020

scheb Jun 24, 2020

wouterj commented Aug 13, 2020

scheb commented Aug 13, 2020

fabpot commented Aug 13, 2020

scheb commented Sep 2, 2020

fabpot commented Sep 2, 2020

fabpot commented Sep 2, 2020

scheb commented Sep 2, 2020

	$authenticationListeners = function () use ($authenticationListeners, $logoutListener) {
	$accessListener = null;

	foreach ($authenticationListeners as $listener) {
	if ($listener instanceof AccessListener) {
	$accessListener = $listener;

	continue;
	}

	yield $listener;
	}

	if (null !== $logoutListener) {
	yield $logoutListener;
	}

	if (null !== $accessListener) {
	yield $accessListener;
	}
	};

[Security] Configurable execution order for firewall listeners #37337

[Security] Configurable execution order for firewall listeners #37337

Conversation

scheb commented Jun 18, 2020

scheb commented Jun 19, 2020

wouterj commented Jun 19, 2020

scheb commented Jun 19, 2020

wouterj commented Jun 19, 2020

scheb commented Jun 19, 2020 • edited Loading

wouterj commented Jun 19, 2020

scheb Jun 23, 2020

Choose a reason for hiding this comment

chalasr Jun 23, 2020

Choose a reason for hiding this comment

scheb Jun 24, 2020

Choose a reason for hiding this comment

wouterj Jun 23, 2020

Choose a reason for hiding this comment

scheb Jun 24, 2020

Choose a reason for hiding this comment

wouterj commented Aug 13, 2020

scheb commented Aug 13, 2020

fabpot commented Aug 13, 2020

scheb commented Sep 2, 2020

fabpot commented Sep 2, 2020

fabpot commented Sep 2, 2020

scheb commented Sep 2, 2020

scheb commented Jun 19, 2020 •

edited

Loading