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

5 files changed, 42 insertions, 40 deletions

diff --git a/app/controllers/admin/application_controller.rb b/app/controllers/admin/application_controller.rb
index 9083bfb41cf..cf795d977ce 100644
--- a/app/controllers/admin/application_controller.rb
+++ b/app/controllers/admin/application_controller.rb
@@ -6,12 +6,6 @@ class Admin::ApplicationController < ApplicationController
   layout 'admin'
 
   def authenticate_admin!
-    return render_404 unless current_user.is_admin?
-  end
-
-  def authorize_impersonator!
-    if session[:impersonator_id]
-      User.find_by!(username: session[:impersonator_id]).admin?
-    end
+    render_404 unless current_user.is_admin?
   end
 end
diff --git a/app/controllers/admin/impersonation_controller.rb b/app/controllers/admin/impersonation_controller.rb
deleted file mode 100644
index 0382402afa6..00000000000
--- a/app/controllers/admin/impersonation_controller.rb
+++ /dev/null
@@ -1,32 +0,0 @@
-class Admin::ImpersonationController < Admin::ApplicationController
-  skip_before_action :authenticate_admin!, only: :destroy
-
-  before_action :user
-  before_action :authorize_impersonator!
-
-  def create
-    session[:impersonator_id] = current_user.username
-    session[:impersonator_return_to] = request.env['HTTP_REFERER']
-
-    warden.set_user(user, scope: 'user')
-
-    flash[:alert] = "You are impersonating #{user.username}."
-
-    redirect_to root_path
-  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
diff --git a/app/controllers/admin/impersonations_controller.rb b/app/controllers/admin/impersonations_controller.rb
new file mode 100644
index 00000000000..2db824c87ef
--- /dev/null
+++ b/app/controllers/admin/impersonations_controller.rb
@@ -0,0 +1,24 @@
+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
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index d7c927d444c..740d4dd5a8f 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -31,6 +31,22 @@ class Admin::UsersController < Admin::ApplicationController
     user
   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
     if user.block
       redirect_back_or_admin_user(notice: "Successfully blocked")
diff --git a/app/views/layouts/header/_default.html.haml b/app/views/layouts/header/_default.html.haml
index 3ca30d3baab..6b850ac5d9c 100644
--- a/app/views/layouts/header/_default.html.haml
+++ b/app/views/layouts/header/_default.html.haml
@@ -15,7 +15,7 @@
               = icon('search')
           - if session[:impersonator_id]
             %li.impersonation
-              = link_to stop_impersonation_admin_users_path, method: :delete, title: 'Stop impersonation', data: { toggle: 'tooltip', placement: 'bottom' } do
+              = link_to admin_impersonation_path, method: :delete, title: 'Stop impersonation', data: { toggle: 'tooltip', placement: 'bottom' } do
                 = icon('user-secret fw')
           - if current_user.is_admin?
             %li