From 078aa8b5ae3b6094f17bb5c09b1c7ad0f7c35c1a Mon Sep 17 00:00:00 2001
From: Emelia Smith <ThisIsMissEm@users.noreply.github.com>
Date: Tue, 3 Apr 2018 13:07:32 +0200
Subject: [PATCH] Improve require_admin! and require_staff! filters (#7018)

Previously these returns 302 redirects instead of 403s, which meant posting links to admin pages in slack caused them to unfurl, rather than stay as a link. Additionally, require_admin! doesn't appear to be actively used, on require_staff!
---
 app/controllers/application_controller.rb     |  4 +-
 .../controllers/admin/base_controller_spec.rb | 25 +++++++----
 .../application_controller_spec.rb            | 42 ++++++++++++++++++-
 3 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 6e50426178..5885264470 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -39,11 +39,11 @@ class ApplicationController < ActionController::Base
   end
 
   def require_admin!
-    redirect_to root_path unless current_user&.admin?
+    forbidden unless current_user&.admin?
   end
 
   def require_staff!
-    redirect_to root_path unless current_user&.staff?
+    forbidden unless current_user&.staff?
   end
 
   def check_suspension
diff --git a/spec/controllers/admin/base_controller_spec.rb b/spec/controllers/admin/base_controller_spec.rb
index 2b60e7e924..9ac833623f 100644
--- a/spec/controllers/admin/base_controller_spec.rb
+++ b/spec/controllers/admin/base_controller_spec.rb
@@ -9,18 +9,25 @@ describe Admin::BaseController, type: :controller do
     end
   end
 
-  it 'renders admin layout' do
+  it 'requires administrator or moderator' do
+    routes.draw { get 'success' => 'admin/base#success' }
+    sign_in(Fabricate(:user, admin: false, moderator: false))
+    get :success
+
+    expect(response).to have_http_status(:forbidden)
+  end
+
+  it 'renders admin layout as a moderator' do
+    routes.draw { get 'success' => 'admin/base#success' }
+    sign_in(Fabricate(:user, moderator: true))
+    get :success
+    expect(response).to render_template layout: 'admin'
+  end
+
+  it 'renders admin layout as an admin' do
     routes.draw { get 'success' => 'admin/base#success' }
     sign_in(Fabricate(:user, admin: true))
     get :success
     expect(response).to render_template layout: 'admin'
   end
-
-  it 'requires administrator' do
-    routes.draw { get 'success' => 'admin/base#success' }
-    sign_in(Fabricate(:user, admin: false))
-    get :success
-
-    expect(response).to redirect_to('/')
-  end
 end
diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb
index 51c0e3c700..3e4d27e055 100644
--- a/spec/controllers/application_controller_spec.rb
+++ b/spec/controllers/application_controller_spec.rb
@@ -181,10 +181,48 @@ describe ApplicationController, type: :controller do
       routes.draw { get 'sucesss' => 'anonymous#sucesss' }
     end
 
-    it 'redirects to root path if current user is not admin' do
+    it 'returns a 403 if current user is not admin' do
       sign_in(Fabricate(:user, admin: false))
       get 'sucesss'
-      expect(response).to redirect_to('/')
+      expect(response).to have_http_status(403)
+    end
+
+    it 'returns a 403 if current user is only a moderator' do
+      sign_in(Fabricate(:user, moderator: true))
+      get 'sucesss'
+      expect(response).to have_http_status(403)
+    end
+
+    it 'does nothing if current user is admin' do
+      sign_in(Fabricate(:user, admin: true))
+      get 'sucesss'
+      expect(response).to have_http_status(200)
+    end
+  end
+
+  describe 'require_staff!' do
+    controller do
+      before_action :require_staff!
+
+      def sucesss
+        head 200
+      end
+    end
+
+    before do
+      routes.draw { get 'sucesss' => 'anonymous#sucesss' }
+    end
+
+    it 'returns a 403 if current user is not admin or moderator' do
+      sign_in(Fabricate(:user, admin: false, moderator: false))
+      get 'sucesss'
+      expect(response).to have_http_status(403)
+    end
+
+    it 'does nothing if current user is moderator' do
+      sign_in(Fabricate(:user, moderator: true))
+      get 'sucesss'
+      expect(response).to have_http_status(200)
     end
 
     it 'does nothing if current user is admin' do