Commit 791cc913 authored by Timothy Andrew's avatar Timothy Andrew

Add a `U2fRegistrations` table/model.

- To hold registrations from U2F devices, and to authenticate them.
- Previously, `User#two_factor_enabled` was aliased to the
  `otp_required_for_login` column on `users`.
- This commit changes things a bit:
    - `User#two_factor_enabled` is not a method anymore
    - `User#two_factor_enabled?` checks both the
      `otp_required_for_login` column, as well as `U2fRegistration`s
    - Change all instances of `User#two_factor_enabled` to
      `User#two_factor_enabled?`
- Add the `u2f` gem, and implement registration/authentication at the
  model level.
parent fc809d68
......@@ -45,6 +45,7 @@ gem 'akismet', '~> 2.0'
gem 'devise-two-factor', '~> 3.0.0'
gem 'rqrcode-rails3', '~> 0.1.7'
gem 'attr_encrypted', '~> 3.0.0'
gem 'u2f', '~> 0.2.1'
# Browser detection
gem "browser", '~> 1.0.0'
......
......@@ -747,6 +747,7 @@ GEM
simple_oauth (~> 0.1.4)
tzinfo (1.2.2)
thread_safe (~> 0.1)
u2f (0.2.1)
uglifier (2.7.2)
execjs (>= 0.3.0)
json (>= 1.8.0)
......@@ -963,6 +964,7 @@ DEPENDENCIES
thin (~> 1.6.1)
tinder (~> 1.10.0)
turbolinks (~> 2.5.0)
u2f (~> 0.2.1)
uglifier (~> 2.7.2)
underscore-rails (~> 1.8.0)
unf (~> 0.1.4)
......
......@@ -182,8 +182,8 @@ class ApplicationController < ActionController::Base
end
def check_2fa_requirement
if two_factor_authentication_required? && current_user && !current_user.two_factor_enabled && !skip_two_factor?
redirect_to new_profile_two_factor_auth_path
if two_factor_authentication_required? && current_user && !current_user.two_factor_enabled? && !skip_two_factor?
redirect_to profile_two_factor_auth_path
end
end
......
......@@ -66,7 +66,7 @@ module AuthHelper
def two_factor_skippable?
current_application_settings.require_two_factor_authentication &&
!current_user.two_factor_enabled &&
!current_user.two_factor_enabled? &&
current_application_settings.two_factor_grace_period &&
!two_factor_grace_period_expired?
end
......
# Registration information for U2F (universal 2nd factor) devices, like Yubikeys
class U2fRegistration < ActiveRecord::Base
belongs_to :user
def self.register(user, app_id, json_response, challenges)
u2f = U2F::U2F.new(app_id)
registration = self.new
begin
response = U2F::RegisterResponse.load_from_json(json_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)
rescue JSON::ParserError, NoMethodError, ArgumentError
registration.errors.add(:base, 'Your U2F device did not send a valid JSON response.')
rescue U2F::Error => e
registration.errors.add(:base, e.message)
end
registration
end
def self.authenticate(user, app_id, json_response, challenges)
response = U2F::SignResponse.load_from_json(json_response)
registration = user.u2f_registrations.find_by_key_handle(response.key_handle)
u2f = U2F::U2F.new(app_id)
if registration
u2f.authenticate!(challenges, response, Base64.decode64(registration.public_key), registration.counter)
registration.update(counter: response.counter)
true
end
rescue JSON::ParserError, NoMethodError, ArgumentError, U2F::Error
false
end
end
......@@ -27,7 +27,6 @@ class User < ActiveRecord::Base
devise :two_factor_authenticatable,
otp_secret_encryption_key: Gitlab::Application.config.secret_key_base
alias_attribute :two_factor_enabled, :otp_required_for_login
devise :two_factor_backupable, otp_number_of_backup_codes: 10
serialize :otp_backup_codes, JSON
......@@ -51,6 +50,7 @@ class User < ActiveRecord::Base
has_many :keys, dependent: :destroy
has_many :emails, dependent: :destroy
has_many :identities, dependent: :destroy, autosave: true
has_many :u2f_registrations, dependent: :destroy
# Groups
has_many :members, dependent: :destroy
......@@ -175,8 +175,16 @@ class User < ActiveRecord::Base
scope :active, -> { with_state(:active) }
scope :not_in_project, ->(project) { project.users.present? ? where("id not in (:ids)", ids: project.users.map(&:id) ) : all }
scope :without_projects, -> { where('id NOT IN (SELECT DISTINCT(user_id) FROM members)') }
scope :with_two_factor, -> { where(two_factor_enabled: true) }
scope :without_two_factor, -> { where(two_factor_enabled: false) }
def self.with_two_factor
joins("LEFT OUTER JOIN u2f_registrations AS u2f ON u2f.user_id = users.id").
where("u2f.id IS NOT NULL OR otp_required_for_login = ?", true).distinct(arel_table[:id])
end
def self.without_two_factor
joins("LEFT OUTER JOIN u2f_registrations AS u2f ON u2f.user_id = users.id").
where("u2f.id IS NULL AND otp_required_for_login = ?", false)
end
#
# Class methods
......@@ -323,14 +331,29 @@ class User < ActiveRecord::Base
end
def disable_two_factor!
transaction do
update_attributes(
two_factor_enabled: false,
otp_required_for_login: false,
encrypted_otp_secret: nil,
encrypted_otp_secret_iv: nil,
encrypted_otp_secret_salt: nil,
otp_grace_period_started_at: nil,
otp_backup_codes: nil
)
self.u2f_registrations.destroy_all
end
end
def two_factor_enabled?
two_factor_otp_enabled? || two_factor_u2f_enabled?
end
def two_factor_otp_enabled?
self.otp_required_for_login?
end
def two_factor_u2f_enabled?
self.u2f_registrations.exists?
end
def namespace_uniq
......
class CreateU2fRegistrations < ActiveRecord::Migration
def change
create_table :u2f_registrations do |t|
t.text :certificate
t.string :key_handle, index: true
t.string :public_key
t.integer :counter
t.references :user, index: true, foreign_key: true
t.timestamps null: false
end
end
end
......@@ -12,7 +12,6 @@
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20160530150109) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
enable_extension "pg_trgm"
......@@ -940,6 +939,19 @@ ActiveRecord::Schema.define(version: 20160530150109) do
add_index "todos", ["target_type", "target_id"], name: "index_todos_on_target_type_and_target_id", using: :btree
add_index "todos", ["user_id"], name: "index_todos_on_user_id", using: :btree
create_table "u2f_registrations", force: :cascade do |t|
t.text "certificate"
t.string "key_handle"
t.string "public_key"
t.integer "counter"
t.integer "user_id"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
end
add_index "u2f_registrations", ["key_handle"], name: "index_u2f_registrations_on_key_handle", using: :btree
add_index "u2f_registrations", ["user_id"], name: "index_u2f_registrations_on_user_id", using: :btree
create_table "users", force: :cascade do |t|
t.string "email", default: "", null: false
t.string "encrypted_password", default: "", null: false
......@@ -1047,4 +1059,5 @@ ActiveRecord::Schema.define(version: 20160530150109) do
add_index "web_hooks", ["created_at", "id"], name: "index_web_hooks_on_created_at_and_id", using: :btree
add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree
add_foreign_key "u2f_registrations", "users"
end
......@@ -30,7 +30,7 @@ module API
expose :identities, using: Entities::Identity
expose :can_create_group?, as: :can_create_group
expose :can_create_project?, as: :can_create_project
expose :two_factor_enabled
expose :two_factor_enabled?, as: :two_factor_enabled
expose :external
end
......
FactoryGirl.define do
factory :u2f_registration do
certificate { FFaker::BaconIpsum.characters(728) }
key_handle { FFaker::BaconIpsum.characters(86) }
public_key { FFaker::BaconIpsum.characters(88) }
counter 0
end
end
......@@ -15,14 +15,26 @@ FactoryGirl.define do
end
trait :two_factor do
two_factor_via_otp
end
trait :two_factor_via_otp do
before(:create) do |user|
user.two_factor_enabled = true
user.otp_required_for_login = true
user.otp_secret = User.generate_otp_secret(32)
user.otp_grace_period_started_at = Time.now
user.generate_otp_backup_codes!
end
end
trait :two_factor_via_u2f do
transient { registrations_count 5 }
after(:create) do |user, evaluator|
create_list(:u2f_registration, evaluator.registrations_count, user: user)
end
end
factory :omniauth_user do
transient do
extern_uid '123456'
......
......@@ -19,7 +19,7 @@ describe "Admin::Users", feature: true do
describe 'Two-factor Authentication filters' do
it 'counts users who have enabled 2FA' do
create(:user, two_factor_enabled: true)
create(:user, :two_factor)
visit admin_users_path
......@@ -29,7 +29,7 @@ describe "Admin::Users", feature: true do
end
it 'filters by users who have enabled 2FA' do
user = create(:user, two_factor_enabled: true)
user = create(:user, :two_factor)
visit admin_users_path
click_link '2FA Enabled'
......@@ -38,7 +38,7 @@ describe "Admin::Users", feature: true do
end
it 'counts users who have not enabled 2FA' do
create(:user, two_factor_enabled: false)
create(:user)
visit admin_users_path
......@@ -48,7 +48,7 @@ describe "Admin::Users", feature: true do
end
it 'filters by users who have not enabled 2FA' do
user = create(:user, two_factor_enabled: false)
user = create(:user)
visit admin_users_path
click_link '2FA Disabled'
......@@ -173,7 +173,7 @@ describe "Admin::Users", feature: true do
describe 'Two-factor Authentication status' do
it 'shows when enabled' do
@user.update_attribute(:two_factor_enabled, true)
@user.update_attribute(:otp_required_for_login, true)
visit admin_user_path(@user)
......
......@@ -121,6 +121,66 @@ describe User, models: true do
end
end
describe "scopes" do
describe ".with_two_factor" do
it "returns users with 2fa enabled via OTP" do
user_with_2fa = create(:user, :two_factor_via_otp)
user_without_2fa = create(:user)
users_with_two_factor = User.with_two_factor.pluck(:id)
expect(users_with_two_factor).to include(user_with_2fa.id)
expect(users_with_two_factor).not_to include(user_without_2fa.id)
end
it "returns users with 2fa enabled via U2F" do
user_with_2fa = create(:user, :two_factor_via_u2f)
user_without_2fa = create(:user)
users_with_two_factor = User.with_two_factor.pluck(:id)
expect(users_with_two_factor).to include(user_with_2fa.id)
expect(users_with_two_factor).not_to include(user_without_2fa.id)
end
it "returns users with 2fa enabled via OTP and U2F" do
user_with_2fa = create(:user, :two_factor_via_otp, :two_factor_via_u2f)
user_without_2fa = create(:user)
users_with_two_factor = User.with_two_factor.pluck(:id)
expect(users_with_two_factor).to eq([user_with_2fa.id])
expect(users_with_two_factor).not_to include(user_without_2fa.id)
end
end
describe ".without_two_factor" do
it "excludes users with 2fa enabled via OTP" do
user_with_2fa = create(:user, :two_factor_via_otp)
user_without_2fa = create(:user)
users_without_two_factor = User.without_two_factor.pluck(:id)
expect(users_without_two_factor).to include(user_without_2fa.id)
expect(users_without_two_factor).not_to include(user_with_2fa.id)
end
it "excludes users with 2fa enabled via U2F" do
user_with_2fa = create(:user, :two_factor_via_u2f)
user_without_2fa = create(:user)
users_without_two_factor = User.without_two_factor.pluck(:id)
expect(users_without_two_factor).to include(user_without_2fa.id)
expect(users_without_two_factor).not_to include(user_with_2fa.id)
end
it "excludes users with 2fa enabled via OTP and U2F" do
user_with_2fa = create(:user, :two_factor_via_otp, :two_factor_via_u2f)
user_without_2fa = create(:user)
users_without_two_factor = User.without_two_factor.pluck(:id)
expect(users_without_two_factor).to include(user_without_2fa.id)
expect(users_without_two_factor).not_to include(user_with_2fa.id)
end
end
end
describe "Respond to" do
it { is_expected.to respond_to(:is_admin?) }
it { is_expected.to respond_to(:name) }
......
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