Commit 413f65cf authored by Drew Blessing's avatar Drew Blessing

Clear session access tokens when starting/stopping impersonation

For project import purposes, GitLab may store third-party
access tokens in the session cookie. When an admin impersonates
another user, the session is not totally unique so we should
clear out any access tokens both when starting and stopping
impersonation. This prevents inadvertently using the wrong
token in the wrong context.

Changelog: security
parent a9fecf61
...@@ -49,6 +49,7 @@ class Admin::UsersController < Admin::ApplicationController ...@@ -49,6 +49,7 @@ class Admin::UsersController < Admin::ApplicationController
session[:impersonator_id] = current_user.id session[:impersonator_id] = current_user.id
warden.set_user(user, scope: :user) warden.set_user(user, scope: :user)
clear_access_token_session_keys!
log_impersonation_event log_impersonation_event
......
...@@ -3,6 +3,12 @@ ...@@ -3,6 +3,12 @@
module Impersonation module Impersonation
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
SESSION_KEYS_TO_DELETE = %w(
github_access_token gitea_access_token gitlab_access_token
bitbucket_token bitbucket_refresh_token bitbucket_server_personal_access_token
bulk_import_gitlab_access_token fogbugz_token
).freeze
def current_user def current_user
user = super user = super
...@@ -27,6 +33,7 @@ module Impersonation ...@@ -27,6 +33,7 @@ module Impersonation
warden.set_user(impersonator, scope: :user) warden.set_user(impersonator, scope: :user)
session[:impersonator_id] = nil session[:impersonator_id] = nil
clear_access_token_session_keys!
current_user current_user
end end
...@@ -35,6 +42,12 @@ module Impersonation ...@@ -35,6 +42,12 @@ module Impersonation
Gitlab::AppLogger.info("User #{impersonator.username} has stopped impersonating #{current_user.username}") Gitlab::AppLogger.info("User #{impersonator.username} has stopped impersonating #{current_user.username}")
end end
def clear_access_token_session_keys!
access_tokens_keys = session.keys & SESSION_KEYS_TO_DELETE
access_tokens_keys.each { |key| session.delete(key) }
end
def impersonator def impersonator
strong_memoize(:impersonator) do strong_memoize(:impersonator) do
User.find(session[:impersonator_id]) if session[:impersonator_id] User.find(session[:impersonator_id]) if session[:impersonator_id]
......
...@@ -92,6 +92,14 @@ RSpec.describe Admin::ImpersonationsController do ...@@ -92,6 +92,14 @@ RSpec.describe Admin::ImpersonationsController do
expect(warden.user).to eq(impersonator) expect(warden.user).to eq(impersonator)
end end
it 'clears token session keys' do
session[:bitbucket_token] = SecureRandom.hex(8)
delete :destroy
expect(session[:bitbucket_token]).to be_nil
end
end end
# base case # base case
......
...@@ -794,6 +794,14 @@ RSpec.describe Admin::UsersController do ...@@ -794,6 +794,14 @@ RSpec.describe Admin::UsersController do
expect(flash[:alert]).to eq("You are now impersonating #{user.username}") expect(flash[:alert]).to eq("You are now impersonating #{user.username}")
end end
it 'clears token session keys' do
session[:github_access_token] = SecureRandom.hex(8)
post :impersonate, params: { id: user.username }
expect(session[:github_access_token]).to be_nil
end
end end
context "when impersonation is disabled" do context "when impersonation is disabled" do
......
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