[Security] [Firewall] Bug fixed in SimplePreAuthenticationListener when createToken() not return TokenInterface object #11414
Conversation
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | no |
| Fixed tickets | #11490 |
| License | MIT |
| Doc PR |
…eturn TokenInterface object
|
I can't use this case because createToken() must return TokenInteface object |
…eturn TokenInterface object
…eturn TokenInterface object
| @@ -3,3 +3,4 @@ composer.lock | |||
| composer.phar | |||
| autoload.php | |||
| /vendor/ | |||
| /nbproject/private/ | |||
There was a problem hiding this comment.
This must be ignored locally instead, because they are specific to your development environment, not to Symfony (and you should ignore all your netbeans files, not only the private ones. We don't want the other netbeans files in the repo either): https://help.github.com/articles/ignoring-files
There was a problem hiding this comment.
@stof 👍 thanks for the response... i will push again
…eturn TokenInterface object
…eturn TokenInterface object
| @@ -3,3 +3,4 @@ composer.lock | |||
| composer.phar | |||
| autoload.php | |||
| /vendor/ | |||
| /nbproject/ | |||
There was a problem hiding this comment.
Use a global ignore file to ignore IDE specific files
There was a problem hiding this comment.
@yguedidi I said it should be ignored locally, not in Symfony
There was a problem hiding this comment.
Yes I know, that's why I said to use a global .gitignore file
EDIT: Or you mean a .gitignore file in /nbproject ?
| } | ||
| $this->securityContext->setToken(null); | ||
|
|
||
| return; |
There was a problem hiding this comment.
you are still modifying the behavior of the code when adding this return statement here.
the code if ($this->simpleAuthenticator instanceof AuthenticationSuccessHandlerInterface) { below cannot be reached anymore while it was previously
There was a problem hiding this comment.
Also, the token on security context is always set to null.
|
I've also realized this "oddity". I wasn't sure if it was a bug, but I realized that it's very inflexible currently (and yes, I think the example he linked to is therefore wrong in the docs). I really like the idea of being able to return nothing from |
|
sorry for the late response :-) and i'm sorry for my stupid code |
|
Can you have a look at that fix? If it gets merged #11490 and symfony/symfony-docs#3815 can be closed. |
|
watching |
|
Returning an anonymous token is the way to go and it is the most "consistent" way. But, as the goal of the Moreover, if we only merge this into master (aka 2.6), the documentation will have to describe both ways: using an anonymous token before 2.6, and So, my proposal is to allow returning |
|
By the way, this PR cannot work as is because when a Token is null, the following code will fail: if ($this->simpleAuthenticator instanceof AuthenticationSuccessHandlerInterface) {
$response = $this->simpleAuthenticator->onAuthenticationSuccess($request, $token); |
|
I agree with merging this into all versions - allowing null suddenly isn't a BC break as returning null currently causes an error. About the code Fabien just posted, I think we should change the $token = $this->simpleAuthenticator->createToken($request, $this->providerKey);
// allow null to be returned to skip authentication
if ($token === null) {
return;
}
$token = $this->authenticationManager->authenticate($token);
$this->securityContext->setToken($token);This also removes a line in the current PR that sets the token to null that I don't think should be there. Thanks! |
|
I've finished this PR in #12002 and based it on 2.4. Thanks @ihsanudin for your work on this PR. |
…Listener when createToken() not return TokenInterface object (adenkejawen, fabpot) This PR was merged into the 2.4 branch. Discussion ---------- [Security] [Firewall] Bug fixed in SimplePreAuthenticationListener when createToken() not return TokenInterface object | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | #11490, #11414 | License | MIT | Doc PR | This is a follow-up for #11414 on the right branch. Commits ------- faa8e98 fixed bug e85cb7f added the possibility to return null from SimplePreAuthenticationListener