[Security] Require entry_point to be configured with multiple authenticators #36575

wouterj · 2020-04-24T21:30:08Z

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

See @weaverryan's comment at #33558 (comment):

I have it on my list to look at the entrypoint stuff more closely. But my gut reaction is this: let's fix them (or try to... or maybe in a PR after this) :). What I mean is this:

It's always been confusing that your firewall may have multiple auth mechanisms that have their own "entry point"... and one is chosen seemingly at random :). I know it's not random, but why does the entrypoint from form_login "win" over http_basic if I have both defined under my firewall?

Since we're moving to a new system, why not throw an exception the moment that a firewall has multiple entrypoints available to it. Then we force the user to choose the one entrypoint that should be used.

Before (one authenticator)

security:
  enable_authenticator_manager: true

  firewalls:
    main:
      form_login: ...

# form login is your entry point

After
Same as before

Before (multiple authenticators)

security:
  enable_authenticator_manager: true

  firewalls:
    main:
      http_basic: ...
      form_login: ...

# for some reason, FormLogin is now your entry point! (config order doesn't matter)

After

security:
  enable_authenticator_manager: true

  firewalls:
    main:
      http_basic: ...
      form_login: ...
      entry_point: form_login

Before (custom entry point service)

security:
  enable_authenticator_manager: true

  firewalls:
    main:
      http_basic: ...
      form_login: ...
      entry_point: App\Security\CustomEntryPoint

After
Same as before

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/HttpBasicFactory.php

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

...ny/Bundle/SecurityBundle/DependencyInjection/Security/Factory/GuardAuthenticationFactory.php

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginFactory.php

wouterj · 2020-04-30T12:46:58Z

Both test failures appear to be unrelated

src/Symfony/Bundle/SecurityBundle/CHANGELOG.md

…icators

fabpot · 2020-04-30T13:24:48Z

Thank you @wouterj.

fabpot · 2020-05-01T06:43:24Z

@wouterj I haven't had time to investigate further, but this PR breaks auth on my app. I get "Full authentication is required to access this resource." when trying to authenticate.

wouterj · 2020-05-01T07:51:58Z

Oh, I think #36650 should fix it. Sorry, I finished this patch on my Windows PC to get it ready before the weekend. I'll try to add some tests for the DI stuff in SecurityBundle in some weeks.

…#36575) (wouterj) This PR was merged into the 5.1-dev branch. Discussion ---------- [Security] Fix bug introduced in entry_point configuration (#36575) | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Commits ------- 6978471 Fixed #36575

fabpot · 2020-05-02T17:22:33Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php

@@ -65,7 +67,7 @@ public function create(ContainerBuilder $container, string $id, array $config, s
        }

        // create entry point if applicable (optional)
-        $entryPointId = $this->createEntryPoint($container, $id, $config, $defaultEntryPointId);
+        $entryPointId = $this->createDefaultEntryPoint($container, $id, $config, $defaultEntryPointId);


This is the BC break that makes my code fail now. AbstractFactory is what we use in Connect Bundle: https://github.com/symfonycorp/connect-bundle/blob/master/src/DependencyInjection/Security/Factory/ConnectFactory.php

/cc @wouterj

So, even if I can probably fix it in Connect, I fear that we won't be alone by this problem. Anyone not using Guard is probably extending AbstractFactory. That means that we cannot mark AbstractFactory as @internal.

Thanks for debugging!
That also means we have to find another method name for the EntryPointInterface interface, to not conflict with the abstract method in this class. ~~I'll do a PR later this evening.~~ See #36661

@fabpot

…d multiple guard entry points (wouterj) This PR was squashed before being merged into the 5.1-dev branch. Discussion ---------- [SecurityBundle] Fixed entry point service ID resolving and multiple guard entry points | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | n/a @fabpot I am not able to reproduce [the error you reported](#36575 (comment)) in any of my demo applications or in the tests introduced in this PR. The error indicates that no entry point is configured in your application, can you maybe try out this patch (given it now makes a hard error when more than one guard is used)? If it still doesn't work, can you maybe share your firewall configuration? --- _build failures are unrelated_ Commits ------- c756593 Do not make AbstractFactory internal and revert method rename 6870a18 Fixed entry point resolving and guard entry point configuration

@fabpot

…d multiple guard entry points (wouterj) This PR was squashed before being merged into the 5.1-dev branch. Discussion ---------- [SecurityBundle] Fixed entry point service ID resolving and multiple guard entry points | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | n/a @fabpot I am not able to reproduce [the error you reported](symfony/symfony#36575 (comment)) in any of my demo applications or in the tests introduced in this PR. The error indicates that no entry point is configured in your application, can you maybe try out this patch (given it now makes a hard error when more than one guard is used)? If it still doesn't work, can you maybe share your firewall configuration? --- _build failures are unrelated_ Commits ------- c75659350e Do not make AbstractFactory internal and revert method rename 6870a18803 Fixed entry point resolving and guard entry point configuration

carsonbot added Status: Needs Review Security labels Apr 24, 2020

fabpot requested changes Apr 25, 2020

View reviewed changes

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php Outdated Show resolved Hide resolved

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 25, 2020

wouterj force-pushed the security/entry-point branch from 8c5fbdd to 6179d5d Compare April 25, 2020 08:52

carsonbot added Status: Needs Review and removed Status: Needs Work labels Apr 25, 2020

fabpot requested changes Apr 25, 2020

View reviewed changes

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 25, 2020

wouterj force-pushed the security/entry-point branch from 6179d5d to 7a63e6a Compare April 25, 2020 09:16

carsonbot added Status: Needs Review and removed Status: Needs Work labels Apr 25, 2020

chalasr reviewed Apr 25, 2020

View reviewed changes

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/HttpBasicFactory.php Outdated Show resolved Hide resolved

nicolas-grekas added this to the next milestone Apr 26, 2020

weaverryan reviewed Apr 26, 2020

View reviewed changes

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php Outdated Show resolved Hide resolved

wouterj force-pushed the security/entry-point branch 2 times, most recently from 98b949b to cc876a2 Compare April 27, 2020 12:10

weaverryan reviewed Apr 28, 2020

View reviewed changes

...ny/Bundle/SecurityBundle/DependencyInjection/Security/Factory/GuardAuthenticationFactory.php Show resolved Hide resolved

wouterj commented Apr 28, 2020

View reviewed changes

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginFactory.php Outdated Show resolved Hide resolved

wouterj force-pushed the security/entry-point branch 3 times, most recently from 7169af4 to 83c9c3a Compare April 30, 2020 12:24

fabpot approved these changes Apr 30, 2020

View reviewed changes

src/Symfony/Bundle/SecurityBundle/CHANGELOG.md Outdated Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 30, 2020

wouterj force-pushed the security/entry-point branch from 83c9c3a to ee5654c Compare April 30, 2020 13:09

[Security] Require entry_point to be configured with multiple authent…

7e86169

…icators

fabpot force-pushed the security/entry-point branch from ee5654c to 7e86169 Compare April 30, 2020 13:22

fabpot mentioned this pull request Apr 30, 2020

[Security] Require entry_point to be configured with multiple authentic… symfony/symfony-docs#13605

Closed

fabpot merged commit a114f8d into symfony:master Apr 30, 2020

wouterj added a commit to wouterj/symfony that referenced this pull request May 1, 2020

Fixed symfony#36575

6978471

wouterj deleted the security/entry-point branch May 1, 2020 07:52

wouterj mentioned this pull request May 2, 2020

[SecurityBundle] Fixed entry point service ID resolving and multiple guard entry points #36661

Merged

fabpot reviewed May 2, 2020

View reviewed changes

vjandrea pushed a commit to vjandrea/symfony that referenced this pull request May 3, 2020

Fixed symfony#36575

6f41c2d

nicolas-grekas modified the milestones: next, 5.1 May 4, 2020

fabpot mentioned this pull request May 5, 2020

Release v5.1.0-BETA1 #36713

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Require entry_point to be configured with multiple authenticators #36575

[Security] Require entry_point to be configured with multiple authenticators #36575

wouterj commented Apr 24, 2020 •

edited

wouterj commented Apr 30, 2020

fabpot commented Apr 30, 2020

fabpot commented May 1, 2020

wouterj commented May 1, 2020

fabpot May 2, 2020

wouterj May 2, 2020 •

edited

[Security] Require entry_point to be configured with multiple authenticators #36575

[Security] Require entry_point to be configured with multiple authenticators #36575

Conversation

wouterj commented Apr 24, 2020 • edited

wouterj commented Apr 30, 2020

fabpot commented Apr 30, 2020

fabpot commented May 1, 2020

wouterj commented May 1, 2020

fabpot May 2, 2020

Choose a reason for hiding this comment

wouterj May 2, 2020 • edited

Choose a reason for hiding this comment

wouterj commented Apr 24, 2020 •

edited

wouterj May 2, 2020 •

edited