From fe75411df8d8ed41e89cfcae73d1ea34b8b339b9 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 27 Mar 2015 15:35:26 -0700 Subject: Add 2 factor authentication gems --- Gemfile | 4 ++++ Gemfile.lock | 16 ++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/Gemfile b/Gemfile index f950c5be154..65a641c602c 100644 --- a/Gemfile +++ b/Gemfile @@ -34,6 +34,10 @@ gem 'omniauth-bitbucket' gem 'doorkeeper', '2.1.3' gem "rack-oauth2", "~> 1.0.5" +# Two-factor authentication +gem 'devise-two-factor' +gem 'rqrcode-rails3' + # Browser detection gem "browser" diff --git a/Gemfile.lock b/Gemfile.lock index 6f58c4f4fda..35faa9895ba 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -46,6 +46,8 @@ GEM ast (2.0.0) astrolabe (1.3.0) parser (>= 2.2.0.pre.3, < 3.0) + attr_encrypted (1.3.3) + encryptor (>= 1.3.0) attr_required (1.0.0) autoprefixer-rails (5.1.11) execjs @@ -136,6 +138,13 @@ GEM warden (~> 1.2.3) devise-async (0.9.0) devise (~> 3.2) + devise-two-factor (1.0.1) + activemodel + activesupport + attr_encrypted (~> 1.3.2) + devise (~> 3.2.4) + rails + rotp (~> 1.6.1) diff-lcs (1.2.5) diffy (3.0.3) docile (1.1.5) @@ -147,6 +156,7 @@ GEM email_spec (1.5.0) launchy (~> 2.1) mail (~> 2.2) + encryptor (1.3.0) enumerize (0.7.0) activesupport (>= 3.2) equalizer (0.0.8) @@ -482,7 +492,11 @@ GEM rest-client (1.6.7) mime-types (>= 1.16) rinku (1.7.3) + rotp (1.6.1) rouge (1.7.7) + rqrcode (0.4.2) + rqrcode-rails3 (0.1.7) + rqrcode (>= 0.4.2) rspec (2.99.0) rspec-core (~> 2.99.0) rspec-expectations (~> 2.99.0) @@ -691,6 +705,7 @@ DEPENDENCIES default_value_for (~> 3.0.0) devise (= 3.2.4) devise-async (= 0.9.0) + devise-two-factor diffy (~> 3.0.3) doorkeeper (= 2.1.3) dropzonejs-rails @@ -762,6 +777,7 @@ DEPENDENCIES redis-rails request_store rerun (~> 0.10.0) + rqrcode-rails3 rspec-rails (= 2.99) rubocop (= 0.28.0) rugments -- cgit v1.2.1 From 7302395142dc93a45239c993b69958ca4a757c92 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 27 Mar 2015 16:23:33 -0700 Subject: Init 2 factor authentication for user model --- app/models/user.rb | 5 ++++- app/views/devise/sessions/_new_base.html.haml | 3 ++- config/initializers/devise.rb | 4 ++++ db/migrate/20150327223628_add_devise_two_factor_to_users.rb | 8 ++++++++ 4 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20150327223628_add_devise_two_factor_to_users.rb diff --git a/app/models/user.rb b/app/models/user.rb index 1cf7cfea974..b9e28900187 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -57,6 +57,9 @@ require 'carrierwave/orm/activerecord' require 'file_size_validator' class User < ActiveRecord::Base + devise :two_factor_authenticatable, + :otp_secret_encryption_key => File.read(Rails.root.join('.secret')).chomp + include Sortable include Gitlab::ConfigHelper include TokenAuthenticatable @@ -70,7 +73,7 @@ class User < ActiveRecord::Base default_value_for :hide_no_password, false default_value_for :theme_id, gitlab_config.default_theme - devise :database_authenticatable, :lockable, :async, + devise :lockable, :async, :recoverable, :rememberable, :trackable, :validatable, :omniauthable, :confirmable, :registerable attr_accessor :force_random_password diff --git a/app/views/devise/sessions/_new_base.html.haml b/app/views/devise/sessions/_new_base.html.haml index 54a39726771..4ecb74fb56e 100644 --- a/app/views/devise/sessions/_new_base.html.haml +++ b/app/views/devise/sessions/_new_base.html.haml @@ -1,6 +1,7 @@ = form_for(resource, as: resource_name, url: session_path(resource_name)) do |f| = f.text_field :login, class: "form-control top", placeholder: "Username or Email", autofocus: "autofocus" - = f.password_field :password, class: "form-control bottom", placeholder: "Password" + = f.password_field :password, class: "form-control middle", placeholder: "Password" + = f.text_field :otp_attempt, class: 'form-control bottom', placeholder: 'Two-factor authentication token' - if devise_mapping.rememberable? .remember-me.checkbox %label{for: "user_remember_me"} diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 8f8c4169740..956bb048b2a 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -1,6 +1,10 @@ # Use this hook to configure devise mailer, warden hooks and so forth. The first # four configuration values can also be set straight in your models. Devise.setup do |config| + config.warden do |manager| + manager.default_strategies(:scope => :user).unshift :two_factor_authenticatable + end + # ==> Mailer Configuration # Configure the class responsible to send e-mails. config.mailer = "DeviseMailer" diff --git a/db/migrate/20150327223628_add_devise_two_factor_to_users.rb b/db/migrate/20150327223628_add_devise_two_factor_to_users.rb new file mode 100644 index 00000000000..11b026ee8f3 --- /dev/null +++ b/db/migrate/20150327223628_add_devise_two_factor_to_users.rb @@ -0,0 +1,8 @@ +class AddDeviseTwoFactorToUsers < ActiveRecord::Migration + def change + add_column :users, :encrypted_otp_secret, :string + add_column :users, :encrypted_otp_secret_iv, :string + add_column :users, :encrypted_otp_secret_salt, :string + add_column :users, :otp_required_for_login, :boolean + end +end -- cgit v1.2.1 From ba7e2fd946ba94a9c0b3b18c3f7fc91f63fc652a Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 27 Mar 2015 17:01:24 -0700 Subject: Create Two-factor authentication resource for user --- .../profiles/two_factor_auths_controller.rb | 22 ++++++++++++++++++++++ app/views/profiles/accounts/show.html.haml | 7 +++++++ app/views/profiles/two_factor_auths/new.html.haml | 18 ++++++++++++++++++ config/routes.rb | 1 + 4 files changed, 48 insertions(+) create mode 100644 app/controllers/profiles/two_factor_auths_controller.rb create mode 100644 app/views/profiles/two_factor_auths/new.html.haml diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb new file mode 100644 index 00000000000..92ba842fac4 --- /dev/null +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -0,0 +1,22 @@ +class Profiles::TwoFactorAuthsController < ApplicationController + def new + issuer = "GitLab | #{current_user.email}" + uri = current_user.otp_provisioning_uri(current_user.email, issuer: issuer) + @qr_code = RQRCode::render_qrcode(uri, :svg, level: :l, unit: 2) + end + + def create + current_user.otp_required_for_login = true + current_user.otp_secret = User.generate_otp_secret + current_user.save! + + redirect_to profile_account_path + end + + def destroy + current_user.otp_required_for_login = false + current_user.save! + + redirect_to profile_account_path + end +end diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml index 1c3a3d68aca..bbcd3baf61b 100644 --- a/app/views/profiles/accounts/show.html.haml +++ b/app/views/profiles/accounts/show.html.haml @@ -26,6 +26,13 @@ %span You don`t have one yet. Click generate to fix it. = f.submit 'Generate', class: "btn success btn-build-token" + %fieldset + %legend Two-Factor Authentication + %p + Keep your account secure by enabling two-factor authentication. + Each time you log in, you’ll be required to provide your password plus a randomly generated access code. + %div + = link_to "Enable 2-Factor Authentication", new_profile_two_factor_auth_path, class: 'btn btn-success' - if show_profile_social_tab? %fieldset diff --git a/app/views/profiles/two_factor_auths/new.html.haml b/app/views/profiles/two_factor_auths/new.html.haml new file mode 100644 index 00000000000..77329de2e01 --- /dev/null +++ b/app/views/profiles/two_factor_auths/new.html.haml @@ -0,0 +1,18 @@ +%h2.page-title Two-Factor Authentication (TFA) +%p + Download the Google Authenticator application from App Store for iOS or + Google Play for Android and scan this code. + +%hr + += form_tag new_profile_two_factor_auth_path, method: :post, class: 'form-horizontal' do |f| + .form-group + .col-sm-2 + .col-sm-10 + = raw @qr_code + .form-group + = label_tag :pin_code, nil, class: "control-label" + .col-sm-10 + = text_field_tag :pin_code, nil, class: "form-control", required: true + .form-actions + = submit_tag 'Submit', class: 'btn btn-success' diff --git a/config/routes.rb b/config/routes.rb index 4b38dede7b4..a76ababb3d9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -226,6 +226,7 @@ Gitlab::Application.routes.draw do resources :keys resources :emails, only: [:index, :create, :destroy] resource :avatar, only: [:destroy] + resource :two_factor_auth, only: [:new, :create, :destroy] end end -- cgit v1.2.1 From cde474a49f0ff44350d813aba83b6880df960f15 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 27 Mar 2015 17:53:08 -0700 Subject: Make 2 factor authentication work --- .../profiles/two_factor_auths_controller.rb | 31 +++++++++++++++++----- app/views/profiles/accounts/show.html.haml | 10 ++++++- app/views/profiles/two_factor_auths/new.html.haml | 5 +++- 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb index 92ba842fac4..ac14d5ca75b 100644 --- a/app/controllers/profiles/two_factor_auths_controller.rb +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -1,16 +1,25 @@ class Profiles::TwoFactorAuthsController < ApplicationController def new - issuer = "GitLab | #{current_user.email}" - uri = current_user.otp_provisioning_uri(current_user.email, issuer: issuer) - @qr_code = RQRCode::render_qrcode(uri, :svg, level: :l, unit: 2) + unless current_user.otp_secret + current_user.otp_secret = User.generate_otp_secret + current_user.save! + end + + @qr_code = build_qr_code end def create - current_user.otp_required_for_login = true - current_user.otp_secret = User.generate_otp_secret - current_user.save! + if current_user.valid_otp?(params[:pin_code]) + current_user.otp_required_for_login = true + #current_user.otp_secret = User.generate_otp_secret + current_user.save! - redirect_to profile_account_path + redirect_to profile_account_path + else + @error = 'Invalid pin code' + @qr_code = build_qr_code + render 'new' + end end def destroy @@ -19,4 +28,12 @@ class Profiles::TwoFactorAuthsController < ApplicationController redirect_to profile_account_path end + + private + + def build_qr_code + issuer = "GitLab | #{current_user.email}" + uri = current_user.otp_provisioning_uri(current_user.email, issuer: issuer) + RQRCode::render_qrcode(uri, :svg, level: :m, unit: 3) + end end diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml index bbcd3baf61b..19b0c5bcb41 100644 --- a/app/views/profiles/accounts/show.html.haml +++ b/app/views/profiles/accounts/show.html.haml @@ -30,9 +30,17 @@ %legend Two-Factor Authentication %p Keep your account secure by enabling two-factor authentication. + %br Each time you log in, you’ll be required to provide your password plus a randomly generated access code. %div - = link_to "Enable 2-Factor Authentication", new_profile_two_factor_auth_path, class: 'btn btn-success' + - if current_user.otp_required_for_login + %strong.text-success + %i.fa.fa-check + 2-Factor Authentication enabled + .pull-right + = link_to "Disable 2-Factor Authentication", profile_two_factor_auth_path, method: :delete, class: 'btn btn-close btn-sm' + - else + = link_to "Enable 2-Factor Authentication", new_profile_two_factor_auth_path, class: 'btn btn-success' - if show_profile_social_tab? %fieldset diff --git a/app/views/profiles/two_factor_auths/new.html.haml b/app/views/profiles/two_factor_auths/new.html.haml index 77329de2e01..8332fc6b8b8 100644 --- a/app/views/profiles/two_factor_auths/new.html.haml +++ b/app/views/profiles/two_factor_auths/new.html.haml @@ -5,7 +5,10 @@ %hr -= form_tag new_profile_two_factor_auth_path, method: :post, class: 'form-horizontal' do |f| += form_tag profile_two_factor_auth_path, method: :post, class: 'form-horizontal' do |f| + - if @error + .alert.alert-danger + = @error .form-group .col-sm-2 .col-sm-10 -- cgit v1.2.1 From 50a2a229e7b8b789a199bd0cf84ce76d25201198 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 27 Mar 2015 17:54:44 -0700 Subject: Fix rubocop complain --- app/controllers/profiles/two_factor_auths_controller.rb | 1 - app/models/user.rb | 2 +- config/initializers/devise.rb | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb index ac14d5ca75b..9b4070a76f7 100644 --- a/app/controllers/profiles/two_factor_auths_controller.rb +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -11,7 +11,6 @@ class Profiles::TwoFactorAuthsController < ApplicationController def create if current_user.valid_otp?(params[:pin_code]) current_user.otp_required_for_login = true - #current_user.otp_secret = User.generate_otp_secret current_user.save! redirect_to profile_account_path diff --git a/app/models/user.rb b/app/models/user.rb index b9e28900187..befbcbf1a16 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -58,7 +58,7 @@ require 'file_size_validator' class User < ActiveRecord::Base devise :two_factor_authenticatable, - :otp_secret_encryption_key => File.read(Rails.root.join('.secret')).chomp + otp_secret_encryption_key: File.read(Rails.root.join('.secret')).chomp include Sortable include Gitlab::ConfigHelper diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 956bb048b2a..c003a7102a5 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -2,7 +2,7 @@ # four configuration values can also be set straight in your models. Devise.setup do |config| config.warden do |manager| - manager.default_strategies(:scope => :user).unshift :two_factor_authenticatable + manager.default_strategies(scope: :user).unshift :two_factor_authenticatable end # ==> Mailer Configuration -- cgit v1.2.1 From de9e1c3bad18e4ca00cfdced75e5cc4c42905761 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 31 Mar 2015 04:19:01 +0300 Subject: Turn 2-factor authentication into 2 steps process. Disabled 2fa UI for ldap users since it is not supported --- app/controllers/application_controller.rb | 2 +- app/controllers/sessions_controller.rb | 24 ++++++++++++++++++++ app/views/devise/sessions/_new_base.html.haml | 4 ++-- app/views/devise/sessions/two_factor.html.haml | 16 +++++++++++++ app/views/profiles/accounts/show.html.haml | 31 +++++++++++++------------- 5 files changed, 59 insertions(+), 18 deletions(-) create mode 100644 app/views/devise/sessions/two_factor.html.haml diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index eee10d6c22a..247f5cc32d3 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -252,7 +252,7 @@ class ApplicationController < ActionController::Base end def configure_permitted_parameters - devise_parameter_sanitizer.sanitize(:sign_in) { |u| u.permit(:username, :email, :password, :login, :remember_me) } + devise_parameter_sanitizer.sanitize(:sign_in) { |u| u.permit(:username, :email, :password, :login, :remember_me, :otp_attempt) } end def hexdigest(string) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 3f11d7afe6f..68cd02b2d79 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -1,4 +1,6 @@ class SessionsController < Devise::SessionsController + prepend_before_filter :two_factor_enabled?, only: :create + def new redirect_path = if request.referer.present? && (params['redirect_to_referer'] == 'yes') @@ -34,4 +36,26 @@ class SessionsController < Devise::SessionsController end end end + + private + + def two_factor_enabled? + user_params = params[:user] + @user = User.by_login(user_params[:login]) + + if user_params[:otp_attempt].present? + unless @user.valid_otp?(user_params[:otp_attempt]) + @error = 'Invalid two-factor code' + render :two_factor and return + end + else + if @user && @user.valid_password?(params[:user][:password]) + self.resource = @user + + if resource.otp_required_for_login + render :two_factor and return + end + end + end + end end diff --git a/app/views/devise/sessions/_new_base.html.haml b/app/views/devise/sessions/_new_base.html.haml index 4ecb74fb56e..dcb989e8d81 100644 --- a/app/views/devise/sessions/_new_base.html.haml +++ b/app/views/devise/sessions/_new_base.html.haml @@ -1,7 +1,7 @@ = form_for(resource, as: resource_name, url: session_path(resource_name)) do |f| = f.text_field :login, class: "form-control top", placeholder: "Username or Email", autofocus: "autofocus" - = f.password_field :password, class: "form-control middle", placeholder: "Password" - = f.text_field :otp_attempt, class: 'form-control bottom', placeholder: 'Two-factor authentication token' + = f.password_field :password, class: "form-control bottom", placeholder: "Password" + = f.hidden_field :otp_attempt, value: '' - if devise_mapping.rememberable? .remember-me.checkbox %label{for: "user_remember_me"} diff --git a/app/views/devise/sessions/two_factor.html.haml b/app/views/devise/sessions/two_factor.html.haml new file mode 100644 index 00000000000..3a19e926b6d --- /dev/null +++ b/app/views/devise/sessions/two_factor.html.haml @@ -0,0 +1,16 @@ +%div + .login-box + .login-heading + %h3 Two-Factor Authentication + .login-body + = form_for(resource, as: resource_name, url: session_path(resource_name), method: :post) do |f| + - if @error + .alert.alert-danger + = @error + .hide + = f.text_field :login, class: "form-control top", placeholder: "Username or Email", autofocus: "autofocus" + = f.password_field :password, class: "form-control bottom", placeholder: "Password" + = f.text_field :otp_attempt, class: 'form-control', + placeholder: 'Two-factor authentication token', required: true, autofocus: true + .prepend-top-20 + = f.submit "Verify code", class: "btn btn-save" diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml index 19b0c5bcb41..dcce29a81f4 100644 --- a/app/views/profiles/accounts/show.html.haml +++ b/app/views/profiles/accounts/show.html.haml @@ -26,21 +26,22 @@ %span You don`t have one yet. Click generate to fix it. = f.submit 'Generate', class: "btn success btn-build-token" - %fieldset - %legend Two-Factor Authentication - %p - Keep your account secure by enabling two-factor authentication. - %br - Each time you log in, you’ll be required to provide your password plus a randomly generated access code. - %div - - if current_user.otp_required_for_login - %strong.text-success - %i.fa.fa-check - 2-Factor Authentication enabled - .pull-right - = link_to "Disable 2-Factor Authentication", profile_two_factor_auth_path, method: :delete, class: 'btn btn-close btn-sm' - - else - = link_to "Enable 2-Factor Authentication", new_profile_two_factor_auth_path, class: 'btn btn-success' + - unless current_user.ldap_user? + %fieldset + %legend Two-Factor Authentication + %p + Keep your account secure by enabling two-factor authentication. + %br + Each time you log in, you’ll be required to provide your password plus a randomly generated access code. + %div + - if current_user.otp_required_for_login + %strong.text-success + %i.fa.fa-check + 2-Factor Authentication enabled + .pull-right + = link_to "Disable 2-Factor Authentication", profile_two_factor_auth_path, method: :delete, class: 'btn btn-close btn-sm' + - else + = link_to "Enable 2-Factor Authentication", new_profile_two_factor_auth_path, class: 'btn btn-success' - if show_profile_social_tab? %fieldset -- cgit v1.2.1 From b66be0a2b3351dd10d96e3d0d0576f6d1444f342 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 31 Mar 2015 05:34:50 +0300 Subject: Use non-broken version of attr_encrypted --- Gemfile | 1 + Gemfile.lock | 11 +++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/Gemfile b/Gemfile index 65a641c602c..ee6072b2da1 100644 --- a/Gemfile +++ b/Gemfile @@ -37,6 +37,7 @@ gem "rack-oauth2", "~> 1.0.5" # Two-factor authentication gem 'devise-two-factor' gem 'rqrcode-rails3' +gem 'attr_encrypted', git: 'https://github.com/attr-encrypted/attr_encrypted.git', ref: '94d901df2ccbc579b981091d53dd641f9bed4c1d' # Browser detection gem "browser" diff --git a/Gemfile.lock b/Gemfile.lock index 35faa9895ba..672bd9950ed 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,3 +1,11 @@ +GIT + remote: https://github.com/attr-encrypted/attr_encrypted.git + revision: 94d901df2ccbc579b981091d53dd641f9bed4c1d + ref: 94d901df2ccbc579b981091d53dd641f9bed4c1d + specs: + attr_encrypted (1.3.3) + encryptor (>= 1.3.0) + GEM remote: https://rubygems.org/ specs: @@ -46,8 +54,6 @@ GEM ast (2.0.0) astrolabe (1.3.0) parser (>= 2.2.0.pre.3, < 3.0) - attr_encrypted (1.3.3) - encryptor (>= 1.3.0) attr_required (1.0.0) autoprefixer-rails (5.1.11) execjs @@ -684,6 +690,7 @@ DEPENDENCIES annotate (~> 2.6.0.beta2) asana (~> 0.0.6) asciidoctor (= 0.1.4) + attr_encrypted! awesome_print better_errors binding_of_caller -- cgit v1.2.1 From 802fcd051fcbc9be99230bddb13c4a7cb8067741 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 31 Mar 2015 22:42:17 +0300 Subject: Add support for backup codes --- .../profiles/two_factor_auths_controller.rb | 6 ++++ app/controllers/sessions_controller.rb | 3 +- app/views/profiles/accounts/show.html.haml | 35 ++++++++++++++-------- config/initializers/devise.rb | 1 + config/routes.rb | 6 +++- ...02_add_devise_two_factor_backupable_to_users.rb | 5 ++++ 6 files changed, 42 insertions(+), 14 deletions(-) create mode 100644 db/migrate/20150331183602_add_devise_two_factor_backupable_to_users.rb diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb index 9b4070a76f7..2841a07efbc 100644 --- a/app/controllers/profiles/two_factor_auths_controller.rb +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -21,6 +21,12 @@ class Profiles::TwoFactorAuthsController < ApplicationController end end + def codes + codes = current_user.generate_otp_backup_codes! + current_user.save! + send_data codes.join("\n"), filename: 'gitlab_recovery_codes.txt' + end + def destroy current_user.otp_required_for_login = false current_user.save! diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 68cd02b2d79..cc9d30d64d5 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -44,7 +44,8 @@ class SessionsController < Devise::SessionsController @user = User.by_login(user_params[:login]) if user_params[:otp_attempt].present? - unless @user.valid_otp?(user_params[:otp_attempt]) + unless @user.valid_otp?(user_params[:otp_attempt]) || + @user.recovery_code?(user_params[:otp_attempt]) @error = 'Invalid two-factor code' render :two_factor and return end diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml index dcce29a81f4..1e024c45f43 100644 --- a/app/views/profiles/accounts/show.html.haml +++ b/app/views/profiles/accounts/show.html.haml @@ -28,20 +28,31 @@ - unless current_user.ldap_user? %fieldset - %legend Two-Factor Authentication - %p - Keep your account secure by enabling two-factor authentication. - %br - Each time you log in, you’ll be required to provide your password plus a randomly generated access code. - %div - - if current_user.otp_required_for_login - %strong.text-success - %i.fa.fa-check - 2-Factor Authentication enabled + - if current_user.otp_required_for_login + %legend.text-success + %i.fa.fa-check + Two-Factor Authentication enabled + %div .pull-right = link_to "Disable 2-Factor Authentication", profile_two_factor_auth_path, method: :delete, class: 'btn btn-close btn-sm' - - else - = link_to "Enable 2-Factor Authentication", new_profile_two_factor_auth_path, class: 'btn btn-success' + %p.slead + %i.fa.fa-warning + Please + %strong #{link_to "download recovery codes", codes_profile_two_factor_auth_path} + so you can access your account if you lose your phone. + %br + %i.fa.fa-warning + Every time you download recovery codes - we generate the new codes. Previously downloaded codes won't work anymore. + + - else + %legend Two-Factor Authentication + %div + %p + Keep your account secure by enabling two-factor authentication. + %br + Each time you log in, you’ll be required to provide your password plus a randomly generated access code. + %div + = link_to "Enable 2-Factor Authentication", new_profile_two_factor_auth_path, class: 'btn btn-success' - if show_profile_social_tab? %fieldset diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index c003a7102a5..091548348b1 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -3,6 +3,7 @@ Devise.setup do |config| config.warden do |manager| manager.default_strategies(scope: :user).unshift :two_factor_authenticatable + manager.default_strategies(scope: :user).unshift :two_factor_backupable end # ==> Mailer Configuration diff --git a/config/routes.rb b/config/routes.rb index a76ababb3d9..bcd68ad6ae2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -226,7 +226,11 @@ Gitlab::Application.routes.draw do resources :keys resources :emails, only: [:index, :create, :destroy] resource :avatar, only: [:destroy] - resource :two_factor_auth, only: [:new, :create, :destroy] + resource :two_factor_auth, only: [:new, :create, :destroy] do + member do + get :codes + end + end end end diff --git a/db/migrate/20150331183602_add_devise_two_factor_backupable_to_users.rb b/db/migrate/20150331183602_add_devise_two_factor_backupable_to_users.rb new file mode 100644 index 00000000000..2feb49f43f1 --- /dev/null +++ b/db/migrate/20150331183602_add_devise_two_factor_backupable_to_users.rb @@ -0,0 +1,5 @@ +class AddDeviseTwoFactorBackupableToUsers < ActiveRecord::Migration + def change + add_column :users, :otp_backup_codes, :string, array: true + end +end -- cgit v1.2.1 From 8ae712ae28eda669987c48a755fc9f94ac48b5e7 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 2 Apr 2015 11:57:36 -0700 Subject: Render 2fa recovery codes instead of downloading it --- app/controllers/profiles/two_factor_auths_controller.rb | 6 +++--- app/views/profiles/accounts/show.html.haml | 13 +++++-------- app/views/profiles/two_factor_auths/_codes.html.haml | 12 ++++++++++++ app/views/profiles/two_factor_auths/codes.html.haml | 3 +++ app/views/profiles/two_factor_auths/create.html.haml | 4 ++++ config/routes.rb | 2 +- 6 files changed, 28 insertions(+), 12 deletions(-) create mode 100644 app/views/profiles/two_factor_auths/_codes.html.haml create mode 100644 app/views/profiles/two_factor_auths/codes.html.haml create mode 100644 app/views/profiles/two_factor_auths/create.html.haml diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb index 2841a07efbc..5c51706c3d4 100644 --- a/app/controllers/profiles/two_factor_auths_controller.rb +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -11,9 +11,10 @@ class Profiles::TwoFactorAuthsController < ApplicationController def create if current_user.valid_otp?(params[:pin_code]) current_user.otp_required_for_login = true + @codes = current_user.generate_otp_backup_codes! current_user.save! - redirect_to profile_account_path + render 'create' else @error = 'Invalid pin code' @qr_code = build_qr_code @@ -22,9 +23,8 @@ class Profiles::TwoFactorAuthsController < ApplicationController end def codes - codes = current_user.generate_otp_backup_codes! + @codes = current_user.generate_otp_backup_codes! current_user.save! - send_data codes.join("\n"), filename: 'gitlab_recovery_codes.txt' end def destroy diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml index 1e024c45f43..e8b4943833e 100644 --- a/app/views/profiles/accounts/show.html.haml +++ b/app/views/profiles/accounts/show.html.haml @@ -35,14 +35,11 @@ %div .pull-right = link_to "Disable 2-Factor Authentication", profile_two_factor_auth_path, method: :delete, class: 'btn btn-close btn-sm' - %p.slead - %i.fa.fa-warning - Please - %strong #{link_to "download recovery codes", codes_profile_two_factor_auth_path} - so you can access your account if you lose your phone. - %br - %i.fa.fa-warning - Every time you download recovery codes - we generate the new codes. Previously downloaded codes won't work anymore. + %p + If you lost your recovery codes - you can + %strong + = link_to "generate new one", codes_profile_two_factor_auth_path, method: :post, + data: { confirm: 'After we generate new recovery codes - old codes will not be valid any more. Are you sure?' } - else %legend Two-Factor Authentication diff --git a/app/views/profiles/two_factor_auths/_codes.html.haml b/app/views/profiles/two_factor_auths/_codes.html.haml new file mode 100644 index 00000000000..dd043f3c5b7 --- /dev/null +++ b/app/views/profiles/two_factor_auths/_codes.html.haml @@ -0,0 +1,12 @@ +%p.slead + Please save this recovery codes so you can access your account if you lose your phone. + +.codes.well + %ul + - @codes.each do |code| + %li + %span.monospace + = code + += link_to profile_account_path, class: 'btn btn-success' do + I saved the codes diff --git a/app/views/profiles/two_factor_auths/codes.html.haml b/app/views/profiles/two_factor_auths/codes.html.haml new file mode 100644 index 00000000000..d826ba25281 --- /dev/null +++ b/app/views/profiles/two_factor_auths/codes.html.haml @@ -0,0 +1,3 @@ +%h3.page-title Two-Factor Authentication Recovery codes +%hr += render 'codes' diff --git a/app/views/profiles/two_factor_auths/create.html.haml b/app/views/profiles/two_factor_auths/create.html.haml new file mode 100644 index 00000000000..27956bd7633 --- /dev/null +++ b/app/views/profiles/two_factor_auths/create.html.haml @@ -0,0 +1,4 @@ +.alert.alert-success + Congratulations! You have enabled Two-Factor Authentication! + += render 'codes' diff --git a/config/routes.rb b/config/routes.rb index bcd68ad6ae2..bf2cb6421c5 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -228,7 +228,7 @@ Gitlab::Application.routes.draw do resource :avatar, only: [:destroy] resource :two_factor_auth, only: [:new, :create, :destroy] do member do - get :codes + post :codes end end end -- cgit v1.2.1 From 35b62facac0737d1173f93a97b8f49876ec8519c Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 4 May 2015 14:09:45 -0400 Subject: Update copy for recovery codes --- app/views/profiles/two_factor_auths/_codes.html.haml | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/app/views/profiles/two_factor_auths/_codes.html.haml b/app/views/profiles/two_factor_auths/_codes.html.haml index dd043f3c5b7..1b1395eaa17 100644 --- a/app/views/profiles/two_factor_auths/_codes.html.haml +++ b/app/views/profiles/two_factor_auths/_codes.html.haml @@ -1,12 +1,11 @@ %p.slead - Please save this recovery codes so you can access your account if you lose your phone. + Should you ever lose your phone, each of these recovery codes can be used one + time each to regain access to your account. Please save them in a safe place. .codes.well %ul - @codes.each do |code| %li - %span.monospace - = code + %span.monospace= code -= link_to profile_account_path, class: 'btn btn-success' do - I saved the codes += link_to 'Proceed', profile_account_path, class: 'btn btn-success' -- cgit v1.2.1 From bd680999f9bf33ba6067a42749139c1d0b52014e Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 4 May 2015 14:11:00 -0400 Subject: Be consistent with what we call the 2FA feature "Two-factor" vs. "2-Factor" --- app/views/devise/sessions/two_factor.html.haml | 2 +- app/views/profiles/accounts/show.html.haml | 18 ++++++++++-------- app/views/profiles/two_factor_auths/codes.html.haml | 2 +- app/views/profiles/two_factor_auths/create.html.haml | 2 +- 4 files changed, 13 insertions(+), 11 deletions(-) diff --git a/app/views/devise/sessions/two_factor.html.haml b/app/views/devise/sessions/two_factor.html.haml index 3a19e926b6d..cfae81367be 100644 --- a/app/views/devise/sessions/two_factor.html.haml +++ b/app/views/devise/sessions/two_factor.html.haml @@ -1,7 +1,7 @@ %div .login-box .login-heading - %h3 Two-Factor Authentication + %h3 Two-factor Authentication .login-body = form_for(resource, as: resource_name, url: session_path(resource_name), method: :post) do |f| - if @error diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml index e8b4943833e..c8158f8f179 100644 --- a/app/views/profiles/accounts/show.html.haml +++ b/app/views/profiles/accounts/show.html.haml @@ -30,26 +30,28 @@ %fieldset - if current_user.otp_required_for_login %legend.text-success - %i.fa.fa-check - Two-Factor Authentication enabled + = icon('check') + Two-factor Authentication enabled %div .pull-right - = link_to "Disable 2-Factor Authentication", profile_two_factor_auth_path, method: :delete, class: 'btn btn-close btn-sm' + = link_to "Disable Two-factor Authentication", profile_two_factor_auth_path, method: :delete, class: 'btn btn-close btn-sm', + data: { confirm: 'Are you sure?' } %p - If you lost your recovery codes - you can + If you lose your recovery codes, you can %strong - = link_to "generate new one", codes_profile_two_factor_auth_path, method: :post, - data: { confirm: 'After we generate new recovery codes - old codes will not be valid any more. Are you sure?' } + = succeed '.' do + = link_to "generate new ones", codes_profile_two_factor_auth_path, method: :post, + data: { confirm: 'This will invalidate the old codes. Are you sure?' } - else - %legend Two-Factor Authentication + %legend Two-factor Authentication %div %p Keep your account secure by enabling two-factor authentication. %br Each time you log in, you’ll be required to provide your password plus a randomly generated access code. %div - = link_to "Enable 2-Factor Authentication", new_profile_two_factor_auth_path, class: 'btn btn-success' + = link_to "Enable Two-factor Authentication", new_profile_two_factor_auth_path, class: 'btn btn-success' - if show_profile_social_tab? %fieldset diff --git a/app/views/profiles/two_factor_auths/codes.html.haml b/app/views/profiles/two_factor_auths/codes.html.haml index d826ba25281..4950f7db122 100644 --- a/app/views/profiles/two_factor_auths/codes.html.haml +++ b/app/views/profiles/two_factor_auths/codes.html.haml @@ -1,3 +1,3 @@ -%h3.page-title Two-Factor Authentication Recovery codes +%h3.page-title Two-factor Authentication Recovery codes %hr = render 'codes' diff --git a/app/views/profiles/two_factor_auths/create.html.haml b/app/views/profiles/two_factor_auths/create.html.haml index 27956bd7633..965466db36e 100644 --- a/app/views/profiles/two_factor_auths/create.html.haml +++ b/app/views/profiles/two_factor_auths/create.html.haml @@ -1,4 +1,4 @@ .alert.alert-success - Congratulations! You have enabled Two-Factor Authentication! + Congratulations! You have enabled Two-factor Authentication! = render 'codes' -- cgit v1.2.1 From c84f1240d4dd72b53361d3cc0bccf5c7d789c8ec Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 4 May 2015 14:18:20 -0400 Subject: prepend_before_filter -> prepend_before_action --- app/controllers/sessions_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index cc9d30d64d5..e72b003f86f 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -1,5 +1,5 @@ class SessionsController < Devise::SessionsController - prepend_before_filter :two_factor_enabled?, only: :create + prepend_before_action :two_factor_enabled?, only: :create def new redirect_path = -- cgit v1.2.1 From 125ee5262a65db71fc8ba2d7a51885039b5ccc1f Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 4 May 2015 14:18:32 -0400 Subject: Don't use hard-coded sign_in path --- app/controllers/sessions_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index e72b003f86f..8bd8fbb692f 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -16,7 +16,7 @@ class SessionsController < Devise::SessionsController # Prevent a 'you are already signed in' message directly after signing: # we should never redirect to '/users/sign_in' after signing in successfully. - unless redirect_path == '/users/sign_in' + unless redirect_path == new_user_session_path store_location_for(:redirect, redirect_path) end -- cgit v1.2.1 From e512f770cd22cf82933118fb30625526dfb68c62 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 4 May 2015 16:02:33 -0400 Subject: Fix Devise parameter sanitizer for otp_attempt --- app/controllers/application_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 247f5cc32d3..8ce881c7414 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -252,7 +252,7 @@ class ApplicationController < ActionController::Base end def configure_permitted_parameters - devise_parameter_sanitizer.sanitize(:sign_in) { |u| u.permit(:username, :email, :password, :login, :remember_me, :otp_attempt) } + devise_parameter_sanitizer.for(:sign_in) { |u| u.permit(:username, :email, :password, :login, :remember_me, :otp_attempt) } end def hexdigest(string) -- cgit v1.2.1 From 6fa294292058dd5ab9bd089c446b3017aa85b868 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 5 May 2015 22:12:35 -0400 Subject: Update login views for two-factor auth --- app/views/devise/sessions/_new_base.html.haml | 1 - app/views/devise/sessions/two_factor.html.haml | 7 ++----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/views/devise/sessions/_new_base.html.haml b/app/views/devise/sessions/_new_base.html.haml index dcb989e8d81..54a39726771 100644 --- a/app/views/devise/sessions/_new_base.html.haml +++ b/app/views/devise/sessions/_new_base.html.haml @@ -1,7 +1,6 @@ = form_for(resource, as: resource_name, url: session_path(resource_name)) do |f| = f.text_field :login, class: "form-control top", placeholder: "Username or Email", autofocus: "autofocus" = f.password_field :password, class: "form-control bottom", placeholder: "Password" - = f.hidden_field :otp_attempt, value: '' - if devise_mapping.rememberable? .remember-me.checkbox %label{for: "user_remember_me"} diff --git a/app/views/devise/sessions/two_factor.html.haml b/app/views/devise/sessions/two_factor.html.haml index cfae81367be..32ba90fd7ce 100644 --- a/app/views/devise/sessions/two_factor.html.haml +++ b/app/views/devise/sessions/two_factor.html.haml @@ -7,10 +7,7 @@ - if @error .alert.alert-danger = @error - .hide - = f.text_field :login, class: "form-control top", placeholder: "Username or Email", autofocus: "autofocus" - = f.password_field :password, class: "form-control bottom", placeholder: "Password" - = f.text_field :otp_attempt, class: 'form-control', - placeholder: 'Two-factor authentication token', required: true, autofocus: true + = f.hidden_field :login + = f.text_field :otp_attempt, class: 'form-control', placeholder: 'Two-factor authentication code', required: true, autofocus: true .prepend-top-20 = f.submit "Verify code", class: "btn btn-save" -- cgit v1.2.1 From 66bc758e2b9ba736035d036b25b17bdbb83c39b3 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 5 May 2015 22:16:09 -0400 Subject: Update User model for two-factor auth --- app/models/user.rb | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index befbcbf1a16..7b1f6fae3be 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -73,8 +73,11 @@ class User < ActiveRecord::Base default_value_for :hide_no_password, false default_value_for :theme_id, gitlab_config.default_theme - devise :lockable, :async, - :recoverable, :rememberable, :trackable, :validatable, :omniauthable, :confirmable, :registerable + devise :two_factor_authenticatable, + otp_secret_encryption_key: File.read(Rails.root.join('.secret')).chomp + devise :two_factor_backupable + devise :lockable, :async, :recoverable, :rememberable, :trackable, + :validatable, :omniauthable, :confirmable, :registerable attr_accessor :force_random_password @@ -663,4 +666,9 @@ class User < ActiveRecord::Base true end + + # Used to populate the hidden form field during Two-factor authentication + def login + username || email + end end -- cgit v1.2.1 From 5520397f048243ad7fd8a39faf9d9f7b5a97c5bc Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 5 May 2015 22:16:45 -0400 Subject: Make two-factor login work and add a feature spec --- app/controllers/sessions_controller.rb | 31 ++++++++++--- spec/features/login_spec.rb | 82 ++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 7 deletions(-) create mode 100644 spec/features/login_spec.rb diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 8bd8fbb692f..21505442e35 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -1,5 +1,5 @@ class SessionsController < Devise::SessionsController - prepend_before_action :two_factor_enabled?, only: :create + prepend_before_action :authenticate_with_two_factor, only: :create def new redirect_path = @@ -29,6 +29,9 @@ class SessionsController < Devise::SessionsController def create super do |resource| + # Remove any lingering user data from login + session.delete(:user) + # User has successfully signed in, so clear any unused reset tokens if resource.reset_password_token.present? resource.update_attributes(reset_password_token: nil, @@ -39,24 +42,38 @@ class SessionsController < Devise::SessionsController private - def two_factor_enabled? - user_params = params[:user] + def user_params + params.require(:user).permit(:login, :password, :remember_me, :otp_attempt) + end + + def authenticate_with_two_factor @user = User.by_login(user_params[:login]) - if user_params[:otp_attempt].present? - unless @user.valid_otp?(user_params[:otp_attempt]) || - @user.recovery_code?(user_params[:otp_attempt]) + if user_params[:otp_attempt].present? && session[:user] + if valid_otp_attempt? + # Insert the saved params from the session into the request parameters + # so they're available to Devise::Strategies::DatabaseAuthenticatable + request.params[:user].merge!(session[:user]) + else @error = 'Invalid two-factor code' render :two_factor and return end else - if @user && @user.valid_password?(params[:user][:password]) + if @user && @user.valid_password?(user_params[:password]) self.resource = @user if resource.otp_required_for_login + # Login is valid, save the values to the session so we can prompt the + # user for a one-time password. + session[:user] = user_params render :two_factor and return end end end end + + def valid_otp_attempt? + @user.valid_otp?(user_params[:otp_attempt]) || + @user.invalidate_otp_backup_code!(user_params[:otp_attempt]) + end end diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb new file mode 100644 index 00000000000..5ffaae54766 --- /dev/null +++ b/spec/features/login_spec.rb @@ -0,0 +1,82 @@ +require 'spec_helper' + +feature 'Login' do + let(:user) { create(:user) } + + context 'with two-factor authentication' do + before do + user.otp_required_for_login = true + user.otp_secret = User.generate_otp_secret + user.save! + end + + context 'with valid username/password' do + before do + login_with(user) + expect(page).to have_content('Two-factor Authentication') + end + + def enter_code(code) + fill_in 'Two-factor authentication code', with: code + click_button 'Verify code' + end + + context 'using one-time code' do + it 'allows login with valid code' do + enter_code(user.current_otp) + expect(current_path).to eq root_path + end + + it 'blocks login with invalid code' do + enter_code('foo') + expect(page).to have_content('Invalid two-factor code') + end + end + + context 'using backup code' do + let(:codes) { user.generate_otp_backup_codes! } + + before do + expect(codes.size).to eq 5 + + # Because `generate_otp_backup_codes!` doesn't actually do this... + user.save + end + + context 'with valid code' do + it 'allows login' do + enter_code(codes.sample) + expect(current_path).to eq root_path + end + + it 'invalidates the used code' do + # FIXME (rspeicher): Broken library is broken + expect { enter_code(codes.sample) }.to change { user.otp_backup_codes.size }.by(-1) + end + end + + context 'with invalid code' do + it 'blocks login' do + # FIXME (rspeicher): Broken library is broken + code = codes.sample + expect(user.invalidate_otp_backup_code!(code)).to eq true + expect(user.otp_backup_codes.size).to eq 4 # Passes + user.save! + user.reload + expect(user.otp_backup_codes.size).to eq 4 # Fails... WAT?! + + enter_code(code) + expect(page).to have_content('Invalid two-factor code') + end + end + end + end + end + + context 'without two-factor authentication' do + it 'allows basic login' do + login_with(user) + expect(current_path).to eq root_path + end + end +end -- cgit v1.2.1 From c891ef93371c3eeeb56b246d8d02a9e2dd5d350e Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 6 May 2015 13:35:37 -0400 Subject: Add page titles for two_factor_auths views --- app/views/profiles/two_factor_auths/codes.html.haml | 2 ++ app/views/profiles/two_factor_auths/create.html.haml | 2 ++ app/views/profiles/two_factor_auths/new.html.haml | 2 ++ 3 files changed, 6 insertions(+) diff --git a/app/views/profiles/two_factor_auths/codes.html.haml b/app/views/profiles/two_factor_auths/codes.html.haml index 4950f7db122..addf356697a 100644 --- a/app/views/profiles/two_factor_auths/codes.html.haml +++ b/app/views/profiles/two_factor_auths/codes.html.haml @@ -1,3 +1,5 @@ +- page_title 'Recovery Codes', 'Two-factor Authentication' + %h3.page-title Two-factor Authentication Recovery codes %hr = render 'codes' diff --git a/app/views/profiles/two_factor_auths/create.html.haml b/app/views/profiles/two_factor_auths/create.html.haml index 965466db36e..e330aadac13 100644 --- a/app/views/profiles/two_factor_auths/create.html.haml +++ b/app/views/profiles/two_factor_auths/create.html.haml @@ -1,3 +1,5 @@ +- page_title 'Two-factor Authentication', 'Account' + .alert.alert-success Congratulations! You have enabled Two-factor Authentication! diff --git a/app/views/profiles/two_factor_auths/new.html.haml b/app/views/profiles/two_factor_auths/new.html.haml index 8332fc6b8b8..5d62aa6339b 100644 --- a/app/views/profiles/two_factor_auths/new.html.haml +++ b/app/views/profiles/two_factor_auths/new.html.haml @@ -1,3 +1,5 @@ +- page_title 'Two-factor Authentication', 'Account' + %h2.page-title Two-Factor Authentication (TFA) %p Download the Google Authenticator application from App Store for iOS or -- cgit v1.2.1 From 6369d23d581ad36e7507d355a69237b90a912697 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 6 May 2015 13:35:55 -0400 Subject: Fix nav and layout for TwoFactorAuthsController --- app/controllers/profiles/two_factor_auths_controller.rb | 2 +- app/views/layouts/nav/_profile.html.haml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb index 5c51706c3d4..60f8ec5cf30 100644 --- a/app/controllers/profiles/two_factor_auths_controller.rb +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -1,4 +1,4 @@ -class Profiles::TwoFactorAuthsController < ApplicationController +class Profiles::TwoFactorAuthsController < Profiles::ApplicationController def new unless current_user.otp_secret current_user.otp_secret = User.generate_otp_secret diff --git a/app/views/layouts/nav/_profile.html.haml b/app/views/layouts/nav/_profile.html.haml index 31d8ed3ed86..ac37fd4c1c1 100644 --- a/app/views/layouts/nav/_profile.html.haml +++ b/app/views/layouts/nav/_profile.html.haml @@ -4,7 +4,7 @@ = icon('user fw') %span Profile - = nav_link(controller: :accounts) do + = nav_link(controller: [:accounts, :two_factor_auths]) do = link_to profile_account_path, title: 'Account', data: {placement: 'right'} do = icon('gear fw') %span -- cgit v1.2.1 From 32971b0af4761fa9527c2fa0922a9b31eec5245f Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 8 May 2015 20:41:53 -0400 Subject: Refactor SessionsController Also adds test case for providing an invalid 2FA code and then a valid one without re-entering username and password. --- app/controllers/sessions_controller.rb | 47 ++++++++++++++------------ app/views/devise/sessions/two_factor.html.haml | 1 - spec/features/login_spec.rb | 8 +++++ 3 files changed, 33 insertions(+), 23 deletions(-) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 21505442e35..e2a5c612579 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -29,10 +29,7 @@ class SessionsController < Devise::SessionsController def create super do |resource| - # Remove any lingering user data from login - session.delete(:user) - - # User has successfully signed in, so clear any unused reset tokens + # User has successfully signed in, so clear any unused reset token if resource.reset_password_token.present? resource.update_attributes(reset_password_token: nil, reset_password_sent_at: nil) @@ -46,34 +43,40 @@ class SessionsController < Devise::SessionsController params.require(:user).permit(:login, :password, :remember_me, :otp_attempt) end + def find_user + if user_params[:login] + User.by_login(user_params[:login]) + elsif user_params[:otp_attempt] && session[:otp_user_id] + User.find(session[:otp_user_id]) + end + end + def authenticate_with_two_factor - @user = User.by_login(user_params[:login]) + user = self.resource = find_user + + return unless user && user.otp_required_for_login - if user_params[:otp_attempt].present? && session[:user] - if valid_otp_attempt? - # Insert the saved params from the session into the request parameters - # so they're available to Devise::Strategies::DatabaseAuthenticatable - request.params[:user].merge!(session[:user]) + if user_params[:otp_attempt].present? && session[:otp_user_id] + if valid_otp_attempt?(user) + # Remove any lingering user data from login + session.delete(:otp_user_id) + + sign_in(user) else @error = 'Invalid two-factor code' render :two_factor and return end else - if @user && @user.valid_password?(user_params[:password]) - self.resource = @user - - if resource.otp_required_for_login - # Login is valid, save the values to the session so we can prompt the - # user for a one-time password. - session[:user] = user_params - render :two_factor and return - end + if user && user.valid_password?(user_params[:password]) + # Save the user's ID to session so we can ask for a one-time password + session[:otp_user_id] = user.id + render :two_factor and return end end end - def valid_otp_attempt? - @user.valid_otp?(user_params[:otp_attempt]) || - @user.invalidate_otp_backup_code!(user_params[:otp_attempt]) + def valid_otp_attempt?(user) + user.valid_otp?(user_params[:otp_attempt]) || + user.invalidate_otp_backup_code!(user_params[:otp_attempt]) end end diff --git a/app/views/devise/sessions/two_factor.html.haml b/app/views/devise/sessions/two_factor.html.haml index 32ba90fd7ce..779928631ac 100644 --- a/app/views/devise/sessions/two_factor.html.haml +++ b/app/views/devise/sessions/two_factor.html.haml @@ -7,7 +7,6 @@ - if @error .alert.alert-danger = @error - = f.hidden_field :login = f.text_field :otp_attempt, class: 'form-control', placeholder: 'Two-factor authentication code', required: true, autofocus: true .prepend-top-20 = f.submit "Verify code", class: "btn btn-save" diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb index 5ffaae54766..f1e24c54240 100644 --- a/spec/features/login_spec.rb +++ b/spec/features/login_spec.rb @@ -31,6 +31,14 @@ feature 'Login' do enter_code('foo') expect(page).to have_content('Invalid two-factor code') end + + it 'allows login with invalid code, then valid code' do + enter_code('foo') + expect(page).to have_content('Invalid two-factor code') + + enter_code(user.current_otp) + expect(current_path).to eq root_path + end end context 'using backup code' do -- cgit v1.2.1 From 4fca1fc5ab7a1fc8719483c78beed08f71144ea2 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 8 May 2015 20:45:50 -0400 Subject: Update copy for generating new recovery codes --- app/views/profiles/accounts/show.html.haml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml index c8158f8f179..9f9a12c7913 100644 --- a/app/views/profiles/accounts/show.html.haml +++ b/app/views/profiles/accounts/show.html.haml @@ -37,11 +37,11 @@ = link_to "Disable Two-factor Authentication", profile_two_factor_auth_path, method: :delete, class: 'btn btn-close btn-sm', data: { confirm: 'Are you sure?' } %p - If you lose your recovery codes, you can + If you lose your recovery codes you can %strong - = succeed '.' do - = link_to "generate new ones", codes_profile_two_factor_auth_path, method: :post, - data: { confirm: 'This will invalidate the old codes. Are you sure?' } + = succeed ',' do + = link_to "generate new ones", codes_profile_two_factor_auth_path, method: :post, data: { confirm: 'Are you sure?' } + invalidating all previous codes. - else %legend Two-factor Authentication -- cgit v1.2.1 From 8eb577ae9852d5304c28a0bc01d462b146427050 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 8 May 2015 20:55:45 -0400 Subject: Improve copy for enabling 2FA --- app/views/profiles/accounts/show.html.haml | 7 ++++--- app/views/profiles/two_factor_auths/new.html.haml | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml index 9f9a12c7913..87f827793a6 100644 --- a/app/views/profiles/accounts/show.html.haml +++ b/app/views/profiles/accounts/show.html.haml @@ -47,9 +47,10 @@ %legend Two-factor Authentication %div %p - Keep your account secure by enabling two-factor authentication. - %br - Each time you log in, you’ll be required to provide your password plus a randomly generated access code. + Increase your account's security by enabling two-factor authentication (2FA). + %p + Each time you log in you’ll be required to provide your username and + password as usual, plus a randomly-generated code from your phone. %div = link_to "Enable Two-factor Authentication", new_profile_two_factor_auth_path, class: 'btn btn-success' diff --git a/app/views/profiles/two_factor_auths/new.html.haml b/app/views/profiles/two_factor_auths/new.html.haml index 5d62aa6339b..f0a4a341878 100644 --- a/app/views/profiles/two_factor_auths/new.html.haml +++ b/app/views/profiles/two_factor_auths/new.html.haml @@ -1,6 +1,6 @@ - page_title 'Two-factor Authentication', 'Account' -%h2.page-title Two-Factor Authentication (TFA) +%h2.page-title Two-Factor Authentication (2FA) %p Download the Google Authenticator application from App Store for iOS or Google Play for Android and scan this code. -- cgit v1.2.1 From a4267033f2cc72e46ba5f1bfdfca296978df2117 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 8 May 2015 20:56:04 -0400 Subject: Add a hint on login form about using a recovery code --- app/views/devise/sessions/two_factor.html.haml | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/devise/sessions/two_factor.html.haml b/app/views/devise/sessions/two_factor.html.haml index 779928631ac..30ddd0bd3c6 100644 --- a/app/views/devise/sessions/two_factor.html.haml +++ b/app/views/devise/sessions/two_factor.html.haml @@ -8,5 +8,6 @@ .alert.alert-danger = @error = f.text_field :otp_attempt, class: 'form-control', placeholder: 'Two-factor authentication code', required: true, autofocus: true + %p.help-block.hint If you've lost your phone, you may enter one of your recovery codes. .prepend-top-20 = f.submit "Verify code", class: "btn btn-save" -- cgit v1.2.1 From 661d09a22b8a9a7fdead09fdb27284ff30cb0308 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 8 May 2015 20:59:09 -0400 Subject: Re-annotate User model --- app/models/user.rb | 5 +++++ spec/models/user_spec.rb | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/app/models/user.rb b/app/models/user.rb index 7b1f6fae3be..9dbc9e1cf25 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -50,6 +50,11 @@ # bitbucket_access_token :string(255) # bitbucket_access_token_secret :string(255) # location :string(255) +# encrypted_otp_secret :string(255) +# encrypted_otp_secret_iv :string(255) +# encrypted_otp_secret_salt :string(255) +# otp_required_for_login :boolean +# otp_backup_codes :text # public_email :string(255) default(""), not null # diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 771709c127a..0dddcd5bda2 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -50,6 +50,11 @@ # bitbucket_access_token :string(255) # bitbucket_access_token_secret :string(255) # location :string(255) +# encrypted_otp_secret :string(255) +# encrypted_otp_secret_iv :string(255) +# encrypted_otp_secret_salt :string(255) +# otp_required_for_login :boolean +# otp_backup_codes :text # public_email :string(255) default(""), not null # -- cgit v1.2.1 From 2ad1334d9e0e143eb1cd1694433bfdf29314a65e Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 9 May 2015 14:23:34 -0400 Subject: Quotes, icon helper in profiles/accounts/show view --- app/views/profiles/accounts/show.html.haml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml index 87f827793a6..6ac60b01f85 100644 --- a/app/views/profiles/accounts/show.html.haml +++ b/app/views/profiles/accounts/show.html.haml @@ -34,13 +34,13 @@ Two-factor Authentication enabled %div .pull-right - = link_to "Disable Two-factor Authentication", profile_two_factor_auth_path, method: :delete, class: 'btn btn-close btn-sm', + = link_to 'Disable Two-factor Authentication', profile_two_factor_auth_path, method: :delete, class: 'btn btn-close btn-sm', data: { confirm: 'Are you sure?' } %p If you lose your recovery codes you can %strong = succeed ',' do - = link_to "generate new ones", codes_profile_two_factor_auth_path, method: :post, data: { confirm: 'Are you sure?' } + = link_to 'generate new ones', codes_profile_two_factor_auth_path, method: :post, data: { confirm: 'Are you sure?' } invalidating all previous codes. - else @@ -52,7 +52,7 @@ Each time you log in you’ll be required to provide your username and password as usual, plus a randomly-generated code from your phone. %div - = link_to "Enable Two-factor Authentication", new_profile_two_factor_auth_path, class: 'btn btn-success' + = link_to 'Enable Two-factor Authentication', new_profile_two_factor_auth_path, class: 'btn btn-success' - if show_profile_social_tab? %fieldset @@ -65,7 +65,7 @@ class: "btn btn-lg #{'active' if oauth_active?(provider)}" - if oauth_active?(provider) = link_to unlink_profile_account_path(provider: provider), method: :delete, class: 'btn btn-lg' do - %i.fa.fa-close + = icon('close') - if show_profile_username_tab? %fieldset.update-username @@ -79,7 +79,7 @@   .loading-gif.hide %p - %i.fa.fa-spinner.fa-spin + = icon('spinner spin') Saving new username %p.light = user_url(@user) -- cgit v1.2.1 From 0c113c8dcb02e02457473823847b41df4eeedb88 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 9 May 2015 14:52:46 -0400 Subject: Make otp_backup_codes a text field --- .../20150331183602_add_devise_two_factor_backupable_to_users.rb | 2 +- db/schema.rb | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/db/migrate/20150331183602_add_devise_two_factor_backupable_to_users.rb b/db/migrate/20150331183602_add_devise_two_factor_backupable_to_users.rb index 2feb49f43f1..913958db7c5 100644 --- a/db/migrate/20150331183602_add_devise_two_factor_backupable_to_users.rb +++ b/db/migrate/20150331183602_add_devise_two_factor_backupable_to_users.rb @@ -1,5 +1,5 @@ class AddDeviseTwoFactorBackupableToUsers < ActiveRecord::Migration def change - add_column :users, :otp_backup_codes, :string, array: true + add_column :users, :otp_backup_codes, :text end end diff --git a/db/schema.rb b/db/schema.rb index 04abf9bb9a6..3e5810d7408 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -492,6 +492,11 @@ ActiveRecord::Schema.define(version: 20150502064022) do t.string "bitbucket_access_token" t.string "bitbucket_access_token_secret" t.string "location" + t.string "encrypted_otp_secret" + t.string "encrypted_otp_secret_iv" + t.string "encrypted_otp_secret_salt" + t.boolean "otp_required_for_login" + t.text "otp_backup_codes" t.string "public_email", default: "", null: false end -- cgit v1.2.1 From b050bb5bada27f38c6308ca33ac081f56cc681a5 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 9 May 2015 15:45:22 -0400 Subject: Fix 2FA backup code removal --- app/models/user.rb | 3 +++ spec/features/login_spec.rb | 12 +++++------- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 9dbc9e1cf25..0969fa93088 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -80,7 +80,10 @@ class User < ActiveRecord::Base devise :two_factor_authenticatable, otp_secret_encryption_key: File.read(Rails.root.join('.secret')).chomp + devise :two_factor_backupable + serialize :otp_backup_codes, JSON + devise :lockable, :async, :recoverable, :rememberable, :trackable, :validatable, :omniauthable, :confirmable, :registerable diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb index f1e24c54240..ca7fb022a2a 100644 --- a/spec/features/login_spec.rb +++ b/spec/features/login_spec.rb @@ -47,7 +47,7 @@ feature 'Login' do before do expect(codes.size).to eq 5 - # Because `generate_otp_backup_codes!` doesn't actually do this... + # Ensure the generated codes get saved user.save end @@ -58,20 +58,18 @@ feature 'Login' do end it 'invalidates the used code' do - # FIXME (rspeicher): Broken library is broken - expect { enter_code(codes.sample) }.to change { user.otp_backup_codes.size }.by(-1) + expect { enter_code(codes.sample) }. + to change { user.reload.otp_backup_codes.size }.by(-1) end end context 'with invalid code' do it 'blocks login' do - # FIXME (rspeicher): Broken library is broken code = codes.sample expect(user.invalidate_otp_backup_code!(code)).to eq true - expect(user.otp_backup_codes.size).to eq 4 # Passes + user.save! - user.reload - expect(user.otp_backup_codes.size).to eq 4 # Fails... WAT?! + expect(user.reload.otp_backup_codes.size).to eq 4 enter_code(code) expect(page).to have_content('Invalid two-factor code') -- cgit v1.2.1 From 5f43cae6ca2c9ebd3f6561a7b40c64c65913e064 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 9 May 2015 15:46:18 -0400 Subject: Add :two_factor trait to User factory --- spec/factories.rb | 7 +++++++ spec/features/login_spec.rb | 12 ++++-------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/spec/factories.rb b/spec/factories.rb index 19f2935f30e..26e8a795fa4 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -28,6 +28,13 @@ FactoryGirl.define do admin true end + trait :two_factor do + before(:create) do |user| + user.otp_required_for_login = true + user.otp_secret = User.generate_otp_secret + end + end + factory :omniauth_user do ignore do extern_uid '123456' diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb index ca7fb022a2a..e44ddc17993 100644 --- a/spec/features/login_spec.rb +++ b/spec/features/login_spec.rb @@ -1,16 +1,10 @@ require 'spec_helper' feature 'Login' do - let(:user) { create(:user) } - context 'with two-factor authentication' do - before do - user.otp_required_for_login = true - user.otp_secret = User.generate_otp_secret - user.save! - end - context 'with valid username/password' do + let(:user) { create(:user, :two_factor) } + before do login_with(user) expect(page).to have_content('Two-factor Authentication') @@ -80,6 +74,8 @@ feature 'Login' do end context 'without two-factor authentication' do + let(:user) { create(:user) } + it 'allows basic login' do login_with(user) expect(current_path).to eq root_path -- cgit v1.2.1 From c845347b233b9bb40d9b304d864ac33e178429c1 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 9 May 2015 15:46:49 -0400 Subject: Generate 10 2FA backup codes instead of the default of 5 --- app/models/user.rb | 2 +- spec/features/login_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 0969fa93088..70972eb2715 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -81,7 +81,7 @@ class User < ActiveRecord::Base devise :two_factor_authenticatable, otp_secret_encryption_key: File.read(Rails.root.join('.secret')).chomp - devise :two_factor_backupable + devise :two_factor_backupable, otp_number_of_backup_codes: 10 serialize :otp_backup_codes, JSON devise :lockable, :async, :recoverable, :rememberable, :trackable, diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb index e44ddc17993..61066e7e923 100644 --- a/spec/features/login_spec.rb +++ b/spec/features/login_spec.rb @@ -39,7 +39,7 @@ feature 'Login' do let(:codes) { user.generate_otp_backup_codes! } before do - expect(codes.size).to eq 5 + expect(codes.size).to eq 10 # Ensure the generated codes get saved user.save @@ -63,7 +63,7 @@ feature 'Login' do expect(user.invalidate_otp_backup_code!(code)).to eq true user.save! - expect(user.reload.otp_backup_codes.size).to eq 4 + expect(user.reload.otp_backup_codes.size).to eq 9 enter_code(code) expect(page).to have_content('Invalid two-factor code') -- cgit v1.2.1 From c40f59d04f6c01b543f793f0884b26732fdcc0f0 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 9 May 2015 16:52:46 -0400 Subject: Add otp_attempt to filtered parameters --- config/application.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/application.rb b/config/application.rb index fa399533e52..7e899cc3b5b 100644 --- a/config/application.rb +++ b/config/application.rb @@ -31,7 +31,7 @@ module Gitlab config.encoding = "utf-8" # Configure sensitive parameters which will be filtered from the log file. - config.filter_parameters.push(:password, :password_confirmation, :private_token) + config.filter_parameters.push(:password, :password_confirmation, :private_token, :otp_attempt) # Enable escaping HTML in JSON. config.active_support.escape_html_entities_in_json = true -- cgit v1.2.1 From 3fb0fedd0415732b3c1bf0aaa66abf1197012d75 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 9 May 2015 17:00:37 -0400 Subject: Autofocus the pin field on 2FA enable form --- app/views/profiles/two_factor_auths/new.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/profiles/two_factor_auths/new.html.haml b/app/views/profiles/two_factor_auths/new.html.haml index f0a4a341878..fe03a259a12 100644 --- a/app/views/profiles/two_factor_auths/new.html.haml +++ b/app/views/profiles/two_factor_auths/new.html.haml @@ -18,6 +18,6 @@ .form-group = label_tag :pin_code, nil, class: "control-label" .col-sm-10 - = text_field_tag :pin_code, nil, class: "form-control", required: true + = text_field_tag :pin_code, nil, class: "form-control", required: true, autofocus: true .form-actions = submit_tag 'Submit', class: 'btn btn-success' -- cgit v1.2.1 From 76873ce4a49be9a591082f7b96482ebdc9cace9d Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 9 May 2015 17:02:06 -0400 Subject: Move "invalid 2FA code" error message to the flash This makes it consistent with the Invalid email/password error message from the previous step. --- app/controllers/sessions_controller.rb | 2 +- app/views/devise/sessions/two_factor.html.haml | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index e2a5c612579..b9757143119 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -63,7 +63,7 @@ class SessionsController < Devise::SessionsController sign_in(user) else - @error = 'Invalid two-factor code' + flash.now[:alert] = 'Invalid two-factor code.' render :two_factor and return end else diff --git a/app/views/devise/sessions/two_factor.html.haml b/app/views/devise/sessions/two_factor.html.haml index 30ddd0bd3c6..22b2c1a186b 100644 --- a/app/views/devise/sessions/two_factor.html.haml +++ b/app/views/devise/sessions/two_factor.html.haml @@ -4,9 +4,6 @@ %h3 Two-factor Authentication .login-body = form_for(resource, as: resource_name, url: session_path(resource_name), method: :post) do |f| - - if @error - .alert.alert-danger - = @error = f.text_field :otp_attempt, class: 'form-control', placeholder: 'Two-factor authentication code', required: true, autofocus: true %p.help-block.hint If you've lost your phone, you may enter one of your recovery codes. .prepend-top-20 -- cgit v1.2.1 From 5cd526f77fa51347ec66ab094b778ca4b83b8fce Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 9 May 2015 17:04:02 -0400 Subject: Prevent "You are already signed in." error message upon 2FA login --- app/controllers/sessions_controller.rb | 10 ++++++++-- spec/features/login_spec.rb | 19 ++++++++++++++++++- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index b9757143119..d4ff0d97561 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -1,5 +1,11 @@ class SessionsController < Devise::SessionsController - prepend_before_action :authenticate_with_two_factor, only: :create + prepend_before_action :authenticate_with_two_factor, only: [:create] + + # This action comes from DeviseController, but because we call `sign_in` + # manually inside `authenticate_with_two_factor`, not skipping this action + # would cause a "You are already signed in." error message to be shown upon + # successful login. + skip_before_action :require_no_authentication, only: [:create] def new redirect_path = @@ -61,7 +67,7 @@ class SessionsController < Devise::SessionsController # Remove any lingering user data from login session.delete(:otp_user_id) - sign_in(user) + sign_in(user) and return else flash.now[:alert] = 'Invalid two-factor code.' render :two_factor and return diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb index 61066e7e923..61defb8a333 100644 --- a/spec/features/login_spec.rb +++ b/spec/features/login_spec.rb @@ -15,6 +15,11 @@ feature 'Login' do click_button 'Verify code' end + it 'does not show a "You are already signed in." error message' do + enter_code(user.current_otp) + expect(page).not_to have_content('You are already signed in.') + end + context 'using one-time code' do it 'allows login with valid code' do enter_code(user.current_otp) @@ -66,7 +71,7 @@ feature 'Login' do expect(user.reload.otp_backup_codes.size).to eq 9 enter_code(code) - expect(page).to have_content('Invalid two-factor code') + expect(page).to have_content('Invalid two-factor code.') end end end @@ -80,5 +85,17 @@ feature 'Login' do login_with(user) expect(current_path).to eq root_path end + + it 'does not show a "You are already signed in." error message' do + login_with(user) + expect(page).not_to have_content('You are already signed in.') + end + + it 'blocks invalid login' do + user = create(:user, password: 'not-the-default') + + login_with(user) + expect(page).to have_content('Invalid email or password.') + end end end -- cgit v1.2.1 From 414ddc0021dfe2c8b594b240b750a700c3af2b14 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 9 May 2015 17:04:32 -0400 Subject: Clear all 2FA-related fields when user disables the feature --- .../profiles/two_factor_auths_controller.rb | 9 +- .../profiles/two_factor_auths_controller_spec.rb | 126 +++++++++++++++++++++ 2 files changed, 133 insertions(+), 2 deletions(-) create mode 100644 spec/controllers/profiles/two_factor_auths_controller_spec.rb diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb index 60f8ec5cf30..30ee6891733 100644 --- a/app/controllers/profiles/two_factor_auths_controller.rb +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -28,8 +28,13 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController end def destroy - current_user.otp_required_for_login = false - current_user.save! + current_user.update_attributes({ + otp_required_for_login: false, + encrypted_otp_secret: nil, + encrypted_otp_secret_iv: nil, + encrypted_otp_secret_salt: nil, + otp_backup_codes: nil + }) redirect_to profile_account_path end diff --git a/spec/controllers/profiles/two_factor_auths_controller_spec.rb b/spec/controllers/profiles/two_factor_auths_controller_spec.rb new file mode 100644 index 00000000000..f05d1f5fbe1 --- /dev/null +++ b/spec/controllers/profiles/two_factor_auths_controller_spec.rb @@ -0,0 +1,126 @@ +require 'spec_helper' + +describe Profiles::TwoFactorAuthsController do + before do + # `user` should be defined within the action-specific describe blocks + sign_in(user) + + allow(subject).to receive(:current_user).and_return(user) + end + + describe 'GET new' do + let(:user) { create(:user) } + + it 'generates otp_secret' do + expect { get :new }.to change { user.otp_secret } + end + + it 'assigns qr_code' do + code = double('qr code') + expect(subject).to receive(:build_qr_code).and_return(code) + + get :new + expect(assigns[:qr_code]).to eq code + end + end + + describe 'POST create' do + let(:user) { create(:user) } + let(:pin) { 'pin-code' } + + def go + post :create, pin_code: pin + end + + context 'with valid pin' do + before do + expect(user).to receive(:valid_otp?).with(pin).and_return(true) + end + + it 'sets otp_required_for_login' do + go + + user.reload + expect(user.otp_required_for_login).to eq true + end + + it 'presents plaintext codes for the user to save' do + expect(user).to receive(:generate_otp_backup_codes!).and_return(%w(a b c)) + + go + + expect(assigns[:codes]).to match_array %w(a b c) + end + + it 'renders create' do + go + expect(response).to render_template(:create) + end + end + + context 'with invalid pin' do + before do + expect(user).to receive(:valid_otp?).with(pin).and_return(false) + end + + it 'assigns error' do + go + expect(assigns[:error]).to eq 'Invalid pin code' + end + + it 'assigns qr_code' do + code = double('qr code') + expect(subject).to receive(:build_qr_code).and_return(code) + + go + expect(assigns[:qr_code]).to eq code + end + + it 'renders new' do + go + expect(response).to render_template(:new) + end + end + end + + describe 'POST codes' do + let(:user) { create(:user, :two_factor) } + + it 'presents plaintext codes for the user to save' do + expect(user).to receive(:generate_otp_backup_codes!).and_return(%w(a b c)) + + post :codes + expect(assigns[:codes]).to match_array %w(a b c) + end + + it 'persists the generated codes' do + post :codes + + user.reload + expect(user.otp_backup_codes).not_to be_empty + end + end + + describe 'DELETE destroy' do + let(:user) { create(:user, :two_factor) } + let!(:codes) { user.generate_otp_backup_codes! } + + it 'clears all 2FA-related fields' do + expect(user.otp_required_for_login).to eq true + expect(user.otp_backup_codes).not_to be_nil + expect(user.encrypted_otp_secret).not_to be_nil + + delete :destroy + + expect(user.otp_required_for_login).to eq false + expect(user.otp_backup_codes).to be_nil + expect(user.encrypted_otp_secret).to be_nil + end + + it 'redirects to profile_account_path' do + delete :destroy + + expect(response).to redirect_to(profile_account_path) + end + end +end -- cgit v1.2.1 From 21be1b415262773e099176dbb3da9846bc36a0ca Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 9 May 2015 17:36:19 -0400 Subject: Add CHANGELOG entry --- CHANGELOG | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 3206c625cd0..b48ef6ea50d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -24,7 +24,7 @@ v 7.11.0 (unreleased) - When use change branches link at MR form - save source branch selection instead of target one - Improve handling of large diffs - Added GitLab Event header for project hooks - - + - Add Two-factor authentication (2FA) for GitLab logins - Show Atom feed buttons everywhere where applicable. - Add project activity atom feed. - Don't crash when an MR from a fork has a cross-reference comment from the target project on one of its commits. -- cgit v1.2.1 From 11989d62d09a628015c68c161898dfabcd966825 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 9 May 2015 17:39:16 -0400 Subject: Remove unnecessary User#login accessor override --- app/models/user.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 70972eb2715..b45c7f20e52 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -674,9 +674,4 @@ class User < ActiveRecord::Base true end - - # Used to populate the hidden form field during Two-factor authentication - def login - username || email - end end -- cgit v1.2.1 From 19b897e998d4b376390a3e0c12ccac4d1e92597d Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sun, 10 May 2015 19:13:47 -0400 Subject: Remove extra `devise` call that got added by accident --- app/models/user.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index b45c7f20e52..aeab503297c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -62,9 +62,6 @@ require 'carrierwave/orm/activerecord' require 'file_size_validator' class User < ActiveRecord::Base - devise :two_factor_authenticatable, - otp_secret_encryption_key: File.read(Rails.root.join('.secret')).chomp - include Sortable include Gitlab::ConfigHelper include TokenAuthenticatable -- cgit v1.2.1 From 24bef5e67a81c5edf9dacb65ecc091cac1f4c528 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 11 May 2015 14:31:31 -0400 Subject: Handle password reset for users with 2FA enabled --- app/controllers/passwords_controller.rb | 21 +++++++++++++ spec/features/login_spec.rb | 4 +-- spec/features/password_reset_spec.rb | 53 +++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 spec/features/password_reset_spec.rb diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index dcbbe5baa4b..88459d4080a 100644 --- a/app/controllers/passwords_controller.rb +++ b/app/controllers/passwords_controller.rb @@ -15,4 +15,25 @@ class PasswordsController < Devise::PasswordsController respond_with(resource) end end + + # After a user resets their password, prompt for 2FA code if enabled instead + # of signing in automatically + # + # See http://git.io/vURrI + def update + super do |resource| + # TODO (rspeicher): In Devise master (> 3.4.1), we can set + # `Devise.sign_in_after_reset_password = false` and avoid this mess. + if resource.errors.empty? && resource.try(:otp_required_for_login?) + resource.unlock_access! if unlockable?(resource) + + # Since we are not signing this user in, we use the :updated_not_active + # message which only contains "Your password was changed successfully." + set_flash_message(:notice, :updated_not_active) if is_flashing_format? + + # Redirect to sign in so they can enter 2FA code + respond_with(resource, location: new_session_path(resource)) and return + end + end + end end diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb index 61defb8a333..046a9f6191d 100644 --- a/spec/features/login_spec.rb +++ b/spec/features/login_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' feature 'Login' do - context 'with two-factor authentication' do + describe 'with two-factor authentication' do context 'with valid username/password' do let(:user) { create(:user, :two_factor) } @@ -78,7 +78,7 @@ feature 'Login' do end end - context 'without two-factor authentication' do + describe 'without two-factor authentication' do let(:user) { create(:user) } it 'allows basic login' do diff --git a/spec/features/password_reset_spec.rb b/spec/features/password_reset_spec.rb new file mode 100644 index 00000000000..a34efce09ef --- /dev/null +++ b/spec/features/password_reset_spec.rb @@ -0,0 +1,53 @@ +require 'spec_helper' + +feature 'Password reset' do + def forgot_password + click_on 'Forgot your password?' + fill_in 'Email', with: user.email + click_button 'Reset password' + user.reload + end + + def get_reset_token + mail = ActionMailer::Base.deliveries.last + body = mail.body.encoded + body.scan(/reset_password_token=(.+)\"/).flatten.first + end + + def reset_password(password = 'password') + visit edit_user_password_path(reset_password_token: get_reset_token) + + fill_in 'New password', with: password + fill_in 'Confirm new password', with: password + click_button 'Change your password' + end + + describe 'with two-factor authentication' do + let(:user) { create(:user, :two_factor) } + + it 'requires login after password reset' do + visit root_path + + forgot_password + reset_password + + expect(page).to have_content("Your password was changed successfully.") + expect(page).not_to have_content("You are now signed in.") + expect(current_path).to eq new_user_session_path + end + end + + describe 'without two-factor authentication' do + let(:user) { create(:user) } + + it 'automatically logs in after password reset' do + visit root_path + + forgot_password + reset_password + + expect(current_path).to eq root_path + expect(page).to have_content("Your password was changed successfully. You are now signed in.") + end + end +end -- cgit v1.2.1 From 22badc13136369e202dc6df06a62456110879ee4 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 12 May 2015 10:41:24 +0300 Subject: Bump attr_encrypted Signed-off-by: Dmitriy Zaporozhets --- Gemfile | 2 +- Gemfile.lock | 12 +++--------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/Gemfile b/Gemfile index ee6072b2da1..a62abf2bada 100644 --- a/Gemfile +++ b/Gemfile @@ -37,7 +37,7 @@ gem "rack-oauth2", "~> 1.0.5" # Two-factor authentication gem 'devise-two-factor' gem 'rqrcode-rails3' -gem 'attr_encrypted', git: 'https://github.com/attr-encrypted/attr_encrypted.git', ref: '94d901df2ccbc579b981091d53dd641f9bed4c1d' +gem 'attr_encrypted', '1.3.4' # Browser detection gem "browser" diff --git a/Gemfile.lock b/Gemfile.lock index 672bd9950ed..0ea8021815d 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,11 +1,3 @@ -GIT - remote: https://github.com/attr-encrypted/attr_encrypted.git - revision: 94d901df2ccbc579b981091d53dd641f9bed4c1d - ref: 94d901df2ccbc579b981091d53dd641f9bed4c1d - specs: - attr_encrypted (1.3.3) - encryptor (>= 1.3.0) - GEM remote: https://rubygems.org/ specs: @@ -54,6 +46,8 @@ GEM ast (2.0.0) astrolabe (1.3.0) parser (>= 2.2.0.pre.3, < 3.0) + attr_encrypted (1.3.4) + encryptor (>= 1.3.0) attr_required (1.0.0) autoprefixer-rails (5.1.11) execjs @@ -690,7 +684,7 @@ DEPENDENCIES annotate (~> 2.6.0.beta2) asana (~> 0.0.6) asciidoctor (= 0.1.4) - attr_encrypted! + attr_encrypted (= 1.3.4) awesome_print better_errors binding_of_caller -- cgit v1.2.1