diff options
author | Robert Speicher <robert@gitlab.com> | 2016-08-18 21:37:56 +0300 |
---|---|---|
committer | Ruben Davila <rdavila84@gmail.com> | 2016-08-19 02:45:39 +0300 |
commit | 117e2842e5944b445fbdaba62ef0945f7863240b (patch) | |
tree | 8b6d6bb57cc2155f81b2c126b033949222b51e03 | |
parent | b07cd8ec641a933b819300e005e7124dd1d11c81 (diff) |
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
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/assets/stylesheets/pages/profile.scss | 6 | ||||
-rw-r--r-- | app/controllers/profiles/two_factor_auths_controller.rb | 12 | ||||
-rw-r--r-- | app/controllers/profiles/u2f_registrations_controller.rb | 7 | ||||
-rw-r--r-- | app/models/u2f_registration.rb | 7 | ||||
-rw-r--r-- | app/views/profiles/two_factor_auths/show.html.haml | 31 | ||||
-rw-r--r-- | app/views/u2f/_register.html.haml | 13 | ||||
-rw-r--r-- | config/routes.rb | 2 | ||||
-rw-r--r-- | db/migrate/20160816161312_add_column_name_to_u2f_registrations.rb | 29 | ||||
-rw-r--r-- | db/schema.rb | 3 | ||||
-rw-r--r-- | spec/features/u2f_spec.rb | 36 | ||||
-rw-r--r-- | spec/support/fake_u2f_device.rb | 5 |
12 files changed, 128 insertions, 24 deletions
diff --git a/CHANGELOG b/CHANGELOG index 71128413ff9..4d9386df64e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -20,6 +20,7 @@ v 8.11.0 (unreleased) - 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 - 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) - Fix CI status icon link underline (ClemMakesApps) - The Repository class is now instrumented diff --git a/app/assets/stylesheets/pages/profile.scss b/app/assets/stylesheets/pages/profile.scss index 46371ec6871..6f58203f49c 100644 --- a/app/assets/stylesheets/pages/profile.scss +++ b/app/assets/stylesheets/pages/profile.scss @@ -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 diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb index e37e9e136db..9eb75bb3891 100644 --- a/app/controllers/profiles/two_factor_auths_controller.rb +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -43,11 +43,11 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController # A U2F (universal 2nd factor) device's information is stored after successful # registration, which is then used while 2FA authentication is taking place. 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? 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 @qr_code = build_qr_code setup_u2f_registration @@ -91,15 +91,19 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController # Actual communication is performed using a Javascript API def setup_u2f_registration @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) 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) gon.push(u2f: { challenges: session[:challenges], app_id: u2f_app_id, register_requests: registration_requests, sign_requests: sign_requests }) end + + def u2f_registration_params + params.require(:u2f_registration).permit(:device_response, :name) + end end diff --git a/app/controllers/profiles/u2f_registrations_controller.rb b/app/controllers/profiles/u2f_registrations_controller.rb new file mode 100644 index 00000000000..c02fe85c3cc --- /dev/null +++ b/app/controllers/profiles/u2f_registrations_controller.rb @@ -0,0 +1,7 @@ +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 diff --git a/app/models/u2f_registration.rb b/app/models/u2f_registration.rb index 00b19686d48..808acec098f 100644 --- a/app/models/u2f_registration.rb +++ b/app/models/u2f_registration.rb @@ -3,18 +3,19 @@ class U2fRegistration < ActiveRecord::Base 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) registration = self.new 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.update(certificate: registration_data.certificate, key_handle: registration_data.key_handle, public_key: registration_data.public_key, counter: registration_data.counter, - user: user) + user: user, + name: params[:name]) rescue JSON::ParserError, NoMethodError, ArgumentError registration.errors.add(:base, 'Your U2F device did not send a valid JSON response.') rescue U2F::Error => e diff --git a/app/views/profiles/two_factor_auths/show.html.haml b/app/views/profiles/two_factor_auths/show.html.haml index 366f1fed35b..03ac739ade5 100644 --- a/app/views/profiles/two_factor_auths/show.html.haml +++ b/app/views/profiles/two_factor_auths/show.html.haml @@ -60,13 +60,38 @@ 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. .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? = form_errors(@u2f_registration) = 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? :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>"; diff --git a/app/views/u2f/_register.html.haml b/app/views/u2f/_register.html.haml index cbb8dfb7829..8f7b42eb351 100644 --- a/app/views/u2f/_register.html.haml +++ b/app/views/u2f/_register.html.haml @@ -28,10 +28,15 @@ %script#js-register-u2f-registered{ type: "text/template" } %div.row.append-bottom-10 - %p Your device was successfully set up! Click this button to register with the GitLab server. - = 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" - = submit_tag "Register U2F Device", class: "btn btn-success" + .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 + .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" :javascript var u2fRegister = new U2FRegister($("#js-register-u2f"), gon.u2f); diff --git a/config/routes.rb b/config/routes.rb index a62c460f055..867f3dac50b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -375,6 +375,8 @@ Rails.application.routes.draw do patch :skip end end + + resources :u2f_registrations, only: [:destroy] end end diff --git a/db/migrate/20160816161312_add_column_name_to_u2f_registrations.rb b/db/migrate/20160816161312_add_column_name_to_u2f_registrations.rb new file mode 100644 index 00000000000..7152bd04331 --- /dev/null +++ b/db/migrate/20160816161312_add_column_name_to_u2f_registrations.rb @@ -0,0 +1,29 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index 666a4bef574..d73c045c57c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # 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 enable_extension "plpgsql" @@ -1015,6 +1015,7 @@ ActiveRecord::Schema.define(version: 20160810142633) do t.integer "user_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.string "name" end add_index "u2f_registrations", ["key_handle"], name: "index_u2f_registrations_on_key_handle", using: :btree diff --git a/spec/features/u2f_spec.rb b/spec/features/u2f_spec.rb index d370f90f7d9..a46e48c76ed 100644 --- a/spec/features/u2f_spec.rb +++ b/spec/features/u2f_spec.rb @@ -12,10 +12,12 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: end 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 click_on 'Setup New U2F Device' expect(page).to have_content('Your device was successfully set up') + fill_in "Pick a name", with: name click_on 'Register U2F Device' u2f_device end @@ -40,13 +42,14 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: end 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 manage_two_factor_authentication 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') end @@ -55,15 +58,31 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: # First device manage_two_factor_authentication - register_u2f_device + first_device = register_u2f_device expect(page.body).to match('Your U2F device was registered') # Second device - manage_two_factor_authentication - register_u2f_device + second_device = register_u2f_device 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 - 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 @@ -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 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) unregistered_device.respond_to_u2f_authentication click_on "Login Via U2F Device" @@ -262,6 +281,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: end it "deletes u2f registrations" do + visit profile_account_path expect { click_on "Disable" }.to change { U2fRegistration.count }.by(-1) end end diff --git a/spec/support/fake_u2f_device.rb b/spec/support/fake_u2f_device.rb index f550e9a0160..8c407b867fe 100644 --- a/spec/support/fake_u2f_device.rb +++ b/spec/support/fake_u2f_device.rb @@ -1,6 +1,9 @@ class FakeU2fDevice - def initialize(page) + attr_reader :name + + def initialize(page, name) @page = page + @name = name end def respond_to_u2f_registration |