diff --git a/changelogs/unreleased/25482-fix-api-sudo.yml b/changelogs/unreleased/25482-fix-api-sudo.yml
new file mode 100644
index 0000000000000000000000000000000000000000..3b23bfd3a210c88e81548e99153921b925e33aa6
--- /dev/null
+++ b/changelogs/unreleased/25482-fix-api-sudo.yml
@@ -0,0 +1,4 @@
+---
+title: 'API: Memoize the current_user so that the sudo can work properly'
+merge_request: 8017
+author:
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 8b0f8deadfa6d7d77104f0302418841cc0993957..2041f0dac6b0ad23f872e0edd120795aa16a682c 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
-
- identifier = sudo_identifier
+ return @current_user if defined?(@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?
+ @current_user = initial_current_user
- @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
@@ -354,6 +310,79 @@ 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.is_a?(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 = User.by_username_or_id(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
+ 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
+ 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
@@ -386,10 +415,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/requests/api/api_helpers_spec.rb b/spec/requests/api/helpers_spec.rb
similarity index 86%
rename from spec/requests/api/api_helpers_spec.rb
rename to spec/requests/api/helpers_spec.rb
index 3f34309f419c0ce4b5ed7042b79c29de6f082a12..573154f69b4d47fe5c72ca0bb83c69209f9b6a1a 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,17 +12,17 @@ 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::PRIVATE_TOKEN_HEADER] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token
env[API::Helpers::SUDO_HEADER] = identifier
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::PRIVATE_TOKEN_PARAM] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token
params[API::Helpers::SUDO_PARAM] = identifier
end
@@ -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