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

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

Merged
merged 24 commits into from Sep 24, 2015

Conversation

Projects
None yet
@weaverryan
Member

weaverryan commented May 17, 2015

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets at least partially: #14300, #11158, #11451, #10035, #10463, #8606, probably more
License MIT
Doc PR symfony/symfony-docs#5265

Hi guys!

Though it got much easier in 2.4 with pre_auth, authentication is a pain in Symfony. This introduces a new authentication provider called guard, with one goal in mind: put everything you need for any authentication system into one spot.

How it works

With guard, you can perform custom authentication just by implementing the GuardAuthenticatorInterface and registering it as a service. It has methods for every part of a custom authentication flow I can think of.

For a working example, see https://github.com/weaverryan/symfony-demo/tree/guard-auth. This uses 2 authenticators simultaneously, creating a system that handles form login and api token auth with a respectable amount of code. The security.yml is also quite simple.

This also supports "manual login" without jumping through hoops: https://github.com/weaverryan/symfony-demo/blob/guard-auth/src/AppBundle/Controller/SecurityController.php#L45

I've also tested with "remember me" and "switch user" - no problems with either.

I hope you like it :).

What's Needed

  1. Other Use-Cases?: Please think about the code and try it. What use-cases are we not covering? I want Guard to be simple, but cover the 99.9% use-cases.

  2. Remember me functionality cannot be triggered via manual login. That's true now, and it's not fixed, and it's tricky.

Deprecations?

This is a new feature, so no deprecations. But, creating a login form with a guard authenticator is a whole heck of a lot easier to understand than form_login or even simple_form. In a perfect world, we'd either deprecate those or make them use "guard" internally so that we have just one way of performing authentication.

Thanks!

@jakzal jakzal added the Security label May 18, 2015

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar May 18, 2015

Contributor

I really love this PR! 👍 It makes things a lot simpler. As you can see I have already worked my way through the changes to comment. It's really nice to see the difference between the pre and post authentication tokens.

Edit: also fixes #14300

Contributor

iltar commented May 18, 2015

I really love this PR! 👍 It makes things a lot simpler. As you can see I have already worked my way through the changes to comment. It's really nice to see the difference between the pre and post authentication tokens.

Edit: also fixes #14300

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz May 18, 2015

Member

👍 congrats @weaverryan! It's a very nice move towards reducing perceived complexity of the Security component. I'd like to make two quick comments regarding the naming and code of some methods:

1) Are you planning to add new methods to get credentials from different sources than the Request object? If not, maybe the ...FromRequest part of the method name is redundant:

// redundant method name?
public function getCredentialsFromRequest(Request $request) { ... }

// acceptable alternative?
public function getCredentials(Request $request) { ... }

2) The autoLogin() code looks very convoluted to me. In an ideal world, I'd like to do the following:

$security->login($user);

I know that we can't do that for Symfony, but could we simplify that code as much as possible?

Member

javiereguiluz commented May 18, 2015

👍 congrats @weaverryan! It's a very nice move towards reducing perceived complexity of the Security component. I'd like to make two quick comments regarding the naming and code of some methods:

1) Are you planning to add new methods to get credentials from different sources than the Request object? If not, maybe the ...FromRequest part of the method name is redundant:

// redundant method name?
public function getCredentialsFromRequest(Request $request) { ... }

// acceptable alternative?
public function getCredentials(Request $request) { ... }

2) The autoLogin() code looks very convoluted to me. In an ideal world, I'd like to do the following:

$security->login($user);

I know that we can't do that for Symfony, but could we simplify that code as much as possible?

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan May 18, 2015

Member

@javiereguiluz

  • Good idea on checkCredentials() - I've changed that.
  • For autoLogin, I hear you. The system is already a big improvement over before, where you had not access to your normal "success listener Response" nor an automatic way to dispatch the "interactive login" event, which now happens. What we're missing to make it easier is "user-land" access to which firewall we are currently under. I'm not aware of any way to access this currently, which is why we have that ugly secured_area thing in there.

Thanks!

Member

weaverryan commented May 18, 2015

@javiereguiluz

  • Good idea on checkCredentials() - I've changed that.
  • For autoLogin, I hear you. The system is already a big improvement over before, where you had not access to your normal "success listener Response" nor an automatic way to dispatch the "interactive login" event, which now happens. What we're missing to make it easier is "user-land" access to which firewall we are currently under. I'm not aware of any way to access this currently, which is why we have that ugly secured_area thing in there.

Thanks!

* @param GuardAuthenticatorInterface[] $guardAuthenticators The authenticators, with keys that match what's passed to GuardAuthenticationProvider
* @param LoggerInterface $logger A LoggerInterface instance
*/
public function __construct(GuardAuthenticatorHandler $guardHandler, AuthenticationManagerInterface $authenticationManager, $providerKey, array $guardAuthenticators, LoggerInterface $logger = null)

This comment has been minimized.

@ogizanagi

ogizanagi May 18, 2015

Member

Is there anything preventing you from using a NullLogger instance if not provided ? In order to avoid such things:

if (null !== $this->logger) {
    ...
}
@ogizanagi

ogizanagi May 18, 2015

Member

Is there anything preventing you from using a NullLogger instance if not provided ? In order to avoid such things:

if (null !== $this->logger) {
    ...
}

This comment has been minimized.

@weaverryan

weaverryan May 18, 2015

Member

Nothing technical preventing that, but NullLogger isn't used anywhere in core currently - having an optional argument like this is used :)

@weaverryan

weaverryan May 18, 2015

Member

Nothing technical preventing that, but NullLogger isn't used anywhere in core currently - having an optional argument like this is used :)

This comment has been minimized.

@iltar

iltar May 18, 2015

Contributor

What I think he means is the following:

$this->logger = $logger ?: new NullLogger();

IMO this is a cleaner solution as you don't have to worry about if($this->logger) statements. You can just log away safely.

@iltar

iltar May 18, 2015

Contributor

What I think he means is the following:

$this->logger = $logger ?: new NullLogger();

IMO this is a cleaner solution as you don't have to worry about if($this->logger) statements. You can just log away safely.

This comment has been minimized.

@ogizanagi
@ogizanagi

ogizanagi May 18, 2015

Member

See #14682

This comment has been minimized.

@dupuchba

dupuchba Jul 18, 2015

Contributor

agree with @iltar it does simplify a little bit IMO

@dupuchba

dupuchba Jul 18, 2015

Contributor

agree with @iltar it does simplify a little bit IMO

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 24, 2015

Member

👎 for a NullLogger, Symfony should take options for most performance.

@nicolas-grekas

nicolas-grekas Aug 24, 2015

Member

👎 for a NullLogger, Symfony should take options for most performance.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi May 18, 2015

Member

Wow ! This looks great ! 👍

Member

ogizanagi commented May 18, 2015

Wow ! This looks great ! 👍

* @param string $providerKey The provider (i.e. firewall) key
* @param UserCheckerInterface $userChecker
*/
public function __construct(array $guardAuthenticators, UserProviderInterface $userProvider, $providerKey, UserCheckerInterface $userChecker)

This comment has been minimized.

@wouterj

wouterj Jun 29, 2015

Member

99 out of 100 times, the user checker passed through here is the default UserChecker, right? What do you think about making this final argument optional and defaulting to the default user checker? E.g. $this->userChecker = $userChecker ?: new UserChecker();

@wouterj

wouterj Jun 29, 2015

Member

99 out of 100 times, the user checker passed through here is the default UserChecker, right? What do you think about making this final argument optional and defaulting to the default user checker? E.g. $this->userChecker = $userChecker ?: new UserChecker();

This comment has been minimized.

@weaverryan

weaverryan Sep 20, 2015

Member

I don't know if I like adding a new UserChecker automatically, but I think I would be for making the argument optional (and just checking for the UserChecker before calling it below). I know your thought is for making these classes somewhat usable directly.

@weaverryan

weaverryan Sep 20, 2015

Member

I don't know if I like adding a new UserChecker automatically, but I think I would be for making the argument optional (and just checking for the UserChecker before calling it below). I know your thought is for making these classes somewhat usable directly.

// this MUST be the same as GuardAuthenticationProvider
$uniqueGuardKey = $this->providerKey.'_'.$key;
$this->executeGuardAuthenticator($uniqueGuardKey, $guardAuthenticator, $event);

This comment has been minimized.

@Taluu

Taluu Jul 18, 2015

Contributor

instead of injecting a uniqueGuardKey, how about having a getKey method on the guard authenticator, and use that key with the providerKey in the executeGuardAuthenticator body ? Granted, this is preventing from having the same authenticator registered several times, and this should be good too, nope ?

@Taluu

Taluu Jul 18, 2015

Contributor

instead of injecting a uniqueGuardKey, how about having a getKey method on the guard authenticator, and use that key with the providerKey in the executeGuardAuthenticator body ? Granted, this is preventing from having the same authenticator registered several times, and this should be good too, nope ?

This comment has been minimized.

@weaverryan

weaverryan Sep 20, 2015

Member

Yes, it would remove this hack-ish looking code, but it's a meaningless key that the user shouldn't need to bother with. And you could have a single authenticator that is able to be configured (via constructor params) in different ways, so that it does make sense to have 2 at once. Anyways, we'll see if others agree with you.

@weaverryan

weaverryan Sep 20, 2015

Member

Yes, it would remove this hack-ish looking code, but it's a meaningless key that the user shouldn't need to bother with. And you could have a single authenticator that is able to be configured (via constructor params) in different ways, so that it does make sense to have 2 at once. Anyways, we'll see if others agree with you.

This comment has been minimized.

@lyrixx

lyrixx Sep 26, 2015

Member

(It's a bit late I know, But I lacked time to review it)

IIUC the code, the last guardAuthenticator have an higher precedence than other one.
I say that because all guardAuthenticators are execute one after an other, and all are able to
set a new response in the event and because you never check if a response it set.

That mean if the first guardAuthenticator allow an user, the second one could reject the user.
So finally, the user will be rejected. I'm not sure that is what end developer wants.

@lyrixx

lyrixx Sep 26, 2015

Member

(It's a bit late I know, But I lacked time to review it)

IIUC the code, the last guardAuthenticator have an higher precedence than other one.
I say that because all guardAuthenticators are execute one after an other, and all are able to
set a new response in the event and because you never check if a response it set.

That mean if the first guardAuthenticator allow an user, the second one could reject the user.
So finally, the user will be rejected. I'm not sure that is what end developer wants.

This comment has been minimized.

@weaverryan

weaverryan Sep 26, 2015

Member

Ah, very interesting - and yes, you do understand the code well.

With normal security, if one listener fails, then the other ones aren't called, because $event->setResponse() is called with the entry point. But on success, it depends: some listeners like UsernamePasswordFormAuthenticationListener set a response on the event, so nobody else would be called. But others - like BasicAuthenticationListener don't set a response. So on success, another authentication listener could be called, which could fail and reject the user (just like you're saying). In other words, this is already the behavior of some core authentication listeners.

See #15925 - I've updated to return on the Event having a response, which should make it fully mimic core. At least with guard, you can control the "priority" of the authenticators, in the edge case that you care.

@weaverryan

weaverryan Sep 26, 2015

Member

Ah, very interesting - and yes, you do understand the code well.

With normal security, if one listener fails, then the other ones aren't called, because $event->setResponse() is called with the entry point. But on success, it depends: some listeners like UsernamePasswordFormAuthenticationListener set a response on the event, so nobody else would be called. But others - like BasicAuthenticationListener don't set a response. So on success, another authentication listener could be called, which could fail and reject the user (just like you're saying). In other words, this is already the behavior of some core authentication listeners.

See #15925 - I've updated to return on the Event having a response, which should make it fully mimic core. At least with guard, you can control the "priority" of the authenticators, in the edge case that you care.

@OskarStark

This comment has been minimized.

Show comment
Hide comment
@OskarStark

OskarStark Jul 19, 2015

Contributor

great work 👍

Contributor

OskarStark commented Jul 19, 2015

great work 👍

@ad3n

This comment has been minimized.

Show comment
Hide comment
@ad3n

ad3n Jul 20, 2015

very nice 👍

ad3n commented Jul 20, 2015

very nice 👍

@malas

This comment has been minimized.

Show comment
Hide comment
@malas

malas Jul 23, 2015

I waited for this so long! Hope it gets into core ASAP

malas commented Jul 23, 2015

I waited for this so long! Hope it gets into core ASAP

@giovkanata

This comment has been minimized.

Show comment
Hide comment
@giovkanata

giovkanata Jul 23, 2015

Well done! This was one of the most lacked features in symfony until now.

giovkanata commented Jul 23, 2015

Well done! This was one of the most lacked features in symfony until now.

@nielvrom

This comment has been minimized.

Show comment
Hide comment
@nielvrom

nielvrom Jul 24, 2015

Very nice! In the beginning of learning Symfony the authentication was not easy for me! This makes it so much easier for new users!

nielvrom commented Jul 24, 2015

Very nice! In the beginning of learning Symfony the authentication was not easy for me! This makes it so much easier for new users!

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 24, 2015

Member

Thank you @weaverryan.

Member

fabpot commented Sep 24, 2015

Thank you @weaverryan.

@fabpot fabpot merged commit a01ed35 into symfony:2.8 Sep 24, 2015

1 of 3 checks passed

continuous-integration/appveyor AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Sep 24, 2015

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

This PR was merged into the 2.8 branch.

Discussion
----------

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

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | at least partially: #14300, #11158, #11451, #10035, #10463, #8606, probably more
| License       | MIT
| Doc PR        | symfony/symfony-docs#5265

Hi guys!

Though it got much easier in 2.4 with `pre_auth`, authentication is a pain in Symfony. This introduces a new authentication provider called guard, with one goal in mind: put everything you need for *any* authentication system into one spot.

### How it works

With guard, you can perform custom authentication just by implementing the [GuardAuthenticatorInterface](https://github.com/weaverryan/symfony/blob/guard/src/Symfony/Component/Security/Guard/GuardAuthenticatorInterface.php) and registering it as a service. It has methods for every part of a custom authentication flow I can think of.

For a working example, see https://github.com/weaverryan/symfony-demo/tree/guard-auth. This uses 2 authenticators simultaneously, creating a system that handles [form login](https://github.com/weaverryan/symfony-demo/blob/guard-auth/src/AppBundle/Security/FormLoginAuthenticator.php) and [api token auth](https://github.com/weaverryan/symfony-demo/blob/guard-auth/src/AppBundle/Security/TokenAuthenticator.php) with a respectable amount of code. The [security.yml](https://github.com/weaverryan/symfony-demo/blob/guard-auth/app/config/security.yml) is also quite simple.

This also supports "manual login" without jumping through hoops: https://github.com/weaverryan/symfony-demo/blob/guard-auth/src/AppBundle/Controller/SecurityController.php#L45

I've also tested with "remember me" and "switch user" - no problems with either.

I hope you like it :).

### What's Needed

1) **Other Use-Cases?**: Please think about the code and try it. What use-cases are we *not* covering? I want Guard to be simple, but cover the 99.9% use-cases.

2) **Remember me** functionality cannot be triggered via manual login. That's true now, and it's not fixed, and it's tricky.

### Deprecations?

This is a new feature, so no deprecations. But, creating a login form with a guard authenticator is a whole heck of a lot easier to understand than `form_login` or even `simple_form`. In a perfect world, we'd either deprecate those or make them use "guard" internally so that we have just **one** way of performing authentication.

Thanks!

Commits
-------

a01ed35 Adding the necessary files so that Guard can be its own installable component
d763134 Removing unnecessary override
e353833 fabbot
dd485f4 Adding a new exception and throwing it when the User changes
302235e Fixing a bug where having an authentication failure would log you out.
396a162 Tweaks thanks to Wouter
c9d9430 Adding logging  on this step and switching the order - not for any huge reason
31f9cae Adding a base class to assist with form login authentication
0501761 Allowing for other authenticators to be checked
293c8a1 meaningless author and license changes
81432f9 Adding missing factory registration
7a94994 Thanks again fabbot!
7de05be A few more changes thanks to @iltar
ffdbc66 Splitting the getting of the user and checking credentials into two steps
6edb9e1 Tweaking docblock on interface thanks to @iltar
d693721 Adding periods at the end of exceptions, and changing one class name to LogicException thanks to @iltar
eb158cb Updating interface method per suggestion - makes sense to me, Request is redundant
c73c32e Thanks fabbot!
6c180c7 Adding an edge case - this should not happen anyways
180e2c7 Properly handles "post auth" tokens that have become not authenticated
873ed28 Renaming the tokens to be clear they are "post" and "pre" auth - also adding an interface
a0bceb4 adding Guard tests
05af97c Initial commit (but after some polished work) of the new Guard authentication system
330aa7f Improving phpdoc on AuthenticationEntryPointInterface so people that implement this understand it
@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 24, 2015

Member

The subtree-split is now active for this repository and available here:

https://github.com/symfony/security-guard
https://packagist.org/packages/symfony/security-guard

Member

fabpot commented Sep 24, 2015

The subtree-split is now active for this repository and available here:

https://github.com/symfony/security-guard
https://packagist.org/packages/symfony/security-guard

@derrabus

This comment has been minimized.

Show comment
Hide comment
@derrabus

derrabus Sep 24, 2015

Contributor

@weaverryan Symfony 2.7 compatibility in composer.json wouldn't be too bad, so I could try out the guard in an existing application without pulling the entire framework from an unstable branch.

Apart from that: 👏 Great work, I'm glad to see this feature being merged. 😃

Contributor

derrabus commented Sep 24, 2015

@weaverryan Symfony 2.7 compatibility in composer.json wouldn't be too bad, so I could try out the guard in an existing application without pulling the entire framework from an unstable branch.

Apart from that: 👏 Great work, I'm glad to see this feature being merged. 😃

@derrabus

This comment has been minimized.

Show comment
Hide comment
@derrabus

derrabus Sep 24, 2015

Contributor

The unit tests pass, if I pull every required Symfony component from 2.7.4 but security-core. As far as I can tell, the only reason why security-core has to be ~2.8@dev is the usage of a new exception class (AuthenticationExpiredException). I'm not sure if you would want to work around this.

But what you can probably do is setting security-http to ~2.7.

Contributor

derrabus commented Sep 24, 2015

The unit tests pass, if I pull every required Symfony component from 2.7.4 but security-core. As far as I can tell, the only reason why security-core has to be ~2.8@dev is the usage of a new exception class (AuthenticationExpiredException). I'm not sure if you would want to work around this.

But what you can probably do is setting security-http to ~2.7.

@TomasVotruba

This comment has been minimized.

Show comment
Hide comment
@TomasVotruba

TomasVotruba Sep 24, 2015

Contributor

@weaverryan Great job!

Contributor

TomasVotruba commented Sep 24, 2015

@weaverryan Great job!

@ad3n

This comment has been minimized.

Show comment
Hide comment
@ad3n

ad3n Sep 25, 2015

thanks @fabpot 👍

ad3n commented Sep 25, 2015

thanks @fabpot 👍

$this->event = null;
$this->logger = null;
$this->request = null;
}

This comment has been minimized.

@lyrixx

lyrixx Sep 26, 2015

Member

Missing $this->rememberMeServices = null;

@lyrixx

lyrixx Sep 26, 2015

Member

Missing $this->rememberMeServices = null;

This comment has been minimized.

@weaverryan

weaverryan Sep 26, 2015

Member

addressed in #15920

@weaverryan

weaverryan Sep 26, 2015

Member

addressed in #15920

@weaverryan weaverryan deleted the weaverryan:guard branch Sep 26, 2015

@weaverryan weaverryan referenced this pull request Sep 26, 2015

Merged

Guard minor tweaks #15920

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Sep 26, 2015

Member

@derrabus I've opened #15920 to lower the http requirement. But I don't think we'll want to add a class_exists with the AuthenticationExpiredException to get 2.7 compatibility

Member

weaverryan commented Sep 26, 2015

@derrabus I've opened #15920 to lower the http requirement. But I don't think we'll want to add a class_exists with the AuthenticationExpiredException to get 2.7 compatibility

fabpot added a commit that referenced this pull request Sep 27, 2015

minor #15910 Add the replace rules for the security-guard component (…
…stof)

This PR was merged into the 2.8 branch.

Discussion
----------

Add the replace rules for the security-guard component

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

The update of composer replacements was forgotten in #14673

Commits
-------

5ef8abc Add the replace rules for the security-guard component

fabpot added a commit that referenced this pull request Sep 27, 2015

minor #15920 Guard minor tweaks (weaverryan)
This PR was merged into the 2.8 branch.

Discussion
----------

Guard minor tweaks

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Various completely minor things, most from suggestions on #14673

Commits
-------

869d5a7 tweaking message related to configuration edge case that we want to be helpful with
da4758a Minor tweaks - lowering the required security-http requirement and nulling out a test field
@rvanlaak

This comment has been minimized.

Show comment
Hide comment
@rvanlaak

rvanlaak Sep 28, 2015

Contributor

Nice! Would this also allow "simple" configuration via annotations? #13950

Contributor

rvanlaak commented Sep 28, 2015

Nice! Would this also allow "simple" configuration via annotations? #13950

fabpot added a commit that referenced this pull request Oct 2, 2015

feature #14721 [Security] Configuring a user checker per firewall (il…
…tar)

This PR was squashed before being merged into the 2.8 branch (closes #14721).

Discussion
----------

[Security] Configuring a user checker per firewall

_Changed my base branch to avoid issues, closed old PR_

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed ticket | #11090 and helps #14673
| License       | MIT
| Doc PR        | symfony/symfony-docs/pull/5530

This pull request adds support for a configurable user checker per firewall. An example could be:

```yml
services:
    app.user_checker:
        class: App\Security\UserChecker
        arguments:
            - "@request_stack"

security:
    firewalls:
        secured_area:
            pattern: ^/
            anonymous: ~
            basic_auth: ~
            user_checker: app.user_checker

```
The above example will use the `UserChecker` defined as `app.user_checker`. If the `user_checker` option is left empty, `security.user_checker` will  be used. If the `user_checkers` option is not defined, it will fall back to the original behavior to not break backwards compatibility and will validate using the existing `UserChecker`: `security.user_checker`.

I left the default argument in the service definitions to be `security.user_checker` to include backwards compatibility for people who for some reason don't have the extension executed. You can obtain the checker for a specific firewall by appending the firewall name to it. For the firewall `secured_area`, this would be `security.user_checker.secured_area`.

Commits
-------

76bc662 [Security] Configuring a user checker per firewall
@Codenator81

This comment has been minimized.

Show comment
Hide comment
@Codenator81

Codenator81 Nov 5, 2015

@weaverryan Super simple! Love it!

Codenator81 commented Nov 5, 2015

@weaverryan Super simple! Love it!

*
* @throws AuthenticationException
*/
public function checkCredentials($credentials, UserInterface $user);

This comment has been minimized.

@patrick-mcdougle

patrick-mcdougle Nov 19, 2015

Contributor

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?

@patrick-mcdougle

patrick-mcdougle Nov 19, 2015

Contributor

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?

This comment has been minimized.

@dunglas

dunglas Nov 19, 2015

Member

👍

@dunglas

dunglas Nov 19, 2015

Member

👍

This comment has been minimized.

@webmozart

webmozart Nov 19, 2015

Contributor

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.

@webmozart

webmozart Nov 19, 2015

Contributor

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.

This comment has been minimized.

@fabpot
@fabpot

fabpot Nov 19, 2015

Member

see #16395

This comment has been minimized.

@patrick-mcdougle

patrick-mcdougle Nov 19, 2015

Contributor

Heh, maybe I should look at the tip of 2.8 before I go commenting on old PRs. Sorry everyone.

@patrick-mcdougle

patrick-mcdougle Nov 19, 2015

Contributor

Heh, maybe I should look at the tip of 2.8 before I go commenting on old PRs. Sorry everyone.

This comment has been minimized.

@weaverryan

weaverryan Nov 19, 2015

Member

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

@weaverryan

weaverryan Nov 19, 2015

Member

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

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Nov 19, 2015

Contributor
  • If you return a boolean, how will you know what went wrong?
  • How will you be able to trace back the reason of not being able to login, to this specific method implementation?

Imo this is a developer mistake and tests should've caught that. There's already a weird case with ParamConverters having to return a boolean for some reason which makes it far more confusion than necessary.

As long as it's stated that an exception should be thrown in the docs: 👎 from my side for this request.

Maybe result objects are a better suggestion, where you can specify a reason. It would conflict with the current flow of AuthenticationException though.

Contributor

iltar commented Nov 19, 2015

  • If you return a boolean, how will you know what went wrong?
  • How will you be able to trace back the reason of not being able to login, to this specific method implementation?

Imo this is a developer mistake and tests should've caught that. There's already a weird case with ParamConverters having to return a boolean for some reason which makes it far more confusion than necessary.

As long as it's stated that an exception should be thrown in the docs: 👎 from my side for this request.

Maybe result objects are a better suggestion, where you can specify a reason. It would conflict with the current flow of AuthenticationException though.

weaverryan added a commit to symfony/symfony-docs that referenced this pull request Nov 27, 2015

feature #5265 Documentation for the new Guard authentication style (w…
…eaverryan)

This PR was merged into the 2.8 branch.

Discussion
----------

Documentation for the new Guard authentication style

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | yes symfony/symfony#14673
| Applies to    | 2.8+
| Fixed tickets | n/a

Hi guys!

This is a WIP documentation for a proposed new authentication system. I've written just enough so people can understand how to use it, but will finish it later once the code has gotten reviewed.

Thanks!

Commits
-------

51720c7 Many fixes thanks to great review from ogizanagi, javiereguiluz and others
4752d4c adding one clarifying message
9782ff1 adding toc entries
62dcae3 Using JsonResponse + cleanup
440fe6f revamping Guard article
bfce91b Fixing minor comments
9e411fe I'm extending the abstract class - so mention that. Also adding anonymous
ac107c7 WIP documentation for the new guard auth

xabbuh added a commit to symfony/symfony-docs that referenced this pull request Feb 10, 2016

feature #5886 [2.8] Add "How to Use Multiple Guard Authenticators" co…
…okbook documentation (mheki)

This PR was squashed before being merged into the 2.8 branch (closes #5886).

Discussion
----------

[2.8] Add "How to Use Multiple Guard Authenticators" cookbook documentation

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | yes (symfony/symfony#14673)
| Applies to    | `2.8` onwards

Hi guys,
this is my first contribution to the symfony docs.
During my preparations for the Symfony Guard component workshops I have spent some time trying to figure out the problem described here.
I hope this cookbook entry will help others save their time.

cc @weaverryan

Thanks!

Commits
-------

121196d [2.8] Add "How to Use Multiple Guard Authenticators" cookbook documentation
@bogdaniel

This comment has been minimized.

Show comment
Hide comment
@bogdaniel

bogdaniel Mar 16, 2016

Question in your working example shouldn't be return true if else?
File: FormLoginAuthenticator.php
Line: 60

        if (!$passwordValid) {
            throw new BadCredentialsException();
        }

To

        if ($passwordValid)
            return true;
        else
            throw new BadCredentialsException();

Because i think it will always hit the user with a BadCredentialsException()

bogdaniel commented Mar 16, 2016

Question in your working example shouldn't be return true if else?
File: FormLoginAuthenticator.php
Line: 60

        if (!$passwordValid) {
            throw new BadCredentialsException();
        }

To

        if ($passwordValid)
            return true;
        else
            throw new BadCredentialsException();

Because i think it will always hit the user with a BadCredentialsException()

@wouterj

This comment has been minimized.

Show comment
Hide comment
@wouterj

wouterj Mar 16, 2016

Member

@bogdaniel this behaviour was changed after this PR (but before releasing 2.8.0) it seems like the demo hasn't been updated.

Member

wouterj commented Mar 16, 2016

@bogdaniel this behaviour was changed after this PR (but before releasing 2.8.0) it seems like the demo hasn't been updated.

@derrabus

This comment has been minimized.

Show comment
Hide comment
@derrabus

derrabus Mar 18, 2016

Contributor

@wouterj This shouldn't be the place to maintain an up-to-date demo for this feature. imho, the PR should be seen as a documentation of the development process and the demo shows how the feature has worked at the time when the PR was merged.

@bogdaniel: Please see the docs for an up-to-date documentation on this feature: http://symfony.com/doc/current/cookbook/security/guard-authentication.html

Contributor

derrabus commented Mar 18, 2016

@wouterj This shouldn't be the place to maintain an up-to-date demo for this feature. imho, the PR should be seen as a documentation of the development process and the demo shows how the feature has worked at the time when the PR was merged.

@bogdaniel: Please see the docs for an up-to-date documentation on this feature: http://symfony.com/doc/current/cookbook/security/guard-authentication.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment