symfony / ux

Symfony UX initiative: a JavaScript ecosystem for Symfony
https://ux.symfony.com/
MIT License
843 stars 308 forks source link

[Twig Component] Test helper traits don't play nicely with CSRF, sessions and authentication #1124

Open benr77 opened 1 year ago

benr77 commented 1 year ago

Thank you @kbond for the awesome test helper traits for Live and Twig components.

However, in some of my new tests that use these traits, I'm running into some issues. All this is PHP 8.2 on Symfony 6.3, with Live/Twig components on the latest 2.11.2 version.

All these tests extend Symfony's KernelTestCase via my own KernelTestCase wrapper, where I add a setUp method to run before all tests cases:

abstract class KernelTestCase extends \Symfony\Bundle\FrameworkBundle\Test\KernelTestCase
{
    /** @var TestContainer */
    public Container|ContainerInterface $container;

    protected function setUp(): void
    {
        self::bootKernel();
        $this->container = static::getContainer();
    }
}

I think this is quite a normal configuration.

However, then with some tests I'm running into the following issues:

1) Session not found

Any Twig or Live Component that touches the session, either in PHP or in the template, will throw SessionNotFoundException: There is currently no session available. when the test is run.

For me this is usually because I have e.g. a Date Range filter stored in the session, and this is then used inside the component to filter e.g. a repository query.

The solution to this is to pass in the value from the session when placing the component in the parent template. i.e.

{{ component('report_income_by_client', { dateRange: app.session.get('dateRange') }) }}

This is actually more elegant in a way, as it keeps the concerns of the component itself to a bare minimum.

Then, in the test, we just pass in the "dateRange" as a parameter that we control from inside the test:

$rendered = $this->renderTwigComponent(
    name: ReportIncomeByClientComponent::class,
    data: [
        'dateRange' => $dateRange, // define this elsewhere in the test
    ],
);

So this is a working solution that I'm more posting here for future reference than anything else. However, it leads nicely on to the second problem:

2) CSRF token not found

If any of the components under test are rending a form, then I get failures with the following exception:

Symfony\Component\HttpFoundation\Exception\SessionNotFoundException: There is currently no session available.

/var/www/app/vendor/symfony/http-foundation/RequestStack.php:107
/var/www/app/vendor/symfony/security-csrf/TokenStorage/SessionTokenStorage.php:110
/var/www/app/vendor/symfony/security-csrf/TokenStorage/SessionTokenStorage.php:74
/var/www/app/vendor/symfony/security-csrf/CsrfTokenManager.php:69
/var/www/app/vendor/symfony/form/Extension/Csrf/Type/FormTypeCsrfExtension.php:82
/var/www/app/vendor/symfony/form/ResolvedFormType.php:134
/var/www/app/vendor/symfony/form/Extension/DataCollector/Proxy/ResolvedTypeDataCollectorProxy.php:95
/var/www/app/vendor/symfony/form/ResolvedFormType.php:128
/var/www/app/vendor/symfony/form/Extension/DataCollector/Proxy/ResolvedTypeDataCollectorProxy.php:95
/var/www/app/vendor/symfony/form/Form.php:908
/var/www/app/vendor/symfony/ux-live-component/src/ComponentWithFormTrait.php:120
/var/www/app/vendor/symfony/ux-live-component/src/ComponentWithFormTrait.php:95
/var/www/app/vendor/symfony/ux-twig-component/src/ComponentFactory.php:198
/var/www/app/vendor/symfony/ux-twig-component/src/ComponentFactory.php:91
/var/www/app/vendor/symfony/ux-twig-component/src/ComponentFactory.php:65
/var/www/app/vendor/symfony/ux-live-component/src/Test/TestLiveComponent.php:45
/var/www/app/vendor/symfony/ux-live-component/src/Test/InteractsWithLiveComponents.php:36
/var/www/app/tests/integration/ReportIncomeByClientComponentTest.php:22

The way I have sorted this is to disable CSRF protection across the entire "test" environment, but this is not great as I do want it enabled for WebTestCase but not for KernelTestCase etc.

// config/packages/framework.php

if ($container->env() === 'test')
{
    $framework->test(true);
    $framework->csrfProtection()->enabled(false);
}

Is there a sensible way to disable the CSRF protection only inside KernelTestCase tests? Or indeed just to create a CSRF token that is accepted?

3) Authenticated Users seem to be ignored

The final issue I'm seeing is that these tests seem to ignore any authenticated users I have. In my extended KernelTestCase base class, I have something like:

public function loginAs(string $username): void
{
    $user = $this->container->get(UserRepository::class)->findByUsername($username);
    $tokenStorage = $this->container->get(TokenStorageInterface::class);
    $token = new TestBrowserToken(roles: [Role::SUPER_ADMIN, Role::USER], user: $user);
    $tokenStorage->setToken($token);
}

This works fine in normal tests but seems to be ignored completely during the Twig/Live Component tests, and I just get Symfony\Component\Security\Core\Exception\AccessDeniedException : Access Denied. on any component that has an IsGranted attribute defined.

Again, a solution to this is to disable access control across the entire "test" environment, but I don't want to do this as the security restrictions also need to be part of the tested behaviour.

Sorry for the long post - but I figure that all these things are going to be a common issue when testing components, and I have a feeling they are all related together somehow.

Thanks!

kbond commented 1 year ago

These all look session related. My initial guess is the container created in setUp isn't the same one used as the trait... I've run into problems keeping the container as state in tests.

I think the problem would be solved by #1116 - this allows you to pass your own test client to the test helper.

benr77 commented 1 year ago

All my test failures that are related to CSRF are LiveComponents so I'm guessing that PR #1116 would do the trick.

I'll test property tomorrow but thanks for the heads-up!

benr77 commented 1 year ago

OK I've upgraded to the latest commit to include the PR, and injecting my own authenticated client using the new client parameter has sorted the access denied issues. So that's great thanks.

Now an authenticated client is being supplied, all remaining failing tests are CSRF related (issue number 2 in my first post above) - because there is a form in the component's template.

My other live component tests that do not use the Symfony form component appear to be just fine.

I'm not that familiar with the Symfony test client or the CSRF component, but should the framework not automatically create both the session, and the CSRF token on demand?

kbond commented 1 year ago

Do any of your other tests using the test client on a non-live csrf-enabled form work as expected? I have tests for vanilla forms using csrf and it just works. Further, the test helper handles csrf for the live component.

Maybe we can add a test to duplicate the issue?

benr77 commented 1 year ago

Do you mean KernelTestCase tests that test a form outside of a live component?

I can see the test helper dealing with the CSRF token in the trait, however the exception I'm seeing occurs on line 46 in the constructor of TestLiveComponent - which is before any handling of CSRF occurs in the test helper (which looks like is only when a request is submitted???).

My issue is when initially rendering the component, it can't find a session (when I think it is trying to add the CSRF token to the initial form rendering on load).

kbond commented 1 year ago

Do you mean KernelTestCase tests that test a form outside of a live component?

Yes.

My issue is when initially rendering the component, it can't find a session

Hmm ok, when looking at the other form fixtures for this library's tests, we explicitly disable csrf protection: https://github.com/symfony/ux/blob/6e6c903109f19d98e9929a8fef79ffae47483a7b/src/LiveComponent/tests/Fixtures/Form/FormWithManyDifferentFieldsType.php#L80

@weaverryan, is csrf on forms not supported? I don't see anything in the docs about this but we disable in our tests.

weaverryan commented 1 year ago

CSRF on forms IS supported. I think we have them disabled in tests for simplicity. But yea, I just triple-checked, and it works fine. Nothing special happens really:

A) The component instantiates your Symfony form B) When it renders, it naturally has a _token field on it. That becomes a value in the formValues prop C) This value never changes, but it IS send back and forth with the requests. So when the form is submitted, _token is there and everything works like a normal controller.

We should install symfony/security-csrf on ux.symfony.com to "show this off". Right now, it's not installed, so no CSRF there. But when I installed it locally, everything worked as expected.

kbond commented 1 year ago

Ok, so this is clearly a bug then. @benr77 could you create a reproducer that we can diagnose?

benr77 commented 1 year ago

You want a failing test case in the Symfony UX repo?

kbond commented 1 year ago

That would be ideal but a public repo that shows the problem would be fine too.

benr77 commented 1 year ago

Here you go: https://github.com/benr77/symfony-ux-component-tests-session-issue

There are commits for each version of the problem:

1) in TwigComponents, any session access fails the test, whereas you'd expect the session to be present or automatically started in a KernelTestCase.

2) in LiveComponents, I'm pretty sure it's the same issue but here we see it when the CSRF component tries to look in the session, which is not existing.

If you toggle the CSRF setting in framework.yaml you can see it fixing the LiveComponent test.

Cheers!

kbond commented 1 year ago

Thanks for the great reproducer! Sorry for the delay in looking into this.

  1. TwigComponent: this is expected based on the way this feature is implemented, we simply render the twig template using the Twig\Environment service (there is no browser client being used so no request/session is available). It's more of an integration test helper than a functional test helper. I don't know that this should be considered a bug.
  2. LiveComponent: the problem here is the component is instantiated before the test client makes a request. I'm looking into a solution for this.
kbond commented 1 year ago

Ok I've found a solution that gets your reproducer's tests passing:

Prefix the failing tests with the following code:

$request = new Request();
$request->setSession($this->createMock(SessionInterface::class));
self::getContainer()->get('request_stack')->push($request);

Let me know if this works in your larger project.

benr77 commented 1 year ago

The fix with the mock of SessionInterface works well - thank you. It fixes both the general session requests and also the CSRF lookups.

I'd been trying to do something similar but without success.

Do you think this is something that should be incorporated directly in to the Live/Twig Component test helpers?

Thanks again!

kbond commented 1 year ago

Do you think this is something that should be incorporated directly in to the Live/Twig Component test helpers?

I'm not sure... I don't like the idea of blindly pushing mock requests/sessions onto the request stack service. Especially for twig components, these aren't necessarily rendered in the browser.

Let's keep this issue open and gather some more feedback from users using these test helpers. When digging into the code, I realized that logging in a user with the client before initiating the test component might not work as expected.

daFish commented 9 months ago

Chiming in because I have the same problem. However, my live component does not use a form but an action. This triggers the CSRF error and is not fixed by the mock of SessionInterface.

Any hints?

kbond commented 8 months ago

I think #1460 will make it easier to track down, replicate and fix this issue.

carsonbot commented 2 months ago

Thank you for this issue. There has not been a lot of activity here for a while. Has this been resolved?

carsonbot commented 2 months ago

Could I get a reply or should I close this?

benr77 commented 2 months ago

Please leave this open for now.

smnandre commented 2 months ago

@benr77 Did #1460 help ?

benr77 commented 2 months ago

@smnandre To be honest I'm not sure. It's been ages since I was working on Live Component tests. I will hopefully be able to check this out in the next few weeks when I get back to some UI work.