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

Introduce a "confirm identity" tab #5071

Merged
merged 2 commits into from Feb 11, 2020
Merged

Conversation

@viroulep
Copy link
Member

viroulep commented Jan 19, 2020

Currently when changing email or password we request the user to provide their password.
However it would make sense to also do the same to display/edit the 2FA settings (eg: to avoid someone accidentally letting their session open to be locked out of their account because someone else would enable 2FA).

So instead of just requiring password on all of these tabs we can introduce a "recently authenticated" concept, where we would require the user to have provided their password (or 2FA if activated) authentication within the last 10 minutes (arbitrary time frame that I picked) to be able to edit their data.

Here is how it looks when trying to edit its password without having fulfilled a recent check:
2fa_check

In my opinion this is slightly better than providing the password for all changes.

Happy to hear @jfly or @lgarron thoughts on this.

@viroulep viroulep force-pushed the viroulep:recent_2fa_check branch 2 times, most recently from 3e01714 to 7cc6bca Jan 19, 2020
@viroulep viroulep mentioned this pull request Jan 25, 2020
3 of 3 tasks complete
@viroulep viroulep force-pushed the viroulep:recent_2fa_check branch from 7cc6bca to eab5bfc Jan 26, 2020
@viroulep

This comment has been minimized.

Copy link
Member Author

viroulep commented Feb 2, 2020

Friendly ping @jfly @lgarron @jonatanklosko.
Setting aside the actual code, what do you think of the general idea of having the concept of "recent authentication confirmation" to unlock the edition of sensitive user information?
(I believe that's what github does, but I'm unsure of the actual timing)

Copy link
Member

jonatanklosko left a comment

I like this idea! It's cleaner and less redundant than being careful to always check the password when necessary.

WcaOnRails/spec/models/user_spec.rb Outdated Show resolved Hide resolved
WcaOnRails/spec/requests/users_spec.rb Show resolved Hide resolved
@viroulep viroulep force-pushed the viroulep:recent_2fa_check branch from eab5bfc to 016cdbc Feb 10, 2020
@viroulep viroulep force-pushed the viroulep:recent_2fa_check branch from 016cdbc to 48c8c6d Feb 10, 2020
@viroulep viroulep merged commit e513974 into thewca:master Feb 11, 2020
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-0.008%) to 96.079%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@viroulep viroulep deleted the viroulep:recent_2fa_check branch Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.