[Security] Refactor authentication success handling #4599
Conversation
|
+1 from me @fabpot, what so you think? |
|
Can you add a note in the CHANGELOG? Thanks. |
|
I will, but I'll first do the same for the failure logic. |
| // success handler | ||
| if (isset($config['success_handler'])) { | ||
| $listener->replaceArgument(6, new Reference($config['success_handler'])); | ||
|
|
|
👍 thank you @stof. I think this is good to go now. |
|
@schmittjoh @fabpot The |
|
Yes, this code was removed, but needs to be re-added here as well. |
| @@ -18,3 +18,6 @@ CHANGELOG | |||
| * `ObjectIdentity::fromDomainObject`, `UserSecurityIdentity::fromAccount` and | |||
| `UserSecurityIdentity::fromToken` now return correct identities for proxies | |||
| objects (e.g. Doctrine proxies) | |||
| * [BC BREAK] moved the default authentication success and failure handling to | |||
There was a problem hiding this comment.
Why do we need to change the order?
I believe we could avoid that either by simply removing the = null part (PHP should not complain if there are required parameters after optional ones although it does not make sense), or the other alternative keep = null and simply instantiate the default handlers if none is passed.
There was a problem hiding this comment.
The contract of the class has changed as the handlers aren't optional anymore. I wasn't aware of the fact that you could have required parameters "after" optional ones.
So the options now are:
- BC break and change the order of the arguments
- Smaller BC break: only remove the
= nullfrom the constructor. People will still have to update their project to inject the failure/success handlers - No BC break by instantiating the handlers when they're not passed
@schmittjoh For the third option, how would passing the options for the handlers work? I guess the listener gets them all and passes them all?
@fabpot Any preference?
There was a problem hiding this comment.
@asm89 you cannot really have required parameters after optional ones. It will simply meant that the previous ones are not optional anyùore
There was a problem hiding this comment.
I know. So the BC break would then be that there are 3 extra required parameters, but no change of order. The last option wouldn't require a BC break at all I guess.
There was a problem hiding this comment.
If we have to do BC I'd vote for pt. 1. IMHO, optional parameters should be after the required ones.
There was a problem hiding this comment.
The point 3 would be an issue to handle the options properly. I would also vote for point 1. This BC break only affects people using the Security component standalone, not the framework users anyway (as the class is configured by SecurityBundle). And the point 2 also breaks BC but leads to weird code.
|
@fabpot Can you make a final decision on this? If you decide on point 3, this code can be merged. I agree with the arguments of @stof about the option handling and it 'only' being a BC break for direct users of the security component. I even think these direct users should be really careful anyway, since the behavior of the success and failurehandlers now change back to how they acted in 2.0. Now I am thinking about it, can't the optional parameters of this class move to setters anyway? That will make it cleaner to extend. |
|
ping @fabpot |
| * Can be optionally be extended from by the developer to alter the behaviour | ||
| * while keeping the default behaviour. | ||
| * | ||
| * @author Alexander <iam.asm89@gmail.com> |
There was a problem hiding this comment.
You should re-add the original authors here too.
|
I'm ok with option 1 (the BC break). After doing the last changes, can you squash your commits before I merge? Thanks. |
| @@ -77,7 +77,7 @@ | |||
| * @param LoggerInterface $logger A LoggerInterface instance | |||
| * @param EventDispatcherInterface $dispatcher An EventDispatcherInterface instance | |||
There was a problem hiding this comment.
As the order of the arguments changed, you should update the phpdoc
|
@fabpot I rebased the PR, added the authors and also ported the fix that was done in 8ffaafa to be contained in the default success handler. I also squashed all the CS and 'small blabla fix' commits. Is it ok now? Edit: travisbot will probably say that the tests in this PR fail, but that is because current master fails on form things |
| @@ -37,15 +37,15 @@ class UsernamePasswordFormAuthenticationListener extends AbstractAuthenticationL | |||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
| public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, array $options = array(), AuthenticationSuccessHandlerInterface $successHandler = null, AuthenticationFailureHandlerInterface $failureHandler = null, LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, CsrfProviderInterface $csrfProvider = null) | |||
| public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler = null, AuthenticationFailureHandlerInterface $failureHandler, array $options = array(), LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, CsrfProviderInterface $csrfProvider = null) | |||
There was a problem hiding this comment.
It seems, $failureHandler should be as optional parameter.
There was a problem hiding this comment.
no, it is the success handler which should now be mandatory as it is now mandatory in the parent class.
…eperate class [Security] Update configuration for changes regarding default success handler [Security] Fix + add AbstractFactory test
…eperate class [Security] Update configuration for changes regarding default failure handler [Security] Fixes + add AbstractFactory test for failure handler
…ure/success handling [Security] Various CS + doc fixes [Security] Exception when authentication failure/success handlers do not return a response [Security] Add authors + fix docblock
|
I rebased the PR, tests are green now: |
Commits ------- bb138da [Security] Fix regression after rebase. Target url should be firewall dependent eb19f2c [Security] Add note to CHANGELOG about refactored authentication failure/success handling [Security] Various CS + doc fixes [Security] Exception when authentication failure/success handlers do not return a response [Security] Add authors + fix docblock f9d5606 [Security] Update AuthenticationFailureHandlerInterface docblock. Never return null 915704c [Security] Move default authentication failure handling strategy to seperate class [Security] Update configuration for changes regarding default failure handler [Security] Fixes + add AbstractFactory test for failure handler c6aa392 [Security] Move default authentication success handling strategy to seperate class [Security] Update configuration for changes regarding default success handler [Security] Fix + add AbstractFactory test Discussion ---------- [Security] Refactor authentication success handling Bug fix: no Feature addition: no Backwards compatibility break: yes Symfony2 tests pass: [](http://travis-ci.org/asm89/symfony) License of the code: MIT This PR extracts the default authentication success handling to its own class as discussed in #4553. In the end the PR will basically revert #3183 (as suggested by @schmittjoh) and fix point one of #838. There are a few noticeable changes in this PR: - This implementation changes the constructor signature of the `AbstractAuthentictionListener` and `UsernamePasswordFormAuthenticationListener` by making the `AuthenticationSuccessHandler` mandatory (BC break). If this WIP is approved I will refactor the failure handling logic too and then this will also move one place in the constructor - This PR reverts the change of making the returning of a `Response` optional in the `AuthenticationSuccessHandlerInterface`. Developers can now extend the default behavior themselves @schmittjoh Any suggestions? Or a +1 to do the failure logic too? --------------------------------------------------------------------------- by schmittjoh at 2012-06-17T23:53:07Z +1 from me @fabpot, what so you think? --------------------------------------------------------------------------- by fabpot at 2012-06-19T08:15:48Z Can you add a note in the CHANGELOG? Thanks. --------------------------------------------------------------------------- by asm89 at 2012-06-19T10:22:20Z I will, but I'll first do the same for the failure logic. --------------------------------------------------------------------------- by travisbot at 2012-06-21T08:03:14Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1671555) (merged 17c8f66f into 55c6df9). --------------------------------------------------------------------------- by asm89 at 2012-06-21T08:45:38Z :+1: thank you @stof. I think this is good to go now. --------------------------------------------------------------------------- by travisbot at 2012-06-21T08:50:28Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1671817) (merged 8982c769 into 55c6df9). --------------------------------------------------------------------------- by asm89 at 2012-06-21T14:23:58Z @schmittjoh @fabpot The `LogoutListener` currently throws an exception when the successhandler doesn't return a `Response` ([link](https://github.com/symfony/symfony/blob/9e9519913d2c5e2bef96070bcb9106e1e389c3bd/src/Symfony/Component/Security/Http/Firewall/LogoutListener.php#L101)). Should this code check for this too? --------------------------------------------------------------------------- by schmittjoh at 2012-06-21T14:26:49Z Yes, this code was removed, but needs to be re-added here as well. --------------------------------------------------------------------------- by travisbot at 2012-06-21T15:08:59Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1674437) (merged 5afa240d into 55c6df9). --------------------------------------------------------------------------- by asm89 at 2012-06-26T06:01:02Z @fabpot Can you make a final decision on this? If you decide on point 3, this code can be merged. I agree with the arguments of @stof about the option handling and it 'only' being a BC break for direct users of the security component. I even think these direct users should be really careful anyway, since the behavior of the success and failurehandlers now change back to how they acted in 2.0. Now I am thinking about it, can't the optional parameters of this class move to setters anyway? That will make it cleaner to extend. --------------------------------------------------------------------------- by asm89 at 2012-06-28T10:29:50Z ping @fabpot --------------------------------------------------------------------------- by fabpot at 2012-06-28T17:23:02Z I'm ok with option 1 (the BC break). After doing the last changes, can you squash your commits before I merge? Thanks. --------------------------------------------------------------------------- by asm89 at 2012-07-06T21:59:54Z @fabpot I rebased the PR, added the authors and also ported the fix that was done in 8ffaafa to be contained in the default success handler. I also squashed all the CS and 'small blabla fix' commits. Is it ok now? Edit: travisbot will probably say that the tests in this PR fail, but that is because current master fails on form things --------------------------------------------------------------------------- by asm89 at 2012-07-08T18:53:05Z I rebased the PR, tests are green now: [](http://travis-ci.org/asm89/symfony).
|
Sorry if it's the wrong place to ask, but I can't figure out how to extend these default handlers. I've set 3rd argument is the options array, which is skipped here and compiled here. How am I supposed to use them?.. |
|
I've been diving into this. I'll create a separate issue. |
|
#4865 address your issue. Be aware that you need to set the options and optionally the |
|
@asm89 sorry but this didn't make it much easier... How do I get access to those options? How do I get access to |
|
You are now setting your own service as the success handler. It is your responsibility to set the appropriate options and the providerkey (by overriding the constructor and setting it up there). |
|
@asm89 so there's no cleaner way than duplicating bunch of option parameters and hardcoding(?) providerkey just to add a small functionality to handlers? |
|
@Inori It is easier now than it was in 2.0 where you even had to copy over all the code to get the same behaviour. |
|
@asm89 yeah it's actually ok after latest changes, ignore my last comment |
Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass:
License of the code: MIT
This PR extracts the default authentication success handling to its own class as discussed in #4553. In the end the PR will basically revert #3183 (as suggested by @schmittjoh) and fix point one of #838.
There are a few noticeable changes in this PR:
AbstractAuthentictionListenerandUsernamePasswordFormAuthenticationListenerby making theAuthenticationSuccessHandlermandatory (BC break). If this WIP is approved I will refactor the failure handling logic too and then this will also move one place in the constructorResponseoptional in theAuthenticationSuccessHandlerInterface. Developers can now extend the default behavior themselves@schmittjoh Any suggestions? Or a +1 to do the failure logic too?