Merge pull request #394 from alphagov/style-2-step

Improve styles of 2-step code entry and locked account pages

app/controllers/devise/two_step_verification_controller.rb

@@ -4,7 +4,7 @@ class Devise::TwoStepVerificationController < Devise::TwoFactorAuthenticationCon
 
   def new
     if current_user.otp_secret_key.present?
-      redirect_to root_path, alert: "Two Step Verification is already set up"
+      redirect_to root_path, alert: "2-step verification is already set up"
     else
       @otp_secret_key = ROTP::Base32.random_base32
     end
@@ -15,9 +15,9 @@ def create
     totp = ROTP::TOTP.new(@otp_secret_key)
     if totp.verify(params[:code])
       current_user.update_attribute(:otp_secret_key, @otp_secret_key)
-      redirect_to "/", notice: "Two Step Verification set up"
+      redirect_to "/", notice: "2-step verification set up"
     else
-      flash.now[:alert] = "Invalid Two Step Verification code. Perhaps you entered it incorrectly?"
+      flash.now[:alert] = "Invalid 2-step verification code. Perhaps you entered it incorrectly?"
       render :new
     end
   end

app/helpers/bootstrap_flash_helper.rb

@@ -0,0 +1,16 @@
+module BootstrapFlashHelper
+  def bootstrap_flash_message_keys
+    [:success, :info, :warning, :danger, :error, :notice, :alert].select { |k| flash[k].present? }
+  end
+
+  def bootstrap_flash_class(flash_key)
+    case flash_key
+    when :notice
+      "success"
+    when :error, :alert
+      "danger"
+    else
+      flash_key
+    end
+  end
+end

app/views/devise/two_factor_authentication/max_login_attempts_reached.html.erb

@@ -0,0 +1,12 @@
+<% content_for :title, "Account locked" %>
+<% content_for :thin_form, true %>
+
+<div class="page-title">
+  <h1>Your account is locked</h1>
+</div>
+
+<div class="callout callout-danger">
+  <p class="lead remove-bottom-margin">
+    Your account has been locked because an incorrect 2-step verification code was entered too many times.
+  </p>
+</div>

app/views/devise/two_factor_authentication/show.html.erb

@@ -0,0 +1,26 @@
+<% content_for :title, "2-step verification" %>
+<% content_for :thin_form, true %>
+<% content_for :suppress_navbar_items, true %>
+
+<div class="page-title">
+  <h1>2-step verification</h1>
+</div>
+
+<%= form_tag([resource_name, :two_factor_authentication], :method => :put) do %>
+  <div class="panel panel-default">
+    <div class="panel-body">
+      <div class="form-group">
+        <%= label_tag :code, 'Verification code' %>
+        <p class="text-muted">Use the verification app on your phone to get your code.<br />You won’t need to do this again for 30 days.</p>
+        <%= text_field_tag :code, nil,
+          class: 'form-control input-md-4 input-lg',
+          placeholder: 'Enter 6 digit code',
+          tabindex: 1,
+          autofocus: 'autofocus' %>
+      </div>
+    </div>
+    <div class="panel-footer">
+      <%= submit_tag "Sign in", tabindex: 2, class: 'btn btn-success btn-lg btn-block' %>
+    </div>
+  </div>
+<% end %>

app/views/layouts/application.html.erb

@@ -8,24 +8,24 @@
 <% end %>
 
 <% if user_signed_in? && params[:controller] !~ %r{doorkeeper/} %>
-  <% content_for :navbar_items do %>
-    <%= nav_link 'Dashboard', root_path %>
-    <% if policy(User).index? %>
-      <%= nav_link 'Users', users_path %>
-    <% end %>
-    <% if policy(ApiUser).index? %>
-      <%= nav_link 'API Users', api_users_path %>
-    <% end %>
-    <% if policy(Doorkeeper::Application).index? %>
-      <%= nav_link 'Applications', doorkeeper_applications_path %>
-    <% end %>
-    <% if policy(Organisation).index? %>
-      <%= nav_link 'Organisations', organisations_path %>
+  <% unless content_for :suppress_navbar_items %>
+    <% content_for :navbar_items do %>
+      <%= nav_link 'Dashboard', root_path %>
+      <% if policy(User).index? %>
+        <%= nav_link 'Users', users_path %>
+      <% end %>
+      <% if policy(ApiUser).index? %>
+        <%= nav_link 'API Users', api_users_path %>
+      <% end %>
+      <% if policy(Doorkeeper::Application).index? %>
+        <%= nav_link 'Applications', doorkeeper_applications_path %>
+      <% end %>
+      <% if policy(Organisation).index? %>
+        <%= nav_link 'Organisations', organisations_path %>
+      <% end %>
     <% end %>
   <% end %>
-<% end %>
 
-<% if user_signed_in? && params[:controller] !~ %r{doorkeeper/} %>
   <% content_for :navbar_right do %>
     <%= link_to current_user.name, user_link_target %>
     &bull; <%= link_to 'Sign out', destroy_user_session_path %>
@@ -34,8 +34,7 @@
 
 <% content_for :content do %>
   <% if content_for?(:thin_form) %><div class="thin-form"><% end %>
-  <%- if notice %><div class="alert alert-success" role="alert"><%= notice %></div><% end -%>
-  <%- if alert %><div class="alert alert-danger" role="alert"><%= alert %></div><% end %>
+  <%= render partial: 'shared/bootstrap_flash_messages' %>
   <%= yield %>
   <% if content_for?(:thin_form) %></div><% end %>
 <% end %>

app/views/shared/_bootstrap_flash_messages.html.erb

@@ -0,0 +1,8 @@
+<% bootstrap_flash_message_keys.each do |k| %>
+  <div class="alert alert-<%= bootstrap_flash_class(k) %>"
+    data-module="auto-track-event"
+    data-track-action="alert-<%= bootstrap_flash_class(k) %>"
+    data-track-label="<%= strip_tags(flash[k]) %>">
+      <%= flash[k] %>
+  </div>
+<% end %>

config/initializers/devise.rb

@@ -320,7 +320,8 @@
                       end
 
   # Configuration for two_factor_authentication
-  config.max_login_attempts = 3
+  #FIXME: Reduce attempt limit when accounts are unlocked after an hour
+  config.max_login_attempts = 10
   config.allowed_otp_drift_seconds = 30
   config.otp_length = 6
   config.remember_otp_session_for_seconds = 30.days

config/locales/en.yml

@@ -32,3 +32,8 @@ en:
       taken_in_past: "was used previously. Please choose a different one."
       equal_to_current_password: "must be different than the current password."
       password_format: "must contain big, small letters and digits"
+
+  devise:
+    two_factor_authentication:
+      success: "Signed in successfully."
+      attempt_failed: "Sorry, that code didn’t work."

test/helpers/user_helpers.rb

@@ -18,7 +18,7 @@ def signin_with_2sv(user_or_options)
 
     Timecop.freeze do
       fill_in :code, with: ROTP::TOTP.new(user.otp_secret_key).now
-      click_button "Submit"
+      click_button "Sign in"
     end
   end
 

test/integration/authorise_application_test.rb

@@ -41,7 +41,7 @@ class AuthoriseApplicationTest < ActionDispatch::IntegrationTest
     ignoring_spurious_error do
       visit "/oauth/authorize?response_type=code&client_id=#{@app.uid}&redirect_uri=#{@app.redirect_uri}"
     end
-    assert_response_contains("Enter your personal code")
+    assert_response_contains("get your code")
     refute Doorkeeper::AccessGrant.find_by(resource_owner_id: @user.id)
   end
 

test/integration/sign_in_test.rb

@@ -72,15 +72,15 @@ class SignInTest < ActionDispatch::IntegrationTest
     should "prompt for a verification code" do
       visit root_path
       signin(email: "email@example.com", password: "some passphrase with various $ymb0l$")
-      assert_response_contains "Enter your personal code"
+      assert_response_contains "get your code"
       assert_selector "input[name=code]"
     end
 
     should "prevent access to signon until fully authenticated" do
       visit root_path
       signin(email: "email@example.com", password: "some passphrase with various $ymb0l$")
       visit root_path
-      assert_response_contains "Enter your personal code"
+      assert_response_contains "get your code"
       assert_selector "input[name=code]"
     end
 
@@ -95,10 +95,10 @@ class SignInTest < ActionDispatch::IntegrationTest
       signin(email: "email@example.com", password: "some passphrase with various $ymb0l$")
       Timecop.freeze do
         fill_in :code, with: ""
-        click_button "Submit"
+        click_button "Sign in"
       end
 
-      assert_response_contains "Enter your personal code"
+      assert_response_contains "get your code"
     end
 
     should "prevent access with an old code" do
@@ -107,18 +107,18 @@ class SignInTest < ActionDispatch::IntegrationTest
       visit root_path
       signin(email: "email@example.com", password: "some passphrase with various $ymb0l$")
       fill_in :code, with: old_code
-      click_button "Submit"
+      click_button "Sign in"
 
-      assert_response_contains "Enter your personal code"
+      assert_response_contains "get your code"
     end
 
     should "prevent access with a garbage code" do
       visit root_path
       signin(email: "email@example.com", password: "some passphrase with various $ymb0l$")
       fill_in :code, with: "abcdef"
-      click_button "Submit"
+      click_button "Sign in"
 
-      assert_response_contains "Enter your personal code"
+      assert_response_contains "get your code"
     end
   end
 end

test/integration/two_step_verification_test.rb

@@ -13,7 +13,7 @@ class TwoStepVerificationTest < ActionDispatch::IntegrationTest
       end
 
       should "redirect to homepage" do
-        assert_response_contains "Two Step Verification is already set up"
+        assert_response_contains "2-step verification is already set up"
         assert_response_contains "Welcome to GOV.UK"
       end
     end
@@ -39,7 +39,7 @@ class TwoStepVerificationTest < ActionDispatch::IntegrationTest
         end
 
         should "reject the code" do
-          assert_response_contains "Invalid Two Step Verification code. Perhaps you entered it incorrectly?"
+          assert_response_contains "Invalid 2-step verification code. Perhaps you entered it incorrectly?"
         end
 
         should "show the same secret" do
@@ -56,7 +56,7 @@ class TwoStepVerificationTest < ActionDispatch::IntegrationTest
         end
 
         should "accept the code" do
-          assert_response_contains "Two Step Verification set up"
+          assert_response_contains "2-step verification set up"
         end
 
         should "persist the confirmed secret" do