[Security] added userChecker to SimpleAuthenticationProvider #26370
Conversation
|
|
||
| public function __construct(SimpleAuthenticatorInterface $simpleAuthenticator, UserProviderInterface $userProvider, $providerKey) | ||
| public function __construct(SimpleAuthenticatorInterface $simpleAuthenticator, UserProviderInterface $userProvider, $providerKey, UserCheckerInterface $userChecker) |
There was a problem hiding this comment.
@chalasr Should it use Symfony\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
| } | ||
|
|
||
| public function authenticate(TokenInterface $token) | ||
| { | ||
| $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.
While I understand the need to have the user here, it would be loaded twice now
There was a problem hiding this comment.
@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.
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.
@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 the checkPreAuth
There was a problem hiding this comment.
While it might seem strange, guard does it before/after the credentials check:
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
| } | ||
|
|
||
| public function authenticate(TokenInterface $token) | ||
| { | ||
| $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.
should be moved after the instanceof check below
apply userChecker after authToken is authenticated
| } | ||
|
|
||
| throw new AuthenticationException('Simple authenticator failed to return an authenticated token.'); | ||
| $this->userChecker->checkPreAuth($authToken->getUser()); |
There was a problem hiding this comment.
Do we need to call $authToken->getUser() twice ?
|
Can we add a test for this class ? |
test for userChecker postAuth and preAuth
|
For 2.7 |
|
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 | no Introduces user checker to the simple authentication provider. Commits ------- cb9c92d [Security] added userChecker to SimpleAuthenticationProvider
|
Hello, I have an issue with this update:
Any idea how to fix that? Thanks! |
|
@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.