From 70a0cc4d7a5ad3c2114e37ad059decd0cebd7d77 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Tue, 13 May 2025 11:10:12 +0200
Subject: [PATCH] DEV: better error message when "becoming" an inactive user
 (#32689)

In development mode, when 'DISCOURSE_DEV_ALLOW_ANON_TO_IMPERSONATE' is
enabled, and going to /session/:username/become, we will now show an
error message when trying to impersonate an inactive user.

This was not obvious why trying to impersonate a user wasn't working
locally because I would hit the URL and be redirected back to the index
without any error and without being logged in.
---
 app/controllers/session_controller.rb    | 11 +++---
 spec/requests/session_controller_spec.rb | 46 ++++++++++++++++++------
 2 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb
index ac600d05c9a..396c9c59110 100644
--- a/app/controllers/session_controller.rb
+++ b/app/controllers/session_controller.rb
@@ -110,19 +110,22 @@ class SessionController < ApplicationController
       raise Discourse::ReadOnly if @readonly_mode
 
       if ENV["DISCOURSE_DEV_ALLOW_ANON_TO_IMPERSONATE"] != "1"
-        render(content_type: "text/plain", inline: <<~TEXT)
+        return render plain: <<~TEXT, status: 403
           To enable impersonating any user without typing passwords set the following ENV var
 
           export DISCOURSE_DEV_ALLOW_ANON_TO_IMPERSONATE=1
 
           You can do that in your bashrc of bash profile file or the script you use to launch the web server
         TEXT
-
-        return
       end
 
       user = User.find_by_username(params[:session_id])
-      raise "User #{params[:session_id]} not found" if user.blank?
+
+      if user.blank?
+        return render plain: "User #{params[:session_id]} not found", status: 403
+      elsif !user.active?
+        return render plain: "User #{params[:session_id]} is not active", status: 403
+      end
 
       log_on_user(user)
 
diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb
index da02b647b50..4955dfeb14f 100644
--- a/spec/requests/session_controller_spec.rb
+++ b/spec/requests/session_controller_spec.rb
@@ -522,20 +522,44 @@ RSpec.describe SessionController do
   describe "#become" do
     let!(:user) { Fabricate(:user) }
 
-    it "does not work when in production mode" do
-      Rails.env.stubs(:production?).returns(true)
-      get "/session/#{user.username}/become.json"
+    describe "when in production mode" do
+      before { Rails.env.stubs(:production?).returns(true) }
 
-      expect(response.status).to eq(403)
-      expect(response.parsed_body["error_type"]).to eq("invalid_access")
-      expect(session[:current_user_id]).to be_blank
+      it "does not work" do
+        get "/session/#{user.username}/become"
+
+        expect(response.status).to eq(403)
+        expect(session[:current_user_id]).to be_blank
+      end
     end
 
-    it "works in development mode" do
-      Rails.env.stubs(:development?).returns(true)
-      get "/session/#{user.username}/become.json"
-      expect(response).to be_redirect
-      expect(session[:current_user_id]).to eq(user.id)
+    describe "when in development mode" do
+      before { Rails.env.stubs(:development?).returns(true) }
+
+      it "works" do
+        get "/session/#{user.username}/become"
+
+        expect(response).to be_redirect
+        expect(session[:current_user_id]).to eq(user.id)
+      end
+
+      it "raises an error if the user is not found" do
+        get "/session/invalid_user/become"
+
+        expect(response.status).to eq(403)
+        expect(response.body).to include("User invalid_user not found")
+        expect(session[:current_user_id]).to be_blank
+      end
+
+      it "raises an error if the user is not active" do
+        user.update!(active: false)
+
+        get "/session/#{user.username}/become"
+
+        expect(response.status).to eq(403)
+        expect(response.body).to include("User #{user.username} is not active")
+        expect(session[:current_user_id]).to be_blank
+      end
     end
   end