summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@mcgivern.me.uk>2016-12-13 18:58:23 +0000
committerRémy Coutable <remy@rymai.me>2016-12-14 14:38:59 +0100
commit0dcb587a2f437d643d034a7cac36ce14c9fffb9a (patch)
treeb746bc767dd61ca086549279001409ab0a8b0e73
parent1747798df0bfe50a73617f82f4cac2af4cb073cf (diff)
downloadgitlab-ce-0dcb587a2f437d643d034a7cac36ce14c9fffb9a.tar.gz
Merge branch '25482-fix-api-sudo' into 'master'
API: Memoize the current_user so that the sudo can work properly Closes #25482 See merge request !8017 Signed-off-by: Rémy Coutable <remy@rymai.me>
-rw-r--r--CHANGELOG3
-rw-r--r--app/models/user.rb5
-rw-r--r--changelogs/unreleased/25482-fix-api-sudo.yml4
-rw-r--r--lib/api/helpers.rb130
-rw-r--r--lib/api/users.rb2
-rw-r--r--spec/models/user_spec.rb96
-rw-r--r--spec/requests/api/helpers_spec.rb (renamed from spec/requests/api/api_helpers_spec.rb)85
-rw-r--r--spec/requests/api/users_spec.rb27
8 files changed, 246 insertions, 106 deletions
diff --git a/CHANGELOG b/CHANGELOG
index 60deb3979ce..880192943c0 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,5 +1,8 @@
Please view this file on the master branch, on stable branches it's out of date.
+v 8.12.13
+ - API: Memoize the current_user so that the sudo can work properly. !8017
+
v 8.12.12
- Replace MR access checks with use of MergeRequestsFinder
- Reenables /user API request to return private-token if user is admin and request is made with sudo
diff --git a/app/models/user.rb b/app/models/user.rb
index 8424cce76a2..5f98212bc3e 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -275,8 +275,9 @@ 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)
+ # 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))
end
def build_user(attrs = {})
diff --git a/changelogs/unreleased/25482-fix-api-sudo.yml b/changelogs/unreleased/25482-fix-api-sudo.yml
new file mode 100644
index 00000000000..4c11fe1622e
--- /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 ec26e079370..9ee7591ff02 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -12,68 +12,37 @@ module API
nil
end
- 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)
+ def declared_params(options = {})
+ options = { include_parent_namespaces: false }.merge(options)
+ declared(params, options).to_h.symbolize_keys
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
@project ||= find_project(params[:id])
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)
project = Project.find_with_namespace(id) || Project.find_by(id: id)
@@ -401,6 +370,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
@@ -433,10 +465,6 @@ module API
links.join(', ')
end
- def private_token_used?
- private_token == @current_user.private_token
- end
-
def secret_token
File.read(Gitlab.config.gitlab_shell.secret_file).chomp
end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 55f9adabf9a..07df21d4078 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -327,7 +327,7 @@ module API
# Example Request:
# GET /user
get do
- present @current_user, with: @impersonator ? Entities::UserWithPrivateToken : Entities::UserPublic
+ present current_user, with: sudo? ? Entities::UserWithPrivateToken : Entities::UserPublic
end
# Get currently authenticated user's keys
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index a1770d96f83..3507c3dcbe3 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -599,14 +599,94 @@ 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
+ describe '.search_with_secondary_emails' do
+ def search_with_secondary_emails(query)
+ described_class.search_with_secondary_emails(query)
+ end
+
+ let!(:user) { create(:user) }
+ let!(:email) { create(:email) }
+
+ it 'returns users with a matching name' do
+ expect(search_with_secondary_emails(user.name)).to eq([user])
+ end
+
+ it 'returns users with a partially matching name' do
+ expect(search_with_secondary_emails(user.name[0..2])).to eq([user])
+ end
+
+ it 'returns users with a matching name regardless of the casing' do
+ expect(search_with_secondary_emails(user.name.upcase)).to eq([user])
+ end
+
+ it 'returns users with a matching email' do
+ expect(search_with_secondary_emails(user.email)).to eq([user])
+ end
+
+ it 'returns users with a partially matching email' do
+ expect(search_with_secondary_emails(user.email[0..2])).to eq([user])
+ end
+
+ it 'returns users with a matching email regardless of the casing' do
+ expect(search_with_secondary_emails(user.email.upcase)).to eq([user])
+ end
+
+ it 'returns users with a matching username' do
+ expect(search_with_secondary_emails(user.username)).to eq([user])
+ end
+
+ it 'returns users with a partially matching username' do
+ expect(search_with_secondary_emails(user.username[0..2])).to eq([user])
+ end
+
+ it 'returns users with a matching username regardless of the casing' do
+ expect(search_with_secondary_emails(user.username.upcase)).to eq([user])
+ end
+
+ it 'returns users with a matching whole secondary email' do
+ expect(search_with_secondary_emails(email.email)).to eq([email.user])
+ end
+
+ it 'returns users with a matching part of secondary email' do
+ expect(search_with_secondary_emails(email.email[1..4])).to eq([email.user])
+ end
+
+ it 'return users with a matching part of secondary email regardless of case' do
+ expect(search_with_secondary_emails(email.email[1..4].upcase)).to eq([email.user])
+ expect(search_with_secondary_emails(email.email[1..4].downcase)).to eq([email.user])
+ expect(search_with_secondary_emails(email.email[1..4].capitalize)).to eq([email.user])
+ end
+
+ it 'returns multiple users with matching secondary emails' do
+ email1 = create(:email, email: '1_testemail@example.com')
+ email2 = create(:email, email: '2_testemail@example.com')
+ email3 = create(:email, email: 'other@email.com')
+ email3.user.update_attributes!(email: 'another@mail.com')
+
+ expect(
+ search_with_secondary_emails('testemail@example.com').map(&:id)
+ ).to include(email1.user.id, email2.user.id)
+
+ expect(
+ search_with_secondary_emails('testemail@example.com').map(&:id)
+ ).not_to include(email3.user.id)
+ end
+ 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) }
+
+ it 'returns the corresponding User' do
+ expect(described_class.find_by_ssh_key_id(key.id)).to eq(user)
+ end
+ 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
+ end
end
end
diff --git a/spec/requests/api/api_helpers_spec.rb b/spec/requests/api/helpers_spec.rb
index 91c4ac2a648..511853d0b1f 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 ffa70dc4901..21b9b1801bf 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -597,13 +597,12 @@ describe API::API, 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
@@ -611,7 +610,7 @@ describe API::API, 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
@@ -622,40 +621,44 @@ describe API::API, 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