Merge branch 'ashmckenzie/provide_gl-type_to_gitlab_shell' into 'master'

Extract /internal/allowed API Actor logic out

See merge request gitlab-org/gitlab-ce!31564

lib/api/helpers/internal_helpers.rb

@@ -17,6 +17,18 @@ module API
         @project # rubocop:disable Gitlab/ModuleWithInstanceVariables
       end
 
+      def access_checker_for(actor, protocol)
+        access_checker_klass.new(actor.key_or_user, project, protocol,
+          authentication_abilities: ssh_authentication_abilities,
+          namespace_path: namespace_path,
+          project_path: project_path,
+          redirected_path: redirected_path)
+      end
+
+      def access_checker_klass
+        repo_type.access_checker_class
+      end
+
       def ssh_authentication_abilities
         [
           :read_project,

lib/api/internal/base.rb

@@ -35,36 +35,14 @@ module API
         #   project - project full_path (not path on disk)
         #   action - git action (git-upload-pack or git-receive-pack)
         #   changes - changes as "oldrev newrev ref", see Gitlab::ChangesList
-        # rubocop: disable CodeReuse/ActiveRecord
         post "/allowed" do
           # Stores some Git-specific env thread-safely
           env = parse_env
           Gitlab::Git::HookEnv.set(gl_repository, env) if project
 
-          actor =
-            if params[:key_id]
-              Key.find_by(id: params[:key_id])
-            elsif params[:user_id]
-              User.find_by(id: params[:user_id])
-            elsif params[:username]
-              UserFinder.new(params[:username]).find_by_username
-            end
-
-          protocol = params[:protocol]
-
-          actor.update_last_used_at if actor.is_a?(Key)
-          user =
-            if actor.is_a?(Key)
-              actor.user
-            else
-              actor
-            end
-
-          access_checker_klass = repo_type.access_checker_class
-          access_checker = access_checker_klass.new(actor, project,
-                                                    protocol, authentication_abilities: ssh_authentication_abilities,
-                                                    namespace_path: namespace_path, project_path: project_path,
-                                                    redirected_path: redirected_path)
+          actor = Support::GitAccessActor.from_params(params)
+          actor.update_last_used_at!
+          access_checker = access_checker_for(actor, params[:protocol])
 
           check_result = begin
                            result = access_checker.check(params[:action], params[:changes])
@@ -78,15 +56,15 @@ module API
                            break response_with_status(code: 404, success: false, message: e.message)
                          end
 
-          log_user_activity(actor)
+          log_user_activity(actor.user)
 
           case check_result
           when ::Gitlab::GitAccessResult::Success
             payload = {
               gl_repository: gl_repository,
               gl_project_path: gl_project_path,
-              gl_id: Gitlab::GlId.gl_id(user),
-              gl_username: user&.username,
+              gl_id: Gitlab::GlId.gl_id(actor.user),
+              gl_username: actor.username,
               git_config_options: [],
               gitaly: gitaly_payload(params[:action]),
               gl_console_messages: check_result.console_messages
@@ -105,7 +83,6 @@ module API
             response_with_status(code: 500, success: false, message: UNKNOWN_CHECK_RESULT_ERROR)
           end
         end
-        # rubocop: enable CodeReuse/ActiveRecord
 
         # rubocop: disable CodeReuse/ActiveRecord
         post "/lfs_authenticate" do

lib/api/support/git_access_actor.rb

+# frozen_string_literal: true
+
+module API
+  module Support
+    class GitAccessActor
+      attr_reader :user
+
+      def initialize(user: nil, key: nil)
+        @user = user
+        @key = key
+
+        @user = key.user if !user && key
+      end
+
+      def self.from_params(params)
+        if params[:key_id]
+          new(key: Key.find_by_id(params[:key_id]))
+        elsif params[:user_id]
+          new(user: UserFinder.new(params[:user_id]).find_by_id)
+        elsif params[:username]
+          new(user: UserFinder.new(params[:username]).find_by_username)
+        end
+      end
+
+      def key_or_user
+        key || user
+      end
+
+      def username
+        user&.username
+      end
+
+      def update_last_used_at!
+        key&.update_last_used_at
+      end
+
+      private
+
+      attr_reader :key
+    end
+  end
+end

spec/lib/api/support/git_access_actor_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Support::GitAccessActor do
+  let(:user) { nil }
+  let(:key) { nil }
+
+  subject { described_class.new(user: user, key: key) }
+
+  describe '.from_params' do
+    context 'with params that are valid' do
+      it 'returns an instance of API::Support::GitAccessActor' do
+        params = { key_id: create(:key).id }
+
+        expect(described_class.from_params(params)).to be_instance_of(described_class)
+      end
+    end
+
+    context 'with params that are invalid' do
+      it 'returns nil' do
+        expect(described_class.from_params({})).to be_nil
+      end
+    end
+  end
+
+  describe 'attributes' do
+    describe '#user' do
+      context 'when initialized with a User' do
+        let(:user) { create(:user) }
+
+        it 'returns the User' do
+          expect(subject.user).to eq(user)
+        end
+      end
+
+      context 'when initialized with a Key' do
+        let(:user_for_key) { create(:user) }
+        let(:key) { create(:key, user: user_for_key) }
+
+        it 'returns the User associated to the Key' do
+          expect(subject.user).to eq(user_for_key)
+        end
+      end
+    end
+  end
+
+  describe '#key_or_user' do
+    context 'when params contains a :key_id' do
+      it 'is an instance of Key' do
+        key = create(:key)
+        params = { key_id: key.id }
+
+        expect(described_class.from_params(params).key_or_user).to eq(key)
+      end
+    end
+
+    context 'when params contains a :user_id' do
+      it 'is an instance of User' do
+        user = create(:user)
+        params = { user_id: user.id }
+
+        expect(described_class.from_params(params).key_or_user).to eq(user)
+      end
+    end
+
+    context 'when params contains a :username' do
+      it 'is an instance of User (via UserFinder)' do
+        user = create(:user)
+        params = { username: user.username }
+
+        expect(described_class.from_params(params).key_or_user).to eq(user)
+      end
+    end
+  end
+
+  describe '#username' do
+    context 'when initialized with a User' do
+      let(:user) { create(:user) }
+
+      it 'returns the username' do
+        expect(subject.username).to eq(user.username)
+      end
+    end
+
+    context 'when initialized with a Key' do
+      let(:key) { create(:key, user: user_for_key) }
+
+      context 'that has no User associated' do
+        let(:user_for_key) { nil }
+
+        it 'returns nil' do
+          expect(subject.username).to be_nil
+        end
+      end
+
+      context 'that has a User associated' do
+        let(:user_for_key) { create(:user) }
+
+        it 'returns the username of the User associated to the Key' do
+          expect(subject.username).to eq(user_for_key.username)
+        end
+      end
+    end
+  end
+
+  describe '#update_last_used_at!' do
+    context 'when initialized with a User' do
+      let(:user) { create(:user) }
+
+      it 'does nothing' do
+        expect(user).not_to receive(:update_last_used_at)
+
+        subject.update_last_used_at!
+      end
+    end
+
+    context 'when initialized with a Key' do
+      let(:key) { create(:key) }
+
+      it 'updates update_last_used_at' do
+        expect(key).to receive(:update_last_used_at)
+
+        subject.update_last_used_at!
+      end
+    end
+  end
+end