Skip to content
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

Email activation, login and reset password. #682

Open
2 tasks done
jtraulle opened this issue Apr 19, 2018 · 8 comments
Open
2 tasks done

Email activation, login and reset password. #682

jtraulle opened this issue Apr 19, 2018 · 8 comments

Comments

@jtraulle
Copy link
Contributor

jtraulle commented Apr 19, 2018

This is a :

  • enhancement
  • 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)
    ->find('active', $options);

    public function findActive(Query $query, array $options = [])
    {
    $query->where([$this->_table->aliasField('active') => 1]);
    return $query;
    }
  • 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
Copy link
Contributor Author

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

@jtraulle
Copy link
Contributor Author

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
Copy link
Contributor Author

jtraulle commented Apr 19, 2018

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 jtraulle changed the title [RFC] Email activation, login and reset password. Email activation, login and reset password. Apr 19, 2018
@steinkel
Copy link
Member

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,

@viniciusbig
Copy link

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.

@mstroink
Copy link

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

@jtraulle
Copy link
Contributor Author

@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 🙂

@ajibarra
Copy link
Member

@steinkel can we close this?

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

No branches or pull requests

5 participants