UX + Security current user password reset #5042

coolaj86 · 2018-10-07T18:29:36Z

Follow up to #5034

I'll annotate these a little later, but it's combos for these states:

not logged in
logged in as expected user
logged in as an unexpected user
valid code
invalid code (or expired code)

Default Condition

Not logged In
Valid Reset Code

Result

Shows email of user for whom the password is to be reset
Shows "Remember me"
Signs in the user

Logged In

Already Logged In
Valid Reset Code

Result

Shows email of user for whom the password is to be reset
Doesn't show "Remember me"
Renews user session

Invalid Code

Login Status N/A
Invalid Code

Result

Error about invalid code
Submits to same error page (could probably just disable submit)

Invalid User

User Logged in as FOO USER
Valid Code for BAR USER

Result

Error about mismatch user
Submits to same error page (could probably just disable submit)

codecov-io · 2018-10-07T20:26:52Z

Codecov Report

❗ No coverage uploaded for pull request base (master@fdb933c). Click here to learn what that means.
The diff coverage is 8.33%.

@@            Coverage Diff            @@
##             master    #5042   +/-   ##
=========================================
  Coverage          ?   41.66%           
=========================================
  Files             ?      414           
  Lines             ?    55985           
  Branches          ?        0           
=========================================
  Hits              ?    23327           
  Misses            ?    29529           
  Partials          ?     3129

Impacted Files	Coverage Δ
models/mail.go	`23.93% <0%> (ø)`
routers/routes/routes.go	`81.99% <100%> (ø)`
routers/user/auth.go	`13.08% <5.26%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdb933c...1688720. Read the comment docs.

routers/user/auth.go

daviian

I'm against auto-signin on password reset. Consequently I find having "Remember me" on Password reset awkward.

IMO we need a "password confirmation" field instead.
Showing Email is fine.

routers/routes/routes.go

coolaj86 · 2018-10-15T18:56:13Z

@daviian The "Remember me" felt a little weird to me too.

What I came to was that it's actually the form was incorrectly labeled. "Password Reset" is the process that happens on the Account page. This process is actually "Account Recovery", which is a different user flow.

The user isn't coming to this page with the intent to change their password (that is already on the Account page), but rather to be able to login to their account. In this case, with the current configuration, that implicitly includes setting a password. However, once more secure options are available (such as simply omitting passwords if a mailer is configured) the user will still want to recover their account (i.e. their primary oauth is down and they can't log in) but a password won't be part of the process.

I'll post new screenshots a little later.

coolaj86 · 2018-10-15T19:47:48Z

Two more things that should change for increased clarity:

"New Password" instead of "Password"
"Remember this device" instead of "Remember me"

stale · 2019-02-17T10:25:19Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

coolaj86 · 2019-04-10T07:23:09Z

I'm back on the scene. I'll see what I can do to get this stuff cleaned up and PR'd again.

coolaj86 · 2019-04-14T07:40:41Z

Rebased and ready for re-review!

adelowo · 2019-04-14T14:31:04Z

routers/routes/routes.go

@@ -273,8 +273,6 @@ func RegisterRoutes(m *macaron.Macaron) {
 		}, openIDSignInEnabled)
 		m.Get("/sign_up", user.SignUp)
 		m.Post("/sign_up", bindIgnErr(auth.RegisterForm{}), user.SignUpPost)
-		m.Get("/reset_password", user.ResetPasswd)


Do we need to atleast redirect to the new route before this is finally removed in 1.10?

@adelowo I don't see why as emails having old route won't be usable anyway as code is valid only for few hours

Ha true... stupid me

lafriks · 2019-04-14T20:28:31Z

@daviian need your approval

coolaj86 force-pushed the ux-current-user-password-reset branch 2 times, most recently from c483c86 to f8cf890 Compare October 7, 2018 20:18

bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 7, 2018

lafriks requested changes Oct 8, 2018

View reviewed changes

routers/user/auth.go Outdated Show resolved Hide resolved

lafriks added the type/enhancement An improvement of existing functionality label Oct 8, 2018

lafriks added this to the 1.7.0 milestone Oct 8, 2018

daviian suggested changes Oct 11, 2018

View reviewed changes

routers/routes/routes.go Outdated Show resolved Hide resolved

coolaj86 force-pushed the ux-current-user-password-reset branch from 4594723 to c8dcf9e Compare October 15, 2018 18:49

techknowlogick mentioned this pull request Nov 1, 2018

Rephrase password reset button labels #5240

Closed

lafriks mentioned this pull request Nov 8, 2018

allow current user to reset their own password #5034

Closed

8 tasks

techknowlogick modified the milestones: 1.7.0, 1.8.0 Dec 19, 2018

stale bot added the issue/stale label Feb 17, 2019

techknowlogick modified the milestones: 1.8.0, 1.9.0 Feb 19, 2019

stale bot removed the issue/stale label Feb 19, 2019

AJ ONeal added 6 commits April 14, 2019 07:18

allow current user to reset their own password

4e0b451

handle reset password edge cases properly and consistently

a93b4ef

remove dangling assignment

44f943e

properly label account recovery instead of reset password

87314f2

remove 'Click here' from button

e7a01b6

update English-only account-recovery templates

3839984

coolaj86 force-pushed the ux-current-user-password-reset branch from 9334293 to 3839984 Compare April 14, 2019 07:40

lafriks approved these changes Apr 14, 2019

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 14, 2019

adelowo approved these changes Apr 14, 2019

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 14, 2019

daviian approved these changes Apr 15, 2019

View reviewed changes

Merge branch 'master' into ux-current-user-password-reset

1688720

lafriks merged commit 6dbd261 into go-gitea:master Apr 18, 2019

alexanderadam mentioned this pull request Jul 19, 2019

How do I enable password reset ? #2247

Closed

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UX + Security current user password reset #5042

UX + Security current user password reset #5042

coolaj86 commented Oct 7, 2018 •

edited

Loading

codecov-io commented Oct 7, 2018 •

edited

Loading

daviian left a comment

coolaj86 commented Oct 15, 2018 •

edited

Loading

coolaj86 commented Oct 15, 2018

stale bot commented Feb 17, 2019

coolaj86 commented Apr 10, 2019

coolaj86 commented Apr 14, 2019

adelowo Apr 14, 2019

lafriks Apr 14, 2019

adelowo Apr 15, 2019

lafriks commented Apr 14, 2019

UX + Security current user password reset #5042

UX + Security current user password reset #5042

Conversation

coolaj86 commented Oct 7, 2018 • edited Loading

Default Condition

Logged In

Invalid Code

Invalid User

codecov-io commented Oct 7, 2018 • edited Loading

Codecov Report

daviian left a comment

Choose a reason for hiding this comment

coolaj86 commented Oct 15, 2018 • edited Loading

coolaj86 commented Oct 15, 2018

stale bot commented Feb 17, 2019

coolaj86 commented Apr 10, 2019

coolaj86 commented Apr 14, 2019

adelowo Apr 14, 2019

Choose a reason for hiding this comment

lafriks Apr 14, 2019

Choose a reason for hiding this comment

adelowo Apr 15, 2019

Choose a reason for hiding this comment

lafriks commented Apr 14, 2019

coolaj86 commented Oct 7, 2018 •

edited

Loading

codecov-io commented Oct 7, 2018 •

edited

Loading

coolaj86 commented Oct 15, 2018 •

edited

Loading