From dcb576618b868b888a5b1b31e35491f300ce4050 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Fri, 24 Jul 2026 13:53:13 +0200 Subject: Complete the login only after the second factor Enrolled users get a pending marker instead of a session after the password step; a valid code through the challenge writes the real session via reset_session. otp_required without enrollment funnels into setup everywhere except the enrollment, user, and login machinery. --- app/controllers/application_controller.rb | 13 ++++++ app/controllers/otp_challenges_controller.rb | 54 ++++++++++++++++++++++ app/controllers/sessions_controller.rb | 20 ++++++-- app/views/otp_challenges/new.html.erb | 13 ++++++ test/controllers/admin_controller_test.rb | 7 +++ test/controllers/otp_challenges_controller_test.rb | 39 ++++++++++++++++ test/controllers/sessions_controller_test.rb | 15 ++++++ 7 files changed, 158 insertions(+), 3 deletions(-) create mode 100644 app/controllers/otp_challenges_controller.rb create mode 100644 app/views/otp_challenges/new.html.erb create mode 100644 test/controllers/otp_challenges_controller_test.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index d8de9750..6d46d522 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -4,6 +4,7 @@ class ApplicationController < ActionController::Base protect_from_forgery before_action :set_locale + before_action :enforce_otp_enrollment helper_method :safe_return_to @@ -30,4 +31,16 @@ class ApplicationController < ActionController::Base rescue URI::InvalidURIError default end + + # The hard gate for the slow transition: a user flagged otp_required + # who has not enrolled can reach only enrollment, their own user page, + # the login machinery, and the challenge -- everything else funnels + # into setup. Anonymous visitors are untouched (not logged_in?). + def enforce_otp_enrollment + return unless logged_in? + return unless current_user.otp_required? && !current_user.otp_enrolled? + return if %w[otp_enrollments otp_challenges sessions users].include?(controller_name) + flash[:error] = "Your account requires a second factor -- set it up to continue." + redirect_to edit_user_path(current_user) + end end diff --git a/app/controllers/otp_challenges_controller.rb b/app/controllers/otp_challenges_controller.rb new file mode 100644 index 00000000..892503a8 --- /dev/null +++ b/app/controllers/otp_challenges_controller.rb @@ -0,0 +1,54 @@ +# The second half of a two-step login. A pending marker (set by +# sessions#create after a correct password) plus deadline and attempt +# counter live in the session; the real user_id is only written after a +# valid code, through a fresh session. +class OtpChallengesController < ApplicationController + + layout 'admin' + + MAX_ATTEMPTS = 5 + + def new + redirect_to login_path unless pending_user + end + + def create + user = pending_user + return redirect_to login_path unless user + + session[:otp_attempts] = session[:otp_attempts].to_i + 1 + if session[:otp_attempts] > MAX_ATTEMPTS + clear_pending + flash[:error] = "Too many attempts -- log in again." + return redirect_to login_path + end + + if user.verify_otp!(params[:code]) + return_to = session[:return_to] + reset_session + self.current_user = user + flash[:notice] = "Logged in successfully" + redirect_to safe_return_to(return_to, :default => admin_path) + else + flash.now[:error] = "That code did not match." + render :new + end + end + + private + + def pending_user + return nil if session[:pending_otp_user_id].blank? + if session[:otp_deadline].to_i < Time.now.to_i + clear_pending + return nil + end + @pending_user ||= User.find_by(:id => session[:pending_otp_user_id]) + end + + def clear_pending + session.delete(:pending_otp_user_id) + session.delete(:otp_deadline) + session.delete(:otp_attempts) + end +end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 64bf951a..f0d5cf9b 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -20,9 +20,23 @@ class SessionsController < ApplicationController # button. Uncomment if you understand the tradeoffs. reset_session - self.current_user = user - redirect_to safe_return_to(return_to, :default => admin_path) - flash[:notice] = "Logged in successfully" + if user.otp_enrolled? + # Half-completed login: no user_id yet, only the pending marker. + session[:pending_otp_user_id] = user.id + session[:otp_deadline] = 2.minutes.from_now.to_i + session[:otp_attempts] = 0 + session[:return_to] = return_to + redirect_to new_otp_challenge_path + else + self.current_user = user + if user.otp_required? + flash[:error] = "Your account requires a second factor -- set it up now." + redirect_to edit_user_path(user) + else + flash[:notice] = "Logged in successfully" + redirect_to safe_return_to(return_to, :default => admin_path) + end + end else note_failed_signin @login = params[:login] diff --git a/app/views/otp_challenges/new.html.erb b/app/views/otp_challenges/new.html.erb new file mode 100644 index 00000000..a9c7a15c --- /dev/null +++ b/app/views/otp_challenges/new.html.erb @@ -0,0 +1,13 @@ +

Second factor

+ +
+
Code
+
+ <%= form_tag otp_challenge_path, :method => :post do %> + <%= text_field_tag :code, nil, :autofocus => true, + :autocomplete => "one-time-code", :inputmode => "numeric" %> + <%= submit_tag "Log in" %> + <% end %> + Enter the six-digit code from your authenticator app. +
+
diff --git a/test/controllers/admin_controller_test.rb b/test/controllers/admin_controller_test.rb index cba4a59b..a177851f 100644 --- a/test/controllers/admin_controller_test.rb +++ b/test/controllers/admin_controller_test.rb @@ -38,4 +38,11 @@ class AdminControllerTest < ActionController::TestCase assert_equal [], json["tags"] assert_equal [], json["nodes"] end + + test "otp_required users without enrollment are funneled to setup" do + users(:quentin).update!(:otp_required => true) + login_as :quentin + get :index + assert_redirected_to edit_user_path(users(:quentin)) + end end diff --git a/test/controllers/otp_challenges_controller_test.rb b/test/controllers/otp_challenges_controller_test.rb new file mode 100644 index 00000000..470e84a2 --- /dev/null +++ b/test/controllers/otp_challenges_controller_test.rb @@ -0,0 +1,39 @@ +require "test_helper" + +class OtpChallengesControllerTest < ActionController::TestCase + fixtures :users + + def setup + @user = users(:quentin) + @user.update!(:otp_secret => ROTP::Base32.random) + @request.session[:pending_otp_user_id] = @user.id + @request.session[:otp_deadline] = 2.minutes.from_now.to_i + @request.session[:otp_attempts] = 0 + end + + test "a valid code completes the login" do + post :create, params: { :code => ROTP::TOTP.new(@user.otp_secret).now } + assert_equal @user.id, session[:user_id] + assert_nil session[:pending_otp_user_id] + end + + test "a wrong code does not log in" do + post :create, params: { :code => "000000" } + assert_nil session[:user_id] + assert_response :success + end + + test "the pending window expires" do + @request.session[:otp_deadline] = 1.minute.ago.to_i + post :create, params: { :code => ROTP::TOTP.new(@user.otp_secret).now } + assert_nil session[:user_id] + assert_redirected_to login_path + end + + test "attempts are limited" do + 5.times { post :create, params: { :code => "000000" } } + post :create, params: { :code => ROTP::TOTP.new(@user.otp_secret).now } + assert_nil session[:user_id] + assert_redirected_to login_path + end +end diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index a5f511f5..62acd28a 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -23,4 +23,19 @@ class SessionsControllerTest < ActionController::TestCase assert_nil session[:user_id] assert_response :redirect end + + test "login with password only is withheld for enrolled users" do + users(:quentin).update!(:otp_secret => ROTP::Base32.random) + post :create, params: { login: 'quentin', password: 'monkey' } + assert_nil session[:user_id] + assert_equal users(:quentin).id, session[:pending_otp_user_id] + assert_redirected_to new_otp_challenge_path + end + + test "otp_required without enrollment logs in but funnels into setup" do + users(:quentin).update!(:otp_required => true) + post :create, params: { login: 'quentin', password: 'monkey' } + assert session[:user_id] + assert_redirected_to edit_user_path(users(:quentin)) + end end -- cgit v1.3