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

Locked user can login by resetting his password #1413

Closed
EmmanuelVella opened this Issue Feb 9, 2014 · 28 comments

Comments

Projects
None yet
@EmmanuelVella
Copy link

EmmanuelVella commented Feb 9, 2014

A locked user can login if he resets his password and click the email link. Maybe a check should be added in the controller ?

@dbu

This comment has been minimized.

Copy link

dbu commented Feb 13, 2014

is that not a feature? i would assume an account to become locked when you enter your password incorrectly too many times - if you want to deactivate an account, you would use enabled rather than locked. but this is just a guess, it looks like the locked information is never set by fosuserbundle. i tried finding information about the locked thing in symfony itself, its part of AdvancedUserInterface but not really explaining the semantics.

@EmmanuelVella

This comment has been minimized.

Copy link
Author

EmmanuelVella commented Feb 14, 2014

@dbu Currently, if a user requests a new password, the enabled field will be set to true (see #1295).

I think that the locked field allows to block an user. Could we have a confirmation ?

Anyway, it seems there is currently no way to completely block a user.

@dbu

This comment has been minimized.

Copy link

dbu commented Feb 14, 2014

oh, i see. we really need a more semantical definition of the locked and
enabled flags.

@vlastimilmaca

This comment has been minimized.

Copy link

vlastimilmaca commented Mar 2, 2014

This may be pretty serious security breach I guess ...

My point of view on flags:

When user is author somewhere, he can't be removed because of data integrity lose. There is need to keep him in database, but still be able to act with him like he is deleted.

Right now fos user has 3 flags 4 flags available - enabled, locked, expired, credentialsExpired.
These flags are checked in UserChecker which is used in UserAuthenticationProvider.

Enabled = true

User is verified, that means user is email owner for sure. This can be verified by resseting password or clicking on confirmation email after registration.
This flag should not be touched by admin or other user.
If enabled = false DisabledException is thrown.

Locked = true

User is forbbiden to manipulate his accout, because it is locked down. That means no password reset, login etc.
This flag allows admin to ban user or don't let him register with his email again.
LockedException is thrown.

Expired = true

User is archived by admin or after some time from last login (CRON service?).~~ When he logs again, revalidation is required.~~
This flag allows admin to force user to revalidate himself, change his password or use it as an inactive users archive, which can't login.
AccountExpiredException is thrown.

CredentialsExpired = true

This is checked after login and if true, user should be forced to change his password and revalidate himself.
CredentialsExpiredException is thrown.

There are 8 states of user total.
Flags are checked in this order:

  1. Locked
  2. Enabled
  3. Expired
  4. CredentialsExpired
Enabled Locked Expired State description
0 0 0 After registration, potential fake user, can be deleted
0 0 1 Not validated for a long time, can be deleted (auto delete recommended)
0 1 0 Blocked, not validated user, no more registrations on this email, can be deleted
0 1 1 Blocked, not validated, inactive, no more registrations on this email, can be deleted
1 1 0 Blocked or banned user
1 1 1 Blocked user for a long time, revalidate after unblock and login
1 0 0 Active, validated user
1 0 1 Inactive user, require revalidation after next login

There may be need to legalize user after registration by admin. It will be nice to have next flag for this case (legalized). This flag may act in same way as locked flag, but use it only if set in configuration...

@dbu

This comment has been minimized.

Copy link

dbu commented Mar 2, 2014

as these flags are defined by core symfony, i would think its a definition that symfony should provide (or maybe it already is somewhere and we did not find it yet). /cc @weaverryan @wouterj

@vlastimilmaca

This comment has been minimized.

Copy link

vlastimilmaca commented Mar 2, 2014

I looked in to code little bit and there is missing call of userChecker->checkPreAuth() which is usually called on login, but not on autologin after resetting password and registration action.

All about this issue happens in AuthenticationListenerer, where only $user->isEnabled() is checked. After that loginManager->loginUser() is called. In loginUser is called userChecker->checkPostAuth() only.

To solve this, there must be called userChecker->checkPreAuth() in AuthenticationListener and replace isEnabled() check (which is done in userChecker PreAuth anyway) or before userChecker->checkPostAuth() in LoginManager

@archie18

This comment has been minimized.

Copy link

archie18 commented Mar 31, 2014

Whoa, this looks pretty serious. Any recommended workaround to really prevent locked users from logging in?
--Andreas

@fabpot

This comment has been minimized.

Copy link

fabpot commented Apr 1, 2014

This part of Symfony is a direct port of Spring Security, which was named Ageci some years ago. Here is an explanation of the semantics of various flags from an old blog post about Ageci:

"
Disabled indicates an account has been administratively or automatically disabled for some reason. Usually some action is required to release it.

Locked indicates an account has been automatically suspended due to invalid login attempts. Usually the passage of time or (less often) requesting manual unlocking is required to release it.

The distinction is not used by Acegi Security code aside from providing more informative errors to the user. There is also an order in which different exceptions should be returned, so that a disabled or locked account for instance will not return a bad credentials exception. Refer to the JavaDocs for more details.
"

see http://forum.spring.io/forum/spring-projects/security/340-whats-the-difference-between-locked-and-disabled-exceptions for the original discussion.

@rayrigam

This comment has been minimized.

Copy link

rayrigam commented Apr 4, 2014

There seems to be a bug indeed! I just checked and can confirm that a locked user cannot login. When trying to do so, one gets the message: User account is locked.. However, as mentioned by @EmmanuelVella if the user resets his/her password with FOSUserBundle, after the reset is completed, the user is automatically logged in even if "locked" remains TRUE . If the user then logs out, he/she is not allowed to log in again (unless he/she resets the password again)...

@jshethctl

This comment has been minimized.

Copy link

jshethctl commented Apr 9, 2014

This is definitely un-intuitive at best, and a severe bug at worst, especially if one has implemented the user administration front-end to change the "locked" flag, instead of the "enabled" flag, when a user is disabled.

A simple work-around would be to extend ResettingController.php from FOSUserBundle, and log the user out after the password has been reset.

Within the resetAction(), one could do:

$userManager->updateUser($user);
                $response = $event->getResponse();
                if (null === $response) {
                    //These two lines are new:
                    $url = $this->container->get('router')->generate('fos_user_security_logout');
                    $response = new RedirectResponse($url);
                }

                $dispatcher->dispatch(FOSUserEvents::RESETTING_RESET_COMPLETED, new FilterUserResponseEvent($user, $request, $response));

One could then create a logout success handler to check the referrer, and show a message asking the user to login again, if he/she just came from the password reset page. (Somewhat related: http://www.reecefowell.com/2011/10/26/redirecting-on-loginlogout-in-symfony2-using-loginhandlers/)

@hellomedia

This comment has been minimized.

Copy link

hellomedia commented Jun 5, 2014

OK, allowing disabled/locked users to re-login by resetting their password (or with their remember me cookie symfony/symfony#10242 ) isn't so great, so I've digged around and here are my findings. There are mainly 2 things at play here:

  • differences in the meaning behind enabled and locked
  • a change in Symfony's handling of checkPreAuth and checkPostAuth

Timeline:

  • Jan 2012 : bug fix by @stof to avoid login from locked or disabled users. At the time, the correct check is made with checkPostAuth . https://github.com/FriendsOfSymfony/FOSUserBundle/pull/486/files .
    Notice how Stof allows a disabled user to reset his password ( #486 ) . According to his understanding ( explained here https://groups.google.com/forum/#!topic/symfony2/aG79IY7Kmc8 ) , and to account for an edgy usecase, it should be allowed.
  • July 2012 : the checkPostAuth call is moved from the controllers to the login manager, where it is today : 8e412a7
  • Nov 2012 : Unlike Stof, this pull request wants to prevent a disabled user to reset his password (and re-enable his account in the process) #1020 . It also uses checkPostAuth .
  • Dec 2013 : @fabpot switches the content of checkPreAuth and checkPostAuth. ( symfony/symfony#9902 ). This makes above bugfixes useless. Easy fix tho, just replace checkPostAuth by checkPreAuth .
  • April 2013: Fabien comments in this thread to explain the meaning of enabled and locked . However, it does not seem to match Stof's understanding above.

@dbu Overall, it seems like @vlastimilmaca 's comment was on the spot and we could start with 2 things:

  • replace the call to checkPostAuth by checkPreAuth in the loginManager.
  • see if everyone agrees on Fabien's / Spring definition of enabled and locked , add it to the doc somewhere, and go from there to tackle the edgy usecases.
@rayrigam

This comment has been minimized.

Copy link

rayrigam commented Jun 5, 2014

@hellomedia thank you for looking into this! Some additional considerations: It seems that according to the above @fabpot / Spring Security definitions, the enabled attribute should be used to manually block a user from authenticated access to a site. Correct? If so, the user should not even be allowed to reset the password when enabled is set to false, just as it should not be possible when locked is set to true. An error should be returned to the user saying something like "You cannot reset the password of this account as it is currently disabled", or in the case of a locked account: "You cannot reset the password just now as the maximum number of invalid login attempts has been reached. Try again in 24 hours."

@hellomedia

This comment has been minimized.

Copy link

hellomedia commented Jun 6, 2014

@rayrigam I agree. My understanding at this point :

  • Should enabled be used instead of locked to administratively block a user ? Yes, that seems to be coherent with Fabien's comment.
  • Can a user reset his password (and re-enable account) when account is disabled ? no.
  • Can a user reset his password (and unlock account) when account is locked ? For @dbu it seems OK. does not seem like a big deal either way.
@jshethctl

This comment has been minimized.

Copy link

jshethctl commented Jun 6, 2014

Hi,
The "enabled" flag is set once the user has verified his or her email. So, some users may assume that "enabled" corresponds to "email verified", and "locked" corresponds to "blocked from system".

I agree with rayrigam - if locked is set to true, or enabled is set to false, the user should not be able to reset his or her password.

@rayrigam

This comment has been minimized.

Copy link

rayrigam commented Jun 6, 2014

@hellomedia I agree with your points. Technically, according to the above definitions, a locked user should not be allowed to reset the password without the passage of time or the manual unlocking; however, given that there is no current system to automatically lock an account and record the passage of time, it does not seem like a big deal to go either way when locked is true. The handling of the disabled case definitely seems more critical.

@stof

This comment has been minimized.

Copy link
Member

stof commented Jun 6, 2014

there is no automatic system to lock the user either (unless you implemented your own)

@gondo

This comment has been minimized.

Copy link

gondo commented Jun 17, 2014

another potential security issue:
there is no time check during token verification when resetting password (or confirmation registration)
imagine situation:

  • i successfully register and confirm registration
  • i click on forgot password and receive reset link (token) via email but i do not click on it
  • admin blocks/disable my account
  • i am not able to request new reset link (token)
  • but i still have my old email and i can use it to create new password and be logged it into the system

so the locked/enabled verification (whatever it ll be at the end) should be done in sendEmailAction and also in resetAction or in corresponding events

@sebastianreloaded

This comment has been minimized.

Copy link

sebastianreloaded commented Jul 8, 2014

sorry, to deviate from the core topic, but are expired and credentials_expired actively used or are they just there for convenience. My guess was they probably are there to implement your own authentication, but i couldnt find any helpful documentation.

@hellomedia

This comment has been minimized.

Copy link

hellomedia commented Jul 8, 2014

I think they are there for convenience. implementation is left up to you.

@gzankevich

This comment has been minimized.

Copy link

gzankevich commented Nov 7, 2014

Any updates on this?

@hellomedia

This comment has been minimized.

Copy link

hellomedia commented Nov 7, 2014

If I am not mistaken, if you use Fabien's definitions above for enabled and locked, you should be fine.
On a side note, I'd like to look into this more in depth but it won't be be until december.

@rayrigam

This comment has been minimized.

Copy link

rayrigam commented Nov 7, 2014

Unless I missed something, I think this is still a serious issue. As indicated in my previous comment, a user MUST NOT be allowed to reset his/her password if enabled is set to false. Currently, a disabled user can regain access to the site by resetting his/her password, so there is no way to block a user.

@hellomedia

This comment has been minimized.

Copy link

hellomedia commented Nov 7, 2014

If a user is disabled, isn't that checked in Authentication Listener ?

@cmodijk

This comment has been minimized.

Copy link

cmodijk commented Nov 8, 2014

Symfony changed the behaviour of the Cookie Based login and I think this is related to the Authentication manager inside the FOSUserBundle symfony/symfony#11058

@hellomedia

This comment has been minimized.

Copy link

hellomedia commented Nov 8, 2014

Here is a pull request for this. Please comment the PR : #1673

@liverbool

This comment has been minimized.

Copy link

liverbool commented Dec 3, 2014

@XWB

This comment has been minimized.

Copy link
Member

XWB commented Sep 6, 2016

@XWB

This comment has been minimized.

Copy link
Member

XWB commented Sep 6, 2016

@XWB XWB closed this Sep 6, 2016

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