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
17 changes: 3 additions & 14 deletions app/decorators/user_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,8 @@ def visible_email_addresses
end
end

def lockout_time_remaining_in_words
current_time = Time.zone.now

distance_of_time_in_words(
current_time,
current_time + lockout_time_remaining,
true,
highest_measures: 2,
)
end

def lockout_time_remaining
(lockout_period - (Time.zone.now - user.second_factor_locked_at)).to_i
def lockout_time_expiration
user.second_factor_locked_at + lockout_period
end

def active_identity_for(service_provider)
Expand Down Expand Up @@ -150,6 +139,6 @@ def lockout_period_config
end

def lockout_period_expired?
(Time.zone.now - user.second_factor_locked_at) > lockout_period
lockout_time_expiration < Time.zone.now
end
end
4 changes: 1 addition & 3 deletions app/presenters/failure_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,9 @@ def title; end

def header; end

def description; end
def description(_view_context); end

def troubleshooting_options
[]
end

def js; end
end
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ class MaxAttemptsReachedPresenter < FailurePresenter

attr_reader :type, :decorated_user

COUNTDOWN_ID = 'countdown'.freeze

def initialize(type, decorated_user)
super(:locked)
@type = type
Expand All @@ -21,23 +19,14 @@ def header
t('titles.account_locked')
end

def description
[locked_reason, please_try_again]
def description(view_context)
[locked_reason, please_try_again(view_context)]
end

def troubleshooting_options
[read_about_two_factor_authentication, contact_support]
end

def js
<<~JS
document.addEventListener('DOMContentLoaded', function() {
var test = #{decorated_user.lockout_time_remaining} * 1000;
window.LoginGov.countdownTimer(document.getElementById('#{COUNTDOWN_ID}'), test);
});
JS
end

def locked_reason
case type.to_s
when 'backup_code_login_attempts'
Expand All @@ -57,11 +46,12 @@ def locked_reason
end
end

def please_try_again
def please_try_again(view_context)
t(
'two_factor_authentication.please_try_again_html',
id: COUNTDOWN_ID,
time_remaining: decorated_user.lockout_time_remaining_in_words,
countdown: view_context.render(
CountdownComponent.new(expiration: decorated_user.lockout_time_expiration),
),
)
end

Expand Down
6 changes: 1 addition & 5 deletions app/views/shared/_failure.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<%= render PageHeadingComponent.new.with_content(presenter.header) %>
<% end %>

<% Array(presenter.description).each do |description_p| %>
<% Array(presenter.description(self)).each do |description_p| %>
Copy link
Copy Markdown
Contributor Author

@aduth aduth Mar 31, 2022

Choose a reason for hiding this comment

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

I don't like it, and we encountered it once before in #6023 (comment), but as noted in extended commit description of dcef566 and original pull request comment text, we can't rely on view_context remaining constant between the time of presenter initialization and view rendering (source, related issue). The value of view_context in a constructed presenter may receive scripts to enqueue associated with the component, but those scripts would not be the same as rendered into the template. We sorta avoided this in #6023 because the presenter was constructed inside a helper, so view rendering had already begun, and the render-bound view_context was available.

I think an ideal long-term solution is to remove these presenters and views altogether, then create a shared "Failure" component that can take the place of this view and the idv/shared/_error.html.erb partial, then each failure type can have a dedicated view that calls directly to the component.

e.g.

<%# app/views/shared/_otp_max_attempts %>

<%= render ErrorPageComponent.new(...) %>

<p><%= description_p %></p>
<% end %>

Expand All @@ -15,7 +15,3 @@
heading: t('components.troubleshooting_options.default_heading'),
options: presenter.troubleshooting_options,
) %>

<% if presenter.js %>
<%= backwards_compatible_javascript_tag presenter.js %>
<% end %>
2 changes: 1 addition & 1 deletion config/locales/two_factor_authentication/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ en:
question: Don’t have your PIV or CAC available?
piv_cac_header_text: Present your PIV/CAC
piv_cac_webauthn_available: Use your security key
please_try_again_html: Please try again in <strong id=%{id}>%{time_remaining}</strong>.
please_try_again_html: Please try again in <strong>%{countdown}</strong>.
read_about_two_factor_authentication: Read about two-factor authentication
totp_fallback:
question: Don’t have your authenticator app?
Expand Down
2 changes: 1 addition & 1 deletion config/locales/two_factor_authentication/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ es:
question: '¿No tienes su PIV o CAC disponibles?'
piv_cac_header_text: Presenta tu PIV/CAC
piv_cac_webauthn_available: Utilice su llave de seguridad
please_try_again_html: Inténtelo de nuevo en <strong id=%{id}>%{time_remaining}</strong>.
please_try_again_html: Inténtelo de nuevo en <strong>%{countdown}</strong>.
read_about_two_factor_authentication: Conozca la autenticación de dos factores
totp_fallback:
question: '¿No tiene su aplicación de autenticación?'
Expand Down
3 changes: 1 addition & 2 deletions config/locales/two_factor_authentication/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,7 @@ fr:
question: Votre PIV ou CAC n’est pas disponible?
piv_cac_header_text: Veuillez présenter votre carte PIV/CAC
piv_cac_webauthn_available: Utilisez votre clé de sécurité
please_try_again_html: Veuillez essayer de nouveau dans <strong
id=%{id}>%{time_remaining}</strong>.
please_try_again_html: Veuillez essayer de nouveau dans <strong>%{countdown}</strong>.
read_about_two_factor_authentication: En savoir plus sur l’authentification à deux facteurs
totp_fallback:
question: Vous n’avez pas votre application d’authentification?
Expand Down
73 changes: 57 additions & 16 deletions spec/decorators/user_decorator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,27 +66,14 @@
end
end

describe '#lockout_time_remaining' do
it 'returns the difference in seconds between otp drift and second_factor_locked_at' do
describe '#lockout_time_expiration' do
it 'returns the time at which lockout will expire' do
freeze_time do
user = build_stubbed(:user, second_factor_locked_at: Time.zone.now - 180)
user_decorator = UserDecorator.new(user)
allow(IdentityConfig.store).to receive(:lockout_period_in_minutes).and_return(8)

expect(user_decorator.lockout_time_remaining).to eq 300
end
end
end

describe '#lockout_time_remaining_in_words' do
it 'converts lockout_time_remaining to words representing minutes and seconds left' do
freeze_time do
user = build_stubbed(:user, second_factor_locked_at: Time.zone.now - 181)
user_decorator = UserDecorator.new(user)
allow(IdentityConfig.store).to receive(:lockout_period_in_minutes).and_return(8)

expect(user_decorator.lockout_time_remaining_in_words).
to eq '4 minutes and 59 seconds'
expect(user_decorator.lockout_time_expiration).to eq Time.zone.now + 300
end
end
end
Expand Down Expand Up @@ -206,6 +193,60 @@
end
end

describe '#locked_out?' do
let(:locked_at) { nil }
let(:user) { User.new }

before { allow(user).to receive(:second_factor_locked_at).and_return(locked_at) }

around do |ex|
freeze_time { ex.run }
end

subject(:locked_out?) { UserDecorator.new(user).locked_out? }

it { expect(locked_out?).to eq(false) }

context 'second factor locked out recently' do
let(:locked_at) { Time.zone.now }

it { expect(locked_out?).to eq(true) }
end

context 'second factor locked out a while ago' do
let(:locked_at) { Time.zone.now - UserDecorator::DEFAULT_LOCKOUT_PERIOD - 1.second }

it { expect(locked_out?).to eq(false) }
end
end

describe '#no_longer_locked_out?' do
let(:locked_at) { nil }
let(:user) { User.new }

before { allow(user).to receive(:second_factor_locked_at).and_return(locked_at) }

around do |ex|
freeze_time { ex.run }
end

subject(:no_longer_locked_out?) { UserDecorator.new(user).no_longer_locked_out? }

it { expect(no_longer_locked_out?).to eq(false) }

context 'second factor locked out recently' do
let(:locked_at) { Time.zone.now }

it { expect(no_longer_locked_out?).to eq(false) }
end

context 'second factor locked out a while ago' do
let(:locked_at) { Time.zone.now - UserDecorator::DEFAULT_LOCKOUT_PERIOD - 1.second }

it { expect(no_longer_locked_out?).to eq(true) }
end
end

describe '#recent_events' do
let!(:user) { create(:user, :signed_up, created_at: Time.zone.now - 100.days) }
let(:decorated_user) { user.decorate }
Expand Down
14 changes: 14 additions & 0 deletions spec/features/two_factor_authentication/sign_in_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -488,4 +488,18 @@ def attempt_to_bypass_2fa
expect(current_url).to eq MarketingSite.security_and_privacy_practices_url
end
end

describe 'second factor locked' do
before do
allow_any_instance_of(UserDecorator).to receive(:locked_out?).and_return(true)
allow_any_instance_of(UserDecorator).to receive(:lockout_time_expiration).
and_return(Time.zone.now + 10.minutes)
end

scenario 'presents the failure screen', :js do
sign_in_user(user_with_2fa)

expect(page).to have_content t('titles.account_locked')
end
end
end
8 changes: 7 additions & 1 deletion spec/presenters/failure_presenter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,19 @@
end

context 'methods with default values of `nil`' do
%i[title header description].each do |method|
%i[title header].each do |method|
describe "##{method}" do
subject { presenter.send(method) }

it { is_expected.to be_nil }
end
end

describe '#description' do
subject { presenter.description(ActionController::Base.new.view_context) }

it { is_expected.to be_nil }
end
end

describe '#troubleshooting_options' do
Expand Down
24 changes: 17 additions & 7 deletions spec/presenters/max_attempts_reached_presenter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,13 @@
describe TwoFactorAuthCode::MaxAttemptsReachedPresenter do
let(:type) { 'otp_requests' }
let(:decorated_user) { mock_decorated_user }
let(:view_context) { ActionController::Base.new.view_context }
let(:presenter) { described_class.new(type, decorated_user) }

around do |ex|
freeze_time { ex.run }
end

describe 'it uses the :locked failure state' do
subject { presenter.state }

Expand All @@ -24,23 +29,29 @@
end

context 'methods are overriden' do
%i[title header description js].each do |method|
%i[title header].each do |method|
describe "##{method}" do
subject { presenter.send(method) }

it { is_expected.to_not be_nil }
end
end

describe '#description' do
subject { presenter.description(view_context) }

it { is_expected.to_not be_nil }
end
end

describe '#description' do
subject(:description) { presenter.description }
subject(:description) { presenter.description(view_context) }

it 'includes failure type and time remaining' do
expect(subject).to eq(
[
presenter.locked_reason,
presenter.please_try_again,
presenter.please_try_again(view_context),
],
)
end
Expand Down Expand Up @@ -76,10 +87,10 @@
end

describe '#please_try_again' do
subject { presenter.send(:please_try_again) }
subject { presenter.please_try_again(view_context) }

it 'includes time remaining' do
expect(subject).to include('1000 years')
expect(subject).to include('1 minute')
end
end

Expand Down Expand Up @@ -109,8 +120,7 @@

def mock_decorated_user
decorated_user = instance_double(UserDecorator)
allow(decorated_user).to receive(:lockout_time_remaining_in_words).and_return('1000 years')
allow(decorated_user).to receive(:lockout_time_remaining).and_return(10_000)
allow(decorated_user).to receive(:lockout_time_expiration).and_return(Time.zone.now + 1.minute)
decorated_user
end
end
3 changes: 0 additions & 3 deletions spec/views/shared/_failure.html.erb_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,11 @@
let(:header) { 'header' }
let(:description) { 'description' }
let(:troubleshooting_options) { [{ text: 'option', url: 'https://example.com' }] }
let(:js) { '/* */' }

before do
allow(presenter).to receive(:header).and_return(header)
allow(presenter).to receive(:description).and_return(description)
allow(presenter).to receive(:troubleshooting_options).and_return(troubleshooting_options)
allow(presenter).to receive(:js).and_return(js)
end

it 'renders content' do
Expand All @@ -34,7 +32,6 @@
troubleshooting_options[0][:text],
href: troubleshooting_options[0][:url],
)
expect(rendered).to have_css('script', visible: :all)
end

context 'with array description' do
Expand Down