Skip to content
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

[1.x] Add forward compat layer for changed logout listener configuration #350

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Dec 7, 2022

See https://github.com/markitosgv/JWTRefreshTokenBundle/pull/347/files/dd75327f8d93d178fa32cbaac929c90a84548ab0#r1037883273 for context

The 2.0 proposal includes changing how the logout listener that handles token and cookie invalidation is configured. In the 2.0 branch, the listener is configured based on the authenticator configuration instead of the global bundle configuration, which allows improving multi-firewall compatibility by configuring the listener differently for each firewall. This provides a compatibility layer to start moving that config in a future 1.x release.

Note that there are a couple of functional B/C breaks here:

  • The LogoutEventListener is turned into an abstract service definition that is used to configure concrete services, this impacts folks who might be using compiler passes in their own application to tweak this service
  • Changes the logout_firewall config node to default to null instead of "api", this is both a bug fix and a B/C break in that with the default config as it was previously you couldn't really turn off the listener without a compiler pass

This also adds tests for the LogoutEventListener class, previously untested.

@mbabker mbabker mentioned this pull request Dec 7, 2022
4 tasks
@mbabker
Copy link
Contributor Author

mbabker commented Dec 7, 2022

CI's blocked by #348

Rebased and G2G

@@ -51,6 +52,10 @@ public function addConfiguration(NodeDefinition $builder): void
->scalarNode('provider')->end()
->scalarNode('success_handler')->end()
->scalarNode('failure_handler')->end()
->booleanNode('invalidate_token_on_logout')
->defaultFalse() // TODO - Enable by default in 2.0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should set it to null here and run a deprecation if the boolean has not been defined in order to activate it by default in 2.0 without BC.

Isn't there a way to get the bundle configuration here or in the createAuthenticator in order to activate it by default when the 'logout_firewall' is null ? This would allow us to keep the default value on 'logout_firewall' without creating a BC and to launch a deprecation when it is defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should set it to null here and run a deprecation if the boolean has not been defined in order to activate it by default in 2.0 without BC.

I'd only do that if we needed the value to actually have a configuration (the same way the deprecation if you don't have the model class set in the bundle config works), because the 1.x branch supports both Symfony 4.4 and 5.4 we can't sanely keep the listener turned on by default for both workflows (which would result in the listener being registered on both the global event dispatcher and the firewall specific event dispatcher, and I have no idea how Symfony deals with merging and de-duping stuff for the firewall specific event dispatcher but I know it does move some stuff around).

Isn't there a way to get the bundle configuration here or in the createAuthenticator in order to activate it by default when the 'logout_firewall' is null ? This would allow us to keep the default value on 'logout_firewall' without creating a BC and to launch a deprecation when it is defined.

We can get to container parameters, but we can't get the processed $config array. For 1.x, I'm choosing to default to the listener being off by default with this backport and then restoring the current 1.1 behavior of it being active by default for 2.0 when we only have a single authentication flow to worry about.

As for the logout_firewall config node, that one's a bit of a pain point to work around. It defaulting to "api" really should've been something added to the Flex recipe for this bundle and not hardcoded into the bundle configuration. So we can't rely on checking if its value matches the current default of "api", we don't know if the user explicitly wanted that or if it was implicitly set to that by way of not configuring it. Changing its default to null gives us a better gauge of whether the node is explicitly configured or not and lets us do stuff with that information at compile time.

@mbabker mbabker force-pushed the 2.x-logout-listener-config-compat branch 2 times, most recently from f2acf8f to 6cd54cd Compare January 2, 2023 17:44
@mbabker mbabker force-pushed the 2.x-logout-listener-config-compat branch from 6cd54cd to c5d7967 Compare January 17, 2023 19:31
@mbabker mbabker force-pushed the 2.x-logout-listener-config-compat branch from c5d7967 to db55b9c Compare July 24, 2023 22:44
@shakaran
Copy link

@mbabker maxhelias any chance of this get merged soon? which is needed to progress this?

@mbabker mbabker force-pushed the 2.x-logout-listener-config-compat branch from db55b9c to 8287fbf Compare January 23, 2024 15:53
@markitosgv
Copy link
Owner

@mbabker is it safe to merge this old PR?

@mbabker mbabker force-pushed the 2.x-logout-listener-config-compat branch from 8287fbf to f75abc4 Compare May 13, 2024 14:45
@mbabker
Copy link
Contributor Author

mbabker commented May 13, 2024

@markitosgv Yes, this is good to merge

@mbabker mbabker force-pushed the 2.x-logout-listener-config-compat branch from f75abc4 to 396b203 Compare May 13, 2024 14:54
@mbabker mbabker force-pushed the 2.x-logout-listener-config-compat branch from 396b203 to bee9f90 Compare December 30, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants