From 842e37658c0e4a0e014a1ae3511840b9c93044f9 Mon Sep 17 00:00:00 2001 From: Josh Pigford Date: Thu, 6 Feb 2025 14:16:53 -0600 Subject: [PATCH] Multi-factor authentication (#1817) * Initial pass * Tests for MFA and locale cleanup * Brakeman * Update two-factor authentication status styling * Update app/models/user.rb Co-authored-by: Zach Gollwitzer Signed-off-by: Josh Pigford * Refactor MFA verification and session handling in tests --------- Signed-off-by: Josh Pigford Co-authored-by: Zach Gollwitzer --- Gemfile | 2 + Gemfile.lock | 8 ++ app/controllers/mfa_controller.rb | 53 ++++++++ app/controllers/sessions_controller.rb | 9 +- .../settings/securities_controller.rb | 4 + app/helpers/application_helper.rb | 4 + app/helpers/mfa_helper.rb | 20 +++ app/helpers/settings_helper.rb | 1 + app/models/user.rb | 57 +++++++++ app/views/mfa/backup_codes.html.erb | 29 +++++ app/views/mfa/new.html.erb | 45 +++++++ app/views/mfa/verify.html.erb | 14 +++ app/views/settings/_nav.html.erb | 3 + app/views/settings/securities/show.html.erb | 45 +++++++ config/locales/views/accounts/en.yml | 5 +- .../views/email_confirmation_mailer/en.yml | 11 +- config/locales/views/imports/en.yml | 8 +- config/locales/views/invitations/en.yml | 2 +- config/locales/views/mfa/en.yml | 35 ++++++ config/locales/views/settings/en.yml | 25 ++-- config/locales/views/settings/hostings/en.yml | 5 +- .../locales/views/settings/securities/en.yml | 12 ++ config/locales/views/users/en.yml | 7 +- config/routes.rb | 8 ++ .../20250206151825_add_mfa_fields_to_users.rb | 9 ++ db/schema.rb | 6 +- test/controllers/mfa_controller_test.rb | 117 ++++++++++++++++++ test/controllers/sessions_controller_test.rb | 19 ++- test/models/user_test.rb | 68 ++++++++++ 29 files changed, 598 insertions(+), 33 deletions(-) create mode 100644 app/controllers/mfa_controller.rb create mode 100644 app/controllers/settings/securities_controller.rb create mode 100644 app/helpers/mfa_helper.rb create mode 100644 app/views/mfa/backup_codes.html.erb create mode 100644 app/views/mfa/new.html.erb create mode 100644 app/views/mfa/verify.html.erb create mode 100644 app/views/settings/securities/show.html.erb create mode 100644 config/locales/views/mfa/en.yml create mode 100644 config/locales/views/settings/securities/en.yml create mode 100644 db/migrate/20250206151825_add_mfa_fields_to_users.rb create mode 100644 test/controllers/mfa_controller_test.rb diff --git a/Gemfile b/Gemfile index ff3c275f..11746759 100644 --- a/Gemfile +++ b/Gemfile @@ -55,6 +55,8 @@ gem "redcarpet" gem "stripe" gem "intercom-rails" gem "plaid" +gem "rotp", "~> 6.3" +gem "rqrcode", "~> 2.2" group :development, :test do gem "debug", platforms: %i[mri windows] diff --git a/Gemfile.lock b/Gemfile.lock index ff15a139..1a9b0209 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -138,6 +138,7 @@ GEM xpath (~> 3.2) childprocess (5.1.0) logger (~> 1.5) + chunky_png (1.4.0) climate_control (1.2.0) concurrent-ruby (1.3.5) connection_pool (2.5.0) @@ -397,6 +398,11 @@ GEM reline (0.6.0) io-console (~> 0.5) rexml (3.4.0) + rotp (6.3.0) + rqrcode (2.2.0) + chunky_png (~> 1.0) + rqrcode_core (~> 1.0) + rqrcode_core (1.2.0) rubocop (1.71.0) json (~> 2.3) language_server-protocol (>= 3.17.0) @@ -561,6 +567,8 @@ DEPENDENCIES rails (~> 7.2.2) rails-settings-cached redcarpet + rotp (~> 6.3) + rqrcode (~> 2.2) rubocop-rails-omakase ruby-lsp-rails selenium-webdriver diff --git a/app/controllers/mfa_controller.rb b/app/controllers/mfa_controller.rb new file mode 100644 index 00000000..f46fbb56 --- /dev/null +++ b/app/controllers/mfa_controller.rb @@ -0,0 +1,53 @@ +class MfaController < ApplicationController + layout :determine_layout + skip_authentication only: [ :verify, :verify_code ] + + def new + redirect_to root_path if Current.user.otp_required? + Current.user.setup_mfa! unless Current.user.otp_secret.present? + end + + def create + if Current.user.verify_otp?(params[:code]) + Current.user.enable_mfa! + @backup_codes = Current.user.otp_backup_codes + render :backup_codes + else + Current.user.disable_mfa! + redirect_to new_mfa_path, alert: t(".invalid_code") + end + end + + def verify + @user = User.find_by(id: session[:mfa_user_id]) + redirect_to new_session_path unless @user + end + + def verify_code + @user = User.find_by(id: session[:mfa_user_id]) + + if @user&.verify_otp?(params[:code]) + session.delete(:mfa_user_id) + @session = create_session_for(@user) + redirect_to root_path + else + flash.now[:alert] = t(".invalid_code") + render :verify, status: :unprocessable_entity + end + end + + def disable + Current.user.disable_mfa! + redirect_to settings_security_path, notice: t(".success") + end + + private + + def determine_layout + if action_name.in?(%w[verify verify_code]) + "auth" + else + "with_sidebar" + end + end +end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index a1fa08c2..3b7357f8 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -9,8 +9,13 @@ class SessionsController < ApplicationController def create if user = User.authenticate_by(email: params[:email], password: params[:password]) - @session = create_session_for(user) - redirect_to root_path + if user.otp_required? + session[:mfa_user_id] = user.id + redirect_to verify_mfa_path + else + @session = create_session_for(user) + redirect_to root_path + end else flash.now[:alert] = t(".invalid_credentials") render :new, status: :unprocessable_entity diff --git a/app/controllers/settings/securities_controller.rb b/app/controllers/settings/securities_controller.rb new file mode 100644 index 00000000..7f19d7ca --- /dev/null +++ b/app/controllers/settings/securities_controller.rb @@ -0,0 +1,4 @@ +class Settings::SecuritiesController < SettingsController + def show + end +end diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index ae9fd978..033014e9 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -19,6 +19,10 @@ module ApplicationHelper content_for(:header_title) { page_title } end + def header_description(page_description) + content_for(:header_description) { page_description } + end + def family_notifications_stream turbo_stream_from [ Current.family, :notifications ] if Current.family end diff --git a/app/helpers/mfa_helper.rb b/app/helpers/mfa_helper.rb new file mode 100644 index 00000000..074b36fd --- /dev/null +++ b/app/helpers/mfa_helper.rb @@ -0,0 +1,20 @@ +module MfaHelper + def generate_mfa_qr_code(provisioning_uri) + qr_code = RQRCode::QRCode.new(provisioning_uri).as_svg( + color: "141414", + module_size: 4, + standalone: true, + use_path: true, + svg_attributes: { + width: "228", + height: "228", + viewBox: "0 0 57 57" + } + ) + + # Whitelist specific SVG attributes and elements that we know are safe + sanitize qr_code, + tags: %w[svg g path rect], + attributes: %w[viewBox height width fill stroke stroke-width d x y class] + end +end diff --git a/app/helpers/settings_helper.rb b/app/helpers/settings_helper.rb index 4f4e0272..66488eff 100644 --- a/app/helpers/settings_helper.rb +++ b/app/helpers/settings_helper.rb @@ -3,6 +3,7 @@ module SettingsHelper { name: I18n.t("settings.nav.profile_label"), path: :settings_profile_path }, { name: I18n.t("settings.nav.preferences_label"), path: :settings_preferences_path }, { name: I18n.t("settings.nav.self_hosting_label"), path: :settings_hosting_path, condition: :self_hosted? }, + { name: I18n.t("settings.nav.security_label"), path: :settings_security_path }, { name: I18n.t("settings.nav.billing_label"), path: :settings_billing_path }, { name: I18n.t("settings.nav.accounts_label"), path: :accounts_path }, { name: I18n.t("settings.nav.imports_label"), path: :imports_path }, diff --git a/app/models/user.rb b/app/models/user.rb index f49b0d06..24932861 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -110,6 +110,41 @@ class User < ApplicationRecord end end + # MFA + def setup_mfa! + update!( + otp_secret: ROTP::Base32.random(32), + otp_required: false, + otp_backup_codes: [] + ) + end + + def enable_mfa! + update!( + otp_required: true, + otp_backup_codes: generate_backup_codes + ) + end + + def disable_mfa! + update!( + otp_secret: nil, + otp_required: false, + otp_backup_codes: [] + ) + end + + def verify_otp?(code) + return false if otp_secret.blank? + return true if verify_backup_code?(code) + totp.verify(code, drift_behind: 15) + end + + def provisioning_uri + return nil unless otp_secret.present? + totp.provisioning_uri(email) + end + private def ensure_valid_profile_image return unless profile_image.attached? @@ -133,4 +168,26 @@ class User < ApplicationRecord errors.add(:profile_image, :invalid_file_size, max_megabytes: 10) end end + + def totp + ROTP::TOTP.new(otp_secret, issuer: "Maybe Finance") + end + + def verify_backup_code?(code) + return false if otp_backup_codes.blank? + + # Find and remove the used backup code + if (index = otp_backup_codes.index(code)) + remaining_codes = otp_backup_codes.dup + remaining_codes.delete_at(index) + update_column(:otp_backup_codes, remaining_codes) + true + else + false + end + end + + def generate_backup_codes + 8.times.map { SecureRandom.hex(4) } + end end diff --git a/app/views/mfa/backup_codes.html.erb b/app/views/mfa/backup_codes.html.erb new file mode 100644 index 00000000..8adc5313 --- /dev/null +++ b/app/views/mfa/backup_codes.html.erb @@ -0,0 +1,29 @@ +<% + header_title t(".title") + header_description t(".description") +%> + +<% content_for :sidebar do %> + <%= render "settings/nav" %> +<% end %> + +
+

<%= t(".page_title") %>

+ <%= settings_section title: t(".backup_codes_title"), subtitle: t(".backup_codes_description") do %> +
+
+ <% @backup_codes.each do |code| %> +
+ <%= code %> +
+ <% end %> +
+ +
+ <%= link_to t(".continue"), settings_security_path, class: "w-full btn btn--primary" %> +
+
+ <% end %> + + <%= settings_nav_footer %> +
\ No newline at end of file diff --git a/app/views/mfa/new.html.erb b/app/views/mfa/new.html.erb new file mode 100644 index 00000000..c19024f5 --- /dev/null +++ b/app/views/mfa/new.html.erb @@ -0,0 +1,45 @@ +<% + header_title t(".title") + header_description t(".description") +%> + +<% content_for :sidebar do %> + <%= render "settings/nav" %> +<% end %> + +
+

<%= t(".page_title") %>

+ <%= settings_section title: t(".scan_title"), subtitle: t(".scan_description") do %> +
+
+ <%= generate_mfa_qr_code(Current.user.provisioning_uri) %> +
+ +
+

<%= t(".verify_title") %>

+
+

<%= t(".verify_description") %>

+
+
+ + <%= styled_form_with url: mfa_path, method: :post, class: "mt-5", data: { turbo: false } do |f| %> +
+ <%= f.text_field :code, + required: true, + autofocus: true, + autocomplete: "one-time-code", + inputmode: "numeric", + pattern: "[0-9]*", + label: t(".code_label"), + placeholder: t(".code_placeholder") %> + +
+ <%= f.submit t(".verify_button"), class: "bg-gray-900 hover:bg-gray-700 cursor-pointer text-white rounded-lg px-3 py-2" %> +
+
+ <% end %> +
+ <% end %> + + <%= settings_nav_footer %> +
diff --git a/app/views/mfa/verify.html.erb b/app/views/mfa/verify.html.erb new file mode 100644 index 00000000..a719a2ae --- /dev/null +++ b/app/views/mfa/verify.html.erb @@ -0,0 +1,14 @@ +<% + header_title t(".title") + header_description t(".description") +%> + +<%= styled_form_with url: verify_mfa_path, method: :post, class: "space-y-4" do |form| %> + <%= form.text_field :code, + required: true, + autofocus: true, + autocomplete: "one-time-code", + label: t(".page_title") %> + + <%= form.submit t(".verify_button") %> +<% end %> \ No newline at end of file diff --git a/app/views/settings/_nav.html.erb b/app/views/settings/_nav.html.erb index cd321c1b..55e20d8d 100644 --- a/app/views/settings/_nav.html.erb +++ b/app/views/settings/_nav.html.erb @@ -21,6 +21,9 @@
  • <%= sidebar_link_to t(".preferences_label"), settings_preferences_path, icon: "bolt" %>
    • +
  • + <%= sidebar_link_to t(".security_label"), settings_security_path, icon: "shield-check" %> +
    • <% if self_hosted? %>
  • <%= sidebar_link_to t(".self_hosting_label"), settings_hosting_path, icon: "database" %> diff --git a/app/views/settings/securities/show.html.erb b/app/views/settings/securities/show.html.erb new file mode 100644 index 00000000..9dbee9a2 --- /dev/null +++ b/app/views/settings/securities/show.html.erb @@ -0,0 +1,45 @@ +<% content_for :sidebar do %> + <%= render "settings/nav" %> +<% end %> + +
    +

    <%= t(".page_title") %>

    + <%= settings_section title: t(".mfa_title"), subtitle: t(".mfa_description") do %> +
    +
    +
    +
    + <%= lucide_icon "shield-check", class: "w-5 h-5 text-gray-500" %> +
    + +
    + <% if Current.user.otp_required? %> +

    Two-factor authentication is enabled

    +

    Your account is protected with an additional layer of security.

    + <% else %> +

    Two-factor authentication is disabled

    +

    Enable 2FA to add an extra layer of security to your account.

    + <% end %> +
    +
    + + <% if Current.user.otp_required? %> + <%= button_to t(".disable_mfa"), disable_mfa_path, + method: :delete, + class: "btn btn--secondary flex items-center gap-1", + data: { turbo_confirm: { + title: t(".disable_mfa_confirm"), + body: t(".disable_mfa_confirm"), + accept: t(".disable_mfa"), + acceptClass: "w-full bg-red-500 text-white rounded-xl text-center p-[10px] border mb-2" + } } %> + <% else %> + <%= link_to t(".enable_mfa"), new_mfa_path, + class: "btn btn--primary flex items-center gap-1" %> + <% end %> +
    +
    + <% end %> + + <%= settings_nav_footer %> +
    \ No newline at end of file diff --git a/config/locales/views/accounts/en.yml b/config/locales/views/accounts/en.yml index 61029610..d13f8c79 100644 --- a/config/locales/views/accounts/en.yml +++ b/config/locales/views/accounts/en.yml @@ -76,5 +76,6 @@ en: success: "%{type} account updated" email_confirmations: new: - success_login: "Your email has been confirmed. Please log in with your new email address." - invalid_token: "Invalid or expired confirmation link." + invalid_token: Invalid or expired confirmation link. + success_login: Your email has been confirmed. Please log in with your new email + address. diff --git a/config/locales/views/email_confirmation_mailer/en.yml b/config/locales/views/email_confirmation_mailer/en.yml index 110746a8..e0caba08 100644 --- a/config/locales/views/email_confirmation_mailer/en.yml +++ b/config/locales/views/email_confirmation_mailer/en.yml @@ -2,8 +2,9 @@ en: email_confirmation_mailer: confirmation_email: - subject: "Maybe: Confirm your email change" - greeting: "Hello!" - body: "You recently requested to change your email address. Click the button below to confirm this change." - cta: "Confirm email change" - expiry_notice: "This link will expire in %{hours} hours." \ No newline at end of file + body: You recently requested to change your email address. Click the button + below to confirm this change. + cta: Confirm email change + expiry_notice: This link will expire in %{hours} hours. + greeting: Hello! + subject: 'Maybe: Confirm your email change' diff --git a/config/locales/views/imports/en.yml b/config/locales/views/imports/en.yml index 2ed7c1ff..77bbf059 100644 --- a/config/locales/views/imports/en.yml +++ b/config/locales/views/imports/en.yml @@ -8,15 +8,15 @@ en: details. title: Clean your data configurations: - trade_import: - date_format_label: Date format mint_import: date_format_label: Date format - transaction_import: - date_format_label: Date format show: description: Select the columns that correspond to each field in your CSV. title: Configure your import + trade_import: + date_format_label: Date format + transaction_import: + date_format_label: Date format confirms: mappings: create_account: Create account diff --git a/config/locales/views/invitations/en.yml b/config/locales/views/invitations/en.yml index db862831..894b9823 100644 --- a/config/locales/views/invitations/en.yml +++ b/config/locales/views/invitations/en.yml @@ -5,9 +5,9 @@ en: failure: Could not send invitation success: Invitation sent successfully destroy: + failure: There was a problem removing the invitation. not_authorized: You are not authorized to manage invitations. success: Invitation was successfully removed. - failure: There was a problem removing the invitation. new: email_label: Email Address email_placeholder: Enter email address diff --git a/config/locales/views/mfa/en.yml b/config/locales/views/mfa/en.yml new file mode 100644 index 00000000..6db9f97f --- /dev/null +++ b/config/locales/views/mfa/en.yml @@ -0,0 +1,35 @@ +--- +en: + mfa: + backup_codes: + backup_codes_description: Each code can only be used once. Keep these codes + safe and secure. + backup_codes_title: Your Backup Codes + continue: Continue to Security Settings + description: Store these backup codes in a safe place - you'll need them if + you lose access to your authenticator app + page_title: Backup Codes + title: Save Your Backup Codes + create: + invalid_code: Invalid verification code. Please try again. + disable: + success: Two-factor authentication has been disabled + new: + code_label: Verification Code + code_placeholder: Enter 6-digit code + description: Enhance your account security by setting up two-factor authentication + page_title: Two-Factor Authentication Setup + scan_description: Use an authenticator app like Google Authenticator or 1Password + to scan this QR code + scan_title: 1. Scan QR Code + title: Set Up Two-Factor Authentication + verify_button: Verify and Enable 2FA + verify_description: Enter the 6-digit code from your authenticator app + verify_title: 2. Enter Verification Code + verify: + description: Enter the code from your authenticator app to continue + page_title: Verify Two-Factor Authentication + title: Two-Factor Authentication + verify_button: Verify + verify_code: + invalid_code: Invalid authentication code. Please try again. diff --git a/config/locales/views/settings/en.yml b/config/locales/views/settings/en.yml index c082139c..41d8e10e 100644 --- a/config/locales/views/settings/en.yml +++ b/config/locales/views/settings/en.yml @@ -18,6 +18,7 @@ en: other_section_title: More preferences_label: Preferences profile_label: Account + security_label: Security self_hosting_label: Self hosting tags_label: Tags transactions_section_title: Transactions @@ -49,27 +50,26 @@ en: timezone: Timezone profiles: destroy: - not_authorized: You are not authorized to remove members. cannot_remove_self: You cannot remove yourself from the account. - member_removed: Member was successfully removed. member_removal_failed: There was a problem removing the member. + member_removed: Member was successfully removed. + not_authorized: You are not authorized to remove members. show: confirm_delete: body: Are you sure you want to permanently delete your account? This action is irreversible. title: Delete account? - confirm_remove_member: - title: Remove Member - body: Are you sure you want to remove %{name} from your account? - remove_member: Remove Member confirm_remove_invitation: - title: Remove Invitation body: Are you sure you want to remove the invitation for %{email}? - remove_invitation: Remove Invitation + title: Remove Invitation + confirm_remove_member: + body: Are you sure you want to remove %{name} from your account? + title: Remove Member danger_zone_title: Danger Zone delete_account: Delete account delete_account_warning: Deleting your account will permanently remove all your data and cannot be undone. + email: Email first_name: First Name household_form_input_placeholder: Enter household name household_form_label: Household name @@ -79,15 +79,16 @@ en: invitation_link: Invitation link invite_member: Add member last_name: Last Name - email: Email page_title: Account pending: Pending profile_subtitle: Customize how you appear on Maybe profile_title: Profile + remove_invitation: Remove Invitation + remove_member: Remove Member save: Save + securities: + show: + page_title: Security user_avatar_field: accepted_formats: JPG or PNG. 5MB max. choose: Choose - users: - update: - success: Profile updated successfully diff --git a/config/locales/views/settings/hostings/en.yml b/config/locales/views/settings/hostings/en.yml index 06334a31..8cb7f6c4 100644 --- a/config/locales/views/settings/hostings/en.yml +++ b/config/locales/views/settings/hostings/en.yml @@ -5,11 +5,12 @@ en: invite_code_settings: description: Every new user that joins your instance of Maybe can only do so via an invite code + email_confirmation_description: When enabled, users must confirm their email + address when changing it. + email_confirmation_title: Require email confirmation generate_tokens: Generate new code generated_tokens: Generated codes title: Require invite code for signup - email_confirmation_title: Require email confirmation - email_confirmation_description: When enabled, users must confirm their email address when changing it. provider_settings: description: Configure settings for your hosting provider render_deploy_hook_label: Render Deploy Hook URL diff --git a/config/locales/views/settings/securities/en.yml b/config/locales/views/settings/securities/en.yml new file mode 100644 index 00000000..af439522 --- /dev/null +++ b/config/locales/views/settings/securities/en.yml @@ -0,0 +1,12 @@ +--- +en: + settings: + securities: + show: + disable_mfa: Disable 2FA + disable_mfa_confirm: Are you sure you want to disable two-factor authentication? + This will make your account less secure. + enable_mfa: Enable 2FA + mfa_description: Add an extra layer of security to your account by requiring + a code from your authenticator app when signing in + mfa_title: Two-Factor Authentication diff --git a/config/locales/views/users/en.yml b/config/locales/views/users/en.yml index 5eaf6f63..e9aae045 100644 --- a/config/locales/views/users/en.yml +++ b/config/locales/views/users/en.yml @@ -4,6 +4,7 @@ en: destroy: success: Your account has been deleted. update: - success: "Your profile has been updated." - email_change_initiated: "Please check your new email address for confirmation instructions." - email_change_failed: "Failed to change email address." + email_change_failed: Failed to change email address. + email_change_initiated: Please check your new email address for confirmation + instructions. + success: Your profile has been updated. diff --git a/config/routes.rb b/config/routes.rb index 33d534a2..bf6eb972 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,4 +1,11 @@ Rails.application.routes.draw do + # MFA routes + resource :mfa, controller: "mfa", only: [ :new, :create ] do + get :verify + post :verify, to: "mfa#verify_code" + delete :disable + end + mount GoodJob::Engine => "good_job" get "changelog", to: "pages#changelog" @@ -25,6 +32,7 @@ Rails.application.routes.draw do resource :preferences, only: :show resource :hosting, only: %i[show update] resource :billing, only: :show + resource :security, only: :show end resource :subscription, only: %i[new show] do diff --git a/db/migrate/20250206151825_add_mfa_fields_to_users.rb b/db/migrate/20250206151825_add_mfa_fields_to_users.rb new file mode 100644 index 00000000..d906a301 --- /dev/null +++ b/db/migrate/20250206151825_add_mfa_fields_to_users.rb @@ -0,0 +1,9 @@ +class AddMfaFieldsToUsers < ActiveRecord::Migration[7.2] + def change + add_column :users, :otp_secret, :string + add_column :users, :otp_required, :boolean, default: false, null: false + add_column :users, :otp_backup_codes, :string, array: true, default: [] + + add_index :users, :otp_secret, unique: true, where: "otp_secret IS NOT NULL" + end +end diff --git a/db/schema.rb b/db/schema.rb index 89e53b87..056caf8c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_02_06_141452) do +ActiveRecord::Schema[7.2].define(version: 2025_02_06_151825) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -666,8 +666,12 @@ ActiveRecord::Schema[7.2].define(version: 2025_02_06_141452) do t.boolean "active", default: true, null: false t.datetime "onboarded_at" t.string "unconfirmed_email" + t.string "otp_secret" + t.boolean "otp_required", default: false, null: false + t.string "otp_backup_codes", default: [], array: true t.index ["email"], name: "index_users_on_email", unique: true t.index ["family_id"], name: "index_users_on_family_id" + t.index ["otp_secret"], name: "index_users_on_otp_secret", unique: true, where: "(otp_secret IS NOT NULL)" end create_table "vehicles", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| diff --git a/test/controllers/mfa_controller_test.rb b/test/controllers/mfa_controller_test.rb new file mode 100644 index 00000000..28b52dee --- /dev/null +++ b/test/controllers/mfa_controller_test.rb @@ -0,0 +1,117 @@ +require "test_helper" + +class MfaControllerTest < ActionDispatch::IntegrationTest + setup do + @user = users(:family_member) + sign_in @user + end + + def sign_out + delete session_path(@user.sessions.last) if @user.sessions.any? + end + + test "redirects to root if MFA already enabled" do + @user.setup_mfa! + @user.enable_mfa! + + get new_mfa_path + assert_redirected_to root_path + end + + test "sets up MFA when visiting new" do + get new_mfa_path + + assert_response :success + assert @user.reload.otp_secret.present? + assert_not @user.otp_required? + assert_select "svg" # QR code should be present + end + + test "enables MFA with valid code" do + @user.setup_mfa! + totp = ROTP::TOTP.new(@user.otp_secret, issuer: "Maybe") + + post mfa_path, params: { code: totp.now } + + assert_response :success + assert @user.reload.otp_required? + assert_equal 8, @user.otp_backup_codes.length + assert_select "div.grid-cols-2" # Check for backup codes grid + end + + test "does not enable MFA with invalid code" do + @user.setup_mfa! + + post mfa_path, params: { code: "invalid" } + + assert_redirected_to new_mfa_path + assert_not @user.reload.otp_required? + assert_empty @user.otp_backup_codes + end + + test "verify shows MFA verification page" do + @user.setup_mfa! + @user.enable_mfa! + sign_out + + post sessions_path, params: { email: @user.email, password: "password" } + assert_redirected_to verify_mfa_path + + get verify_mfa_path + assert_response :success + assert_select "form[action=?]", verify_mfa_path + end + + test "verify_code authenticates with valid TOTP" do + @user.setup_mfa! + @user.enable_mfa! + sign_out + + post sessions_path, params: { email: @user.email, password: "password" } + totp = ROTP::TOTP.new(@user.otp_secret, issuer: "Maybe") + + post verify_mfa_path, params: { code: totp.now } + + assert_redirected_to root_path + assert Session.exists?(user_id: @user.id) + end + + test "verify_code authenticates with valid backup code" do + @user.setup_mfa! + @user.enable_mfa! + sign_out + + post sessions_path, params: { email: @user.email, password: "password" } + backup_code = @user.otp_backup_codes.first + + post verify_mfa_path, params: { code: backup_code } + + assert_redirected_to root_path + assert Session.exists?(user_id: @user.id) + assert_not @user.reload.otp_backup_codes.include?(backup_code) + end + + test "verify_code rejects invalid codes" do + @user.setup_mfa! + @user.enable_mfa! + sign_out + + post sessions_path, params: { email: @user.email, password: "password" } + post verify_mfa_path, params: { code: "invalid" } + + assert_response :unprocessable_entity + assert_not Session.exists?(user_id: @user.id) + end + + test "disable removes MFA" do + @user.setup_mfa! + @user.enable_mfa! + + delete disable_mfa_path + + assert_redirected_to settings_security_path + assert_not @user.reload.otp_required? + assert_nil @user.otp_secret + assert_empty @user.otp_backup_codes + end +end diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index a7c04b5c..1ea37ca7 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -13,6 +13,7 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest test "can sign in" do sign_in @user assert_redirected_to root_url + assert Session.exists?(user_id: @user.id) get root_url assert_response :success @@ -26,10 +27,14 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest test "can sign out" do sign_in @user + session_record = @user.sessions.last - delete session_url(@user.sessions.order(:created_at).last) + delete session_url(session_record) assert_redirected_to new_session_path assert_equal "You have signed out successfully.", flash[:notice] + + # Verify session is destroyed + assert_nil Session.find_by(id: session_record.id) end test "super admins can access the jobs page" do @@ -42,4 +47,16 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest get good_job_url assert_response :not_found end + + test "redirects to MFA verification when MFA enabled" do + @user.setup_mfa! + @user.enable_mfa! + @user.sessions.destroy_all # Clean up any existing sessions + + post sessions_path, params: { email: @user.email, password: "password" } + + assert_redirected_to verify_mfa_path + assert_equal @user.id, session[:mfa_user_id] + assert_not Session.exists?(user_id: @user.id) + end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index dde2c44e..85e3c010 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -70,4 +70,72 @@ class UserTest < ActiveSupport::TestCase assert_equal "Bob", @user.first_name assert_equal "Dylan", @user.last_name end + + # MFA Tests + test "setup_mfa! generates required fields" do + user = users(:family_member) + user.setup_mfa! + + assert user.otp_secret.present? + assert_not user.otp_required? + assert_empty user.otp_backup_codes + end + + test "enable_mfa! enables MFA and generates backup codes" do + user = users(:family_member) + user.setup_mfa! + user.enable_mfa! + + assert user.otp_required? + assert_equal 8, user.otp_backup_codes.length + assert user.otp_backup_codes.all? { |code| code.length == 8 } + end + + test "disable_mfa! removes all MFA data" do + user = users(:family_member) + user.setup_mfa! + user.enable_mfa! + user.disable_mfa! + + assert_nil user.otp_secret + assert_not user.otp_required? + assert_empty user.otp_backup_codes + end + + test "verify_otp? validates TOTP codes" do + user = users(:family_member) + user.setup_mfa! + + totp = ROTP::TOTP.new(user.otp_secret, issuer: "Maybe") + valid_code = totp.now + + assert user.verify_otp?(valid_code) + assert_not user.verify_otp?("invalid") + assert_not user.verify_otp?("123456") + end + + test "verify_otp? accepts backup codes" do + user = users(:family_member) + user.setup_mfa! + user.enable_mfa! + + backup_code = user.otp_backup_codes.first + assert user.verify_otp?(backup_code) + + # Backup code should be consumed + assert_not user.otp_backup_codes.include?(backup_code) + assert_equal 7, user.otp_backup_codes.length + + # Used backup code should not work again + assert_not user.verify_otp?(backup_code) + end + + test "provisioning_uri generates correct URI" do + user = users(:family_member) + user.setup_mfa! + + assert_match %r{otpauth://totp/}, user.provisioning_uri + assert_match %r{secret=#{user.otp_secret}}, user.provisioning_uri + assert_match %r{issuer=Maybe}, user.provisioning_uri + end end