Sessionless and API endpoints bypass session for admin mode

Use new `CurrentUserMode.bypass_session!` method to ignore sessions in
both sessionless and API endpoints for admin mode

app/controllers/application_controller.rb

@@ -34,6 +34,7 @@ class ApplicationController < ActionController::Base
   before_action :check_impersonation_availability
   before_action :required_signup_info
 
+  around_action :sessionless_bypass_admin_mode!, if: :sessionless_user?
   around_action :set_current_context
   around_action :set_locale
   around_action :set_session_storage

app/controllers/concerns/sessionless_authentication.rb

@@ -5,12 +5,6 @@
 # Controller concern to handle PAT, RSS, and static objects token authentication methods
 #
 module SessionlessAuthentication
-  extend ActiveSupport::Concern
-
-  included do
-    before_action :enable_admin_mode!, if: :sessionless_user?
-  end
-
   # This filter handles personal access tokens, atom requests with rss tokens, and static object tokens
   def authenticate_sessionless_user!(request_format)
     user = Gitlab::Auth::RequestAuthenticator.new(request).find_sessionless_user(request_format)
@@ -32,9 +26,9 @@ module SessionlessAuthentication
     end
   end
 
-  def enable_admin_mode!
-    return unless Feature.enabled?(:user_mode_in_session)
+  def sessionless_bypass_admin_mode!(&block)
+    return yield unless Feature.enabled?(:user_mode_in_session)
 
-    current_user_mode.enable_sessionless_admin_mode!
+    Gitlab::Auth::CurrentUserMode.bypass_session!(current_user.id, &block)
   end
 end

app/controllers/graphql_controller.rb

@@ -15,6 +15,11 @@ class GraphqlController < ApplicationController
   before_action :authorize_access_api!
   before_action(only: [:execute]) { authenticate_sessionless_user!(:api) }
 
+  # Since we deactivate authentication from the main ApplicationController and
+  # defer it to :authorize_access_api!, we need to override the bypass session
+  # callback execution order here
+  around_action :sessionless_bypass_admin_mode!, if: :sessionless_user?
+
   def execute
     result = multiplex? ? execute_multiplex : execute_query
 

changelogs/unreleased/refactor-bypass-session-admin-mode.yml

+---
+title: Sessionless and API endpoints bypass session for admin mode
+merge_request: 25056
+author: Diego Louzán
+type: changed

lib/api/api_guard.rb

@@ -50,17 +50,13 @@ module API
         user = find_user_from_sources
         return unless user
 
+        # Sessions are enforced to be unavailable for API calls, so ignore them for admin mode
+        Gitlab::Auth::CurrentUserMode.bypass_session!(user.id) if Feature.enabled?(:user_mode_in_session)
+
         unless api_access_allowed?(user)
           forbidden!(api_access_denied_message(user))
         end
 
-        # Set admin mode for API requests (if admin)
-        if Feature.enabled?(:user_mode_in_session)
-          current_user_mode = Gitlab::Auth::CurrentUserMode.new(user)
-
-          current_user_mode.enable_sessionless_admin_mode!
-        end
-
         user
       end
 
@@ -154,19 +150,13 @@ module API
     end
 
     class AdminModeMiddleware < ::Grape::Middleware::Base
-      def initialize(app, **options)
-        super
-      end
+      def after
+        # Use a Grape middleware since the Grape `after` blocks might run
+        # before we are finished rendering the `Grape::Entity` classes
+        Gitlab::Auth::CurrentUserMode.reset_bypass_session! if Feature.enabled?(:user_mode_in_session)
 
-      def call(env)
-        if Feature.enabled?(:user_mode_in_session)
-          session = {}
-          Gitlab::Session.with_session(session) do
-            app.call(env)
-          end
-        else
-          app.call(env)
-        end
+        # Explicit nil is needed or the api call return value will be overwritten
+        nil
       end
     end
   end

lib/gitlab/auth/current_user_mode.rb

@@ -23,15 +23,26 @@ module Gitlab
 
       class << self
         # Admin mode activation requires storing a flag in the user session. Using this
-        # method when scheduling jobs in Sidekiq will bypass the session check for a
-        # user that was already in admin mode
+        # method when scheduling jobs in sessionless environments (e.g. Sidekiq, API)
+        # will bypass the session check for a user that was already in admin mode
+        #
+        # If passed a block, it will surround the block execution and reset the session
+        # bypass at the end; otherwise use manually '.reset_bypass_session!'
         def bypass_session!(admin_id)
           Gitlab::SafeRequestStore[CURRENT_REQUEST_BYPASS_SESSION_ADMIN_ID_RS_KEY] = admin_id
 
           Gitlab::AppLogger.debug("Bypassing session in admin mode for: #{admin_id}")
 
+          if block_given?
+            begin
               yield
             ensure
+              reset_bypass_session!
+            end
+          end
+        end
+
+        def reset_bypass_session!
           Gitlab::SafeRequestStore.delete(CURRENT_REQUEST_BYPASS_SESSION_ADMIN_ID_RS_KEY)
         end
 
@@ -90,10 +101,6 @@ module Gitlab
         current_session_data[ADMIN_MODE_START_TIME_KEY] = Time.now
       end
 
-      def enable_sessionless_admin_mode!
-        request_admin_mode! && enable_admin_mode!(skip_password_validation: true)
-      end
-
       def disable_admin_mode!
         return unless user&.admin?
 

spec/lib/gitlab/auth/current_user_mode_spec.rb

@@ -3,12 +3,13 @@
 require 'spec_helper'
 
 describe Gitlab::Auth::CurrentUserMode, :do_not_mock_admin_mode, :request_store do
-  include_context 'custom session'
-
   let(:user) { build_stubbed(:user) }
 
   subject { described_class.new(user) }
 
+  context 'when session is available' do
+    include_context 'custom session'
+
     before do
       allow(ActiveSession).to receive(:list_sessions).with(user).and_return([session])
     end
@@ -199,16 +200,6 @@ describe Gitlab::Auth::CurrentUserMode, :do_not_mock_admin_mode, :request_store
       end
     end
 
-  describe '#enable_sessionless_admin_mode!' do
-    let(:user) { build_stubbed(:user, :admin) }
-
-    it 'enabled admin mode without password' do
-      subject.enable_sessionless_admin_mode!
-
-      expect(subject.admin_mode?).to be(true)
-    end
-  end
-
     describe '#disable_admin_mode!' do
       let(:user) { build_stubbed(:user, :admin) }
 
@@ -220,32 +211,6 @@ describe Gitlab::Auth::CurrentUserMode, :do_not_mock_admin_mode, :request_store
       end
     end
 
-  describe '.bypass_session!' do
-    context 'with a regular user' do
-      it 'admin mode is false' do
-        described_class.bypass_session!(user.id) do
-          expect(subject.admin_mode?).to be(false)
-          expect(described_class.bypass_session_admin_id).to be(user.id)
-        end
-
-        expect(described_class.bypass_session_admin_id).to be_nil
-      end
-    end
-
-    context 'with an admin user' do
-      let(:user) { build_stubbed(:user, :admin) }
-
-      it 'admin mode is true' do
-        described_class.bypass_session!(user.id) do
-          expect(subject.admin_mode?).to be(true)
-          expect(described_class.bypass_session_admin_id).to be(user.id)
-        end
-
-        expect(described_class.bypass_session_admin_id).to be_nil
-      end
-    end
-  end
-
     describe '.with_current_request_admin_mode' do
       context 'with a regular user' do
         it 'user is not available inside nor outside the yielded block' do
@@ -293,4 +258,75 @@ describe Gitlab::Auth::CurrentUserMode, :do_not_mock_admin_mode, :request_store
           Gitlab::Auth::CurrentUserMode::ADMIN_MODE_START_TIME_KEY => value_matcher)
       }
     end
+  end
+
+  context 'when no session available' do
+    around do |example|
+      Gitlab::Session.with_session(nil) do
+        example.run
+      end
+    end
+
+    describe '.bypass_session!' do
+      context 'when providing a block' do
+        context 'with a regular user' do
+          it 'admin mode is false' do
+            described_class.bypass_session!(user.id) do
+              expect(Gitlab::Session.current).to be_nil
+              expect(subject.admin_mode?).to be(false)
+              expect(described_class.bypass_session_admin_id).to be(user.id)
+            end
+
+            expect(described_class.bypass_session_admin_id).to be_nil
+          end
+        end
+
+        context 'with an admin user' do
+          let(:user) { build_stubbed(:user, :admin) }
+
+          it 'admin mode is true' do
+            described_class.bypass_session!(user.id) do
+              expect(Gitlab::Session.current).to be_nil
+              expect(subject.admin_mode?).to be(true)
+              expect(described_class.bypass_session_admin_id).to be(user.id)
+            end
+
+            expect(described_class.bypass_session_admin_id).to be_nil
+          end
+        end
+      end
+
+      context 'when not providing a block' do
+        context 'with a regular user' do
+          it 'admin mode is false' do
+            described_class.bypass_session!(user.id)
+
+            expect(Gitlab::Session.current).to be_nil
+            expect(subject.admin_mode?).to be(false)
+            expect(described_class.bypass_session_admin_id).to be(user.id)
+
+            described_class.reset_bypass_session!
+
+            expect(described_class.bypass_session_admin_id).to be_nil
+          end
+        end
+
+        context 'with an admin user' do
+          let(:user) { build_stubbed(:user, :admin) }
+
+          it 'admin mode is true' do
+            described_class.bypass_session!(user.id)
+
+            expect(Gitlab::Session.current).to be_nil
+            expect(subject.admin_mode?).to be(true)
+            expect(described_class.bypass_session_admin_id).to be(user.id)
+
+            described_class.reset_bypass_session!
+
+            expect(described_class.bypass_session_admin_id).to be_nil
+          end
+        end
+      end
+    end
+  end
 end

spec/requests/api/api_guard/admin_mode_middleware_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::APIGuard::AdminModeMiddleware, :do_not_mock_admin_mode, :request_store do
+  let(:user) { create(:admin) }
+
+  it 'is loaded' do
+    expect(API::API.middleware).to include([:use, described_class])
+  end
+
+  context 'when there is an exception in the api call' do
+    let(:app) do
+      Class.new(API::API) do
+        get 'willfail' do
+          raise StandardError.new('oh noes!')
+        end
+      end
+    end
+
+    it 'resets admin mode' do
+      Gitlab::Auth::CurrentUserMode.bypass_session!(user.id)
+
+      expect(Gitlab::Auth::CurrentUserMode.bypass_session_admin_id).to be(user.id)
+      expect(Gitlab::Auth::CurrentUserMode).to receive(:reset_bypass_session!).and_call_original
+
+      get api('/willfail')
+
+      expect(response.status).to eq(500)
+      expect(response.body).to include('oh noes!')
+
+      expect(Gitlab::Auth::CurrentUserMode.bypass_session_admin_id).to be_nil
+    end
+  end
+end

spec/requests/api/graphql/group_query_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 # Based on spec/requests/api/groups_spec.rb
 # Should follow closely in order to ensure all situations are covered
-describe 'getting group information' do
+describe 'getting group information', :do_not_mock_admin_mode do
   include GraphqlHelpers
   include UploadHelpers
 

spec/requests/api/graphql/mutations/snippets/mark_as_spam_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe 'Mark snippet as spam' do
+describe 'Mark snippet as spam', :do_not_mock_admin_mode do
   include GraphqlHelpers
 
   let_it_be(:admin) { create(:admin) }

spec/requests/api/users_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe API::Users do
+describe API::Users, :do_not_mock_admin_mode do
   let(:user)  { create(:user, username: 'user.with.dot') }
   let(:admin) { create(:admin) }
   let(:key) { create(:key, user: user) }