Commit ba047489 authored by Małgorzata Ksionek's avatar Małgorzata Ksionek

Remove impersonation token from api response for non-admin user

Changelog: security
parent cec85a0a
# frozen_string_literal: true # frozen_string_literal: true
class PersonalAccessTokenPolicy < BasePolicy class PersonalAccessTokenPolicy < BasePolicy
condition(:is_owner) { user && subject.user_id == user.id } condition(:is_owner) { user && subject.user_id == user.id && !subject.impersonation }
rule { (is_owner | admin) & ~blocked }.policy do rule { (is_owner | admin) & ~blocked }.policy do
enable :read_token enable :read_token
......
...@@ -23,7 +23,7 @@ module API ...@@ -23,7 +23,7 @@ module API
helpers do helpers do
def finder_params(current_user) def finder_params(current_user)
current_user.admin? ? { user: user(params[:user_id]) } : { user: current_user } current_user.admin? ? { user: user(params[:user_id]) } : { user: current_user, impersonation: false }
end end
def user(user_id) def user(user_id)
......
...@@ -41,6 +41,13 @@ RSpec.describe PersonalAccessTokenPolicy do ...@@ -41,6 +41,13 @@ RSpec.describe PersonalAccessTokenPolicy do
it { is_expected.to be_allowed(:read_token) } it { is_expected.to be_allowed(:read_token) }
it { is_expected.to be_allowed(:revoke_token) } it { is_expected.to be_allowed(:revoke_token) }
end end
context 'subject of the impersonated token' do
let_it_be(:token) { build_stubbed(:personal_access_token, user: current_user, impersonation: true) }
it { is_expected.to be_disallowed(:read_token) }
it { is_expected.to be_disallowed(:revoke_token) }
end
end end
context 'current_user is a blocked administrator', :enable_admin_mode do context 'current_user is a blocked administrator', :enable_admin_mode do
......
...@@ -6,6 +6,7 @@ RSpec.describe API::PersonalAccessTokens do ...@@ -6,6 +6,7 @@ RSpec.describe API::PersonalAccessTokens do
let_it_be(:path) { '/personal_access_tokens' } let_it_be(:path) { '/personal_access_tokens' }
let_it_be(:token1) { create(:personal_access_token) } let_it_be(:token1) { create(:personal_access_token) }
let_it_be(:token2) { create(:personal_access_token) } let_it_be(:token2) { create(:personal_access_token) }
let_it_be(:token_impersonated) { create(:personal_access_token, impersonation: true, user: token1.user) }
let_it_be(:current_user) { create(:user) } let_it_be(:current_user) { create(:user) }
describe 'GET /personal_access_tokens' do describe 'GET /personal_access_tokens' do
...@@ -24,8 +25,9 @@ RSpec.describe API::PersonalAccessTokens do ...@@ -24,8 +25,9 @@ RSpec.describe API::PersonalAccessTokens do
get api(path, current_user), params: { user_id: token1.user.id } get api(path, current_user), params: { user_id: token1.user.id }
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
expect(json_response.count).to eq(1) expect(json_response.count).to eq(2)
expect(json_response.first['user_id']).to eq(token1.user.id) expect(json_response.first['user_id']).to eq(token1.user.id)
expect(json_response.last['id']).to eq(token_impersonated.id)
end end
end end
...@@ -34,6 +36,7 @@ RSpec.describe API::PersonalAccessTokens do ...@@ -34,6 +36,7 @@ RSpec.describe API::PersonalAccessTokens do
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let_it_be(:token) { create(:personal_access_token, user: current_user)} let_it_be(:token) { create(:personal_access_token, user: current_user)}
let_it_be(:other_token) { create(:personal_access_token, user: user) } let_it_be(:other_token) { create(:personal_access_token, user: user) }
let_it_be(:token_impersonated) { create(:personal_access_token, impersonation: true, user: current_user) }
it 'returns all PATs belonging to the signed-in user' do it 'returns all PATs belonging to the signed-in user' do
get api(path, current_user, personal_access_token: token) get api(path, current_user, personal_access_token: token)
...@@ -95,6 +98,7 @@ RSpec.describe API::PersonalAccessTokens do ...@@ -95,6 +98,7 @@ RSpec.describe API::PersonalAccessTokens do
context 'when current_user is not an administrator' do context 'when current_user is not an administrator' do
let_it_be(:user_token) { create(:personal_access_token, user: current_user) } let_it_be(:user_token) { create(:personal_access_token, user: current_user) }
let_it_be(:user_token_path) { "/personal_access_tokens/#{user_token.id}" } let_it_be(:user_token_path) { "/personal_access_tokens/#{user_token.id}" }
let_it_be(:token_impersonated) { create(:personal_access_token, impersonation: true, user: current_user) }
it 'fails revokes a different users token' do it 'fails revokes a different users token' do
delete api(path, current_user) delete api(path, current_user)
...@@ -107,6 +111,12 @@ RSpec.describe API::PersonalAccessTokens do ...@@ -107,6 +111,12 @@ RSpec.describe API::PersonalAccessTokens do
expect(response).to have_gitlab_http_status(:no_content) expect(response).to have_gitlab_http_status(:no_content)
end end
it 'cannot revoke impersonation token' do
delete api("/personal_access_tokens/#{token_impersonated.id}", current_user)
expect(response).to have_gitlab_http_status(:bad_request)
end
end end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment