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

scheb · 2021-10-23T09:41:54Z

Q	A
Branch?	5.4
Bug fix?	no
New feature?	no
Deprecations?	yes
License	MIT

As discussed with @chalasr, I'm adding these deprecations in preparation for #43548, which is targeting Symfony 6.0.

What's this PR about?

The 3rd element in the array returned by FirewallMapInterface::getListeners() containing the LogoutListener will be deprecated. Instead, in Symfony 6.0 the logout listener will be included in the first element, which is then containing all firewall listeners and removing some unnecessary complexity (-> #43548).

This change is a follow-up of introducing the ability to sort security listeners, which was introduced in #37337. There is no longer the need for treating the LogoutListener as a special case to get its execution order right.

chalasr

Thanks for the PR, Christian! I have some concerns with the current BC layer

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

nicolas-grekas

Here are some comments, but the 2nd one might not be relevant after @stof's comments (which I agree with: the current deprecation layer should be improved)

src/Symfony/Component/Security/Http/CHANGELOG.md

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

fabpot · 2021-10-30T10:20:16Z

@scheb Do you think you can work on this one soon? We are closing 5.4 for new features.

scheb · 2021-10-30T10:34:46Z

I got time this weekend!

My suggestion would be to combine adding the new methods (as discussed here) with the changes from #43548 and target 5.4. With new methods added and the existing method becoming deprecated, it should be possible to do it all at once within the 5.4 release.

scheb · 2021-10-30T17:29:53Z

Force-pushed a draft to deprecate getListeners method and add getFirewallListeners and getExceptionListener as a replacement. Didn't touch the tests yet, would like to get some feedback first if this would be the way to go.

Considerations:

Wasn't sure of constructor arguments need to be kept backwards compatible. Since FirewallContext and LazyFirewallContext are somewhat internal to security-bundle, I went with changing them.
Came to the conclusion that deprecation and [Security] Remove sorting of security listeners at runtime from Firewall #43548 can't be done both in 5.4, because there's no guarantee the new methods are actually implemented by the user.

chalasr · 2021-11-12T23:51:56Z

would like to get some feedback first if this would be the way to go.

To me it is, yes 👍 .

Wasn't sure of constructor arguments need to be kept backwards compatible. Since FirewallContext and LazyFirewallContext are somewhat internal to security-bundle, I went with changing them.

We cannot remove them as these classes are not marked as internal. We need to deprecate these constructor parameters first (happy to help with the BC layer.)

Came to the conclusion that deprecation and [Security] Remove sorting of security listeners at runtime from Firewall #43548 can't be done both in 5.4, because there's no guarantee the new methods are actually implemented by the user.

Indeed.

scheb · 2021-11-13T11:37:31Z

We cannot remove them as these classes are not marked as internal. We need to deprecate these constructor parameters first (happy to help with the BC layer.)

@chalasr Ok, that was what I was afraid of 😟. For derprecating arguments at the end of the argument list it's easy, make it optional and trigger deprecation warning when it's set. Though for arguments in between I'm not really sure how to proceed. Can you point me to an example how to approach this? We could also have a chat on Slack, since that works better for back and forth comms.

wouterj · 2021-11-13T11:52:26Z

Though for arguments in between I'm not really sure how to proceed. Can you point me to an example how to approach this?

See e.g:

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

Lines 42 to 59 in 0d15daf

    
           public function __construct(TokenStorageInterface $tokenStorage, AccessDecisionManagerInterface $accessDecisionManager, AccessMapInterface $map, /*bool*/ $exceptionOnNoToken = true) 
        
           { 
        
               if ($exceptionOnNoToken instanceof AuthenticationManagerInterface) { 
        
                   trigger_deprecation('symfony/security-http', '5.4', 'The $authManager argument of "%s" is deprecated.', __METHOD__); 
        
                   $authManager = $exceptionOnNoToken; 
        
                   $exceptionOnNoToken = \func_num_args() > 4 ? func_get_arg(4) : true; 
        
               } 
        
               if (false !== $exceptionOnNoToken) { 
        
                   trigger_deprecation('symfony/security-http', '5.4', 'Not setting the $exceptionOnNoToken argument of "%s" to "false" is deprecated.', __METHOD__); 
        
               } 
        
               $this->tokenStorage = $tokenStorage; 
        
               $this->accessDecisionManager = $accessDecisionManager; 
        
               $this->map = $map; 
        
               $this->authManager = $authManager ?? (class_exists(AuthenticationManagerInterface::class) ? new NoopAuthenticationManager() : null); 
        
               $this->exceptionOnNoToken = $exceptionOnNoToken; 
        
           }

Basically:

Update the argument list to match the new signature
Remove all type hints from the removed argument to the end of the argument list
Check one instanceof to see if the old or new signature is used, and activate the BC layer when necessary

…Listeners

scheb · 2021-11-15T20:33:01Z

So here's a suggestion. The fabbot.io failing seems to be unrelated to my changes.

Considerations:

The fact that $listeners is an iterable makes things messy when trying to ensure the LogoutListener is contained, since it can be either an array or a (lazy-loaded) iterator. Tried to not mess up the lazy-loading iterators.
LazyFirewallContext extending the constructor makes things complicated. Decided to just get the argument that matters for that class (the $tokenStorage) from the constructor arguments and push other responsibilities down to be handled in the FirewallContext superclass.

nicolas-grekas · 2025-02-20T11:35:35Z

Let me close here as I think that while cleanups are welcome, this one is hard (with BC implications) for little practical benefit. 3 years passed also :)

scheb requested review from chalasr and wouterj as code owners October 23, 2021 09:41

carsonbot added Status: Needs Review Deprecation labels Oct 23, 2021

carsonbot added this to the 5.4 milestone Oct 23, 2021

scheb force-pushed the deprecate-return-logout-listener branch from 690b40c to 913a9c4 Compare October 23, 2021 09:57

derrabus added the Security label Oct 23, 2021

carsonbot changed the title ~~Deprecate LogoutListener being returned as 3rd element by FirewallMapInterface::getListeners~~ [Security] Deprecate LogoutListener being returned as 3rd element by FirewallMapInterface::getListeners Oct 23, 2021

chalasr requested changes Oct 23, 2021

View reviewed changes

src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 23, 2021

nicolas-grekas reviewed Oct 27, 2021

View reviewed changes

src/Symfony/Component/Security/Http/CHANGELOG.md Outdated Show resolved Hide resolved

src/Symfony/Component/Security/Http/FirewallMapInterface.php Outdated Show resolved Hide resolved

scheb force-pushed the deprecate-return-logout-listener branch from 913a9c4 to 982d79f Compare October 30, 2021 17:26

Deprecate FirewallMapInterface::getListeners and FirewallContext::get…

50b7a99

…Listeners

scheb force-pushed the deprecate-return-logout-listener branch from 982d79f to 50b7a99 Compare November 15, 2021 20:21

Fix psalm type annotation

a42a367

fabpot modified the milestones: 5.4, 6.1 Nov 16, 2021

fabpot modified the milestones: 6.1, 6.2 May 20, 2022

nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022

nicolas-grekas removed this from the 6.3 milestone May 23, 2023

nicolas-grekas added this to the 6.4 milestone May 23, 2023

nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 6, 2023

nicolas-grekas removed the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 18, 2023

nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023

xabbuh modified the milestones: 7.1, 7.2 May 15, 2024

fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024

nicolas-grekas closed this Feb 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

scheb commented Oct 23, 2021

chalasr left a comment

nicolas-grekas left a comment

fabpot commented Oct 30, 2021

scheb commented Oct 30, 2021

scheb commented Oct 30, 2021

chalasr commented Nov 12, 2021

scheb commented Nov 13, 2021

wouterj commented Nov 13, 2021

scheb commented Nov 15, 2021

nicolas-grekas commented Feb 20, 2025

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

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

Conversation

scheb commented Oct 23, 2021

chalasr left a comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

fabpot commented Oct 30, 2021

scheb commented Oct 30, 2021

scheb commented Oct 30, 2021

chalasr commented Nov 12, 2021

scheb commented Nov 13, 2021

wouterj commented Nov 13, 2021

scheb commented Nov 15, 2021

nicolas-grekas commented Feb 20, 2025