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

---
 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 +++++-
 11 files changed, 40 insertions(+), 26 deletions(-)

(limited to 'test')

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


From 3885308e9a2aa48f25313567525e375362253f47 Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Thu, 10 Jul 2014 12:54:34 +0200
Subject: minor: fix identity test for storing certs

we compare the cert that expires last to the one we just saved. So we need to make sure the one we saved is the one that expires last.
---
 test/unit/identity_test.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

(limited to 'test')

diff --git a/test/unit/identity_test.rb b/test/unit/identity_test.rb
index cb0f6bd..77104b6 100644
--- a/test/unit/identity_test.rb
+++ b/test/unit/identity_test.rb
@@ -177,7 +177,9 @@ class IdentityTest < ActiveSupport::TestCase
   end
 
   def cert_stub
-    @cert_stub ||= stub expiry: 1.month.from_now,
+    # make this expire later than the others so it's on top
+    # when sorting by expiry descending.
+    @cert_stub ||= stub expiry: 2.month.from_now,
     fingerprint: SecureRandom.hex
   end
 end
-- 
cgit v1.2.3


From 2f1ceb63bfef2fa7d92fcbad73a5ead5bd17b23e Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Mon, 14 Jul 2014 17:59:09 +0200
Subject: minor: remove @s added by search and replace

meant to move id -> @id, also turned identity in the test titles into
@identity.
---
 test/unit/identity_test.rb | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

(limited to 'test')

diff --git a/test/unit/identity_test.rb b/test/unit/identity_test.rb
index 77104b6..f5c95f8 100644
--- a/test/unit/identity_test.rb
+++ b/test/unit/identity_test.rb
@@ -14,23 +14,23 @@ class IdentityTest < ActiveSupport::TestCase
     end
   end
 
-  test "blank @identity does not crash on valid?" do
+  test "blank identity does not crash on valid?" do
     @id = Identity.new
     assert !@id.valid?
   end
 
-  test "enabled @identity requires destination" do
+  test "enabled identity requires destination" do
     @id = Identity.new user: @user, address: @user.email_address
     assert !@id.valid?
     assert_equal ["can't be blank"], @id.errors[:destination]
   end
 
-  test "disabled @identity requires no destination" do
+  test "disabled identity requires no destination" do
     @id = Identity.new address: @user.email_address
     assert @id.valid?
   end
 
-  test "initial @identity for a user" do
+  test "initial identity for a user" do
     @id = Identity.for(@user)
     assert_equal @user.email_address, @id.address
     assert_equal @user.email_address, @id.destination
@@ -90,7 +90,7 @@ class IdentityTest < ActiveSupport::TestCase
     assert_equal @id.keys[:pgp], result["value"]
   end
 
-  test "fail to add non-local email address as @identity address" do
+  test "fail to add non-local email address as identity address" do
     @id = Identity.for @user, address: forward_address
     assert !@id.valid?
     assert_match /needs to end in/, @id.errors[:address].first
@@ -110,7 +110,7 @@ class IdentityTest < ActiveSupport::TestCase
     assert @id.errors.messages[:destination].include? "needs to be a valid email address"
   end
 
-  test "disabled @identity" do
+  test "disabled identity" do
     @id = Identity.for(@user)
     @id.disable
     assert_equal @user.email_address, @id.address
@@ -120,7 +120,7 @@ class IdentityTest < ActiveSupport::TestCase
     assert @id.valid?
   end
 
-  test "disabled @identity blocks handle" do
+  test "disabled identity blocks handle" do
     @id = Identity.for(@user)
     @id.disable
     @id.save
-- 
cgit v1.2.3


From e8a3df62d14c8dd775811f4af885cf7e76d5d3f6 Mon Sep 17 00:00:00 2001
From: Azul <azul@leap.se>
Date: Thu, 17 Jul 2014 11:18:57 +0200
Subject: clean up error assertions in tests

We're not testing the redirects anymore. But the error messages should be pretty clear already. We can start testing redirects again once we redirect to different places for different actions.
---
 test/integration/api/smtp_cert_test.rb |  2 +-
 test/integration/api/srp_test.rb       |  1 -
 test/support/api_integration_test.rb   |  5 -----
 test/support/assert_responses.rb       | 19 +++++++++++++++++++
 test/support/auth_test_helper.rb       | 20 --------------------
 test/support/rack_test.rb              | 11 +----------
 6 files changed, 21 insertions(+), 37 deletions(-)

(limited to 'test')

diff --git a/test/integration/api/smtp_cert_test.rb b/test/integration/api/smtp_cert_test.rb
index aee52cf..b1bfd43 100644
--- a/test/integration/api/smtp_cert_test.rb
+++ b/test/integration/api/smtp_cert_test.rb
@@ -42,7 +42,7 @@ class SmtpCertTest < ApiIntegrationTest
   test "fetching smtp certs requires email account" do
     login
     post '/1/smtp_cert', {}, RACK_ENV
-    assert_json_response error: I18n.t(:not_authorized)
+    assert_access_denied
   end
 
   test "no anonymous smtp certs" do
diff --git a/test/integration/api/srp_test.rb b/test/integration/api/srp_test.rb
index 26adc8c..946450e 100644
--- a/test/integration/api/srp_test.rb
+++ b/test/integration/api/srp_test.rb
@@ -1,5 +1,4 @@
 class SrpTest < RackTest
-  include AssertResponses
 
   teardown do
     if @user
diff --git a/test/support/api_integration_test.rb b/test/support/api_integration_test.rb
index ccf7066..bd10f11 100644
--- a/test/support/api_integration_test.rb
+++ b/test/support/api_integration_test.rb
@@ -14,11 +14,6 @@ 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/assert_responses.rb b/test/support/assert_responses.rb
index 19c2768..1c9d49d 100644
--- a/test/support/assert_responses.rb
+++ b/test/support/assert_responses.rb
@@ -55,6 +55,25 @@ module AssertResponses
       get_response.headers["Content-Disposition"]
   end
 
+  def assert_login_required
+    assert_error_response :not_authorized_login, :unauthorized
+  end
+
+  def assert_access_denied
+    assert_error_response :not_authorized, :forbidden
+  end
+
+  def assert_error_response(key, status=nil)
+    message = I18n.t(key)
+    if content_type == 'application/json'
+      status ||= :unprocessable_entity
+      assert_json_response('error' => key.to_s, 'message' => message)
+      assert_response status
+    else
+      assert_equal({:alert => message}, flash.to_hash)
+    end
+  end
+
 end
 
 class ::ActionController::TestCase
diff --git a/test/support/auth_test_helper.rb b/test/support/auth_test_helper.rb
index 79d07d6..7af3341 100644
--- a/test/support/auth_test_helper.rb
+++ b/test/support/auth_test_helper.rb
@@ -19,26 +19,6 @@ module AuthTestHelper
     return @current_user
   end
 
-  def assert_login_required
-    assert_error_response :not_authorized_login, :unauthorized, login_url
-  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_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"
diff --git a/test/support/rack_test.rb b/test/support/rack_test.rb
index 83adf6c..2c9fa9a 100644
--- a/test/support/rack_test.rb
+++ b/test/support/rack_test.rb
@@ -3,6 +3,7 @@ require_relative 'assert_responses'
 class RackTest < ActiveSupport::TestCase
   include Rack::Test::Methods
   include Warden::Test::Helpers
+  include AssertResponses
 
   CONFIG_RU = (Rails.root + 'config.ru').to_s
   OUTER_APP = Rack::Builder.parse_file(CONFIG_RU).first
@@ -11,16 +12,6 @@ class RackTest < ActiveSupport::TestCase
     OUTER_APP
   end
 
-  def assert_access_denied
-    assert_json_response('error' => I18n.t(:not_authorized))
-    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
   # -> actionpack/lib/action_dispatch/testing/assertions/response.rb
   def assert_response(type, message = nil)
-- 
cgit v1.2.3