From bd3a4840329160a64c0cac25ed6c1d3b22f5bdb4 Mon Sep 17 00:00:00 2001 From: Imre Farkas Date: Sat, 24 Nov 2018 13:39:16 +0100 Subject: [PATCH] Add config to disable impersonation Adds gitlab.impersonation_enabled config option defaulting to true to keep the current default behaviour. Only the act of impersonation is modified, impersonation token management is not affected. --- .../admin/impersonations_controller.rb | 13 +-- app/controllers/admin/users_controller.rb | 5 + app/controllers/application_controller.rb | 25 +++++ app/helpers/users_helper.rb | 4 + .../access_token_validation_service.rb | 6 + app/views/admin/users/_head.html.haml | 2 +- .../40385-prohibit_impersonation.yml | 5 + config/gitlab.yml.example | 3 + config/initializers/1_settings.rb | 1 + doc/api/README.md | 39 ++++++- lib/api/api_guard.rb | 6 + lib/gitlab/auth/user_auth_finders.rb | 3 + locale/gitlab.pot | 3 + .../admin/users_controller_spec.rb | 12 ++ spec/features/admin/admin_users_spec.rb | 105 ++++++++++++------ .../lib/gitlab/auth/user_auth_finders_spec.rb | 15 +++ spec/requests/api/helpers_spec.rb | 13 +++ 17 files changed, 215 insertions(+), 45 deletions(-) create mode 100644 changelogs/unreleased/40385-prohibit_impersonation.yml diff --git a/app/controllers/admin/impersonations_controller.rb b/app/controllers/admin/impersonations_controller.rb index 08d7e3b4fa2..65fe22bd8f4 100644 --- a/app/controllers/admin/impersonations_controller.rb +++ b/app/controllers/admin/impersonations_controller.rb @@ -5,23 +5,12 @@ class Admin::ImpersonationsController < Admin::ApplicationController before_action :authenticate_impersonator! def destroy - original_user = current_user - - warden.set_user(impersonator, scope: :user) - - Gitlab::AppLogger.info("User #{impersonator.username} has stopped impersonating #{original_user.username}") - - session[:impersonator_id] = nil - + original_user = stop_impersonation redirect_to admin_user_path(original_user), status: :found end private - def impersonator - @impersonator ||= User.find(session[:impersonator_id]) if session[:impersonator_id] - end - def authenticate_impersonator! render_404 unless impersonator && impersonator.admin? && !impersonator.blocked? end diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index b783c0e2a6f..e93be1c1ba2 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -2,6 +2,7 @@ class Admin::UsersController < Admin::ApplicationController before_action :user, except: [:index, :new, :create] + before_action :check_impersonation_availability, only: :impersonate def index @users = User.order_name_asc.filter(params[:filter]) @@ -227,4 +228,8 @@ class Admin::UsersController < Admin::ApplicationController result[:status] == :success end + + def check_impersonation_availability + access_denied! unless Gitlab.config.gitlab.impersonation_enabled + end end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index dbb22127e82..65c1576d9d2 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -28,6 +28,7 @@ class ApplicationController < ActionController::Base before_action :configure_permitted_parameters, if: :devise_controller? before_action :require_email, unless: :devise_controller? before_action :set_usage_stats_consent_flag + before_action :check_impersonation_availability around_action :set_locale @@ -462,4 +463,28 @@ class ApplicationController < ActionController::Base .new(settings, current_user, application_setting_params) .execute end + + def check_impersonation_availability + return unless session[:impersonator_id] + + unless Gitlab.config.gitlab.impersonation_enabled + stop_impersonation + access_denied! _('Impersonation has been disabled') + end + end + + def stop_impersonation + impersonated_user = current_user + + Gitlab::AppLogger.info("User #{impersonator.username} has stopped impersonating #{impersonated_user.username}") + + warden.set_user(impersonator, scope: :user) + session[:impersonator_id] = nil + + impersonated_user + end + + def impersonator + @impersonator ||= User.find(session[:impersonator_id]) if session[:impersonator_id] + end end diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 42b533ad772..bde9ca0cbf2 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -70,6 +70,10 @@ module UsersHelper end end + def impersonation_enabled? + Gitlab.config.gitlab.impersonation_enabled + end + private def get_profile_tabs diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb index 2a337918d21..40aa9250885 100644 --- a/app/services/access_token_validation_service.rb +++ b/app/services/access_token_validation_service.rb @@ -6,6 +6,7 @@ class AccessTokenValidationService EXPIRED = :expired REVOKED = :revoked INSUFFICIENT_SCOPE = :insufficient_scope + IMPERSONATION_DISABLED = :impersonation_disabled attr_reader :token, :request @@ -24,6 +25,11 @@ class AccessTokenValidationService elsif !self.include_any_scope?(scopes) return INSUFFICIENT_SCOPE + elsif token.respond_to?(:impersonation) && + token.impersonation && + !Gitlab.config.gitlab.impersonation_enabled + return IMPERSONATION_DISABLED + else return VALID end diff --git a/app/views/admin/users/_head.html.haml b/app/views/admin/users/_head.html.haml index bfbc16d37a0..a733f420d11 100644 --- a/app/views/admin/users/_head.html.haml +++ b/app/views/admin/users/_head.html.haml @@ -8,7 +8,7 @@ %span.cred (Admin) .float-right - - if @user != current_user && @user.can?(:log_in) + - if impersonation_enabled? && @user != current_user && @user.can?(:log_in) = link_to 'Impersonate', impersonate_admin_user_path(@user), method: :post, class: "btn btn-nr btn-grouped btn-info" = link_to edit_admin_user_path(@user), class: "btn btn-nr btn-grouped" do %i.fa.fa-pencil-square-o diff --git a/changelogs/unreleased/40385-prohibit_impersonation.yml b/changelogs/unreleased/40385-prohibit_impersonation.yml new file mode 100644 index 00000000000..dd061b17939 --- /dev/null +++ b/changelogs/unreleased/40385-prohibit_impersonation.yml @@ -0,0 +1,5 @@ +--- +title: Add config to prohibit impersonation +merge_request: 23338 +author: +type: added diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 09e21b2c6f2..58b7c248aaf 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -114,6 +114,9 @@ production: &base # The default is 'shared/cache/archive/' relative to the root of the Rails app. # repository_downloads_path: shared/cache/archive/ + ## Impersonation settings + impersonation_enabled: true + ## Reply by email # Allow users to comment on issues and merge requests by replying to notification emails. # For documentation on how to set this up, see http://doc.gitlab.com/ce/administration/reply_by_email.html diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index bd02b85c7ce..82e3b490378 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -153,6 +153,7 @@ Settings.gitlab['domain_whitelist'] ||= [] Settings.gitlab['import_sources'] ||= Gitlab::ImportSources.values Settings.gitlab['trusted_proxies'] ||= [] Settings.gitlab['no_todos_messages'] ||= YAML.load_file(Rails.root.join('config', 'no_todos_messages.yml')) +Settings.gitlab['impersonation_enabled'] ||= true if Settings.gitlab['impersonation_enabled'].nil? Settings.gitlab['usage_ping_enabled'] = true if Settings.gitlab['usage_ping_enabled'].nil? # diff --git a/doc/api/README.md b/doc/api/README.md index 0e8d1aaf86e..848a6e6b72b 100644 --- a/doc/api/README.md +++ b/doc/api/README.md @@ -225,6 +225,43 @@ For more information, refer to the Impersonation tokens are used exactly like regular personal access tokens, and can be passed in either the `private_token` parameter or the `Private-Token` header. +#### Disable impersonation + +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/40385) in GitLab +11.6. + +By default, impersonation is enabled. To disable impersonation, GitLab must be +reconfigured: + +**For Omnibus installations** + +1. Edit `/etc/gitlab/gitlab.rb`: + + ```ruby + gitlab_rails['impersonation_enabled'] = false + ``` + +1. Save the file and [reconfigure](../administration/restart_gitlab.md#omnibus-gitlab-reconfigure) + GitLab for the changes to take effect. + +To re-enable impersonation, remove this configuration and reconfigure GitLab. + +--- + +**For installations from source** + +1. Edit `config/gitlab.yml`: + + ```yaml + gitlab: + impersonation_enabled: false + ``` + +1. Save the file and [restart](../administration/restart_gitlab.md#installations-from-source) + GitLab for the changes to take effect. + +To re-enable impersonation, remove this configuration and restart GitLab. + ### Sudo NOTE: **Note:** @@ -540,7 +577,7 @@ When you try to access an API URL that does not exist you will receive 404 Not F ``` HTTP/1.1 404 Not Found Content-Type: application/json -{ +{ f "error": "404 Not Found" } ``` diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index 61357b3f1d6..af9b519ed9e 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -94,6 +94,7 @@ module API Gitlab::Auth::TokenNotFoundError, Gitlab::Auth::ExpiredError, Gitlab::Auth::RevokedError, + Gitlab::Auth::ImpersonationDisabled, Gitlab::Auth::InsufficientScopeError] base.__send__(:rescue_from, *error_classes, oauth2_bearer_token_error_handler) # rubocop:disable GitlabSecurity/PublicSend @@ -121,6 +122,11 @@ module API :invalid_token, "Token was revoked. You have to re-authorize from the user.") + when Gitlab::Auth::ImpersonationDisabled + Rack::OAuth2::Server::Resource::Bearer::Unauthorized.new( + :invalid_token, + "Token is an impersonation token but impersonation was disabled.") + when Gitlab::Auth::InsufficientScopeError # FIXME: ForbiddenError (inherited from Bearer::Forbidden of Rack::Oauth2) # does not include WWW-Authenticate header, which breaks the standard. diff --git a/lib/gitlab/auth/user_auth_finders.rb b/lib/gitlab/auth/user_auth_finders.rb index adba9084845..a5efe33bdc6 100644 --- a/lib/gitlab/auth/user_auth_finders.rb +++ b/lib/gitlab/auth/user_auth_finders.rb @@ -7,6 +7,7 @@ module Gitlab TokenNotFoundError = Class.new(AuthenticationError) ExpiredError = Class.new(AuthenticationError) RevokedError = Class.new(AuthenticationError) + ImpersonationDisabled = Class.new(AuthenticationError) UnauthorizedError = Class.new(AuthenticationError) class InsufficientScopeError < AuthenticationError @@ -67,6 +68,8 @@ module Gitlab raise ExpiredError when AccessTokenValidationService::REVOKED raise RevokedError + when AccessTokenValidationService::IMPERSONATION_DISABLED + raise ImpersonationDisabled end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index e6cc5ee79a0..5672df5f965 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3448,6 +3448,9 @@ msgstr "" msgid "ImageDiffViewer|Swipe" msgstr "" +msgid "Impersonation has been disabled" +msgstr "" + msgid "Import" msgstr "" diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index f350641a643..3dd0b2623ac 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -264,5 +264,17 @@ describe Admin::UsersController do expect(flash[:alert]).to eq("You are now impersonating #{user.username}") end end + + context "when impersonation is disabled" do + before do + stub_config_setting(impersonation_enabled: false) + end + + it "shows error page" do + post :impersonate, id: user.username + + expect(response).to have_gitlab_http_status(404) + end + end end end diff --git a/spec/features/admin/admin_users_spec.rb b/spec/features/admin/admin_users_spec.rb index f7c7a257075..d5516b334b9 100644 --- a/spec/features/admin/admin_users_spec.rb +++ b/spec/features/admin/admin_users_spec.rb @@ -205,74 +205,117 @@ describe "Admin::Users" do describe 'Impersonation' do let(:another_user) { create(:user) } - before do - visit admin_user_path(another_user) - end - context 'before impersonating' do - it 'shows impersonate button for other users' do - expect(page).to have_content('Impersonate') + subject { visit admin_user_path(user_to_visit) } + + let(:user_to_visit) { another_user } + + context 'for other users' do + it 'shows impersonate button for other users' do + subject + + expect(page).to have_content('Impersonate') + end end - it 'does not show impersonate button for admin itself' do - visit admin_user_path(current_user) + context 'for admin itself' do + let(:user_to_visit) { current_user } - expect(page).not_to have_content('Impersonate') + it 'does not show impersonate button for admin itself' do + subject + + expect(page).not_to have_content('Impersonate') + end end - it 'does not show impersonate button for blocked user' do - another_user.block + context 'for blocked user' do + before do + another_user.block + end - visit admin_user_path(another_user) + it 'does not show impersonate button for blocked user' do + subject - expect(page).not_to have_content('Impersonate') + expect(page).not_to have_content('Impersonate') + end + end - another_user.activate + context 'when impersonation is disabled' do + before do + stub_config_setting(impersonation_enabled: false) + end + + it 'does not show impersonate button' do + subject + + expect(page).not_to have_content('Impersonate') + end end end context 'when impersonating' do + subject { click_link 'Impersonate' } + before do - click_link 'Impersonate' + visit admin_user_path(another_user) end it 'logs in as the user when impersonate is clicked' do + subject + expect(page.find(:css, '.header-user .profile-link')['data-user']).to eql(another_user.username) end it 'sees impersonation log out icon' do - icon = first('.fa.fa-user-secret') + subject + icon = first('.fa.fa-user-secret') expect(icon).not_to be nil end + context 'a user with an expired password' do + before do + another_user.update(password_expires_at: Time.now - 5.minutes) + end + + it 'does not redirect to password change page' do + subject + + expect(current_path).to eq('/') + end + end + end + + context 'ending impersonation' do + subject { find(:css, 'li.impersonation a').click } + + before do + visit admin_user_path(another_user) + click_link 'Impersonate' + end + it 'logs out of impersonated user back to original user' do - find(:css, 'li.impersonation a').click + subject expect(page.find(:css, '.header-user .profile-link')['data-user']).to eq(current_user.username) end it 'is redirected back to the impersonated users page in the admin after stopping' do - find(:css, 'li.impersonation a').click + subject expect(current_path).to eq("/admin/users/#{another_user.username}") end - end - context 'when impersonating a user with an expired password' do - before do - another_user.update(password_expires_at: Time.now - 5.minutes) - click_link 'Impersonate' - end + context 'a user with an expired password' do + before do + another_user.update(password_expires_at: Time.now - 5.minutes) + end - it 'does not redirect to password change page' do - expect(current_path).to eq('/') - end + it 'is redirected back to the impersonated users page in the admin after stopping' do + subject - it 'is redirected back to the impersonated users page in the admin after stopping' do - find(:css, 'li.impersonation a').click - - expect(current_path).to eq("/admin/users/#{another_user.username}") + expect(current_path).to eq("/admin/users/#{another_user.username}") + end end end end diff --git a/spec/lib/gitlab/auth/user_auth_finders_spec.rb b/spec/lib/gitlab/auth/user_auth_finders_spec.rb index 5d3fbba264f..4e4c8b215c2 100644 --- a/spec/lib/gitlab/auth/user_auth_finders_spec.rb +++ b/spec/lib/gitlab/auth/user_auth_finders_spec.rb @@ -279,5 +279,20 @@ describe Gitlab::Auth::UserAuthFinders do expect { validate_access_token!(scopes: [:sudo]) }.to raise_error(Gitlab::Auth::InsufficientScopeError) end end + + context 'with impersonation token' do + let(:personal_access_token) { create(:personal_access_token, :impersonation, user: user) } + + context 'when impersonation is disabled' do + before do + stub_config_setting(impersonation_enabled: false) + allow_any_instance_of(described_class).to receive(:access_token).and_return(personal_access_token) + end + + it 'returns Gitlab::Auth::ImpersonationDisabled' do + expect { validate_access_token! }.to raise_error(Gitlab::Auth::ImpersonationDisabled) + end + end + end end end diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb index cca449e9e56..2c40e266f5f 100644 --- a/spec/requests/api/helpers_spec.rb +++ b/spec/requests/api/helpers_spec.rb @@ -206,6 +206,19 @@ describe API::Helpers do expect { current_user }.to raise_error Gitlab::Auth::ExpiredError end + + context 'when impersonation is disabled' do + let(:personal_access_token) { create(:personal_access_token, :impersonation, user: user) } + + before do + stub_config_setting(impersonation_enabled: false) + env[Gitlab::Auth::UserAuthFinders::PRIVATE_TOKEN_HEADER] = personal_access_token.token + end + + it 'does not allow impersonation tokens' do + expect { current_user }.to raise_error Gitlab::Auth::ImpersonationDisabled + end + end end end