diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 5b4ca6b5..a483617e 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -10,6 +10,7 @@ def current_user = nil # TODO: this is a temp hack to fix partials until /backen helper_method :detected_country_alpha2 before_action :invalidate_v1_sessions, :authenticate_identity!, :set_honeybadger_context + before_action :require_two_factor_enrollment! before_action :set_paper_trail_whodunnit before_action :touch_session_last_seen_at @@ -44,6 +45,19 @@ def authenticate_identity! end end + def require_two_factor_enrollment! + return unless identity_signed_in? + return unless current_identity.two_factor_enrollment_required? + + flash[:error] = "Your account requires two-factor authentication. Set up an authenticator app or passkey to continue." + if request.headers["HX-Request"] + response.headers["HX-Redirect"] = security_path + head :ok + else + redirect_to security_path + end + end + def set_honeybadger_context return unless current_identity diff --git a/app/controllers/authorized_applications_controller.rb b/app/controllers/authorized_applications_controller.rb index 085c6354..ccaeec13 100644 --- a/app/controllers/authorized_applications_controller.rb +++ b/app/controllers/authorized_applications_controller.rb @@ -1,6 +1,10 @@ class AuthorizedApplicationsController < ApplicationController include AhoyAnalytics + # Rendered as a panel on the security page, which is the enrollment gate's + # destination — and revoking app access is safe for un-enrolled users. + skip_before_action :require_two_factor_enrollment! + def index @access_tokens = current_identity.access_tokens .includes(:application) diff --git a/app/controllers/backend/identities_controller.rb b/app/controllers/backend/identities_controller.rb index 562b9784..65d31736 100644 --- a/app/controllers/backend/identities_controller.rb +++ b/app/controllers/backend/identities_controller.rb @@ -279,7 +279,7 @@ def set_identity end def identity_params - permitted = [ :first_name, :last_name, :legal_first_name, :legal_last_name, :primary_email, :phone_number, :birthday, :country, :hq_override, :ysws_eligible, :permabanned, :disallow_slack ] + permitted = [ :first_name, :last_name, :legal_first_name, :legal_last_name, :primary_email, :phone_number, :birthday, :country, :hq_override, :ysws_eligible, :permabanned, :disallow_slack, :two_factor_required ] permitted << :can_hq_officialize if current_user&.super_admin? params.require(:identity).permit(permitted) end diff --git a/app/controllers/identities_controller.rb b/app/controllers/identities_controller.rb index 14b5dce0..bc141d27 100644 --- a/app/controllers/identities_controller.rb +++ b/app/controllers/identities_controller.rb @@ -238,6 +238,13 @@ def toggle_2fa return end + # Admin-required 2FA can't be disabled by the user + if current_identity.two_factor_required? + flash[:error] = "Two-factor authentication is required for your account and can't be disabled." + redirect_to security_path + return + end + # Disabling 2FA requires step-up auth redirect_to new_step_up_path(action_type: "disable_2fa") end diff --git a/app/controllers/identity_backup_codes_controller.rb b/app/controllers/identity_backup_codes_controller.rb index 0df60952..76228974 100644 --- a/app/controllers/identity_backup_codes_controller.rb +++ b/app/controllers/identity_backup_codes_controller.rb @@ -1,4 +1,6 @@ class IdentityBackupCodesController < ApplicationController + skip_before_action :require_two_factor_enrollment! + def index @backup_codes = current_identity.backup_codes.active.order(created_at: :desc) diff --git a/app/controllers/identity_sessions_controller.rb b/app/controllers/identity_sessions_controller.rb index e8fcb966..3923ca38 100644 --- a/app/controllers/identity_sessions_controller.rb +++ b/app/controllers/identity_sessions_controller.rb @@ -1,4 +1,6 @@ class IdentitySessionsController < ApplicationController + skip_before_action :require_two_factor_enrollment! + def index @sessions = current_identity.sessions .where(signed_out_at: nil) diff --git a/app/controllers/identity_totps_controller.rb b/app/controllers/identity_totps_controller.rb index 016ed64e..a7a7a5ee 100644 --- a/app/controllers/identity_totps_controller.rb +++ b/app/controllers/identity_totps_controller.rb @@ -1,4 +1,6 @@ class IdentityTotpsController < ApplicationController + skip_before_action :require_two_factor_enrollment! + def index @totp = current_identity.totp diff --git a/app/controllers/identity_webauthn_credentials_controller.rb b/app/controllers/identity_webauthn_credentials_controller.rb index 3bce81fe..efda7abb 100644 --- a/app/controllers/identity_webauthn_credentials_controller.rb +++ b/app/controllers/identity_webauthn_credentials_controller.rb @@ -1,6 +1,8 @@ class IdentityWebauthnCredentialsController < ApplicationController include WebauthnAuthenticatable + skip_before_action :require_two_factor_enrollment! + def index @webauthn_credentials = current_identity.webauthn_credentials.order(created_at: :desc) render layout: request.headers["HX-Request"] ? "htmx" : false diff --git a/app/controllers/logins_controller.rb b/app/controllers/logins_controller.rb index ae6b699f..6df9b0d7 100644 --- a/app/controllers/logins_controller.rb +++ b/app/controllers/logins_controller.rb @@ -8,6 +8,7 @@ class LoginsController < ApplicationController WEBAUTHN_SESSION_KEY = :webauthn_authentication_challenge skip_before_action :authenticate_identity! + skip_before_action :require_two_factor_enrollment! before_action :set_return_to, only: [ :new, :create ] before_action :set_attempt, except: [ :new, :create ] before_action :validate_browser_token, except: [ :new, :create ] diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 0f818c04..0a524963 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -1,4 +1,6 @@ class SessionsController < ApplicationController + skip_before_action :require_two_factor_enrollment! + def logout flash[:info] = "You've been logged out. Nice seeing you!" sign_out diff --git a/app/controllers/static_pages_controller.rb b/app/controllers/static_pages_controller.rb index a9cf38be..bb1ff069 100644 --- a/app/controllers/static_pages_controller.rb +++ b/app/controllers/static_pages_controller.rb @@ -1,5 +1,6 @@ class StaticPagesController < ApplicationController skip_before_action :authenticate_identity!, only: [ :external_api_docs, :welcome, :oauth_welcome ] + skip_before_action :require_two_factor_enrollment!, only: [ :security ] def home @sso_apps = SAMLService::Entities.service_providers.values.select do |sp| diff --git a/app/controllers/step_up_controller.rb b/app/controllers/step_up_controller.rb index 12715b1c..b19fb2a7 100644 --- a/app/controllers/step_up_controller.rb +++ b/app/controllers/step_up_controller.rb @@ -1,6 +1,8 @@ class StepUpController < ApplicationController include WebauthnAuthenticatable + skip_before_action :require_two_factor_enrollment! + helper_method :step_up_cancel_path WEBAUTHN_SESSION_KEY = :step_up_webauthn_challenge @@ -147,6 +149,11 @@ def complete_step_up(action_type, return_to) case action_type when "remove_totp" totp = current_identity.totp + if current_identity.two_factor_required? && (current_identity.two_factor_methods - [ totp ]).empty? + redirect_to security_path, alert: "Two-factor authentication is required for your account. Add another method before removing this one." + return + end + totp&.destroy TwoFactorMailer.authentication_method_disabled(current_identity).deliver_later @@ -159,6 +166,11 @@ def complete_step_up(action_type, return_to) redirect_to security_path, notice: "Two-factor authentication disabled" when "disable_2fa" + if current_identity.two_factor_required? + redirect_to security_path, alert: "Two-factor authentication is required for your account and can't be disabled." + return + end + current_identity.update!(use_two_factor_authentication: false) TwoFactorMailer.required_authentication_disabled(current_identity).deliver_later consume_step_up! @@ -175,6 +187,10 @@ def complete_step_up(action_type, return_to) when "remove_passkey" credential_id = session.delete(:pending_destroy_credential_id) credential = current_identity.webauthn_credentials.find_by(id: credential_id) if credential_id + if credential && current_identity.two_factor_required? && (current_identity.two_factor_methods - [ credential ]).empty? + redirect_to security_path, alert: "Two-factor authentication is required for your account. Add another method before removing this one." + return + end if credential credential.destroy consume_step_up! diff --git a/app/models/identity.rb b/app/models/identity.rb index ce714e3f..880a557c 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -370,9 +370,13 @@ def has_two_factor_method? = two_factor_methods.any? def primary_two_factor_method = two_factor_methods.first def requires_two_factor? - use_two_factor_authentication? && has_two_factor_method? + (use_two_factor_authentication? || two_factor_required?) && has_two_factor_method? end + # Admin-set override without any enrolled method: the user must enroll before + # they can use HCA (enforced by ApplicationController#require_two_factor_enrollment!) + def two_factor_enrollment_required? = two_factor_required? && !has_two_factor_method? + def legacy_migrated? = legacy_migrated_at.present? def suggested_aadhaar_password diff --git a/app/views/backend/identities/edit.html.erb b/app/views/backend/identities/edit.html.erb index a96741c2..526b46c5 100644 --- a/app/views/backend/identities/edit.html.erb +++ b/app/views/backend/identities/edit.html.erb @@ -42,6 +42,7 @@ + <% super_admin_tool do %> diff --git a/app/views/backend/identities/show.html.erb b/app/views/backend/identities/show.html.erb index b3bc2c74..2033c08b 100644 --- a/app/views/backend/identities/show.html.erb +++ b/app/views/backend/identities/show.html.erb @@ -134,7 +134,8 @@ <% if @identity.came_in_through_adult_program %>adult program<% end %> <% if @identity.developer_mode %>developer<% end %> <% if @identity.disallow_slack %>no slack<% end %> - <% unless @identity.hq_override || @identity.can_hq_officialize || @identity.is_alum || @identity.came_in_through_adult_program || @identity.developer_mode || @identity.disallow_slack %> + <% if @identity.two_factor_required %>2fa required<%= " — not enrolled" unless @identity.has_two_factor_method? %><% end %> + <% unless @identity.hq_override || @identity.can_hq_officialize || @identity.is_alum || @identity.came_in_through_adult_program || @identity.developer_mode || @identity.disallow_slack || @identity.two_factor_required %> none <% end %> diff --git a/app/views/identity_totps/index.html.erb b/app/views/identity_totps/index.html.erb index 12d3ef06..78582c11 100644 --- a/app/views/identity_totps/index.html.erb +++ b/app/views/identity_totps/index.html.erb @@ -18,7 +18,9 @@ <%= t(".require_2fa_title") %>

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

- <% if current_identity.use_two_factor_authentication? %> + <% if current_identity.two_factor_required? %> + <%= t(".required_by_hq") %> + <% elsif current_identity.use_two_factor_authentication? %> <%= button_to t(".disable_requirement"), toggle_2fa_identity_path, method: :post, diff --git a/app/views/static_pages/security.html.erb b/app/views/static_pages/security.html.erb index 50ba0e50..d663862c 100644 --- a/app/views/static_pages/security.html.erb +++ b/app/views/static_pages/security.html.erb @@ -5,6 +5,13 @@
+ <% if current_identity.two_factor_enrollment_required? %> +
+

<%= t ".enrollment_required_heading" %>

+

<%= t ".enrollment_required_description" %>

+
+ <% end %> +

<%= t ".sessions" %>

<%= render Components::BootlegTurbo.new(identity_sessions_path, id: "sessions-container", hx_swap: "innerHTML") %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 188d18f4..bb4e6c87 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -479,6 +479,8 @@ en: security_link: Security security: heading: Security + enrollment_required_heading: Two-factor authentication required + enrollment_required_description: Your account requires two-factor authentication. Set up an authenticator app or passkey below to continue using Hack Club Auth. sessions: Active Sessions mfa: Authenticator App webauthn: Passkeys @@ -514,6 +516,7 @@ en: require_2fa_description: When enabled, you must use a second factor (authenticator app or backup code) when signing in with an email code. disable_requirement: Disable Requirement enable_requirement: Enable Requirement + required_by_hq: Required by Hack Club HQ — this can't be disabled. setup_description: Add an extra layer of security to email sign-ins by requiring a code from an authenticator app. setup_authenticator: Set up two-factor authentication show: diff --git a/db/migrate/20260703000002_add_two_factor_required_to_identities.rb b/db/migrate/20260703000002_add_two_factor_required_to_identities.rb new file mode 100644 index 00000000..c8e4f741 --- /dev/null +++ b/db/migrate/20260703000002_add_two_factor_required_to_identities.rb @@ -0,0 +1,5 @@ +class AddTwoFactorRequiredToIdentities < ActiveRecord::Migration[8.0] + def change + add_column :identities, :two_factor_required, :boolean, default: false, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 45336f93..e714716d 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[8.0].define(version: 2026_06_22_000001) do +ActiveRecord::Schema[8.0].define(version: 2026_07_03_000002) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pg_trgm" @@ -318,6 +318,7 @@ t.boolean "can_hq_officialize", default: false, null: false t.string "persona_account_id" t.boolean "disallow_slack", default: false, null: false + t.boolean "two_factor_required", default: false, null: false t.index "lower((primary_email)::text)", name: "idx_identities_unique_primary_email", unique: true, where: "(deleted_at IS NULL)" t.index ["aadhaar_number_bidx"], name: "index_identities_on_aadhaar_number_bidx", unique: true t.index ["deleted_at"], name: "index_identities_on_deleted_at" diff --git a/spec/models/identity_spec.rb b/spec/models/identity_spec.rb index 8c809683..c67ec400 100644 --- a/spec/models/identity_spec.rb +++ b/spec/models/identity_spec.rb @@ -88,4 +88,49 @@ expect(other).not_to be_valid end end + + describe "#requires_two_factor?" do + it "is false by default" do + expect(identity.requires_two_factor?).to be false + end + + it "is true when the user enabled 2FA and has a method" do + identity.update!(use_two_factor_authentication: true) + identity.totps.create!.mark_verified! + + expect(identity.requires_two_factor?).to be true + end + + it "is true when the admin override is set and a method exists, even without the user flag" do + identity.update!(two_factor_required: true) + identity.totps.create!.mark_verified! + + expect(identity.requires_two_factor?).to be true + end + + it "is false when the admin override is set but no method exists" do + identity.update!(two_factor_required: true) + + expect(identity.requires_two_factor?).to be false + end + end + + describe "#two_factor_enrollment_required?" do + it "is true when the override is set and no method is enrolled" do + identity.update!(two_factor_required: true) + + expect(identity.two_factor_enrollment_required?).to be true + end + + it "is false once a method is enrolled" do + identity.update!(two_factor_required: true) + identity.totps.create!.mark_verified! + + expect(identity.two_factor_enrollment_required?).to be false + end + + it "is false without the override" do + expect(identity.two_factor_enrollment_required?).to be false + end + end end diff --git a/spec/requests/two_factor_enrollment_gate_spec.rb b/spec/requests/two_factor_enrollment_gate_spec.rb new file mode 100644 index 00000000..438e6ab3 --- /dev/null +++ b/spec/requests/two_factor_enrollment_gate_spec.rb @@ -0,0 +1,92 @@ +require "rails_helper" + +RSpec.describe "Two-factor enrollment gate", type: :request do + let(:identity) { create(:identity, two_factor_required: true) } + let(:session) do + identity.sessions.create!( + session_token: SecureRandom.hex(32), + expires_at: 1.week.from_now + ) + end + + before do + allow_any_instance_of(ApplicationController).to receive(:current_identity).and_return(identity) + allow_any_instance_of(ApplicationController).to receive(:current_session).and_return(session) + allow_any_instance_of(ApplicationController).to receive(:identity_signed_in?).and_return(true) + end + + context "when the identity has no 2FA method enrolled" do + it "redirects the home page to the security page" do + get "/" + + expect(response).to redirect_to(security_path) + expect(flash[:error]).to include("requires two-factor authentication") + end + + it "blocks OAuth authorization" do + get "/oauth/authorize" + + expect(response).to redirect_to(security_path) + end + + it "responds to HTMX requests with an HX-Redirect header" do + get "/", headers: { "HX-Request" => "true" } + + expect(response).to have_http_status(:ok) + expect(response.headers["HX-Redirect"]).to eq(security_path) + end + + it "allows the security page" do + get "/security" + + expect(response).to have_http_status(:ok) + end + + it "allows TOTP enrollment" do + get "/identity_totps" + + expect(response).to have_http_status(:ok) + end + + it "allows the authorized applications panel on the security page" do + get "/authorized_applications", headers: { "HX-Request" => "true" } + + expect(response).to have_http_status(:ok) + expect(response.headers["HX-Redirect"]).to be_nil + end + + it "allows passkey enrollment" do + get "/passkeys" + + expect(response).to have_http_status(:ok) + end + + it "allows logging out" do + delete "/logout" + + expect(response).to redirect_to(welcome_path) + end + end + + context "when the identity has a verified TOTP" do + before do + identity.totps.create!.mark_verified! + end + + it "does not redirect" do + get "/" + + expect(response).to have_http_status(:ok) + end + end + + context "when the identity does not have the override" do + let(:identity) { create(:identity) } + + it "does not redirect" do + get "/" + + expect(response).to have_http_status(:ok) + end + end +end diff --git a/spec/requests/two_factor_required_disable_spec.rb b/spec/requests/two_factor_required_disable_spec.rb new file mode 100644 index 00000000..c4469cb6 --- /dev/null +++ b/spec/requests/two_factor_required_disable_spec.rb @@ -0,0 +1,71 @@ +require "rails_helper" + +RSpec.describe "Disabling 2FA when required by admin", type: :request do + let(:identity) { create(:identity, two_factor_required: true, use_two_factor_authentication: true) } + let(:session) do + identity.sessions.create!( + session_token: SecureRandom.hex(32), + expires_at: 1.week.from_now + ) + end + let!(:totp) do + t = identity.totps.create! + t.mark_verified! + t + end + + before do + allow_any_instance_of(ApplicationController).to receive(:current_identity).and_return(identity) + allow_any_instance_of(ApplicationController).to receive(:current_session).and_return(session) + allow_any_instance_of(ApplicationController).to receive(:identity_signed_in?).and_return(true) + end + + describe "POST /identity/toggle_2fa" do + it "refuses to start the disable flow" do + post toggle_2fa_identity_path + + expect(response).to redirect_to(security_path) + expect(flash[:error]).to include("can't be disabled") + expect(identity.reload.use_two_factor_authentication).to be true + end + end + + describe "step-up disable_2fa completion" do + it "refuses to disable the 2FA requirement" do + code = ROTP::TOTP.new(totp.secret).now + + post "/step_up/verify", params: { action_type: "disable_2fa", method: "totp", code: code } + + expect(response).to redirect_to(security_path) + expect(flash[:alert]).to include("can't be disabled") + expect(identity.reload.use_two_factor_authentication).to be true + end + end + + describe "step-up remove_totp completion" do + it "refuses to remove the last 2FA method" do + code = ROTP::TOTP.new(totp.secret).now + + post "/step_up/verify", params: { action_type: "remove_totp", method: "totp", code: code } + + expect(response).to redirect_to(security_path) + expect(flash[:alert]).to include("Add another method") + expect(identity.reload.totp).to be_present + end + + it "allows removing the TOTP when a passkey remains" do + identity.webauthn_credentials.create!( + external_id: SecureRandom.hex(16), + public_key: SecureRandom.hex(32), + nickname: "test key", + sign_count: 0 + ) + code = ROTP::TOTP.new(totp.secret).now + + post "/step_up/verify", params: { action_type: "remove_totp", method: "totp", code: code } + + expect(response).to redirect_to(security_path) + expect(identity.reload.totp).to be_nil + end + end +end