Refactor admin base controller (#18453)

* DEV: Add a dedicated Admin::StaffController base controller

The current parent(Admin:AdminController) for all admin-related controllers
uses a filter that allows only staff(admin, moderator) users.

This refactor makes Admin::AdminController filter for only admins as the name suggests and
introduces a base controller dedicated for staff-related endpoints.

* DEV: Set staff-only controllers parent to Admin::StaffController

Refactor staff-only controllers to inherit newly introduced
Admin::StaffController abstract controller. This conveys the
purpose of the parent controller better unlike the previously used parent
controller.

app/controllers/admin/admin_controller.rb

@@ -1,9 +1,8 @@
 # frozen_string_literal: true
 
 class Admin::AdminController < ApplicationController
-
   requires_login
-  before_action :ensure_staff
+  before_action :ensure_admin
 
   def index
     render body: nil

app/controllers/admin/dashboard_controller.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-class Admin::DashboardController < Admin::AdminController
+class Admin::DashboardController < Admin::StaffController
   def index
     data = AdminDashboardIndexData.fetch_cached_stats
 

app/controllers/admin/groups_controller.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-class Admin::GroupsController < Admin::AdminController
+class Admin::GroupsController < Admin::StaffController
   def create
     guardian.ensure_can_create_group!
 

app/controllers/admin/plugins_controller.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-class Admin::PluginsController < Admin::AdminController
+class Admin::PluginsController < Admin::StaffController
 
   def index
     render_serialized(Discourse.visible_plugins, AdminPluginSerializer, root: 'plugins')

app/controllers/admin/reports_controller.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-class Admin::ReportsController < Admin::AdminController
+class Admin::ReportsController < Admin::StaffController
   def index
     reports_methods = ['page_view_total_reqs'] +
       ApplicationRequest.req_types.keys

app/controllers/admin/screened_emails_controller.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-class Admin::ScreenedEmailsController < Admin::AdminController
+class Admin::ScreenedEmailsController < Admin::StaffController
 
   def index
     screened_emails = ScreenedEmail.limit(200).order('last_match_at desc').to_a

app/controllers/admin/screened_ip_addresses_controller.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-class Admin::ScreenedIpAddressesController < Admin::AdminController
+class Admin::ScreenedIpAddressesController < Admin::StaffController
 
   before_action :fetch_screened_ip_address, only: [:update, :destroy]
 

app/controllers/admin/screened_urls_controller.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-class Admin::ScreenedUrlsController < Admin::AdminController
+class Admin::ScreenedUrlsController < Admin::StaffController
 
   def index
     screened_urls = ScreenedUrl.select("domain, sum(match_count) as match_count, max(last_match_at) as last_match_at, min(created_at) as created_at").group(:domain).order('last_match_at DESC').to_a

app/controllers/admin/search_logs_controller.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-class Admin::SearchLogsController < Admin::AdminController
+class Admin::SearchLogsController < Admin::StaffController
 
   def index
     period = params[:period] || "all"

app/controllers/admin/staff_action_logs_controller.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-class Admin::StaffActionLogsController < Admin::AdminController
+class Admin::StaffActionLogsController < Admin::StaffController
 
   def index
     filters = params.slice(*UserHistory.staff_filters + [:page, :limit])

app/controllers/admin/staff_controller.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+class Admin::StaffController < ApplicationController
+  requires_login
+  before_action :ensure_staff
+end

app/controllers/admin/users_controller.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-class Admin::UsersController < Admin::AdminController
+class Admin::UsersController < Admin::StaffController
 
   before_action :fetch_user, only: [:suspend,
                                     :unsuspend,

app/controllers/admin/versions_controller.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-class Admin::VersionsController < Admin::AdminController
+class Admin::VersionsController < Admin::StaffController
   def show
     render json: DiscourseUpdates.check_version
   end

app/controllers/admin/watched_words_controller.rb

@@ -2,7 +2,7 @@
 
 require 'csv'
 
-class Admin::WatchedWordsController < Admin::AdminController
+class Admin::WatchedWordsController < Admin::StaffController
   skip_before_action :check_xhr, only: [:download]
 
   def index

spec/requests/admin/dashboard_controller_spec.rb

@@ -6,8 +6,8 @@ RSpec.describe Admin::DashboardController do
     Jobs::VersionCheck.any_instance.stubs(:execute).returns(true)
   end
 
-  it "is a subclass of AdminController" do
-    expect(Admin::DashboardController < Admin::AdminController).to eq(true)
+  it "is a subclass of StaffController" do
+    expect(Admin::DashboardController < Admin::StaffController).to eq(true)
   end
 
   context 'while logged in as an admin' do

spec/requests/admin/email_styles_controller_spec.rb

@@ -14,6 +14,10 @@ RSpec.describe Admin::EmailStylesController do
     SiteSetting.remove_override!(:email_custom_css)
   end
 
+  it "is a subclass of AdminController" do
+    expect(Admin::EmailStylesController < Admin::AdminController).to eq(true)
+  end
+
   describe 'show' do
     it 'returns default values' do
       get '/admin/customize/email_style.json'

spec/requests/admin/email_templates_controller_spec.rb

@@ -2,6 +2,7 @@
 
 RSpec.describe Admin::EmailTemplatesController do
   fab!(:admin) { Fabricate(:admin) }
+  fab!(:moderator) { Fabricate(:moderator) }
   fab!(:user) { Fabricate(:user) }
 
   def original_text(key)
@@ -17,6 +18,10 @@ RSpec.describe Admin::EmailTemplatesController do
     I18n.reload!
   end
 
+  it "is a subclass of AdminController" do
+    expect(Admin::EmailTemplatesController < Admin::AdminController).to eq(true)
+  end
+
   describe "#index" do
     it "raises an error if you aren't logged in" do
       get '/admin/customize/email_templates.json'
@@ -29,6 +34,12 @@ RSpec.describe Admin::EmailTemplatesController do
       expect(response.status).to eq(404)
     end
 
+    it "raises an error if you are a moderator" do
+      sign_in(moderator)
+      get "/admin/customize/email_templates.json"
+      expect(response.status).to eq(404)
+    end
+
     it "should work if you are an admin" do
       sign_in(admin)
       get '/admin/customize/email_templates.json'
@@ -79,6 +90,14 @@ RSpec.describe Admin::EmailTemplatesController do
       expect(response.status).to eq(404)
     end
 
+    it "raises an error if you are a moderator" do
+      sign_in(moderator)
+      put "/admin/customize/email_templates/some_id", params: {
+        email_template: { subject: "Subject", body: "Body" }
+      }, headers: headers
+      expect(response.status).to eq(404)
+    end
+
     context "when logged in as admin" do
       before do
         sign_in(admin)
@@ -268,6 +287,12 @@ RSpec.describe Admin::EmailTemplatesController do
       expect(response.status).to eq(404)
     end
 
+    it "raises an error if you are a moderator" do
+      sign_in(moderator)
+      delete "/admin/customize/email_templates/some_id", headers: headers
+      expect(response.status).to eq(404)
+    end
+
     context "when logged in as admin" do
       before do
         sign_in(admin)

spec/requests/admin/groups_controller_spec.rb

@@ -5,6 +5,10 @@ RSpec.describe Admin::GroupsController do
   fab!(:user) { Fabricate(:user) }
   fab!(:group) { Fabricate(:group) }
 
+  it 'is a subclass of StaffController' do
+    expect(Admin::UsersController < Admin::StaffController).to eq(true)
+  end
+
   before do
     sign_in(admin)
   end

spec/requests/admin/plugins_controller_spec.rb

@@ -2,8 +2,8 @@
 
 RSpec.describe Admin::PluginsController do
 
-  it "is a subclass of AdminController" do
-    expect(Admin::PluginsController < Admin::AdminController).to eq(true)
+  it "is a subclass of StaffController" do
+    expect(Admin::PluginsController < Admin::StaffController).to eq(true)
   end
 
   context "while logged in as an admin" do

spec/requests/admin/reports_controller_spec.rb

@@ -1,8 +1,8 @@
 # frozen_string_literal: true
 
 RSpec.describe Admin::ReportsController do
-  it "is a subclass of AdminController" do
-    expect(Admin::ReportsController < Admin::AdminController).to eq(true)
+  it "is a subclass of StaffController" do
+    expect(Admin::ReportsController < Admin::StaffController).to eq(true)
   end
 
   context 'while logged in as an admin' do

spec/requests/admin/robots_txt_controller_spec.rb

@@ -6,27 +6,40 @@ RSpec.describe Admin::RobotsTxtController do
   end
 
   fab!(:admin) { Fabricate(:admin) }
+  fab!(:moderator) { Fabricate(:moderator) }
   fab!(:user) { Fabricate(:user) }
 
-  describe "non-admin users" do
-    before { sign_in(user) }
+  context "when logged in as a non-admin user" do
+    shared_examples "access_forbidden" do
+      it "can't see #show" do
+        get "/admin/customize/robots.json"
+        expect(response.status).to eq(404)
+      end
 
-    it "can't see #show" do
-      get "/admin/customize/robots.json"
-      expect(response.status).to eq(404)
+      it "can't perform #update" do
+        put "/admin/customize/robots.json", params: { robots_txt: "adasdasd" }
+        expect(response.status).to eq(404)
+        expect(SiteSetting.overridden_robots_txt).to eq("")
+      end
+
+      it "can't perform #reset" do
+        SiteSetting.overridden_robots_txt = "overridden_content"
+        delete "/admin/customize/robots.json"
+        expect(response.status).to eq(404)
+        expect(SiteSetting.overridden_robots_txt).to eq("overridden_content")
+      end
     end
 
-    it "can't perform #update" do
-      put "/admin/customize/robots.json", params: { robots_txt: "adasdasd" }
-      expect(response.status).to eq(404)
-      expect(SiteSetting.overridden_robots_txt).to eq("")
+    context "when logged in as a moderator" do
+      before { sign_in(moderator) }
+
+      include_examples "access_forbidden"
     end
 
-    it "can't perform #reset" do
-      SiteSetting.overridden_robots_txt = "overridden_content"
-      delete "/admin/customize/robots.json"
-      expect(response.status).to eq(404)
-      expect(SiteSetting.overridden_robots_txt).to eq("overridden_content")
+    context "when logged in as non-staff user" do
+      before { sign_in(user) }
+
+      include_examples "access_forbidden"
     end
   end
 

spec/requests/admin/screened_emails_controller_spec.rb

@@ -1,8 +1,8 @@
 # frozen_string_literal: true
 
 RSpec.describe Admin::ScreenedEmailsController do
-  it "is a subclass of AdminController" do
-    expect(Admin::ScreenedEmailsController < Admin::AdminController).to eq(true)
+  it "is a subclass of StaffController" do
+    expect(Admin::ScreenedEmailsController < Admin::StaffController).to eq(true)
   end
 
   describe '#index' do

spec/requests/admin/screened_ip_addresses_controller_spec.rb

@@ -2,8 +2,8 @@
 
 RSpec.describe Admin::ScreenedIpAddressesController do
 
-  it "is a subclass of AdminController" do
-    expect(Admin::ScreenedIpAddressesController < Admin::AdminController).to eq(true)
+  it "is a subclass of StaffController" do
+    expect(Admin::ScreenedIpAddressesController < Admin::StaffController).to eq(true)
   end
 
   fab!(:admin) { Fabricate(:admin) }

spec/requests/admin/screened_urls_controller_spec.rb

@@ -1,8 +1,8 @@
 # frozen_string_literal: true
 
 RSpec.describe Admin::ScreenedUrlsController do
-  it "is a subclass of AdminController" do
-    expect(Admin::ScreenedUrlsController < Admin::AdminController).to eq(true)
+  it "is a subclass of StaffController" do
+    expect(Admin::ScreenedUrlsController < Admin::StaffController).to eq(true)
   end
 
   describe '#index' do

spec/requests/admin/search_logs_spec.rb

@@ -2,6 +2,7 @@
 
 RSpec.describe Admin::SearchLogsController do
   fab!(:admin) { Fabricate(:admin) }
+  fab!(:moderator) { Fabricate(:moderator) }
   fab!(:user) { Fabricate(:user) }
 
   before do
@@ -12,6 +13,10 @@ RSpec.describe Admin::SearchLogsController do
     SearchLog.clear_debounce_cache!
   end
 
+  it "is a subclass of StaffController" do
+    expect(Admin::SearchLogsController < Admin::StaffController).to eq(true)
+  end
+
   describe "#index" do
     it "raises an error if you aren't logged in" do
       get '/admin/logs/search_logs.json'
@@ -35,6 +40,18 @@ RSpec.describe Admin::SearchLogsController do
       expect(json[0]['searches']).to eq(1)
       expect(json[0]['ctr']).to eq(0)
     end
+
+    it "should work if you are a moderator" do
+      sign_in(moderator)
+      get "/admin/logs/search_logs.json"
+
+      expect(response.status).to eq(200)
+
+      json = response.parsed_body
+      expect(json[0]["term"]).to eq("ruby")
+      expect(json[0]["searches"]).to eq(1)
+      expect(json[0]["ctr"]).to eq(0)
+    end
   end
 
   describe "#term" do
@@ -69,5 +86,19 @@ RSpec.describe Admin::SearchLogsController do
       expect(json['term']['type']).to eq('search_log_term')
       expect(json['term']['search_result']).to be_present
     end
+
+    it "should work if you are a moderator" do
+      sign_in(moderator)
+
+      get "/admin/logs/search_logs/term.json", params: {
+        term: "ruby"
+      }
+
+      expect(response.status).to eq(200)
+
+      json = response.parsed_body
+      expect(json["term"]["type"]).to eq("search_log_term")
+      expect(json["term"]["search_result"]).to be_present
+    end
   end
 end

spec/requests/admin/staff_action_logs_controller_spec.rb

@@ -1,8 +1,8 @@
 # frozen_string_literal: true
 
 RSpec.describe Admin::StaffActionLogsController do
-  it "is a subclass of AdminController" do
-    expect(Admin::StaffActionLogsController < Admin::AdminController).to eq(true)
+  it "is a subclass of StaffController" do
+    expect(Admin::StaffActionLogsController < Admin::StaffController).to eq(true)
   end
 
   fab!(:admin) { Fabricate(:admin) }

spec/requests/admin/themes_controller_spec.rb

@@ -4,7 +4,7 @@ RSpec.describe Admin::ThemesController do
   fab!(:admin) { Fabricate(:admin) }
 
   it "is a subclass of AdminController" do
-    expect(Admin::UsersController < Admin::AdminController).to eq(true)
+    expect(Admin::ThemesController < Admin::AdminController).to eq(true)
   end
 
   before do

spec/requests/admin/users_controller_spec.rb

@@ -8,8 +8,8 @@ RSpec.describe Admin::UsersController do
   fab!(:user) { Fabricate(:user) }
   fab!(:coding_horror) { Fabricate(:coding_horror) }
 
-  it 'is a subclass of AdminController' do
-    expect(Admin::UsersController < Admin::AdminController).to eq(true)
+  it 'is a subclass of StaffController' do
+    expect(Admin::UsersController < Admin::StaffController).to eq(true)
   end
 
   before do

spec/requests/admin/versions_controller_spec.rb

@@ -9,8 +9,8 @@ RSpec.describe Admin::VersionsController do
     DiscourseUpdates.stubs(:critical_updates_available?).returns(false)
   end
 
-  it "is a subclass of AdminController" do
-    expect(Admin::VersionsController < Admin::AdminController).to eq(true)
+  it "is a subclass of StaffController" do
+    expect(Admin::VersionsController < Admin::StaffController).to eq(true)
   end
 
   context 'while logged in as an admin' do

spec/requests/admin/watched_words_controller_spec.rb

@@ -4,31 +4,65 @@ require 'csv'
 
 RSpec.describe Admin::WatchedWordsController do
   fab!(:admin) { Fabricate(:admin) }
+  fab!(:user) { Fabricate(:user) }
+
+  it "is a subclass of StaffController" do
+    expect(Admin::WatchedWordsController < Admin::StaffController).to eq(true)
+  end
 
   describe '#destroy' do
     fab!(:watched_word) { Fabricate(:watched_word) }
 
-    before do
-      sign_in(admin)
+    context "when logged in as a non-staff user" do
+      before do
+        sign_in(user)
+      end
+
+      it "can't delete a watched word" do
+        delete "/admin/customize/watched_words/#{watched_word.id}.json"
+
+        expect(response.status).to eq(404)
+      end
     end
 
-    it 'should return the right response when given an invalid id param' do
-      delete '/admin/customize/watched_words/9999.json'
+    context "when logged in as staff user" do
+      before do
+        sign_in(admin)
+      end
 
-      expect(response.status).to eq(400)
-    end
+      it 'should return the right response when given an invalid id param' do
+        delete "/admin/customize/watched_words/9999.json"
 
-    it 'should be able to delete a watched word' do
-      delete "/admin/customize/watched_words/#{watched_word.id}.json"
+        expect(response.status).to eq(400)
+      end
 
-      expect(response.status).to eq(200)
-      expect(WatchedWord.find_by(id: watched_word.id)).to eq(nil)
-      expect(UserHistory.where(action: UserHistory.actions[:watched_word_destroy]).count).to eq(1)
+      it "should be able to delete a watched word" do
+        delete "/admin/customize/watched_words/#{watched_word.id}.json"
+
+        expect(response.status).to eq(200)
+        expect(WatchedWord.find_by(id: watched_word.id)).to eq(nil)
+        expect(UserHistory.where(action: UserHistory.actions[:watched_word_destroy]).count).to eq(1)
+      end
     end
   end
 
   describe '#create' do
-    context 'when logged in as admin' do
+    context "when logged in as a non-staff user" do
+      before do
+        sign_in(user)
+      end
+
+      it "can't create a watched word" do
+        post "/admin/customize/watched_words.json", params: {
+          action_key: 'flag',
+          word: 'Fr33'
+        }
+
+        expect(response.status).to eq(404)
+      end
+    end
+
+    context "when logged in as a staff user" do
       before do
         sign_in(admin)
       end
@@ -54,11 +88,25 @@ RSpec.describe Admin::WatchedWordsController do
         expect(WatchedWord.take.case_sensitive?).to eq(true)
         expect(WatchedWord.take.word).to eq('PNG')
       end
-
     end
   end
 
   describe '#upload' do
+    context "when logged in as a non-staff user" do
+      before do
+        sign_in(user)
+      end
+
+      it "can't create watched words via file upload" do
+        post "/admin/customize/watched_words/upload.json", params: {
+          action_key: 'flag',
+          file: Rack::Test::UploadedFile.new(file_from_fixtures("words.csv", "csv"))
+        }
+
+        expect(response.status).to eq(404)
+      end
+    end
+
     context 'when logged in as admin' do
       before do
         sign_in(admin)