Merge branch 'refactor/bypass-session-admin-mode' into 'master'

Bypass Session for API & Sesssionless Endpoints in Admin Mode

Closes #42685

See merge request gitlab-org/gitlab!25056

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}")
 
-          yield
-        ensure
+          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,294 +3,330 @@
 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) }
 
-  before do
-    allow(ActiveSession).to receive(:list_sessions).with(user).and_return([session])
-  end
-
-  shared_examples 'admin mode cannot be enabled' do
-    it 'is false by default' do
-      expect(subject.admin_mode?).to be(false)
-    end
-
-    it 'cannot be enabled with a valid password' do
-      subject.enable_admin_mode!(password: user.password)
-
-      expect(subject.admin_mode?).to be(false)
-    end
-
-    it 'cannot be enabled with an invalid password' do
-      subject.enable_admin_mode!(password: nil)
-
-      expect(subject.admin_mode?).to be(false)
-    end
-
-    it 'cannot be enabled with empty params' do
-      subject.enable_admin_mode!
+  context 'when session is available' do
+    include_context 'custom session'
 
-      expect(subject.admin_mode?).to be(false)
+    before do
+      allow(ActiveSession).to receive(:list_sessions).with(user).and_return([session])
     end
 
-    it 'disable has no effect' do
-      subject.enable_admin_mode!
-      subject.disable_admin_mode!
-
-      expect(subject.admin_mode?).to be(false)
-    end
+    shared_examples 'admin mode cannot be enabled' do
+      it 'is false by default' do
+        expect(subject.admin_mode?).to be(false)
+      end
 
-    context 'skipping password validation' do
       it 'cannot be enabled with a valid password' do
-        subject.enable_admin_mode!(password: user.password, skip_password_validation: true)
+        subject.enable_admin_mode!(password: user.password)
 
         expect(subject.admin_mode?).to be(false)
       end
 
       it 'cannot be enabled with an invalid password' do
-        subject.enable_admin_mode!(skip_password_validation: true)
+        subject.enable_admin_mode!(password: nil)
 
         expect(subject.admin_mode?).to be(false)
       end
-    end
-  end
 
-  describe '#admin_mode?' do
-    context 'when the user is a regular user' do
-      it_behaves_like 'admin mode cannot be enabled'
+      it 'cannot be enabled with empty params' do
+        subject.enable_admin_mode!
 
-      context 'bypassing session' do
-        it_behaves_like 'admin mode cannot be enabled' do
-          around do |example|
-            described_class.bypass_session!(user.id) { example.run }
-          end
-        end
+        expect(subject.admin_mode?).to be(false)
       end
-    end
-
-    context 'when the user is an admin' do
-      let(:user) { build_stubbed(:user, :admin) }
 
-      context 'when admin mode not requested' do
-        it 'is false by default' do
-          expect(subject.admin_mode?).to be(false)
-        end
-
-        it 'raises exception if we try to enable it' do
-          expect do
-            subject.enable_admin_mode!(password: user.password)
-          end.to raise_error(::Gitlab::Auth::CurrentUserMode::NotRequestedError)
+      it 'disable has no effect' do
+        subject.enable_admin_mode!
+        subject.disable_admin_mode!
 
-          expect(subject.admin_mode?).to be(false)
-        end
+        expect(subject.admin_mode?).to be(false)
       end
 
-      context 'when admin mode requested first' do
-        before do
-          subject.request_admin_mode!
-        end
+      context 'skipping password validation' do
+        it 'cannot be enabled with a valid password' do
+          subject.enable_admin_mode!(password: user.password, skip_password_validation: true)
 
-        it 'is false by default' do
           expect(subject.admin_mode?).to be(false)
         end
 
         it 'cannot be enabled with an invalid password' do
-          subject.enable_admin_mode!(password: nil)
+          subject.enable_admin_mode!(skip_password_validation: true)
 
           expect(subject.admin_mode?).to be(false)
         end
+      end
+    end
 
-        it 'can be enabled with a valid password' do
-          subject.enable_admin_mode!(password: user.password)
+    describe '#admin_mode?' do
+      context 'when the user is a regular user' do
+        it_behaves_like 'admin mode cannot be enabled'
 
-          expect(subject.admin_mode?).to be(true)
+        context 'bypassing session' do
+          it_behaves_like 'admin mode cannot be enabled' do
+            around do |example|
+              described_class.bypass_session!(user.id) { example.run }
+            end
+          end
         end
+      end
 
-        it 'can be disabled' do
-          subject.enable_admin_mode!(password: user.password)
-          subject.disable_admin_mode!
+      context 'when the user is an admin' do
+        let(:user) { build_stubbed(:user, :admin) }
 
-          expect(subject.admin_mode?).to be(false)
+        context 'when admin mode not requested' do
+          it 'is false by default' do
+            expect(subject.admin_mode?).to be(false)
+          end
+
+          it 'raises exception if we try to enable it' do
+            expect do
+              subject.enable_admin_mode!(password: user.password)
+            end.to raise_error(::Gitlab::Auth::CurrentUserMode::NotRequestedError)
+
+            expect(subject.admin_mode?).to be(false)
+          end
         end
 
-        it 'will expire in the future' do
-          subject.enable_admin_mode!(password: user.password)
-          expect(subject.admin_mode?).to be(true), 'admin mode is not active in the present'
+        context 'when admin mode requested first' do
+          before do
+            subject.request_admin_mode!
+          end
 
-          Timecop.freeze(Gitlab::Auth::CurrentUserMode::MAX_ADMIN_MODE_TIME.from_now) do
-            # in the future this will be a new request, simulate by clearing the RequestStore
-            Gitlab::SafeRequestStore.clear!
+          it 'is false by default' do
+            expect(subject.admin_mode?).to be(false)
+          end
+
+          it 'cannot be enabled with an invalid password' do
+            subject.enable_admin_mode!(password: nil)
 
-            expect(subject.admin_mode?).to be(false), 'admin mode did not expire in the future'
+            expect(subject.admin_mode?).to be(false)
           end
-        end
 
-        context 'skipping password validation' do
           it 'can be enabled with a valid password' do
-            subject.enable_admin_mode!(password: user.password, skip_password_validation: true)
+            subject.enable_admin_mode!(password: user.password)
 
             expect(subject.admin_mode?).to be(true)
           end
 
-          it 'can be enabled with an invalid password' do
-            subject.enable_admin_mode!(skip_password_validation: true)
+          it 'can be disabled' do
+            subject.enable_admin_mode!(password: user.password)
+            subject.disable_admin_mode!
 
-            expect(subject.admin_mode?).to be(true)
+            expect(subject.admin_mode?).to be(false)
           end
-        end
 
-        context 'with two independent sessions' do
-          let(:another_session) { {} }
-          let(:another_subject) { described_class.new(user) }
+          it 'will expire in the future' do
+            subject.enable_admin_mode!(password: user.password)
+            expect(subject.admin_mode?).to be(true), 'admin mode is not active in the present'
 
-          before do
-            allow(ActiveSession).to receive(:list_sessions).with(user).and_return([session, another_session])
+            Timecop.freeze(Gitlab::Auth::CurrentUserMode::MAX_ADMIN_MODE_TIME.from_now) do
+              # in the future this will be a new request, simulate by clearing the RequestStore
+              Gitlab::SafeRequestStore.clear!
+
+              expect(subject.admin_mode?).to be(false), 'admin mode did not expire in the future'
+            end
           end
 
-          it 'can be enabled in one and seen in the other' do
-            Gitlab::Session.with_session(another_session) do
-              another_subject.request_admin_mode!
-              another_subject.enable_admin_mode!(password: user.password)
+          context 'skipping password validation' do
+            it 'can be enabled with a valid password' do
+              subject.enable_admin_mode!(password: user.password, skip_password_validation: true)
+
+              expect(subject.admin_mode?).to be(true)
             end
 
-            expect(subject.admin_mode?).to be(true)
+            it 'can be enabled with an invalid password' do
+              subject.enable_admin_mode!(skip_password_validation: true)
+
+              expect(subject.admin_mode?).to be(true)
+            end
           end
-        end
-      end
 
-      context 'bypassing session' do
-        it 'is active by default' do
-          described_class.bypass_session!(user.id) do
-            expect(subject.admin_mode?).to be(true)
+          context 'with two independent sessions' do
+            let(:another_session) { {} }
+            let(:another_subject) { described_class.new(user) }
+
+            before do
+              allow(ActiveSession).to receive(:list_sessions).with(user).and_return([session, another_session])
+            end
+
+            it 'can be enabled in one and seen in the other' do
+              Gitlab::Session.with_session(another_session) do
+                another_subject.request_admin_mode!
+                another_subject.enable_admin_mode!(password: user.password)
+              end
+
+              expect(subject.admin_mode?).to be(true)
+            end
           end
         end
 
-        it 'enable has no effect' do
-          described_class.bypass_session!(user.id) do
-            subject.request_admin_mode!
-            subject.enable_admin_mode!(password: user.password)
+        context 'bypassing session' do
+          it 'is active by default' do
+            described_class.bypass_session!(user.id) do
+              expect(subject.admin_mode?).to be(true)
+            end
+          end
 
-            expect(subject.admin_mode?).to be(true)
+          it 'enable has no effect' do
+            described_class.bypass_session!(user.id) do
+              subject.request_admin_mode!
+              subject.enable_admin_mode!(password: user.password)
+
+              expect(subject.admin_mode?).to be(true)
+            end
           end
-        end
 
-        it 'disable has no effect' do
-          described_class.bypass_session!(user.id) do
-            subject.disable_admin_mode!
+          it 'disable has no effect' do
+            described_class.bypass_session!(user.id) do
+              subject.disable_admin_mode!
 
-            expect(subject.admin_mode?).to be(true)
+              expect(subject.admin_mode?).to be(true)
+            end
           end
         end
       end
     end
-  end
 
-  describe '#enable_admin_mode!' do
-    let(:user) { build_stubbed(:user, :admin) }
+    describe '#enable_admin_mode!' do
+      let(:user) { build_stubbed(:user, :admin) }
 
-    it 'creates a timestamp in the session' do
-      subject.request_admin_mode!
-      subject.enable_admin_mode!(password: user.password)
+      it 'creates a timestamp in the session' do
+        subject.request_admin_mode!
+        subject.enable_admin_mode!(password: user.password)
 
-      expect(session).to include(expected_session_entry(be_within(1.second).of Time.now))
+        expect(session).to include(expected_session_entry(be_within(1.second).of Time.now))
+      end
     end
-  end
 
-  describe '#enable_sessionless_admin_mode!' do
-    let(:user) { build_stubbed(:user, :admin) }
+    describe '#disable_admin_mode!' do
+      let(:user) { build_stubbed(:user, :admin) }
 
-    it 'enabled admin mode without password' do
-      subject.enable_sessionless_admin_mode!
+      it 'sets the session timestamp to nil' do
+        subject.request_admin_mode!
+        subject.disable_admin_mode!
 
-      expect(subject.admin_mode?).to be(true)
+        expect(session).to include(expected_session_entry(be_nil))
+      end
     end
-  end
 
-  describe '#disable_admin_mode!' do
-    let(:user) { build_stubbed(:user, :admin) }
+    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
+          described_class.with_current_admin(user) do
+            expect(described_class.current_admin).to be_nil
+          end
 
-    it 'sets the session timestamp to nil' do
-      subject.request_admin_mode!
-      subject.disable_admin_mode!
+          expect(described_class.bypass_session_admin_id).to be_nil
+        end
+      end
 
-      expect(session).to include(expected_session_entry(be_nil))
-    end
-  end
+      context 'with an admin user' do
+        let(:user) { build_stubbed(:user, :admin) }
 
-  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)
+        context 'admin mode is disabled' do
+          it 'user is not available inside nor outside the yielded block' do
+            described_class.with_current_admin(user) do
+              expect(described_class.current_admin).to be_nil
+            end
+
+            expect(described_class.bypass_session_admin_id).to be_nil
+          end
         end
 
-        expect(described_class.bypass_session_admin_id).to be_nil
-      end
-    end
+        context 'admin mode is enabled' do
+          before do
+            subject.request_admin_mode!
+            subject.enable_admin_mode!(password: user.password)
+          end
 
-    context 'with an admin user' do
-      let(:user) { build_stubbed(:user, :admin) }
+          it 'user is available only inside the yielded block' do
+            described_class.with_current_admin(user) do
+              expect(described_class.current_admin).to be(user)
+            end
 
-      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)
+            expect(described_class.current_admin).to be_nil
+          end
         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
-        described_class.with_current_admin(user) do
-          expect(described_class.current_admin).to be_nil
-        end
+    def expected_session_entry(value_matcher)
+      {
+        Gitlab::Auth::CurrentUserMode::SESSION_STORE_KEY => a_hash_including(
+          Gitlab::Auth::CurrentUserMode::ADMIN_MODE_START_TIME_KEY => value_matcher)
+      }
+    end
+  end
 
-        expect(described_class.bypass_session_admin_id).to be_nil
+  context 'when no session available' do
+    around do |example|
+      Gitlab::Session.with_session(nil) do
+        example.run
       end
     end
 
-    context 'with an admin user' do
-      let(:user) { build_stubbed(:user, :admin) }
+    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
 
-      context 'admin mode is disabled' do
-        it 'user is not available inside nor outside the yielded block' do
-          described_class.with_current_admin(user) do
-            expect(described_class.current_admin).to be_nil
+            expect(described_class.bypass_session_admin_id).to be_nil
           end
+        end
 
-          expect(described_class.bypass_session_admin_id).to be_nil
+        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 'admin mode is enabled' do
-        before do
-          subject.request_admin_mode!
-          subject.enable_admin_mode!(password: user.password)
-        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)
 
-        it 'user is available only inside the yielded block' do
-          described_class.with_current_admin(user) do
-            expect(described_class.current_admin).to be(user)
+            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
 
-          expect(described_class.current_admin).to be_nil
+        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
-
-  def expected_session_entry(value_matcher)
-    {
-      Gitlab::Auth::CurrentUserMode::SESSION_STORE_KEY => a_hash_including(
-        Gitlab::Auth::CurrentUserMode::ADMIN_MODE_START_TIME_KEY => value_matcher)
-    }
-  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) }