[Security] Lazily load the user during the check passport event #37846

wouterj · 2020-08-16T10:06:52Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	yes
Tickets	Fix #37436
License	MIT
Doc PR	tbd

Before

class ApiKeyAuthenticator extends AbstractAuthenticator
{
    // ...

    public function authenticate(Request $request): PassportInterface
    {
        $email = $request->headers->get('X-USER-EMAIL');
        if (false === strpos($email, '@')) {
            throw new BadCredentialsException('Email is not a valid email address.');
        }

        $user = $this->userRepository->findOneBy(['email' => $email]);
        if (null === $user) {
            throw new UsernameNotFoundException();
        }

        return new SelfValidatingPassport($user);
    }
}

After

class ApiKeyAuthenticator extends AbstractAuthenticator
{
    // ...

    public function authenticate(Request $request): PassportInterface
    {
        $email = $request->headers->get('X-USER-EMAIL');
        if (false === strpos($email, '@')) {
            throw new BadCredentialsException('Email is not a valid email address.');
        }

        // a global ChainUserProvider (or firewall provider if explicitly configured) will be
        // used to load the User with $email as username
        return new SelfValidatingPassport($email);

        // or a custom closure to load the user
        return new SelfValidatingPassport(new UserBadge($email, function ($username) {
            return $this->userRepository->findOneBy(['email' => $username]);
        });
    }
}

Doing it this way has a couple advantages (some of which are already mentioned in the issue):

Some listeners on CheckPassportEvent need to execute before loading the user - to reduce resources (e.g. CSRF protection, if CSRF fails, no DB call should be made to load user - and also login throttling);
Some listeners require knowing the username of the login action (e.g. login throttling on IP and username);
The UserProviderListener allows to remove yet another centralized action in the authentication process from the authenticator class to the Symfony framework.

Automatic User Provider integration

Instead of passing the credentials and a closure to UserBadge, you can also just pass a (string) username. The user provider will then be used to load the user. This only works for custom_authenticators as of this moment.

By default, a chain user provider with all configured user_providers will be used as the user provider;
However, if you explicitly configure a provider for that firewall, that provider will be used (using a listener with higher priority).

fabpot · 2020-08-16T14:33:22Z

I like this very much.

wouterj · 2020-08-17T20:04:42Z

Thank you @fabpot 😃

This PR is ready for a review now, I've added some functional tests to make sure things are working.

There are 2 future considerations here:

Should we also rely on the UserProviderListener in the core authenticators? (removing their provider option and dependency on UserProviderInterface)?

User providers are also often used to dynamically add the PasswordUpgradeBadge. Should we move this logic to the UserProviderListener instead?

symfony/src/Symfony/Component/Security/Http/Authenticator/FormLoginAuthenticator.php

Lines 93 to 95 in c4ff074

    
           if ($this->userProvider instanceof PasswordUpgraderInterface) { 
        
               $passport->addBadge(new PasswordUpgradeBadge($credentials['password'], $this->userProvider)); 
        
           }

fabpot · 2020-08-18T05:53:25Z

I think you 2 additional changes you mentioned are worth investigating.

fabpot

I've made a small suggestion, not sure if that makes sense. What do you think about it?

src/Symfony/Component/Security/Http/Authenticator/Passport/Passport.php

weaverryan · 2020-08-20T19:13:42Z

src/Symfony/Component/Security/Http/Authenticator/Passport/Badge/UserBadge.php

+    private $userLoader;
+    private $user;
+
+    public function __construct(?string $username, ?callable $userLoader = null)


We should probably document both of these, because:

We need to communicate that $username is not necessarily a username... but just some "identifier". We probably need to communicate its purpose. For example, suppose I have a login form that has an email field AND a company drop-down. So, when I query the database for the user, the query is something like:

SELECT * FROM user WHERE email = :email AND company_id = :companyId

Obviously, I would probably make the query and load the user by passing a 2nd argument to new UserBadge() with a callback that makes the custom query.

The question is: what value should I pass as the $username? Should it just be $email? Probably not. Should it be ['email' => $email, 'companyId' => $companyId]? Well, right now, the first argument must be a string... and probably (?) we want to keep it that way. So, should the user pass $email'._'.$companyId... because the purpose of this argument is to be some "unique string identifier" of the user?

We should document that the 2nd arg is optional, must return UserInterface or null and will be passed the $username.

maybe $username should be renamed.

I've added some PHPdocs to this method (a bit too much to my liking, happy to accept more brief suggestions).

I've also renamed $username to $userIdentifier, please let me known if that doesn't make sense :)

src/Symfony/Component/Security/Http/Authenticator/Passport/Badge/UserBadge.php

weaverryan · 2020-08-20T19:17:22Z

src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator.php

+            ->args([
+                service('security.user_providers'),
+            ])
+            ->tag('kernel.event_listener', ['event' => CheckPassportEvent::class, 'priority' => 1024, 'method' => 'checkPassport'])


Is this first service used? I may just be missing it

Yes, the idea is:

Always have this service, which chains all configured user providers and runs at 1024

If a firewall has provider set, register the same listener for that specific firewall with only the configured firewall at priority 2048

The listener does not configure the user provider if a user loader is already set (e.g. by the authenticator using a closure or by the firewall specific listener).

src/Symfony/Component/Security/Guard/Authenticator/GuardBridgeAuthenticator.php

src/Symfony/Component/Security/Http/Authentication/AuthenticatorManager.php

weaverryan · 2020-08-20T19:23:42Z

src/Symfony/Component/Security/Http/EventListener/UserProviderListener.php

+            return;
+        }
+
+        $badge->setUserLoader([$this->userProvider, 'loadUserByUsername']);


That's a nice implementation

chalasr

Nice one, 1 minor comment. Should we add a CHANGELOG entry?

src/Symfony/Component/Security/Http/Authenticator/Passport/SelfValidatingPassport.php

src/Symfony/Component/Security/Http/Authenticator/Passport/Passport.php

fabpot · 2020-08-27T14:29:12Z

Thank you @wouterj.

@jrushlow

…ith the new UserBadge (wouterj) This PR was merged into the 5.2 branch. Discussion ---------- [Security] Update password upgrader listener to work with the new UserBadge | Q | A | ------------- | --- | Branch? | 5.2 | Bug fix? | yes | New feature? | yes | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - While working on a new amazing `make:auth` maker, @jrushlow discovered that we forgot to update the `PasswordUpgradeBadge` with the 5.2 `UserBadge` changes (ref #37846). This PR fixes it, by making the password upgrader optional and falling back to the user provider instead. Without these changes, each authenticator still needs to know the user repository/user provider in order to pass it to `PasswordUpgradeBadge`. I'm sorry for catching this soo late in the release cycle. There is a BC break involved here, but it's (a) very unlikely to impact application code and (b) in an experimental class. Commits ------- e39e844 Default to user provider, if available, in password upgrader

…rInterface implementation to Passport (wouterj) This PR was merged into the 5.3-dev branch. Discussion ---------- [Security] Remove deprecated support for passing a UserInterface implementation to Passport | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - In #37846 (review) , we agreed to have a deprecation path of only one minor release as the `Passport` feature is still experimental. Commits ------- 99cf2a3 [Security] Disallow passing a UserInterface to Passport

carsonbot added Status: Needs Review Security Feature Deprecation labels Aug 16, 2020

wouterj force-pushed the issue-37436/user-in-checkpassword-event branch 4 times, most recently from 4099e8f to 8220186 Compare August 17, 2020 20:01

wouterj marked this pull request as ready for review August 17, 2020 20:04

fabpot approved these changes Aug 18, 2020

View reviewed changes

src/Symfony/Component/Security/Http/Authenticator/Passport/Passport.php Outdated Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Aug 18, 2020

weaverryan reviewed Aug 20, 2020

View reviewed changes

wouterj force-pushed the issue-37436/user-in-checkpassword-event branch 4 times, most recently from 5154f9b to 42c1a7e Compare August 22, 2020 15:56

fabpot approved these changes Aug 23, 2020

View reviewed changes

chalasr reviewed Aug 23, 2020

View reviewed changes

src/Symfony/Component/Security/Http/Authenticator/Passport/SelfValidatingPassport.php Outdated Show resolved Hide resolved

fabpot added this to the next milestone Aug 23, 2020

chalasr reviewed Aug 23, 2020

View reviewed changes

src/Symfony/Component/Security/Http/Authenticator/Passport/Passport.php Outdated Show resolved Hide resolved

wouterj force-pushed the issue-37436/user-in-checkpassword-event branch from 42c1a7e to e61adff Compare August 26, 2020 08:22

fabpot approved these changes Aug 27, 2020

View reviewed changes

Lazily load the user during the check passport event

907ef31

fabpot force-pushed the issue-37436/user-in-checkpassword-event branch from e61adff to 907ef31 Compare August 27, 2020 14:29

fabpot mentioned this pull request Aug 27, 2020

[Security] Lazily load the user during the check passport event symfony/symfony-docs#14138

Closed

fabpot merged commit 2248639 into symfony:master Aug 27, 2020

wouterj deleted the issue-37436/user-in-checkpassword-event branch August 27, 2020 19:20

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

fabpot mentioned this pull request Oct 5, 2020

Release v5.2.0-BETA1 #38417

Merged

This was referenced Nov 14, 2020

keeping aliens at bay with maker + new security 5.2 features symfony/maker-bundle#736

Merged

[Security] Update password upgrader listener to work with the new UserBadge #39111

Merged

duboiss mentioned this pull request Dec 7, 2020

[Security] Update experimental_authenticators : add UserBadge symfony/symfony-docs#14672

Closed

wouterj mentioned this pull request Mar 16, 2021

[Security] Remove deprecated support for passing a UserInterface implementation to Passport #40487

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Lazily load the user during the check passport event #37846

[Security] Lazily load the user during the check passport event #37846

wouterj commented Aug 16, 2020 •

edited

Loading

fabpot commented Aug 16, 2020

wouterj commented Aug 17, 2020 •

edited

Loading

fabpot commented Aug 18, 2020

fabpot left a comment

weaverryan Aug 20, 2020

stof Aug 21, 2020

wouterj Aug 22, 2020 •

edited

Loading

weaverryan Aug 20, 2020

wouterj Aug 22, 2020

weaverryan Aug 20, 2020

chalasr left a comment

fabpot commented Aug 27, 2020

[Security] Lazily load the user during the check passport event #37846

[Security] Lazily load the user during the check passport event #37846

Conversation

wouterj commented Aug 16, 2020 • edited Loading

Automatic User Provider integration

fabpot commented Aug 16, 2020

wouterj commented Aug 17, 2020 • edited Loading

fabpot commented Aug 18, 2020

fabpot left a comment

Choose a reason for hiding this comment

weaverryan Aug 20, 2020

Choose a reason for hiding this comment

stof Aug 21, 2020

Choose a reason for hiding this comment

wouterj Aug 22, 2020 • edited Loading

Choose a reason for hiding this comment

weaverryan Aug 20, 2020

Choose a reason for hiding this comment

wouterj Aug 22, 2020

Choose a reason for hiding this comment

weaverryan Aug 20, 2020

Choose a reason for hiding this comment

chalasr left a comment

Choose a reason for hiding this comment

fabpot commented Aug 27, 2020

wouterj commented Aug 16, 2020 •

edited

Loading

wouterj commented Aug 17, 2020 •

edited

Loading

wouterj Aug 22, 2020 •

edited

Loading