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

Revamped "I forgot my password" screen for a simple and usable solution #3334

Closed
mattab opened this issue Aug 19, 2012 · 11 comments
Closed

Revamped "I forgot my password" screen for a simple and usable solution #3334

mattab opened this issue Aug 19, 2012 · 11 comments
Assignees
Labels
Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Milestone

Comments

@mattab
Copy link
Member

mattab commented Aug 19, 2012

Password reset process is a fairly common experience for all piwik users. We could simplify the workflow by removing the 4 steps process in email but simply leave url, and by hiding the input field on the password reset.

@robocoder
Copy link
Contributor

IIRC the token was removed from the URL because it was a security vulnerability.

A better user experience might be to remove the Reset form entirely. Instead:

  • Lost password form: enter your email address + new password; click on submit; wait for email; click on confirmation link (which updates the user record with the new password) and then autologin

or

  • Lost password form: enter your email address; click on submit; wait for email; receive an auto-generated password; click on link to autologin; redirect to user settings to set a new password

@robocoder
Copy link
Contributor

The token in the URL vulnerability is mitigated by #3080.

Should also think about how to migrate users when passwords are rehashed by #308.

@mattab
Copy link
Member Author

mattab commented Aug 20, 2012

Definitely #3080 would be a nice to have improvement for few reasons.

vipsoft would you be interested in #308? this would be a strong security improvement and limiting hijacking.

As to this ticket I think it's still worth doing the simplification in the UI, and also will leave #3080 with high priority.

@mattab
Copy link
Member Author

mattab commented Aug 22, 2012

A better user experience might be to remove the Reset form entirely. Instead:

  • Lost password form: enter your email address + new password; click on submit; wait for email; click on confirmation link (which updates the user record with the new password) and then autologin

Very cool proposal indeed! +1

@diosmosis
Copy link
Member

Attachment: Patch for this issue. Some files that will be removed aren't included in the patch.
3334.diff.tar.gz

@diosmosis
Copy link
Member

I've uploaded a patch for this issue that implements vipsoft's first suggestion. It also combines the login, resetPassword & lostPassword into the login form and uses AJAX and transitions for a smoother user experience. Let me know what you think and if it's good to commit.

@mattab
Copy link
Member Author

mattab commented Aug 27, 2012

Excellent improvements to the workflow!
The feature is now very user friendly and it seems still secure as it should be :)

Code review:

  • "Confirm Password Reset" maybe could be "Confirm Password Change" since hte password is not "reset" but changed?
  • Check your e-mail and visit this link to finish resetting your password. --> "to validate your new password" ?
  • Your password has been reset. --> "Your password has been changed", or maybe there's better option
  • refactor Piwik_Option::getInstance()->delete($login.'_reset_password_info'); and call it from the catch block if the email send didn't work, as to keep option table clean?
  • checkPassword / getPasswordHash / isValidPasswordString in API is changed to public so should have keyword @ignore to be hidden from api listing since they aren't useful to expose at this stage
  • $passwordHash is not defined in the following code
        if(empty($password) && empty($passwordHash))
        {
            $password = $userInfo['password'];
        }
  • $optionName = $login.'_reset_password_info'; should be refactored as it's used twice (refactoring ensures that the 2 classes will then be tightly coupled together and the dependency between both is visible)
  • after clicking on Lost password, put the cursor active in the login input field so users can start typing
  • $isPasswordHashed could be $_isPasswordHashed so that the parameter is not displayed in the auto generated API doc page?
  • on click on change pwd, can you display a loading icon since sending email can take 1-2 seconds?
  • Could you please create a new JS file login.js where you'd put the focusit() function and your new JS code? this file will only be included in the login screen.

@diosmosis
Copy link
Member

Attachment: New patch for this issue.
3334.diff.tar.2.gz

@diosmosis
Copy link
Member

I uploaded a new patch for this issue which should address all of your comments. Let me know if it's good to commit.

@diosmosis
Copy link
Member

(In [6900]) Fixes #3334, redesigned the reset password functionality.

Notes:

  • Resetting password is done through AJAX and the reset token does not need to be entered in a form.
  • Moved password related utility functions in UsersManager_API to UsersManager as static functions.
  • Added hidden _isPasswordHashed parameter to UsersManager::updateUser.
  • Make sure superuser login is set in Access instance when setSuperUser(true) is used.
  • Add ability to get rendered form data as array in QuickForm2 (moved existing logic in Piwik_View into new function).

@mattab
Copy link
Member Author

mattab commented Sep 2, 2012

Very nice change! Many users will benefit from this, as this is a very common action that most PIwik users do one day or another :)

@mattab mattab added this to the 1.8.4 - Piwik 1.8.4 milestone Jul 8, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

No branches or pull requests

3 participants