8000 New Guard Authentication System (e.g. putting the joy back into security) by weaverryan · Pull Request #14673 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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.

Sign up for GitHub

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

Merged
merged 24 commits into from
Sep 24, 2015
Merged

New Guard Authentication System (e.g. putting the joy back into security) #14673

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
Tweaks thanks to Wouter
  • Loading branch information
@weaverryan
weaverryan committed Sep 20, 2015
commit 396a1622dc69b53c552659a4839e4f22834976a8
7 changes: 2 additions & 5 deletions src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,24 +114,21 @@ private function authenticateViaGuard(GuardAuthenticatorInterface $guardAuthenti

if (!$user instanceof UserInterface) {
throw new \UnexpectedValueException(sprintf(
'The %s::getUser method must return a UserInterface. You returned %s.',
'The %s::getUser() method must return a UserInterface. You returned %s.',
get_class($guardAuthenticator),
is_object($user) ? get_class($user) : gettype($user)
));
}

// check the preAuth UserChecker
$this->userChecker->checkPreAuth($user);
// check the credentials
$guardAuthenticator->checkCredentials($token->getCredentials(), $user);
// check the postAuth UserChecker
$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
$authenticatedToken = $guardAuthenticator->createAuthenticatedToken($user, $this->providerKey);
if (!$authenticatedToken instanceof TokenInterface) {
throw new \UnexpectedValueException(sprintf(
'The %s::createAuthenticatedToken method must return a TokenInterface. You returned %s.',
'The %s::createAuthenticatedToken() method must return a TokenInterface. You returned %s.',
get_class($guardAuthenticator),
is_object($authenticatedToken) ? get_class($authenticatedToken) : gettype($authenticatedToken)
));
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Security/Guard/Token/PreAuthenticationGuardToken.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,6 @@ public function getCredentials()

public function setAuthenticated($authenticated)
{
throw new \LogicException('The PreAuthenticationGuardToken is *always* not authenticated.');
throw new \LogicException('The PreAuthenticationGuardToken is *never* authenticated.');
}
}
0