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

Fix register csrf protection listener #37261

Merged
merged 1 commit into from Jun 13, 2020
Merged

Fix register csrf protection listener #37261

merged 1 commit into from Jun 13, 2020

Conversation

Ne-Lexa
Copy link
Contributor

@Ne-Lexa Ne-Lexa commented Jun 12, 2020

Q A
Branch? 5.1.1
Bug fix? yes
New feature? no
Deprecations? np
Tickets #37254
License MIT

Fix TypeError. Expected instance CsrfTokenManagerInterface, but SessionTokenStorage was given.

Uncaught Error: Argument 1 passed to Symfony\Component\Security\Http\EventListener\CsrfProtectionListener::__construct() must implement interface Symfony\Component\Security\Csrf\CsrfTokenManagerInterface, instance of Symfony\Component\Security\Csrf\TokenStorage\SessionTokenStorage given

Uncaught PHP Exception TypeError: "Argument 1 passed to Symfony\Component\Security\Http\EventListener\CsrfProtectionListener::__construct() must implement interface Symfony\Component\Security\Csrf\CsrfTokenManagerInterface, instance of Symfony\Component\Security\Csrf\TokenStorage\SessionTokenStorage given

@wouterj
Copy link
Member

wouterj commented Jun 13, 2020

That looks like the right fix, thanks for contributing this! However, can you please also tweak the if statements now (so they cover the correct service existence):

  • The if statement on line 30 should be removed and && !$container->has('security.csrf.token_storage') should be added to the if statement on line 52
  • The if statement on line 40 should add !$container->has('security.csrf.token_manager')

You can do so by adding a new commit to your branch (Ne-Lexa:5.1).

Quick tip for next time: it's often better to create a new branch specifically for this PR. That allows you to work on multiple PRs at the same time.

(for the maintainers: I'm working on significantly extending the functional testsuite of SecurityBundle for the new system, to avoid errors like this in the feature)

@Ne-Lexa
Copy link
Contributor Author

Ne-Lexa commented Jun 13, 2020

@wouterj, thank you for the advice about creating a new branch.
I've made the changes you've proposed.

@fabpot
Copy link
Member

fabpot commented Jun 13, 2020

@wouterj Can you check again?

$this->registerCsrfProtectionListener($container);
$this->registerLogoutHandler($container);
}

private function registerCsrfProtectionListener(ContainerBuilder $container)
{
if (!$container->has('security.authenticator.manager')) {
if (!$container->has('security.csrf.token_manager')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep the check for security.authenticator.manager here as well, because that's checking if the new system is used. If it isn't used, there is no need to register the listener service. (it won't be automatically removed IIRC, as it's registered as a listener)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I have done it.

Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your very quick responses!

@fabpot
Copy link
Member

fabpot commented Jun 13, 2020

Thank you @Ne-Lexa.

@fabpot fabpot merged commit 080eef0 into symfony:5.1 Jun 13, 2020
fabpot added a commit that referenced this pull request Jun 13, 2020
…tor system (wouterj)

This PR was merged into the 5.1 branch.

Discussion
----------

[SecurityBundle] Run functional tests for the authenticator system

| Q             | A
| ------------- | ---
| Branch?       | 5.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

<s>Includes #37261 until it's merged.</s>

This runs all relevant functional tests in the security bundle for both the traditional and the authenticator system. This will hopefully avoid breaking more code in further releases.

deps=high builds will be green once this has been merged up into master.

---

During the functional tests, some inconsistencies were fixed. Three tests revealed larger inconsistencies that couldn't be fixed easily. These are not run for the new system as of now, we need to investigate further how to proceed with them. I'll create a separate issue/discussion for these:

* `Symfony\Bundle\SecurityBundle\Tests\Functional\FirewallEntryPointTest::testItUsesTheConfiguredEntryPointWhenUsingUnknownCredentials`
* `Symfony\Bundle\SecurityBundle\Tests\Functional\CsrfFormLoginTest::testFormLoginWithInvalidCsrfToken`
* `Symfony\Bundle\SecurityBundle\Tests\Functional\SecurityRoutingIntegrationTest::testSecurityConfigurationForExpression`

Commits
-------

49639ca [Security] Run functional tests also for the authenticator system
@fabpot fabpot mentioned this pull request Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants