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

Unactivated users get wrong error message #35

Open
AIC-BV opened this issue Apr 4, 2023 · 14 comments
Open

Unactivated users get wrong error message #35

AIC-BV opened this issue Apr 4, 2023 · 14 comments

Comments

@AIC-BV
Copy link
Contributor

AIC-BV commented Apr 4, 2023

Hello,

My users have to activate their account before they can sign in. (Activation type: User)
This is to protect guest users from evil people 'stealing' their accounts. (my way of handling the important text block)

When a user tries to log in, that didn't activate his account yet, they get an error message like
The details you entered did not match our records. Please double-check and try again.

This error is wrong, and it should be something like
You didn't activate your account yet. Please check your email and activate your account.
and, ideally, resend the activation email.

User exists in backend user list, in database and with the onCheckEmail method. So I can change my password, but can never log in because I forgot to activate my account.

The current error message sends me in an infinite loop.

  1. It basically tells me to register
  2. But when registering it says my account already exists
  3. Which suggests me to reset my password
  4. Which when you do, you cannot log in because you are not activated yet
  5. Which resets the whole loop
@AIC-BV
Copy link
Contributor Author

AIC-BV commented Jun 6, 2023

hmm I am very confused now because it does seem to work locally. (maybe unintentionally fixed with Winter 1.2?)
Can be ignored for now ^^
https://github.com/wintercms/wn-user-plugin/blob/main/models/User.php#L284

@AIC-BV AIC-BV closed this as not planned Won't fix, can't repro, duplicate, stale Jun 6, 2023
@LukeTowers
Copy link
Member

@AIC-BV the issue that you are running into is that the exception being thrown is an AuthException. When app.debug is enabled the message will be returned unaltered, but when it is false (i.e. in production) then the message will be replaced with a generic message for security (i.e. you don't want a bad actor to be able to figure out who has an account on your service or not).

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Jun 7, 2023

Yes, Thanks for pointing that out!
I tested it and it is indeed a 'problem' with debug mode true/false.

When it is true, it is throwing the expected answers.
For example:

  • ... is suspended
  • ... is not yet activated
  • ...

But as you mentioned, when debug mode is false, it is throwing that generic message.

I understand your argument, but then how would I let my user know their account is not yet activated when debug mode is false? They cannot proceed as long as it is not activated, so it is a really important step. They get a popup to activate their account when registering but some people (like 1/100) just ignore the popup and click it away and then 'complain' they cannot log in

@AIC-BV AIC-BV reopened this Jun 7, 2023
@LukeTowers
Copy link
Member

It depends on your specific use case and users, one idea might be to resend the activation email when they attempt to login. Another (probably better approach) would be to check if the user is activated immediately after successfully validating their credentials but before authenticating their session or sending any auth cookies back and if they aren't activated at that point you can send the more helpful error message without being concerned about the security implications.

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Jun 7, 2023

The second one sounds a bit complex,
but the first option sounds good enough to capture the 1% of users struggling with this issue.

Do I understand you correctly that I have to do some magic to check wether the user is activated or not inside the following event:

Event::listen('winter.user.beforeAuthenticate', function($component, $credentials) {
    $login = array_get($credentials, 'login');
    $user = Auth::findUserByLogin($login);
    if (!$user) return;

   // check if user is activated or not to throw a proper notifcation with a button to resend the activation e-mail if not yet activated
});

@LukeTowers
Copy link
Member

Yes, that would probably work.

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Jun 8, 2023

@LukeTowers
My solution is the following:

// extending login - make sure user is activated. If not, resend activation e-mail
Event::listen('winter.user.beforeAuthenticate', function($component, $credentials) {
    $login = array_get($credentials, 'login');
    $user = Auth::findUserByLogin($login);
    if (!$user) return;
        
    // if user is NOT activated, show a popup with a button to resend activation e-mail
    if (!$user->is_activated) {

        // save email to know when to show the popup and feed the onSendActivationEmail button
        Session::put('user.not_activated', $login);

        // refresh page to display message
        // NOT WORKING :(
        return redirect()->refresh();

    }
});
==
<?php
public function onStart()
{
    $this['user_not_activated'] = Session::pull('user.not_activated', false);
}
?>
==
{% if user_not_activated %}
    <button data-request='onSendActivationEmail' data-request-data='email: "{{ user_not_activated }}"'>Resend activation email</button>
{% endif %}

The code above (except the refresh 😞) would work, BUT;
Guest users that require activation before login are currently not allowed to request an activation e-mail because you have to be logged in: https://github.com/wintercms/wn-user-plugin/blob/main/components/Account.php#L476

To solve this, I added the following code:

public function onSendActivationEmail()
{
    try {
        $user = $this->user();
        if (!$user) {
                
            // check for login (fix guest users)
            $login = Input::get('email', false);
            if ($login) $user = Auth::findUserByLogin($login);
            if (!$user) throw new ApplicationException(Lang::get(/*You must be logged in first!*/'winter.user::lang.account.login_first'));
        }
        
        ...

Would this, or something like this, be accepted in PR?
It is a must to properly support this option? What if they didn't get the e-mail, removed it before, ...?
image

@bennothommo
Copy link
Member

I don't think that option is needed. I think it's common knowledge that signing into an account requires it to be activated.

It is a must to properly support this option? What if they didn't get the e-mail, removed it before, ...?

Account activation via confirming via email is a very common scenario - I don't think it's necessarily likely that people will miss the email.

What I would suggest is that - if the User plugin doesn't do this already - the Forgotten Password feature should count as activation (if it's set to "User") because it confirms that the user resetting their password owns the account by clicking on the link provided in the email. Then, at least, there's an out if people do actually miss the flash message, email, etc.

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Jun 8, 2023

That is one way of fixing the issue!
Didn't think about that, but it is still confusing because they had the correct password.
But it will get them out of the 'activation loop' and they will think they had a typo by accident 😈

Automatically activating a user on password reset sounds good, however, it feels like its a workaround of the actual issue?
The user will still not get a message that their account is not yet activated when they try to login

We get atleast two phone calls a week because of people running in to this issue.

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Jun 8, 2023

@AIC-BV the issue that you are running into is that the exception being thrown is an AuthException. When app.debug is enabled the message will be returned unaltered, but when it is false (i.e. in production) then the message will be replaced with a generic message for security (i.e. you don't want a bad actor to be able to figure out who has an account on your service or not).

Isn't that what onCheckEmail does? 😅 (letting a bad actor to be able to figure out who has an account on your service or not)

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Jun 8, 2023

Only today I have 3 users that registered but didn't activate their account yet
I just know atleast one of them will contact us 😂
As long as they don't click the e-mail, they will be in the 'activation loop'
image

@LukeTowers
Copy link
Member

Isn't that what onCheckEmail does? 😅 (letting a bad actor to be able to figure out who has an account on your service or not)

@AIC-BV yes but it is not provided in the core plugin for a reason, it's up to the developer to make the decision if that is an acceptable risk or not.

@bennothommo
Copy link
Member

@AIC-BV I've used the User plugin several times with the User activation enabled, and I cannot say I've had many people ask why they could not log in that turned out to be an activation issue (maybe 3 or 4 in the same amount of years?).

It's all about how you onboard them after completing the registration - you could perhaps redirect them to a content page that welcomes them to the site/app and tells them to check their email for the activation message? Or write a Snowboard plugin that increases the timeout of the flash message?

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Jun 9, 2023

After registration there is a popup to welcome them and saying they have to activate their account via email before they can log in. Some people just ignore/don't pay attention to it.... These people get stuck in the 'activation loop'.
(or click it away by accident which could be solved by an entirely seperate page I guess)

I'm tasked with preventing the 'activation loop'.
I'm not entirely convinced that this issue is specific to my case only.
I believe it is a general WinterCMS issue, in which our opinions differ.

The users that didn't check their e-mail yet will never get a correct error message to let them know what they did wrong. Regardless wether it is common knowledge or not, to me, this feels like a fundamental UX issue...
We have many new users every single day and a very few of them appear not to know this.

But; it is not a problem; I will keep my modified Account.php file from user component and update it manually 👍
(it is already modified with the rules for guest user anyways)

I'm glad you guys helped me think of a proper solution 🤟
The workaround would be on password reset to activate the user, which would already be a great addition, but it doesn't tackle the problem directly.

bennothommo added a commit that referenced this issue Jun 9, 2023
Since the reset password flow involves an email being sent to the user to confirm the password reset, this should satisfy user activation as well as the same requirement is met.

Refs: #35 (comment)
bennothommo added a commit that referenced this issue Jun 11, 2023
Since the reset password flow involves an email being sent to the user to confirm the password reset, this should satisfy user activation as well as the same requirement is met.

Refs: #35 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants