users icon indicating copy to clipboard operation
users copied to clipboard

Email activation, login and reset password.

Open jtraulle opened this issue 6 years ago • 8 comments

This is a :

  • [x] enhancement
  • [x] feature-discussion (RFC)

If I am not wrong, the same token field is used in database to validate the user email address when using Users.Email.validate and when the user request to reset password.

So, if a user :

  • register through the app but does not validate his/her email
  • try to login directly through the app (Flash message indicates that "Username or password is incorrect" as there is no specific message for an existing user with correct password but inactive account — as the check for both username, password AND active fields seems to be done in the same query) https://github.com/CakeDC/users/blob/601e483e64f5e8acc572de20cde775842ce19048/src/Model/Behavior/AuthFinderBehavior.php#L58 https://github.com/CakeDC/users/blob/601e483e64f5e8acc572de20cde775842ce19048/src/Model/Behavior/AuthFinderBehavior.php#L30-L35
  • as the app says "Username or password is incorrect", the user think that he/she mistyped the password and request to reset the password
  • doing so, the token field which previously contained the token used to validate the user's email address is overridden by the token used to reset the password
  • the user successfully reset its password but he/she still cannot login because his/her account is still inactive

In my opinion, when a user logs in, the plugin should first check if username and password are correct, then, if user is not activated, display a specific error message.

  • if the token is still valid, output a Flash message to tell the user to click the link on the email ; else resend a new activation email and tell the user to click the link

On the other hand, if the user request to reset its password and his/her account is not active, then, the plugin should :

  • prevent the user to reset his/her password by telling that it is not possible as he/she muste activate his/her account first
  • if the token is still valid, output a Flash message to tell the user to click the link on the email ; else resend a new activation email and tell the user to click the link

jtraulle avatar Apr 19 '18 13:04 jtraulle

Seems related to #269, #306, #320, #454, #590 and #644.

jtraulle avatar Apr 19 '18 13:04 jtraulle

Ok, just saw the Users.Registration.ensureActive from /Docs/Documentation/Configuration.md . I think it should be defined to true by default in the Docs.

jtraulle avatar Apr 19 '18 14:04 jtraulle

I solved it that way :

Firstly, define Users.Registration.ensureActive config key to true.

After that, my class UsersTable extends \CakeDC\Users\Model\Table\UsersTable and override findAuth() method to remove the ->find('active', $options); line.

public function findAuth(Query $query, array $options = [])
{
    $identifier = Hash::get($options, 'username');
    if (empty($identifier)) {
        throw new \BadMethodCallException(__d('Missing \'username\' in options data'));
    }
    $where = $query->clause('where') ?: [];
    $query
        ->where(function ($exp) use ($identifier, $where) {
            $or = $exp->or_([$this->aliasField('email') => $identifier]);
            return $or->add($where);
        }, [], true);
    return $query;
}

My class UsersController extends \CakeDC\Users\Controller\UsersController and override login() method to check if user is active and redirect it to a custom page to let it know that he/she must validate email before login.

public function login()
{
    $this->set('titleForLayout', __('Sign in'));

    $this->getEventManager()->on(UsersAuthComponent::EVENT_AFTER_LOGIN, function (Event $event, $data) {
        if($data['active'] === false) {
            $this->RememberMe->destroy($event);
            $this->Auth->logout();
            return $this->redirect('/users/confirmEmail');
        }
    });

    parent::login();
}

I do not know if this is the way to go but it certainly address my issues.

jtraulle avatar Apr 19 '18 16:04 jtraulle

I remember some discussion regarding a similar issue, and at the end we need to keep some balance between usability and security, we should not provide messages that could expose existing user names to attackers, in this case an attacker could determine a valid username that is not yet activated because he would get a specific message. I like the way you did, checking first the user/pw is correct and then forcing a redirect to validate the token for accounts not yet validated. In this case we could even go to a more detailed page that shows exactly what happened to the user.

I'll mark this ticket as improvement. Thanks,

steinkel avatar Apr 25 '18 13:04 steinkel

What is this ticket status? Any updates?

I got he same scenario in the Cake4 version of the plugin.

In my opinion, we could simplify the process a little bit. (using the initial scenario in this ticket)

.... if the user request to reset its password and his/her account is not active, then, the plugin should :

  • change user password AND activate his/her account. Since the user is using his/her email to reset the password, this is a verification process by itself.

  • if the token is not valid (the reset token has expired for instance), resend a new activation email and tell the user to click the link.

viniciusbig avatar Jun 17 '20 01:06 viniciusbig

@jtraulle do you have an example of the confirmEmail() method? Is the user required to re-enter his username/email?

mstroink avatar Jun 27 '20 13:06 mstroink

@mstroink The confirmEmail() method is just here for defining the route.

/**
 * Display a simple page to instruct user to validate his/her account
 *
 * @return void
 */
public function confirmEmail()
{
    $this->set('titleForLayout', __('Confirm your email address'));
}

The corresponding view is just a basic message instructing the user to click on the link sent via email to validate its account.

When the user click on the link, its account is validated and it needs to authenticate to login :slightly_smiling_face:

jtraulle avatar Jun 27 '20 15:06 jtraulle