Skip to content
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

KernelBrowser::loginUser does not work when called multiple times #41590

Closed
guillaumesmo opened this issue Jun 7, 2021 · 6 comments
Closed

Comments

@guillaumesmo
Copy link
Contributor

Symfony version(s) affected: 5.3.1

Description
Since the introduction of storage_factory_id to replace storage_id, it's not possible to switch to another user in the tests any more

I have pushed a sample project which reproduces the issue here: https://github.com/guillaumesmo/symfony-login-user-issue

How to reproduce

in a controller:

    /**
     * @Route("/admin-page")
     * @Security("is_granted('ROLE_ADMIN')")
     */
    public function adminPage()
    {
		return new Response('OK');
    }

in a WebTestCase:

$client = self::createClient();

$client->loginUser(new InMemoryUser('admin', 'password', ['ROLE_ADMIN']));
$client->request('GET', '/admin-page');
self::assertResponseIsSuccessful();

$client->loginUser(new InMemoryUser('other', 'password', ['ROLE_OTHER']));
$client->request('GET', '/admin-page');
self::assertResponseStatusCodeSame(403);

run bin/phpunit:

There was 1 failure:

1) App\Tests\Controller\DefaultControllerTest::test
Failed asserting that the Response status code is 403.
HTTP/1.1 200 OK

Now in config/packages/framework.yaml, revert to the Symfony 5.2 default config:

  • remove the line storage_factory_id: session.storage.factory.native
  • change storage_factory_id: session.storage.factory.mock_file to storage_id: session.storage.mock_file

run bin/phpunit:

OK (1 test, 2 assertions)
@gndk
Copy link
Contributor

gndk commented Jun 9, 2021

I think an issue that I discovered in our tests is related to this.

We have two helper methods in our integration tests to check authentication, which we use around $client->loginUser() like this:

public function testSessionContainsSecurityToken(): void
{
    $client = self::createClient();

    self::assertUserIsNotAuthenticated();

    $client->loginUser(new InMemoryUser('admin', 'password', ['ROLE_ADMIN']));

    self::assertUserIsAuthenticated();
}

private static function session(): SessionInterface
{
    return static::getContainer()->get(SessionInterface::class);
}

protected static function assertUserIsNotAuthenticated(): void
{
    $token = self::session()->get('_security_main');

    self::assertNull($token, 'The user is authenticated');
}

protected static function assertUserIsAuthenticated(): void
{
    $token = self::session()->get('_security_main');

    self::assertIsString($token, 'The user is not authenticated');
}

In 5.2.9 both work fine.

In 5.3.1 with the new session configuration, assertUserIsAuthenticated() fails, because the token is not saved in the session.

Authentication still works, but its not possible to access the token. I guess thats why changing the logged in user is not possible either.

I added the tests in @guillaumesmo's reproducer here because I think it is related: guillaumesmo/symfony-login-user-issue#1, but I can also open a separate issue if you think it makes sense.

@chalasr
Copy link
Member

chalasr commented Jun 9, 2021

/cc @jderusse

@jderusse
Copy link
Member

jderusse commented Jun 9, 2021

fixed by #41505

@jderusse jderusse closed this as completed Jun 9, 2021
@guillaumesmo
Copy link
Contributor Author

Tested with symfony/framework-bundle (v5.3.0 => 5.3.x-dev 5548342):, works as expected!

thanks

@adrienfr
Copy link
Contributor

adrienfr commented Sep 29, 2021

@guillaumesmo could you try to check if it still works for you with SF 5.3.9? I have an issue with multiple loginUser() since the new release, see here.

@guillaumesmo
Copy link
Contributor Author

guillaumesmo commented Oct 4, 2021

@adrienfr I can confirm the issue is back, tested in 5.3.9, worked fine in 5.3.7

probably introduced by #42979

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

No branches or pull requests

6 participants