Merge pull request #70 from enjaku4/controller-access-fix

CHANGELOG.md

@@ -1,3 +1,13 @@
+## v4.1.1
+
+### Bugs:
+
+- Fixed an issue where controller-wide `grant_access` calls would overwrite each other instead of being additive, causing inconsistent access control based on statement order
+
+### Misc:
+
+- Minor performance improvement for authorization checks
+
 ## v4.1.0
 
 ### Features:

Gemfile

@@ -7,6 +7,7 @@ gemspec
 rails_version = ENV.fetch("RAILS_VERSION", "~> 7")
 
 gem "byebug"
+gem "concurrent-ruby", "1.3.4"
 gem "database_cleaner-active_record"
 gem "grepfruit"
 gem "rails", rails_version

README.md

@@ -323,6 +323,21 @@ end
 ```
 This means that `Crm::InvoicesController` is still accessible to `admin` but is also accessible to `accountant`.
 
+This applies as well to multiple rules defined for the same controller or action:
+```rb
+class Crm::OrdersController < ApplicationController
+  grant_access roles: :manager, context: Order
+  grant_access roles: :admin
+
+  grant_access action: :show, roles: :client, context: -> { Order.find(params[:id]) }
+  grant_access action: :show, roles: :accountant
+  def show
+    # ...
+  end
+end
+```
+This will add rules for `manager` and `admin` roles for all actions in `Crm::OrdersController`, and for `client` and `accountant` roles for the `show` action.
+
 ## Dynamic Authorization Rules
 
 For more complex cases, Rabarber provides dynamic rules:

lib/rabarber/core/access.rb

@@ -8,15 +8,13 @@ def access_granted?(roleable, action, controller_instance)
       end
 
       def controller_accessible?(roleable, controller_instance)
-        controller_rules.any? do |rule_controller, rule|
-          controller_instance.is_a?(rule_controller) && rule.verify_access(roleable, controller_instance)
+        controller_rules.any? do |controller, rules|
+          controller_instance.is_a?(controller) && rules.any? { _1.verify_access(roleable, controller_instance) }
         end
       end
 
       def action_accessible?(roleable, action, controller_instance)
-        action_rules[controller_instance.class].any? do |rule|
-          rule.action == action && rule.verify_access(roleable, controller_instance)
-        end
+        action_rules[controller_instance.class][action].any? { _1.verify_access(roleable, controller_instance) }
       end
     end
   end

lib/rabarber/core/permissions.rb

@@ -13,13 +13,16 @@ class Permissions
       attr_reader :storage
 
       def initialize
-        @storage = { controller_rules: Hash.new({}), action_rules: Hash.new([]) }
+        @storage = {
+          controller_rules: Hash.new { |h, k| h[k] = [] },
+          action_rules: Hash.new { |h1, k1| h1[k1] = Hash.new { |h2, k2| h2[k2] = [] } }
+        }
       end
 
       class << self
         def add(controller, action, roles, context, dynamic_rule, negated_dynamic_rule)
-          rule = Rabarber::Core::Rule.new(action, roles, context, dynamic_rule, negated_dynamic_rule)
-          action ? action_rules[controller] += [rule] : controller_rules[controller] = rule
+          rule = Rabarber::Core::Rule.new(roles, context, dynamic_rule, negated_dynamic_rule)
+          action ? action_rules[controller][action] += [rule] : controller_rules[controller] += [rule]
         end
 
         def controller_rules

lib/rabarber/core/permissions_integrity_checker.rb

@@ -21,8 +21,8 @@ def run!
       private
 
       def missing_list
-        @missing_list ||= action_rules.each_with_object([]) do |(controller, rules), arr|
-          missing_actions = rules.map(&:action) - controller.action_methods.map(&:to_sym)
+        @missing_list ||= action_rules.each_with_object([]) do |(controller, hash), arr|
+          missing_actions = hash.keys - controller.action_methods.map(&:to_sym)
           arr << { controller => missing_actions } if missing_actions.any?
         end
       end

lib/rabarber/core/rule.rb

@@ -3,10 +3,9 @@
 module Rabarber
   module Core
     class Rule
-      attr_reader :action, :roles, :context, :dynamic_rule, :negated_dynamic_rule
+      attr_reader :roles, :context, :dynamic_rule, :negated_dynamic_rule
 
-      def initialize(action, roles, context, dynamic_rule, negated_dynamic_rule)
-        @action = action
+      def initialize(roles, context, dynamic_rule, negated_dynamic_rule)
         @roles = Array(roles)
         @context = context
         @dynamic_rule = dynamic_rule || -> { true }

lib/rabarber/version.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module Rabarber
-  VERSION = "4.1.0"
+  VERSION = "4.1.1"
 end

spec/rabarber/controllers/concerns/authorization/all_access_controller_spec.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+require_relative "shared_examples"
+
+RSpec.describe AllAccessController, type: :controller do
+  let(:user) { User.create! }
+
+  before { allow(controller).to receive(:current_user).and_return(user) }
+
+  describe "when everyone is allowed" do
+    context "when the user has a role" do
+      before { user.assign_roles(:maintainer) }
+
+      it_behaves_like "it allows access", get: :quux
+    end
+
+    context "when the user has no roles" do
+      it_behaves_like "it allows access", get: :quux
+    end
+
+    it_behaves_like "it does not allow access when user must have roles", get: :quux
+    it_behaves_like "it checks permissions integrity", get: :quux
+  end
+end

spec/rabarber/controllers/concerns/authorization/mulitpler_rules_controller_spec.rb

@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+require_relative "shared_examples"
+
+RSpec.describe MultipleRulesController, type: :controller do
+  let(:user) { User.create! }
+
+  before { allow(controller).to receive(:current_user).and_return(user) }
+
+  describe "when multitple controller rules are applied" do
+    context "when one of the user's roles allows access" do
+      before { user.assign_roles(:maintainer) }
+
+      it_behaves_like "it allows access", delete: :qux
+    end
+
+    context "when another one of the user's roles allows access" do
+      before { user.assign_roles(:user, context: Project) }
+
+      it_behaves_like "it allows access", delete: :qux
+    end
+
+    context "when the user's role does not allow access" do
+      before { user.assign_roles(:admin) }
+
+      it_behaves_like "it does not allow access", delete: :qux
+    end
+
+    context "when the user does not have any roles" do
+      it_behaves_like "it does not allow access", delete: :qux
+    end
+
+    it_behaves_like "it does not allow access when user must have roles", delete: :qux
+    it_behaves_like "it checks permissions integrity", delete: :qux
+  end
+end

spec/rabarber/core/access_spec.rb

@@ -52,7 +52,7 @@
 
       context "if role has access to the controller" do
         before do
-          allow(permissions.controller_rules[controller]).to receive(:verify_access)
+          allow(permissions.controller_rules[controller].first).to receive(:verify_access)
             .with(user, controller_instance).and_return(true)
         end
 
@@ -61,7 +61,7 @@
 
       context "if role doesn't have access to the controller" do
         before do
-          allow(permissions.controller_rules[controller]).to receive(:verify_access)
+          allow(permissions.controller_rules[controller].first).to receive(:verify_access)
             .with(user, controller_instance).and_return(false)
         end
 
@@ -74,7 +74,7 @@
 
       context "if role has access to the controller's parent" do
         before do
-          allow(permissions.controller_rules[DummyParentController]).to receive(:verify_access)
+          allow(permissions.controller_rules[DummyParentController].first).to receive(:verify_access)
             .with(user, controller_instance).and_return(true)
         end
 
@@ -83,7 +83,7 @@
 
       context "if role doesn't have access to the controller's parent" do
         before do
-          allow(permissions.controller_rules[DummyParentController]).to receive(:verify_access)
+          allow(permissions.controller_rules[DummyParentController].first).to receive(:verify_access)
             .with(user, controller_instance).and_return(false)
         end
 
@@ -114,7 +114,7 @@
 
         context "if role has access to the action" do
           before do
-            allow(permissions.action_rules[controller].first).to receive(:verify_access)
+            allow(permissions.action_rules[controller][:index].first).to receive(:verify_access)
               .with(user, controller_instance).and_return(true)
           end
 

spec/rabarber/core/permissions_spec.rb

@@ -9,23 +9,23 @@
 
     context "when action is given" do
       before do
-        allow(Rabarber::Core::Rule).to receive(:new).with(:index, [:admin], :context, :dynamic_rule, false).and_return(rule)
+        allow(Rabarber::Core::Rule).to receive(:new).with([:admin], :context, :dynamic_rule, false).and_return(rule)
       end
 
       it "adds permissions to the action rules storage" do
         expect { permissions.add(DummyController, :index, [:admin], :context, :dynamic_rule, false) }
           .to change { permissions.instance.storage[:action_rules] }
-          .to({ DummyController => [rule] })
+          .to({ DummyController => { index: [rule] } })
       end
     end
 
     context "when no action is given" do
-      before { allow(Rabarber::Core::Rule).to receive(:new).with(nil, [:admin, :manager], nil, nil, nil).and_return(rule) }
+      before { allow(Rabarber::Core::Rule).to receive(:new).with([:admin, :manager], nil, nil, nil).and_return(rule) }
 
       it "adds permissions to the controller rules storage" do
         expect { permissions.add(DummyController, nil, [:admin, :manager], nil, nil, nil) }
           .to change { permissions.instance.storage[:controller_rules] }
-          .to({ DummyController => rule })
+          .to({ DummyController => [rule] })
       end
     end
   end

spec/rabarber/core/rule_spec.rb

@@ -5,7 +5,7 @@
     subject { rule.verify_access(roleable, DummyController) }
 
     let(:roleable) { double }
-    let(:rule) { described_class.new(:index, :admin, -> { Project }, -> { true }, nil) }
+    let(:rule) { described_class.new(:admin, -> { Project }, -> { true }, nil) }
 
     context "if all conditions are met" do
       before do
@@ -40,7 +40,7 @@
   describe "#roles_permitted?" do
     subject { rule.roles_permitted?(roleable, DummyController.new) }
 
-    let(:rule) { described_class.new(:index, roles, { context_type: "Project", context_id: nil }, nil, nil) }
+    let(:rule) { described_class.new(roles, { context_type: "Project", context_id: nil }, nil, nil) }
 
     context "when roleable is a User" do
       let(:roleable) { User.create! }
@@ -125,7 +125,7 @@
     subject { rule.dynamic_rules_followed?(controller_instance) }
 
     let(:controller_instance) { double }
-    let(:rule) { described_class.new(:index, :manager, nil, dynamic_rule, negated_dynamic_rule) }
+    let(:rule) { described_class.new(:manager, nil, dynamic_rule, negated_dynamic_rule) }
 
     context "both dynamic rules are empty" do
       let(:dynamic_rule) { nil }

spec/support/application.rb

@@ -38,6 +38,14 @@ class DummyApplication < Rails::Application
     end
   end
 
+  resources :multiple_rules, only: [] do
+    delete :qux, on: :collection
+  end
+
+  resources :all_access, only: [] do
+    get :quux, on: :collection
+  end
+
   resources :no_user, only: [] do
     collection do
       put :access_with_roles

spec/support/controllers.rb

@@ -64,6 +64,19 @@ def baz = head(:ok)
   def bad = head(:ok)
 end
 
+class MultipleRulesController < ApplicationController
+  grant_access roles: [:maintainer]
+  grant_access roles: [:admin, :user], context: Project
+
+  def qux = head(:ok)
+end
+
+class AllAccessController < ApplicationController
+  grant_access
+
+  def quux = head(:ok)
+end
+
 class NoUserController < ApplicationController
   grant_access action: :access_with_roles, roles: :admin
   def access_with_roles = head(:ok)