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

Fix ContextInvalidationLogoutHandler #394

Merged
merged 13 commits into from Nov 30, 2017

Conversation

Projects
None yet
3 participants
@wesnick
Copy link
Contributor

commented Oct 13, 2017

This fixes #392

  • doc update
  • changelog
  • unit test
  • deprecate old handler
@dbu
Copy link
Contributor

left a comment

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.

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

This comment has been minimized.

Copy link
@dbu

dbu Oct 13, 2017

Contributor

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.

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

This comment has been minimized.

Copy link
@dbu

dbu Oct 13, 2017

Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2017

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

@wesnick

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2017

@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
Copy link
Contributor

left a comment

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.

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

This comment has been minimized.

Copy link
@dbu

dbu Nov 9, 2017

Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2017

I will update docs later this afternoon

wesnick added some commits Nov 13, 2017

@wesnick

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2017

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

@dbu
Copy link
Contributor

left a comment

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.

@@ -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.

This comment has been minimized.

Copy link
@dbu

dbu Nov 13, 2017

Contributor

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."


* 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

This comment has been minimized.

Copy link
@dbu

dbu Nov 13, 2017

Contributor

... 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.

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.

This comment has been minimized.

Copy link
@dbu

dbu Nov 13, 2017

Contributor

... firewall configuration.

wesnick added some commits Nov 16, 2017

@wesnick

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2017

@dbu I think this is complete.

@dbu
Copy link
Contributor

left a comment

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

@@ -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

This comment has been minimized.

Copy link
@dbu

dbu Nov 21, 2017

Contributor

s/seesion/session/


* 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

This comment has been minimized.

Copy link
@dbu

dbu Nov 21, 2017

Contributor

s/seesion/session/

@@ -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

This comment has been minimized.

Copy link
@dbu

dbu Nov 21, 2017

Contributor

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

double whitespace after the . on this line.

.. 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

This comment has been minimized.

Copy link
@dbu

dbu Nov 21, 2017

Contributor

s/placea/places/

/**
* @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

This comment has been minimized.

Copy link
@dbu

dbu Nov 21, 2017

Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2017

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

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

3 checks passed

Scrutinizer 1 new issues, 6 updated code elements
Details
continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dbu

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2017

thanks a lot for fixing this problem, @wesnick !

@wesnick wesnick deleted the wesnick:context_invalidation_392 branch Nov 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.