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
Conversation
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):
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) |
@wouterj, thank you for the advice about creating a new branch. |
@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')) { |
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.
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)
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.
You're right. I have done it.
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.
Thanks for your very quick responses!
Thank you @Ne-Lexa. |
…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
Fix TypeError. Expected instance
CsrfTokenManagerInterface
, butSessionTokenStorage
was given.