Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] added userChecker to SimpleAuthenticationProvider#26370
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| private$userChecker; | ||
| publicfunction__construct(SimpleAuthenticatorInterface$simpleAuthenticator,UserProviderInterface$userProvider,$providerKey) | ||
| publicfunction__construct(SimpleAuthenticatorInterface$simpleAuthenticator,UserProviderInterface$userProvider,$providerKey,UserCheckerInterface$userChecker) |
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.
must be default null for BC
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.
@chalasr Should it useSymfony\Component\Security\Core\User\UserChecker as default if null is present?
This would leave the method as is (without if statements)?
Added UserChecker to SimpleAuthenticationProvider and updated SimpleFormFactory
| { | ||
| $user =$this->userProvider->loadUserByUsername($token->getUsername()); | ||
| $this->userChecker->checkPreAuth($user); | ||
| $authToken =$this->simpleAuthenticator->authenticateToken($token,$this->userProvider,$this->providerKey); |
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.
While I understand the need to have the user here, it would be loaded twice now
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.
@iltar maybe then skip the pre auth check since it can be done during authentication of the token.
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.
What if you do$this->userChecker->checkPreAuth($authToken->getUser())? I know it's not exactly correct flow wise, but it beats fetching the user twice 🤔
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.
@iltar I was thinking about it but it sounds strange to me to pre authenticate authenticated token.
Changin interface to exclude the userProvider would cause BC breaks.
You are correct about twice loading the user and simple authenticator could do the pre-check if needed.
Maybe best is to remove thecheckPreAuth
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.
While it might seem strange, guard does it before/after the credentials check:
symfony/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php
Lines 124 to 128 ina8dc953
| $this->userChecker->checkPreAuth($user); | |
| if (true !==$guardAuthenticator->checkCredentials($token->getCredentials(),$user)) { | |
| thrownewBadCredentialsException(sprintf('Authentication failed because %s::checkCredentials() did not return true.',get_class($guardAuthenticator))); | |
| } | |
| $this->userChecker->checkPostAuth($user); |
I don't see another option to be honest, leaving it out would be as not having it at all.
retrieve user from authToken, rather then fetching it from userProvider
| { | ||
| $authToken =$this->simpleAuthenticator->authenticateToken($token,$this->userProvider,$this->providerKey); | ||
| $this->userChecker->checkPreAuth($authToken->getUser()); | ||
| $this->userChecker->checkPostAuth($authToken->getUser()); |
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.
should be moved after the instanceof check below
apply userChecker after authToken is authenticated
| } | ||
| thrownewAuthenticationException('Simple authenticator failed to return an authenticated token.'); | ||
| $this->userChecker->checkPreAuth($authToken->getUser()); |
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.
Do we need to call$authToken->getUser() twice ?
Simperfit commentedMar 4, 2018
Can we add a test for this class ? |
test for userChecker postAuth and preAuth
chalasr commentedMar 6, 2018
For 2.7 |
nicolas-grekas commentedMar 19, 2018
Thank you@i3or1s. |
…er (i3or1s)This PR was submitted for the 2.8 branch but it was squashed and merged into the 2.7 branch instead (closes#26370).Discussion----------[Security] added userChecker to SimpleAuthenticationProvider[Security] added userChecker to SimpleAuthenticationProvider[SecurityBundle] [DependencyInjection] updated SimpleFormFactory| Q | A| ------------- | ---| Branch? | 2.8| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#26314| License | MIT| Doc PR | noIntroduces user checker to the simple authentication provider.Commits-------cb9c92d [Security] added userChecker to SimpleAuthenticationProvider
Arthur-min commentedApr 3, 2018
Hello, I have an issue with this update:
Any idea how to fix that? Thanks! |
nicolas-grekas commentedApr 3, 2018
@Arthur-min see#26762 (please don't comment on closed issues/PRs) |
[Security] added userChecker to SimpleAuthenticationProvider
[SecurityBundle] [DependencyInjection] updated SimpleFormFactory
Introduces user checker to the simple authentication provider.