Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

scheb
Copy link
Contributor

@scheb scheb commented Oct 17, 2021

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

This is follow-up of introducing the ability to sort security listeners, which was introduced in #37337. Part of the discussion at that time was whether we'd still need to hardcoded sorting algorithm in the Firewall class, which was previously needed to sort in the LogoutListener at the correct position. With the ability to sort by a priority that was actually no longer needed.

I had this change temporarily implemented in 1972786, though in the discussion it was decided against and instead keep the code for backwards compatibility reasons. Otherwise we'd need to declare a conflict with security-bundle in the composer.json and it was argued by @chalasr against making the security component aware of security-bundle (see #37337 (comment)).

Though I've recognized, things have changed in the meantime, and security component is now declaring conflict for security-bundle (introduced by @nicolas-grekas in 314ef9f)

So I want to give this another shot and clean up the Firewall class, targeting potentially the 5.4 branch, definitely for the 6.0 branch.

Considerations
To make the clean-up work, the LogoutListener service needs to be added to the list of listeners generated by SecurityExtension. Until now, it's intentionally (?) left out. In the 5.4 branch this could be considered a breaking change. Let me know what you think, if this is in fact considered as breaking, I'm happy to change the target to 6.0.

@scheb scheb requested review from chalasr and wouterj as code owners October 17, 2021 16:08
@carsonbot carsonbot added this to the 5.4 milestone Oct 17, 2021
@chalasr
Copy link
Member

chalasr commented Oct 17, 2021

Thanks for the PR.
The logout listener has always been special as in separated from other listeners:

* @return array of the format [[AuthenticationListener], ExceptionListener, LogoutListener]
*/
public function getListeners(Request $request);

With the proposed patch, the logout listener returned from FirewallMap is not used anymore which looks like a BC break.
Since there is no more reason to separate the logout listener from other listeners thanks to priorities, I suppose we can deprecate returning the logout listener separately from other listeners in FirewallMapInterface, then remove it along with the sorting logic in 6.0.

@scheb
Copy link
Contributor Author

scheb commented Oct 19, 2021

Excellent 👍

I'll re-target the PR to 6.0. How should we move ahead with the deprecation? (How do you deprecate a value in a returned array? 🤔)

@scheb scheb changed the base branch from 5.4 to 6.0 October 19, 2021 16:23
@scheb scheb force-pushed the runtime-sorting-security-listener branch from 29a751c to 3ca2103 Compare October 19, 2021 16:28
@chalasr
Copy link
Member

chalasr commented Oct 19, 2021

Triggering a notice from Firewall when $listeners[2] is set and is a logout listener should be enough in this case. The return annotation should be updated to reflect what is expected on 6.0 also (no logout listener in the array)

@carsonbot carsonbot changed the title Remove sorting of security listeners at runtime from Firewall [Security] Remove sorting of security listeners at runtime from Firewall Oct 19, 2021
@scheb
Copy link
Contributor Author

scheb commented Oct 21, 2021

I can take care for a PR to add that deprecation.

Could someone update the milestone on this PR to 6.0 (I can't). Thanks! 🙇

@scheb
Copy link
Contributor Author

scheb commented Oct 23, 2021

PR for deprecation is open. Once that's done, I'll rebase this one and include a clean-up of deprecations.

@fabpot fabpot modified the milestones: 5.4, 6.1 Nov 16, 2021
@fabpot fabpot modified the milestones: 6.1, 6.2 May 20, 2022
@nicolas-grekas nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@nicolas-grekas
Copy link
Member

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants