Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
330aa7f
Improving phpdoc on AuthenticationEntryPointInterface so people that …
weaverryan May 17, 2015
05af97c
Initial commit (but after some polished work) of the new Guard authen…
weaverryan May 17, 2015
a0bceb4
adding Guard tests
weaverryan May 17, 2015
873ed28
Renaming the tokens to be clear they are "post" and "pre" auth - also…
weaverryan May 17, 2015
180e2c7
Properly handles "post auth" tokens that have become not authenticated
weaverryan May 17, 2015
6c180c7
Adding an edge case - this should not happen anyways
weaverryan May 17, 2015
c73c32e
Thanks fabbot!
weaverryan May 17, 2015
eb158cb
Updating interface method per suggestion - makes sense to me, Request…
weaverryan May 18, 2015
d693721
Adding periods at the end of exceptions, and changing one class name …
weaverryan May 18, 2015
6edb9e1
Tweaking docblock on interface thanks to @iltar
weaverryan May 18, 2015
ffdbc66
Splitting the getting of the user and checking credentials into two s…
weaverryan May 18, 2015
7de05be
A few more changes thanks to @iltar
weaverryan May 18, 2015
7a94994
Thanks again fabbot!
weaverryan May 18, 2015
81432f9
Adding missing factory registration
weaverryan May 25, 2015
293c8a1
meaningless author and license changes
weaverryan Sep 20, 2015
0501761
Allowing for other authenticators to be checked
weaverryan Sep 20, 2015
31f9cae
Adding a base class to assist with form login authentication
weaverryan Sep 20, 2015
c9d9430
Adding logging on this step and switching the order - not for any hu…
weaverryan Sep 20, 2015
396a162
Tweaks thanks to Wouter
weaverryan Sep 20, 2015
302235e
Fixing a bug where having an authentication failure would log you out.
weaverryan Sep 21, 2015
dd485f4
Adding a new exception and throwing it when the User changes
weaverryan Sep 21, 2015
e353833
fabbot
weaverryan Sep 21, 2015
d763134
Removing unnecessary override
weaverryan Sep 22, 2015
a01ed35
Adding the necessary files so that Guard can be its own installable c…
weaverryan Sep 24, 2015
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Splitting the getting of the user and checking credentials into two s…
…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
weaverryan committed Sep 20, 2015
commit ffdbc665347c935be76c537021a86fbfa77e658b
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ interface GuardAuthenticatorInterface extends AuthenticationEntryPointInterface
* as any type (e.g. an associate array). If you return null, authentication
* will be skipped.
*
* Whatever value you return here will be passed to authenticate()
* Whatever value you return here will be passed to getUser() and checkCredentials()
*
* For example, for a form login, you might:
*
Expand All @@ -47,19 +47,33 @@ interface GuardAuthenticatorInterface extends AuthenticationEntryPointInterface
public function getCredentials(Request $request);

/**
* Return a UserInterface object based on the credentials OR throw
* an AuthenticationException.
* Return a UserInterface object based on the credentials
*
* The *credentials* are the return value from getCredentials()
*
* You may throw an AuthenticationException if you wish. If you return
* null, then a UsernameNotFoundException is thrown for you.
*
* @param mixed $credentials
* @param UserProviderInterface $userProvider
*
* @throws AuthenticationException
*
* @return UserInterface
* @return UserInterface|null
*/
public function getUser($credentials, UserProviderInterface $userProvider);

/**
* Throw an AuthenticationException if the credentials are invalid
*
* The *credentials* are the return value from getCredentials()
*
* @param mixed $credentials
* @param UserInterface $user
* @throws AuthenticationException
* @return void
*/
public function authenticate($credentials, UserProviderInterface $userProvider);
public function checkCredentials($credentials, UserInterface $user);
Copy link
Contributor

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 an AuthenticationException 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #16395

Copy link
Contributor

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.

Copy link
Member Author

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 :).


/**
* Create an authenticated token for the given user.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be consistent, use ::getUser()

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
Copy link
Member

Choose a reason for hiding this comment

The 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 humanize(method_name) property_name).

$this->userChecker->checkPostAuth($user);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the checkPostAuth() be done after createAuthenticatedToken()?

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 GuardAuthenticatorInterface. Neither of those solutions seems feasible for me as I would have to create either a weird inheritance tree or duplicate code in my authenticators. It would be great for you (and others ofc!) to think with me on the following points:

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
I can also imagine the possibility to automate certain validation points by using a FormType with constraints; With a data transformer to put the user in there automatically via a user provider based on the username.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot here :)

  1. Actually, checkPostAuth() is right, but checkPreAuth() was wrong - it should happen right after fetching the User, but before checking credentials (see UserAuthenticationProvider). I've just split a method on the interface to make this possible and moved the checkPreAuth() in the provider. Thanks for asking about this - I hadn't really realized I had things in the wrong spot.

  2. About the IP timeout idea, was this simpler with the simple-authenticator? Or are you saying that in both cases, you'd need to use inheritance or duplication to put the code into the "simple authenticator" or the "guard authenticator", so it's the same, but no better? I just commented on symfony/security - tagging UserCheckerInterface #11090 - I think it can be solved there.

  3. We should avoid using forms or validation anywhere. But more importantly, the "guard authenticators" are so simple that you can do whatever you want, including using forms and validation.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 checkPreUserLoad() in a custom UserCheckerInterface implementation and created an abstract authenticator that all our different authentication classes (simple-form, simple-pre-auth) extend. In your implementation, I would probably put half of this inside the part where you get the values from the request and pass the captcha valid/invalid/not required from there.

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');
    }

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 security.user_checker and voila, everything magically works without needing to change this code.


// turn the UserInterface into a TokenInterface
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,21 +43,25 @@ public function testAuthenticate()
'username' => '_weaverryan_test_user',
'password' => 'guard_auth_ftw',
);
$this->preAuthenticationToken->expects($this->once())
$this->preAuthenticationToken->expects($this->atLeastOnce())
->method('getCredentials')
->will($this->returnValue($enteredCredentials));

// authenticators A and C are never called
$authenticatorA->expects($this->never())
->method('authenticate');
->method('getUser');
$authenticatorC->expects($this->never())
->method('authenticate');
->method('getUser');

$mockedUser = $this->getMock('Symfony\Component\Security\Core\User\UserInterface');
$authenticatorB->expects($this->once())
->method('authenticate')
->method('getUser')
->with($enteredCredentials, $this->userProvider)
->will($this->returnValue($mockedUser));
// checkCredentials is called
$authenticatorB->expects($this->once())
->method('checkCredentials')
->with($enteredCredentials, $mockedUser);
$authedToken = $this->getMock('Symfony\Component\Security\Core\Authentication\Token\TokenInterface');
$authenticatorB->expects($this->once())
->method('createAuthenticatedToken')
Expand Down