Merge branch '17341-firefox-u2f' into 'master'

Allow U2F devices to be used in Firefox

- Adds U2F support for Firefox
- Improve U2F feature detection logic
- Have authentication flow be closer to the spec (single challenge instead of a challenge for each `signRequest`)

- Closes #17341 
- Related to #15337 

See merge request !5177

16 files changed, 85 insertions, 47 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 8524c4082f3..cd94872eeed 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -27,6 +27,7 @@ v 8.10.0 (unreleased)
   - Make images fit to the size of the viewport !4810
   - Fix check for New Branch button on Issue page !4630 (winniehell)
   - Fix MR-auto-close text added to description. !4836
+  - Support U2F devices in Firefox. !5177
   - Fix issue, preventing users w/o push access to sort tags !5105 (redetection)
   - Add Spring EmojiOne updates.
   - Add syntax for multiline blockquote using `>>>` fence !3954
diff --git a/app/assets/javascripts/application.js.coffee b/app/assets/javascripts/application.js.coffee
index c98763d6271..eceff6d91d5 100644
--- a/app/assets/javascripts/application.js.coffee
+++ b/app/assets/javascripts/application.js.coffee
@@ -53,7 +53,6 @@
 #= require_directory ./u2f
 #= require_directory .
 #= require fuzzaldrin-plus
-#= require u2f
 
 window.slugify = (text) ->
   text.replace(/[^-a-zA-Z0-9]+/g, '_').toLowerCase()
diff --git a/app/assets/javascripts/u2f/authenticate.js.coffee b/app/assets/javascripts/u2f/authenticate.js.coffee
index 6deb902c8de..918c0a560fd 100644
--- a/app/assets/javascripts/u2f/authenticate.js.coffee
+++ b/app/assets/javascripts/u2f/authenticate.js.coffee
@@ -6,8 +6,20 @@
 class @U2FAuthenticate
   constructor: (@container, u2fParams) ->
     @appId = u2fParams.app_id
-    @challenges = u2fParams.challenges
-    @signRequests = u2fParams.sign_requests
+    @challenge = u2fParams.challenge
+
+    # The U2F Javascript API v1.1 requires a single challenge, with
+    # _no challenges per-request_. The U2F Javascript API v1.0 requires a
+    # challenge per-request, which is done by copying the single challenge
+    # into every request.
+    #
+    # In either case, we don't need the per-request challenges that the server
+    # has generated, so we can remove them.
+    #
+    # Note: The server library fixes this behaviour in (unreleased) version 1.0.0.
+    # This can be removed once we upgrade.
+    # https://github.com/castle/ruby-u2f/commit/103f428071a81cd3d5f80c2e77d522d5029946a4
+    @signRequests = u2fParams.sign_requests.map (request) -> _(request).omit('challenge')
 
   start: () =>
     if U2FUtil.isU2FSupported()
@@ -16,7 +28,7 @@ class @U2FAuthenticate
       @renderNotSupported()
 
   authenticate: () =>
-    u2f.sign(@appId, @challenges, @signRequests, (response) =>
+    u2f.sign(@appId, @challenge, @signRequests, (response) =>
       if response.errorCode
         error = new U2FError(response.errorCode)
         @renderError(error);
diff --git a/app/assets/javascripts/u2f/util.js.coffee b/app/assets/javascripts/u2f/util.js.coffee
new file mode 100644
index 00000000000..5ef324f609d
--- /dev/null
+++ b/app/assets/javascripts/u2f/util.js.coffee
@@ -0,0 +1,3 @@
+class @U2FUtil
+  @isU2FSupported: ->
+    window.u2f
diff --git a/app/assets/javascripts/u2f/util.js.coffee.erb b/app/assets/javascripts/u2f/util.js.coffee.erb
deleted file mode 100644
index d59341c38b9..00000000000
--- a/app/assets/javascripts/u2f/util.js.coffee.erb
+++ /dev/null
@@ -1,15 +0,0 @@
-# Helper class for U2F (universal 2nd factor) device registration and authentication.
-
-class @U2FUtil
-  @isU2FSupported: ->
-    if @testMode
-      true
-    else
-      gon.u2f.browser_supports_u2f
-
-  @enableTestMode: ->
-    @testMode = true
-
-<% if Rails.env.test? %>
-U2FUtil.enableTestMode();
-<% end %>
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 9cc31620d9f..a1004d9bcea 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -344,10 +344,6 @@ class ApplicationController < ActionController::Base
     session[:skip_tfa] && session[:skip_tfa] > Time.current
   end
 
-  def browser_supports_u2f?
-    browser.chrome? && browser.version.to_i >= 41 && !browser.device.mobile?
-  end
-
   def redirect_to_home_page_url?
     # If user is not signed-in and tries to access root_path - redirect him to landing page
     # Don't redirect to the default URL to prevent endless redirections
diff --git a/app/controllers/concerns/authenticates_with_two_factor.rb b/app/controllers/concerns/authenticates_with_two_factor.rb
index 998b8adc411..ba07cea569c 100644
--- a/app/controllers/concerns/authenticates_with_two_factor.rb
+++ b/app/controllers/concerns/authenticates_with_two_factor.rb
@@ -57,7 +57,7 @@ module AuthenticatesWithTwoFactor
 
   # Authenticate using the response from a U2F (universal 2nd factor) device
   def authenticate_with_two_factor_via_u2f(user)
-    if U2fRegistration.authenticate(user, u2f_app_id, user_params[:device_response], session[:challenges])
+    if U2fRegistration.authenticate(user, u2f_app_id, user_params[:device_response], session[:challenge])
       # Remove any lingering user data from login
       session.delete(:otp_user_id)
       session.delete(:challenges)
@@ -77,11 +77,9 @@ module AuthenticatesWithTwoFactor
 
     if key_handles.present?
       sign_requests = u2f.authentication_requests(key_handles)
-      challenges = sign_requests.map(&:challenge)
-      session[:challenges] = challenges
-      gon.push(u2f: { challenges: challenges, app_id: u2f_app_id,
-                      sign_requests: sign_requests,
-                      browser_supports_u2f: browser_supports_u2f? })
+      session[:challenge] ||= u2f.challenge
+      gon.push(u2f: { challenge: session[:challenge], app_id: u2f_app_id,
+                      sign_requests: sign_requests })
     end
   end
 end
diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb
index 6a358fdcc05..e37e9e136db 100644
--- a/app/controllers/profiles/two_factor_auths_controller.rb
+++ b/app/controllers/profiles/two_factor_auths_controller.rb
@@ -100,7 +100,6 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
 
     gon.push(u2f: { challenges: session[:challenges], app_id: u2f_app_id,
                     register_requests: registration_requests,
-                    sign_requests: sign_requests,
-                    browser_supports_u2f: browser_supports_u2f? })
+                    sign_requests: sign_requests })
   end
 end
diff --git a/app/helpers/u2f_helper.rb b/app/helpers/u2f_helper.rb
new file mode 100644
index 00000000000..143b4ca6b51
--- /dev/null
+++ b/app/helpers/u2f_helper.rb
@@ -0,0 +1,5 @@
+module U2fHelper
+  def inject_u2f_api?
+    browser.chrome? && browser.version.to_i >= 41 && !browser.device.mobile?
+  end
+end
diff --git a/app/views/devise/sessions/two_factor.html.haml b/app/views/devise/sessions/two_factor.html.haml
index a373f61bd3c..4debd3d608f 100644
--- a/app/views/devise/sessions/two_factor.html.haml
+++ b/app/views/devise/sessions/two_factor.html.haml
@@ -1,3 +1,7 @@
+- if inject_u2f_api?
+  - content_for :page_specific_javascripts do
+    = page_specific_javascript_tag('u2f.js')
+
 %div
   .login-box
     .login-heading
diff --git a/app/views/profiles/two_factor_auths/show.html.haml b/app/views/profiles/two_factor_auths/show.html.haml
index 8780da1dec4..366f1fed35b 100644
--- a/app/views/profiles/two_factor_auths/show.html.haml
+++ b/app/views/profiles/two_factor_auths/show.html.haml
@@ -2,6 +2,10 @@
 - header_title "Two-Factor Authentication", profile_two_factor_auth_path
 = render 'profiles/head'
 
+- if inject_u2f_api?
+  - content_for :page_specific_javascripts do
+    = page_specific_javascript_tag('u2f.js')
+
 .row.prepend-top-default
   .col-lg-3
     %h4.prepend-top-0
diff --git a/config/application.rb b/config/application.rb
index 21e7cc7b6e8..5f7b6a3c049 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -87,6 +87,7 @@ module Gitlab
     config.assets.precompile << "profile/application.js"
     config.assets.precompile << "lib/utils/*.js"
     config.assets.precompile << "lib/*.js"
+    config.assets.precompile << "u2f.js"
 
     # Version of your assets, change this if you want to expire all your assets
     config.assets.version = '1.0'
diff --git a/spec/features/u2f_spec.rb b/spec/features/u2f_spec.rb
index 14613754f74..9335f5bf120 100644
--- a/spec/features/u2f_spec.rb
+++ b/spec/features/u2f_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: true, js: true do
+  before { allow_any_instance_of(U2fHelper).to receive(:inject_u2f_api?).and_return(true) }
+
   def register_u2f_device(u2f_device = nil)
     u2f_device ||= FakeU2fDevice.new(page)
     u2f_device.respond_to_u2f_registration
@@ -208,21 +210,52 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature:
         expect(page.body).to match('Authentication via U2F device failed')
       end
     end
-  end
 
-  describe "when two-factor authentication is disabled" do
-    let(:user) { create(:user) }
+    describe "when more than one device has been registered by the same user" do
+      it "allows logging in with either device" do
+        # Register first device
+        user = login_as(:user)
+        user.update_attribute(:otp_required_for_login, true)
+        visit profile_two_factor_auth_path
+        expect(page).to have_content("Your U2F device needs to be set up.")
+        first_device = register_u2f_device
+
+        # Register second device
+        visit profile_two_factor_auth_path
+        expect(page).to have_content("Your U2F device needs to be set up.")
+        second_device = register_u2f_device
+        logout
+
+        # Authenticate as both devices
+        [first_device, second_device].each do |device|
+          login_as(user)
+          device.respond_to_u2f_authentication
+          click_on "Login Via U2F Device"
+          expect(page.body).to match('We heard back from your U2F device')
+          click_on "Authenticate via U2F Device"
 
-    before do
-      login_as(user)
-      user.update_attribute(:otp_required_for_login, true)
-      visit profile_account_path
-      click_on 'Manage Two-Factor Authentication'
-      register_u2f_device
+          expect(page.body).to match('Signed in successfully')
+
+          logout
+        end
+      end
     end
 
-    it "deletes u2f registrations" do
-      expect { click_on "Disable" }.to change { U2fRegistration.count }.from(1).to(0)
+    describe "when two-factor authentication is disabled" do
+      let(:user) { create(:user) }
+
+      before do
+        user = login_as(:user)
+        user.update_attribute(:otp_required_for_login, true)
+        visit profile_account_path
+        click_on 'Manage Two-Factor Authentication'
+        expect(page).to have_content("Your U2F device needs to be set up.")
+        register_u2f_device
+      end
+
+      it "deletes u2f registrations" do
+        expect { click_on "Disable" }.to change { U2fRegistration.count }.by(-1)
+      end
     end
   end
 end
diff --git a/spec/javascripts/u2f/authenticate_spec.coffee b/spec/javascripts/u2f/authenticate_spec.coffee
index e8a2892d678..8ffeda11704 100644
--- a/spec/javascripts/u2f/authenticate_spec.coffee
+++ b/spec/javascripts/u2f/authenticate_spec.coffee
@@ -5,13 +5,12 @@
 #= require ./mock_u2f_device
 
 describe 'U2FAuthenticate', ->
-  U2FUtil.enableTestMode()
   fixture.load('u2f/authenticate')
 
   beforeEach ->
     @u2fDevice = new MockU2FDevice
     @container = $("#js-authenticate-u2f")
-    @component = new U2FAuthenticate(@container, {}, "token")
+    @component = new U2FAuthenticate(@container, {sign_requests: []}, "token")
     @component.start()
 
   it 'allows authenticating via a U2F device', ->
diff --git a/spec/javascripts/u2f/register_spec.js.coffee b/spec/javascripts/u2f/register_spec.js.coffee
index 0858abeca1a..87dc769792b 100644
--- a/spec/javascripts/u2f/register_spec.js.coffee
+++ b/spec/javascripts/u2f/register_spec.js.coffee
@@ -5,7 +5,6 @@
 #= require ./mock_u2f_device
 
 describe 'U2FRegister', ->
-  U2FUtil.enableTestMode()
   fixture.load('u2f/register')
 
   beforeEach ->
diff --git a/spec/support/fake_u2f_device.rb b/spec/support/fake_u2f_device.rb
index 553fe9f1fbc..f550e9a0160 100644
--- a/spec/support/fake_u2f_device.rb
+++ b/spec/support/fake_u2f_device.rb
@@ -18,8 +18,8 @@ class FakeU2fDevice
 
   def respond_to_u2f_authentication
     app_id = @page.evaluate_script('gon.u2f.app_id')
-    challenges = @page.evaluate_script('gon.u2f.challenges')
-    json_response = u2f_device(app_id).sign_response(challenges[0])
+    challenge = @page.evaluate_script('gon.u2f.challenge')
+    json_response = u2f_device(app_id).sign_response(challenge)
 
     @page.execute_script("
     u2f.sign = function(appId, challenges, signRequests, callback) {