From 88f54b16d7c0f1ea81784b1a30ed1f5d66e368d9 Mon Sep 17 00:00:00 2001 From: Urban Rotnik Date: Wed, 24 Jun 2020 16:23:52 +0200 Subject: [PATCH 01/17] Add 2fa fields to user --- Gemfile | 1 + Gemfile.lock | 3 ++ app/models/user.rb | 10 ++++++ db/migrate/20200622140843_add2fa_to_users.rb | 10 ++++++ db/structure.sql | 7 ++-- spec/models/user_spec.rb | 38 ++++++++++++++++++++ 6 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20200622140843_add2fa_to_users.rb diff --git a/Gemfile b/Gemfile index 047d64b3b..7cf883a0b 100644 --- a/Gemfile +++ b/Gemfile @@ -75,6 +75,7 @@ gem 'nokogiri', '~> 1.10.8' # HTML/XML parser gem 'rails_autolink', '~> 1.1', '>= 1.1.6' gem 'rgl' # Graph framework for project diagram calculations gem 'roo', '~> 2.8.2' # Spreadsheet parser +gem 'rotp' gem 'rubyzip' gem 'scenic', '~> 1.4' gem 'sdoc', '~> 1.0', group: :doc diff --git a/Gemfile.lock b/Gemfile.lock index f6c6c236a..0f2a73a0b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -469,6 +469,8 @@ GEM roo (2.8.2) nokogiri (~> 1) rubyzip (>= 1.2.1, < 2.0.0) + rotp (6.0.0) + addressable (~> 2.7) rspec-core (3.8.2) rspec-support (~> 3.8.0) rspec-expectations (3.8.4) @@ -668,6 +670,7 @@ DEPENDENCIES recaptcha rgl roo (~> 2.8.2) + rotp rspec-rails (>= 4.0.0.beta2) rubocop (>= 0.75.0) rubocop-performance diff --git a/app/models/user.rb b/app/models/user.rb index 45907fcf0..b843ea102 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -286,6 +286,8 @@ class User < ApplicationRecord foreign_key: :resource_owner_id, dependent: :delete_all + before_save :ensure_2fa_token, if: ->(user) { user.changed.include?('twofa_enabled') } + before_create :generate_2fa_token before_destroy :destroy_notifications def name @@ -658,4 +660,12 @@ class User < ApplicationRecord def clear_view_cache Rails.cache.delete_matched(%r{^views\/users\/#{id}-}) end + + def generate_2fa_token + self.otp_secret = ROTP::Base32.random + end + + def ensure_2fa_token + generate_2fa_token unless otp_secret + end end diff --git a/db/migrate/20200622140843_add2fa_to_users.rb b/db/migrate/20200622140843_add2fa_to_users.rb new file mode 100644 index 000000000..98d429169 --- /dev/null +++ b/db/migrate/20200622140843_add2fa_to_users.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class Add2faToUsers < ActiveRecord::Migration[6.0] + def change + change_table :users, bulk: true do |t| + t.boolean :twofa_enabled, default: false, null: false + t.string :otp_secret + end + end +end diff --git a/db/structure.sql b/db/structure.sql index 951f4faa2..0126d461d 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -2647,7 +2647,9 @@ CREATE TABLE public.users ( current_team_id bigint, authentication_token character varying(30), settings jsonb DEFAULT '{}'::jsonb NOT NULL, - variables jsonb DEFAULT '{}'::jsonb NOT NULL + variables jsonb DEFAULT '{}'::jsonb NOT NULL, + twofa_enabled boolean DEFAULT false NOT NULL, + otp_secret character varying ); @@ -7194,6 +7196,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20200113143828'), ('20200204100934'), ('20200326114643'), -('20200331183640'); +('20200331183640'), +('20200622140843'); diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 8c6ff65ff..a12cf0637 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -331,4 +331,42 @@ describe User, type: :model do describe 'Associations' do it { is_expected.to have_many(:system_notifications) } end + + describe 'Callbacks' do + describe 'after_create' do + it 'sets token' do + user = create :user + + expect(user.otp_secret).to be_kind_of String + end + end + + describe 'before_save' do + let(:user) { create :user } + + context 'when changing twofa_enabled' do + context 'when user does not have otp_secret' do + it 'sets token before save' do + user.update_column(:otp_secret, nil) + + expect { user.update(twofa_enabled: true) }.to(change { user.otp_secret }) + end + end + + context 'when user does have otp_secret' do + it 'does not set new token before save' do + expect { user.update(twofa_enabled: true) }.not_to(change { user.otp_secret }) + end + end + end + + context 'when changing not twofa_enabled and user does have otp_secret' do + it 'does not set token before save' do + user.update_column(:otp_secret, nil) + + expect { user.update(name: 'SomeNewName') }.not_to(change { user.otp_secret }) + end + end + end + end end From de8aca9dbd92799f1baa6b79dbe3bee04884612e Mon Sep 17 00:00:00 2001 From: Urban Rotnik Date: Mon, 29 Jun 2020 10:42:19 +0200 Subject: [PATCH 02/17] Rename column --- app/models/user.rb | 8 ++++---- db/migrate/20200622140843_add2fa_to_users.rb | 2 +- db/structure.sql | 2 +- spec/models/user_spec.rb | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index b843ea102..c3614802d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -286,8 +286,8 @@ class User < ApplicationRecord foreign_key: :resource_owner_id, dependent: :delete_all - before_save :ensure_2fa_token, if: ->(user) { user.changed.include?('twofa_enabled') } - before_create :generate_2fa_token + before_save :ensure_2fa_token, if: ->(user) { user.changed.include?('two_factor_auth_enabled') } + before_create :assign_2fa_token before_destroy :destroy_notifications def name @@ -661,11 +661,11 @@ class User < ApplicationRecord Rails.cache.delete_matched(%r{^views\/users\/#{id}-}) end - def generate_2fa_token + def assign_2fa_token self.otp_secret = ROTP::Base32.random end def ensure_2fa_token - generate_2fa_token unless otp_secret + assign_2fa_token unless otp_secret end end diff --git a/db/migrate/20200622140843_add2fa_to_users.rb b/db/migrate/20200622140843_add2fa_to_users.rb index 98d429169..88494cc52 100644 --- a/db/migrate/20200622140843_add2fa_to_users.rb +++ b/db/migrate/20200622140843_add2fa_to_users.rb @@ -3,7 +3,7 @@ class Add2faToUsers < ActiveRecord::Migration[6.0] def change change_table :users, bulk: true do |t| - t.boolean :twofa_enabled, default: false, null: false + t.boolean :two_factor_auth_enabled, default: false, null: false t.string :otp_secret end end diff --git a/db/structure.sql b/db/structure.sql index 0126d461d..3843bfc00 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -2648,7 +2648,7 @@ CREATE TABLE public.users ( authentication_token character varying(30), settings jsonb DEFAULT '{}'::jsonb NOT NULL, variables jsonb DEFAULT '{}'::jsonb NOT NULL, - twofa_enabled boolean DEFAULT false NOT NULL, + two_factor_auth_enabled boolean DEFAULT false NOT NULL, otp_secret character varying ); diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a12cf0637..f9af6ec2a 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -349,13 +349,13 @@ describe User, type: :model do it 'sets token before save' do user.update_column(:otp_secret, nil) - expect { user.update(twofa_enabled: true) }.to(change { user.otp_secret }) + expect { user.update(two_factor_auth_enabled: true) }.to(change { user.otp_secret }) end end context 'when user does have otp_secret' do it 'does not set new token before save' do - expect { user.update(twofa_enabled: true) }.not_to(change { user.otp_secret }) + expect { user.update(two_factor_auth_enabled: true) }.not_to(change { user.otp_secret }) end end end From 6fa97d3639a3340019f8d50af13c0663672389b9 Mon Sep 17 00:00:00 2001 From: aignatov-bio Date: Mon, 29 Jun 2020 10:56:06 +0200 Subject: [PATCH 03/17] Add enable 2fa button to user settings --- app/assets/javascripts/users/registrations/edit.js | 4 ++++ app/views/users/registrations/_2fa_modal.html.erb | 12 ++++++++++++ app/views/users/registrations/edit.html.erb | 3 ++- config/locales/en.yml | 1 + 4 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 app/views/users/registrations/_2fa_modal.html.erb diff --git a/app/assets/javascripts/users/registrations/edit.js b/app/assets/javascripts/users/registrations/edit.js index cd2a5d551..7d02da59b 100644 --- a/app/assets/javascripts/users/registrations/edit.js +++ b/app/assets/javascripts/users/registrations/edit.js @@ -79,4 +79,8 @@ } $fileInput[0].value = ''; }); + + $('#twoFactorAuthentication').click(function() { + $('#twoFactorAuthenticationModal').modal('show'); + }); }()); diff --git a/app/views/users/registrations/_2fa_modal.html.erb b/app/views/users/registrations/_2fa_modal.html.erb new file mode 100644 index 000000000..04e217a2b --- /dev/null +++ b/app/views/users/registrations/_2fa_modal.html.erb @@ -0,0 +1,12 @@ + diff --git a/app/views/users/registrations/edit.html.erb b/app/views/users/registrations/edit.html.erb index 3f087e933..aca9cef15 100644 --- a/app/views/users/registrations/edit.html.erb +++ b/app/views/users/registrations/edit.html.erb @@ -142,7 +142,7 @@ <% end %> - +
@@ -176,6 +176,7 @@
+<%= render partial: '2fa_modal' %> <%= render partial: 'users/shared/user_avatars_modal' %> <%= javascript_pack_tag 'custom/croppie' %> diff --git a/config/locales/en.yml b/config/locales/en.yml index ef998024b..81a487b8b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1593,6 +1593,7 @@ en: password_title: "Change password" new_password_label: "New password" new_password_2_label: "New password confirmation" + 2fa_button: "Enable two-factor authentication" new: head_title: "Sign up" team_name_label: "Team name" From fe79fa47889bf43e468e32bc003546837b868e45 Mon Sep 17 00:00:00 2001 From: aignatov-bio Date: Mon, 29 Jun 2020 10:57:41 +0200 Subject: [PATCH 04/17] Fix markup --- app/views/users/registrations/_2fa_modal.html.erb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/users/registrations/_2fa_modal.html.erb b/app/views/users/registrations/_2fa_modal.html.erb index 04e217a2b..fea5db397 100644 --- a/app/views/users/registrations/_2fa_modal.html.erb +++ b/app/views/users/registrations/_2fa_modal.html.erb @@ -10,3 +10,4 @@ + From 62a4a5f2030f6943d86cc588277ba46919abb236 Mon Sep 17 00:00:00 2001 From: Urban Rotnik Date: Tue, 30 Jun 2020 14:16:00 +0200 Subject: [PATCH 05/17] Add 2fa login --- app/controllers/users/sessions_controller.rb | 64 ++++++++--- app/models/user.rb | 7 ++ app/views/users/sessions/new.html.erb | 2 +- .../users/sessions/two_factor_auth.html.erb | 19 ++++ config/locales/en.yml | 6 + config/routes.rb | 4 +- .../users/sessions_controller_spec.rb | 107 ++++++++++++++++++ spec/models/user_spec.rb | 21 ++++ 8 files changed, 212 insertions(+), 18 deletions(-) create mode 100644 app/views/users/sessions/two_factor_auth.html.erb create mode 100644 spec/controllers/users/sessions_controller_spec.rb diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index a52c97e8e..50d7c7258 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -5,6 +5,7 @@ class Users::SessionsController < Devise::SessionsController # before_filter :configure_sign_in_params, only: [:create] after_action :after_sign_in, only: :create + prepend_before_action :redirect_2fa, only: :create rescue_from ActionController::InvalidAuthenticityToken do redirect_to new_user_session_path @@ -24,21 +25,7 @@ class Users::SessionsController < Devise::SessionsController def create super - # Schedule templates creation for user - TemplatesService.new.schedule_creation_for_user(current_user) - - # Schedule demo project creation for user - current_user.created_teams.each do |team| - FirstTimeDataGenerator.delay( - queue: :new_demo_project, - priority: 10 - ).seed_demo_data_with_id(current_user.id, team.id) - end - rescue StandardError => e - Rails.logger.fatal( - "User ID #{current_user.id}: Error creating inital projects on sign_in: "\ - "#{e.message}" - ) + generate_demo_project end # DELETE /resource/sign_out @@ -75,6 +62,27 @@ class Users::SessionsController < Devise::SessionsController flash[:system_notification_modal] = true end + def authenticate_with_two_factor + user = User.find_by(id: session[:otp_user_id]) + + unless user + flash[:alert] = t('devise.sessions.2fa.no_user_error') + redirect_to root_path && return + end + + if user.valid_otp?(params[:otp]) + session.delete(:otp_user_id) + + sign_in(user) + generate_demo_project + flash[:notice] = t('devise.sessions.signed_in') + redirect_to root_path + else + flash.now[:alert] = t('devise.sessions.2fa.error_message') + render :two_factor_auth + end + end + protected # If you have extra params to permit, append them to the sanitizer. @@ -84,6 +92,32 @@ class Users::SessionsController < Devise::SessionsController private + def redirect_2fa + user = User.find_by(email: params[:user][:email]) + + return unless user&.valid_password?(params[:user][:password]) + + if user&.two_factor_auth_enabled? + session[:otp_user_id] = user.id + render :two_factor_auth + end + end + + def generate_demo_project + # Schedule templates creation for user + TemplatesService.new.schedule_creation_for_user(current_user) + + # Schedule demo project creation for user + current_user.created_teams.each do |team| + FirstTimeDataGenerator.delay( + queue: :new_demo_project, + priority: 10 + ).seed_demo_data_with_id(current_user.id, team.id) + end + rescue StandardError => e + Rails.logger.fatal("User ID #{current_user.id}: Error creating inital projects on sign_in: #{e.message}") + end + def session_layout if @simple_sign_in 'sign_in_halt' diff --git a/app/models/user.rb b/app/models/user.rb index c3614802d..5b94c7b7f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -623,6 +623,13 @@ class User < ApplicationRecord avatar.blob&.filename&.sanitized end + def valid_otp?(otp) + raise StandardError, 'Missing otp_secret' unless otp_secret + + totp = ROTP::TOTP.new(otp_secret, issuer: 'sciNote') + totp.verify(otp, drift_behind: 10) + end + protected def confirmation_required? diff --git a/app/views/users/sessions/new.html.erb b/app/views/users/sessions/new.html.erb index 9d162a5a3..a12098fd2 100644 --- a/app/views/users/sessions/new.html.erb +++ b/app/views/users/sessions/new.html.erb @@ -43,4 +43,4 @@ <%= render partial: "users/shared/links", locals: {linkedin_skip: true} unless @simple_sign_in %> - \ No newline at end of file + diff --git a/app/views/users/sessions/two_factor_auth.html.erb b/app/views/users/sessions/two_factor_auth.html.erb new file mode 100644 index 000000000..ec1de0fa4 --- /dev/null +++ b/app/views/users/sessions/two_factor_auth.html.erb @@ -0,0 +1,19 @@ +<% provide(:head_title, t("devise.sessions.new.head_title")) %> +<% content_for(:body_class, 'sign-in-layout') %> + diff --git a/config/locales/en.yml b/config/locales/en.yml index 81a487b8b..8b1cfce2a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -33,6 +33,12 @@ en: password_placeholder: "Enter password" remember_me: "Remember me" submit: "Log in" + 2fa: + title: "Two factor check" + field: "One time password" + placeholder: "Enter code" + error_message: "One Time Password is not correct." + no_user_error: "Cannot find user!" create: team_name: "%{user}'s projects" auth_token_create: diff --git a/config/routes.rb b/config/routes.rb index 206b1649d..c5c38009e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -695,8 +695,8 @@ Rails.application.routes.draw do get 'avatar/:id/:style' => 'users/registrations#avatar', as: 'avatar' get 'users/auth_token_sign_in' => 'users/sessions#auth_token_create' get 'users/sign_up_provider' => 'users/registrations#new_with_provider' - post 'users/complete_sign_up_provider' => - 'users/registrations#create_with_provider' + post 'users/authenticate_with_two_factor' => 'users/sessions#authenticate_with_two_factor' + post 'users/complete_sign_up_provider' => 'users/registrations#create_with_provider' end namespace :api, defaults: { format: 'json' } do diff --git a/spec/controllers/users/sessions_controller_spec.rb b/spec/controllers/users/sessions_controller_spec.rb new file mode 100644 index 000000000..718939da1 --- /dev/null +++ b/spec/controllers/users/sessions_controller_spec.rb @@ -0,0 +1,107 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Users::SessionsController, type: :controller do + describe 'POST #create' do + before do + @request.env['devise.mapping'] = Devise.mappings[:user] + end + + let(:user) { create :user } + let(:password) { 'asdf1243' } + let(:params) do + { user: { + email: user.email, + password: password + } } + end + + let(:action) do + post :create, params: params + end + + context 'when have invalid email or password' do + let(:password) { '123' } + + it 'returns error message' do + action + + expect(flash[:alert]).to eq('Invalid Email or password.') + end + + it 'does not set current user' do + expect { action }.not_to(change { subject.current_user }) + end + end + + context 'when have valid email and password' do + context 'when user has 2FA disabled' do + it 'returns successfully log in' do + action + + expect(flash[:notice]).to eq('Logged in successfully.') + end + + it 'sets current user' do + expect { action }.to(change { subject.current_user }.from(nil).to(User)) + end + end + + context 'when user has 2FA enabled' do + it 'renders 2FA page' do + user.two_factor_auth_enabled = true + user.save! + + expect(action).to render_template('users/sessions/two_factor_auth') + end + end + end + end + + describe 'POST #authenticate_with_two_factor' do + before do + @request.env['devise.mapping'] = Devise.mappings[:user] + end + + let(:user) { create :user } + let(:params) { { otp: '123123' } } + let(:otp_user_id) { user.id } + let(:action) do + post :authenticate_with_two_factor, params: params, session: { otp_user_id: otp_user_id } + end + + context 'when have valid otp' do + it 'sets current user' do + allow_any_instance_of(User).to receive(:valid_otp?).and_return(true) + + expect { action }.to(change { subject.current_user }.from(nil).to(User)) + end + end + + context 'when have invalid valid otp' do + it 'returns error message' do + allow_any_instance_of(User).to receive(:valid_otp?).and_return(nil) + action + + expect(flash[:alert]).to eq('One Time Password is not correct.') + end + + it 'does not set current user' do + allow_any_instance_of(User).to receive(:valid_otp?).and_return(nil) + + expect { action }.not_to(change { subject.current_user }) + end + end + + context 'when user is not found' do + let(:otp_user_id) { -1 } + + it 'returns error message' do + action + + expect(flash[:alert]).to eq('Cannot find user!') + end + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index f9af6ec2a..ae6bbf1c4 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -369,4 +369,25 @@ describe User, type: :model do end end end + + describe 'valid_otp?' do + let(:user) { create :user } + before do + allow_any_instance_of(ROTP::TOTP).to receive(:verify).and_return(nil) + end + + context 'when user has set otp_secret' do + it 'returns nil' do + expect(user.valid_otp?('someString')).to be_nil + end + end + + context 'when user does not have otp_secret' do + it 'raises an error' do + user.update_column(:otp_secret, nil) + + expect { user.valid_otp?('someString') }.to raise_error(StandardError, 'Missing otp_secret') + end + end + end end From 4b9881e31edfa1103b4a52f2ee75b861f6f41a29 Mon Sep 17 00:00:00 2001 From: aignatov-bio Date: Wed, 1 Jul 2020 11:07:33 +0200 Subject: [PATCH 06/17] Add 2fa to user settings page --- Gemfile | 1 + Gemfile.lock | 6 +++ .../javascripts/users/registrations/edit.js | 17 ++++++++- .../users/registrations_controller.rb | 26 +++++++++++++ app/models/user.rb | 12 +++--- .../users/registrations/_2fa_modal.html.erb | 30 ++++++++++++++- app/views/users/registrations/edit.html.erb | 6 ++- config/locales/en.yml | 9 +++++ config/routes.rb | 4 ++ spec/models/user_spec.rb | 38 ------------------- 10 files changed, 102 insertions(+), 47 deletions(-) diff --git a/Gemfile b/Gemfile index 7cf883a0b..cacc6aa73 100644 --- a/Gemfile +++ b/Gemfile @@ -76,6 +76,7 @@ gem 'rails_autolink', '~> 1.1', '>= 1.1.6' gem 'rgl' # Graph framework for project diagram calculations gem 'roo', '~> 2.8.2' # Spreadsheet parser gem 'rotp' +gem 'rqrcode' # QR code generator gem 'rubyzip' gem 'scenic', '~> 1.4' gem 'sdoc', '~> 1.0', group: :doc diff --git a/Gemfile.lock b/Gemfile.lock index 0f2a73a0b..599242fc6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -186,6 +186,7 @@ GEM activesupport childprocess (1.0.1) rake (< 13.0) + chunky_png (1.3.11) coderay (1.1.2) coffee-rails (5.0.0) coffee-script (>= 2.2.0) @@ -471,6 +472,10 @@ GEM rubyzip (>= 1.2.1, < 2.0.0) rotp (6.0.0) addressable (~> 2.7) + rqrcode (1.1.2) + chunky_png (~> 1.0) + rqrcode_core (~> 0.1) + rqrcode_core (0.1.2) rspec-core (3.8.2) rspec-support (~> 3.8.0) rspec-expectations (3.8.4) @@ -671,6 +676,7 @@ DEPENDENCIES rgl roo (~> 2.8.2) rotp + rqrcode rspec-rails (>= 4.0.0.beta2) rubocop (>= 0.75.0) rubocop-performance diff --git a/app/assets/javascripts/users/registrations/edit.js b/app/assets/javascripts/users/registrations/edit.js index 7d02da59b..388db117b 100644 --- a/app/assets/javascripts/users/registrations/edit.js +++ b/app/assets/javascripts/users/registrations/edit.js @@ -80,7 +80,22 @@ $fileInput[0].value = ''; }); - $('#twoFactorAuthentication').click(function() { + $('#twoFactorAuthenticationDisable').click(function() { $('#twoFactorAuthenticationModal').modal('show'); }); + + $('#twoFactorAuthenticationEnable').click(function() { + $.get(this.dataset.qrCodeUrl, function(result) { + $('#twoFactorAuthenticationModal .qr-code').html(result.qr_code); + $('#twoFactorAuthenticationModal').modal('show'); + }); + }); + + $('#twoFactorAuthenticationModal .2fa-enable-form').on('ajax:error', function(e, data) { + $(this).find('.submit-code-field').addClass('error').attr('data-error-text', data.responseJSON.error); + }); + + $('#twoFactorAuthenticationModal .2fa-disable-form').on('ajax:error', function(e, data) { + $(this).find('.password-field').addClass('error').attr('data-error-text', data.responseJSON.error); + }); }()); diff --git a/app/controllers/users/registrations_controller.rb b/app/controllers/users/registrations_controller.rb index ea2fac89e..67801ccaf 100644 --- a/app/controllers/users/registrations_controller.rb +++ b/app/controllers/users/registrations_controller.rb @@ -182,6 +182,32 @@ class Users::RegistrationsController < Devise::RegistrationsController end end + def two_factor_enable + totp = ROTP::TOTP.new(current_user.otp_secret, issuer: 'SciNote') + if totp.verify(params[:submit_code], drift_behind: 10) + current_user.update!(two_factor_auth_enabled: true) + redirect_to edit_user_registration_path + else + render json: { error: t('users.registrations.edit.2fa_errors.wrong_submit_code') }, status: :unprocessable_entity + end + end + + def two_factor_disable + if current_user.valid_password?(params[:password]) + current_user.update!(two_factor_auth_enabled: false, otp_secret: nil) + redirect_to edit_user_registration_path + else + render json: { error: t('users.registrations.edit.2fa_errors.wrong_password') }, status: :forbidden + end + end + + def two_factor_qr_code + current_user.ensure_2fa_token + qr_code_url = ROTP::TOTP.new(current_user.otp_secret, issuer: 'SciNote').provisioning_uri(current_user.email) + qr_code = RQRCode::QRCode.new(qr_code_url) + render json: { qr_code: qr_code.as_svg } + end + protected # Called upon creating User (before .save). Permits parameters and extracts diff --git a/app/models/user.rb b/app/models/user.rb index 5b94c7b7f..eed3623c0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -286,8 +286,6 @@ class User < ApplicationRecord foreign_key: :resource_owner_id, dependent: :delete_all - before_save :ensure_2fa_token, if: ->(user) { user.changed.include?('two_factor_auth_enabled') } - before_create :assign_2fa_token before_destroy :destroy_notifications def name @@ -630,6 +628,13 @@ class User < ApplicationRecord totp.verify(otp, drift_behind: 10) end + def ensure_2fa_token + return if otp_secret + + assign_2fa_token + save! + end + protected def confirmation_required? @@ -672,7 +677,4 @@ class User < ApplicationRecord self.otp_secret = ROTP::Base32.random end - def ensure_2fa_token - assign_2fa_token unless otp_secret - end end diff --git a/app/views/users/registrations/_2fa_modal.html.erb b/app/views/users/registrations/_2fa_modal.html.erb index fea5db397..9b07d15bc 100644 --- a/app/views/users/registrations/_2fa_modal.html.erb +++ b/app/views/users/registrations/_2fa_modal.html.erb @@ -3,9 +3,35 @@
-

+

<%= t("users.registrations.edit.2fa_modal.title") %>

-
+ <% if current_user.two_factor_auth_enabled? %> + <%= form_with(url: users_2fa_disable_path, method: "post", class: "2fa-disable-form") do %> +
+
+ <%= label_tag :password, t("users.registrations.edit.2fa_modal.password_label") %> + <%= password_field_tag :password, '', class: "sci-input-field" %> +
+
+
+ <%= button_tag t("users.registrations.edit.2fa_modal.submit_button"), type: 'submit', class: "btn btn-danger" %> +
+ <% end %> + <% else %> + <%= form_with(url: users_2fa_enable_path, method: "post", class: "2fa-enable-form") do %> +
+
+
+ <%= label_tag :submit_code, t("users.registrations.edit.2fa_modal.submit_code_label") %> + <%= text_field_tag :submit_code, '', class: "sci-input-field" %> +
+
+
+ <%= button_tag t("users.registrations.edit.2fa_modal.submit_button"), type: 'submit', class: "btn btn-primary" %> +
+ <% end %> + <% end %> +
diff --git a/app/views/users/registrations/edit.html.erb b/app/views/users/registrations/edit.html.erb index aca9cef15..2fe957e58 100644 --- a/app/views/users/registrations/edit.html.erb +++ b/app/views/users/registrations/edit.html.erb @@ -142,7 +142,11 @@ <% end %> - + <% if current_user.two_factor_auth_enabled? %> + + <% else %> + + <% end %>
diff --git a/config/locales/en.yml b/config/locales/en.yml index 8b1cfce2a..f235c0db7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1600,6 +1600,15 @@ en: new_password_label: "New password" new_password_2_label: "New password confirmation" 2fa_button: "Enable two-factor authentication" + 2fa_button_disable: "Disable two-factor authentication" + 2fa_modal: + title: 'Two-factor authentication' + password_label: 'Password' + submit_code_label: 'Submit code' + submit_button: 'Submit' + 2fa_errors: + wrong_submit_code: 'Not correct code' + wrong_password: 'Not correct password' new: head_title: "Sign up" team_name_label: "Team name" diff --git a/config/routes.rb b/config/routes.rb index c5c38009e..24f9fb9d5 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -697,6 +697,10 @@ Rails.application.routes.draw do get 'users/sign_up_provider' => 'users/registrations#new_with_provider' post 'users/authenticate_with_two_factor' => 'users/sessions#authenticate_with_two_factor' post 'users/complete_sign_up_provider' => 'users/registrations#create_with_provider' + + post 'users/2fa_enable' => 'users/registrations#two_factor_enable' + post 'users/2fa_disable' => 'users/registrations#two_factor_disable' + get 'users/2fa_qr_code' => 'users/registrations#two_factor_qr_code' end namespace :api, defaults: { format: 'json' } do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ae6bbf1c4..ff51c9e7b 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -332,44 +332,6 @@ describe User, type: :model do it { is_expected.to have_many(:system_notifications) } end - describe 'Callbacks' do - describe 'after_create' do - it 'sets token' do - user = create :user - - expect(user.otp_secret).to be_kind_of String - end - end - - describe 'before_save' do - let(:user) { create :user } - - context 'when changing twofa_enabled' do - context 'when user does not have otp_secret' do - it 'sets token before save' do - user.update_column(:otp_secret, nil) - - expect { user.update(two_factor_auth_enabled: true) }.to(change { user.otp_secret }) - end - end - - context 'when user does have otp_secret' do - it 'does not set new token before save' do - expect { user.update(two_factor_auth_enabled: true) }.not_to(change { user.otp_secret }) - end - end - end - - context 'when changing not twofa_enabled and user does have otp_secret' do - it 'does not set token before save' do - user.update_column(:otp_secret, nil) - - expect { user.update(name: 'SomeNewName') }.not_to(change { user.otp_secret }) - end - end - end - end - describe 'valid_otp?' do let(:user) { create :user } before do From 742fb0d27b1a24a67b51cf7d4cc85ca9d5328bfb Mon Sep 17 00:00:00 2001 From: aignatov-bio Date: Wed, 1 Jul 2020 14:41:55 +0200 Subject: [PATCH 07/17] Small 2fa improvments --- app/controllers/users/registrations_controller.rb | 7 +++---- app/controllers/users/sessions_controller.rb | 2 +- app/models/user.rb | 8 ++++++++ 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/app/controllers/users/registrations_controller.rb b/app/controllers/users/registrations_controller.rb index 67801ccaf..1de739f67 100644 --- a/app/controllers/users/registrations_controller.rb +++ b/app/controllers/users/registrations_controller.rb @@ -183,9 +183,8 @@ class Users::RegistrationsController < Devise::RegistrationsController end def two_factor_enable - totp = ROTP::TOTP.new(current_user.otp_secret, issuer: 'SciNote') - if totp.verify(params[:submit_code], drift_behind: 10) - current_user.update!(two_factor_auth_enabled: true) + if current_user.valid_otp?(params[:submit_code]) + current_user.enable_2fa redirect_to edit_user_registration_path else render json: { error: t('users.registrations.edit.2fa_errors.wrong_submit_code') }, status: :unprocessable_entity @@ -194,7 +193,7 @@ class Users::RegistrationsController < Devise::RegistrationsController def two_factor_disable if current_user.valid_password?(params[:password]) - current_user.update!(two_factor_auth_enabled: false, otp_secret: nil) + current_user.disable_2fa redirect_to edit_user_registration_path else render json: { error: t('users.registrations.edit.2fa_errors.wrong_password') }, status: :forbidden diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index 50d7c7258..aef23a8c1 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -4,7 +4,7 @@ class Users::SessionsController < Devise::SessionsController layout :session_layout # before_filter :configure_sign_in_params, only: [:create] - after_action :after_sign_in, only: :create + after_action :after_sign_in, only: %i(create authenticate_with_two_factor) prepend_before_action :redirect_2fa, only: :create rescue_from ActionController::InvalidAuthenticityToken do diff --git a/app/models/user.rb b/app/models/user.rb index eed3623c0..c8e400c23 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -635,6 +635,14 @@ class User < ApplicationRecord save! end + def enable_2fa + update!(two_factor_auth_enabled: true) + end + + def disable_2fa + update!(two_factor_auth_enabled: false, otp_secret: nil) + end + protected def confirmation_required? From 46256c89c9fdcf342f04c36ff725d4424842583f Mon Sep 17 00:00:00 2001 From: aignatov-bio Date: Wed, 1 Jul 2020 15:44:52 +0200 Subject: [PATCH 08/17] Fix tests --- app/controllers/users/registrations_controller.rb | 6 +++--- app/models/user.rb | 13 ++++--------- spec/models/user_spec.rb | 1 + 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/app/controllers/users/registrations_controller.rb b/app/controllers/users/registrations_controller.rb index 1de739f67..c8b44c20a 100644 --- a/app/controllers/users/registrations_controller.rb +++ b/app/controllers/users/registrations_controller.rb @@ -184,7 +184,7 @@ class Users::RegistrationsController < Devise::RegistrationsController def two_factor_enable if current_user.valid_otp?(params[:submit_code]) - current_user.enable_2fa + current_user.enable_2fa! redirect_to edit_user_registration_path else render json: { error: t('users.registrations.edit.2fa_errors.wrong_submit_code') }, status: :unprocessable_entity @@ -193,7 +193,7 @@ class Users::RegistrationsController < Devise::RegistrationsController def two_factor_disable if current_user.valid_password?(params[:password]) - current_user.disable_2fa + current_user.disable_2fa! redirect_to edit_user_registration_path else render json: { error: t('users.registrations.edit.2fa_errors.wrong_password') }, status: :forbidden @@ -201,7 +201,7 @@ class Users::RegistrationsController < Devise::RegistrationsController end def two_factor_qr_code - current_user.ensure_2fa_token + current_user.ensure_2fa_token! qr_code_url = ROTP::TOTP.new(current_user.otp_secret, issuer: 'SciNote').provisioning_uri(current_user.email) qr_code = RQRCode::QRCode.new(qr_code_url) render json: { qr_code: qr_code.as_svg } diff --git a/app/models/user.rb b/app/models/user.rb index c8e400c23..b281dd57b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -628,18 +628,18 @@ class User < ApplicationRecord totp.verify(otp, drift_behind: 10) end - def ensure_2fa_token + def ensure_2fa_token! return if otp_secret - assign_2fa_token + self.otp_secret = ROTP::Base32.random save! end - def enable_2fa + def enable_2fa! update!(two_factor_auth_enabled: true) end - def disable_2fa + def disable_2fa! update!(two_factor_auth_enabled: false, otp_secret: nil) end @@ -680,9 +680,4 @@ class User < ApplicationRecord def clear_view_cache Rails.cache.delete_matched(%r{^views\/users\/#{id}-}) end - - def assign_2fa_token - self.otp_secret = ROTP::Base32.random - end - end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ff51c9e7b..2cd07954b 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -335,6 +335,7 @@ describe User, type: :model do describe 'valid_otp?' do let(:user) { create :user } before do + user.ensure_2fa_token! allow_any_instance_of(ROTP::TOTP).to receive(:verify).and_return(nil) end From 9b33cbc1040d8a7711fc52902c35f00de5900bb2 Mon Sep 17 00:00:00 2001 From: aignatov-bio Date: Thu, 2 Jul 2020 11:24:33 +0200 Subject: [PATCH 09/17] Rename ensure 2fa method --- app/controllers/users/registrations_controller.rb | 2 +- app/models/user.rb | 4 +--- spec/models/user_spec.rb | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/controllers/users/registrations_controller.rb b/app/controllers/users/registrations_controller.rb index c8b44c20a..7c196ed69 100644 --- a/app/controllers/users/registrations_controller.rb +++ b/app/controllers/users/registrations_controller.rb @@ -201,7 +201,7 @@ class Users::RegistrationsController < Devise::RegistrationsController end def two_factor_qr_code - current_user.ensure_2fa_token! + current_user.assign_2fa_token! qr_code_url = ROTP::TOTP.new(current_user.otp_secret, issuer: 'SciNote').provisioning_uri(current_user.email) qr_code = RQRCode::QRCode.new(qr_code_url) render json: { qr_code: qr_code.as_svg } diff --git a/app/models/user.rb b/app/models/user.rb index b281dd57b..89609fadc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -628,9 +628,7 @@ class User < ApplicationRecord totp.verify(otp, drift_behind: 10) end - def ensure_2fa_token! - return if otp_secret - + def assign_2fa_token! self.otp_secret = ROTP::Base32.random save! end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 2cd07954b..376050d13 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -335,7 +335,7 @@ describe User, type: :model do describe 'valid_otp?' do let(:user) { create :user } before do - user.ensure_2fa_token! + user.assign_2fa_token! allow_any_instance_of(ROTP::TOTP).to receive(:verify).and_return(nil) end From be3a1994f836a4fc7d5edbd8c7ef3e1cf88fca99 Mon Sep 17 00:00:00 2001 From: aignatov-bio Date: Tue, 7 Jul 2020 12:59:48 +0200 Subject: [PATCH 10/17] Add basic UX to 2FA --- .../javascripts/users/registrations/edit.js | 5 + app/assets/stylesheets/settings/users.scss | 49 ++++ .../users/registrations_controller.rb | 2 +- app/javascript/packs/fontawesome.scss | 1 + .../users/registrations/_2fa_modal.html.erb | 35 ++- app/views/users/registrations/edit.html.erb | 218 +++++------------- 6 files changed, 126 insertions(+), 184 deletions(-) diff --git a/app/assets/javascripts/users/registrations/edit.js b/app/assets/javascripts/users/registrations/edit.js index 388db117b..38969d9a3 100644 --- a/app/assets/javascripts/users/registrations/edit.js +++ b/app/assets/javascripts/users/registrations/edit.js @@ -87,6 +87,7 @@ $('#twoFactorAuthenticationEnable').click(function() { $.get(this.dataset.qrCodeUrl, function(result) { $('#twoFactorAuthenticationModal .qr-code').html(result.qr_code); + $('#twoFactorAuthenticationModal').find('[href="#2fa-step-1"]').tab('show'); $('#twoFactorAuthenticationModal').modal('show'); }); }); @@ -98,4 +99,8 @@ $('#twoFactorAuthenticationModal .2fa-disable-form').on('ajax:error', function(e, data) { $(this).find('.password-field').addClass('error').attr('data-error-text', data.responseJSON.error); }); + + $('#twoFactorAuthenticationModal').on('click', '.btn-next-step', function() { + $('#twoFactorAuthenticationModal').find(`[href="${$(this).data('step')}"]`).tab('show'); + }) }()); diff --git a/app/assets/stylesheets/settings/users.scss b/app/assets/stylesheets/settings/users.scss index 90840a469..204355d72 100644 --- a/app/assets/stylesheets/settings/users.scss +++ b/app/assets/stylesheets/settings/users.scss @@ -5,3 +5,52 @@ display: block; margin-bottom: 20px; } + +.two-factor-modal { + .two-factor-apps { + align-items: center; + display: flex; + margin: 2em 0; + + .app { + align-items: center; + display: flex; + margin: 2em 0; + + .app-information { + margin-left: 1.5em; + + .app-name { + @include font-h3; + } + } + + .store { + margin-right: 1em; + } + } + + .apps-list { + flex-shrink: 0; + z-index: 2; + } + + .install-mobile { + margin-left: -150px; + } + } + + .modal-footer { + text-align: center; + } + + .tab-footer { + text-align: center; + } + + .qr-code { + display: flex; + justify-content: center; + padding: 4em; + } +} diff --git a/app/controllers/users/registrations_controller.rb b/app/controllers/users/registrations_controller.rb index 7c196ed69..4e2b65929 100644 --- a/app/controllers/users/registrations_controller.rb +++ b/app/controllers/users/registrations_controller.rb @@ -204,7 +204,7 @@ class Users::RegistrationsController < Devise::RegistrationsController current_user.assign_2fa_token! qr_code_url = ROTP::TOTP.new(current_user.otp_secret, issuer: 'SciNote').provisioning_uri(current_user.email) qr_code = RQRCode::QRCode.new(qr_code_url) - render json: { qr_code: qr_code.as_svg } + render json: { qr_code: qr_code.as_svg(module_size: 4) } end protected diff --git a/app/javascript/packs/fontawesome.scss b/app/javascript/packs/fontawesome.scss index fb98b131a..3d61ff3cc 100644 --- a/app/javascript/packs/fontawesome.scss +++ b/app/javascript/packs/fontawesome.scss @@ -3,3 +3,4 @@ $fa-font-path: "~@fortawesome/fontawesome-free/webfonts/"; @import "~@fortawesome/fontawesome-free/scss/fontawesome"; @import "~@fortawesome/fontawesome-free/scss/solid"; @import "~@fortawesome/fontawesome-free/scss/regular"; +@import "~@fortawesome/fontawesome-free/scss/brands"; diff --git a/app/views/users/registrations/_2fa_modal.html.erb b/app/views/users/registrations/_2fa_modal.html.erb index 9b07d15bc..ecbf62bba 100644 --- a/app/views/users/registrations/_2fa_modal.html.erb +++ b/app/views/users/registrations/_2fa_modal.html.erb @@ -1,38 +1,35 @@ -