FIX: return 429 when admin api key is limited on admin route

This also handles a general case where exceptions leak out prior to being handled by the application controller

app/controllers/application_controller.rb

@@ -115,7 +115,6 @@ class ApplicationController < ActionController::Base
   end
 
   rescue_from Discourse::NotLoggedIn do |e|
-    raise e if Rails.env.test?
     if (request.format && request.format.json?) || request.xhr? || !request.get?
       rescue_discourse_actions(:not_logged_in, 403, include_ember: true)
     else
@@ -123,6 +122,15 @@ class ApplicationController < ActionController::Base
     end
   end
 
+  rescue_from Discourse::InvalidParameters do |e|
+    message = I18n.t('invalid_params', message: e.message)
+    if (request.format && request.format.json?) || request.xhr? || !request.get?
+      rescue_discourse_actions(:invalid_parameters, 400, include_ember: true, custom_message_translated: message)
+    else
+      rescue_discourse_actions(:not_found, 400, custom_message_translated: message)
+    end
+  end
+
   rescue_from ActiveRecord::StatementInvalid do |e|
     Discourse.reset_active_record_cache_if_needed(e)
     raise e
@@ -162,18 +170,20 @@ class ApplicationController < ActionController::Base
                        (request.xhr?) ||
                        ((params[:external_id] || '').ends_with? '.json')
 
+    message = opts[:custom_message_translated] || I18n.t(opts[:custom_message] || type)
+
     if show_json_errors
       # HACK: do not use render_json_error for topics#show
       if request.params[:controller] == 'topics' && request.params[:action] == 'show'
-        return render status: status_code, layout: false, plain: (status_code == 404 || status_code == 410) ? build_not_found_page(status_code) : I18n.t(type)
+        return render status: status_code, layout: false, plain: (status_code == 404 || status_code == 410) ? build_not_found_page(status_code) : message
       end
 
-      render_json_error I18n.t(opts[:custom_message] || type), type: type, status: status_code
+      render_json_error message, type: type, status: status_code
     else
       begin
         current_user
       rescue Discourse::InvalidAccess
-        return render plain: I18n.t(opts[:custom_message] || type), status: status_code
+        return render plain: message, status: status_code
       end
 
       render html: build_not_found_page(status_code, opts[:include_ember] ? 'application' : 'no_ember')

config/application.rb

@@ -158,8 +158,8 @@ module Discourse
     # supports etags (post 1.7)
     config.middleware.delete Rack::ETag
 
-    # route all exceptions via our router
+    require 'middleware/discourse_public_exceptions'
-    config.exceptions_app = self.routes
+    config.exceptions_app = Middleware::DiscoursePublicExceptions.new(Rails.public_path)
 
     # Our templates shouldn't start with 'discourse/templates'
     config.handlebars.templates_root = 'discourse/templates'

config/environments/test.rb

@@ -10,12 +10,15 @@ Discourse::Application.configure do
   # Configure static asset server for tests with Cache-Control for performance
   config.public_file_server.enabled = true
 
-  # Show full error reports and disable caching
+  # don't consider reqs local so we can properly handle exceptions like we do in prd
-  config.consider_all_requests_local       = true
+  config.consider_all_requests_local       = false
+
+  # disable caching
   config.action_controller.perform_caching = false
 
-  # Raise exceptions instead of rendering exception templates
+  # production has "show exceptions" on so we better have it
-  config.action_dispatch.show_exceptions = false
+  # in test as well
+  config.action_dispatch.show_exceptions = true
 
   # Disable request forgery protection in test environment
   config.action_controller.allow_forgery_protection = false

config/locales/server.en.yml

@@ -162,6 +162,7 @@ en:
     not_enough_space_on_disk: "There is not enough space on disk to upload this backup."
     invalid_filename: "The backup filename contains invalid characters. Valid characters are a-z 0-9 . - _."
 
+  invalid_params: "You supplied invalid parameters to the request: %{message}"
   not_logged_in: "You need to be logged in to do that."
   not_found: "The requested URL or resource could not be found."
   invalid_access: "You are not permitted to view the requested resource."

lib/middleware/discourse_public_exceptions.rb

@@ -0,0 +1,31 @@
+# since all the rescue from clauses are not caught by the application controller for matches
+# we need to handle certain exceptions here
+module Middleware
+  class DiscoursePublicExceptions < ::ActionDispatch::PublicExceptions
+
+    def initialize(path)
+      super
+    end
+
+    def call(env)
+      # this is so so gnarly
+      # sometimes we leak out exceptions prior to creating a controller instance
+      # this can happen if we have an exception in a route constraint in some cases
+      # this code re-dispatches the exception to our application controller so we can
+      # properly translate the exception to a page
+      exception = env["action_dispatch.exception"]
+      response = ActionDispatch::Response.new
+
+      if exception
+        fake_controller = ApplicationController.new
+        fake_controller.response = response
+
+        if ApplicationController.rescue_with_handler(exception, object: fake_controller)
+          return [response.status, response.headers, response.body]
+        end
+      end
+      super
+    end
+
+  end
+end

spec/controllers/admin/admin_controller_spec.rb

@@ -5,13 +5,12 @@ describe Admin::AdminController do
   context 'index' do
 
     it 'needs you to be logged in' do
-      expect do
+      get :index, format: :json
-        get :index, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     it "raises an error if you aren't an admin" do
-      user = log_in
+      _user = log_in
       get :index, format: :json
       expect(response).to be_forbidden
     end

spec/controllers/categories_controller_spec.rb

@@ -4,7 +4,8 @@ describe CategoriesController do
   describe "create" do
 
     it "requires the user to be logged in" do
-      expect { post :create, format: :json }.to raise_error(Discourse::NotLoggedIn)
+      post :create, format: :json
+      expect(response.status).to eq(403)
     end
 
     describe "logged in" do
@@ -90,8 +91,8 @@ describe CategoriesController do
   describe "destroy" do
 
     it "requires the user to be logged in" do
-      expect { delete :destroy, params: { id: "category" }, format: :json }
+      delete :destroy, params: { id: "category" }, format: :json
-        .to raise_error(Discourse::NotLoggedIn)
+      expect(response.status).to eq(403)
     end
 
     describe "logged in" do
@@ -158,7 +159,8 @@ describe CategoriesController do
   describe "update" do
 
     it "requires the user to be logged in" do
-      expect { put :update, params: { id: 'category' }, format: :json }.to raise_error(Discourse::NotLoggedIn)
+      put :update, params: { id: 'category' }, format: :json
+      expect(response.status).to eq(403)
     end
 
     describe "logged in" do
@@ -302,9 +304,8 @@ describe CategoriesController do
 
   describe 'update_slug' do
     it 'requires the user to be logged in' do
-      expect do
+      put :update_slug, params: { category_id: 'category' }, format: :json
-        put :update_slug, params: { category_id: 'category' }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     describe 'logged in' do

spec/controllers/category_hashtags_controller_spec.rb

@@ -44,9 +44,8 @@ describe CategoryHashtagsController do
 
     describe "not logged in" do
       it 'raises an exception' do
-        expect do
+        get :check, params: { category_slugs: [] }, format: :json
-          get :check, params: { category_slugs: [] }, format: :json
+        expect(response.status).to eq(403)
-        end.to raise_error(Discourse::NotLoggedIn)
       end
     end
   end

spec/controllers/composer_messages_controller_spec.rb

@@ -5,7 +5,8 @@ describe ComposerMessagesController do
   context '.index' do
 
     it 'requires you to be logged in' do
-      expect { get :index, format: :json }.to raise_error(Discourse::NotLoggedIn)
+      get :index, format: :json
+      expect(response.status).to eq(403)
     end
 
     context 'when logged in' do

spec/controllers/draft_controller_spec.rb

@@ -3,7 +3,8 @@ require 'rails_helper'
 describe DraftController do
 
   it 'requires you to be logged in' do
-    expect { post :update }.to raise_error(Discourse::NotLoggedIn)
+    post :update
+    expect(response.status).to eq(403)
   end
 
   it 'saves a draft on update' do

spec/controllers/email_controller_spec.rb

@@ -5,7 +5,8 @@ describe EmailController do
   context '.preferences_redirect' do
 
     it 'requires you to be logged in' do
-      expect { get :preferences_redirect, format: :json }.to raise_error(Discourse::NotLoggedIn)
+      get :preferences_redirect, format: :json
+      expect(response.status).to eq(403)
     end
 
     context 'when logged in' do

spec/controllers/finish_installation_controller_spec.rb

@@ -50,13 +50,12 @@ describe FinishInstallationController do
       end
 
       it "raises an error when the email is not in the allowed list" do
-        expect do
+        post :register, params: {
-          post :register, params: {
+          email: 'notrobin@example.com',
-            email: 'notrobin@example.com',
+          username: 'eviltrout',
-            username: 'eviltrout',
+          password: 'disismypasswordokay'
-            password: 'disismypasswordokay'
+        }, format: :json
-          }, format: :json
+        expect(response.status).to eq(400)
-        end.to raise_error(Discourse::InvalidParameters)
       end
 
       it "doesn't redirect when fields are wrong" do

spec/controllers/inline_onebox_controller_spec.rb

@@ -3,9 +3,8 @@ require 'rails_helper'
 describe InlineOneboxController do
 
   it "requires the user to be logged in" do
-    expect do
+    get :show, params: { urls: [] }, format: :json
-      get :show, params: { urls: [] }, format: :json
+    expect(response.status).to eq(403)
-    end.to raise_error(Discourse::NotLoggedIn)
   end
 
   context "logged in" do

spec/controllers/invites_controller_spec.rb

@@ -32,11 +32,10 @@ describe InvitesController do
   context '.destroy' do
 
     it 'requires you to be logged in' do
-      expect do
+      delete :destroy,
-        delete :destroy,
+        params: { email: 'jake@adventuretime.ooo' },
-          params: { email: 'jake@adventuretime.ooo' },
+        format: :json
-          format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     context 'while logged in' do
@@ -49,15 +48,13 @@ describe InvitesController do
       end
 
       it "raises an error when the email cannot be found" do
-        expect do
+        delete :destroy, params: { email: 'finn@adventuretime.ooo' }, format: :json
-          delete :destroy, params: { email: 'finn@adventuretime.ooo' }, format: :json
+        expect(response.status).to eq(400)
-        end.to raise_error(Discourse::InvalidParameters)
       end
 
       it 'raises an error when the invite is not yours' do
-        expect do
+        delete :destroy, params: { email: another_invite.email }, format: :json
-          delete :destroy, params: { email: another_invite.email }, format: :json
+        expect(response.status).to eq(400)
-        end.to raise_error(Discourse::InvalidParameters)
       end
 
       it "destroys the invite" do
@@ -71,9 +68,8 @@ describe InvitesController do
 
   context '#create' do
     it 'requires you to be logged in' do
-      expect do
+      post :create, params: { email: 'jake@adventuretime.ooo' }, format: :json
-        post :create, params: { email: 'jake@adventuretime.ooo' }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     context 'while logged in' do
@@ -136,11 +132,10 @@ describe InvitesController do
 
   context '.create_invite_link' do
     it 'requires you to be logged in' do
-      expect {
+      post :create_invite_link, params: {
-        post :create_invite_link, params: {
+        email: 'jake@adventuretime.ooo'
-          email: 'jake@adventuretime.ooo'
+      }, format: :json
-        }, format: :json
+      expect(response.status).to eq(403)
-      }.to raise_error(Discourse::NotLoggedIn)
     end
 
     context 'while logged in' do
@@ -363,9 +358,8 @@ describe InvitesController do
   context '.resend_invite' do
 
     it 'requires you to be logged in' do
-      expect {
+      delete :resend_invite, params: { email: 'first_name@example.com' }, format: :json
-        delete :resend_invite, params: { email: 'first_name@example.com' }, format: :json
+      expect(response.status).to eq(403)
-      }.to raise_error(Discourse::NotLoggedIn)
     end
 
     context 'while logged in' do
@@ -378,15 +372,13 @@ describe InvitesController do
       end
 
       it "raises an error when the email cannot be found" do
-        expect do
+        post :resend_invite, params: { email: 'first_name@example.com' }, format: :json
-          post :resend_invite, params: { email: 'first_name@example.com' }, format: :json
+        expect(response.status).to eq(400)
-        end.to raise_error(Discourse::InvalidParameters)
       end
 
       it 'raises an error when the invite is not yours' do
-        expect do
+        post :resend_invite, params: { email: another_invite.email }, format: :json
-          post :resend_invite, params: { email: another_invite.email }, format: :json
+        expect(response.status).to eq(400)
-        end.to raise_error(Discourse::InvalidParameters)
       end
 
       it "resends the invite" do
@@ -400,9 +392,8 @@ describe InvitesController do
 
   context '.upload_csv' do
     it 'requires you to be logged in' do
-      expect {
+      post :upload_csv, format: :json
-        post :upload_csv, format: :json
+      expect(response.status).to eq(403)
-      }.to raise_error(Discourse::NotLoggedIn)
     end
 
     context 'while logged in' do

spec/controllers/list_controller_spec.rb

@@ -297,7 +297,8 @@ describe ListController do
 
   context 'read' do
     it 'raises an error when not logged in' do
-      expect { get :read }.to raise_error(Discourse::NotLoggedIn)
+      get :read
+      expect(response.status).to eq(404)
     end
 
     context 'when logged in' do

spec/controllers/notifications_controller_spec.rb

@@ -17,7 +17,7 @@ describe NotificationsController do
       end
 
       it 'should mark notifications as viewed' do
-        notification = Fabricate(:notification, user: user)
+        _notification = Fabricate(:notification, user: user)
         expect(user.reload.unread_notifications).to eq(1)
         expect(user.reload.total_unread_notifications).to eq(1)
         get :index, params: { recent: true }, format: :json
@@ -26,7 +26,7 @@ describe NotificationsController do
       end
 
       it 'should not mark notifications as viewed if silent param is present' do
-        notification = Fabricate(:notification, user: user)
+        _notification = Fabricate(:notification, user: user)
         expect(user.reload.unread_notifications).to eq(1)
         expect(user.reload.total_unread_notifications).to eq(1)
         get :index, params: { recent: true, silent: true }
@@ -63,7 +63,7 @@ describe NotificationsController do
     end
 
     it "updates the `read` status" do
-      notification = Fabricate(:notification, user: user)
+      _notification = Fabricate(:notification, user: user)
       expect(user.reload.unread_notifications).to eq(1)
       expect(user.reload.total_unread_notifications).to eq(1)
       put :mark_read, format: :json
@@ -75,9 +75,8 @@ describe NotificationsController do
 
   context 'when not logged in' do
     it 'should raise an error' do
-      expect do
+      get :index, params: { recent: true }, format: :json
-        get :index, params: { recent: true }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
   end
 

spec/controllers/onebox_controller_spec.rb

@@ -5,9 +5,8 @@ describe OneboxController do
   let(:url) { "http://google.com" }
 
   it "requires the user to be logged in" do
-    expect do
+    get :show, params: { url: url }, format: :json
-      get :show, params: { url: url }, format: :json
+    expect(response.status).to eq(403)
-    end.to raise_error(Discourse::NotLoggedIn)
   end
 
   describe "logged in" do

spec/controllers/post_actions_controller_spec.rb

@@ -3,11 +3,6 @@ require 'rails_helper'
 describe PostActionsController do
 
   describe 'create' do
-    it 'requires you to be logged in' do
-      expect do
-        post :create, format: :json
-      end.to raise_error(Discourse::NotLoggedIn)
-    end
 
     context 'logged in as user' do
       let(:user) { Fabricate(:user) }
@@ -32,12 +27,6 @@ describe PostActionsController do
 
     let(:post) { Fabricate(:post, user: Fabricate(:coding_horror)) }
 
-    it 'requires you to be logged in' do
-      expect do
-        delete :destroy, params: { id: post.id }, format: :json
-      end.to raise_error(Discourse::NotLoggedIn)
-    end
-
     context 'logged in' do
       let!(:user) { log_in }
 
@@ -89,9 +78,8 @@ describe PostActionsController do
 
     context "not logged in" do
       it "should not allow them to clear flags" do
-        expect do
+        post :defer_flags, format: :json
-          post :defer_flags, format: :json
+        expect(response.status).to eq(403)
-        end.to raise_error(Discourse::NotLoggedIn)
       end
     end
 

spec/controllers/posts_controller_spec.rb

@@ -47,11 +47,10 @@ end
 
 shared_examples 'action requires login' do |method, action, params|
   it 'raises an exception when not logged in' do
-    expect do
+    options = { format: :json }
-      options = { format: :json }
+    options.merge!(params: params) if params
-      options.merge!(params: params) if params
+    self.public_send(method, action, options)
-      self.public_send(method, action, options)
+    expect(response.status).to eq(403)
-    end.to raise_error(Discourse::NotLoggedIn)
   end
 end
 
@@ -268,9 +267,8 @@ describe PostsController do
       end
 
       it "raises invalid parameters with missing ids" do
-        expect do
+        delete :destroy_many, params: { post_ids: [12345] }, format: :json
-          delete :destroy_many, params: { post_ids: [12345] }, format: :json
+        expect(response.status).to eq(400)
-        end.to raise_error(Discourse::InvalidParameters)
       end
 
       it "raises an error when the user doesn't have permission to delete the posts" do
@@ -855,11 +853,10 @@ describe PostsController do
     let(:post_revision) { Fabricate(:post_revision, post: post) }
 
     it "throws an exception when revision is < 2" do
-      expect {
+      get :revisions, params: {
-        get :revisions, params: {
+        post_id: post_revision.post_id, revision: 1
-          post_id: post_revision.post_id, revision: 1
+      }, format: :json
-        }, format: :json
+      expect(response.status).to eq(400)
-      }.to raise_error(Discourse::InvalidParameters)
     end
 
     context "when edit history is not visible to the public" do
@@ -984,10 +981,9 @@ describe PostsController do
     describe "when logged in as staff" do
       let(:logged_in_as) { log_in(:moderator) }
 
-      it "throws an exception when revision is < 2" do
+      it "fails when revision is < 2" do
-        expect {
+        put :revert, params:  { post_id: post.id, revision: 1 }, format: :json
-          put :revert, params:  { post_id: post.id, revision: 1 }, format: :json
+        expect(response.status).to eq(400)
-        }.to raise_error(Discourse::InvalidParameters)
       end
 
       it "fails when post_revision record is not found" do

spec/controllers/search_controller_spec.rb

@@ -125,19 +125,17 @@ describe SearchController do
 
   context "search context" do
     it "raises an error with an invalid context type" do
-      expect do
+      get :query, params: {
-        get :query, params: {
+        term: 'test', search_context: { type: 'security', id: 'hole' }
-          term: 'test', search_context: { type: 'security', id: 'hole' }
+      }, format: :json
-        }, format: :json
+      expect(response.status).to eq(400)
-      end.to raise_error(Discourse::InvalidParameters)
     end
 
     it "raises an error with a missing id" do
-      expect do
+      get :query,
-        get :query,
+        params: { term: 'test', search_context: { type: 'user' } },
-          params: { term: 'test', search_context: { type: 'user' } },
+        format: :json
-          format: :json
+      expect(response.status).to eq(400)
-      end.to raise_error(Discourse::InvalidParameters)
     end
 
     context "with a user" do
@@ -148,7 +146,6 @@ describe SearchController do
         get :query, params: {
           term: 'test', search_context: { type: 'user', id: user.username }
         }, format: :json
-
         expect(response).not_to be_success
       end
 

spec/controllers/steps_controller_spec.rb

@@ -7,11 +7,10 @@ describe StepsController do
   end
 
   it 'needs you to be logged in' do
-    expect do
+    put :update, params: {
-      put :update, params: {
+      id: 'made-up-id', fields: { forum_title: "updated title" }
-        id: 'made-up-id', fields: { forum_title: "updated title" }
+    }, format: :json
-      }, format: :json
+    expect(response.status).to eq(403)
-    end.to raise_error(Discourse::NotLoggedIn)
   end
 
   it "raises an error if you aren't an admin" do

spec/controllers/topics_controller_spec.rb

@@ -9,12 +9,8 @@ def topics_controller_show_gen_perm_tests(expected, ctx)
 
     method = <<~TEXT
     it 'returns #{status} for #{sym}' do
-      begin
+      get :show, params: { #{params} }
-        get :show, params: { #{params} }
+      expect(response.status).to eq(#{status})
-        expect(response.status).to eq(#{status})
-      rescue Discourse::NotLoggedIn
-        expect(302).to eq(#{status})
-      end
     end
     TEXT
 
@@ -65,13 +61,12 @@ describe TopicsController do
 
   context 'move_posts' do
     it 'needs you to be logged in' do
-      expect do
+      post :move_posts, params: {
-        post :move_posts, params: {
+        topic_id: 111,
-          topic_id: 111,
+        title: 'blah',
-          title: 'blah',
+        post_ids: [1, 2, 3]
-          post_ids: [1, 2, 3]
+      }, format: :json
-        }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     describe 'moving to a new topic' do
@@ -244,11 +239,10 @@ describe TopicsController do
 
   context "merge_topic" do
     it 'needs you to be logged in' do
-      expect do
+      post :merge_topic, params: {
-        post :merge_topic, params: {
+        topic_id: 111, destination_topic_id: 345
-          topic_id: 111, destination_topic_id: 345
+      }, format: :json
-        }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     describe 'moving to a new topic' do
@@ -299,13 +293,12 @@ describe TopicsController do
 
   context 'change_post_owners' do
     it 'needs you to be logged in' do
-      expect do
+      post :change_post_owners, params: {
-        post :change_post_owners, params: {
+        topic_id: 111,
-          topic_id: 111,
+        username: 'user_a',
-          username: 'user_a',
+        post_ids: [1, 2, 3]
-          post_ids: [1, 2, 3]
+      }, format: :json
-        }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     describe 'forbidden to moderators' do
@@ -402,9 +395,8 @@ describe TopicsController do
     let(:params) { { topic_id: 1, timestamp: Time.zone.now } }
 
     it 'needs you to be logged in' do
-      expect do
+      put :change_timestamps, params: params, format: :json
-        put :change_timestamps, params: params, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     [:moderator, :trust_level_4].each do |user|
@@ -446,9 +438,8 @@ describe TopicsController do
 
   context 'clear_pin' do
     it 'needs you to be logged in' do
-      expect do
+      put :clear_pin, params: { topic_id: 1 }, format: :json
-        put :clear_pin, params: { topic_id: 1 }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     context 'when logged in' do
@@ -479,11 +470,10 @@ describe TopicsController do
 
   context 'status' do
     it 'needs you to be logged in' do
-      expect do
+      put :status, params: {
-        put :status, params: {
+        topic_id: 1, status: 'visible', enabled: true
-          topic_id: 1, status: 'visible', enabled: true
+      }, format: :json
-        }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     describe 'when logged in' do
@@ -519,11 +509,10 @@ describe TopicsController do
       end
 
       it 'raises an error with a status not in the whitelist' do
-        expect do
+        put :status, params: {
-          put :status, params: {
+          topic_id: @topic.id, status: 'title', enabled: 'true'
-            topic_id: @topic.id, status: 'title', enabled: 'true'
+        }, format: :json
-          }, format: :json
+        expect(response.status).to eq(400)
-        end.to raise_error(Discourse::InvalidParameters)
       end
 
       it 'should update the status of the topic correctly' do
@@ -551,9 +540,8 @@ describe TopicsController do
   context 'delete_timings' do
 
     it 'needs you to be logged in' do
-      expect do
+      delete :destroy_timings, params: { topic_id: 1 }, format: :json
-        delete :destroy_timings, params: { topic_id: 1 }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     context 'when logged in' do
@@ -575,23 +563,20 @@ describe TopicsController do
   describe 'mute/unmute' do
 
     it 'needs you to be logged in' do
-      expect do
+      put :mute, params: { topic_id: 99 }, format: :json
-        put :mute, params: { topic_id: 99 }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     it 'needs you to be logged in' do
-      expect do
+      put :unmute, params: { topic_id: 99 }, format: :json
-        put :unmute, params: { topic_id: 99 }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
   end
 
   describe 'recover' do
     it "won't allow us to recover a topic when we're not logged in" do
-      expect do
+      put :recover, params: { topic_id: 1 }, format: :json
-        put :recover, params: { topic_id: 1 }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     describe 'when logged in' do
@@ -622,9 +607,8 @@ describe TopicsController do
 
   describe 'delete' do
     it "won't allow us to delete a topic when we're not logged in" do
-      expect do
+      delete :destroy, params: { id: 1 }, format: :json
-        delete :destroy, params: { id: 1 }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     describe 'when logged in' do
@@ -822,10 +806,10 @@ describe TopicsController do
         expected = {
           normal_topic: 200,
           secure_topic: 403,
-          private_topic: 302,
+          private_topic: 404,
           deleted_topic: 410,
           deleted_secure_topic: 403,
-          deleted_private_topic: 302,
+          deleted_private_topic: 404,
           nonexist: 404
         }
         topics_controller_show_gen_perm_tests(expected, self)
@@ -1094,9 +1078,8 @@ describe TopicsController do
 
   describe 'update' do
     it "won't allow us to update a topic when we're not logged in" do
-      expect do
+      put :update, params: { topic_id: 1, slug: 'xyz' }, format: :json
-        put :update, params: { topic_id: 1, slug: 'xyz' }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     describe 'when logged in' do
@@ -1286,11 +1269,10 @@ describe TopicsController do
     end
 
     it "won't allow us to invite toa topic when we're not logged in" do
-      expect do
+      post :invite, params: {
-        post :invite, params: {
+        topic_id: 1, email: 'jake@adventuretime.ooo'
-          topic_id: 1, email: 'jake@adventuretime.ooo'
+      }, format: :json
-        }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     describe 'when logged in as group manager' do
@@ -1422,9 +1404,8 @@ describe TopicsController do
 
   describe "bulk" do
     it 'needs you to be logged in' do
-      expect do
+      put :bulk, format: :json
-        put :bulk, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     describe "when logged in" do
@@ -1500,9 +1481,8 @@ describe TopicsController do
 
   describe 'reset_new' do
     it 'needs you to be logged in' do
-      expect do
+      put :reset_new, format: :json
-        put :reset_new, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     let(:user) { log_in(:user) }
@@ -1587,9 +1567,8 @@ describe TopicsController do
 
   context "convert_topic" do
     it 'needs you to be logged in' do
-      expect do
+      put :convert_topic, params: { id: 111, type: "private" }, format: :json
-        put :convert_topic, params: { id: 111, type: "private" }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     describe 'converting public topic to private message' do

spec/controllers/uploads_controller_spec.rb

@@ -5,7 +5,8 @@ describe UploadsController do
   context '.create' do
 
     it 'requires you to be logged in' do
-      expect { post :create, format: :json }.to raise_error(Discourse::NotLoggedIn)
+      post :create, format: :json
+      expect(response.status).to eq(403)
     end
 
     context 'logged in' do

spec/controllers/user_api_keys_controller_spec.rb

@@ -55,9 +55,8 @@ describe UserApiKeysController do
   context 'create' do
 
     it "does not allow anon" do
-      expect {
+      post :create, params: args, format: :json
-        post :create, params: args, format: :json
+      expect(response.status).to eq(403)
-      }.to raise_error(Discourse::NotLoggedIn)
     end
 
     it "refuses to redirect to disallowed place" do

spec/controllers/users_controller_spec.rb

@@ -126,22 +126,7 @@ describe UsersController do
           topic_post_count = JSON.parse(response.body).dig("user", "topic_post_count")
           expect(topic_post_count[topic.id.to_s]).to eq(2)
         end
-
       end
-
-    end
-
-  end
-
-  describe '.user_preferences_redirect' do
-    it 'requires the user to be logged in' do
-      expect { get :user_preferences_redirect }.to raise_error(Discourse::NotLoggedIn)
-    end
-
-    it "redirects to their profile when logged in" do
-      user = log_in
-      get :user_preferences_redirect
-      expect(response).to redirect_to("/u/#{user.username_lower}/preferences")
     end
   end
 
@@ -1048,9 +1033,8 @@ describe UsersController do
 
   context '#username' do
     it 'raises an error when not logged in' do
-      expect do
+      put :username, params: { username: 'somename' }, format: :json
-        put :username, params: { username: 'somename' }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     context 'while logged in' do
@@ -1429,9 +1413,8 @@ describe UsersController do
   describe '#update' do
     context 'with guest' do
       it 'raises an error' do
-        expect do
+        put :update, params: { username: 'guest' }, format: :json
-          put :update, params: { username: 'guest' }, format: :json
+        expect(response.status).to eq(403)
-        end.to raise_error(Discourse::NotLoggedIn)
       end
     end
 
@@ -1827,11 +1810,10 @@ describe UsersController do
   describe '.pick_avatar' do
 
     it 'raises an error when not logged in' do
-      expect {
+      put :pick_avatar, params: {
-        put :pick_avatar, params: {
+        username: 'asdf', avatar_id: 1, type: "custom"
-          username: 'asdf', avatar_id: 1, type: "custom"
+      }, format: :json
-        }, format: :json
+      expect(response.status).to eq(403)
-      }.to raise_error(Discourse::NotLoggedIn)
     end
 
     context 'while logged in' do
@@ -1902,11 +1884,10 @@ describe UsersController do
   describe '.destroy_user_image' do
 
     it 'raises an error when not logged in' do
-      expect do
+      delete :destroy_user_image,
-        delete :destroy_user_image,
+        params: { type: 'profile_background', username: 'asdf' },
-          params: { type: 'profile_background', username: 'asdf' },
+        format: :json
-          format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     context 'while logged in' do
@@ -1930,11 +1911,10 @@ describe UsersController do
       end
 
       it "only allows certain `types`" do
-        expect do
+        delete :destroy_user_image,
-          delete :destroy_user_image,
+          params: { username: user.username, type: 'wat' },
-            params: { username: user.username, type: 'wat' },
+          format: :json
-            format: :json
+        expect(response.status).to eq(400)
-        end.to raise_error(Discourse::InvalidParameters)
       end
 
       it 'can clear the profile background' do
@@ -1951,9 +1931,8 @@ describe UsersController do
 
   describe '.destroy' do
     it 'raises an error when not logged in' do
-      expect do
+      delete :destroy, params: { username: 'nobody' }, format: :json
-        delete :destroy, params: { username: 'nobody' }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     context 'while logged in' do
@@ -2012,9 +1991,8 @@ describe UsersController do
   describe '.check_emails' do
 
     it 'raises an error when not logged in' do
-      expect do
+      put :check_emails, params: { username: 'zogstrip' }, format: :json
-        put :check_emails, params: { username: 'zogstrip' }, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     context 'while logged in' do
@@ -2166,9 +2144,8 @@ describe UsersController do
 
     context 'anon' do
       it "raises an error on anon for topic_tracking_state" do
-        expect {
+        get :topic_tracking_state, params: { username: user.username }, format: :json
-          get :topic_tracking_state, params: { username: user.username }, format: :json
+        expect(response.status).to eq(403)
-        }.to raise_error(Discourse::NotLoggedIn)
       end
     end
 

spec/controllers/wizard_controller_spec.rb

@@ -10,9 +10,8 @@ describe WizardController do
     end
 
     it 'needs you to be logged in' do
-      expect do
+      get :index, format: :json
-        get :index, format: :json
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     it "raises an error if you aren't an admin" do

spec/integration/rate_limiting_spec.rb

@@ -13,25 +13,25 @@ describe 'admin rate limit' do
   end
 
   it 'can cleanly limit requests' do
+    #request.set_header("action_dispatch.show_exceptions", true)
 
     admin = Fabricate(:admin)
     api_key = Fabricate(:api_key, key: SecureRandom.hex, user: admin)
 
     global_setting :max_admin_api_reqs_per_key_per_minute, 1
 
-    get '/admin/users.json', params: {
+    get '/admin/api/keys.json', params: {
       api_key: api_key.key,
       api_username: admin.username
     }
 
     expect(response.status).to eq(200)
 
-    get '/admin/users.json', params: {
+    get '/admin/api/keys.json', params: {
       api_key: api_key.key,
       api_username: admin.username
     }
 
     expect(response.status).to eq(429)
-
   end
 end

spec/requests/admin/admin_controller_spec.rb

@@ -2,8 +2,7 @@ require 'rails_helper'
 
 RSpec.describe Admin::AdminController do
   it "should return the right response if user isn't a staff" do
-    expect do
+    get "/admin", params: { api_key: 'asdiasiduga' }
-      get "/admin", params: { api_key: 'asdiasiduga' }
+    expect(response.status).to eq(404)
-    end.to raise_error(ActionController::RoutingError)
   end
 end

spec/requests/admin/backups_controller_spec.rb

@@ -25,8 +25,8 @@ RSpec.describe Admin::BackupsController do
     end
 
     it 'should not allow rollback via a GET request' do
-      expect { get "/admin/backups/rollback.json" }
+      get "/admin/backups/rollback.json"
-        .to raise_error(ActionController::RoutingError)
+      expect(response.status).to eq(404)
     end
   end
 
@@ -40,8 +40,8 @@ RSpec.describe Admin::BackupsController do
     end
 
     it 'should not allow cancel via a GET request' do
-      expect { get "/admin/backups/cancel.json" }
+      get "/admin/backups/cancel.json"
-        .to raise_error(ActionController::RoutingError)
+      expect(response.status).to eq(404)
     end
   end
 

spec/requests/admin/email_templates_controller_spec.rb

@@ -19,16 +19,14 @@ RSpec.describe Admin::EmailTemplatesController do
 
   context "#index" do
     it "raises an error if you aren't logged in" do
-      expect do
+      get '/admin/customize/email_templates.json'
-        get '/admin/customize/email_templates.json'
+      expect(response.status).to eq(404)
-      end.to raise_error(ActionController::RoutingError)
     end
 
     it "raises an error if you aren't an admin" do
       sign_in(user)
-      expect do
+      get '/admin/customize/email_templates.json'
-        get '/admin/customize/email_templates.json'
+      expect(response.status).to eq(404)
-      end.to raise_error(ActionController::RoutingError)
     end
 
     it "should work if you are an admin" do
@@ -44,20 +42,18 @@ RSpec.describe Admin::EmailTemplatesController do
 
   context "#update" do
     it "raises an error if you aren't logged in" do
-      expect do
+      put '/admin/customize/email_templates/some_id', params: {
-        put '/admin/customize/email_templates/some_id', params: {
+        email_template: { subject: 'Subject', body: 'Body' }
-          email_template: { subject: 'Subject', body: 'Body' }
+      }, headers: headers
-        }, headers: headers
+      expect(response.status).to eq(404)
-      end.to raise_error(ActionController::RoutingError)
     end
 
     it "raises an error if you aren't an admin" do
       sign_in(user)
-      expect do
+      put '/admin/customize/email_templates/some_id', params: {
-        put '/admin/customize/email_templates/some_id', params: {
+        email_template: { subject: 'Subject', body: 'Body' }
-          email_template: { subject: 'Subject', body: 'Body' }
+      }, headers: headers
-        }, headers: headers
+      expect(response.status).to eq(404)
-      end.to raise_error(ActionController::RoutingError)
     end
 
     context "when logged in as admin" do
@@ -224,16 +220,14 @@ RSpec.describe Admin::EmailTemplatesController do
 
   context "#revert" do
     it "raises an error if you aren't logged in" do
-      expect do
+      delete '/admin/customize/email_templates/some_id', headers: headers
-        delete '/admin/customize/email_templates/some_id', headers: headers
+      expect(response.status).to eq(404)
-      end.to raise_error(ActionController::RoutingError)
     end
 
     it "raises an error if you aren't an admin" do
       sign_in(user)
-      expect do
+      delete '/admin/customize/email_templates/some_id', headers: headers
-        delete '/admin/customize/email_templates/some_id', headers: headers
+      expect(response.status).to eq(404)
-      end.to raise_error(ActionController::RoutingError)
     end
 
     context "when logged in as admin" do

spec/requests/admin/search_logs_spec.rb

@@ -10,16 +10,14 @@ RSpec.describe Admin::SearchLogsController do
 
   context "#index" do
     it "raises an error if you aren't logged in" do
-      expect do
+      get '/admin/logs/search_logs.json'
-        get '/admin/logs/search_logs.json'
+      expect(response.status).to eq(404)
-      end.to raise_error(ActionController::RoutingError)
     end
 
     it "raises an error if you aren't an admin" do
       sign_in(user)
-      expect do
+      get '/admin/logs/search_logs.json'
-        get '/admin/logs/search_logs.json'
+      expect(response.status).to eq(404)
-      end.to raise_error(ActionController::RoutingError)
     end
 
     it "should work if you are an admin" do
@@ -35,16 +33,14 @@ RSpec.describe Admin::SearchLogsController do
 
   context "#term" do
     it "raises an error if you aren't logged in" do
-      expect do
+      get '/admin/logs/search_logs/term/ruby.json'
-        get '/admin/logs/search_logs/term/ruby.json'
+      expect(response.status).to eq(404)
-      end.to raise_error(ActionController::RoutingError)
     end
 
     it "raises an error if you aren't an admin" do
       sign_in(user)
-      expect do
+      get '/admin/logs/search_logs/term/ruby.json'
-        get '/admin/logs/search_logs/term/ruby.json'
+      expect(response.status).to eq(404)
-      end.to raise_error(ActionController::RoutingError)
     end
 
     it "should work if you are an admin" do

spec/requests/composer_controller_spec.rb

@@ -6,11 +6,10 @@ RSpec.describe ComposerController do
   describe '#parse_html' do
 
     it "should not be able access without sign in" do
-      expect {
+      post "/composer/parse_html.json", params: {
-        post "/composer/parse_html.json", params: {
+        html: "<strong>hello</strong>"
-          html: "<strong>hello</strong>"
+      }
-        }
+      expect(response.status).to eq(403)
-      }.to raise_error(Discourse::NotLoggedIn)
     end
 
     it "should convert html tags to markdown text" do

spec/requests/directory_items_controller_spec.rb

@@ -4,9 +4,8 @@ describe DirectoryItemsController do
   let!(:user) { Fabricate(:user) }
 
   it "requires a `period` param" do
-    expect do
+    get '/directory_items.json'
-      get '/directory_items.json'
+    expect(response.status).to eq(400)
-    end.to raise_error(ActionController::ParameterMissing)
   end
 
   it "requires a proper `period` param" do

spec/requests/email_controller_spec.rb

@@ -5,7 +5,6 @@ RSpec.describe EmailController do
     describe 'when email is invalid' do
       it 'should return the right response' do
         get '/email/unsubscribed', params: { email: 'somerandomstring' }
-
         expect(response.status).to eq(404)
       end
     end

spec/requests/groups_controller_spec.rb

@@ -245,11 +245,11 @@ describe GroupsController do
             public_exit: true
           )
 
-          expect { put "/groups/#{group.id}/members.json", params: { usernames: "bob" } }
+          put "/groups/#{group.id}/members.json", params: { usernames: "bob" }
-            .to raise_error(Discourse::NotLoggedIn)
+          expect(response.status).to eq(403)
 
-          expect { delete "/groups/#{group.id}/members.json", params: { username: "bob" } }
+          delete "/groups/#{group.id}/members.json", params: { username: "bob" }
-            .to raise_error(Discourse::NotLoggedIn)
+          expect(response.status).to eq(403)
         end
       end
     end
@@ -499,9 +499,8 @@ describe GroupsController do
   describe "group histories" do
     context 'when user is not signed in' do
       it 'should raise the right error' do
-        expect do
+        get "/groups/#{group.name}/logs.json"
-          get "/groups/#{group.name}/logs.json"
+        expect(response.status).to eq(403)
-        end.to raise_error(Discourse::NotLoggedIn)
       end
     end
 
@@ -587,17 +586,15 @@ describe GroupsController do
     let(:new_user) { Fabricate(:user) }
 
     it 'requires the user to log in' do
-      expect do
+      post "/groups/#{group.name}/request_membership.json"
-        post "/groups/#{group.name}/request_membership.json"
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     it 'requires a reason' do
       sign_in(user)
 
-      expect do
+      post "/groups/#{group.name}/request_membership.json"
-        post "/groups/#{group.name}/request_membership.json"
+      expect(response.status).to eq(400)
-      end.to raise_error(ActionController::ParameterMissing)
     end
 
     it 'should create the right PM' do
@@ -649,9 +646,8 @@ describe GroupsController do
 
     context 'as an anon user' do
       it "returns the right response" do
-        expect do
+        get '/groups/search.json'
-          get '/groups/search.json'
+        expect(response.status).to eq(403)
-        end.to raise_error(Discourse::NotLoggedIn)
       end
     end
 

spec/requests/post_actions_controller_spec.rb

@@ -1,7 +1,22 @@
 require 'rails_helper'
 
 RSpec.describe PostActionsController do
+  describe '#destroy' do
+    let(:post) { Fabricate(:post, user: Fabricate(:coding_horror)) }
+
+    it 'requires you to be logged in' do
+      delete '/post_action.json', params: { id: post.id }
+      expect(response.status).to eq(404)
+    end
+  end
+
   describe '#create' do
+
+    it 'requires you to be logged in' do
+      post '/post_actions.json'
+      expect(response.status).to eq(403)
+    end
+
     describe 'as a moderator' do
       let(:user) { Fabricate(:moderator) }
       let(:post_1) { Fabricate(:post, user: Fabricate(:coding_horror)) }
@@ -11,11 +26,10 @@ RSpec.describe PostActionsController do
       end
 
       it 'raises an error when the id is missing' do
-        expect do
+        post "/post_actions.json", params: {
-          post "/post_actions.json", params: {
+          post_action_type_id: PostActionType.types[:like]
-            post_action_type_id: PostActionType.types[:like]
+        }
-          }
+        expect(response.status).to eq(400)
-        end.to raise_error(ActionController::ParameterMissing)
       end
 
       it 'fails when the id is invalid' do
@@ -27,9 +41,8 @@ RSpec.describe PostActionsController do
       end
 
       it 'raises an error when the post_action_type_id index is missing' do
-        expect do
+        post "/post_actions.json", params: { id: post_1.id }
-          post "/post_actions.json", params: { id: post_1.id }
+        expect(response.status).to eq(400)
-        end.to raise_error(ActionController::ParameterMissing)
       end
 
       it "fails when the user doesn't have permission to see the post" do

spec/requests/topics_controller_spec.rb

@@ -44,12 +44,11 @@ RSpec.describe TopicsController do
   describe '#timer' do
     context 'when a user is not logged in' do
       it 'should return the right response' do
-        expect do
+        post "/t/#{topic.id}/timer.json", params: {
-          post "/t/#{topic.id}/timer.json", params: {
+          time: '24',
-            time: '24',
+          status_type: TopicTimer.types[1]
-            status_type: TopicTimer.types[1]
+        }
-          }
+        expect(response.status).to eq(403)
-        end.to raise_error(Discourse::NotLoggedIn)
       end
     end
 
@@ -75,8 +74,6 @@ RSpec.describe TopicsController do
       end
 
       it 'should be able to create a topic status update' do
-        time = 24
-
         post "/t/#{topic.id}/timer.json", params: {
           time: 24,
           status_type: TopicTimer.types[1]
@@ -148,12 +145,12 @@ RSpec.describe TopicsController do
 
       describe 'invalid status type' do
         it 'should raise the right error' do
-          expect do
+          post "/t/#{topic.id}/timer.json", params: {
-            post "/t/#{topic.id}/timer.json", params: {
+            time: 10,
-              time: 10,
+            status_type: 'something'
-              status_type: 'something'
+          }
-            }
+          expect(response.status).to eq(400)
-          end.to raise_error(Discourse::InvalidParameters)
+          expect(response.body).to include('status_type')
         end
       end
     end

spec/requests/users_controller_spec.rb

@@ -327,4 +327,17 @@ RSpec.describe UsersController do
       end
     end
   end
+
+  describe '.user_preferences_redirect' do
+    it 'requires the user to be logged in' do
+      get '/user_preferences'
+      expect(response.status).to eq(404)
+    end
+
+    it "redirects to their profile when logged in" do
+      sign_in(user)
+      get '/user_preferences'
+      expect(response).to redirect_to("/u/#{user.username_lower}/preferences")
+    end
+  end
 end

spec/requests/users_email_controller_spec.rb

@@ -67,9 +67,8 @@ describe UsersEmailController do
     let(:new_email) { 'bubblegum@adventuretime.ooo' }
 
     it "requires you to be logged in" do
-      expect do
+      put "/u/asdf/preferences/email.json"
-        put "/u/asdf/preferences/email.json"
+      expect(response.status).to eq(403)
-      end.to raise_error(Discourse::NotLoggedIn)
     end
 
     context 'when logged in' do
@@ -80,9 +79,8 @@ describe UsersEmailController do
       end
 
       it 'raises an error without an email parameter' do
-        expect do
+        put "/u/#{user.username}/preferences/email.json"
-          put "/u/#{user.username}/preferences/email.json"
+        expect(response.status).to eq(400)
-        end.to raise_error(ActionController::ParameterMissing)
       end
 
       it "raises an error if you can't edit the user's email" do