Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fix bad method call with guard authentication + session migration#27581
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
The original setter was put onto the wrong class. The handler is a bitmore difficult, as there is one handler only. So, we need to pass ina statelessFirewalls array so we know whether or not to migrate thesession
7d943c1 tocd73af2Compare| * Authenticates the given token in the system. | ||
| */ | ||
| publicfunctionauthenticateWithToken(TokenInterface$token,Request$request) | ||
| publicfunctionauthenticateWithToken(TokenInterface$token,Request$request,$providerKey =null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
even optional, this would break a child (signature mismatch).func_get_arg() should be used here :)
weaverryan commentedJun 12, 2018
Thanks@chalasr! Made the change |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
With minor cs comments.
| publicfunction__construct(TokenStorageInterface$tokenStorage,EventDispatcherInterface$eventDispatcher =null) | ||
| /** | ||
| * @param TokenStorageInterface $tokenStorage | ||
| * @param EventDispatcherInterface|null $eventDispatcher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We're ok with partial docblocks, so these 2 above lines can be removed.
| publicfunctionauthenticateWithToken(TokenInterface$token,Request$request) | ||
| { | ||
| $this->migrateSession($request,$token); | ||
| $providerKey =func_num_args() >2 ?func_get_arg(2) :null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
\func_num_args()
The new argument should be added on the signature between /**/, and on the docblock.
| privatefunctionmigrateSession(Request$request,TokenInterface$token,$providerKey) | ||
| { | ||
| if (!$this->sessionStrategy || !$request->hasSession() || !$request->hasPreviousSession()) { | ||
| if (!$this->sessionStrategy || !$request->hasSession() || !$request->hasPreviousSession() ||in_array($providerKey,$this->statelessProviderKeys)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
\in_array(..., true)
weaverryan commentedJun 12, 2018
Let's try this :) - not 100% sure I got that format for the documented new arg right 😇 |
| * @param string $providerKey The name of the provider/firewall being used for authentication | ||
| */ | ||
| publicfunctionauthenticateWithToken(TokenInterface$token,Request$request) | ||
| publicfunctionauthenticateWithToken(TokenInterface$token,Request$request/* $providerKey */) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(TokenInterface $token, Request $request/*, string $providerKey */)
chalasr commentedJun 12, 2018
Thank you@weaverryan. |
…gration (weaverryan)This PR was squashed before being merged into the 2.8 branch (closes#27581).Discussion----------Fix bad method call with guard authentication + session migration| Q | A| ------------- | ---| Branch? | 2.8| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no (but there needs to be on master)| Tests pass? | yes| Fixed tickets |#27577| License | MIT| Doc PR | n/aI messed up#27452 :/. Guard is the one class where the session migration is not on the listener, it's on the handler. The tricky part is that there is only ONE handler (unlike listeners where there is 1 listener per firewall). That means that implementing a session migration strategy that avoids stateless firewalls was a bit more tricky: I could only think to inject a map into `GuardAuthenticationHandler`. On the bright side, this also fixes session migration (not happening) when people call the `authenticateUserAndHandleSuccess()` method directly.On master, we'll need to add a deprecation to make the 3rd argument of `authenticateWithToken()` required - it's optional now for BC. We may also need to re-order the constructor args.I DID test this in a real 2.8 project, to make sure that things were properly wired up. Apologies for not doing that for the other PR.Cheers!Commits-------2c0ac93 Fix bad method call with guard authentication + session migration
I messed up#27452 :/. Guard is the one class where the session migration is not on the listener, it's on the handler. The tricky part is that there is only ONE handler (unlike listeners where there is 1 listener per firewall). That means that implementing a session migration strategy that avoids stateless firewalls was a bit more tricky: I could only think to inject a map into
GuardAuthenticationHandler. On the bright side, this also fixes session migration (not happening) when people call theauthenticateUserAndHandleSuccess()method directly.On master, we'll need to add a deprecation to make the 3rd argument of
authenticateWithToken()required - it's optional now for BC. We may also need to re-order the constructor args.I DID test this in a real 2.8 project, to make sure that things were properly wired up. Apologies for not doing that for the other PR.
Cheers!