8000 [Security] Require entry_point to be configured with multiple authenticators by wouterj · Pull Request #36575 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 30, 2020
Merged

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

merged 1 commit into from
Apr 30, 2020

Conversation

wouterj
Copy link
Member
@wouterj wouterj commented Apr 24, 2020 •
edited
Loading
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

8000
@wouterj wouterj force-pushed the security/entry-point branch from 8c5fbdd to 6179d5d Compare April 25, 2020 08:52
@wouterj wouterj force-pushed the security/entry-point branch from 6179d5d to 7a63e6a Compare April 25, 2020 09:16
@nicolas-grekas nicolas-grekas added this to the next milestone Apr 26, 2020
@wouterj wouterj force-pushed the security/entry-point branch 2 times, most recently from 98b949b to cc876a2 Compare April 27, 2020 12:10
@wouterj wouterj force-pushed the security/entry-point branch 3 times, most recently from 7169af4 to 83c9c3a Compare April 30, 2020 12:24
@wouterj
Copy link
Member Author
wouterj commented Apr 30, 2020

Both test failures appear to be unrelated

fabpot
fabpot approved these changes Apr 30, 2020
View reviewed changes
@wouterj wouterj force-pushed the security/entry-point branch from 83c9c3a to ee5654c Compare April 30, 2020 13:09
@fabpot fabpot force-pushed the security/entry-point branch from ee5654c to 7e86169 Compare April 30, 2020 13:22
@fabpot
Copy link
Member
fabpot commented Apr 30, 2020

Thank you @wouterj.

@fabpot fabpot mentioned this pull request Apr 30, 2020
Closed
@fabpot fabpot merged commit a114f8d into symfony:master Apr 30, 2020
@fabpot
Copy link
Member
fabpot commented May 1, 2020

@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 added a commit to wouterj/symfony that referenced this pull request May 1, 2020
@wouterj
Fixed symfony#36575
6978471
@wouterj
Copy link
Member Author
wouterj commented May 1, 2020

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.

@wouterj wouterj deleted the security/entry-point branch May 1, 2020 07:52
fabpot added a commit that referenced this pull request May 1, 2020
@fabpot
bug #36650 [Security] Fix bug introduced in entry_point configuration (…
c30d6f9 
…#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
@wouterj wouterj mentioned this pull request May 2, 2020
Merged
fabpot
fabpot reviewed May 2, 2020
View reviewed changes
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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author
@wouterj wouterj May 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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 added a commit that referenced this pull request May 3, 2020
@fabpot
bug #36661 [SecurityBundle] Fixed entry point service ID resolving an…
28bb74c 
…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
symfony-splitter pushed a commit to symfony/security-bundle that referenced this pull request May 3, 2020
@fabpot
bug #36661 [SecurityBundle] Fixed entry point service ID resolving an…
e0fa597 
…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
vjandrea pushed a commit to vjandrea/symfony that referenced this pull request May 3, 2020
@wouterj @vjandrea
Fixed symfony#36575
6f41c2d
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@weaverryan weaverryan weaverryan left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@chalasr chalasr chalasr left review comments

Assignees
No one assigned
Labels
Security Status: Reviewed
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.
6 participants
@wouterj @fabpot @weaverryan @nicolas-grekas @chalasr @carsonbot
0