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

[SecurityBundle] fix setLogoutOnUserChange calls for context listeners #25272

Closed
wants to merge 7 commits into
base: 3.4
from

Conversation

Projects
None yet
7 participants
@dmaicher
Contributor

dmaicher commented Dec 2, 2017

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #25267
License MIT
Doc PR -

As pointed out in #25267 the setLogoutOnUserChange method calls were added to the parent definition security.context_listener instead of the concrete child definitions security.context_listener.*.

ping @iltar @chalasr

@chalasr

chalasr approved these changes Dec 3, 2017

@Xymanek

This comment has been minimized.

Show comment
Hide comment
@Xymanek

Xymanek Dec 3, 2017

How will this work if multiple firewalls share the same context?

Xymanek commented Dec 3, 2017

How will this work if multiple firewalls share the same context?

@dmaicher

This comment has been minimized.

Show comment
Hide comment
@dmaicher

dmaicher Dec 3, 2017

Contributor

@Xymanek hmm I think you are right. That case is still not handled properly 😢

I will look into it.

Contributor

dmaicher commented Dec 3, 2017

@Xymanek hmm I think you are right. That case is still not handled properly 😢

I will look into it.

@dmaicher

This comment has been minimized.

Show comment
Hide comment
@dmaicher

dmaicher Dec 3, 2017

Contributor

@iltar @chalasr any idea how to fix this?

Seems ContextListener::$logoutOnUserChange needs some rethinking as indeed the context instance can be shared amongst multiple firewalls.

Within the ContextListener I don't see any way of knowing which firewall(s) its currently handling? So how to decide $logoutOnUserChange per firewall then?

Or we enforce (with an error when validating the config?) that all firewalls that share a context have the same value for logout_on_user_change?

Contributor

dmaicher commented Dec 3, 2017

@iltar @chalasr any idea how to fix this?

Seems ContextListener::$logoutOnUserChange needs some rethinking as indeed the context instance can be shared amongst multiple firewalls.

Within the ContextListener I don't see any way of knowing which firewall(s) its currently handling? So how to decide $logoutOnUserChange per firewall then?

Or we enforce (with an error when validating the config?) that all firewalls that share a context have the same value for logout_on_user_change?

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Dec 3, 2017

Member

@dmaicher I think it should throw indeed

Member

chalasr commented Dec 3, 2017

@dmaicher I think it should throw indeed

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Dec 4, 2017

@chalasr

chalasr approved these changes Dec 4, 2017

@@ -340,17 +340,6 @@ private function addFirewallsSection(ArrayNodeDefinition $rootNode, array $facto
return $firewall;
})
->end()
->validate()

This comment has been minimized.

@dmaicher

dmaicher Dec 4, 2017

Contributor

@chalasr this is now actually not needed anymore since triggering the deprecation is moved and it's not triggered anymore at all if stateless = true or security = false.

@dmaicher

dmaicher Dec 4, 2017

Contributor

@chalasr this is now actually not needed anymore since triggering the deprecation is moved and it's not triggered anymore at all if stateless = true or security = false.

This comment has been minimized.

@chalasr

chalasr Dec 4, 2017

Member

good catch!

@chalasr

chalasr Dec 4, 2017

Member

good catch!

@@ -43,6 +43,7 @@ class SecurityExtension extends Extension
private $factories = array();
private $userProviderFactories = array();
private $expressionLanguage;
private $logoutOnUserChangeByContextKey = array();

This comment has been minimized.

@dmaicher

dmaicher Dec 4, 2017

Contributor

@chalasr should I add some deprecation info here? Did not see that for private member variables elsewhere?

@dmaicher

dmaicher Dec 4, 2017

Contributor

@chalasr should I add some deprecation info here? Did not see that for private member variables elsewhere?

This comment has been minimized.

@chalasr

chalasr Dec 4, 2017

Member

Let's let it as is and keep it in mind, note for mergers: this prop should be removed when merging in 4.0 (I can take care of the merge)

@chalasr

chalasr Dec 4, 2017

Member

Let's let it as is and keep it in mind, note for mergers: this prop should be removed when merging in 4.0 (I can take care of the merge)

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Dec 4, 2017

Member

Thanks for fixing this bug @dmaicher.

Member

chalasr commented Dec 4, 2017

Thanks for fixing this bug @dmaicher.

chalasr added a commit that referenced this pull request Dec 4, 2017

bug #25272 [SecurityBundle] fix setLogoutOnUserChange calls for conte…
…xt listeners (dmaicher)

This PR was squashed before being merged into the 3.4 branch (closes #25272).

Discussion
----------

[SecurityBundle] fix setLogoutOnUserChange calls for context listeners

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #25267
| License       | MIT
| Doc PR        | -

As pointed out in #25267 the `setLogoutOnUserChange` method calls were added to the parent definition `security.context_listener` instead of the concrete child definitions `security.context_listener.*`.

ping @iltar @chalasr

Commits
-------

4eff146 [SecurityBundle] fix setLogoutOnUserChange calls for context listeners

@fabpot fabpot closed this Dec 4, 2017

This was referenced Dec 4, 2017

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