Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromwouterj:security/entry-point
Apr 30, 2020

Conversation

wouterj
Copy link
Member

@wouterjwouterj commentedApr 24, 2020•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?no
Deprecations?no
Tickets-
LicenseMIT
Doc PRtbd

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 fromform_login "win" overhttp_basic if I have both defined under my firewall?

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

Before (one authenticator)

security:enable_authenticator_manager:truefirewalls:main:form_login:...# form login is your entry point

After
Same as before

Before (multiple authenticators)

security:enable_authenticator_manager:truefirewalls:main:http_basic:...form_login:...# for some reason, FormLogin is now your entry point! (config order doesn't matter)

After

security:enable_authenticator_manager:truefirewalls:main:http_basic:...form_login:...entry_point:form_login

Before (custom entry point service)

security:enable_authenticator_manager:truefirewalls:main:http_basic:...form_login:...entry_point:App\Security\CustomEntryPoint

After
Same as before

@nicolas-grekasnicolas-grekas added this to thenext milestoneApr 26, 2020
@wouterjwouterjforce-pushed thesecurity/entry-point branch 2 times, most recently from98b949b tocc876a2CompareApril 27, 2020 12:10
@wouterjwouterjforce-pushed thesecurity/entry-point branch 3 times, most recently from7169af4 to83c9c3aCompareApril 30, 2020 12:24
@wouterj
Copy link
MemberAuthor

Both test failures appear to be unrelated

@fabpot
Copy link
Member

Thank you@wouterj.

@fabpot
Copy link
Member

@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 requestMay 1, 2020
@wouterj
Copy link
MemberAuthor

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.

@wouterjwouterj deleted the security/entry-point branchMay 1, 2020 07:52
fabpot added a commit that referenced this pull requestMay 1, 2020
…#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-------6978471Fixed#36575
@@ -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 extendingAbstractFactory. That means that we cannot markAbstractFactory as@internal.

Copy link
MemberAuthor

@wouterjwouterjMay 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 theEntryPointInterface 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 requestMay 3, 2020
…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 rename6870a18 Fixed entry point resolving and guard entry point configuration
symfony-splitter pushed a commit to symfony/security-bundle that referenced this pull requestMay 3, 2020
…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 rename6870a18803 Fixed entry point resolving and guard entry point configuration
vjandrea pushed a commit to vjandrea/symfony that referenced this pull requestMay 3, 2020
@nicolas-grekasnicolas-grekas modified the milestones:next,5.1May 4, 2020
@fabpotfabpot mentioned this pull requestMay 5, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@fabpotfabpotfabpot approved these changes

@weaverryanweaverryanweaverryan left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@chalasrchalasrchalasr left review comments

Assignees
No one assigned
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
[8]ページ先頭
©2009-2025 Movatter.jp