[Security] Automatically provide entry points from the custom_authenticator #37075

wouterj · 2020-06-02T15:18:11Z

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

This PR allows the custom authenticator factory to automatically configure the entry point or fail with an error if more than 1 entry point could be configured. This makes it behave the same as the guard factory.

security:
  firewalls:
    main:
      # 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,
      # no entry point is configured automatically
      custom_authenticator: App\Security\NoEntryPointAuthenticator

      # if more than one authenticator implements AuthenticationEntryPointInterface,
      # the entry point must be explicitly set
      custom_authenticators:
        services: [App\Security\CustomAuthenticator, App\Security\AnotherCustomAuthenticator]
        entry_point: App\Security\CustomAuthenticator

wouterj · 2020-06-02T16:25:32Z

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

+
+        $entryPoints = [];
+        foreach ($config['services'] as $authenticatorId) {
+            if (class_exists($authenticatorId) && is_subclass_of($authenticatorId, AuthenticationEntryPointInterface::class)) {


Sadly, we can't do $container->findDefinition($authenticatorId)->getClass() here, as the ContainerBuilder is scoped for this extension.

Is there some magic trick we can do here to also check for non-FQCN service IDs?

wouterj · 2020-06-03T12:52:56Z

Another suggestion - provided by @althaus:

What if the listener would add a info/warning to the log before converting it instantly into a HttpEx? Like "Hey there's no entry point set, so I make this an error now. You could set the.config.value to change my behaviour."

cc @weaverryan do you have an opinion on how to handle this? (you're often better in recognizing the difference between "too magic" and "DX-friendly" than I am)

weaverryan · 2020-06-11T13:14:07Z

My first impression was: yay! But... when I think further, I realize that eventually, you might have ugly config like this:

main:
    form_login: ~
    entry_point: form_login
    custom_authenticators:
        services: [App\Security\CustomAuthenticator, App\Security\AnotherCustomAuthenticator]
        entry_point: App\Security\CustomAuthenticator

In this case, the form_login entrypoint would win... which is fine, but it means that the entry_point config under custom_authenticators is 100% pointless 🙃 . I really don't like that.

So, I can think of 2 options:

On a missing entrypoint, we throw a better exception - basically the suggestion from @althaus. Though... instead of registering a listener to do this, the "authenticator" system could add a "dummy entrypoint" service to the system that would throw this error when called. The idea would be that if (A) you have at least one custom_authenticator then the dummy entrypoint is added and (B) if you have no other authentication mechanisms, then it would be used (and you would see the error).
Or... probably do (1), but also go a step further. If there is ONE custom_authenticator (that is an entrypoint), then we automatically register that as an entrypoint. If there are no other authentication mechanisms that provide an entrypoint, it will be used. Yay! As soon as there are TWO custom_authenticators (that have an entrypoint), then we register the fake entrypoint described in (1) and tell people to configure the top-level entry_point under the firewall. We can even tell them what their options are (i.e. list the 2/3/4 custom authenticator service ids).

So... probably 2 is best. In both cases, I'd like to avoid the entry_point that's below custom_authenticators as this is really a duplicate of the entry_point that's under the firewall (and this was my mistake from the original Guard implementation).

wouterj · 2020-06-11T14:19:00Z

@weaverryan 2 notes and that's why I think improving the log message is the best we can do:

It's perfectly fine to not have an entry point. This will result in an HTTP Unauthorized response, which is perfect for e.g. APIs
An authenticator does not have to implement AuthenticationEntryPointInterface. So we can't blindly register any authenticator configured using custom_authenticators

wouterj · 2020-09-17T08:08:16Z

Closing, see my previous comment on why I no longer believe this PR can ever work. I'll create another one that improves logging.

@weaverryan

… as entry_point (if supported) (wouterj) This PR was squashed before being merged into the 5.2 branch. Discussion ---------- [Security] Automatically register custom authenticator as entry_point (if supported) | 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: ```yaml 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. Commits ------- cab0672 [Security] Automatically register custom authenticator as entry_point (if supported)

carsonbot added Status: Needs Review Security Bug Feature labels Jun 2, 2020

Automatically provide entry points from the custom_authenticator

9e7959d

wouterj force-pushed the issue-37068/custom-authenticator-entrypoint branch from 28515bf to 9e7959d Compare June 2, 2020 16:19

wouterj commented Jun 2, 2020

View reviewed changes

wouterj mentioned this pull request Jun 2, 2020

[Security] New LoginFormAuthenticator fails on InsufficientAuthenticationException with missing AuthenticationEntryPoint #37068

Closed

nicolas-grekas added this to the 5.1 milestone Jun 4, 2020

wouterj closed this Sep 17, 2020

wouterj deleted the issue-37068/custom-authenticator-entrypoint branch September 17, 2020 08:08

wouterj mentioned this pull request Nov 23, 2020

[Security] Automatically register custom authenticator as entry_point (if supported) #39153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Automatically provide entry points from the custom_authenticator #37075

[Security] Automatically provide entry points from the custom_authenticator #37075

wouterj commented Jun 2, 2020

wouterj Jun 2, 2020 •

edited

Loading

wouterj commented Jun 3, 2020 •

edited

Loading

weaverryan commented Jun 11, 2020

wouterj commented Jun 11, 2020

wouterj commented Sep 17, 2020

[Security] Automatically provide entry points from the custom_authenticator #37075

[Security] Automatically provide entry points from the custom_authenticator #37075

Conversation

wouterj commented Jun 2, 2020

wouterj Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

wouterj commented Jun 3, 2020 • edited Loading

weaverryan commented Jun 11, 2020

wouterj commented Jun 11, 2020

wouterj commented Sep 17, 2020

wouterj Jun 2, 2020 •

edited

Loading

wouterj commented Jun 3, 2020 •

edited

Loading