Merge branch 'bvl-remove-user-id-discover-from-internal-api' into 'master'

Remove user_id param from the discover endpoint in the internal api

Closes #33647

See merge request gitlab-org/gitlab!18277

app/finders/user_finder.rb

@@ -52,6 +52,12 @@ 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

app/models/user.rb

@@ -533,7 +533,7 @@ class User < ApplicationRecord
 
     # Returns a user for the given SSH key.
     def find_by_ssh_key_id(key_id)
-      Key.find_by(id: key_id)&.user
+      find_by('EXISTS (?)', Key.select(1).where('keys.user_id = users.id').where(id: key_id))
     end
 
     def find_by_full_path(path, follow_redirects: false)

doc/development/internal_api.md

@@ -159,7 +159,6 @@ discovers the user associated with an SSH key.
 |:----------|:-------|:---------|:------------|
 | `key_id` | integer | no | The id of the SSH key used as found in the authorized-keys file or through the `/authorized_keys` check |
 | `username` | string | no | Username of the user being looked up, used by GitLab-shell when authenticating using a certificate |
-| `user_id` | integer | no | **Deprecated** User_id of the user being looked up |
 
 ```
 GET /internal/discover

lib/api/internal/base.rb

@@ -129,20 +129,15 @@ module API
         #
         # Discover user by ssh key, user id or username
         #
-        # rubocop: disable CodeReuse/ActiveRecord
-        get "/discover" do
+        get '/discover' do
           if params[:key_id]
-            key = Key.find(params[:key_id])
-            user = key.user
-          elsif params[:user_id]
-            user = User.find_by(id: params[:user_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
         end
-        # rubocop: enable CodeReuse/ActiveRecord
 
         get "/check" do
           {

spec/finders/user_finder_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe UserFinder do
-  set(:user) { create(:user) }
+  let_it_be(:user) { create(:user) }
 
   describe '#find_by_id' do
     context 'when the user exists' do
@@ -24,7 +24,7 @@ describe UserFinder do
 
     context 'when the user does not exist' do
       it 'returns nil' do
-        found = described_class.new(1).find_by_id
+        found = described_class.new(-1).find_by_id
 
         expect(found).to be_nil
       end
@@ -84,7 +84,7 @@ describe UserFinder do
 
     context 'when the user does not exist' do
       it 'returns nil' do
-        found = described_class.new(1).find_by_id_or_username
+        found = described_class.new(-1).find_by_id_or_username
 
         expect(found).to be_nil
       end
@@ -110,7 +110,7 @@ describe UserFinder do
 
     context 'when the user does not exist' do
       it 'raises ActiveRecord::RecordNotFound' do
-        finder = described_class.new(1)
+        finder = described_class.new(-1)
 
         expect { finder.find_by_id! }.to raise_error(ActiveRecord::RecordNotFound)
       end
@@ -170,10 +170,32 @@ describe UserFinder do
 
     context 'when the user does not exist' do
       it 'raises ActiveRecord::RecordNotFound' do
-        finder = described_class.new(1)
+        finder = described_class.new(-1)
 
         expect { finder.find_by_id_or_username! }.to raise_error(ActiveRecord::RecordNotFound)
       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/models/user_spec.rb

@@ -1554,15 +1554,22 @@ describe User do
   end
 
   describe '.find_by_ssh_key_id' do
-    context 'using an existing SSH key ID' do
-      let(:user) { create(:user) }
-      let(:key) { create(:key, user: user) }
+    let_it_be(:user) { create(:user) }
+    let_it_be(:key) { create(:key, user: user) }
 
+    context 'using an existing SSH key ID' do
       it 'returns the corresponding User' do
         expect(described_class.find_by_ssh_key_id(key.id)).to eq(user)
       end
     end
 
+    it 'only performs a single query' do
+      key # Don't count the queries for creating the key and user
+
+      expect { described_class.find_by_ssh_key_id(key.id) }
+        .not_to exceed_query_limit(1)
+    end
+
     context 'using an invalid SSH key ID' do
       it 'returns nil' do
         expect(described_class.find_by_ssh_key_id(-1)).to be_nil

spec/requests/api/internal/base_spec.rb

@@ -237,14 +237,6 @@ describe API::Internal::Base do
       expect(json_response['name']).to eq(user.name)
     end
 
-    it "finds a user by user id" do
-      get(api("/internal/discover"), params: { user_id: user.id, secret_token: secret_token })
-
-      expect(response).to have_gitlab_http_status(200)
-
-      expect(json_response['name']).to eq(user.name)
-    end
-
     it "finds a user by username" do
       get(api("/internal/discover"), params: { username: user.username, secret_token: secret_token })