Fix ContextInvalidationLogoutHandler #394

wesnick · 2017-10-13T10:33:10Z

This fixes #392

doc update
changelog
unit test
deprecate old handler

dbu

thanks a lot! this looks sane to me, i think its a good approach.

i wonder whether we should integrate the invalidation handler into the decorator class directly, as the composition is not important and the context invalidation method is not very complicated.

either way, we should keep the context invalidation logout handler for BC (as doc said to configure it in your security config) and instead of doing the pointless invalidation request, it would just output a deprecation warning that the new way of doing things does not require this handler anymore.

i know this is just the POC, but just so we dont forget: documentation will have to be updated when we really change this.

dbu · 2017-10-13T12:23:29Z

src/DependencyInjection/FOSHttpCacheExtension.php

@@ -295,6 +296,13 @@ private function loadUserContext(ContainerBuilder $container, XmlFileLoader $loa
            $container->getDefinition($this->getAlias().'.user_context.logout_handler')
                ->replaceArgument(1, $config['user_identifier_headers'])
                ->replaceArgument(2, $config['match']['accept']);
+
+            $container->register($this->getAlias().'.user_context.logout.handler.session', ContextInvalidationLogoutHandler::class)


i'd prefer to declare this in the user context xml service definition file. https://symfony.com/doc/current/service_container/service_decoration.html also has an example for that.

dbu · 2017-10-13T12:25:15Z

src/Security/Http/Logout/ContextInvalidationLogoutSuccessHandler.php

+use Symfony\Component\Security\Http\Logout\LogoutHandlerInterface;
+use Symfony\Component\Security\Http\Logout\SessionLogoutHandler;
+
+class ContextInvalidationSessionLogoutHandler implements LogoutHandlerInterface


should be final imho.

we should add a docblock with documentation to explain why we build this as decorator of the session logout handler rather than a stand-alone handler.

dbu · 2017-10-18T07:32:28Z

@wesnick do you have time to continue on this? otherwise i will try to follow up, its one of the 2 things i want to have for the 2.2 release.

wesnick · 2017-10-18T08:08:24Z

@dbu I don't have time to get to it this week, but probably next week. Feel free to move it forward.

A task still to take care of is the configuration scenario where you don't want to invalidate sessions in your logout handler but do want to invalidate user context cache (not sure the use case, but I suppose it could happen) needs some additional thought as it would not be possible to do that with the current direction of this PR.

NavyCoat · 2017-11-07T11:53:14Z

@wesnick Will you have some time to look at this again? I'm looking for next version of this bundle.
Not sure if i can help with this issue. This is the only reason why i don't make any contribution to this.

wesnick · 2017-11-08T14:18:43Z

I am looking at this now and will push some changes shortly.

…o xml

…ion_392

wesnick · 2017-11-08T15:07:47Z

@dbu This is how I think this issue should be solved. There is a simple context invalidation service that is shared between the old logout handler and the new logout handler. Old logout handler is deprecated for but remains for BC. New logout handler aliases security.logout.handler.session . The documentation can indicate that context invalidation is for all firewalls. If the advanced developer needs to invalidate context only on some firewalls, this scenario might require some more work. We might have to change enabled parameter to allow additional values since currently if the service is not enabled, I remove the context invalidation service from the container. At any rate, if you could review this code, I will continue with the additional items in the checklist.

dbu

hey, i like this a lot and the code looks very clean! just one comment where i would opt to be a bit more paranoid.

i think the super custom complicated cases should just be mentioned in the doc that you will need to create your own handlers and stuff. people that do that hopefully understand enough about the firewalls to do it - and i think for 99% of the cases this is enough. having more makes it a lot more complicated only to cover weird edge cases.

does out session handler alias win over the default service, regardless of whether security bundle or our bundle is loaded first?

please also add a note to the CHANGELOG - most important is to remove the config to use the fos_http_cache logout handler. and then mention that the new way replaces the default logout handler.

dbu · 2017-11-09T19:37:06Z

src/Security/Http/Logout/ContextInvalidationSessionLogoutHandler.php

+use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+use Symfony\Component\Security\Http\Logout\LogoutHandlerInterface;
+
+final class ContextInvalidationSessionLogoutHandler implements LogoutHandlerInterface


can we add a phpdoc that this replaces the session logout handler? i wonder if we should extend Symfony\Component\Security\Http\Logout\SessionLogoutHandler and call parent::logout. this increases the coupling, but i prefer that over not noticing when symfony changes that handler somehow...

wesnick · 2017-11-10T16:29:45Z

does out session handler alias win over the default service, regardless of whether security bundle or our bundle is loaded first?

I don't know the answer to this. I don't know who I would ask either. I could move to a compiler pass to be sure?

dbu · 2017-11-13T10:03:48Z

i just looked at the code of container.php and found this:

// Attempt to retrieve the service by checking first aliases then
// available services.

so its clear that its intentional that the alias wins over a service, so it won't accidentally change i think. can you please rebase to solve the merge conflict and extend the base logout handler and call its method, to make it explicit what we still do the same as that listener does?

wesnick · 2017-11-13T10:57:59Z

I will update docs later this afternoon

wesnick · 2017-11-13T15:42:09Z

tests fail on 7.2 with a segmentation fault, not sure what I can do about it

dbu

thank you, looks good. the segfault is weird - master builds fine. but i don't see anything unusual in your code that would explain to me why it would segfault. lets see if it was a one-time oddity if you push again with the tweaks to the doc i ask for. if it keeps happening, we could create a branch where you partially revert the changes to isolate the exact line that causes the problem.

dbu · 2017-11-13T19:21:47Z

Resources/doc/reference/configuration/user-context.rst

@@ -139,26 +139,18 @@ or you will end up with mixed up caches.
 ~~~~~~~~~~~~~~~~~~

 The logout handler will invalidate any cached user hashes when the user logs
-out.
+out. This behavior applies to all stateful firewalls.


i would prefer a .. note:: block with this plus the extra text you add below. something like: "The logout handler is active on all firewalls. If your application has multiple firewalls with different user contexts, you need to create your own custom invalidation handler. Be aware that Symfony's LogoutSuccessHandler places the SessionLogoutHandler before any configured logout handlers."

dbu · 2017-11-13T19:23:36Z

CHANGELOG.md

+
+* The ContextInvalidationLogoutHandler has been deprecated in favor of the 
+  ContextInvalidationSessionLogoutHandler.  The original handler was called 
+  after the invalidation of the session so did not correctly invalidate the 


... session so did not correctly invalidate the session ID in cache... => session, and thus did not invalidate the session it should have but a newly created one.

dbu · 2017-11-13T19:23:50Z

CHANGELOG.md

+  after the invalidation of the session so did not correctly invalidate the 
+  session ID in cache.  You should remove the deprecated service 
+  `fos_http_cache.user_context.logout_handler` from the logout.handlers section
+  of your firewall. 


... firewall configuration.

wesnick · 2017-11-21T14:36:55Z

@dbu I think this is complete.

dbu

great, looks good. i only have some spelling issues that i would like you to fix, then i can merge this.

dbu · 2017-11-21T15:50:40Z

CHANGELOG.md

@@ -27,10 +27,10 @@ Changelog

 * The ContextInvalidationLogoutHandler has been deprecated in favor of the 
  ContextInvalidationSessionLogoutHandler.  The original handler was called 
-  after the invalidation of the session so did not correctly invalidate the 
-  session ID in cache.  You should remove the deprecated service 
+  after the invalidation of the session, and thus did not invalidate the seesion


s/seesion/session/

dbu · 2017-11-21T15:51:07Z

CHANGELOG.md

+
+* The ContextInvalidationLogoutHandler has been deprecated in favor of the 
+  ContextInvalidationSessionLogoutHandler.  The original handler was called 
+  after the invalidation of the session, and thus did not invalidate the seesion


s/seesion/session/

dbu · 2017-11-21T15:51:52Z

CHANGELOG.md

@@ -24,6 +24,13 @@ Changelog

 * User context is more reliable not cache when the hash mismatches. (E.g. after
  login/logout.)
+
+* The ContextInvalidationLogoutHandler has been deprecated in favor of the 
+  ContextInvalidationSessionLogoutHandler.  The original handler was called 


please use backticks around class names (this is markdown, single backtick is good).

double whitespace after the . on this line.

dbu · 2017-11-21T15:52:34Z

Resources/doc/reference/configuration/user-context.rst

+.. note::
+    The logout handler is active on all firewalls.  If your application has multiple firewalls
+    with different user context, you need to create your own custom invalidation handler. Be
+    aware that Symfony's ``LogoutSuccessHandler`` placea the ``SessionLogoutHandler`` before


s/placea/places/

dbu · 2017-11-21T15:54:34Z

src/Security/Http/Logout/ContextInvalidationLogoutHandler.php

+/**
+ * @deprecated use ContextInvalidationSessionLogoutHandler in this same namespace as a replacement
+ *
+ * This handler is deprecated since in most cases the session has already been invalidated by the SessionLogoutHandler


lets be more affirmative: "This handler is deprecated because it never did what it was supposed to do. The session is already invalidated by the SessionLogoutHandler which is always the first logout handler executed."

wesnick · 2017-11-30T09:17:02Z

@dbu I was out for a bit, now back and I updated PR per comments

dbu · 2017-11-30T09:59:44Z

thanks a lot for fixing this problem, @wesnick !

POC decorating SessionLogoutHandler

033e8ca

dbu reviewed Oct 13, 2017

View reviewed changes

wesnick added 5 commits November 8, 2017 15:39

define independent context invalidation service, move configuration t…

525f207

…o xml

Merge remote-tracking branch 'upstream/master' into context_invalidat…

c5f4bcd

…ion_392

fix style issues

534f5c8

fix additional style issue

5a104a7

deprecate ContextInvalidationLogoutHandler

4b96b5b

dbu reviewed Nov 9, 2017

View reviewed changes

wesnick added 2 commits November 13, 2017 11:49

merge master

a71165c

changes per PR comments

3c6a976

wesnick added 2 commits November 13, 2017 16:04

extend class without using parent definition

6f42bf6

add some docs

3453740

dbu reviewed Nov 13, 2017

View reviewed changes

wesnick added 2 commits November 16, 2017 18:42

update documentation

047877f

trigger new tests

8af1dc3

dbu requested changes Nov 21, 2017

View reviewed changes

fixes per PR review

ddcba7d

dbu merged commit 3ec143a into FriendsOfSymfony:master Nov 30, 2017

wesnick deleted the context_invalidation_392 branch November 30, 2017 10:09

Fix ContextInvalidationLogoutHandler #394

Fix ContextInvalidationLogoutHandler #394

Uh oh!

Conversation

wesnick commented Oct 13, 2017 • edited by dbu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Oct 18, 2017

Uh oh!

wesnick commented Oct 18, 2017

Uh oh!

NavyCoat commented Nov 7, 2017

Uh oh!

wesnick commented Nov 8, 2017

Uh oh!

wesnick commented Nov 8, 2017

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesnick commented Nov 10, 2017

Uh oh!

dbu commented Nov 13, 2017

Uh oh!

wesnick commented Nov 13, 2017

Uh oh!

wesnick commented Nov 13, 2017

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesnick commented Nov 21, 2017

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesnick commented Nov 30, 2017

Uh oh!

dbu commented Nov 30, 2017

Uh oh!

Uh oh!

wesnick commented Oct 13, 2017 •

edited by dbu

Loading