[WIP] Reencrypt pii on pw change #518
Conversation
**Why**: Multi-factor authentication requires confirmation of all factors before changing any single factor. **How**: Implement a ?reauthn=true param on top of the usual MFA flow, and protect all the relevant controllers with `confirm_recently_authenticated` before action. An authentication window (default 30 seconds) is defined, within which changes can be made w/o requiring re-authentication. This allows the user to log in and immediately change factor(s) without needing to immediately re-authenticate.
**Why**: PII is encrypted with the user password. When the password changes, so must the encryption.
|
|
||
| def recently_authenticated? | ||
| return false unless user_session.present? | ||
| authn_at = user_session[:authn_at] |
There was a problem hiding this comment.
throw this into a private method so recently_authenticated? is a tad shorter / easier to read?
| class ReauthnRequiredController < ApplicationController | ||
| before_action :confirm_recently_authenticated | ||
|
|
||
| def recently_authenticated? |
There was a problem hiding this comment.
since this is only used within this controller, make it a private method?
| end | ||
|
|
||
| def handle_success | ||
| re_encrypt_active_profile | ||
|
|
There was a problem hiding this comment.
I am interested in why are are adding extra newlines like this to methods in some parts of this app. Is there a rule of thumb for when are are doing it?
| @@ -8,6 +8,8 @@ p.mt-tiny.mb0#2fa-description | |||
| url: otp_send_path, | |||
| method: :get, | |||
| role: 'form', class: 'mt3') do |f| | |||
| - if reauthn? | |||
| = f.input :reauthn, as: :hidden, input_html: { value: 'true' } | |||
There was a problem hiding this comment.
alternatively, we could always have this hidden field and have the value be reauthn? Then we wouldn't have to check for reauthn.present? in the controller, just for its value.
|
|
||
| context 'authenticated outside the authn window' do | ||
| before do | ||
| controller.user_session[:authn_at] -= 60 |
There was a problem hiding this comment.
60 feels like a magic number here -- why did you pick this val? Perhaps putting it in a var would clarify
| allow(@user_decorator).to receive(:lockout_time_remaining_in_words).and_return('1000 years') | ||
| user_decorator = instance_double(UserDecorator) | ||
| allow(view).to receive(:decorated_user).and_return(user_decorator) | ||
| allow(user_decorator).to receive(:lockout_time_remaining_in_words).and_return('1000 years') |
There was a problem hiding this comment.
ha what!! where does 1000 years come from?
|
will re-create when #599 is merged. |
DO NOT MERGE
Requires reconciliation of #498 and #507