[security] Fetching current stored context when not explicitly specified #13342
Conversation
| */ | ||
| public function __construct(ContainerInterface $container, UrlGeneratorInterface $router) | ||
| public function __construct(RequestStack $requestStack, UrlGeneratorInterface $router, TokenStorageInterface $tokenStorage) |
There was a problem hiding this comment.
This was an optional fix. I can change this back without any problems.
|
@stof I've implemented your comments, and reverted the BC break. |
|
It's still a BC break because of the extra required argument. |
|
@Tobion I see. I thought you were only talking about the container/requeststack change. Since we already injected the container, we could (ab)use this to fetch the token-storage from that. I agree this would not be a nice thing to (this is why i removed to container in favor of the request-stack in the first place), but it would not break the signature of the constructor. Would that be a viable option (if not, i'll just add this feature as BC break)? |
| @@ -68,7 +72,7 @@ public function registerListener($key, $logoutPath, $csrfTokenId, $csrfParameter | |||
| * | |||
| * @return string The logout path | |||
| */ | |||
| public function getLogoutPath($key) | |||
| public function getLogoutPath($key = null) | |||
There was a problem hiding this comment.
phpdoc needs update with string|null and description what it means
There was a problem hiding this comment.
and this change must be documented in the UPGRADE file
There was a problem hiding this comment.
People extending the class will have to update their method signatures. That's why the BC promise requires this change.
There was a problem hiding this comment.
@xabbuh The getLogoutPath() and getLogoutUrl() from the helper can indeed used with a default argument and do not have to change.
Only the getLogoutPath() and getLogoutUrl() from the LogoutUrlExtension must have a default value, which according to the BC promise is allowed. The only question I have, in which UPGRADE file must I add this information?
|
You can write the constructor so that you remove the typehint and allow both the container or the request stack to be passed. So both versions work. And then you can mention in the phpdoc that passing the container is deprecated. And the TokenStorage as third parameter can be made optional ($storage = null) and say document that it will be required in symfony 3.0. If not given, fetch it from the container. |
|
@Tobion Thanks, that makes sense.. I'll try and fix that (and also the missing docblocks).. |
|
@Tobion I've fixed the BC break (+squashed the commits). Does it look all right now or are there still issues? |
| */ | ||
| public function __construct(ContainerInterface $container, UrlGeneratorInterface $router) | ||
| public function __construct($container, UrlGeneratorInterface $router, TokenStorageInterface $tokenStorage = null) |
There was a problem hiding this comment.
please rename $container to $requestStack to be future ready
|
@Tobion Updated. The |
|
👍 |
| $key = $token->getProviderKey(); | ||
| } | ||
| } | ||
|
|
||
| if (!array_key_exists($key, $this->listeners)) { | ||
| throw new \InvalidArgumentException(sprintf('No LogoutListener found for firewall key "%s".', $key)); |
There was a problem hiding this comment.
Would it be better to throw a specific exception when key is null? Probably in the condition above when we are not able to find the corresponding key? Something like "Unable to find the current firewall LogoutListener, please specify the provider key."
There was a problem hiding this comment.
@fabpot the feature ia about handling the current context. By specifying null, it will use the current provider key so we can reuse a twig template with logout_url and/or not dependable anymore of the firewall name. If a default key cannot be found, the invalidargumentexception on the lines below will be triggered as a fallback anyway.
There was a problem hiding this comment.
I do understand the change, but if the getProviderKey() method does not exist, the $key will still be null here, in which case, the error message is not the right one.
There was a problem hiding this comment.
ok, I'll see if I can find an exception that can be added.
|
What about triggering a deprecation notice when a |
|
@xabbuh i'll add one today |
| */ | ||
| private function generateLogoutUrl($key, $referenceType) | ||
| { | ||
| // Fetch the current provider key from token, if possible | ||
| if (null === $key && $this->tokenStorage) { |
There was a problem hiding this comment.
I would be more explicit when checking the tokenStorage: null !== $this->tokenStorage
|
@jaytaph This PR looks like a mess, something wrong happened. Can you create a new one? |
|
@fabpot Rebase gone wild, it seems. I've rebased it correctly and it should be ready. |
| $this->container = $container; | ||
| if ($requestStack instanceof ContainerInterface) { | ||
| $this->requestStack = $requestStack->get('request_stack'); | ||
| trigger_error('The '.__CLASS__.' constructor will require a RequestStack instead of a containerInterface instance in 3.0.', E_USER_DEPRECATED); |
There was a problem hiding this comment.
/containerInterface/ContainerInterface/
|
@hhamon Fixed |
|
👍 |
|
Thank you @jaytaph. |
This patch will use the current stored context found in a token (provided, there is one), if none has been specified.
According to a quick scan of the code, this will be the only place where
getProviderKey()is used outside a specific class (the authentication providers will check token type before callinggetProviderKey(), but maybe it's be a good idea to implement a "providerKeyTokenInterface" or something. It's a nicer solution imho than the currentmethod_exists()