diff --git a/CHANGELOG.md b/CHANGELOG.md index 73c89cf..09ca1f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,54 @@ # Changelog +## 1.7.0 (UNRELEASED) + +* Introduce new `:before_attr_assign` policy chain which allows to + access the `model` instance before the parameters are assigned. + +* Fix bug with lazy authorization in `RailsOps::Operation::Model::Update` + operation which used a version of the `model` which was missing some + attributes. + +* Deprecate lazy authorization in `RailsOps::Operation::Model::Update` + operations. + +### Migrating from earlier versions + +* Make sure you use the correct policy chains, depending on the state you + need the `model` to be in. If you need the model before the attributes are + assigned to the passed-in params, use the `:before_attr_assign` chain. + In all other chains, the `model` instance has its attributes assigned to the + params you supplied to the operation. + +* If you use `lazy` authorizaion in any of your `Update` operations, you are + advised to remove them and replace the lazy authorization by a custom functionality. + For example, this is the operation before: + + ```ruby + class Operations::User::Update < RailsOps::Operation::Model::Update + model User + + model_authorization_action :update, lazy: true + end + ``` + + and this is the operation afterwards: + + ```ruby + class Operations::User::Update < RailsOps::Operation::Model::Update + model User + + # Disable automatically authorizing against the `:update` action + model_authorization_action nil + + policy :before_perform do + # Using "find_model" to retrieve the model from the database with + # the attributes before assigning the params to the model instance. + authorize_model! :update, find_model + end + end + ``` + ## 1.6.1 (2025-01-24) * Fix lazy authorization in `RailsOps::Operation::Model::Update` operation diff --git a/README.md b/README.md index a976a82..ee31d24 100644 --- a/README.md +++ b/README.md @@ -454,6 +454,14 @@ an appropriate exception. As mentioned above, policies can be executed at various points in your operation's lifecycle. This is possible using *policy chains*: +- `:before_attr_assign` + + Policies in this chain run before assigning the attributes to the model. This chain is only run + in `Model` operations, which at some point call the `assign_attributes` method. This chain is + the only chain in which the model is in the state *before* the passed in params are assigned. + If you need to run any code which needs the state of the model from the database (e.g. to run + custom authentications), this is the correct place. + - `:on_init` Policies in this chain run after the operation class is instantiated. @@ -1392,6 +1400,10 @@ sensible default. See the respective class' source code for details. #### Lazy model update authorization +*Please note that using lazy model update authorization is deprecated any may +be removed in a future release. See the changelog for instructions on how to +adapt your application.* + In case of operations inheriting from `RailsOps::Operation::Model::Update`, you can specify the `model_authorization_action` to be `lazy`, meaning that it will only be checked when *performing* the operation, but not on initialization. This diff --git a/lib/rails_ops/mixins/model/authorization.rb b/lib/rails_ops/mixins/model/authorization.rb index deb5823..c85e7d7 100644 --- a/lib/rails_ops/mixins/model/authorization.rb +++ b/lib/rails_ops/mixins/model/authorization.rb @@ -12,6 +12,8 @@ module RailsOps::Mixins::Model::Authorization module ClassMethods # Gets or sets the action verb used for authorizing models. def model_authorization_action(*action, lazy: false) + RailsOps.deprecator.warn('Using `lazy` model authorization is deprecated and will be removed in a future version.') if lazy + if action.size == 1 self._model_authorization_action = action.first self._model_authorization_lazy = lazy diff --git a/lib/rails_ops/mixins/policies.rb b/lib/rails_ops/mixins/policies.rb index 54f433e..1ac05d3 100644 --- a/lib/rails_ops/mixins/policies.rb +++ b/lib/rails_ops/mixins/policies.rb @@ -6,6 +6,7 @@ module RailsOps::Mixins::Policies extend ActiveSupport::Concern POLICY_CHAIN_KEYS = %i[ + before_attr_assign on_init before_perform after_perform @@ -26,6 +27,12 @@ def policy(chain = :before_perform, prepend_action: false, &block) fail "Unknown policy chain #{chain.inspect}, available are #{POLICY_CHAIN_KEYS.inspect}." end + # The `before_attr_assign` chain is only allowed if the operation is a model + # operation, i.e. it needs to implement the `build_model` method. + if chain == :before_attr_assign && !method_defined?(:assign_attributes) + fail 'Policy :before_attr_assign may not be used unless your operation defines the `assign_attributes` method!' + end + self._policy_chains = _policy_chains.dup if prepend_action _policy_chains[chain] = [block] + _policy_chains[chain] diff --git a/lib/rails_ops/operation/model.rb b/lib/rails_ops/operation/model.rb index dd0ff54..9f99b6a 100644 --- a/lib/rails_ops/operation/model.rb +++ b/lib/rails_ops/operation/model.rb @@ -131,6 +131,8 @@ def model # are meant for nested models (registered via `nested_model_op`) will not # be assigned. You can turn this filtering off by passing `false`. def assign_attributes(attributes = nil, model: nil, without_protection: false, without_nested_models: true) + run_policies :before_attr_assign + model ||= self.model attributes ||= extract_attributes_from_params(model) diff --git a/lib/rails_ops/operation/model/update.rb b/lib/rails_ops/operation/model/update.rb index d75724e..a4dd274 100644 --- a/lib/rails_ops/operation/model/update.rb +++ b/lib/rails_ops/operation/model/update.rb @@ -19,8 +19,17 @@ def self.always_extend_model_class? policy :before_perform do # If the authorization is configured to be lazy, we need to call the authorization - # on the copy of the model that we made before assigning the new attributes. - authorize_model! model_authorization_action, @model_before_assigning_attributes if self.class._model_authorization_lazy + # on a fresh copy of the model, before assigning the attributes. We simply use the `find_model` + # method from our parent class and then run the authorization on this instance. + if self.class._model_authorization_lazy + model_from_database = find_model + + if model_from_database.respond_to?(:parent_op=) + model_from_database.parent_op = self + end + + authorize_model! model_authorization_action, model_from_database + end end def model_authorization @@ -37,13 +46,8 @@ def build_model build_nested_model_ops :update # Perform update authorization BEFORE assigning attributes. If the authorization is lazy, - # we copy the model before assigning the attributes, such that we can call the authorization - # later on. - if self.class._model_authorization_lazy - @model_before_assigning_attributes = @model.deep_dup - else - model_authorization - end + # we'll call the authorization later on in the `before_perform` block. + model_authorization unless self.class._model_authorization_lazy # Assign attributes assign_attributes diff --git a/test/unit/rails_ops/mixins/policies_test.rb b/test/unit/rails_ops/mixins/policies_test.rb index 727b9d6..306ec2b 100644 --- a/test/unit/rails_ops/mixins/policies_test.rb +++ b/test/unit/rails_ops/mixins/policies_test.rb @@ -33,6 +33,58 @@ def perform op.run!.sequence end + def test_basic_policies_with_model + group = Group.create! + + op = Class.new(RailsOps::Operation::Model::Update) do + attr_reader :sequence + + model Group + + policy do + @sequence << :default + end + + policy :before_attr_assign do + @sequence = [] + @sequence << :before_attr_assign + end + + policy :on_init do + @sequence << :on_init + end + + policy :before_perform do + @sequence << :before_perform + end + + policy :after_perform do + @sequence << :after_perform + end + + def perform + @sequence << :perform + end + end + + assert_equal %i[before_attr_assign on_init default before_perform perform after_perform], + op.run!(id: group.id).sequence + end + + def test_before_attr_assign_needs_build_model + # When trying to use the `:before_attr_assign` chain, we need + # to have the `assign_attributes` method implemented, which usually + # is implemented in the `RailsOps::Operation::Model` base class + # and runs the `before_attr_assign` policy chain. + assert_raises RuntimeError, match: /Policy :before_attr_assign may not be used unless your operation defines the `assign_attributes` method!/ do + Class.new(RailsOps::Operation) do + policy :before_attr_assign do + # Nothing needed here + end + end + end + end + def test_prepend_action op = Class.new(RailsOps::Operation) do attr_reader :sequence diff --git a/test/unit/rails_ops/operation/model/create_test.rb b/test/unit/rails_ops/operation/model/create_test.rb index 0cb388c..6063295 100644 --- a/test/unit/rails_ops/operation/model/create_test.rb +++ b/test/unit/rails_ops/operation/model/create_test.rb @@ -65,4 +65,51 @@ def test_build op.build_model end end + + def test_policies + op_klass = Class.new(RailsOps::Operation::Model::Create) do + model ::Group + + policy do + # Here, we need the model to have the new name assigned + fail 'Attribute should be assigned to new value' unless model.name == 'new_name' + + # However, the model should not be persisted yet + fail 'Model should not be persisted to the database yet' if model.persisted? + end + + policy :before_attr_assign do + # The name of the model itself should still be nil + fail 'Attribute should not be assigned to a value yet' if model.name.present? + end + + policy :on_init do + # Here, we need the model to have the new name assigned + fail 'Attribute should be assigned to new value' unless model.name == 'new_name' + + # However, the model should not be persisted yet + fail 'Model should not be persisted to the database yet' if model.persisted? + end + + policy :before_perform do + # Here, we need the model to have the new name assigned + fail 'Attribute should be assigned to new value' unless model.name == 'new_name' + + # However, the model should not be persisted yet + fail 'Model should not be persisted to the database yet' if model.persisted? + end + + policy :after_perform do + # Here, we need the model to have the new name assigned + fail 'Attribute should be assigned to new value' unless model.name == 'new_name' + + # Now, the model should be persisted to the database + fail 'Model should not be persisted to the database yet' unless model.persisted? + end + end + + assert_nothing_raised do + op_klass.run!(group: { name: 'new_name' }) + end + end end diff --git a/test/unit/rails_ops/operation/model/update_test.rb b/test/unit/rails_ops/operation/model/update_test.rb index f2d5292..01d198a 100644 --- a/test/unit/rails_ops/operation/model/update_test.rb +++ b/test/unit/rails_ops/operation/model/update_test.rb @@ -163,4 +163,52 @@ def initialize ensure RailsOps.config.authorization_backend = nil end + + def test_policies + op_klass = Class.new(RailsOps::Operation::Model::Update) do + model ::Group + + policy do + # Here, we need the model to have the new name assigned + fail 'Attribute should be assigned to new value' unless model.name == 'new_name' + + # However, the new name should not be persisted to the database yet + fail 'Attribute change should not be persisted yet' unless Group.find(model.id).name == 'foobar' + end + + policy :before_attr_assign do + # The name of the model itself should still be the initial value + fail 'Attribute should not be assigned to new value yet' unless model.name == 'foobar' + end + + policy :on_init do + # Here, we need the model to have the new name assigned + fail 'Attribute should be assigned to new value' unless model.name == 'new_name' + + # However, the new name should not be persisted to the database yet + fail 'Attribute change should not be persisted yet' unless Group.find(model.id).name == 'foobar' + end + + policy :before_perform do + # Here, we need the model to have the new name assigned + fail 'Attribute should be assigned to new value' unless model.name == 'new_name' + + # However, the new name should not be persisted to the database yet + fail 'Attribute change should not be persisted yet' unless Group.find(model.id).name == 'foobar' + end + + policy :after_perform do + # Here, we need the model to have the new name assigned + fail 'Attribute should be assigned to new value' unless model.name == 'new_name' + + # Also, the new name should be persisted to the database + fail 'Attribute change should not be persisted yet' unless Group.find(model.id).name == 'new_name' + end + end + + model = Group.create!(name: 'foobar') + assert_nothing_raised do + op_klass.run!(id: model.id, group: { name: 'new_name' }) + end + end end