app controller: standardize access denied error handling.

Refactor to use `render_error_page` to handle User::PrivilegeError
exceptions. This way these exceptions are logged to New Relic.

Changes:

* Anonymous users aren't automatically redirected to the login page.
  Instead they're taken to the access denied page, which links to the
  login/signup pages.

* JSON/XML error responses return `message` instead of `reason`.

app/controllers/application_controller.rb

@@ -13,7 +13,6 @@ class ApplicationController < ActionController::Base
   layout "default"
 
   rescue_from Exception, :with => :rescue_exception
-  rescue_from User::PrivilegeError, :with => :access_denied
 
   protected
 
@@ -59,12 +58,14 @@ class ApplicationController < ActionController::Base
       render_error_page(401, exception)
     when ActionController::InvalidAuthenticityToken, ActionController::UnpermittedParameters, ActionController::InvalidCrossOriginRequest
       render_error_page(403, exception)
+    when User::PrivilegeError
+      render_error_page(403, exception, template: "static/access_denied")
     when ActiveRecord::RecordNotFound
       render_error_page(404, exception, message: "That record was not found.")
     when ActionController::RoutingError
       render_error_page(405, exception)
     when ActionController::UnknownFormat, ActionView::MissingTemplate
-      render_error_page(406, exception, message: "#{request.format.to_s} is not a supported format for this page", format: :html)
+      render_error_page(406, exception, message: "#{request.format.to_s} is not a supported format for this page")
     when Danbooru::Paginator::PaginationError
       render_error_page(410, exception)
     when Post::SearchError
@@ -80,45 +81,19 @@ class ApplicationController < ActionController::Base
     end
   end
 
-  def render_error_page(status, exception, message: exception.message, format: request.format.symbol)
+  def render_error_page(status, exception, message: exception.message, template: "static/error")
     @exception = exception
     @expected = status < 500
     @message = message.encode("utf-8", { invalid: :replace, undef: :replace })
     @backtrace = Rails.backtrace_cleaner.clean(@exception.backtrace)
-    format = :html unless format.in?(%i[html json xml js atom])
 
     # if InvalidAuthenticityToken was raised, CurrentUser isn't set so we have to use the blank layout.
     layout = CurrentUser.user.present? ? "default" : "blank"
 
     DanbooruLogger.log(@exception, expected: @expected)
-    render "static/error", layout: layout, status: status, formats: format
+    render template, layout: layout, status: status
-  end
+  rescue ActionView::MissingTemplate
-
+    render "static/error.html", layout: layout, status: status
-  def access_denied(exception = nil)
-    previous_url = params[:url] || request.fullpath
-
-    respond_to do |fmt|
-      fmt.html do
-        if CurrentUser.is_anonymous?
-          if request.get?
-            redirect_to new_session_path(:url => previous_url), :notice => "Access denied"
-          else
-            redirect_to new_session_path, :notice => "Access denied"
-          end
-        else
-          render :template => "static/access_denied", :status => 403
-        end
-      end
-      fmt.xml do
-        render :xml => {:success => false, :reason => "access denied"}.to_xml(:root => "response"), :status => 403
-      end
-      fmt.json do
-        render :json => {:success => false, :reason => "access denied"}.to_json, :status => 403
-      end
-      fmt.js do
-        render js: "", :status => 403
-      end
-    end
   end
 
   def set_current_user

app/views/static/access_denied.html.erb

@@ -1,6 +1,13 @@
 <h1>Access Denied</h1>
 
-<p>You do not have permission to visit this page.</p>
+<p>
+  You do not have permission to visit this page.
+
+  <% if CurrentUser.is_anonymous? %>
+    Try <%= link_to "logging in", new_session_path(url: request.fullpath) %> or
+    <%= link_to "signing up", new_user_path %>.
+  <% end %>
+</p>
 
 <%= link_to "Go back", :back, :rel => "prev" %>
 

test/functional/application_controller_test.rb

@@ -120,13 +120,6 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest
         end
       end
 
-      context "without any authentication" do
-        should "redirect to the login page" do
-          get edit_user_path(@user)
-          assert_redirected_to new_session_path(url: edit_user_path(@user))
-        end
-      end
-
       context "with cookie-based authentication" do
         should "not allow non-GET requests without a CSRF token" do
           # get the csrf token from the login page so we can login
@@ -158,6 +151,15 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest
       end
     end
 
+    context "accessing an unauthorized page" do
+      should "render the access denied page" do
+        get news_updates_path
+
+        assert_response 403
+        assert_select "h1", /Access Denied/
+      end
+    end
+
     context "when the api limit is exceeded" do
       should "fail with a 429 error" do
         user = create(:user)

test/functional/post_votes_controller_test.rb

@@ -17,7 +17,8 @@ class PostVotesControllerTest < ActionDispatch::IntegrationTest
       end
 
       should "not allow banned users to vote" do
-        @banned = create(:banned_user)
+        @banned = create(:user)
+        @ban = create(:ban, user: @banned)
         post_auth post_votes_path(post_id: @post.id), @banned, params: {:score => "up", :format => "js"}
         assert_response 403
         assert_equal(0, @post.reload.score)