[Security] make it possible to override the default success/failure handler #11993
Conversation
|
This implementation does not work as we need to introspect the custom handler definition, which we cannot as we are in the security bundle extension at this point without any access to externally defined services. The only option would be to move some code to a compiler pass. |
842360c to
13d37e7
Compare
13d37e7 to
36116fc
Compare
|
I'm going to rewrite this PR with a different approach. Stay tuned. |
|
@fabpot : This ties my previous suggestion and should cover all cases 👍 |
167c3f2 to
f1e4d45
Compare
|
@ogizanagi The new implementation I've just merged should work properly now and there is no ugly hacks/conventions anymore. There is one slight side-effect: as we are passing options and the provider key, you must not use the same custom handler in two different firewalls. It was not a problem in the current implementation as the handler was independent of the firewall, but that's not the case anymore. The documentation should mention this to avoid hard to debug problems. |
f1e4d45 to
4c08db8
Compare
4c08db8 to
810eeaf
Compare
|
ping @symfony/deciders |
|
Seems ok to me, not very symfonish as we usually don't call custom things custom_lala but just lala but BC is BC so I'd guess 👍 |
| @@ -34,6 +34,12 @@ class DefaultAuthenticationFailureHandler implements AuthenticationFailureHandle | |||
| protected $httpUtils; | |||
| protected $logger; | |||
| protected $options; | |||
| protected $defaultOptions = array( | |||
…ccess/failure handler (fabpot) This PR was merged into the 2.6-dev branch. Discussion ---------- [Security] make it possible to override the default success/failure handler | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #5432, #9272, #10417, #11926 | License | MIT | Doc PR | symfony/symfony-docs#4258 Overriding the default success/failure handler of the security firewalls is possible via the `success_handler` and `failure_handler` setting but this approach is not flexible as it does not allow you to get the options/provider key. To sum up the problem: * Overriding the default success/failure handler is possible via a service; * When not overridden, the default success/failure handler gets options and the provider key; * Those options and the provider key are injected by the factory as they are dynamic (they depend on the firewall and the provider key), so getting those options/provider key is not possible for a custom service that is only configured via the container configuration; * Extending the default handler does not help as the injection mechanism is only triggered when no custom provider is set; * Wrapping the default handler is not possible as the service id is dynamic. ... and of course we need to keep BC and make it work for people extending the default handler but also for people just using the interface. Instead of the current PR, I propose this slightly different approach. It's not perfect, but given the above constraint, I think this is an acceptable trade-of. So, several use cases: * Using the default handler (no change); * Using a custom handler that implements `AuthenticationSuccessHandlerInterface` directly and does not need any options (no change); * Using a custom handler that needs the options/provider key (that's the new use case this PR supports). This PR introduces 2 new classes that wrap custom handlers. If those classes define the `setOptions()` and/or `setProviderKey()` methods, they are automatically called with the correct arguments. Yours handler does not need to extend the default handler `DefaultAuthentication*Handler`, but doing so helps as the setters are already defined there. Commits ------- 810eeaf [Security] made it possible to override the default success/failure handler (take 2) 36116fc [Security] made it possible to override the default success/failure handler
|
I like this aproach. It's a tricky problem but this seems to solve it. Thanks Fabien! 👍 |
Overriding the default success/failure handler of the security firewalls is possible via the
success_handlerandfailure_handlersetting but this approach is not flexible as it does not allow you to get the options/provider key.To sum up the problem:
... and of course we need to keep BC and make it work for people extending the default handler but also for people just using the interface.
Instead of the current PR, I propose this slightly different approach. It's not perfect, but given the above constraint, I think this is an acceptable trade-of.
So, several use cases:
AuthenticationSuccessHandlerInterfacedirectly and does not need any options (no change);This PR introduces 2 new classes that wrap custom handlers. If those classes define the
setOptions()and/orsetProviderKey()methods, they are automatically called with the correct arguments. Yours handler does not need to extend the default handlerDefaultAuthentication*Handler, but doing so helps as the setters are already defined there.