-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
New Guard Authentication System (e.g. putting the joy back into security) #14673
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
Changes from 1 commit
330aa7f
05af97c
a0bceb4
873ed28
180e2c7
6c180c7
c73c32e
eb158cb
d693721
6edb9e1
ffdbc66
7de05be
7a94994
81432f9
293c8a1
0501761
31f9cae
c9d9430
396a162
302235e
dd485f4
e353833
d763134
a01ed35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
…teps This looks like a subjective change (one more method, but the method implementations are simpler), but it wasn't. The problem was that the UserChecker checkPreAuth should happen *after* we get the user, but *before* the credentials are checked, and that wasn't possible before this change. Now it is.
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
|
||
use Symfony\Component\Security\Core\Authentication\Provider\AuthenticationProviderInterface; | ||
use Symfony\Component\Security\Core\Authentication\Token\AnonymousToken; | ||
use Symfony\Component\Security\Core\Exception\UsernameNotFoundException; | ||
use Symfony\Component\Security\Guard\GuardAuthenticatorInterface; | ||
use Symfony\Component\Security\Guard\Token\GuardTokenInterface; | ||
use Symfony\Component\Security\Guard\Token\PreAuthenticationGuardToken; | ||
|
@@ -95,18 +96,28 @@ public function authenticate(TokenInterface $token) | |
private function authenticateViaGuard(GuardAuthenticatorInterface $guardAuthenticator, PreAuthenticationGuardToken $token) | ||
{ | ||
// get the user from the GuardAuthenticator | ||
$user = $guardAuthenticator->authenticate($token->getCredentials(), $this->userProvider); | ||
$user = $guardAuthenticator->getUser($token->getCredentials(), $this->userProvider); | ||
|
||
if (null === $user) { | ||
throw new UsernameNotFoundException(sprintf( | ||
'Null returned from %s::getUser()', | ||
get_class($guardAuthenticator) | ||
)); | ||
} | ||
|
||
if (!$user instanceof UserInterface) { | ||
throw new \UnexpectedValueException(sprintf( | ||
'The %s::authenticate method must return a UserInterface. You returned %s.', | ||
'The %s::getUser method must return a UserInterface. You returned %s.', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to be consistent, use |
||
get_class($guardAuthenticator), | ||
is_object($user) ? get_class($user) : gettype($user) | ||
)); | ||
} | ||
|
||
// check the AdvancedUserInterface methods! | ||
// check the preAuth UserChecker | ||
$this->userChecker->checkPreAuth($user); | ||
// check the credentials | ||
$guardAuthenticator->checkCredentials($token->getCredentials(), $user); | ||
// check the postAuth UserChecker | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think these comments explain anything more (as it's just |
||
$this->userChecker->checkPostAuth($user); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the I have a small concern regarding the user checker though. Say that before querying anything to find a user, I want to check if the IP has a time out, how would I do it here? In the old situation I would have created my own simple-authenticator which did this. After this PR, I'd have to put it in a
Regarding those points, after an x amount of failed attempts, I have to show a captcha which has to be displayed until either valid or another x attempts have failed. This bundle would add the 'captcha' type to a form which you can then re-use for both the validation and display. I want to do this before the authentication takes place (why query the database for a blocked IP). https://github.com/Gregwar/CaptchaBundle Just a random brainfart There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A lot here :)
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding point 2, It will be similar. In my case I've actually implement a custom UserChecker, which I manually had to inject (can't configure that at all). I've made a custom method called I was wondering if it would be possible to tackle that problem while we are at it. public function checkPreUserLoad()
{
$request = $this->request_stack->getCurrentRequest();
$resolved = $this->login_ip_resolver->resolveStatus($request->getClientIp());
if ($resolved->requiresBlock()) {
throw new BlockThresholdExceededException('Too many failed login attempts, ip blocked');
}
if (!$resolved->requiresCaptcha()) {
return;
}
// captcha is required
$form = $this->form_factory->create('login', null, ['add_captcha' => true]);
$form->handleRequest($request);
// only check the captcha, there is currently no username/password validation
if ($request->request->has('login') && $form->has('captcha')) {
if ($form->isSubmitted() && !$form->get('captcha')->isValid()) {
throw new InvalidCaptchaException('Provided captcha was not valid');
}
// captcha is valid, we can skip it
return;
}
throw new CaptchaThresholdExceededException('Too many failed login attempts, captcha required');
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would the user checker stuff in #11090 solve this? If not, what do you propose? This looks like a lot of business logic to me, and putting that either into the user checker or somewhere in the authenticator (or the authenticator calls out to another service which holds the logic) seems right and simple to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the solution I proposed in #11090 will in fact solve this issue. That is Backwards compatible, just need to modify the configuration for this factory. Eventually I can use a service decorator to replace the |
||
|
||
// turn the UserInterface into a TokenInterface | ||
|
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.
It makes be nervous that this method, by default, results in a user being successfully authenticated and requires throwing an exception in the case that the user should NOT be authenticated. In the case that a developer accidentally writes a bug that doesn't throw an exception in a complex (or any) setup, the mistake results in _users being authenticated that shouldn't have been._
I think a safer way to do this would be to explicitly return a value (perhaps a boolean) indicating whether the credential check was successful or not. In the
GuardAuthenticationProvider
you could very easily check this boolean value for falseness (in the case that someone doesn't return) and then throw anAuthenticationException
to keep the rest of the logic in the Provider the same.TLDR; Which is worse? Authenticating a user who has invalid credentials? Or not authenticating a user who has valid credentials? I guess I'd rather not take a chance with security. Thoughts?
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.
👍
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.
Not sure I agree - the same could be said for returning
false
. For example, if you collect the result in a boolean variable$valid
which is returned, that variable could easily have the wrong truth value simply by mistaking&&
for||
somewhere in your code.The only way to prevent bugs like the one you described is by properly testing the authenticator. I don't think that throwing an exception vs. returning
false
makes any difference here.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.
see #16395
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.
Heh, maybe I should look at the tip of 2.8 before I go commenting on old PRs. Sorry everyone.
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.
@patrick-mcdougle No, thank you! I was trying to find you at the conference - I made a pull request within an hour after your asked this very good question - and wanted to tell you about it :).