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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bad method call with guard authentication + session migration #27581

Closed
wants to merge 4 commits into
base: 2.8
from

Conversation

Projects
None yet
4 participants
@weaverryan
Member

weaverryan commented Jun 11, 2018

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/a

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 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!

Fixing guard authentication + session migration
The original setter was put onto the wrong class. The handler is a bit
more difficult, as there is one handler only. So, we need to pass in
a statelessFirewalls array so we know whether or not to migrate the
session
}
/**
* Authenticates the given token in the system.
*/
public function authenticateWithToken(TokenInterface $token, Request $request)
public function authenticateWithToken(TokenInterface $token, Request $request, $providerKey = null)

This comment has been minimized.

@chalasr

chalasr Jun 11, 2018

Member

even optional, this would break a child (signature mismatch). func_get_arg() should be used here :)

@weaverryan

This comment has been minimized.

Member

weaverryan commented Jun 12, 2018

Thanks @chalasr! Made the change

@nicolas-grekas

With minor cs comments.

public function __construct(TokenStorageInterface $tokenStorage, EventDispatcherInterface $eventDispatcher = null)
/**
* @param TokenStorageInterface $tokenStorage
* @param EventDispatcherInterface|null $eventDispatcher

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 12, 2018

Member

We're ok with partial docblocks, so these 2 above lines can be removed.

}
/**
* Authenticates the given token in the system.
*/
public function authenticateWithToken(TokenInterface $token, Request $request)
{
$this->migrateSession($request, $token);
$providerKey = func_num_args() > 2 ? func_get_arg(2) : null;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 12, 2018

Member

\func_num_args()
The new argument should be added on the signature between /**/, and on the docblock.

{
if (!$this->sessionStrategy || !$request->hasSession() || !$request->hasPreviousSession()) {
if (!$this->sessionStrategy || !$request->hasSession() || !$request->hasPreviousSession() || in_array($providerKey, $this->statelessProviderKeys)) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 12, 2018

Member

\in_array(..., true)

@weaverryan

This comment has been minimized.

Member

weaverryan commented Jun 12, 2018

Let's try this :) - not 100% sure I got that format for the documented new arg right 😇

*/
public function authenticateWithToken(TokenInterface $token, Request $request)
public function authenticateWithToken(TokenInterface $token, Request $request /* $providerKey */)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 12, 2018

Member

(TokenInterface $token, Request $request/*, string $providerKey */)

@chalasr

This comment has been minimized.

Member

chalasr commented Jun 12, 2018

Thank you @weaverryan.

chalasr added a commit that referenced this pull request Jun 12, 2018

bug #27581 Fix bad method call with guard authentication + session mi…
…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/a

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 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

@chalasr chalasr closed this Jun 12, 2018

@weaverryan weaverryan deleted the weaverryan:fix-guard-auth-session-migration branch Jun 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment