Commit 4f5eeb2a authored by Robert Speicher's avatar Robert Speicher

Merge branch 'fix-impersonation-issue' into 'master'

Prevent privilege escalation via "impersonate" feature

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/15548

See merge request !1956
parents 7e47f876 c6c985bc
...@@ -6,12 +6,6 @@ class Admin::ApplicationController < ApplicationController ...@@ -6,12 +6,6 @@ class Admin::ApplicationController < ApplicationController
layout 'admin' layout 'admin'
def authenticate_admin! def authenticate_admin!
return render_404 unless current_user.is_admin? render_404 unless current_user.is_admin?
end
def authorize_impersonator!
if session[:impersonator_id]
User.find_by!(username: session[:impersonator_id]).admin?
end
end end
end end
class Admin::ImpersonationController < Admin::ApplicationController
skip_before_action :authenticate_admin!, only: :destroy
before_action :user
before_action :authorize_impersonator!
def create
if @user.blocked?
flash[:alert] = "You cannot impersonate a blocked user"
redirect_to admin_user_path(@user)
else
session[:impersonator_id] = current_user.username
session[:impersonator_return_to] = admin_user_path(@user)
warden.set_user(user, scope: 'user')
flash[:alert] = "You are impersonating #{user.username}."
redirect_to root_path
end
end
def destroy
redirect = session[:impersonator_return_to]
warden.set_user(user, scope: 'user')
session[:impersonator_return_to] = nil
session[:impersonator_id] = nil
redirect_to redirect || root_path
end
def user
@user ||= User.find_by!(username: params[:id] || session[:impersonator_id])
end
end
class Admin::ImpersonationsController < Admin::ApplicationController
skip_before_action :authenticate_admin!
before_action :authenticate_impersonator!
def destroy
original_user = current_user
warden.set_user(impersonator, scope: :user)
session[:impersonator_id] = nil
redirect_to admin_user_path(original_user)
end
private
def impersonator
@impersonator ||= User.find(session[:impersonator_id]) if session[:impersonator_id]
end
def authenticate_impersonator!
render_404 unless impersonator && impersonator.is_admin? && !impersonator.blocked?
end
end
...@@ -31,6 +31,22 @@ class Admin::UsersController < Admin::ApplicationController ...@@ -31,6 +31,22 @@ class Admin::UsersController < Admin::ApplicationController
user user
end end
def impersonate
if user.blocked?
flash[:alert] = "You cannot impersonate a blocked user"
redirect_to admin_user_path(user)
else
session[:impersonator_id] = current_user.id
warden.set_user(user, scope: :user)
flash[:alert] = "You are now impersonating #{user.username}"
redirect_to root_path
end
end
def block def block
if user.block if user.block
redirect_back_or_admin_user(notice: "Successfully blocked") redirect_back_or_admin_user(notice: "Successfully blocked")
......
...@@ -15,7 +15,7 @@ ...@@ -15,7 +15,7 @@
- if current_user - if current_user
- if session[:impersonator_id] - if session[:impersonator_id]
%li.impersonation %li.impersonation
= link_to stop_impersonation_admin_users_path, method: :delete, title: 'Stop Impersonation', data: { toggle: 'tooltip', placement: 'bottom', container: 'body' } do = link_to admin_impersonation_path, method: :delete, title: 'Stop Impersonation', data: { toggle: 'tooltip', placement: 'bottom', container: 'body' } do
= icon('user-secret fw') = icon('user-secret fw')
- if current_user.is_admin? - if current_user.is_admin?
%li %li
......
...@@ -212,8 +212,6 @@ Rails.application.routes.draw do ...@@ -212,8 +212,6 @@ Rails.application.routes.draw do
resources :keys, only: [:show, :destroy] resources :keys, only: [:show, :destroy]
resources :identities, except: [:show] resources :identities, except: [:show]
delete 'stop_impersonation' => 'impersonation#destroy', on: :collection
member do member do
get :projects get :projects
get :keys get :keys
...@@ -223,12 +221,14 @@ Rails.application.routes.draw do ...@@ -223,12 +221,14 @@ Rails.application.routes.draw do
put :unblock put :unblock
put :unlock put :unlock
put :confirm put :confirm
post 'impersonate' => 'impersonation#create' post :impersonate
patch :disable_two_factor patch :disable_two_factor
delete 'remove/:email_id', action: 'remove_email', as: 'remove_email' delete 'remove/:email_id', action: 'remove_email', as: 'remove_email'
end end
end end
resource :impersonation, only: :destroy
resources :abuse_reports, only: [:index, :destroy] resources :abuse_reports, only: [:index, :destroy]
resources :spam_logs, only: [:index, :destroy] resources :spam_logs, only: [:index, :destroy]
......
require 'spec_helper'
describe Admin::ImpersonationController do
let(:admin) { create(:admin) }
before do
sign_in(admin)
end
describe 'CREATE #impersonation when blocked' do
let(:blocked_user) { create(:user, state: :blocked) }
it 'does not allow impersonation' do
post :create, id: blocked_user.username
expect(flash[:alert]).to eq 'You cannot impersonate a blocked user'
end
end
end
require 'spec_helper'
describe Admin::ImpersonationsController do
let(:impersonator) { create(:admin) }
let(:user) { create(:user) }
describe "DELETE destroy" do
context "when not signed in" do
it "redirects to the sign in page" do
delete :destroy
expect(response).to redirect_to(new_user_session_path)
end
end
context "when signed in" do
before do
sign_in(user)
end
context "when not impersonating" do
it "responds with status 404" do
delete :destroy
expect(response.status).to eq(404)
end
it "doesn't sign us in" do
delete :destroy
expect(warden.user).to eq(user)
end
end
context "when impersonating" do
before do
session[:impersonator_id] = impersonator.id
end
context "when the impersonator is not admin (anymore)" do
before do
impersonator.admin = false
impersonator.save
end
it "responds with status 404" do
delete :destroy
expect(response.status).to eq(404)
end
it "doesn't sign us in as the impersonator" do
delete :destroy
expect(warden.user).to eq(user)
end
end
context "when the impersonator is admin" do
context "when the impersonator is blocked" do
before do
impersonator.block!
end
it "responds with status 404" do
delete :destroy
expect(response.status).to eq(404)
end
it "doesn't sign us in as the impersonator" do
delete :destroy
expect(warden.user).to eq(user)
end
end
context "when the impersonator is not blocked" do
it "redirects to the impersonated user's page" do
delete :destroy
expect(response).to redirect_to(admin_user_path(user))
end
it "signs us in as the impersonator" do
delete :destroy
expect(warden.user).to eq(impersonator)
end
end
end
end
end
end
end
...@@ -2,9 +2,10 @@ require 'spec_helper' ...@@ -2,9 +2,10 @@ require 'spec_helper'
describe Admin::UsersController do describe Admin::UsersController do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:admin) { create(:admin) }
before do before do
sign_in(create(:admin)) sign_in(admin)
end end
describe 'DELETE #user with projects' do describe 'DELETE #user with projects' do
...@@ -112,4 +113,50 @@ describe Admin::UsersController do ...@@ -112,4 +113,50 @@ describe Admin::UsersController do
patch :disable_two_factor, id: user.to_param patch :disable_two_factor, id: user.to_param
end end
end end
describe "POST impersonate" do
context "when the user is blocked" do
before do
user.block!
end
it "shows a notice" do
post :impersonate, id: user.username
expect(flash[:alert]).to eq("You cannot impersonate a blocked user")
end
it "doesn't sign us in as the user" do
post :impersonate, id: user.username
expect(warden.user).to eq(admin)
end
end
context "when the user is not blocked" do
it "stores the impersonator in the session" do
post :impersonate, id: user.username
expect(session[:impersonator_id]).to eq(admin.id)
end
it "signs us in as the user" do
post :impersonate, id: user.username
expect(warden.user).to eq(user)
end
it "redirects to root" do
post :impersonate, id: user.username
expect(response).to redirect_to(root_path)
end
it "shows a notice" do
post :impersonate, id: user.username
expect(flash[:alert]).to eq("You are now impersonating #{user.username}")
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