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
5 changes: 5 additions & 0 deletions app/assets/javascripts/app/utils/auto-logout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export default () => {
window.onbeforeunload = null;
window.onunload = null;
window.location.href = '/timeout';
};
21 changes: 21 additions & 0 deletions app/assets/javascripts/app/utils/countdown-timer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import msFormatter from './ms-formatter';
import autoLogout from './auto-logout';

export default (targetSelector, timeLeft = 0, interval = 1000) => {
const countdownTarget = document.querySelector(targetSelector);
let remaining = timeLeft;

if (!countdownTarget) return;

(function tick() {
countdownTarget.innerHTML = msFormatter(remaining);

if (remaining <= 0) {
autoLogout();
return;
}

remaining -= interval;
setTimeout(tick, interval);
}());
};
9 changes: 9 additions & 0 deletions app/assets/javascripts/app/utils/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import autoLogout from './auto-logout';
import countdownTimer from './countdown-timer';
import msFormatter from './ms-formatter';

const LoginGov = window.LoginGov = (window.LoginGov || {});

LoginGov.autoLogout = autoLogout;
LoginGov.countdownTimer = countdownTimer;
LoginGov.msFormatter = msFormatter;
24 changes: 24 additions & 0 deletions app/assets/javascripts/app/utils/ms-formatter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
function pluralize(word, count) {
return `${word}${count !== 1 ? 's' : ''}`;
}

function formatMinutes(minutes) {
if (!minutes) return 0;

return `${minutes} ${pluralize('minute', minutes)}`;
}

function formatSeconds(seconds) {
return `${seconds} ${pluralize('second', seconds)}`;
}

export default (milliseconds) => {
const seconds = milliseconds / 1000;
const minutes = parseInt(seconds / 60, 10);
const remainingSeconds = parseInt(seconds % 60, 10);

const displayMinutes = formatMinutes(minutes);
const displaySeconds = formatSeconds(remainingSeconds);

return `${displayMinutes} and ${displaySeconds}`;
};
1 change: 1 addition & 0 deletions app/assets/javascripts/application.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import 'app/utils/index';
import 'app/pw-toggle';
import 'app/form-validation';
import 'app/form-field-format';
Expand Down
2 changes: 1 addition & 1 deletion app/views/layouts/application.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ html lang="#{I18n.locale}"
= render 'shared/footer_lite'

#session-timeout-cntnr
- if current_user
- if user_fully_authenticated?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change means that a user who signs in with email and password and sits on the 2FA delivery page will never auto time out. The consequence is that if I leave my computer and phone unattended, someone can sign in to my account without needing to know my email and password. That's probably an edge case, but still something to think about and decide whether or not this is what we want.

The solution to keeping the user on the account locked screen is to reverse the order of these 2 lines: https://github.com/18F/identity-idp/blob/master/app/controllers/concerns/two_factor_authenticatable.rb#L16-L18

The reason is that application.html.slim wasn't called after signing the user out, and therefore the JS was still active. By reversing the order, we force a new call to the layout template, which then sees that a current user does not exist, and therefore does not trigger the timeout JS.

I will submit a PR shortly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Disregard my first comment about not timing out. I forgot that Devise will time you out regardless of any JS we have. The JS is just for the modal to appear, but I think we do want it to appear during 2FA, right?

= auto_session_timeout_js
-else
= auto_session_expired_js
Expand Down
36 changes: 4 additions & 32 deletions app/views/session_timeout/_ping.js.erb
Original file line number Diff line number Diff line change
Expand Up @@ -28,41 +28,13 @@ function success(data) {

if (show_warning & !el) {
cntnr.insertAdjacentHTML('afterbegin', warning_info);
initTimer(warning);
window.LoginGov.countdownTimer('#countdown', warning);
}
if (!show_warning & el) el.remove();
if (data.live == false) {
window.onbeforeunload = null;
window.onunload = null;
window.location.href = '/timeout';
}
}

function initTimer(duration) {
var countdown = document.getElementById('countdown');
var timeLeft = duration;
var interval = 1000;

var format = function(milliseconds) {
var seconds = milliseconds / 1000;
var minutes = parseInt(seconds / 60, 10);
var remainingSeconds = parseInt(seconds % 60, 10);

var displayMinutes = minutes == 0 ? '' :
minutes + ' minute' + (minutes !== 1 ? 's' : '') + ' and ';
var displaySeconds = remainingSeconds + ' second' + (remainingSeconds !== 1 ? 's' : '');

return (displayMinutes + displaySeconds);
};

function tick() {
countdown.innerHTML = format(timeLeft);
if (timeLeft <= 0) return;
timeLeft -= interval;
setTimeout(tick, interval);
if (!show_warning & el) el.remove();
if (!data.live) {
window.LoginGov.autoLogout();
}

tick();
}

setTimeout(ping, start);
3 changes: 2 additions & 1 deletion app/views/session_timeout/_warning.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
.mx-auto.p4.cntnr-skinny.border-box.bg-white.rounded.relative
= image_tag(asset_url('clock.svg'), class: 'modal-ico')
h3.mt0.mb2 = t('headings.session_timeout_warning')
p.mb3 == t('session_timeout_warning', time_left_in_session: time_left_in_session)
p.mb3 == t('session_timeout_warning',
time_left_in_session: content_tag(:span, time_left_in_session, id: 'countdown'))
= link_to t('forms.buttons.continue_browsing'),
request.original_url, class: 'btn btn-primary'
= link_to t('forms.buttons.sign_out'),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<% lockout_time_in_words = decorated_user.lockout_time_remaining_in_words %>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not a huge fan of declaring vars in views like this (because I would expect this to be a helper method and woud look for it by grepping for def lockout_time_in_words), but this may be the style of the rest of the app

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I used a variable here because the view originally had the decorated_user.lockout_time_remaining_in_words sitting directly in the markup taking up a bunch of characters 😄. I saw variables in other views, and since I'm still learning how you all write rails code, I just went with that. I certainly don't mind making a helper method in this case if that's the style we're should to be adhering to!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think a helper method is needed, but I do think that using that veryyyy long decorated_user.lockout_time_remaining_in_words directly in the markup would be fine by me!

<% title t('titles.account_locked') %>

<h1 class="h3 my0">
<%= t('titles.account_locked') %>
</h1>
<p>
<%= t('devise.two_factor_authentication.max_login_attempts_reached') %>
</p>
<p>
<%= t('devise.two_factor_authentication.please_try_again_html',
time_remaining: content_tag(:span, lockout_time_in_words, id: 'countdown')) %>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The upside of this (vs having the HTML in the i18n string itself):

  • feels more railsy
  • easier to read

Downside:

  • when just looking at the translation itself (please_try_again_html), it is not obvious that there is HTML used in the translation. Need to look at where it is called.

Because of this downside, I think we've mostly been including the HTML directly in I18n translation. At least that was the case the last time I checked.

What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do agree with the downside, I originally had the html directly in the locale file. However, per @zachmargolis comment above, I do think that it is a little cleaner to have the html live in the view.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do we, strictly speaking, need to know that the translation includes HTML?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We do not, but if I were buzzing around in the locale files and saw an _html translation without HTML, I might be tempted to refactor that and remove the _html, which would break this.

I am fine with keeping as-is and will try to move in this direction when I am using html-ified translations in the future in this proj.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah that part does require one to know that the _html automatically calls html_safe on the output, but rails is already so full of magic what's one more 😬

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, that is the magic ✨ -- what I meant to convey is that, as someone who does know this magic, if I saw a translation without HTML but with _html, I might want to refactor to remove _html since it would not be necessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ahhh I see! The ✨ was not known to me. 😢

</p>

<%= nonced_javascript_tag do %>
var test = <%= decorated_user.lockout_time_remaining %> * 1000;
window.LoginGov.countdownTimer('#countdown', test);
<% end %>

This file was deleted.

2 changes: 1 addition & 1 deletion config/locales/devise/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ en:
max_login_attempts_reached: >
Your account is temporarily locked because you have entered the
one-time passcode incorrectly too many times.
please_try_again: Please try again in %{time_remaining}.
please_try_again_html: Please try again in %{time_remaining}.
recovery_code_header_text: Enter your recovery code
recovery_code_prompt: >
You can use this recovery code once. If you still need a code after signing in, go to your
Expand Down
2 changes: 1 addition & 1 deletion config/locales/notices/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,4 @@ en:
session_timedout: >
We signed you out due to inactivity. This helps keep your account safe. Please sign in again.
session_timeout_warning: >-
Your session will end in <span id="countdown">%{time_left_in_session}</span> due to inactivity.
Your session will end in %{time_left_in_session} due to inactivity.
4 changes: 4 additions & 0 deletions spec/views/layouts/application.html.slim_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
describe 'layouts/application.html.slim' do
include Devise::Test::ControllerHelpers

before do
allow(view).to receive(:user_fully_authenticated?).and_return(true)
end

context 'when i18n mode enabled' do
before do
allow(FeatureManagement).to receive(:enable_i18n_mode?).and_return(true)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
require 'rails_helper'

describe 'two_factor_authentication/shared/max_login_attempts_reached.html.slim' do
describe 'two_factor_authentication/shared/max_login_attempts_reached.html.erb' do
context 'locked out account' do
it 'includes localized error message with time remaining' do
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')
allow(user_decorator).to receive(:lockout_time_remaining).and_return(10_000)

render

Expand Down