From a38aef8c7c836eaeb21646755c45da2580278cdf Mon Sep 17 00:00:00 2001 From: Rasmus Kjellberg <2277443+kjellberg@users.noreply.github.com> Date: Mon, 1 Apr 2024 15:12:42 +0200 Subject: [PATCH] feat: two factor authentication --- app/controllers/accounts_controller.rb | 2 +- app/controllers/application_controller.rb | 23 +++++----- .../users/onboarding_controller.rb | 2 +- app/controllers/users/sessions_controller.rb | 44 +++++++++++++++++++ app/models/user.rb | 14 +++--- app/views/users/sessions/new.html.erb | 1 + app/views/users/sessions/otp.html.erb | 18 ++++++++ config/credentials/test.key | 1 + config/credentials/test.yml.enc | 1 + .../initializers/filter_parameter_logging.rb | 2 +- config/locales/kiqr.en.yml | 9 ++++ .../rails/credentials/credentials.yml.tt | 13 ++++++ .../users/sessions_controller_test.rb | 17 +++++++ test/factories/user.rb | 5 +++ test/system/signin_test.rb | 14 ++++++ 15 files changed, 147 insertions(+), 19 deletions(-) create mode 100644 app/views/users/sessions/otp.html.erb create mode 100644 config/credentials/test.key create mode 100644 config/credentials/test.yml.enc create mode 100644 lib/templates/rails/credentials/credentials.yml.tt create mode 100644 test/controllers/users/sessions_controller_test.rb diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 3773aa6..eda9b7b 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -5,7 +5,7 @@ def edit end def update - if @account.update(account_params) + if @account.update(account_permitted_parameters) redirect_to edit_account_path, notice: I18n.t("accounts.update.success") else render :edit, status: :unprocessable_entity diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index a5d5899..fdf79f7 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -4,22 +4,15 @@ class ApplicationController < ActionController::Base before_action :authenticate_user! before_action :ensure_onboarded, unless: :devise_controller? - before_action :configure_permitted_parameters, if: :devise_controller? - - protected - - def configure_permitted_parameters - devise_parameter_sanitizer.permit(:sign_in, keys: [:otp_attempt]) - end - - private # Strong parameters for account. # Used for account creation and update. - def account_params + def account_permitted_parameters params.require(:account).permit(:name) end + private + # Automatically include account_id in all URL options if it is already present in the params. # This is used to ensure that all routes are scoped to the current team. Personal account # routes are not affected. @@ -32,8 +25,16 @@ def ensure_onboarded redirect_to onboarding_path if user_signed_in? && !current_user.onboarded? end - # Redirect to dashboard after sign in + # Override the method to change the sign-in redirect path def after_sign_in_path_for(resource) dashboard_path end + + # Override the method to change the sign-out redirect path + def after_sign_out_path_for(resource_or_scope) + # Generate the root path without default URL options + uri = URI.parse(root_url) + uri.query = nil # Remove any query parameters + uri.to_s + end end diff --git a/app/controllers/users/onboarding_controller.rb b/app/controllers/users/onboarding_controller.rb index b56aa11..5c2777b 100644 --- a/app/controllers/users/onboarding_controller.rb +++ b/app/controllers/users/onboarding_controller.rb @@ -21,7 +21,7 @@ def new end def create - @account = current_user.build_personal_account(account_params) + @account = current_user.build_personal_account(account_permitted_parameters) @account.personal = true if current_user.save diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index d2daa38..be0d75d 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -1,2 +1,46 @@ class Users::SessionsController < Devise::SessionsController + # prepend_before_action :configure_permitted_parameters, if: :devise_controller? + prepend_before_action :otp_authentication, only: :create + + def new + # New login attempts should reset the otp_user_id + session.delete(:otp_user_id) + super + end + + # Override the default Devise create method and check if the user has enabled 2FA + # If 2FA is enabled, render the OTP form page. Otherwise, proceed with the default login flow. + def otp_authentication + devise_parameter_sanitizer.permit(:sign_in, keys: [:otp_attempt]) + + if sign_in_params[:email] + show_otp_code_form + elsif session[:otp_user_id] + validate_otp_code + end + end + + def show_otp_code_form + # Reset the session if the user is trying to login again. + session.delete(:otp_user_id) + + self.resource = User.find_by(email: sign_in_params[:email]) + if resource.valid_password?(sign_in_params[:password]) && resource.otp_required_for_login? + session[:otp_user_id] = resource.id + render :otp, status: :unprocessable_entity + end + end + + def validate_otp_code + self.resource = User.find(session[:otp_user_id]) + if resource.validate_and_consume_otp!(sign_in_params[:otp_attempt]) + + set_flash_message!(:notice, :signed_in) + sign_in(resource_name, resource) + redirect_to after_sign_in_path_for(resource) + else + resource.errors.add(:otp_attempt, :invalid) + render :otp, status: :unprocessable_entity + end + end end diff --git a/app/models/user.rb b/app/models/user.rb index 69496f6..3ffc210 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,14 +1,18 @@ class User < ApplicationRecord - # Include default devise modules. - devise :registerable, :recoverable, :rememberable, :validatable, :confirmable, :lockable, :timeoutable, :trackable, :two_factor_authenticatable # , :omniauthable + # Default devise modules. + devise :registerable, :recoverable, :rememberable, :validatable, :confirmable, :lockable, :timeoutable, :trackable - # Access the users personal account. + # Enable two-factor authentication. + devise :two_factor_authenticatable + + # User belongs to a personal account. belongs_to :personal_account, class_name: "Account", optional: true, dependent: :destroy + validates_associated :personal_account + + # User can have many team accounts. has_many :account_users, dependent: :destroy has_many :accounts, through: :account_users - validates_associated :personal_account - def onboarded? personal_account.present? && personal_account.persisted? end diff --git a/app/views/users/sessions/new.html.erb b/app/views/users/sessions/new.html.erb index dcda14e..5b87404 100644 --- a/app/views/users/sessions/new.html.erb +++ b/app/views/users/sessions/new.html.erb @@ -9,6 +9,7 @@ <%= simple_form_for(resource, as: resource_name, url: session_path(resource_name)) do |f| %> <%= f.input :email, placeholder: t(".form.email.placeholder"), required: true, autofocus: true, input_html: { autocomplete: "email" } %> <%= f.input :password, placeholder: t(".form.password.placeholder"), required: true, input_html: { autocomplete: "current-password" } %> + <%= f.input :remember_me, label: t(".form.remember_me.label"), as: :boolean, wrapper: :inline_checkbox if devise_mapping.rememberable? %>
diff --git a/app/views/users/sessions/otp.html.erb b/app/views/users/sessions/otp.html.erb new file mode 100644 index 0000000..3f9ef6d --- /dev/null +++ b/app/views/users/sessions/otp.html.erb @@ -0,0 +1,18 @@ +<% title "Two factor authentication" %> + +<%= fullscreen do |screen| %> + <%= screen.with_form( + title: t(".heading.title", app_name: Kiqr::Config.app_name), + description: t(".heading.description") + ) do %> +
+ <%= simple_form_for(resource, as: resource_name, url: new_user_session_path, method: :post) do |f| %> + <%= f.input :otp_attempt, label: t(".form.otp.label"), placeholder: t(".form.otp.placeholder") %> + +
+ <%= f.button :submit, t(".form.submit_button") %> +
+ <% end %> +
+ <% end %> +<% end %> diff --git a/config/credentials/test.key b/config/credentials/test.key new file mode 100644 index 0000000..6f003ab --- /dev/null +++ b/config/credentials/test.key @@ -0,0 +1 @@ +aecc9f0ffc88c629a51ee97520c76963 \ No newline at end of file diff --git a/config/credentials/test.yml.enc b/config/credentials/test.yml.enc new file mode 100644 index 0000000..0526e49 --- /dev/null +++ b/config/credentials/test.yml.enc @@ -0,0 +1 @@ +nl6YcBTnI/xYrRDaLXfnCNI8HV9AhUXELHeFfAZ7A7StirCmjsNU1RN64n0XHs9nwaANuWrVpuEmyw1nSkpKBkkaOTY/OFNIjua7gULz43Z0u+yuSxNDXVD1REVWmtKfAioQUWy564MbLNDcbic8zlJJsv3zgEN5cNe6/nLBDov8aG9LFyJg948ZO5QBj2N41dZl5MjesyPIPfFpYtzEwNdrGlHxYtGmmAcl9nRIMBAN6T+26RAkQoxIDPwOk+N384Wxisc5AvenRj+GCLgCZoC9pbNdWYnnZck/yxB8/rA9slCdmTrPxsFTdP2O8ho6LLe23HthKzqVkEwgNXUTiDHeGV76Z83u27tNBclGjxuCtE8GZZsNmq8onexj2PLxy8VlZ4iQhxPu9k7cPEm7t+te476FvSWwpSJUyq3H1+L7OOdMhhVYEEjie8Ue+vmifiZKbxDGMBIEi9/tY3yNmLdHBT/JnXr3AegJIS5+UHh4J5Cl8naTWYrXU2E7/o8x8Hl2Rr4uWXSpuKPIdd0cqzayITl+mDrXfEH0OE/n4WcMYWn5fAnf90AfFgSZ32+WufkisKl7E1cWWDOFpTh+16bz1v7clAazhv9oKmCOOko=--MHfJClNqfFA2siBd--fWeUh7gKNWGNnxnxulvNZQ== \ No newline at end of file diff --git a/config/initializers/filter_parameter_logging.rb b/config/initializers/filter_parameter_logging.rb index c2d89e2..3cf4e88 100644 --- a/config/initializers/filter_parameter_logging.rb +++ b/config/initializers/filter_parameter_logging.rb @@ -4,5 +4,5 @@ # Use this to limit dissemination of sensitive information. # See the ActiveSupport::ParameterFilter documentation for supported notations and behaviors. Rails.application.config.filter_parameters += [ - :passw, :secret, :token, :_key, :crypt, :salt, :certificate, :otp, :ssn + :passw, :secret, :token, :_key, :crypt, :salt, :certificate, :otp, :ssn, :otp_attempt ] diff --git a/config/locales/kiqr.en.yml b/config/locales/kiqr.en.yml index 8b3a8d6..07de1a1 100644 --- a/config/locales/kiqr.en.yml +++ b/config/locales/kiqr.en.yml @@ -47,6 +47,15 @@ en: submit_button: "Sign in" no_account: "Don't have an account?" sign_up: "Sign up!" + otp: + heading: + title: "Two-factor authentication" + description: "Enter the code from your authenticator app." + form: + otp: + label: "One-time password" + placeholder: "Enter the code" + submit_button: "Verify" registrations: new: heading: diff --git a/lib/templates/rails/credentials/credentials.yml.tt b/lib/templates/rails/credentials/credentials.yml.tt new file mode 100644 index 0000000..9097a04 --- /dev/null +++ b/lib/templates/rails/credentials/credentials.yml.tt @@ -0,0 +1,13 @@ +<% unless options.skip_secret_key_base? -%> +# Used as the base secret for all MessageVerifiers in Rails, including the one protecting cookies. +secret_key_base: <%= secret_key_base %> + +<% end -%> +# The new encryption features in Active Record. This will enable the new `encrypts` method to encrypt attributes. +# This is a required configuration if you want to enable two factor authentication. +# +# Read more: https://guides.rubyonrails.org/active_record_encryption.html +active_record_encryption: + primary_key: <%= SecureRandom.alphanumeric(32) %> + deterministic_key: <%= SecureRandom.alphanumeric(32) %> + key_derivation_salt: <%= SecureRandom.alphanumeric(32) %> diff --git a/test/controllers/users/sessions_controller_test.rb b/test/controllers/users/sessions_controller_test.rb new file mode 100644 index 0000000..89a8b15 --- /dev/null +++ b/test/controllers/users/sessions_controller_test.rb @@ -0,0 +1,17 @@ +require "test_helper" + +class Users::SessionsControllerTest < ActionDispatch::IntegrationTest + test "require otp if otp is enabled" do + user = create(:user, :otp_enabled) + post user_session_path, params: {user: {email: user.email, password: user.password}} + assert_response :unprocessable_entity + assert_template "users/sessions/otp" + end + + test "can sign in if otp is disabled" do + user = create(:user) + post user_session_path, params: {user: {email: user.email, password: user.password}} + assert_response :redirect + assert_redirected_to dashboard_path + end +end diff --git a/test/factories/user.rb b/test/factories/user.rb index 74c8314..0eb428a 100644 --- a/test/factories/user.rb +++ b/test/factories/user.rb @@ -9,5 +9,10 @@ trait :unconfirmed do confirmed_at { nil } end + + trait :otp_enabled do + otp_required_for_login { true } + otp_secret { User.generate_otp_secret } + end end end diff --git a/test/system/signin_test.rb b/test/system/signin_test.rb index b7c0627..aa7046e 100644 --- a/test/system/signin_test.rb +++ b/test/system/signin_test.rb @@ -12,6 +12,20 @@ class SigninTest < ApplicationSystemTestCase assert_current_path dashboard_path end + test "signs in with otp code" do + user = create(:user, :otp_enabled) + + visit new_user_session_path + fill_in "user[email]", with: user.email + fill_in "user[password]", with: user.password + click_on "commit" + + fill_in "user[otp_attempt]", with: user.current_otp + click_on "commit" + + assert_current_path dashboard_path + end + test "displays unconfirmed email message" do unconfirmed_user = create(:user, :unconfirmed)