[Security] Move AbstractListener abstract methods to the new FirewallListenerInterface #38751

chalasr · 2020-10-25T14:08:43Z

Q	A
Branch?	5.x
Bug fix?	no
New feature?	-
Deprecations?	no
Tickets	-
License	MIT
Doc PR	-

We added a FirewallListenerInterface in 5.2, let's make it complete to allow for cleaner firewall listener implementations.

…ListenerInterface

wouterj · 2020-10-26T11:52:32Z

Hmm, I'm not very sure about these changes. But the listener logic is quite complex, so I might completely miss something. What I see:

The only requirement for firewall listeners is to be callable:

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

Lines 110 to 119 in 93d0276

    
           protected function callListeners(RequestEvent $event, iterable $listeners) 
        
           { 
        
               foreach ($listeners as $listener) { 
        
                   $listener($event); 
        
                   if ($event->hasResponse()) { 
        
                       break; 
        
                   } 
        
               } 
        
           }

Only the AbstractListener implements this callable functionality using supports()/authenticate():

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

Lines 24 to 29 in 93d0276

    
           final public function __invoke(RequestEvent $event) 
        
8000
{ 
        
               if (false !== $this->supports($event->getRequest())) { 
        
                   $this->authenticate($event); 
        
               } 
        
           }

So, it should be possible to have a class with just an __invoke() and getPriority() method, right? AbstractListener is only relevant when you want your listener to support laziness.

chalasr · 2020-10-26T14:13:56Z

AbstractListener is only relevant when you want your listener to support laziness.

Actually supports() is also relevant for listeners that don't need laziness (yet), as they can opt-in using the special null return type for this method.

At the time lazy firewalls were introduced, @nicolas-grekas and I talked about deprecating not extending AbstractListener at some point, so that we have a consistent API for firewall listeners (see https://github.com/orgs/symfony/projects/1#card-30499343).
I'm still a bit annoyed by the fact we need to rely on inheritance instead of a clear contract if we do so.

Meanwhile we are reintroducing an interface which, right now, only covers the priority feature.
This PR takes it as an opportunity to not force extending a base class, making the concept more structured and more flexible. That's worth the hassle to me.

About adding __invoke() to the contract, I'm mitigated: should we just accept callable|FirewallListenerinterface for firewall listeners?
The interface allows you to opt-in into more complex features like priority, laziness and the ability to separate concerns between the guard clause and the actual authentication logic.

Does this make sense?

nicolas-grekas

big 👍 on my side
this change enables lazy firewalls (but is not ad hoc for them) because it enables a better design, with a better split of the steps of a firewall

wouterj

Ah sorry, I originally was looking at this PR as "purely simplifying" instead of moving to more consistency.

If I understand correctly, the contract already is callable|FirewallListenerInterface (it needs to be, for BC reasons IIRC). In that case 👍 and we might indeed want to deprecate callable in Symfony 5.3 (I don't think it's possible to introduce new deprecations after feature freeze?)

nicolas-grekas · 2020-10-27T10:43:34Z

Thank you @chalasr.

chalasr added the Security label Oct 25, 2020

chalasr requested a review from wouterj as a code owner October 25, 2020 14:08

carsonbot added Status: Needs Review Feature labels Oct 25, 2020

chalasr force-pushed the complete-firewall-listener branch from 4b108ee to 170a564 Compare October 25, 2020 14:09

chalasr added this to the 5.2 milestone Oct 25, 2020

chalasr force-pushed the complete-firewall-listener branch from 170a564 to 7de8b4e Compare October 25, 2020 21:26

[Security] Move AbstractListener abstract methods to the new Firewall…

5dd70bd

…ListenerInterface

chalasr force-pushed the complete-firewall-listener branch from 7de8b4e to 5dd70bd Compare October 25, 2020 21:42

chalasr removed the Feature label Oct 25, 2020

nicolas-grekas approved these changes Oct 27, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 27, 2020

wouterj approved these changes Oct 27, 2020

View reviewed changes

nicolas-grekas merged commit 824bc44 into symfony:5.x Oct 27, 2020

chalasr deleted the complete-firewall-listener branch October 27, 2020 10:47

fabpot mentioned this pull request Oct 28, 2020

Release v5.2.0-BETA3 #38854

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Move AbstractListener abstract methods to the new FirewallListenerInterface #38751

[Security] Move AbstractListener abstract methods to the new FirewallListenerInterface #38751

[Security] Move AbstractListener abstract methods to the new FirewallListenerInterface #38751

[Security] Move AbstractListener abstract methods to the new FirewallListenerInterface #38751

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment