Commit 6f3fe923 authored by Robert Speicher's avatar Robert Speicher

Merge branch '17334-u2f-device-identifiers' into 'master'

Allow naming (and deleting) U2F devices.

## What does this MR do?

- Allow giving each U2F device a name (at the time of registration).
- Allow deleting individual U2F devices.
- Display a list of registered U2F devices.

## What are the relevant issue numbers?

- Closes #17334 
- Closes #17335 

See merge request !5833
parents ba0c6e4d dbedf3a6
...@@ -21,6 +21,7 @@ v 8.11.0 (unreleased) ...@@ -21,6 +21,7 @@ v 8.11.0 (unreleased)
- Remove magic comments (`# encoding: UTF-8`) from Ruby files. !5456 (winniehell) - Remove magic comments (`# encoding: UTF-8`) from Ruby files. !5456 (winniehell)
- GitLab Performance Monitoring can now track custom events such as the number of tags pushed to a repository - GitLab Performance Monitoring can now track custom events such as the number of tags pushed to a repository
- Add support for relative links starting with ./ or / to RelativeLinkFilter (winniehell) - Add support for relative links starting with ./ or / to RelativeLinkFilter (winniehell)
- Allow naming U2F devices !5833
- Ignore URLs starting with // in Markdown links !5677 (winniehell) - Ignore URLs starting with // in Markdown links !5677 (winniehell)
- Fix CI status icon link underline (ClemMakesApps) - Fix CI status icon link underline (ClemMakesApps)
- The Repository class is now instrumented - The Repository class is now instrumented
......
...@@ -228,3 +228,9 @@ ...@@ -228,3 +228,9 @@
} }
} }
} }
table.u2f-registrations {
th:not(:last-child), td:not(:last-child) {
border-right: solid 1px transparent;
}
}
\ No newline at end of file
...@@ -43,11 +43,11 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController ...@@ -43,11 +43,11 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
# A U2F (universal 2nd factor) device's information is stored after successful # A U2F (universal 2nd factor) device's information is stored after successful
# registration, which is then used while 2FA authentication is taking place. # registration, which is then used while 2FA authentication is taking place.
def create_u2f def create_u2f
@u2f_registration = U2fRegistration.register(current_user, u2f_app_id, params[:device_response], session[:challenges]) @u2f_registration = U2fRegistration.register(current_user, u2f_app_id, u2f_registration_params, session[:challenges])
if @u2f_registration.persisted? if @u2f_registration.persisted?
session.delete(:challenges) session.delete(:challenges)
redirect_to profile_account_path, notice: "Your U2F device was registered!" redirect_to profile_two_factor_auth_path, notice: "Your U2F device was registered!"
else else
@qr_code = build_qr_code @qr_code = build_qr_code
setup_u2f_registration setup_u2f_registration
...@@ -91,15 +91,19 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController ...@@ -91,15 +91,19 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
# Actual communication is performed using a Javascript API # Actual communication is performed using a Javascript API
def setup_u2f_registration def setup_u2f_registration
@u2f_registration ||= U2fRegistration.new @u2f_registration ||= U2fRegistration.new
@registration_key_handles = current_user.u2f_registrations.pluck(:key_handle) @u2f_registrations = current_user.u2f_registrations
u2f = U2F::U2F.new(u2f_app_id) u2f = U2F::U2F.new(u2f_app_id)
registration_requests = u2f.registration_requests registration_requests = u2f.registration_requests
sign_requests = u2f.authentication_requests(@registration_key_handles) sign_requests = u2f.authentication_requests(@u2f_registrations.map(&:key_handle))
session[:challenges] = registration_requests.map(&:challenge) session[:challenges] = registration_requests.map(&:challenge)
gon.push(u2f: { challenges: session[:challenges], app_id: u2f_app_id, gon.push(u2f: { challenges: session[:challenges], app_id: u2f_app_id,
register_requests: registration_requests, register_requests: registration_requests,
sign_requests: sign_requests }) sign_requests: sign_requests })
end end
def u2f_registration_params
params.require(:u2f_registration).permit(:device_response, :name)
end
end end
class Profiles::U2fRegistrationsController < Profiles::ApplicationController
def destroy
u2f_registration = current_user.u2f_registrations.find(params[:id])
u2f_registration.destroy
redirect_to profile_two_factor_auth_path, notice: "Successfully deleted U2F device."
end
end
...@@ -3,18 +3,19 @@ ...@@ -3,18 +3,19 @@
class U2fRegistration < ActiveRecord::Base class U2fRegistration < ActiveRecord::Base
belongs_to :user belongs_to :user
def self.register(user, app_id, json_response, challenges) def self.register(user, app_id, params, challenges)
u2f = U2F::U2F.new(app_id) u2f = U2F::U2F.new(app_id)
registration = self.new registration = self.new
begin begin
response = U2F::RegisterResponse.load_from_json(json_response) response = U2F::RegisterResponse.load_from_json(params[:device_response])
registration_data = u2f.register!(challenges, response) registration_data = u2f.register!(challenges, response)
registration.update(certificate: registration_data.certificate, registration.update(certificate: registration_data.certificate,
key_handle: registration_data.key_handle, key_handle: registration_data.key_handle,
public_key: registration_data.public_key, public_key: registration_data.public_key,
counter: registration_data.counter, counter: registration_data.counter,
user: user) user: user,
name: params[:name])
rescue JSON::ParserError, NoMethodError, ArgumentError rescue JSON::ParserError, NoMethodError, ArgumentError
registration.errors.add(:base, 'Your U2F device did not send a valid JSON response.') registration.errors.add(:base, 'Your U2F device did not send a valid JSON response.')
rescue U2F::Error => e rescue U2F::Error => e
......
...@@ -60,13 +60,38 @@ ...@@ -60,13 +60,38 @@
two-factor authentication app before a U2F device. That way you'll always be able to two-factor authentication app before a U2F device. That way you'll always be able to
log in - even when you're using an unsupported browser. log in - even when you're using an unsupported browser.
.col-lg-9 .col-lg-9
%p
- if @registration_key_handles.present?
= icon "check inverse", base: "circle", class: "text-success", text: "You have #{pluralize(@registration_key_handles.size, 'U2F device')} registered with GitLab."
- if @u2f_registration.errors.present? - if @u2f_registration.errors.present?
= form_errors(@u2f_registration) = form_errors(@u2f_registration)
= render "u2f/register" = render "u2f/register"
%hr
%h5 U2F Devices (#{@u2f_registrations.length})
- if @u2f_registrations.present?
.table-responsive
%table.table.table-bordered.u2f-registrations
%colgroup
%col{ width: "50%" }
%col{ width: "30%" }
%col{ width: "20%" }
%thead
%tr
%th Name
%th Registered On
%th
%tbody
- @u2f_registrations.each do |registration|
%tr
%td= registration.name.presence || "<no name set>"
%td= registration.created_at.to_date.to_s(:medium)
%td= link_to "Delete", profile_u2f_registration_path(registration), method: :delete, class: "btn btn-danger pull-right", data: { confirm: "Are you sure you want to delete this device? This action cannot be undone." }
- else
.settings-message.text-center
You don't have any U2F devices registered yet.
- if two_factor_skippable? - if two_factor_skippable?
:javascript :javascript
var button = "<a class='btn btn-xs btn-warning pull-right' data-method='patch' href='#{skip_profile_two_factor_auth_path}'>Configure it later</a>"; var button = "<a class='btn btn-xs btn-warning pull-right' data-method='patch' href='#{skip_profile_two_factor_auth_path}'>Configure it later</a>";
......
...@@ -28,9 +28,14 @@ ...@@ -28,9 +28,14 @@
%script#js-register-u2f-registered{ type: "text/template" } %script#js-register-u2f-registered{ type: "text/template" }
%div.row.append-bottom-10 %div.row.append-bottom-10
%p Your device was successfully set up! Click this button to register with the GitLab server. .col-md-12
%p Your device was successfully set up! Give it a name and register it with the GitLab server.
= form_tag(create_u2f_profile_two_factor_auth_path, method: :post) do = form_tag(create_u2f_profile_two_factor_auth_path, method: :post) do
= hidden_field_tag :device_response, nil, class: 'form-control', required: true, id: "js-device-response" .row.append-bottom-10
.col-md-3
= text_field_tag 'u2f_registration[name]', nil, class: 'form-control', placeholder: "Pick a name"
.col-md-3
= hidden_field_tag 'u2f_registration[device_response]', nil, class: 'form-control', required: true, id: "js-device-response"
= submit_tag "Register U2F Device", class: "btn btn-success" = submit_tag "Register U2F Device", class: "btn btn-success"
:javascript :javascript
......
...@@ -375,6 +375,8 @@ Rails.application.routes.draw do ...@@ -375,6 +375,8 @@ Rails.application.routes.draw do
patch :skip patch :skip
end end
end end
resources :u2f_registrations, only: [:destroy]
end end
end end
......
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddColumnNameToU2fRegistrations < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
# When a migration requires downtime you **must** uncomment the following
# constant and define a short and easy to understand explanation as to why the
# migration requires downtime.
# DOWNTIME_REASON = ''
# When using the methods "add_concurrent_index" or "add_column_with_default"
# you must disable the use of transactions as these methods can not run in an
# existing transaction. When using "add_concurrent_index" make sure that this
# method is the _only_ method called in the migration, any other changes
# should go in a separate migration. This ensures that upon failure _only_ the
# index creation fails and can be retried or reverted easily.
#
# To disable transactions uncomment the following line and remove these
# comments:
# disable_ddl_transaction!
def change
add_column :u2f_registrations, :name, :string
end
end
...@@ -11,7 +11,7 @@ ...@@ -11,7 +11,7 @@
# #
# It's strongly recommended that you check this file into your version control system. # It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20160810142633) do ActiveRecord::Schema.define(version: 20160816161312) do
# These are extensions that must be enabled in order to support this database # These are extensions that must be enabled in order to support this database
enable_extension "plpgsql" enable_extension "plpgsql"
...@@ -1016,6 +1016,7 @@ ActiveRecord::Schema.define(version: 20160810142633) do ...@@ -1016,6 +1016,7 @@ ActiveRecord::Schema.define(version: 20160810142633) do
t.integer "user_id" t.integer "user_id"
t.datetime "created_at", null: false t.datetime "created_at", null: false
t.datetime "updated_at", null: false t.datetime "updated_at", null: false
t.string "name"
end end
add_index "u2f_registrations", ["key_handle"], name: "index_u2f_registrations_on_key_handle", using: :btree add_index "u2f_registrations", ["key_handle"], name: "index_u2f_registrations_on_key_handle", using: :btree
......
...@@ -12,10 +12,12 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: ...@@ -12,10 +12,12 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature:
end end
def register_u2f_device(u2f_device = nil) def register_u2f_device(u2f_device = nil)
u2f_device ||= FakeU2fDevice.new(page) name = FFaker::Name.first_name
u2f_device ||= FakeU2fDevice.new(page, name)
u2f_device.respond_to_u2f_registration u2f_device.respond_to_u2f_registration
click_on 'Setup New U2F Device' click_on 'Setup New U2F Device'
expect(page).to have_content('Your device was successfully set up') expect(page).to have_content('Your device was successfully set up')
fill_in "Pick a name", with: name
click_on 'Register U2F Device' click_on 'Register U2F Device'
u2f_device u2f_device
end end
...@@ -40,13 +42,14 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: ...@@ -40,13 +42,14 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature:
end end
describe 'when 2FA via OTP is enabled' do describe 'when 2FA via OTP is enabled' do
it 'allows registering a new device' do it 'allows registering a new device with a name' do
visit profile_account_path visit profile_account_path
manage_two_factor_authentication manage_two_factor_authentication
expect(page.body).to match("You've already enabled two-factor authentication using mobile") expect(page.body).to match("You've already enabled two-factor authentication using mobile")
register_u2f_device u2f_device = register_u2f_device
expect(page.body).to match(u2f_device.name)
expect(page.body).to match('Your U2F device was registered') expect(page.body).to match('Your U2F device was registered')
end end
...@@ -55,15 +58,31 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: ...@@ -55,15 +58,31 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature:
# First device # First device
manage_two_factor_authentication manage_two_factor_authentication
register_u2f_device first_device = register_u2f_device
expect(page.body).to match('Your U2F device was registered') expect(page.body).to match('Your U2F device was registered')
# Second device # Second device
manage_two_factor_authentication second_device = register_u2f_device
register_u2f_device
expect(page.body).to match('Your U2F device was registered') expect(page.body).to match('Your U2F device was registered')
expect(page.body).to match(first_device.name)
expect(page.body).to match(second_device.name)
expect(U2fRegistration.count).to eq(2)
end
it 'allows deleting a device' do
visit profile_account_path
manage_two_factor_authentication manage_two_factor_authentication
expect(page.body).to match('You have 2 U2F devices registered') expect(page.body).to match("You've already enabled two-factor authentication using mobile")
first_u2f_device = register_u2f_device
second_u2f_device = register_u2f_device
click_on "Delete", match: :first
expect(page.body).to match('Successfully deleted')
expect(page.body).not_to match(first_u2f_device.name)
expect(page.body).to match(second_u2f_device.name)
end end
end end
...@@ -208,7 +227,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: ...@@ -208,7 +227,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature:
describe "when a given U2F device has not been registered" do describe "when a given U2F device has not been registered" do
it "does not allow logging in with that particular device" do it "does not allow logging in with that particular device" do
unregistered_device = FakeU2fDevice.new(page) unregistered_device = FakeU2fDevice.new(page, FFaker::Name.first_name)
login_as(user) login_as(user)
unregistered_device.respond_to_u2f_authentication unregistered_device.respond_to_u2f_authentication
click_on "Login Via U2F Device" click_on "Login Via U2F Device"
...@@ -262,6 +281,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: ...@@ -262,6 +281,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature:
end end
it "deletes u2f registrations" do it "deletes u2f registrations" do
visit profile_account_path
expect { click_on "Disable" }.to change { U2fRegistration.count }.by(-1) expect { click_on "Disable" }.to change { U2fRegistration.count }.by(-1)
end end
end end
......
class FakeU2fDevice class FakeU2fDevice
def initialize(page) attr_reader :name
def initialize(page, name)
@page = page @page = page
@name = name
end end
def respond_to_u2f_registration def respond_to_u2f_registration
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment