-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] Automatically register custom authenticator as entry_point (if supported) #39153
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
[Security] Automatically register custom authenticator as entry_point (if supported) #39153
Conversation
66eeb90
to
8f88fe6
Compare
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.
👍 For 5.2
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.
HUGE thanks Wouter for spending your time with this last-second change. My apologies for not catching it sooner. I do think it's important, as the error if you don't have any entrypoints (but you have some custom authenticators) is poor as is trying to debug why one entrypoint is being used (e.g. from form_login
) but your custom authenticator's start()
method is being ignored.
There is one case that the PR doesn't address yet:
security:
firewalls:
main:
http_basic: ~
custom_authenticator: App\Security\CustomAuthenticatorWithEntrypointInterface
What should happen in this case (where no specific entry_point
has been chosen)? Right now, the new compiler pass is written to just use http_basic
. I think it should throw an exception. I don't think this should be treated any differently than 2 custom_authenticators
that both have the entrypoint interface. To say it differently, we should collect the entire collection of "potential entry points" (from built-in authenticators and custom authenticators) and if that collection > 1, we throw an exception and tell the user which options they can choose from.
WDYT? I think it's not too difficult - I think we would need to:
A) Do not throw this Exception inside of SecurityExtension
- https://github.com/symfony/symfony/blob/5.x/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php#L606
B) In SecurityExtension
, we would probably return the entrypoint from -> configuredEntryPoint()
as a Reference to some fake service - e.g. security.FIREWALLNAME.entrypoint
(this is maybe important the entrypoint is also passed to the security config object - https://github.com/symfony/symfony/blob/5.x/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php#L501 ). Then, later in the compiler pass, we'll figure out the real entrypoint, and set an alias for that fake service that points to the real entrypoint.
C) Also in SecurityExtension, in addition to the 'security.'.$firewallName.'_custom_authenticators'
parameter, we would set another parameter that held a key->value of all entrypoints for that firewall - e.g. ['form_login' => 'SERVICE ID FOR THIS ENTRYPOINT', 'http_basic' => 'SERVICE ID FOR THIS ENTRYPONT']
.
D) Finally, this would give us everything we need in the compiler pass to get a full list of the entrypoints and, if there were more than 1, throw a really nice exception with all the possible keys :).
Let me know if you'd like to chat with this or need any helping pushing it through!
Thank you!
...ony/Bundle/SecurityBundle/DependencyInjection/Compiler/CustomAuthenticatorEntryPointPass.php
Outdated
Show resolved
Hide resolved
...ony/Bundle/SecurityBundle/DependencyInjection/Compiler/CustomAuthenticatorEntryPointPass.php
Outdated
Show resolved
Hide resolved
8f88fe6
to
a2cbf37
Compare
I've completely changed the implementation of this PR to simply the implementation suggested by @weaverryan. If you want entry point to be configured by default, you should now implement I'll add some tests for the new compiler pass tomorrow evening. |
...ny/Bundle/SecurityBundle/DependencyInjection/Security/Factory/EntryPointFactoryInterface.php
Show resolved
Hide resolved
src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginFactory.php
Show resolved
Hide resolved
src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Authenticator/FormLoginAuthenticator.php
Show resolved
Hide resolved
b3d71a7
to
54ea6a9
Compare
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 just tested this extensively - it works perfectly. The internals to get this working are a bit complex, but I'm super happy with the end result: if more than 1 authentication mechanism supplies an "entry point", then you get a clear error instructed you to choose one.
src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/RegisterEntryPointPass.php
Outdated
Show resolved
Hide resolved
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.
Some tests are failing
@wouterj Can you have a look at the failing tests and apply fabbot's patch which seems relevant? |
54ea6a9
to
43543ca
Compare
Fixed. The remaining failing test seems to be due to outdated dependencies for some reason. If I apply the patches in this PR to |
Thank you @wouterj. |
43543ca
to
cab0672
Compare
This PR was merged into the 7.x-dev branch. Discussion ---------- Fix compatibility with SecurityBundle 5.2 The `EntryPointFactoryInterface` interface has been removed in 5.2 (symfony/symfony#39153) from SY'mfony's changelog > * [BC break] Removed `EntryPointFactoryInterface`, authenticators must now implement `AuthenticationEntryPointInterface` if they require autoregistration of a Security entry point. The ConnectAuthenticator already implements the `AuthenticationEntryPointInterface` interface, so I think this patch should be enough, but I'm not sure. @wouterj could you please have a look? Commits ------- 858861e Fix compatibility with SecurityBundle 5.2
@weaverryan came up with exactly the same issue as proposed by a contributor before (see referenced ticket). Back then, it was decided impossible to fix: see #37075. However, after some thinking we came up with a way to fix the issue and improve the DX for custom authenticators.
The new authenticators are no longer required to implement
AuthenticationEntryPointInterface
(required for internal authenticators like theRememberMeAuthenticator
and pre authenticated ones). This PR uses a compiler pass to check if a custom authenticator is supported:I know this is very late for Symfony 5.2. It would be good to decide whether this can be included in the release, in order to smooth out the biggest struggle for people using custom authenticators for the first time.