Merge branch 'bvl-unify-internal-api-actor' into 'master'

Unify finding an actor in the internal API

See merge request gitlab-org/gitlab!20788

app/finders/user_finder.rb

@@ -52,12 +52,6 @@ class UserFinder
     end
   end
 
-  def find_by_ssh_key_id
-    return unless input_is_id?
-
-    User.find_by_ssh_key_id(@username_or_id)
-  end
-
   def input_is_id?
     @username_or_id.is_a?(Numeric) || @username_or_id =~ /^\d+$/
   end

lib/api/helpers/internal_helpers.rb

@@ -7,6 +7,10 @@ module API
 
       delegate :wiki?, to: :repo_type
 
+      def actor
+        @actor ||= Support::GitAccessActor.from_params(params)
+      end
+
       def repo_type
         set_project unless defined?(@repo_type) # rubocop:disable Gitlab/ModuleWithInstanceVariables
         @repo_type # rubocop:disable Gitlab/ModuleWithInstanceVariables

lib/api/internal/base.rb

@@ -7,7 +7,6 @@ module API
       before { authenticate_by_gitlab_shell_token! }
 
       helpers ::API::Helpers::InternalHelpers
-      helpers ::Gitlab::Identifier
 
       UNKNOWN_CHECK_RESULT_ERROR = 'Unknown check result'.freeze
 
@@ -35,7 +34,6 @@ module API
           env = parse_env
           Gitlab::Git::HookEnv.set(gl_repository, env) if project
 
-          actor = Support::GitAccessActor.from_params(params)
           actor.update_last_used_at!
           access_checker = access_checker_for(actor, params[:protocol])
 
@@ -103,36 +101,30 @@ module API
           check_allowed(params)
         end
 
-        # rubocop: disable CodeReuse/ActiveRecord
         post "/lfs_authenticate" do
           status 200
 
-          if params[:key_id]
-            actor = Key.find(params[:key_id])
-            actor.update_last_used_at
-          elsif params[:user_id]
-            actor = User.find_by(id: params[:user_id])
-            raise ActiveRecord::RecordNotFound.new("No such user id!") unless actor
-          else
-            raise ActiveRecord::RecordNotFound.new("No key_id or user_id passed!")
+          unless actor.key_or_user
+            raise ActiveRecord::RecordNotFound.new('User not found!')
           end
 
+          actor.update_last_used_at!
+
           Gitlab::LfsToken
-            .new(actor)
+            .new(actor.key_or_user)
             .authentication_payload(lfs_authentication_url(project))
         end
-        # rubocop: enable CodeReuse/ActiveRecord
 
         #
         # Get a ssh key using the fingerprint
         #
         # rubocop: disable CodeReuse/ActiveRecord
-        get "/authorized_keys" do
+        get '/authorized_keys' do
           fingerprint = params.fetch(:fingerprint) do
             Gitlab::InsecureKeyFingerprint.new(params.fetch(:key)).fingerprint
           end
           key = Key.find_by(fingerprint: fingerprint)
-          not_found!("Key") if key.nil?
+          not_found!('Key') if key.nil?
           present key, with: Entities::SSHKey
         end
         # rubocop: enable CodeReuse/ActiveRecord
@@ -141,16 +133,10 @@ module API
         # Discover user by ssh key, user id or username
         #
         get '/discover' do
-          if params[:key_id]
-            user = UserFinder.new(params[:key_id]).find_by_ssh_key_id
-          elsif params[:username]
-            user = UserFinder.new(params[:username]).find_by_username
-          end
-
-          present user, with: Entities::UserSafe
+          present actor.user, with: Entities::UserSafe
         end
 
-        get "/check" do
+        get '/check' do
           {
             api_version: API.version,
             gitlab_version: Gitlab::VERSION,
@@ -158,35 +144,26 @@ module API
             redis: redis_ping
           }
         end
-
-        # rubocop: disable CodeReuse/ActiveRecord
         post '/two_factor_recovery_codes' do
           status 200
 
-          if params[:key_id]
-            key = Key.find_by(id: params[:key_id])
+          actor.update_last_used_at!
+          user = actor.user
 
-            if key
-              key.update_last_used_at
-            else
-              break { 'success' => false, 'message' => 'Could not find the given key' }
+          if params[:key_id]
+            unless actor.key
+              break { success: false, message: 'Could not find the given key' }
             end
 
-            if key.is_a?(DeployKey)
+            if actor.key.is_a?(DeployKey)
               break { success: false, message: 'Deploy keys cannot be used to retrieve recovery codes' }
             end
 
-            user = key.user
-
             unless user
               break { success: false, message: 'Could not find a user for the given key' }
             end
-          elsif params[:user_id]
-            user = User.find_by(id: params[:user_id])
-
-            unless user
-              break { success: false, message: 'Could not find the given user' }
-            end
+          elsif params[:user_id] && user.nil?
+            break { success: false, message: 'Could not find the given user' }
           end
 
           unless user.two_factor_enabled?
@@ -201,7 +178,6 @@ module API
 
           { success: true, recovery_codes: codes }
         end
-        # rubocop: enable CodeReuse/ActiveRecord
 
         post '/pre_receive' do
           status 200
@@ -211,7 +187,7 @@ module API
           { reference_counter_increased: reference_counter_increased }
         end
 
-        post "/notify_post_receive" do
+        post '/notify_post_receive' do
           status 200
 
           # TODO: Re-enable when Gitaly is processing the post-receive notification
@@ -229,8 +205,7 @@ module API
           status 200
 
           response = Gitlab::InternalPostReceive::Response.new
-          user = identify(params[:identifier])
-          project = Gitlab::GlRepository.parse(params[:gl_repository]).first
+          user = actor.user
           push_options = Gitlab::PushOptions.new(params[:push_options])
 
           response.reference_counter_decreased = Gitlab::ReferenceCounter.new(params[:gl_repository]).decrease

lib/api/support/git_access_actor.rb

@@ -3,7 +3,9 @@
 module API
   module Support
     class GitAccessActor
-      attr_reader :user
+      extend ::Gitlab::Identifier
+
+      attr_reader :user, :key
 
       def initialize(user: nil, key: nil)
         @user = user
@@ -19,6 +21,10 @@ module API
           new(user: UserFinder.new(params[:user_id]).find_by_id)
         elsif params[:username]
           new(user: UserFinder.new(params[:username]).find_by_username)
+        elsif params[:identifier]
+          new(user: identify(params[:identifier]))
+        else
+          new
         end
       end
 
@@ -33,10 +39,6 @@ module API
       def update_last_used_at!
         key&.update_last_used_at
       end
-
-      private
-
-      attr_reader :key
     end
   end
 end

spec/finders/user_finder_spec.rb

@@ -176,26 +176,4 @@ describe UserFinder do
       end
     end
   end
-
-  describe '#find_by_ssh_key_id' do
-    let_it_be(:ssh_key) { create(:key, user: user) }
-
-    it 'returns the user when passing the ssh key id' do
-      found = described_class.new(ssh_key.id).find_by_ssh_key_id
-
-      expect(found).to eq(user)
-    end
-
-    it 'returns the user when passing the ssh key id (string)' do
-      found = described_class.new(ssh_key.id.to_s).find_by_ssh_key_id
-
-      expect(found).to eq(user)
-    end
-
-    it 'returns nil when the id does not exist' do
-      found = described_class.new(-1).find_by_ssh_key_id
-
-      expect(found).to be_nil
-    end
-  end
 end

spec/lib/api/support/git_access_actor_spec.rb

@@ -9,17 +9,26 @@ describe API::Support::GitAccessActor do
   subject { described_class.new(user: user, key: key) }
 
   describe '.from_params' do
+    let(:key) { create(:key) }
+
     context 'with params that are valid' do
       it 'returns an instance of API::Support::GitAccessActor' do
-        params = { key_id: create(:key).id }
+        params = { key_id: 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
+      it "returns an instance of #{described_class}" do
+        expect(described_class.from_params({})).to be_instance_of(described_class)
+      end
+    end
+
+    context 'when passing an identifier used gitaly' do
+      it 'finds the user based on an identifier' do
+        expect(described_class).to receive(:identify).and_call_original
+        expect(described_class.from_params(identifier: "key-#{key.id}").user).to eq(key.user)
       end
     end
   end

spec/requests/api/internal/base_spec.rb

@@ -193,7 +193,15 @@ describe API::Internal::Base do
     end
 
     it 'responds successfully when a user is not found' do
-      get(api("/internal/discover"), params: { username: 'noone', secret_token: secret_token })
+      get(api('/internal/discover'), params: { username: 'noone', secret_token: secret_token })
+
+      expect(response).to have_gitlab_http_status(200)
+
+      expect(response.body).to eq('null')
+    end
+
+    it 'response successfully when passing invalid params' do
+      get(api('/internal/discover'), params: { nothing: 'to find a user', secret_token: secret_token })
 
       expect(response).to have_gitlab_http_status(200)
 
@@ -819,7 +827,6 @@ describe API::Internal::Base do
 
     before do
       project.add_developer(user)
-      allow(described_class).to receive(:identify).and_return(user)
       allow_any_instance_of(Gitlab::Identifier).to receive(:identify).and_return(user)
     end