Skip to content

LG-10022: Avoid second MFA prompt for strict MFA requirement #9263

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.

Sign up for GitHub

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

Merged
aduth merged 1 commit into from
Sep 25, 2023
Merged

LG-10022: Avoid second MFA prompt for strict MFA requirement #9263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion app/controllers/concerns/second_mfa_reminder_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
module SecondMfaReminderConcern
def user_needs_second_mfa_reminder?
return false unless IdentityConfig.store.second_mfa_reminder_enabled
return false if user_has_dismissed_second_mfa_reminder? || user_has_multiple_mfa_methods?
return false if user_has_dismissed_second_mfa_reminder?
Copy link
Contributor

@jmdembe jmdembe Sep 25, 2023

Choose a reason for hiding this comment

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

if each conditional returns false, why not || operator instead of multiple return false statements?

Copy link
Contributor Author

@aduth aduth Sep 25, 2023

Choose a reason for hiding this comment

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

|| operator can work. It's a minor thing, but stylistically I felt the multiple returns are a little easier to scan as associated with the return value when split across multiple lines, though I preferred the || when it could be contained to a single line.

For example, in this code snippet, it's a little less obvious to me that user_has_multiple_mfa_methods? ties back to the return false, since it's so far apart.

return false if !IdentityConfig.store.second_mfa_reminder_enabled ||
  user_has_dismissed_second_mfa_reminder? ||
  second_mfa_enrollment_may_downgrade_for_service_provider_mfa_requirement? ||
  user_has_multiple_mfa_methods?

return false if second_mfa_enrollment_may_downgrade_for_service_provider_mfa_requirement?
return false if user_has_multiple_mfa_methods?
exceeded_sign_in_count_for_second_mfa_reminder? || exceeded_account_age_for_second_mfa_reminder?
end

private

def second_mfa_enrollment_may_downgrade_for_service_provider_mfa_requirement?
service_provider_mfa_policy.phishing_resistant_required? ||
service_provider_mfa_policy.piv_cac_required?
end

def user_has_dismissed_second_mfa_reminder?
current_user.second_mfa_reminder_dismissed_at.present?
end
Expand Down
56 changes: 42 additions & 14 deletions spec/controllers/concerns/second_mfa_reminder_concern_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,51 @@
Class.new do
include SecondMfaReminderConcern

attr_reader :current_user
attr_reader :current_user, :service_provider_mfa_policy

def initialize(current_user:)
def initialize(current_user:, service_provider_mfa_policy:)
@current_user = current_user
@service_provider_mfa_policy = service_provider_mfa_policy
end
end
end
let(:user) { build(:user) }
let(:instance) { test_class.new(current_user: user) }
let(:phishing_resistant_required) { false }
let(:piv_cac_required) { false }
let(:service_provider_mfa_policy) do
instance_double(
ServiceProviderMfaPolicy,
phishing_resistant_required?: phishing_resistant_required,
piv_cac_required?: piv_cac_required,
)
end
let(:instance) { test_class.new(current_user: user, service_provider_mfa_policy:) }

describe '#user_needs_second_mfa_reminder?' do
subject(:user_needs_second_mfa_reminder) { instance.user_needs_second_mfa_reminder? }

shared_examples 'second mfa reminder feature is disabled' do
before do
allow(IdentityConfig.store).to receive(:second_mfa_reminder_account_age_in_days).
and_return(10)
allow(IdentityConfig.store).to receive(:second_mfa_reminder_enabled).and_return(false)
end

it { expect(user_needs_second_mfa_reminder).to eq(false) }
end

shared_examples 'second mfa reminder with phishing-resistant required request' do
let(:phishing_resistant_required) { true }

it { expect(user_needs_second_mfa_reminder).to eq(false) }
end

shared_examples 'second mfa reminder with piv required request' do
let(:piv_cac_required) { true }

it { expect(user_needs_second_mfa_reminder).to eq(false) }
end

context 'user has already dismissed second mfa reminder' do
let(:user) { build(:user, second_mfa_reminder_dismissed_at: Time.zone.now) }

Expand Down Expand Up @@ -47,6 +79,10 @@ def initialize(current_user:)
end

it { expect(user_needs_second_mfa_reminder).to eq(true) }

it_behaves_like 'second mfa reminder feature is disabled'
it_behaves_like 'second mfa reminder with phishing-resistant required request'
it_behaves_like 'second mfa reminder with piv required request'
end

context 'user has exceeded account age threshold for reminder' do
Expand All @@ -58,18 +94,10 @@ def initialize(current_user:)
end

it { expect(user_needs_second_mfa_reminder).to eq(true) }
end

context 'user meets threshold but feature is disabled' do
let(:user) { build(:user, :with_phone, created_at: 11.days.ago) }

before do
allow(IdentityConfig.store).to receive(:second_mfa_reminder_account_age_in_days).
and_return(10)
allow(IdentityConfig.store).to receive(:second_mfa_reminder_enabled).and_return(false)
end

it { expect(user_needs_second_mfa_reminder).to eq(false) }
it_behaves_like 'second mfa reminder feature is disabled'
it_behaves_like 'second mfa reminder with phishing-resistant required request'
it_behaves_like 'second mfa reminder with piv required request'
end
end
end
Expand Down