Require current password for sensitive user changes (#512)

Adds re-authentication (current password) to sensitive account actions,
so a session cannot make them without the acting user proving their
identity.

Changes:

- Password change: sets config.check_current_password_before_update =
:password. Previously current_password was validated only if supplied;
omitting it bypassed the check. Now it is required whenever the password
changes. The token-based reset flow stays exempt (allow_password_change
bypasses the check in DTA).

- Role changes: require_current_password! gates all role assignment and
removal on UserRolesController (not just promotions, so it stays correct
if non-hierarchical roles are added later). Re-authentication runs after
authorization, so a user who isn't permitted to change roles gets 403
(forbidden) rather than being prompted for a password.

- Email change: same gate on UsersController#update, but only when the
email is actually changing. Dormant for SG (update_email is disabled, so
email is stripped from permitted attributes); active on installs that
enable email editing.

The gate (require_current_password! in ApplicationController) is
defence-in-depth: the client sends current_password on these requests,
so it primarily blocks direct API calls (e.g. a hijacked session).

Existing UserRoles controller and API specs updated to send
current_password on the success cases and to reflect the
authorization-before-re-authentication ordering.

Verified on UAT: password change and role assign/remove reject without
current_password (401/422) and succeed with it. Email gate covered by
spec (stubbed on), since it is disabled on this build.

Depends on #508. Based on that branch; the email gate builds on its
permitted-attributes change.

Client side implementation still TODO

Author: tmfrnz

app/controllers/application_controller.rb

@@ -85,4 +85,30 @@ def user_not_authorized
   def skip_authentication?
     devise_or_devise_token_auth_controller? || action_name == "index"
   end
+
+  # Re-authentication gate for sensitive actions (role and email changes).
+  # Routed through Devise's valid_for_authentication? so failures count toward
+  # :lockable - a bare valid_password? here would be an unthrottled password
+  # oracle, usable even while the account is locked out of sign-in.
+  #
+  # Success resets failed_attempts explicitly: Devise normally does that in a
+  # Warden after_set_user hook, which does not fire on this path.
+  #
+  def require_current_password!
+    password = request.request_parameters[:current_password]
+
+    if password.present? &&
+        current_user&.valid_for_authentication? { current_user.valid_password?(password) }
+      if current_user.failed_attempts.to_i.positive?
+        current_user.update_column(:failed_attempts, 0)
+      end
+      return true
+    end
+
+    render json: {
+      status: "error",
+      errors: {current_password: ["is incorrect or missing"]}
+    }, status: :unauthorized
+    false
+  end
 end

app/controllers/user_roles_controller.rb

@@ -1,5 +1,6 @@
 class UserRolesController < ApplicationController
   before_action :set_and_authorize_user_role, only: [:destroy]
+  before_action :require_current_password!, only: [:destroy]
 
   # GET /user_roles
   def index
@@ -14,6 +15,7 @@ def create
     @user_role = UserRole.new
     @user_role.assign_attributes(permitted_attributes(@user_role))
     authorize @user_role
+    return unless require_current_password!
 
     if @user_role.save
       render json: serialize(@user_role), status: :created, location: @user_role

app/controllers/users_controller.rb

@@ -1,7 +1,7 @@
 class UsersController < ApplicationController
   before_action :authenticate_user!
-
   before_action :set_and_authorize_user, only: [:update]
+  before_action :require_current_password!, only: [:update], if: :email_change_requested?
 
   # GET /users
   def index
@@ -41,4 +41,9 @@ def set_and_authorize_user
     @user = policy_scope(base_object).find(params[:id])
     authorize @user
   end
+
+  def email_change_requested?
+    new_email = permitted_attributes(@user)[:email]
+    new_email.present? && new_email != @user.email
+  end
 end

config/initializers/devise_token_auth.rb

@@ -29,7 +29,7 @@
   # Uncomment to enforce current_password param to be checked before all
   # attribute updates. Set it to :password if you want it to be checked only if
   # password is updated.
-  # config.check_current_password_before_update = :attributes
+  config.check_current_password_before_update = :password
 
   # By default we will use callbacks for single omniauth.
   # It depends on fields like email, provider and uid.

spec/controllers/user_roles_controller_spec.rb

@@ -2,6 +2,8 @@
 require "json"
 
 RSpec.describe UserRolesController, type: :controller do
+  let(:current_password) { "SecurePassword123!" }
+
   describe "Get index" do
     subject { get :index, format: :json }
 
@@ -127,6 +129,7 @@ def self.all_roles
               sign_in user
 
               response = post :create, format: :json, params: {
+                current_password: current_password,
                 user_role: {
                   user_id: target_user.id,
                   role_id: target_role_record.id
@@ -156,6 +159,7 @@ def self.all_roles
                 sign_in user
 
                 response = post :create, format: :json, params: {
+                  current_password: current_password,
                   user_role: {
                     user_id: target_user.id,
                     role_id: target_role_record.id
@@ -196,6 +200,7 @@ def self.all_roles
         admin = FactoryBot.create(:user, :admin)
         sign_in admin
         post :create, format: :json, params: {
+          current_password: current_password,
           user_role: {description: "desc only", taxonomy_id: 999}
         }
         expect(response).to have_http_status(422)
@@ -207,6 +212,7 @@ def self.all_roles
         sign_in admin
 
         response = post :create, format: :json, params: {
+          current_password: current_password,
           user_role: {
             user_id: guest.id,
             role_id: admin_role.id
@@ -262,6 +268,7 @@ def self.all_roles
               sign_in user
 
               response = delete :destroy, format: :json, params: {
+                current_password: current_password,
                 id: target_user.user_roles.first.id
               }
               expect(response).to be_no_content
@@ -288,6 +295,7 @@ def self.all_roles
                 sign_in user
 
                 response = delete :destroy, format: :json, params: {
+                  current_password: current_password,
                   id: target_user.user_roles.first.id
                 }
                 expect(response).to be_no_content

spec/requests/current_password_gate_throttling_spec.rb

@@ -0,0 +1,84 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+# The re-authentication gate (ApplicationController#require_current_password!)
+# routes failures through Devise's valid_for_authentication?, so a wrong
+# current_password counts toward :lockable. Without that it would be an
+# unthrottled password oracle, usable even while the account is locked out of
+# sign-in. Exercised here via POST /user_roles; the gate is shared with
+# UsersController#update.
+RSpec.describe "Current password gate throttling", type: :request do
+  let(:password) { "SecurePassword123!" }
+  let(:admin) { FactoryBot.create(:user, :admin, password:, password_confirmation: password) }
+  let(:target) { FactoryBot.create(:user) }
+  let(:role) { FactoryBot.create(:role, :contributor) }
+
+  def sign_in_headers
+    allow(Rails.application.config).to receive(:enable_mfa).and_return(false)
+    post "/auth/sign_in", params: {email: admin.email, password:}, as: :json
+    expect(response).to have_http_status(:success)
+    {
+      "access-token" => response.headers["access-token"],
+      "client" => response.headers["client"],
+      "uid" => response.headers["uid"]
+    }
+  end
+
+  # Signed in once, then reused: a fresh sign-in would reset failed_attempts
+  # via Warden's after_set_user hook and mask the counting under test.
+  let!(:headers) { sign_in_headers }
+
+  def attempt(current_password)
+    post "/user_roles",
+      params: {current_password:, user_role: {user_id: target.id, role_id: role.id}},
+      headers:, as: :json
+  end
+
+  it "counts a wrong current_password toward lockout" do
+    attempt("WrongPassword999!")
+
+    expect(response).to have_http_status(401)
+    expect(admin.reload.failed_attempts).to eq(1)
+  end
+
+  it "rejects a wrong current_password without locking below the threshold" do
+    (Devise.maximum_attempts - 1).times { attempt("WrongPassword999!") }
+
+    expect(response).to have_http_status(401)
+    expect(admin.reload.failed_attempts).to eq(Devise.maximum_attempts - 1)
+    expect(admin.reload).not_to be_access_locked
+  end
+
+  it "locks the account after maximum_attempts" do
+    Devise.maximum_attempts.times { attempt("WrongPassword999!") }
+
+    expect(admin.reload).to be_access_locked
+  end
+
+  it "rejects a request from a locked account" do
+    admin.lock_access!
+
+    expect { attempt(password) }.not_to change(UserRole, :count)
+
+    expect(response).to have_http_status(401)
+  end
+
+  it "resets failed_attempts on success" do
+    admin.update_column(:failed_attempts, 2)
+
+    attempt(password)
+
+    expect(response).to have_http_status(:created)
+    expect(admin.reload.failed_attempts).to eq(0)
+  end
+
+  it "does not count a missing current_password" do
+    post "/user_roles",
+      params: {user_role: {user_id: target.id, role_id: role.id}},
+      headers:, as: :json
+
+    expect(response).to have_http_status(401)
+    expect(admin.reload.failed_attempts).to eq(0)
+  end
+end

spec/requests/email_change_requires_current_password_spec.rb

@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+RSpec.describe "Email change requires current password", type: :request do
+  let(:password) { "SecurePassword123!" }
+  let(:admin) { FactoryBot.create(:user, :admin, password:, password_confirmation: password) }
+
+  before do
+    allow_any_instance_of(UserPolicy).to receive(:permitted_attributes)
+      .and_return([:name, :email])
+  end
+
+  def sign_in_headers
+    allow(Rails.application.config).to receive(:enable_mfa).and_return(false)
+    post "/auth/sign_in", params: {email: admin.email, password:}, as: :json
+    expect(response).to have_http_status(:success)
+    {
+      "access-token" => response.headers["access-token"],
+      "client" => response.headers["client"],
+      "uid" => response.headers["uid"]
+    }
+  end
+
+  it "rejects an email change without current_password" do
+    headers = sign_in_headers
+
+    put "/users/#{admin.id}",
+      params: {user: {email: "new@example.com"}}, headers:, as: :json
+
+    expect(response).to have_http_status(401)
+    expect(admin.reload.email).not_to eq("new@example.com")
+  end
+
+  it "allows an email change with correct current_password" do
+    headers = sign_in_headers
+
+    put "/users/#{admin.id}",
+      params: {current_password: password, user: {email: "new@example.com"}},
+      headers:, as: :json
+
+    expect(response).to have_http_status(:success)
+    expect(admin.reload.email).to eq("new@example.com")
+  end
+
+  it "does not require current_password for a name-only update" do
+    headers = sign_in_headers
+
+    put "/users/#{admin.id}",
+      params: {user: {name: "New Name"}}, headers:, as: :json
+
+    expect(response).to have_http_status(:success)
+    expect(admin.reload.name).to eq("New Name")
+  end
+end

spec/requests/password_change_requires_current_password_spec.rb

@@ -0,0 +1,66 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+RSpec.describe "In-app password change requires current password", type: :request do
+  let(:current_password) { "SecurePassword123!" }
+  let(:new_password) { "SecurePassword456!" }
+
+  let(:user) do
+    FactoryBot.create(:user, password: current_password, password_confirmation: current_password)
+  end
+
+  def sign_in_headers
+    allow(Rails.application.config).to receive(:enable_mfa).and_return(false)
+    post "/auth/sign_in", params: {email: user.email, password: current_password}, as: :json
+    expect(response).to have_http_status(:success)
+    {
+      "access-token" => response.headers["access-token"],
+      "client" => response.headers["client"],
+      "uid" => response.headers["uid"]
+    }
+  end
+
+  it "rejects a password change without current_password" do
+    headers = sign_in_headers
+
+    put "/auth",
+      params: {password: new_password, password_confirmation: new_password},
+      headers: headers, as: :json
+
+    expect(response).to have_http_status(422)
+    expect(user.reload.valid_password?(current_password)).to be(true)
+    expect(user.reload.valid_password?(new_password)).to be(false)
+  end
+
+  it "rejects a password change with an incorrect current_password" do
+    headers = sign_in_headers
+
+    put "/auth",
+      params: {
+        current_password: "WrongPassword999!",
+        password: new_password,
+        password_confirmation: new_password
+      },
+      headers: headers, as: :json
+
+    expect(response).to have_http_status(422)
+    expect(user.reload.valid_password?(current_password)).to be(true)
+    expect(user.reload.valid_password?(new_password)).to be(false)
+  end
+
+  it "allows a password change with the correct current_password" do
+    headers = sign_in_headers
+
+    put "/auth",
+      params: {
+        current_password: current_password,
+        password: new_password,
+        password_confirmation: new_password
+      },
+      headers: headers, as: :json
+
+    expect(response).to have_http_status(:success)
+    expect(user.reload.valid_password?(new_password)).to be(true)
+  end
+end