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

[WIP] Reencrypt pii on pw change #518

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions .reek
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ FeatureEnvy:
- track_registration
- append_info_to_payload
- generate_slo_request
- reauthn?
IrresponsibleModule:
enabled: false
NilCheck:
Expand Down Expand Up @@ -51,6 +52,7 @@ UtilityFunction:
- complete_idv_session
DuplicateMethodCall:
exclude:
- complete_2fa_confirmation
- complete_idv_profile
- stub_subject
- stub_idv_session
Expand Down
13 changes: 11 additions & 2 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class ApplicationController < ActionController::Base
rescue_from ActionController::InvalidAuthenticityToken,
with: :invalid_auth_token

helper_method :user_fully_authenticated?, :generate_warning, :read_help_content
helper_method :decorated_user, :reauthn?

prepend_before_action :session_expires_at
after_action :track_get_requests
Expand Down Expand Up @@ -56,6 +56,15 @@ def render_401
render file: 'public/401.html', status: 401
end

def reauthn_param
params[:reauthn]
end

def reauthn?
reauthn = reauthn_param
reauthn.present? && reauthn == 'true'
end

def invalid_auth_token
analytics.track_event('InvalidAuthenticityToken')
sign_out
Expand All @@ -64,7 +73,7 @@ def invalid_auth_token
end

def user_fully_authenticated?
user_signed_in? && current_user.two_factor_enabled? && is_fully_authenticated?
!reauthn? && user_signed_in? && current_user.two_factor_enabled? && is_fully_authenticated?
end

def confirm_two_factor_authenticated
Expand Down
18 changes: 9 additions & 9 deletions app/controllers/concerns/two_factor_authenticatable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ module TwoFactorAuthenticatable
private

def verify_user_is_not_second_factor_locked
handle_second_factor_locked_user if user_decorator.blocked_from_entering_2fa_code?
handle_second_factor_locked_user if decorated_user.blocked_from_entering_2fa_code?
end

def handle_second_factor_locked_user
Expand All @@ -28,16 +28,20 @@ def check_already_authenticated
end

def reset_attempt_count_if_user_no_longer_locked_out
return unless user_decorator.no_longer_blocked_from_entering_2fa_code?
return unless decorated_user.no_longer_blocked_from_entering_2fa_code?

current_user.update(second_factor_attempts_count: 0, second_factor_locked_at: nil)
end

def handle_valid_otp
def mark_user_session_authenticated
user_session[TwoFactorAuthentication::NEED_AUTHENTICATION] = false
user_session[:authn_at] = Time.zone.now
end

def handle_valid_otp
mark_user_session_authenticated
bypass_sign_in current_user
flash[:notice] = t('devise.two_factor_authentication.success')
flash[:notice] = t('devise.two_factor_authentication.success') unless reauthn?

analytics.track_event('User 2FA successful')

Expand All @@ -56,7 +60,7 @@ def handle_invalid_otp(type: 'otp')

flash[:error] = t("devise.two_factor_authentication.invalid_#{type}")

if user_decorator.blocked_from_entering_2fa_code?
if decorated_user.blocked_from_entering_2fa_code?
handle_second_factor_locked_user
else
render :show
Expand All @@ -70,10 +74,6 @@ def update_invalid_user
current_user.save
end

def user_decorator
@user_decorator ||= current_user.decorate
end

def after_2fa_path
if decorated_user.should_acknowledge_recovery_code?(session)
settings_recovery_code_url
Expand Down
11 changes: 8 additions & 3 deletions app/controllers/devise/two_factor_authentication_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def show
if current_user.totp_enabled?
redirect_to login_two_factor_authenticator_path
else
@phone_number = user_decorator.masked_two_factor_phone_number
@phone_number = decorated_user.masked_two_factor_phone_number
@otp_delivery_selection_form = OtpDeliverySelectionForm.new
end
end
Expand All @@ -23,17 +23,22 @@ def send_code
if result[:success?]
handle_valid_delivery_method(delivery_params[:otp_method])
else
redirect_to user_two_factor_authentication_path
redirect_to user_two_factor_authentication_path(reauthn: reauthn?)
end
end

private

def reauthn_param
otp_form = params.permit(otp_delivery_selection_form: [:reauthn])
super || otp_form.dig(:otp_delivery_selection_form, :reauthn)
end

def handle_valid_delivery_method(method)
send_user_otp(method)

flash[:success] = t("notices.send_code.#{method}")
redirect_to login_two_factor_path(delivery_method: method)
redirect_to login_two_factor_path(delivery_method: method, reauthn: reauthn?)
end

def send_user_otp(method)
Expand Down
18 changes: 18 additions & 0 deletions app/controllers/reauthn_required_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
class ReauthnRequiredController < ApplicationController
before_action :confirm_recently_authenticated

def recently_authenticated?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is only used within this controller, make it a private method?

return false unless user_session.present?
authn_at = user_session[:authn_at]
Copy link
Contributor

@jessieay jessieay Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw this into a private method so recently_authenticated? is a tad shorter / easier to read?

return false unless authn_at.present?
authn_at > Time.zone.now - Figaro.env.reauthn_window.to_i
end

def confirm_recently_authenticated
@reauthn = reauthn?
return unless user_signed_in?
return if recently_authenticated?
store_location_for(:user, request.url)
redirect_to user_password_confirm_url
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ class OtpVerificationController < DeviseController
include TwoFactorAuthenticatable

def show
@phone_number = user_decorator.masked_two_factor_phone_number
@phone_number = decorated_user.masked_two_factor_phone_number
@code_value = current_user.direct_otp if FeatureManagement.prefill_otp_codes?
@delivery_method = params[:delivery_method]
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/users/edit_email_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
module Users
class EditEmailController < ApplicationController
class EditEmailController < ReauthnRequiredController
before_action :confirm_two_factor_authenticated

def edit
Expand Down
15 changes: 13 additions & 2 deletions app/controllers/users/edit_password_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
module Users
class EditPasswordController < ApplicationController
class EditPasswordController < ReauthnRequiredController
before_action :confirm_two_factor_authenticated

def edit
Expand All @@ -23,15 +23,26 @@ def update
private

def user_params
params.require(:update_user_password_form).permit(:password, :current_password)
params.require(:update_user_password_form).permit(:password)
end

def handle_success
re_encrypt_active_profile

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

bypass_sign_in current_user

redirect_to profile_url, notice: t('notices.password_changed')

EmailNotifier.new(current_user).send_password_changed_email
end

def re_encrypt_active_profile
active_profile = current_user.active_profile
return unless active_profile.present?
cacher = Pii::Cacher.new(current_user, user_session)
pii = cacher.fetch
active_profile.encrypt_pii(user_params[:password], pii)
active_profile.save!
end
end
end
2 changes: 1 addition & 1 deletion app/controllers/users/edit_phone_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
module Users
class EditPhoneController < ApplicationController
class EditPhoneController < ReauthnRequiredController
include PhoneConfirmation

before_action :confirm_two_factor_authenticated
Expand Down
19 changes: 19 additions & 0 deletions app/controllers/users/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,25 @@ class SessionsController < Devise::SessionsController

skip_before_action :session_expires_at, only: [:active]
skip_after_action :track_get_requests, only: [:active]
before_action :confirm_two_factor_authenticated, only: [:update]

def create
track_authentication_attempt(params[:user][:email])
super
end

def show
end

def update
if current_user.valid_password?(password)
redirect_to user_two_factor_authentication_path(reauthn: true)
else
flash[:error] = t('errors.confirm_password_incorrect')
redirect_to user_password_confirm_path
end
end

def active
response.headers['Etag'] = '' # clear etags to prevent caching
session[:pinged_at] = now
Expand Down Expand Up @@ -50,5 +63,11 @@ def track_authentication_attempt(email)

analytics.track_event('Authentication Attempt with nonexistent user')
end

def password
params.require(:user)[:password]
rescue ActionController::ParameterMissing
''
end
end
end
14 changes: 3 additions & 11 deletions app/forms/update_user_password_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,16 @@ class UpdateUserPasswordForm
include ActiveModel::Model
include FormPasswordValidator

validates :current_password, presence: true
validate :verify_current_password
validates :password, presence: true

def initialize(user)
@user = user
end

def submit(params)
self.password = params[:password]
self.current_password = params[:current_password]

@success = valid? && user.update_with_password(params)
@success = valid? && user.update(params)

result
end
Expand All @@ -22,13 +20,7 @@ def submit(params)

attr_reader :user, :success

attr_accessor :password, :current_password

def verify_current_password
return if user.valid_password?(current_password)

errors.add(:current_password, I18n.t('errors.incorrect_password'))
end
attr_accessor :password

def result
{
Expand Down
8 changes: 8 additions & 0 deletions app/views/devise/sessions/show.html.slim
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
- title t('titles.passwords.confirm')

h1.heading = t('headings.passwords.confirm')
= simple_form_for(current_user,
url: reauthn_user_password_path,
html: { autocomplete: 'off', role: 'form', method: 'put' }) do |f|
= f.input :password, required: true
= f.button :submit, t('headings.passwords.confirm'), class: 'btn-wide mt2 mb4'
2 changes: 2 additions & 0 deletions app/views/devise/two_factor_authentication/show.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -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' }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.mb3
= label_tag 'otp_method',
t('devise.two_factor_authentication.otp_method.title'),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ h1.heading = t('devise.two_factor_authentication.header_text')
p.mt-tiny.mb0 == t('instructions.2fa.confirm_code', number: "<strong>#{@phone_number}</strong>")

= form_tag(:login_otp, method: :post, role: 'form', class: 'mt4') do
= hidden_field_tag(:reauthn, reauthn?)
= label_tag 'code', \
raw(t('simple_form.required.html')) + t('forms.two_factor.code'), \
class: 'block bold'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@
h1.heading = t('titles.account_locked')
p = t('devise.two_factor_authentication.max_login_attempts_reached')
p = t('devise.two_factor_authentication.please_try_again',
time_remaining: @user_decorator.lockout_time_remaining_in_words)
time_remaining: decorated_user.lockout_time_remaining_in_words)
3 changes: 1 addition & 2 deletions app/views/users/edit_password/edit.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,10 @@ p.mt-tiny.mb0#password-description
= simple_form_for(@update_user_password_form, url: settings_password_path,
html: { autocomplete: 'off', method: :patch, role: 'form' }) do |f|
= f.error_notification
= f.input :current_password, required: true
= f.input :password, label: t('forms.passwords.edit.labels.password'), required: true,
input_html: { 'aria-describedby': 'password-description' }
= render 'devise/shared/password_strength'
= f.button :submit, t('forms.buttons.submit.update'), class: 'mt2'
.mt1 = link_to t('forms.buttons.cancel'), :back, class: 'underline'
.mt1 = link_to t('forms.buttons.cancel'), profile_path, class: 'underline'

== javascript_include_tag 'misc/pw-strength'
2 changes: 1 addition & 1 deletion app/views/users/edit_phone/edit.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ h1.heading = t('headings.edit_info.phone')
= f.error_notification
= f.input :phone, as: :tel, required: true
= f.button :submit, t('forms.buttons.submit.update'), class: 'mt2'
.mt1 = link_to t('forms.buttons.cancel'), :back, class: 'underline'
.mt1 = link_to t('forms.buttons.cancel'), profile_path, class: 'underline'
3 changes: 3 additions & 0 deletions config/application.yml.example
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ development:
proofing_kbv: 'false'
proofing_vendors: 'mock'
rack_mini_profiler: 'off'
reauthn_window: '30'
redis_url: 'redis://localhost:6379/0'
requests_per_ip_limit: '300'
requests_per_ip_period: '300'
Expand All @@ -72,6 +73,7 @@ production:
participate_in_dap: 'no'
proxy_addr: '123.456.789'
proxy_port: '80'
reauthn_window: '30'
redis_url: 'redis://localhost:6379/0'
requests_per_ip_limit: '300'
requests_per_ip_period: '300'
Expand All @@ -96,6 +98,7 @@ test:
otp_delivery_blocklist_maxretry: '2'
proofing_kbv: 'false'
proofing_vendors: 'mock'
reauthn_window: '30'
redis_url: 'redis://localhost:6379/0'
requests_per_ip_limit: '3'
requests_per_ip_period: '60'
Expand Down
2 changes: 1 addition & 1 deletion config/initializers/figaro.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
Figaro.require_keys(
'allow_third_party_auth', 'domain_name', 'enable_test_routes', 'idp_sso_target_url',
'logins_per_ip_limit', 'logins_per_ip_period', 'otp_delivery_blocklist_bantime',
'otp_delivery_blocklist_findtime', 'otp_delivery_blocklist_maxretry',
'otp_delivery_blocklist_findtime', 'otp_delivery_blocklist_maxretry', 'reauthn_window',
'recovery_code_length', 'requests_per_ip_limit', 'requests_per_ip_period',
'saml_passphrase', 'secret_key_base', 'session_timeout_in', 'support_email',
'twilio_accounts', 'valid_service_providers'
Expand Down
4 changes: 3 additions & 1 deletion config/locales/en.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
en:
errors:
incorrect_password: is incorrect
confirm_password_incorrect: The password you provided is incorrect.
invalid_totp: Invalid code entered. Please try again.
not_authorized: You are not authorized to perform this action.
invalid_authenticity_token: Oops, something went wrong. Please sign in again.
Expand Down Expand Up @@ -147,6 +147,7 @@ en:
passwords:
change: Change the password for your account
forgot: Reset the password for your account
confirm: Confirm the password for your account
profile: Profile
recovery_code: Just in case
registrations:
Expand Down Expand Up @@ -181,6 +182,7 @@ en:
passwords:
change: Change your password
forgot: Forgot password
confirm: Confirm your password
profile:
agencies: Agencies I've logged into
profile_info: Profile information
Expand Down