diff --git a/app/models/user.rb b/app/models/user.rb
index b9bb4a9e3f7e98ec24e216f28bb8c0463cb45f5c..1bd28203523f460de46f88d6a04d8922b6b773c1 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
new file mode 100644
index 0000000000000000000000000000000000000000..4c11fe1622eb5ad3d2ce499e3936267b46e2db16
--- /dev/null
+++ b/changelogs/unreleased/25482-fix-api-sudo.yml
@@ -0,0 +1,4 @@
+---
+title: 'API: Memoize the current_user so that sudo can work properly'
+merge_request: 8017
+author:
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index f03d8da732e05a7a244b371d12ab525977dcf061..746849ef4c01d75bd12adfc9e9554f733154ff62 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -7,67 +7,23 @@ module API
SUDO_HEADER = "HTTP_SUDO"
SUDO_PARAM = :sudo
- def private_token
- params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]
- end
-
- def warden
- env['warden']
- end
-
- # Check the Rails session for valid authentication details
- #
- # Until CSRF protection is added to the API, disallow this method for
- # state-changing endpoints
- def find_user_from_warden
- warden.try(:authenticate) if %w[GET HEAD].include?(env['REQUEST_METHOD'])
- end
-
def declared_params(options = {})
options = { include_parent_namespaces: false }.merge(options)
declared(params, options).to_h.symbolize_keys
end
- def find_user_by_private_token
- token = private_token
- return nil unless token.present?
-
- User.find_by_authentication_token(token) || User.find_by_personal_access_token(token)
- end
-
def current_user
- @current_user ||= find_user_by_private_token
- @current_user ||= doorkeeper_guard
- @current_user ||= find_user_from_warden
-
- unless @current_user && Gitlab::UserAccess.new(@current_user).allowed?
- return nil
- end
+ return @current_user if defined?(@current_user)
- identifier = sudo_identifier
+ @current_user = initial_current_user
- if identifier
- # We check for private_token because we cannot allow PAT to be used
- forbidden!('Must be admin to use sudo') unless @current_user.is_admin?
- forbidden!('Private token must be specified in order to use sudo') unless private_token_used?
-
- @impersonator = @current_user
- @current_user = User.by_username_or_id(identifier)
- not_found!("No user id or username for: #{identifier}") if @current_user.nil?
- end
+ sudo!
@current_user
end
- def sudo_identifier
- identifier ||= params[SUDO_PARAM] || env[SUDO_HEADER]
-
- # Regex for integers
- if !!(identifier =~ /\A[0-9]+\z/)
- identifier.to_i
- else
- identifier
- end
+ def sudo?
+ initial_current_user != current_user
end
def user_project
@@ -78,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)
@@ -343,6 +307,69 @@ module API
private
+ def private_token
+ params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]
+ end
+
+ def warden
+ env['warden']
+ end
+
+ # Check the Rails session for valid authentication details
+ #
+ # Until CSRF protection is added to the API, disallow this method for
+ # state-changing endpoints
+ def find_user_from_warden
+ warden.try(:authenticate) if %w[GET HEAD].include?(env['REQUEST_METHOD'])
+ end
+
+ def find_user_by_private_token
+ token = private_token
+ return nil unless token.present?
+
+ User.find_by_authentication_token(token) || User.find_by_personal_access_token(token)
+ end
+
+ def initial_current_user
+ return @initial_current_user if defined?(@initial_current_user)
+
+ @initial_current_user ||= find_user_by_private_token
+ @initial_current_user ||= doorkeeper_guard
+ @initial_current_user ||= find_user_from_warden
+
+ unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed?
+ @initial_current_user = nil
+ end
+
+ @initial_current_user
+ end
+
+ def sudo!
+ return unless sudo_identifier
+ return unless initial_current_user
+
+ unless initial_current_user.is_admin?
+ forbidden!('Must be admin to use sudo')
+ end
+
+ # Only private tokens should be used for the SUDO feature
+ unless private_token == initial_current_user.private_token
+ forbidden!('Private token must be specified in order to use sudo')
+ end
+
+ sudoed_user = find_user(sudo_identifier)
+
+ if sudoed_user
+ @current_user = sudoed_user
+ else
+ not_found!("No user id or username for: #{sudo_identifier}")
+ end
+ end
+
+ def sudo_identifier
+ @sudo_identifier ||= params[SUDO_PARAM] || env[SUDO_HEADER]
+ end
+
def add_pagination_headers(paginated_data)
header 'X-Total', paginated_data.total_count.to_s
header 'X-Total-Pages', paginated_data.total_pages.to_s
@@ -375,10 +402,6 @@ module API
links.join(', ')
end
- def private_token_used?
- private_token == @current_user.private_token
- end
-
def secret_token
Gitlab::Shell.secret_token
end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 1dab799dd61e6fc2ba42d11f3b1193d8db7cd7c2..c7db2d7101715d324dcb8ba82c71a32e5a9dd9b0 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -353,7 +353,7 @@ module API
success Entities::UserPublic
end
get do
- present current_user, with: @impersonator ? Entities::UserWithPrivateToken : Entities::UserPublic
+ present current_user, with: sudo? ? Entities::UserWithPrivateToken : Entities::UserPublic
end
desc "Get the currently authenticated user's SSH keys" do
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index bad6ed9e146f963724912c5670a0919f0ffab5d1..8b20ee816140831c2dbd75bf4793412073722643 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/api_helpers_spec.rb b/spec/requests/api/helpers_spec.rb
similarity index 85%
rename from spec/requests/api/api_helpers_spec.rb
rename to spec/requests/api/helpers_spec.rb
index 3f34309f419c0ce4b5ed7042b79c29de6f082a12..4035fd97af59a52ff0eafccb8bfaf25d12774912 100644
--- a/spec/requests/api/api_helpers_spec.rb
+++ b/spec/requests/api/helpers_spec.rb
@@ -2,7 +2,6 @@ require 'spec_helper'
describe API::Helpers, api: true do
include API::Helpers
- include ApiHelpers
include SentryHelper
let(:user) { create(:user) }
@@ -13,18 +12,18 @@ describe API::Helpers, api: true do
let(:env) { { 'REQUEST_METHOD' => 'GET' } }
let(:request) { Rack::Request.new(env) }
- def set_env(token_usr, identifier)
+ def set_env(user_or_token, identifier)
clear_env
clear_param
- env[API::Helpers::PRIVATE_TOKEN_HEADER] = token_usr.private_token
- env[API::Helpers::SUDO_HEADER] = identifier
+ 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.to_s
end
- def set_param(token_usr, identifier)
+ def set_param(user_or_token, identifier)
clear_env
clear_param
- params[API::Helpers::PRIVATE_TOKEN_PARAM] = token_usr.private_token
- params[API::Helpers::SUDO_PARAM] = identifier
+ 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.to_s
end
def clear_env
@@ -163,6 +162,13 @@ describe API::Helpers, api: true do
expect(current_user).to eq(user)
end
+ it 'memoize the current_user: sudo permissions are not run against the sudoed user' do
+ set_env(admin, user.id)
+
+ expect(current_user).to eq(user)
+ expect(current_user).to eq(user)
+ end
+
it 'handles sudo to oneself' do
set_env(admin, admin.id)
@@ -294,33 +300,48 @@ describe API::Helpers, api: true do
end
end
- describe '.sudo_identifier' do
- it "returns integers when input is an int" do
- set_env(admin, '123')
- expect(sudo_identifier).to eq(123)
- set_env(admin, '0001234567890')
- expect(sudo_identifier).to eq(1234567890)
-
- set_param(admin, '123')
- expect(sudo_identifier).to eq(123)
- set_param(admin, '0001234567890')
- expect(sudo_identifier).to eq(1234567890)
+ describe '.sudo?' do
+ context 'when no sudo env or param is passed' do
+ before do
+ doorkeeper_guard_returns(nil)
+ end
+
+ it 'returns false' do
+ expect(sudo?).to be_falsy
+ end
end
- it "returns string when input is an is not an int" do
- set_env(admin, '12.30')
- expect(sudo_identifier).to eq("12.30")
- set_env(admin, 'hello')
- expect(sudo_identifier).to eq('hello')
- set_env(admin, ' 123')
- expect(sudo_identifier).to eq(' 123')
-
- set_param(admin, '12.30')
- expect(sudo_identifier).to eq("12.30")
- set_param(admin, 'hello')
- expect(sudo_identifier).to eq('hello')
- set_param(admin, ' 123')
- expect(sudo_identifier).to eq(' 123')
+ context 'when sudo env or param is passed', 'user is not an admin' do
+ before do
+ set_env(user, '123')
+ end
+
+ it 'returns an 403 Forbidden' do
+ expect { sudo? }.to raise_error '403 - {"message"=>"403 Forbidden - Must be admin to use sudo"}'
+ end
+ end
+
+ context 'when sudo env or param is passed', 'user is admin' do
+ context 'personal access token is used' do
+ before do
+ personal_access_token = create(:personal_access_token, user: admin)
+ set_env(personal_access_token.token, user.id)
+ end
+
+ it 'returns an 403 Forbidden' do
+ expect { sudo? }.to raise_error '403 - {"message"=>"403 Forbidden - Private token must be specified in order to use sudo"}'
+ end
+ end
+
+ context 'private access token is used' do
+ before do
+ set_env(admin.private_token, user.id)
+ end
+
+ it 'returns true' do
+ expect(sudo?).to be_truthy
+ end
+ end
end
end
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index c37dbfa0a3350bd5ada1a6d31ca08bf07e6f4ba3..9e317f3a7e91498abf37ebff32771bdcc972fdf5 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -651,13 +651,12 @@ describe API::Users, api: true do
end
describe "GET /user" do
- let(:personal_access_token) { create(:personal_access_token, user: user) }
- let(:private_token) { user.private_token }
+ let(:personal_access_token) { create(:personal_access_token, user: user).token }
context 'with regular user' do
context 'with personal access token' do
it 'returns 403 without private token when sudo is defined' do
- get api("/user?private_token=#{personal_access_token.token}&sudo=#{user.id}")
+ get api("/user?private_token=#{personal_access_token}&sudo=123")
expect(response).to have_http_status(403)
end
@@ -665,7 +664,7 @@ describe API::Users, api: true do
context 'with private token' do
it 'returns 403 without private token when sudo defined' do
- get api("/user?private_token=#{private_token}&sudo=#{user.id}")
+ get api("/user?private_token=#{user.private_token}&sudo=123")
expect(response).to have_http_status(403)
end
@@ -676,40 +675,44 @@ describe API::Users, api: true do
expect(response).to have_http_status(200)
expect(response).to match_response_schema('user/public')
+ expect(json_response['id']).to eq(user.id)
end
end
context 'with admin' do
- let(:user) { create(:admin) }
+ let(:admin_personal_access_token) { create(:personal_access_token, user: admin).token }
context 'with personal access token' do
it 'returns 403 without private token when sudo defined' do
- get api("/user?private_token=#{personal_access_token.token}&sudo=#{user.id}")
+ get api("/user?private_token=#{admin_personal_access_token}&sudo=#{user.id}")
expect(response).to have_http_status(403)
end
- it 'returns current user without private token when sudo not defined' do
- get api("/user?private_token=#{personal_access_token.token}")
+ it 'returns initial current user without private token when sudo not defined' do
+ get api("/user?private_token=#{admin_personal_access_token}")
expect(response).to have_http_status(200)
expect(response).to match_response_schema('user/public')
+ expect(json_response['id']).to eq(admin.id)
end
end
context 'with private token' do
- it 'returns current user with private token when sudo defined' do
- get api("/user?private_token=#{private_token}&sudo=#{user.id}")
+ it 'returns sudoed user with private token when sudo defined' do
+ get api("/user?private_token=#{admin.private_token}&sudo=#{user.id}")
expect(response).to have_http_status(200)
expect(response).to match_response_schema('user/login')
+ expect(json_response['id']).to eq(user.id)
end
- it 'returns current user without private token when sudo not defined' do
- get api("/user?private_token=#{private_token}")
+ it 'returns initial current user without private token when sudo not defined' do
+ get api("/user?private_token=#{admin.private_token}")
expect(response).to have_http_status(200)
expect(response).to match_response_schema('user/public')
+ expect(json_response['id']).to eq(admin.id)
end
end
end