Be smarter when finding a sudoed user in API::Helpers25482-fix-api-sudo

Signed-off-by: Rémy Coutable <remy@rymai.me>

5 files changed, 15 insertions, 32 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index b9bb4a9e3f7..1bd28203523 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -304,10 +304,6 @@ class User < ActiveRecord::Base
       personal_access_token.user if personal_access_token
     end
 
-    def by_username_or_id(name_or_id)
-      find_by('users.username = ? OR users.id = ?', name_or_id.to_s, name_or_id.to_i)
-    end
-
     # Returns a user for the given SSH key.
     def find_by_ssh_key_id(key_id)
       find_by(id: Key.unscoped.select(:user_id).where(id: key_id))
diff --git a/changelogs/unreleased/25482-fix-api-sudo.yml b/changelogs/unreleased/25482-fix-api-sudo.yml
index 3b23bfd3a21..4c11fe1622e 100644
--- a/changelogs/unreleased/25482-fix-api-sudo.yml
+++ b/changelogs/unreleased/25482-fix-api-sudo.yml
@@ -1,4 +1,4 @@
 ---
-title: 'API: Memoize the current_user so that the sudo can work properly'
+title: 'API: Memoize the current_user so that sudo can work properly'
 merge_request: 8017
-author: 
+author:
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 2041f0dac6b..8260fcab80a 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -34,6 +34,14 @@ module API
       @available_labels ||= LabelsFinder.new(current_user, project_id: user_project.id).execute
     end
 
+    def find_user(id)
+      if id =~ /^\d+$/
+        User.find_by(id: id)
+      else
+        User.find_by(username: id)
+      end
+    end
+
     def find_project(id)
       if id =~ /^\d+$/
         Project.find_by(id: id)
@@ -349,7 +357,7 @@ module API
 
     def sudo!
       return unless sudo_identifier
-      return unless initial_current_user.is_a?(User)
+      return unless initial_current_user
 
       unless initial_current_user.is_admin?
         forbidden!('Must be admin to use sudo')
@@ -360,7 +368,7 @@ module API
         forbidden!('Private token must be specified in order to use sudo')
       end
 
-      sudoed_user = User.by_username_or_id(sudo_identifier)
+      sudoed_user = find_user(sudo_identifier)
 
       if sudoed_user
         @current_user = sudoed_user
@@ -370,17 +378,7 @@ module API
     end
 
     def sudo_identifier
-      return @sudo_identifier if defined?(@sudo_identifier)
-
-      identifier ||= params[SUDO_PARAM] || env[SUDO_HEADER]
-
-      # Regex for integers
-      @sudo_identifier =
-        if !!(identifier =~ /\A[0-9]+\z/)
-          identifier.to_i
-        else
-          identifier
-        end
+      @sudo_identifier ||= params[SUDO_PARAM] || env[SUDO_HEADER]
     end
 
     def add_pagination_headers(paginated_data)
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index bad6ed9e146..8b20ee81614 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -727,17 +727,6 @@ describe User, models: true do
     end
   end
 
-  describe 'by_username_or_id' do
-    let(:user1) { create(:user, username: 'foo') }
-
-    it "gets the correct user" do
-      expect(User.by_username_or_id(user1.id)).to eq(user1)
-      expect(User.by_username_or_id('foo')).to eq(user1)
-      expect(User.by_username_or_id(-1)).to be_nil
-      expect(User.by_username_or_id('bar')).to be_nil
-    end
-  end
-
   describe '.find_by_ssh_key_id' do
     context 'using an existing SSH key ID' do
       let(:user) { create(:user) }
diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb
index 573154f69b4..4035fd97af5 100644
--- a/spec/requests/api/helpers_spec.rb
+++ b/spec/requests/api/helpers_spec.rb
@@ -16,14 +16,14 @@ describe API::Helpers, api: true do
     clear_env
     clear_param
     env[API::Helpers::PRIVATE_TOKEN_HEADER] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token
-    env[API::Helpers::SUDO_HEADER] = identifier
+    env[API::Helpers::SUDO_HEADER] = identifier.to_s
   end
 
   def set_param(user_or_token, identifier)
     clear_env
     clear_param
     params[API::Helpers::PRIVATE_TOKEN_PARAM] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token
-    params[API::Helpers::SUDO_PARAM] = identifier
+    params[API::Helpers::SUDO_PARAM] = identifier.to_s
   end
 
   def clear_env