From 18686aa084c596564e92d9d6ee5e861f60cdd849 Mon Sep 17 00:00:00 2001
From: tobiasfeistmantl <tobias@feistmantl.io>
Date: Thu, 4 Mar 2021 09:54:08 +0000
Subject: [PATCH] Implement authentication/authorization verifier

---
 README.md                                     | 42 +++++++------------
 lib/active_entry.rb                           | 22 +++++++++-
 lib/active_entry/errors.rb                    | 20 ++++++++-
 spec/authentication_spec.rb                   |  4 +-
 spec/authorization_spec.rb                    |  4 +-
 ...ation_verification_test_controller_spec.rb | 15 +++++++
 ...ation_verification_test_controller_spec.rb | 15 +++++++
 ...entication_verification_test_controller.rb | 15 +++++++
 ...horization_verification_test_controller.rb | 15 +++++++
 spec/dummy/config/routes.rb                   |  6 +++
 spec/verify_authentication_spec.rb            | 34 +++++++++++++++
 spec/verify_authorization_spec.rb             | 34 +++++++++++++++
 12 files changed, 191 insertions(+), 35 deletions(-)
 create mode 100644 spec/controllers/authentication_verification_test_controller_spec.rb
 create mode 100644 spec/controllers/authorization_verification_test_controller_spec.rb
 create mode 100644 spec/dummy/app/controllers/authentication_verification_test_controller.rb
 create mode 100644 spec/dummy/app/controllers/authorization_verification_test_controller.rb
 create mode 100644 spec/verify_authentication_spec.rb
 create mode 100644 spec/verify_authorization_spec.rb

diff --git a/README.md b/README.md
index 0108453..4005289 100644
--- a/README.md
+++ b/README.md
@@ -23,16 +23,29 @@ $ gem install active_entry
 
 ## Usage
 With Active Entry authentication and authorization is done in your Rails controllers. To enable authentication and authorization in one of your controllers, just add a before action for `authenticate!` and `authorize!` and the user has to authenticate and authorize on every call.
-You probably want to control authentication and authorization for every controller action you have in your app. To enable this, just add the before action to the `ApplicationController`.
+
+### Verify authentication and authorization
+You probably want to control authentication and authorization for every controller action you have in your app. As a safeguard to ensure, that auth is performed in every controller and the call for auth is not forgotten in development, add the `#verify_authentication!` and `#verify_authorization` as after action callbacks to your `ApplicationController`.
 
 ```ruby
 class ApplicationController < ActionController::Base
+  before_action :verify_authentication!, :verify_authorization!
+  # ...
+end
+```
+This ensures, that you call `authenticate!` and/or `authorize!` in all your controllers and raises an `ActiveEntry::AuthenticationNotPerformedError` / `ActiveEntry::AuthorizationNotPerformedError` if not.
+
+### Perform authentication and authorization
+in order to do the actual authentication and authorization, you have to add `authenticate!` and `authorize!` as before action callback in your controllers.
+
+```ruby
+class DashboardController < ApplicationController
   before_action :authenticate!, :authorize!
   # ...
 end
 ```
 
-If you try to open a page, you will get an `ActiveEntry::AuthenticationNotPerformedError` or `ActiveEntry::AuthorizationNotPerformedError`. This means that you have to instruct Active Entry when a user is authenticated/authorized and when not.
+If you try to open a page, you will get an `ActiveEntry::AuthenticationDecisionMakerMissingError` or `ActiveEntry::AuthorizationDecisionMakerMissingError`. This means that you have to instruct Active Entry when a user is authenticated/authorized and when not.
 You can do this by defining the methods `authenticated?` and `authorized?` in your controller.
 
 ```ruby
@@ -194,31 +207,6 @@ class ApplicationController < ActionController::Base
 end
 ```
 
-## Known Issues
-The authentication/authorization is done in a before action. These Rails controller before callbacks are done in defined order. If you set an instance variable which is needed in the `authenticated?` or `authorized?` method, you have to call the before action after the other method again.
-
-For example if you set `@user` in your controller in the `set_user` before action and you want to use the variable in `authorized?` action, you have to add the `authenticate!` or `authorize!` method after the `set_user` again, otherwise `@user` won't be available in `authenticate!` or `authorized?` yet.
-
-```ruby
-class UsersController < ApplicationController
-  before_action :set_user
-  before_action :authenticate!, :authorize!
-
-  def show
-  end
-
-  private
-
-  def authenticated?
-    return true if user_signed_in?
-  end
-
-  def authorized?
-    return true if current_user == @user
-  end
-end
-```
-
 ## Contributing
 Create pull requests on Github and help us to improve this Gem. There are some guidelines to follow:
 
diff --git a/lib/active_entry.rb b/lib/active_entry.rb
index dbd56b3..b71b0c2 100644
--- a/lib/active_entry.rb
+++ b/lib/active_entry.rb
@@ -4,6 +4,11 @@
 require "active_entry/railtie" if defined? Rails::Railtie
 
 module ActiveEntry
+  # Verifies that #authenticate! has been called in the controller.
+  def verify_authentication!
+    raise ActiveEntry::AuthenticationNotPerformedError unless @_authentication_done == true
+  end
+
   # Authenticates the user
   def authenticate!
     general_decision_maker_method_name = :authenticated?
@@ -20,7 +25,7 @@ def authenticate!
     #
     # This ensures that you actually do authentication in your controller.
     if !scoped_decision_maker_defined && !general_decision_maker_defined 
-      raise ActiveEntry::AuthenticationNotPerformedError
+      raise ActiveEntry::AuthenticationDecisionMakerMissingError
     end
     
     error = {}
@@ -37,6 +42,15 @@ def authenticate!
     # Use the .rescue_from method from ActionController::Base
     # to catch the exception and show the user a proper error message.
     raise ActiveEntry::NotAuthenticatedError.new(error) unless is_authenticated == true
+
+    # Tell #verify_authentication! that authentication
+    # has been performed.
+    @_authentication_done = true
+  end
+
+  # Verifies that #authorize! has been called in the controller.
+  def verify_authorization!
+    raise ActiveEntry::AuthorizationNotPerformedError unless @_authorization_done == true
   end
   
   # Authorizes the user.
@@ -55,7 +69,7 @@ def authorize!
     #
     # This ensures that you actually do authorization in your controller. 
     if !scoped_decision_maker_defined && !general_decision_maker_defined 
-      raise ActiveEntry::AuthorizationNotPerformedError
+      raise ActiveEntry::AuthorizationDecisionMakerMissingError
     end
     
     error = {}
@@ -72,5 +86,9 @@ def authorize!
     # Use the .rescue_from method from ActionController::Base
     # to catch the exception and show the user a proper error message.
     raise ActiveEntry::NotAuthorizedError.new(error) unless is_authorized == true
+
+    # Tell #verify_authorization! that authorization
+    # has been performed.
+    @_authorization_done = true
   end
 end
diff --git a/lib/active_entry/errors.rb b/lib/active_entry/errors.rb
index d0b280e..4b6d0b6 100644
--- a/lib/active_entry/errors.rb
+++ b/lib/active_entry/errors.rb
@@ -11,11 +11,19 @@ class AuthorizationError < StandardError
   # Error for controllers in which authorization isn't handled.
   #
   # @raise [AuthorizationNotPerformedError]
-  #    if the #authorized? method isn't defined
+  #    if authorize! is not called
   #    in the controller class.
   class AuthorizationNotPerformedError < AuthorizationError
   end
 
+  # Error for controllers in which authorization decision maker is missing.
+  #
+  # @raise [AuthorizationDecisionMakerMissingError]
+  #    if the #authorized? method isn't defined
+  #    in the controller class.
+  class AuthorizationDecisionMakerMissingError < AuthorizationError
+  end
+
   # Error if user unauthorized.
   #
   # @raise [NotAuthorizedError]
@@ -43,11 +51,19 @@ class AuthenticationError < StandardError
   # Error for controllers in which authentication isn't handled.
   #
   # @raise [AuthenticationNotPerformedError]
-  #    if the #authenticated? method isn't defined
+  #    if authenticate! is not called
   #    in the controller class.
   class AuthenticationNotPerformedError < AuthenticationError
   end
 
+  # Error for controllers in which authentication decision maker is missing.
+  #
+  # @raise [AuthenticationDecisionMakerMissingError]
+  #    if the #authenticated? method isn't defined
+  #    in the controller class.
+  class AuthenticationDecisionMakerMissingError < AuthenticationError
+  end
+
   # Error if user not authenticated
   #
   # @raise [NotAuthenticatedError]
diff --git a/spec/authentication_spec.rb b/spec/authentication_spec.rb
index 9046070..08159c1 100644
--- a/spec/authentication_spec.rb
+++ b/spec/authentication_spec.rb
@@ -6,8 +6,8 @@
   before { dummy_class.define_method(:index) {} }
   before { dummy_class.define_method(:action_name) { "index" } }
 
-  it 'raises `AuthenticationNotPerformedError` if #authenticated? is not defined' do
-    expect{ dummy_class.new.authenticate! }.to raise_error ActiveEntry::AuthenticationNotPerformedError
+  it 'raises `AuthenticationDecisionMakerMissingError` if #authenticated? is not defined' do
+    expect{ dummy_class.new.authenticate! }.to raise_error ActiveEntry::AuthenticationDecisionMakerMissingError
   end
 
   it 'raises `NotAuthenticatedError` if #authenticated? is false' do
diff --git a/spec/authorization_spec.rb b/spec/authorization_spec.rb
index d8a8070..ce3aa98 100644
--- a/spec/authorization_spec.rb
+++ b/spec/authorization_spec.rb
@@ -6,8 +6,8 @@
   before { dummy_class.define_method(:index) {} }
   before { dummy_class.define_method(:action_name) { "index" } }
 
-  it 'raises `AuthorizationNotPerformedError` if #authorized? is not defined' do
-    expect{ dummy_class.new.authorize! }.to raise_error ActiveEntry::AuthorizationNotPerformedError
+  it 'raises `AuthorizationDecisionMakerMissingError` if #authorized? is not defined' do
+    expect{ dummy_class.new.authorize! }.to raise_error ActiveEntry::AuthorizationDecisionMakerMissingError
   end
 
   it 'raises `NotAuthorizedError` if #authorized? is false' do
diff --git a/spec/controllers/authentication_verification_test_controller_spec.rb b/spec/controllers/authentication_verification_test_controller_spec.rb
new file mode 100644
index 0000000..27f486f
--- /dev/null
+++ b/spec/controllers/authentication_verification_test_controller_spec.rb
@@ -0,0 +1,15 @@
+require 'rails_helper'
+
+describe AuthenticationVerificationTestController, type: :controller do
+  describe "#authentication_not_performed" do
+    it "does raise error" do
+      expect{ get :authentication_not_performed }.to raise_error ActiveEntry::AuthenticationNotPerformedError
+    end
+  end
+
+  describe "#authentication_performed" do
+    it "does not raise error" do
+      expect{ get :authentication_performed }.to_not raise_error
+    end
+  end
+end
diff --git a/spec/controllers/authorization_verification_test_controller_spec.rb b/spec/controllers/authorization_verification_test_controller_spec.rb
new file mode 100644
index 0000000..8eb43de
--- /dev/null
+++ b/spec/controllers/authorization_verification_test_controller_spec.rb
@@ -0,0 +1,15 @@
+require 'rails_helper'
+
+describe AuthorizationVerificationTestController, type: :controller do
+  describe "#authorization_not_performed" do
+    it "does raise error" do
+      expect{ get :authorization_not_performed }.to raise_error ActiveEntry::AuthorizationNotPerformedError
+    end
+  end
+
+  describe "#authorization_performed" do
+    it "does not raise error" do
+      expect{ get :authorization_performed }.to_not raise_error
+    end
+  end
+end
diff --git a/spec/dummy/app/controllers/authentication_verification_test_controller.rb b/spec/dummy/app/controllers/authentication_verification_test_controller.rb
new file mode 100644
index 0000000..40e9463
--- /dev/null
+++ b/spec/dummy/app/controllers/authentication_verification_test_controller.rb
@@ -0,0 +1,15 @@
+class AuthenticationVerificationTestController < ApplicationController
+  after_action :verify_authentication!
+  before_action :authenticate!, only: :authentication_performed
+
+  def authentication_not_performed
+    head :no_content
+  end
+
+  def authentication_performed_authenticated?
+    true
+  end
+  def authentication_performed
+    head :no_content
+  end
+end
diff --git a/spec/dummy/app/controllers/authorization_verification_test_controller.rb b/spec/dummy/app/controllers/authorization_verification_test_controller.rb
new file mode 100644
index 0000000..470eb68
--- /dev/null
+++ b/spec/dummy/app/controllers/authorization_verification_test_controller.rb
@@ -0,0 +1,15 @@
+class AuthorizationVerificationTestController < ApplicationController
+  after_action :verify_authorization!
+  before_action :authorize!, only: :authorization_performed
+
+  def authorization_not_performed
+    head :no_content
+  end
+
+  def authorization_performed_authorized?
+    true
+  end
+  def authorization_performed
+    head :no_content
+  end
+end
diff --git a/spec/dummy/config/routes.rb b/spec/dummy/config/routes.rb
index 0a683e7..c5fde2a 100644
--- a/spec/dummy/config/routes.rb
+++ b/spec/dummy/config/routes.rb
@@ -7,4 +7,10 @@
   get "scoped_other" => "scoped_decision_maker_test#other"
   get "scoped_non_authenticated" => "scoped_decision_maker_test#non_authenticated"
   get "scoped_non_authorized" => "scoped_decision_maker_test#non_authorized"
+
+  get "authentication_not_performed" => "authentication_verification_test#authentication_not_performed"
+  get "authentication_performed" => "authentication_verification_test#authentication_performed"
+
+  get "authorization_not_performed" => "authorization_verification_test#authorization_not_performed"
+  get "authorization_performed" => "authorization_verification_test#authorization_performed"
 end
diff --git a/spec/verify_authentication_spec.rb b/spec/verify_authentication_spec.rb
new file mode 100644
index 0000000..90601a4
--- /dev/null
+++ b/spec/verify_authentication_spec.rb
@@ -0,0 +1,34 @@
+require "rails_helper"
+
+describe "Authentication verification" do
+  let(:dummy_class) { Class.new { include ActiveEntry } }
+  let(:dummy_obj) { dummy_class.new }
+
+  before { dummy_class.define_method(:action_name) { "index" } }
+
+  it 'raises `AuthenticationNotPerformedError` if #authenticate! is not called' do
+    expect{ dummy_obj.verify_authentication! }.to raise_error ActiveEntry::AuthenticationNotPerformedError
+  end
+
+  it 'does not raise error if @_authentication_done is true' do
+    dummy_obj.instance_variable_set :@_authentication_done, true
+    expect{ dummy_obj.verify_authentication! }.to_not raise_error
+  end
+
+  it 'does not raise error if #authenticate! is called' do
+    dummy_class.define_method(:authenticated?) { true }
+
+    expect do
+      dummy_obj.authenticate!
+      dummy_obj.verify_authentication!
+    end.to_not raise_error
+  end
+  
+  describe '#authenticate!' do
+    it "sets @_authentication_done" do
+      dummy_class.define_method(:authenticated?) { true }
+      dummy_obj.authenticate!
+      expect(dummy_obj.instance_variable_get(:@_authentication_done)).to be true
+    end
+  end
+end
\ No newline at end of file
diff --git a/spec/verify_authorization_spec.rb b/spec/verify_authorization_spec.rb
new file mode 100644
index 0000000..8bbb930
--- /dev/null
+++ b/spec/verify_authorization_spec.rb
@@ -0,0 +1,34 @@
+require "rails_helper"
+
+describe "Authorization verification" do
+  let(:dummy_class) { Class.new { include ActiveEntry } }
+  let(:dummy_obj) { dummy_class.new }
+
+  before { dummy_class.define_method(:action_name) { "index" } }
+
+  it 'raises `AuthorizationNotPerformedError` if #authorize! is not called' do
+    expect{ dummy_obj.verify_authorization! }.to raise_error ActiveEntry::AuthorizationNotPerformedError
+  end
+
+  it 'does not raise error if @_authorization_done is true' do
+    dummy_obj.instance_variable_set :@_authorization_done, true
+    expect{ dummy_obj.verify_authorization! }.to_not raise_error
+  end
+
+  it 'does not raise error if #authenticate! is called' do
+    dummy_class.define_method(:authorized?) { true }
+
+    expect do
+      dummy_obj.authorize!
+      dummy_obj.verify_authorization!
+    end.to_not raise_error
+  end
+  
+  describe '#authorize!' do
+    it "sets @_authorization_done" do
+      dummy_class.define_method(:authorized?) { true }
+      dummy_obj.authorize!
+      expect(dummy_obj.instance_variable_get(:@_authorization_done)).to be true
+    end
+  end
+end
\ No newline at end of file