From 60052d15ca02b1c40ed265bed6515880d2851b8f Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Thu, 10 Jul 2014 12:13:30 +0200
Subject: clean up and simplify error responses and test code

---
 .../controller_extension/authentication.rb         | 24 +++++++--------
 .../controller_extension/token_authentication.rb   |  2 +-
 .../test/functional/customers_controller_test.rb   |  6 ++--
 .../test/functional/tickets_controller_test.rb     |  2 +-
 test/functional/application_controller_test.rb     |  4 +--
 test/functional/users_controller_test.rb           |  2 +-
 test/functional/v1/messages_controller_test.rb     |  2 +-
 test/functional/v1/users_controller_test.rb        |  4 ++-
 test/integration/api/cert_test.rb                  |  2 +-
 test/integration/api/login_test.rb                 |  2 +-
 test/integration/api/smtp_cert_test.rb             |  2 +-
 test/integration/api/update_account_test.rb        |  2 +-
 test/support/api_integration_test.rb               |  5 ++++
 test/support/auth_test_helper.rb                   | 34 ++++++++++++----------
 test/support/rack_test.rb                          |  7 ++++-
 15 files changed, 56 insertions(+), 44 deletions(-)

diff --git a/app/controllers/controller_extension/authentication.rb b/app/controllers/controller_extension/authentication.rb
index fae5145..687bc6e 100644
--- a/app/controllers/controller_extension/authentication.rb
+++ b/app/controllers/controller_extension/authentication.rb
@@ -27,26 +27,24 @@ module ControllerExtension::Authentication
   end
 
   def access_denied
-    respond_to do |format|
-      format.html do
-        redirect_to home_url, :alert => t(:not_authorized)
-      end
-      format.json do
-        render :json => {'error' => t(:not_authorized)}, status: :forbidden
-      end
-    end
+    respond_to_error :not_authorized, :forbidden, home_url
   end
 
   def login_required
+    # Warden will intercept the 401 response and call
+    # SessionController#unauthenticated instead.
+    respond_to_error :not_authorized_login, :unauthorized, login_url
+  end
+
+  def respond_to_error(message, status=nil, redirect=nil)
+    message = t(message) if message.is_a?(Symbol)
     respond_to do |format|
       format.html do
-        redirect_to login_url, alert: t(:not_authorized_login)
+        redirect_to redirect, alert: message
       end
       format.json do
-        # Warden will intercept the 401 response and call
-        # SessionController#unauthenticated instead.
-        render json: {error: t(:not_authorized_login)},
-          status: :unauthorized
+        status ||= :unprocessable_entity
+        render json: {error: message}, status: status
       end
     end
   end
diff --git a/app/controllers/controller_extension/token_authentication.rb b/app/controllers/controller_extension/token_authentication.rb
index b0ed624..1cb6ffa 100644
--- a/app/controllers/controller_extension/token_authentication.rb
+++ b/app/controllers/controller_extension/token_authentication.rb
@@ -12,7 +12,7 @@ module ControllerExtension::TokenAuthentication
   end
 
   def require_token
-    access_denied unless token_authenticate
+    login_required unless token_authenticate
   end
 
   def logout
diff --git a/engines/billing/test/functional/customers_controller_test.rb b/engines/billing/test/functional/customers_controller_test.rb
index 46c33c9..cc82fc1 100644
--- a/engines/billing/test/functional/customers_controller_test.rb
+++ b/engines/billing/test/functional/customers_controller_test.rb
@@ -27,11 +27,11 @@ class CustomersControllerTest < ActionController::TestCase
 
   test "no access if not logged in" do
     get :new
-    assert_access_denied(true, false)
+    assert_login_required
     get :show, :id => @customer.braintree_customer_id
-    assert_access_denied(true, false)
+    assert_login_required
     get :edit, :id => @customer.braintree_customer_id
-    assert_access_denied(true, false)
+    assert_login_required
   end
 
 
diff --git a/engines/support/test/functional/tickets_controller_test.rb b/engines/support/test/functional/tickets_controller_test.rb
index e36f5f6..a7a2011 100644
--- a/engines/support/test/functional/tickets_controller_test.rb
+++ b/engines/support/test/functional/tickets_controller_test.rb
@@ -45,7 +45,7 @@ class TicketsControllerTest < ActionController::TestCase
     user = find_record :user
     ticket = find_record :ticket, :created_by => user.id
     get :show, :id => ticket.id
-    assert_login_required
+    assert_access_denied
   end
 
   test "user tickets are visible to creator" do
diff --git a/test/functional/application_controller_test.rb b/test/functional/application_controller_test.rb
index c4c922b..b558ad8 100644
--- a/test/functional/application_controller_test.rb
+++ b/test/functional/application_controller_test.rb
@@ -9,13 +9,13 @@ class ApplicationControllerTest < ActionController::TestCase
 
   def test_require_login_redirect
     @controller.send(:require_login)
-    assert_access_denied(true, false)
+    assert_login_required
   end
 
   def test_require_login
     login
     @controller.send(:require_login)
-    assert_access_denied(false)
+    assert_access_granted
   end
 
   def test_require_admin
diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb
index 4af9ca6..7d1745c 100644
--- a/test/functional/users_controller_test.rb
+++ b/test/functional/users_controller_test.rb
@@ -52,7 +52,7 @@ class UsersControllerTest < ActionController::TestCase
     nonid = 'thisisnotanexistinguserid'
 
     get :show, :id => nonid
-    assert_access_denied(true, false)
+    assert_login_required
   end
 
   test "may not show non-existing user without admin" do
diff --git a/test/functional/v1/messages_controller_test.rb b/test/functional/v1/messages_controller_test.rb
index 24a5b1f..a50fded 100644
--- a/test/functional/v1/messages_controller_test.rb
+++ b/test/functional/v1/messages_controller_test.rb
@@ -51,7 +51,7 @@ class V1::MessagesControllerTest < ActionController::TestCase
 
   test "fails if not authenticated" do
     get :index, :format => :json
-    assert_access_denied
+    assert_login_required
   end
 
 end
diff --git a/test/functional/v1/users_controller_test.rb b/test/functional/v1/users_controller_test.rb
index fe3cfe7..ffe2484 100644
--- a/test/functional/v1/users_controller_test.rb
+++ b/test/functional/v1/users_controller_test.rb
@@ -34,7 +34,9 @@ class V1::UsersControllerTest < ActionController::TestCase
   test "user cannot update other user" do
     user = find_record :user
     login
-    put :update, :user => record_attributes_for(:user_with_settings), :id => user.id, :format => :json
+    put :update, id: user.id,
+      user: record_attributes_for(:user_with_settings),
+      :format => :json
     assert_access_denied
   end
 
diff --git a/test/integration/api/cert_test.rb b/test/integration/api/cert_test.rb
index 74d439a..118fb9f 100644
--- a/test/integration/api/cert_test.rb
+++ b/test/integration/api/cert_test.rb
@@ -14,7 +14,7 @@ class CertTest < ApiIntegrationTest
 
   test "fetching certs requires login by default" do
     get '/1/cert', {}, RACK_ENV
-    assert_json_response error: I18n.t(:not_authorized)
+    assert_login_required
   end
 
   test "retrieve anonymous eip cert" do
diff --git a/test/integration/api/login_test.rb b/test/integration/api/login_test.rb
index 92d153f..f37639e 100644
--- a/test/integration/api/login_test.rb
+++ b/test/integration/api/login_test.rb
@@ -45,6 +45,6 @@ class LoginTest < SrpTest
   test "logout requires token" do
     authenticate
     logout(nil, {})
-    assert_equal 422, last_response.status
+    assert_login_required
   end
 end
diff --git a/test/integration/api/smtp_cert_test.rb b/test/integration/api/smtp_cert_test.rb
index 7697e44..aee52cf 100644
--- a/test/integration/api/smtp_cert_test.rb
+++ b/test/integration/api/smtp_cert_test.rb
@@ -48,7 +48,7 @@ class SmtpCertTest < ApiIntegrationTest
   test "no anonymous smtp certs" do
     with_config allow_anonymous_certs: true do
       post '/1/smtp_cert', {}, RACK_ENV
-      assert_json_response error: I18n.t(:not_authorized)
+      assert_login_required
     end
   end
 end
diff --git a/test/integration/api/update_account_test.rb b/test/integration/api/update_account_test.rb
index 63429e7..16bbb8c 100644
--- a/test/integration/api/update_account_test.rb
+++ b/test/integration/api/update_account_test.rb
@@ -16,7 +16,7 @@ class UpdateAccountTest < SrpTest
     authenticate
     put "http://api.lvh.me:3000/1/users/" + @user.id + '.json',
       user_params(password: "No! Verify me instead.")
-    assert_access_denied
+    assert_login_required
   end
 
   test "update password via api" do
diff --git a/test/support/api_integration_test.rb b/test/support/api_integration_test.rb
index bd10f11..ccf7066 100644
--- a/test/support/api_integration_test.rb
+++ b/test/support/api_integration_test.rb
@@ -14,6 +14,11 @@ class ApiIntegrationTest < ActionDispatch::IntegrationTest
     @token.save
   end
 
+  def assert_login_required
+    assert_equal 401, get_response.status
+    assert_json_response error: I18n.t(:not_authorized_login)
+  end
+
   teardown do
     if @user && @user.persisted?
       Identity.destroy_all_for @user
diff --git a/test/support/auth_test_helper.rb b/test/support/auth_test_helper.rb
index 38c2ea1..79d07d6 100644
--- a/test/support/auth_test_helper.rb
+++ b/test/support/auth_test_helper.rb
@@ -20,28 +20,30 @@ module AuthTestHelper
   end
 
   def assert_login_required
-    assert_access_denied(true, false)
+    assert_error_response :not_authorized_login, :unauthorized, login_url
   end
 
-  def assert_access_denied(denied = true, logged_in = true)
-    if denied
-      if @response.content_type == 'application/json'
-        assert_json_response('error' => I18n.t(:not_authorized))
-        assert_response :unprocessable_entity
-      else
-        if logged_in
-          assert_equal({:alert => I18n.t(:not_authorized)}, flash.to_hash)
-          assert_redirected_to home_url
-        else
-          assert_equal({:alert => I18n.t(:not_authorized_login)}, flash.to_hash)
-          assert_redirected_to login_url
-        end
-      end
+  def assert_access_denied
+    assert_error_response :not_authorized, :forbidden, home_url
+  end
+
+  def assert_error_response(message, status=nil, redirect=nil)
+    message = I18n.t(message) if message.is_a? Symbol
+    if @response.content_type == 'application/json'
+      status ||= :unprocessable_entity
+      assert_json_response('error' => message)
+      assert_response status
     else
-      assert flash[:alert].blank?
+      assert_equal({:alert => message}, flash.to_hash)
+      assert_redirected_to redirect
     end
   end
 
+  def assert_access_granted
+    assert flash[:alert].blank?,
+      "expected to have access but there was a flash alert"
+  end
+
   def expect_logout
     expect_warden_logout
     @token.expects(:destroy) if @token
diff --git a/test/support/rack_test.rb b/test/support/rack_test.rb
index 806339a..83adf6c 100644
--- a/test/support/rack_test.rb
+++ b/test/support/rack_test.rb
@@ -13,7 +13,12 @@ class RackTest < ActiveSupport::TestCase
 
   def assert_access_denied
     assert_json_response('error' => I18n.t(:not_authorized))
-    assert_response :unprocessable_entity
+    assert_response :forbidden
+  end
+
+  def assert_login_required
+    assert_json_response('error' => I18n.t(:not_authorized_login))
+    assert_response :unauthorized
   end
 
   # inspired by rails 4
-- 
cgit v1.2.3