Skip to content

[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

Merged
merged 0 commits into from
Nov 27, 2020

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Nov 23, 2020

Q A
Branch? 5.2 (hopefully?)
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #37068
License MIT
Doc PR -

@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 the RememberMeAuthenticator and pre authenticated ones). This PR uses a compiler pass to check if a custom authenticator is supported:

security:
  firewalls:
    main:
      # in any case, if an entry_point is already configured it'll not be overriden
      # (http_basic remains the entry point here)
      http_basic: ~
      custom_authenticator: App\Security\CustomAuthenticator
      entry_point: http_basic

      # if only one custom authenticator implements AuthenticationEntryPointInterface,
      # it's automatically configured as the entry point
      custom_authenticator: App\Security\CustomAuthenticator
      custom_authenticators: [App\Security\CustomAuthenticator, App\Security\NoEntryPointAuthenticator]

      # if no custom authenticator implements AuthenticationEntryPointInterface,
      # an error is thrown
      custom_authenticator: App\Security\NoEntryPointAuthenticator

      # if more than one authenticator implements AuthenticationEntryPointInterface,
      # the entry point must be configured explicitly (or an error is thrown)
      custom_authenticators: [App\Security\CustomAuthenticator, App\Security\AnotherCustomAuthenticator]
      entry_point: App\Security\CustomAuthenticator

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.

@wouterj wouterj requested a review from chalasr as a code owner November 23, 2020 23:31
@carsonbot carsonbot added this to the 5.2 milestone Nov 23, 2020
@wouterj wouterj force-pushed the security/custom-authenticator-entry-point branch from 66eeb90 to 8f88fe6 Compare November 23, 2020 23:35
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

👍 For 5.2

Copy link
Member

@weaverryan weaverryan left a 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!

@wouterj
Copy link
Member Author

wouterj commented Nov 24, 2020

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 AuthenticationEntryPointInterface on the authenticator. This already used to be a requirement with Guard and also the core authenticators were almost all supporting this already. If only one entry point is found, it's registered automatically. If you need another class to be the entry point, you would need to configure entry_point explicitly.

I'll add some tests for the new compiler pass tomorrow evening.

@wouterj wouterj force-pushed the security/custom-authenticator-entry-point branch 2 times, most recently from b3d71a7 to 54ea6a9 Compare November 24, 2020 22:45
Copy link
Member

@weaverryan weaverryan left a 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.

Copy link
Member

@chalasr chalasr left a 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

@fabpot
Copy link
Member

fabpot commented Nov 27, 2020

@wouterj Can you have a look at the failing tests and apply fabbot's patch which seems relevant?

@wouterj wouterj force-pushed the security/custom-authenticator-entry-point branch from 54ea6a9 to 43543ca Compare November 27, 2020 09:18
@wouterj
Copy link
Member Author

wouterj commented Nov 27, 2020

@wouterj Can you have a look at the failing tests and apply fabbot's patch which seems relevant?

Fixed. The remaining failing test seems to be due to outdated dependencies for some reason. If I apply the patches in this PR to FormLoginAuthenticator and GuardBridgeAuthenticator in the vendor/ of SecurityBundle, the tests pass. However, I do require ^5.2 in the composer.json. So I think (hope) these will be fixed automatically after this PR is merged.

@fabpot
Copy link
Member

fabpot commented Nov 27, 2020

Thank you @wouterj.

@fabpot fabpot closed this Nov 27, 2020
@fabpot fabpot force-pushed the security/custom-authenticator-entry-point branch from 43543ca to cab0672 Compare November 27, 2020 10:25
@fabpot fabpot merged commit df7099c into symfony:5.2 Nov 27, 2020
@wouterj wouterj deleted the security/custom-authenticator-entry-point branch November 27, 2020 12:52
@fabpot fabpot mentioned this pull request Nov 30, 2020
fabpot added a commit to symfonycorp/connect that referenced this pull request Apr 8, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants