Refactor Webui::WebuiController#check_user

The logic of WebuiControllerService::UserChecker isn't leaking out of
the service anymore. This is to reuse the service.

src/api/app/controllers/webui/webui_controller.rb

@@ -119,25 +119,9 @@ def check_user
     @spider_bot = request.bot? && Rails.env.production?
     User.session = nil # reset old users hanging around
 
-    user_checker = WebuiControllerService::UserChecker.new(http_request: request, config: CONFIG)
-
-    if user_checker.proxy_enabled?
-      if user_checker.user_login.blank?
-        User.session = User.find_nobody!
-        return
-      end
-
-      User.session = user_checker.find_or_create_user!
-
-      if User.session!.is_active?
-        User.session!.update_login_values(request.env)
-      else
-        User.session!.count_login_failure
-        session[:login] = nil
-        User.session = User.find_nobody!
-        redirect_to(CONFIG['proxy_auth_logout_page'], error: 'Your account is disabled. Please contact the administrator for details.')
-        return
-      end
+    unless WebuiControllerService::UserChecker.new(http_request: request, config: CONFIG).call
+      redirect_to(CONFIG['proxy_auth_logout_page'], error: 'Your account is disabled. Please contact the administrator for details.')
+      return
     end
 
     User.session = User.find_by_login(session[:login]) if session[:login]

src/api/app/services/webui_controller_service/user_checker.rb

@@ -8,16 +8,32 @@ def initialize(http_request:, config:)
       @user_login = extract_user_login_from_http_request
     end
 
-    def proxy_enabled?
-      @config['proxy_auth_mode'] == :on
-    end
+    # Returns false if a user with a disabled account is trying to authenticate through the proxy
+    def call
+      return true unless proxy_enabled?
 
-    def extract_user_login_from_http_request
-      http_request.env['HTTP_X_USERNAME']
+      if user_login.blank?
+        User.session = User.find_nobody!
+        return true
+      end
+
+      User.session = find_or_create_user!
+
+      if User.session!.is_active?
+        User.session!.update_login_values(http_request.env)
+        true
+      else
+        User.session!.count_login_failure
+        http_request.session[:login] = nil
+        User.session = User.find_nobody!
+        false
+      end
     end
 
+    private
+
     def find_or_create_user!
-      if user.exists?
+      if User.exists?(login: user_login)
         User.find_by!(login: user_login)
       else
         # This will end up in a before_validation(on: :create) that updates last_logged_in_at.
@@ -28,16 +44,16 @@ def find_or_create_user!
       end
     end
 
-    def login_exists?
-      user.exists?
-    end
-
-    def user
-      User.where(login: user_login)
+    def proxy_enabled?
+      @config['proxy_auth_mode'] == :on
     end
 
     def realname
       "#{http_request.env['HTTP_X_FIRSTNAME']} #{http_request.env['HTTP_X_LASTNAME']}".strip
     end
+
+    def extract_user_login_from_http_request
+      http_request.env['HTTP_X_USERNAME']
+    end
   end
 end

src/api/spec/services/webui_controller_service/user_checker_spec.rb

@@ -2,67 +2,106 @@
 require 'ostruct'
 
 RSpec.describe ::WebuiControllerService::UserChecker do
-  let(:user) { create(:confirmed_user, login: 'foo') }
   let(:user_checker) { described_class.new(http_request: request, config: config) }
 
-  describe '#proxy_enabled?' do
-    let(:user_login) { user.login }
-    let(:request) { OpenStruct.new(env: { 'HTTP_X_USERNAME' => user_login }) }
+  describe '#call' do
+    subject { user_checker.call }
 
-    subject { user_checker.proxy_enabled? }
-
-    context 'when its enabled' do
-      let(:config) { { 'proxy_auth_mode' => :on } }
-
-      it { expect(subject).to be(true) }
-    end
-
-    context 'when its disabled' do
+    context 'when the proxy authentication is disabled' do
+      let(:request) do
+        OpenStruct.new(env: { 'HTTP_X_USERNAME' => 'something',
+                              'HTTP_X_FIRSTNAME' => 'foo', 'HTTP_X_LASTNAME' => 'bar',
+                              'HTTP_X_EMAIL' => 'foo@example.org' })
+      end
       let(:config) { { 'proxy_auth_mode' => :off } }
 
-      it { expect(subject).to be(false) }
-    end
-  end
-
-  describe '#login_exists?' do
-    let(:request) { OpenStruct.new(env: { 'HTTP_X_USERNAME' => user_login }) }
-    let(:config) { { 'proxy_auth_mode' => :on } }
-
-    subject { user_checker.login_exists? }
-
-    context 'user exists' do
-      let(:user_login) { user.login }
+      before do
+        allow(User).to receive(:find_nobody!)
+      end
 
-      it { expect(subject).to be(true) }
-    end
-
-    context 'user does not exist' do
-      let(:user_login) { 'bar' }
-
-      it { expect(subject).to be(false) }
-    end
-  end
+      it { is_expected.to be(true) }
 
-  describe '#find_or_create_user!' do
-    let(:request) do
-      OpenStruct.new(env: { 'HTTP_X_USERNAME' => user_login,
-                            'HTTP_X_FIRSTNAME' => 'foo', 'HTTP_X_LASTNAME' => 'bar',
-                            'HTTP_X_EMAIL' => 'foo@example.org' })
+      it 'does not set the current user' do
+        subject
+        expect(User).not_to have_received(:find_nobody!)
+      end
     end
-    let(:config) { { 'proxy_auth_mode' => :on } }
 
-    subject { user_checker.find_or_create_user! }
-
-    context 'it will find an user' do
-      let(:user_login) { user.login }
-
-      it { expect(subject).to eq(user) }
-    end
-
-    context 'it will create an user' do
-      let(:user_login) { 'bar' }
+    context 'when the proxy authentication is enabled' do
+      let(:config) { { 'proxy_auth_mode' => :on } }
 
-      it { expect { subject }.to change(User, :count).by(1) }
+      context 'without a username in the request' do
+        let(:request) do
+          OpenStruct.new(env: { 'HTTP_X_FIRSTNAME' => 'foo', 'HTTP_X_LASTNAME' => 'bar',
+                                'HTTP_X_EMAIL' => 'foo@example.org' })
+        end
+
+        before do
+          allow(User).to receive(:find_nobody!)
+        end
+
+        it { is_expected.to be(true) }
+
+        it 'sets the current user to the anonymous user' do
+          subject
+          expect(User).to have_received(:find_nobody!)
+        end
+      end
+
+      context 'with the username of a nonexistent user in the request' do
+        let(:request) do
+          OpenStruct.new(env: { 'HTTP_X_USERNAME' => 'foo',
+                                'HTTP_X_FIRSTNAME' => 'foo', 'HTTP_X_LASTNAME' => 'bar',
+                                'HTTP_X_EMAIL' => 'foo@example.org' },
+                         session: {})
+        end
+
+        it { is_expected.to be(true) }
+
+        it 'creates a user' do
+          expect { subject }.to change(User, :count).by(1)
+        end
+      end
+
+      context 'with the username of an active user in the request' do
+        let(:user) { create(:confirmed_user, login: 'foo') }
+        let(:request) do
+          OpenStruct.new(env: { 'HTTP_X_USERNAME' => user.login,
+                                'HTTP_X_FIRSTNAME' => 'foo', 'HTTP_X_LASTNAME' => 'bar',
+                                'HTTP_X_EMAIL' => 'foo@example.org' })
+        end
+
+        let!(:user_previous_email) { user.email }
+        let!(:user_previous_realname) { user.realname }
+
+        it { is_expected.to be(true) }
+
+        it 'sets the current user to the user matching the request and also updates their email and realname' do
+          subject
+          expect(User.session!).to have_attributes(id: user.id, email: 'foo@example.org', realname: 'foo bar')
+        end
+      end
+
+      context 'with the username of an inactive user in the request' do
+        let(:user) { create(:user, login: 'foo') }
+        let(:request) do
+          OpenStruct.new(env: { 'HTTP_X_USERNAME' => user.login,
+                                'HTTP_X_FIRSTNAME' => 'foo', 'HTTP_X_LASTNAME' => 'bar',
+                                'HTTP_X_EMAIL' => 'foo@example.org' },
+                         session: {})
+        end
+
+        it { is_expected.to be(false) }
+
+        it 'increases the number of login failures by one' do
+          expect { subject }.to change { user.reload.login_failure_count }.by(1)
+        end
+
+        it 'sets login to nil in the session' do
+          subject
+          expect(request.session).to eq({ login: nil })
+        end
+      end
     end
   end
 end