Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,10 @@ Layout/MultilineHashBraceLayout:
Layout/MultilineMethodCallBraceLayout:
Enabled: true

Layout/MultilineMethodCallIndentation:
Enabled: true
EnforcedStyle: indented

Layout/MultilineMethodDefinitionBraceLayout:
Enabled: true
EnforcedStyle: symmetrical
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/accounts/personal_keys_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def send_new_personal_key_notifications
end

telephony_responses = MfaContext.new(current_user).
phone_configurations.map do |phone_configuration|
phone_configurations.map do |phone_configuration|
phone = phone_configuration.phone
Telephony.send_personal_key_regeneration_notice(
to: phone,
Expand Down
22 changes: 15 additions & 7 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -317,15 +317,23 @@ def reauthn?

def confirm_two_factor_authenticated(id = nil)
return prompt_to_sign_in_with_request_id(id) if user_needs_new_session_with_request_id?(id)

authenticate_user!(force: true)
return prompt_to_setup_mfa unless two_factor_enabled?
return prompt_to_verify_mfa unless user_fully_authenticated?
return prompt_to_setup_mfa if service_provider_mfa_policy.
user_needs_sp_auth_method_setup?
return prompt_to_setup_non_restricted_mfa if two_factor_kantara_enabled?
return prompt_to_verify_sp_required_mfa if service_provider_mfa_policy.
user_needs_sp_auth_method_verification?

if !two_factor_enabled?
return prompt_to_setup_mfa
elsif !user_fully_authenticated?
return prompt_to_verify_mfa
elsif service_provider_mfa_policy.user_needs_sp_auth_method_setup?
return prompt_to_setup_mfa
elsif two_factor_kantara_enabled?
return prompt_to_setup_non_restricted_mfa
elsif service_provider_mfa_policy.user_needs_sp_auth_method_verification?
return prompt_to_verify_sp_required_mfa
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zachmargolis I'm on the fence here. I think this is better, but it doesn't look as good.

Also someone should double-check my logic since I just some unlesses into if !s and accidentally inverting logic here would probably be quite bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • I spot checked the unless -> if ! and that looks good
  • I think this if/else looks great


enforce_total_session_duration_timeout

true
end

Expand Down
5 changes: 3 additions & 2 deletions app/controllers/concerns/unconfirmed_user_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,9 @@ def stop_if_invalid_token
end

def email_confirmation_token_validator
@email_confirmation_token_validator ||= EmailConfirmationTokenValidator.
new(@email_address, current_user)
@email_confirmation_token_validator ||= begin
EmailConfirmationTokenValidator.new(@email_address, current_user)
end
end

def process_valid_confirmation_token
Expand Down
11 changes: 7 additions & 4 deletions app/controllers/openid_connect/authorization_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,12 @@ def check_sp_handoff_bounced

def confirm_user_is_authenticated_with_fresh_mfa
bump_auth_count unless user_fully_authenticated?
return confirm_two_factor_authenticated(request_id) unless user_fully_authenticated? &&
service_provider_mfa_policy.
auth_method_confirms_to_sp_request?

unless user_fully_authenticated? && service_provider_mfa_policy.
auth_method_confirms_to_sp_request?
return confirm_two_factor_authenticated(request_id)
end
Copy link
Contributor Author

@n1zyy n1zyy Aug 18, 2022

Choose a reason for hiding this comment

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

This is what rubocop made me do. I'm not sure I love this; the unless continuation is further indented than the block contents.

The good news is that the method looks less like a map of Oklahoma. (Which is surely a great state, just not the shape a block of code should take.)


redirect_to user_two_factor_authentication_url if device_not_remembered?
end

Expand Down Expand Up @@ -82,7 +85,7 @@ def profile_or_identity_needs_verification?

def track_authorize_analytics(result)
analytics_attributes = result.to_h.except(:redirect_uri).
merge(user_fully_authenticated: user_fully_authenticated?)
merge(user_fully_authenticated: user_fully_authenticated?)

analytics.openid_connect_request_authorization(**analytics_attributes)
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/saml_idp_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def confirm_user_is_authenticated_with_fresh_mfa
bump_auth_count unless user_fully_authenticated?
return confirm_two_factor_authenticated(request_id) unless user_fully_authenticated? &&
service_provider_mfa_policy.
auth_method_confirms_to_sp_request?
auth_method_confirms_to_sp_request?
redirect_to user_two_factor_authentication_url if remember_device_expired_for_sp?
end

Expand Down
8 changes: 6 additions & 2 deletions app/controllers/users/email_confirmations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,12 @@ def email_address
end

def email_confirmation_token_validator
@email_confirmation_token_validator ||= EmailConfirmationTokenValidator.
new(email_address, current_user)
@email_confirmation_token_validator ||= begin
EmailConfirmationTokenValidator.new(
email_address,
current_user,
)
end
end

def email_address_already_confirmed?
Expand Down
2 changes: 1 addition & 1 deletion app/decorators/user_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ def no_longer_locked_out?

def recent_events
events = Event.where(user_id: user.id).order('created_at DESC').limit(MAX_RECENT_EVENTS).
map(&:decorate)
map(&:decorate)
(events + identity_events).sort_by(&:happened_at).reverse
end

Expand Down
2 changes: 1 addition & 1 deletion app/forms/openid_connect_token_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ def find_identity_with_code
return if code.blank? || code.include?("\x00")

@identity = ServiceProviderIdentity.where(session_uuid: code).
order(updated_at: :desc).first
order(updated_at: :desc).first
end

def pkce?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def generate_sprints(ret, date, today_date)
start = date
finish = date.next_day(14)
ret << Db::DocAuthLog::BlanketDropOffRatesAllSpsInRange.new.
call('Sprint', fmt(start), fmt(finish))
call('Sprint', fmt(start), fmt(finish))
date = finish
end
end
Expand Down
24 changes: 12 additions & 12 deletions app/jobs/reports/doc_auth_drop_off_rates_report.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,62 +67,62 @@ def generate_overall_report_all_sps(ret)

def overall_drop_off_rates_all_sps_all_time(ret)
ret << Db::DocAuthLog::OverallDropOffRatesAllSpsAllTime.new.
call('Overall drop off rates for all SPs all time')
call('Overall drop off rates for all SPs all time')
end

def overall_drop_off_rates_all_sps_last_24_hours(ret)
ret << Db::DocAuthLog::OverallDropOffRatesAllSpsInRange.new.
call('Overall drop off rates for all SPs last 24 hours', Date.yesterday, today)
call('Overall drop off rates for all SPs last 24 hours', Date.yesterday, today)
end

def overall_drop_off_rates_all_sps_last_30_days(ret)
ret << Db::DocAuthLog::OverallDropOffRatesAllSpsInRange.new.
call('Overall drop off rates for all SPs last 30 days', today - 30.days, today)
call('Overall drop off rates for all SPs last 30 days', today - 30.days, today)
end

def blanket_drop_off_rates_all_sps_all_time(ret)
ret << Db::DocAuthLog::BlanketDropOffRatesAllSpsAllTime.new.
call('Blanket drop off rates for all SPs all time')
call('Blanket drop off rates for all SPs all time')
end

def blanket_drop_off_rates_all_sps_last_24_hours(ret)
ret << Db::DocAuthLog::BlanketDropOffRatesAllSpsInRange.new.
call('Blanket drop off rates for all SPs last 24 hours', Date.yesterday, today)
call('Blanket drop off rates for all SPs last 24 hours', Date.yesterday, today)
end

def blanket_drop_off_rates_all_sps_last_30_days(ret)
ret << Db::DocAuthLog::BlanketDropOffRatesAllSpsInRange.new.
call('Blanket drop off rates for all SPs last 30 days', today - 30.days, today)
call('Blanket drop off rates for all SPs last 30 days', today - 30.days, today)
end

def blanket_drop_off_rates_per_sp_all_time(ret, sp)
ret << Db::DocAuthLog::BlanketDropOffRatesPerSpAllTime.new.
call('Blanket drop off rates per SP all time', sp.issuer)
call('Blanket drop off rates per SP all time', sp.issuer)
end

def blanket_drop_off_rates_per_sp_last_24_hours(ret, sp)
ret << Db::DocAuthLog::BlanketDropOffRatesPerSpInRange.new.
call('Blanket drop off rates last 24 hours', sp.issuer, Date.yesterday, today)
call('Blanket drop off rates last 24 hours', sp.issuer, Date.yesterday, today)
end

def blanket_drop_off_rates_per_sp_last_30_days(ret, sp)
ret << Db::DocAuthLog::BlanketDropOffRatesPerSpInRange.new.
call('Blanket drop off rates last 30 days', sp.issuer, today - 30.days, today)
call('Blanket drop off rates last 30 days', sp.issuer, today - 30.days, today)
end

def overall_drop_off_rates_per_sp_all_time(ret, sp)
ret << Db::DocAuthLog::OverallDropOffRatesPerSpAllTime.new.
call('Overall drop off rates per SP all time', sp.issuer)
call('Overall drop off rates per SP all time', sp.issuer)
end

def overall_drop_off_rates_per_sp_last_24_hours(ret, sp)
ret << Db::DocAuthLog::OverallDropOffRatesPerSpInRange.new.
call('Overall drop off rates last 24 hours', sp.issuer, Date.yesterday, today)
call('Overall drop off rates last 24 hours', sp.issuer, Date.yesterday, today)
end

def overall_drop_off_rates_per_sp_last_30_days(ret, sp)
ret << Db::DocAuthLog::OverallDropOffRatesPerSpInRange.new.
call('Overall drop off rates last 30 days', sp.issuer, today - 30.days, today)
call('Overall drop off rates last 30 days', sp.issuer, today - 30.days, today)
end

def today
Expand Down
10 changes: 5 additions & 5 deletions app/models/in_person_enrollment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ class InPersonEnrollment < ApplicationRecord
# Find enrollments that need a status check via the USPS API
def self.needs_usps_status_check(check_interval)
where(status: :pending).
and(
where(status_check_attempted_at: check_interval).
or(where(status_check_attempted_at: nil)),
).
order(status_check_attempted_at: :asc)
and(
where(status_check_attempted_at: check_interval).
or(where(status_check_attempted_at: nil)),
).
order(status_check_attempted_at: :asc)
end

# Does this enrollment need a status check via the USPS API?
Expand Down
6 changes: 3 additions & 3 deletions app/services/idv/gpo_mail.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ def any_mail_sent?

def user_mail_events
@user_mail_events ||= current_user.events.
gpo_mail_sent.
order('updated_at DESC').
limit(MAX_MAIL_EVENTS)
gpo_mail_sent.
order('updated_at DESC').
limit(MAX_MAIL_EVENTS)
end

def max_events?
Expand Down
2 changes: 1 addition & 1 deletion config/initializers/devise.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
if auth.env['action_dispatch.cookies']
expected_cookie_value = "#{user.class}-#{user.id}"
actual_cookie_value = auth.env['action_dispatch.cookies'].
signed[TwoFactorAuthenticatable::REMEMBER_2FA_COOKIE]
signed[TwoFactorAuthenticatable::REMEMBER_2FA_COOKIE]
bypass_by_cookie = actual_cookie_value == expected_cookie_value
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def index; end

before do
allow(IdentityConfig.store).to receive(:proofing_device_profiling_collecting_enabled).
and_return(ff_enabled)
and_return(ff_enabled)
end

context 'ff is set' do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@
with(:mfa_verify_backup_code, success: false)

expect(@analytics).to receive(:track_event).
with('Multi-Factor Authentication: max attempts reached')
with('Multi-Factor Authentication: max attempts reached')
expect(PushNotification::HttpPush).to receive(:deliver).
with(PushNotification::MfaLimitAccountLockedEvent.new(user: subject.current_user))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@
with(properties)

expect(@analytics).to receive(:track_event).
with('Multi-Factor Authentication: max attempts reached')
with('Multi-Factor Authentication: max attempts reached')
expect(PushNotification::HttpPush).to receive(:deliver).
with(PushNotification::MfaLimitAccountLockedEvent.new(user: subject.current_user))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@
expect(@analytics).to receive(:track_mfa_submit_event).
with(properties)
expect(@analytics).to receive(:track_event).
with('Multi-Factor Authentication: max attempts reached')
with('Multi-Factor Authentication: max attempts reached')
expect(PushNotification::HttpPush).to receive(:deliver).
with(PushNotification::MfaLimitAccountLockedEvent.new(user: subject.current_user))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@
)

expect(@analytics).to receive(:track_event).
with('Multi-Factor Authentication: max attempts reached')
with('Multi-Factor Authentication: max attempts reached')
expect(PushNotification::HttpPush).to receive(:deliver).
with(PushNotification::MfaLimitAccountLockedEvent.new(user: subject.current_user))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@
expect(@analytics).to receive(:track_mfa_submit_event).
with(attributes)
expect(@analytics).to receive(:track_event).
with('Multi-Factor Authentication: max attempts reached')
with('Multi-Factor Authentication: max attempts reached')
expect(@irs_attempts_api_tracker).to receive(:track_event).
with(:mfa_verify_totp, success: false)
expect(PushNotification::HttpPush).to receive(:deliver).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,8 @@
let(:view_context) { ActionController::Base.new.view_context }
before do
allow_any_instance_of(TwoFactorAuthCode::WebauthnAuthenticationPresenter).
to receive(:multiple_factors_enabled?).
and_return(true)
to receive(:multiple_factors_enabled?).
and_return(true)
end

it 'redirects to webauthn show page' do
Expand All @@ -175,8 +175,8 @@
context 'User only has webauthn as an MFA method' do
before do
allow_any_instance_of(TwoFactorAuthCode::WebauthnAuthenticationPresenter).
to receive(:multiple_factors_enabled?).
and_return(false)
to receive(:multiple_factors_enabled?).
and_return(false)
end

it 'redirects to webauthn error page ' do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
let(:enforcement_date) { Time.zone.today + 6.days }
before do
allow(IdentityConfig.store).to receive(:kantara_restriction_enforcement_date).
and_return(enforcement_date)
and_return(enforcement_date)
end

context 'before enforcement date' do
Expand All @@ -55,7 +55,7 @@

user.reload
expect(user.non_restricted_mfa_required_prompt_skip_date).
to eq Time.zone.today
to eq Time.zone.today
end

it 'does not allow unauthenticated users' do
Expand Down
6 changes: 3 additions & 3 deletions spec/controllers/users/email_confirmations_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
)).ordered

expect(PushNotification::HttpPush).to receive(:deliver).once.
with(PushNotification::RecoveryInformationChangedEvent.new(
user: user,
)).ordered
with(PushNotification::RecoveryInformationChangedEvent.new(
user: user,
)).ordered

add_email_form = AddUserEmailForm.new
add_email_form.submit(user, email: new_email)
Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/users/rules_of_use_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@
it 'logs a failure analytics event' do
stub_analytics
expect(@analytics).to receive(:track_event).
with('Rules of Use Submitted', hash_including(success: false))
with('Rules of Use Submitted', hash_including(success: false))

action
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
subject
end
end.to change { @identity.reload.deleted_at&.to_i }.
from(nil).to(now.to_i)
from(nil).to(now.to_i)

expect(response).to redirect_to(account_connected_accounts_path)
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@
stub_attempts_tracker

expect(@irs_attempts_api_tracker).to receive(:track_event).
with(:mfa_enroll_options_selected, success: true,
mfa_device_types: ['voice', 'auth_app'])
with(:mfa_enroll_options_selected, success: true,
mfa_device_types: ['voice', 'auth_app'])

patch :create, params: {
two_factor_options_form: {
Expand Down
4 changes: 2 additions & 2 deletions spec/features/account/backup_codes_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@
click_continue

generated_at = user.backup_code_configurations.
order(created_at: :asc).first.created_at.
in_time_zone('UTC')
order(created_at: :asc).first.created_at.
in_time_zone('UTC')
formatted_generated_at = l(generated_at, format: t('time.formats.event_timestamp'))

expected_message = "#{t('account.index.backup_codes_exist')} #{formatted_generated_at}"
Expand Down
Loading