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.

7 files changed, 158 insertions, 3 deletions

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 @@
+<h1>Second factor</h1>
+
+<div id="page_editor">
+  <div class="node_description">Code</div>
+  <div class="node_content">
+    <%= 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 %>
+    <span class="field_hint">Enter the six-digit code from your authenticator app.</span>
+  </div>
+</div>
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