diff --git a/.github/workflows/ruby.yml b/.github/workflows/ruby.yml index e913b5c..401718e 100644 --- a/.github/workflows/ruby.yml +++ b/.github/workflows/ruby.yml @@ -19,3 +19,4 @@ jobs: with: token: ${{ secrets.CODECOV_TOKEN }} files: ./coverage/lcov.info + diff --git a/.rubocop.yml b/.rubocop.yml index 5917619..8685ba6 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -3,7 +3,13 @@ require: rubocop-rspec RSpec/NestedGroups: Enabled: false -Metrics/LineLength: +# Policy specs build a small world before they can assert anything - a record, a +# relation, a nested policy, a viewer - and naming each piece reads better than +# folding them into one setup block. +RSpec/MultipleMemoizedHelpers: + Enabled: false + +Layout/LineLength: Enabled: true Max: 120 @@ -22,7 +28,7 @@ Lint/AmbiguousBlockAssociation: Exclude: - spec/**/*.rb -Naming/UncommunicativeMethodParamName: +Naming/MethodParameterName: AllowedNames: - 'to' - 'at' diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fa3d9c..39015e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,11 +5,22 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [Unreleased] +## [2.0.0] * Updated: Ruby to 3.2+ * Added: short-hand method for getting allowed actions * Added: support for proc type validator options +* Added: nested read protection. `protected_resource` now returns a protected resource for + values guarded by a policy of their own, so nested reads run nested rules. Attributes + returning a record declare either `.nested { SomePolicy.new(_1) }` or + `.unprotected(because: '...')`. Configured via `ResourcePolicy.configure` with + `config.protectable_class` (asked about a class, never a value, so a list attribute is + decided without loading a row), `config.nested_protection` (`:soft` reports and changes + nothing for callers, `:hard` enforces) and `config.reporter`. Off until + `config.protectable_class` is set, so existing consumers keep their current behaviour. On + `:soft` nothing is ever withheld: an undeclared value, a nested object the viewer may not + read and an attribute of one the viewer may not read are all reported and handed over, so + adding a declaration cannot start hiding data until `:hard` is switched on. * Added/Changed/Deprecated/Removed/Fixed/Security: YOUR CHANGE HERE ## [1.1.0] diff --git a/Gemfile.lock b/Gemfile.lock index aadfb39..86f46d3 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - resource_policy (1.1.0) + resource_policy (2.0.0) GEM remote: https://rubygems.org/ diff --git a/docs/README.md b/docs/README.md index ee3ff07..7cff5f5 100644 --- a/docs/README.md +++ b/docs/README.md @@ -145,6 +145,36 @@ protected_user.id #=> 1337 protected_user.email # nil ``` +#### Protecting nested values + +A protected resource never hands out a raw record. If an attribute returns something guarded by +its own policy, declare which one, and nested reads run nested rules automatically: + +```ruby +class UserPolicy + include ResourcePolicy::Policy + + policy do |c| + c.attribute(:current_contract) + .allowed(:read) + .nested { |contract| ContractPolicy.new(contract, current_user) } + end + + ... +end +``` + +```ruby +protected_user = UserPolicy.new(user, current_user).protected_resource + +protected_user.current_contract # => protected resource, not a raw Contract +protected_user.current_contract.salary # => nil unless ContractPolicy allows reading it +``` + +This is opt-in and needs a small amount of setup, including what the gem should do when an +attribute returns a record nobody declared a policy for. See +[Configuration](components/configuration) for the full details. + ## Development After checking out the repo, run `bin/setup` to install dependencies. Then, run `rake spec` to run the tests. You can also run `bin/console` for an interactive prompt that will allow you to experiment. diff --git a/docs/_sidebar.md b/docs/_sidebar.md index 4f83cb4..df52acc 100644 --- a/docs/_sidebar.md +++ b/docs/_sidebar.md @@ -5,3 +5,4 @@ * [ActionValidator](components/action_validator) * [AttributesPolicy](components/attributes_policy) * [AttributesValidator](components/attributes_validator) + * [Configuration](components/configuration) diff --git a/docs/components/configuration.md b/docs/components/configuration.md new file mode 100644 index 0000000..21dc7c8 --- /dev/null +++ b/docs/components/configuration.md @@ -0,0 +1,253 @@ +# ResourcePolicy::Configuration + +A protected resource never hands out a raw record. When an attribute's value is itself +guarded by a policy, `protected_resource` returns that policy's own protected resource, so +nested reads run nested rules without every call site having to remember to ask. + +For that to work the gem needs to know two things it cannot work out on its own: which values +must carry a policy, and how hard to lean on one that has none. Both are configured here. + +Nested protection is **off until you configure it**. A gem consumer which sets nothing keeps +exactly the behaviour it has today. + +### Setting it up + +In a Rails app, add an initializer: + +```ruby +# config/initializers/resource_policy.rb +ResourcePolicy.configure do |config| + config.protectable_class = ->(klass) { klass <= ActiveRecord::Base } + config.nested_protection = Rails.env.local? ? :hard : :soft + config.reporter = ->(event) { Rails.logger.warn(event.message) } +end +``` + +### config#protectable_class + +A callable which answers **"does this kind of value have to carry a policy of its own?"** + +It is called with the **class** of every value a protected resource is about to return, never +with the value: + +```ruby +protected_user.first_name # String -> not a record -> returned as is +protected_user.current_contract # Contract -> a record -> must be declared +``` + +Asking about a class rather than a value is what keeps list attributes free. An +`ActiveRecord::Relation` answers `klass` without loading anything, so `protected_user.contracts` +is decided with no query; asking about a value would mean fetching a row behind every list read +on every screen, including the reads which turn out to need no protection at all. + +Classes which pass the check need either a `.nested` or an `.unprotected` declaration on the +attribute. Everything else — strings, numbers, dates, enums, plain value objects — is handed +back untouched. + +The default is `->(_klass) { false }`, which means nothing is a record and the guard never +fires. Setting it is what switches nested protection on. + +`ActiveRecord::Base` is the usual answer, but it is a dial rather than a constant. Widen it if +you have plain Ruby objects, or view-layer objects, holding sensitive fields: + +```ruby +config.protectable_class = ->(klass) { klass <= ActiveRecord::Base || klass <= GraphqlRails::Decorator } +``` + +The trade-off is that the decision cannot depend on the value — "only protect persisted +records" is not expressible. That is deliberate: a per-value rule cannot be answered for a +relation without loading it. + +### config#nested_protection + +How hard the gem leans on a nested value it cannot vouch for. + +**`:soft` changes nothing at all.** It is an observation mode: a value is handed back exactly +as the policy target returned it — the same record, the same relation, still lazy, with every +item in it. The only thing it does is report the values nobody has declared yet. +**`:hard` enforces**, and is the only mode in which a `.nested` declaration does anything. + +| situation | `:soft` | `:hard` | +|---|---|---| +| protectable value with no declaration | warns, returns the value | raises `UnprotectedNestedValueError` | +| value with a `.nested` declaration | returns the value untouched | returns the nested policy's protected resource | +| collection with a `.nested` declaration | returns the collection untouched | returns an `Array` of protected resources | +| nested policy denies the object | — the declaration is inert | returns `nil` | +| nested policy denies one of its attributes | — the declaration is inert | returns `nil` | + +That split is the point: declarations can be written, reviewed and deployed while the +application behaves exactly as it did, and the flip to `:hard` is the single moment anything +changes. It does mean `:soft` cannot tell you *which* nested values `:hard` would hide — +nothing is wrapped, so nothing is there to notice it. Run your test suite or a staging +environment in `:hard` to get that list: a missing declaration raises, and a denied read shows +up as a failing expectation. + +`:hard` is the default on purpose. + +A developer who adds an attribute returning a record and forgets to declare it finds out on +their first test run, with a message naming the fix: + +``` +UserPolicy attribute :absences returned a Absence which has no policy of its own, so its +read rules never run. Declare it with `c.attribute(:absences).nested { SomePolicy.new(_1) }`, +or, if the value needs no policy, with `c.attribute(:absences).unprotected(because: '...')`. +``` + +### config#reporter + +Called on `:soft` with an `UnprotectedNestedValue`: a value whose class is protectable and +whose attribute carries no declaration. It exists so you can attach context the gem knows +nothing about: + +```ruby +config.reporter = lambda do |event| + Rails.logger.warn( + message: event.message, + policy: event.policy.class.name, + attribute: event.attribute.name, + user_id: Current.user&.id + ) +end +``` + +The event exposes `#policy`, `#attribute`, `#message` and `#value_class` — the class, not the +record, so log events never carry the data they warn about. The default writes the message to +`stderr`. + +### Declaring nested attributes + +Once configured, each attribute returning a record needs one of two declarations. + +`nested` says which policy guards the value. The block runs on the parent policy instance, so +that policy's own dependencies are in scope — which matters when nested policies do not all +take the same arguments: + +```ruby +class UserPolicy + include ResourcePolicy::Policy + + policy do |c| + c.attribute(:current_contract) + .allowed(:read) + .nested { |contract| ContractPolicy.new(contract, app_context: app_context) } + + c.attribute(:contracts) + .allowed(:read, if: :read_contracts_allowed?) + .nested { |contract| ContractPolicy.new(contract, app_context: app_context) } + end +end +``` + +`unprotected` says the value needs no policy. The reason is required to be written down, so the +decision is visible in review rather than being a silent default: + +```ruby +c.attribute(:custom_fields) + .allowed(:read) + .unprotected(because: 'plain value objects with no sensitive fields') +``` + +### When `nested` is the wrong tool + +A protected resource answers the attributes its policy declares and nothing else. That is the +point of it, but it means a nested value is a narrow object, not a stand-in for the record: + +```ruby +protected_user.current_contract.hours_week # => the value, it is declared +protected_user.current_contract.shop # => NoMethodError, no rule declares it +``` + +So `nested` fits a value which is consumed *through the policy* — a controller rendering off +`protected_resource`, a serialiser built on one. It does not fit a value handed to something +which treats it as the record: a decorator, a presenter, anything reaching for an association +or a helper. Those break the moment `:hard` is switched on, with a `NoMethodError` rather than +a hidden field. + +Where a decorator is what exposes the value, the decorator is what has to apply the policy: + +```ruby +# not this - the decorator receives a proxy and falls over +c.attribute(:current_contract).allowed(:read).nested { ContractPolicy.new(_1) } + +# this - the value leaves as a decorator which applies ContractPolicy itself +def contract + ContractDecorator.decorate(protected_user.current_contract, app_context:) +end + +c.attribute(:current_contract) + .allowed(:read) + .unprotected(because: 'exposed only through ContractDecorator, which applies ContractPolicy') +``` + +The `unprotected` declaration is doing real work there: it records *why* this value needs no +policy of its own, so the next reader can check the claim rather than assume it. + +### Reading nested values + +Nothing changes at the call site. Reads simply run the nested rules too: + +```ruby +policy = UserPolicy.new(user, app_context: app_context) +protected_user = policy.protected_resource + +protected_user.current_contract # => ProtectedResource wrapping ContractPolicy +protected_user.current_contract.hours_week # => nil unless ContractPolicy allows reading it +protected_user.contracts.map(&:salary_nr) # => each item protected by ContractPolicy +``` + +On `:hard`, a nested value the viewer may not read **at all** is withheld outright rather than +handed over as a proxy answering `nil` to everything. On `:soft` it is handed over and a warning +is logged instead: + +```ruby +protected_user.current_contract # => nil on :hard when ContractPolicy denies the read + # => ProtectedResource on :soft, plus a logged warning + +protected_user.current_contract.salary_nr # => nil on :hard when ContractPolicy denies salary_nr + # => the value on :soft, plus a logged warning +``` + +"May not read at all" means the nested policy's `:read` action is denied, or — when it declares +no `:read` action — that none of its attributes are readable. + +### Collections + +A collection is one decision, not one per item, because the alternative is expensive: + +```ruby +protected_user.contracts.includes(:employer) # still a relation when nothing needs wrapping +``` + +- **undeclared, or `unprotected`** — the collection is handed back as it came, so an + `ActiveRecord::Relation` stays a relation and callers can keep chaining `.where`, `.includes` + and `.order`. +- **`nested`, on `:soft`** — handed back as it came, exactly as above: the declaration is inert. +- **`nested`, on `:hard`** — every item is wrapped, which forces the query and returns an + `Array`. Items the nested policy withholds are dropped; `nil` entries survive, because only + the items the viewer may not read are meant to disappear. +- **empty relation** — still decided, because a relation knows its item class whether or not it + holds rows. A missing declaration is therefore caught on the same code path every time, + rather than only when the data happens to be there. +- **empty `Array`** — the one case which cannot be decided: an array which holds nothing says + nothing about what it would have held, so it is handed back untouched and nothing is reported. + +**Deciding a collection costs no query at all.** Nothing reads an element: whether an attribute +is declared is a property of the attribute, and the item class of an unloaded relation is a +property of the relation. Only wrapping touches the rows, and that happens on `:hard` alone. + +### Rolling it out + +Switching `protectable_class` on in an existing app will surface every record-returning +attribute at once, so introduce it in stages: + +1. Set `nested_protection` to `:soft` everywhere. Nothing a caller receives changes, so this is + safe to deploy on its own, and the warnings are the inventory of attributes needing + declarations. +2. Declare them, policy by policy, with `nested` or `unprotected`. These are safe to deploy + too: on `:soft` a declaration does nothing. +3. Move test and development to `:hard`. This is where declarations start acting, so it is + where you find out what they change — a run of the suite lists it. +4. Fix what that surfaces, then flip production to `:hard`. + +Step 3 carries the risk, and it is deliberately the step you take on a machine rather than in +production. Everything before it is inert by construction. diff --git a/lib/resource_policy.rb b/lib/resource_policy.rb index f46c0ee..571cfdc 100644 --- a/lib/resource_policy.rb +++ b/lib/resource_policy.rb @@ -6,6 +6,7 @@ module ResourcePolicy class Error < StandardError; end + require 'resource_policy/configuration' require 'resource_policy/policy' # Your code goes here... end diff --git a/lib/resource_policy/configuration.rb b/lib/resource_policy/configuration.rb new file mode 100644 index 0000000..6f374fd --- /dev/null +++ b/lib/resource_policy/configuration.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true + +# Root namespace +module ResourcePolicy + # Global configuration. + # + # Usage example: + # + # ResourcePolicy.configure do |config| + # config.protectable_class = ->(klass) { klass <= ActiveRecord::Base } + # config.nested_protection = Rails.env.local? ? :hard : :soft + # config.reporter = ->(event) { Rails.logger.warn(event.message) } + # end + # + class Configuration + class UnprotectedNestedValueError < ResourcePolicy::Error; end + + MODES = %i[soft hard].freeze + + # Describes a value a policy was asked to hand out but cannot vouch for. It carries the + # class rather than the value: it is all the message needs, it is all a relation can give + # without loading a row, and it keeps records out of log events. + UnprotectedNestedValue = Struct.new(:policy, :attribute, :value_class, keyword_init: true) do + def message + "#{policy.class} attribute #{attribute.name.inspect} returned a #{value_class} " \ + 'which has no policy of its own, so its read rules never run. Declare it with ' \ + "`c.attribute(#{attribute.name.inspect}).nested { SomePolicy.new(_1) }`, or, if the " \ + "value needs no policy, with `c.attribute(#{attribute.name.inspect}).unprotected(because: '...')`." + end + end + + # Nothing is protectable until the host app says what a guarded value looks like, so a + # gem consumer that has not opted in keeps its current behaviour exactly. + DEFAULT_PROTECTABLE_CLASS = ->(_klass) { false } + DEFAULT_REPORTER = ->(event) { warn(event.message) } + + # Answers "does this kind of value have to carry a policy of its own?". + # + # Called with the class of every value a protected resource is about to return, never with + # the value itself. A class is what an unloaded relation can answer for free, so a list + # attribute is decided without a query; asking about the value would mean loading a row + # behind every list read. A class which passes needs a `.nested` or `.unprotected` + # declaration, anything else (strings, numbers, dates, value objects) is handed back + # untouched. Rails apps usually want `->(klass) { klass <= ActiveRecord::Base }`. Leaving it + # unset disables nested protection entirely. + attr_accessor :protectable_class + + # Called with an UnprotectedNestedValue whenever the `:soft` mode is in use. + # Exists so the host app can add its own context (current user, client, request id) to the + # log line, which this gem has no way of knowing. + attr_accessor :reporter + + # How hard the gem leans on a nested value it cannot vouch for: + # + # :soft - never changes what a caller gets. An undeclared value is handed over, a value + # whose policy denies the read is handed over, and both are reported. This is the + # mode an app runs in while the gaps are collected from its logs. + # :hard - enforces. An undeclared value raises, because nobody can vouch for it; a value + # whose policy denies the read is withheld. + attr_reader :nested_protection + + def initialize + @protectable_class = DEFAULT_PROTECTABLE_CLASS + @reporter = DEFAULT_REPORTER + @nested_protection = :hard + end + + def nested_protection=(mode) + mode = mode.to_sym + + MODES.include?(mode) || raise(ArgumentError, "unknown mode #{mode.inspect}, expected one of #{MODES.inspect}") + + @nested_protection = mode + end + + def hard? + nested_protection == :hard + end + + def protectable_class?(klass) + protectable_class.call(klass) + end + + # Raises (`:hard`) or reports (`:soft`). Deliberately returns nothing useful: the caller + # decides what to hand back in soft mode, because a collection and a single value are + # handed back differently. + def report_unprotected_nested_value(policy:, attribute:, value_class:) + event = UnprotectedNestedValue.new(policy: policy, attribute: attribute, value_class: value_class) + + raise UnprotectedNestedValueError, event.message if hard? + + reporter.call(event) + nil + end + end + + def self.config + @config ||= Configuration.new + end + + def self.configure + yield(config) + config + end +end diff --git a/lib/resource_policy/policy/attributes_policy/attribute_configuration.rb b/lib/resource_policy/policy/attributes_policy/attribute_configuration.rb index 90ad689..430e7fa 100644 --- a/lib/resource_policy/policy/attributes_policy/attribute_configuration.rb +++ b/lib/resource_policy/policy/attributes_policy/attribute_configuration.rb @@ -10,14 +10,40 @@ class AttributeConfiguration DEFAULT_OPTIONS = { if: [] }.freeze ALLOWED_ACTIONS = %i[read write].freeze - attr_reader :name + attr_reader :name, :nested_policy_builder, :unprotected_reason def initialize(name, policy_configuration:) + @unprotected = false @name = name @allowed_actions = {} @policy_configuration = policy_configuration end + # Declares which policy guards this attribute's value. The block runs on the parent + # policy instance, so its own dependencies (app_context, current_user, ...) are in scope. + def nested(&builder) + @nested_policy_builder = builder + self + end + + # Declares that the value needs no policy of its own. Explicit so that it is greppable + # and shows up in review, rather than being the silent default it is today. + def unprotected(because: nil) + @unprotected = true + @unprotected_reason = because + self + end + + def unprotected? + @unprotected + end + + # Whether a nested policy is declared at all. Answerable without a value, which is what + # lets a collection be decided before anything is read from it. + def nested? + !nested_policy_builder.nil? + end + def initialize_copy(other) super @allowed_actions = @allowed_actions.dup.transform_values(&:dup) @@ -52,9 +78,19 @@ def merge(other) other.defined_actions.each do |action| new_attribute.allowed(action, if: other.conditions_for(action)) end + new_attribute.copy_protection_from(other) end end + # `merge` and MergePolicies rebuild attributes from their actions only, so without this + # a `.nested` declaration silently vanishes inside a `c.group` block or an inherited + # policy - which is where most of them live. + def copy_protection_from(other) + nested(&other.nested_policy_builder) if other.nested_policy_builder + unprotected(because: other.unprotected_reason) if other.unprotected? + self + end + private attr_reader :allowed_actions, :policy_configuration diff --git a/lib/resource_policy/policy/attributes_policy/attribute_policy.rb b/lib/resource_policy/policy/attributes_policy/attribute_policy.rb index a7be8b3..ae8c55e 100644 --- a/lib/resource_policy/policy/attributes_policy/attribute_policy.rb +++ b/lib/resource_policy/policy/attributes_policy/attribute_policy.rb @@ -24,6 +24,22 @@ def writable? allowed_to?(:write) end + # Builds the policy guarding this attribute's value, or nil when none is declared. + def nested_policy_for(value) + builder = attribute_config.nested_policy_builder + return nil unless builder + + policy.instance_exec(value, &builder) + end + + def unprotected? + attribute_config.unprotected? + end + + def nested? + attribute_config.nested? + end + def allowed_to?(access_level) @allowed_to ||= {} level_name = access_level.to_sym diff --git a/lib/resource_policy/policy/merge_policies.rb b/lib/resource_policy/policy/merge_policies.rb index b5b278f..46f0298 100644 --- a/lib/resource_policy/policy/merge_policies.rb +++ b/lib/resource_policy/policy/merge_policies.rb @@ -37,6 +37,7 @@ def merge_attributes conditions = attribute.conditions_for(action_name) policy.attribute(attribute.name).allowed(action_name, if: conditions) end + policy.attribute(attribute.name).copy_protection_from(attribute) end end end diff --git a/lib/resource_policy/protected_resource.rb b/lib/resource_policy/protected_resource.rb index c9fbe78..89285ca 100644 --- a/lib/resource_policy/protected_resource.rb +++ b/lib/resource_policy/protected_resource.rb @@ -2,7 +2,18 @@ module ResourcePolicy # Generates resource which has same attributes as policy target, - # but returns `nil` when attribute in not readable according to policy + # but returns `nil` when attribute in not readable according to policy. + # + # On `:hard` a protected resource never hands out a raw record: a value which is itself + # guarded by a policy comes back as that policy's own protected resource, so nested reads run + # nested rules without every call site having to remember to ask. + # + # On `:soft` none of that happens. A `.nested` declaration is inert: the value is handed back + # exactly as the target returned it - the same record, the same relation, still lazy, with + # every item in it. Soft exists to be switched on in a running application without changing + # anything a caller receives, so declarations can be written and reviewed long before the + # behaviour they describe is turned on. What `:hard` would do instead is found by running in + # `:hard`, not by reading soft's output. class ProtectedResource def initialize(policy) @policy = policy @@ -10,9 +21,11 @@ def initialize(policy) def method_missing(method_name, *args) return super unless target_respond_to?(method_name, *args) - return nil unless policy.attribute(method_name).readable? - policy_target.public_send(method_name, *args) + attribute = policy.attribute(method_name) + return nil unless attribute.readable? + + protect(attribute, policy_target.public_send(method_name, *args)) end def respond_to_missing?(*args) @@ -23,6 +36,99 @@ def respond_to_missing?(*args) attr_reader :policy + def protect(attribute, value) + return value if value.nil? + return value if attribute.unprotected? + return protect_collection(attribute, value) if collection?(value) + return protect_declared(attribute, value) if attribute.nested? + + report_undeclared(attribute, value.class) + value + end + + # A declaration says what `:hard` does. On `:soft` it is inert, so the value is handed back + # as it came and nothing is reported: the declaration is already visible in the policy. + def protect_declared(attribute, value) + return value unless ResourcePolicy.config.hard? + + nested_policy = attribute.nested_policy_for(value) + return value unless nested_policy + + protect_nested(attribute, nested_policy) + end + + # Raises in `:hard`. In `:soft` the warning is the whole signal and the data still flows, + # so an app mid-migration keeps working. + def report_undeclared(attribute, value_class) + return unless ResourcePolicy.config.protectable_class?(value_class) + + ResourcePolicy.config.report_unprotected_nested_value( + policy: policy, attribute: attribute, value_class: value_class + ) + end + + # Reached on `:hard` only. A nested object the viewer may not read at all is withheld + # entirely, rather than handed out as a proxy whose every attribute answers nil. + def protect_nested(_attribute, nested_policy) + return nil unless readable_policy?(nested_policy) + + nested_policy.protected_resource + end + + def readable_policy?(nested_policy) + read_action = nested_policy.action(:read) if nested_policy.respond_to?(:action) + return read_action.allowed? if read_action + + nested_policy.attributes_policy.all_allowed_to(:read).any? + end + + # A collection is one decision, not one per item, and the decision is taken from the item + # class rather than from a row. Nothing here reads an element: a declaration is a property + # of the attribute, and the class of an unloaded relation's items is a property of the + # relation, so a list attribute costs no query and keeps `.includes`, `.where` and the rest + # for the callers which never needed wrapping. + def protect_collection(attribute, collection) + return wrap_items(attribute, collection) if attribute.nested? + + item_class = collection_item_class(collection) + return collection if item_class.nil? + return collection unless ResourcePolicy.config.protectable_class?(item_class) + + ResourcePolicy.config.report_unprotected_nested_value( + policy: policy, attribute: attribute, value_class: item_class + ) + collection + end + + # Wrapping items is the only case which has to give up the relation, so it happens on + # `:hard` alone: a relation handed to a caller on `:soft` is the one the target returned, + # lazy and chainable, holding everything it held. `nil` entries survive the wrapping, + # because only the items the viewer may not read are meant to disappear. + def wrap_items(attribute, collection) + return collection unless ResourcePolicy.config.hard? + + collection.each_with_object([]) do |item, wrapped| + next wrapped << nil if item.nil? + + protected_item = protect(attribute, item) + wrapped << protected_item unless protected_item.nil? + end + end + + # `klass` is what an ActiveRecord::Relation answers without loading anything, so a relation + # is decided whether or not it holds rows. Any other enumerable is already in memory, so + # reading its first item costs nothing - and an empty one says nothing about its contents, + # which is the one case left where a missing declaration goes unnoticed. + def collection_item_class(collection) + return collection.klass if collection.respond_to?(:klass) + + collection.first&.class + end + + def collection?(value) + value.is_a?(Enumerable) && !value.is_a?(Hash) && !value.is_a?(Struct) + end + def target_respond_to?(method_name, *args) accessible_attributes.include?(method_name.to_sym) && policy_target.respond_to?(method_name, *args) diff --git a/lib/resource_policy/validators/attributes_validator.rb b/lib/resource_policy/validators/attributes_validator.rb index 63641f6..7cdfe8c 100644 --- a/lib/resource_policy/validators/attributes_validator.rb +++ b/lib/resource_policy/validators/attributes_validator.rb @@ -39,14 +39,12 @@ def validate_each(record, _attribute, policy) private def validate_attribute_policy(attribute_policy, record:, hash_attribute:) - if attribute_policy.nil? - return add_missing_policy_error_for(record, attribute: hash_attribute) - end + return add_missing_policy_error_for(record, attribute: hash_attribute) if attribute_policy.nil? access_level = access_level_for(record) - if !attribute_policy.allowed_to?(access_level) - return add_not_permitted_error_for(record, attribute: hash_attribute) - end + return if attribute_policy.allowed_to?(access_level) + + add_not_permitted_error_for(record, attribute: hash_attribute) end def access_level_for(record) diff --git a/lib/resource_policy/version.rb b/lib/resource_policy/version.rb index 2e0d9d9..465ace4 100644 --- a/lib/resource_policy/version.rb +++ b/lib/resource_policy/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module ResourcePolicy - VERSION = '1.1.0' + VERSION = '2.0.0' end diff --git a/resource_policy.gemspec b/resource_policy.gemspec index 97c745b..a98a76b 100644 --- a/resource_policy.gemspec +++ b/resource_policy.gemspec @@ -37,11 +37,11 @@ Gem::Specification.new do |spec| spec.add_development_dependency 'bundler', '~> 4.0' spec.add_development_dependency 'pry-byebug' + spec.add_development_dependency 'rails', '~> 8.0.0' spec.add_development_dependency 'rake', '~> 13.0' spec.add_development_dependency 'rspec', '~> 3.0' spec.add_development_dependency 'rubocop' spec.add_development_dependency 'rubocop-performance' spec.add_development_dependency 'rubocop-rails' spec.add_development_dependency 'rubocop-rspec' - spec.add_development_dependency 'rails', '~> 8.0.0' end diff --git a/spec/lib/resource_policy/configuration_spec.rb b/spec/lib/resource_policy/configuration_spec.rb new file mode 100644 index 0000000..4774c60 --- /dev/null +++ b/spec/lib/resource_policy/configuration_spec.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +require 'spec_helper' + +module ResourcePolicy + RSpec.describe Configuration do + subject(:config) { described_class.new } + + # The configuration only ever reads `.name` off an attribute and `.class` off a policy, so + # standing them up as plain objects keeps the examples about the toggle itself. + let(:attribute) { Struct.new(:name).new(:current_contract) } + let(:policy) { Struct.new(:target).new(nil) } + let(:record) { Struct.new(:id).new(1) } + let(:reported) { [] } + + before { config.reporter = ->(event) { reported << event } } + + describe '#nested_protection' do + it 'enforces by default, so a gap fails on the first test run' do + expect(config.nested_protection).to eq(:hard) + end + + it 'is switched by assigning a mode' do + config.nested_protection = :soft + + expect(config).not_to be_hard + end + + it 'accepts a string, so the mode can come straight from an env var' do + config.nested_protection = 'soft' + + expect(config.nested_protection).to eq(:soft) + end + + it 'refuses a mode it does not know, rather than silently enforcing nothing' do + expect { config.nested_protection = :warn } + .to raise_error(ArgumentError, /unknown mode :warn/) + end + end + + describe '#protectable?' do + it 'treats nothing as protectable until the host app says otherwise' do + expect(config).not_to be_protectable_class(record.class) + end + + it 'asks the configured callable about the class, never the value' do + config.protectable_class = ->(klass) { klass <= record.class } + + expect(config).to be_protectable_class(record.class) + end + end + + describe '#report_unprotected_nested_value' do + def report + config.report_unprotected_nested_value(policy: policy, attribute: attribute, value_class: record.class) + end + + context 'when the mode is hard' do + it 'raises, because nobody can vouch for the value' do + expect { report }.to raise_error(Configuration::UnprotectedNestedValueError, /:current_contract/) + end + + it 'does not report, because raising is the whole signal' do + expect { report }.to raise_error(Configuration::UnprotectedNestedValueError) & change { reported }.by([]) + end + end + + context 'when the mode is soft' do + before { config.nested_protection = :soft } + + it 'reports instead of raising' do + report + + expect(reported.map(&:attribute)).to eq([attribute]) + end + + it 'names the declaration which would fix it' do + report + + expect(reported.first.message).to include('c.attribute(:current_contract).nested') + end + end + end + + describe '.configure' do + after { ResourcePolicy.instance_variable_set(:@config, nil) } + + it 'yields the global configuration' do + ResourcePolicy.configure { |c| c.nested_protection = :soft } + + expect(ResourcePolicy.config.nested_protection).to eq(:soft) + end + + it 'returns the configuration it configured' do + expect(ResourcePolicy.configure { |c| c.nested_protection = :soft }).to be(ResourcePolicy.config) + end + end + end +end diff --git a/spec/lib/resource_policy/nested_protection_spec.rb b/spec/lib/resource_policy/nested_protection_spec.rb new file mode 100644 index 0000000..6238f2c --- /dev/null +++ b/spec/lib/resource_policy/nested_protection_spec.rb @@ -0,0 +1,757 @@ +# frozen_string_literal: true + +require 'spec_helper' + +module ResourcePolicy + RSpec.describe 'nested attribute protection' do + subject(:protected_resource) { ProtectedResource.new(policy) } + + # Stands in for an ActiveRecord model: something the host app says must carry its own policy. + let(:record_class) { Class.new { attr_accessor :role, :secret } } + + # Stands in for an ActiveRecord::Relation: enumerable, but carrying query methods which are + # lost the moment anything calls `map` on it. + let(:relation_class) do + Class.new do + include Enumerable + + attr_reader :klass, :loads + + def initialize(items, klass:) + @items = items + @klass = klass + @loads = 0 + end + + def each(&block) + @loads += 1 + @items.each(&block) + end + + def includes(*) + self + end + end + end + + let(:contract_class) do + Class.new(record_class) do + def initialize(hours) + super() + @hours = hours + end + + attr_reader :hours + end + end + + let(:contract_policy_class) do + Struct.new(:contract, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :contract + c.attribute(:hours).allowed(:read, if: :hours_readable?) + end + + private + + def hours_readable? + viewer.hours_reader? + end + end + end + + let(:target_class) { Struct.new(:current_contract, :contracts, :name) } + + let(:policy_class) do + nested_class = contract_policy_class + + Struct.new(:target, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :target + c.attribute(:current_contract) + .allowed(:read) + .nested { |contract| nested_class.new(contract, viewer) } + c.attribute(:contracts) + .allowed(:read) + .nested { |contract| nested_class.new(contract, viewer) } + c.attribute(:name).allowed(:read) + end + end + end + + let(:viewer) { double('Viewer', hours_reader?: true) } # rubocop:disable RSpec/VerifiedDoubles + let(:contract) { contract_class.new(37) } + let(:target) { target_class.new(contract, [contract], 'John') } + let(:policy) { policy_class.new(target, viewer) } + + before do + ResourcePolicy.config.protectable_class = ->(klass) { klass <= record_class } + end + + after do + ResourcePolicy.instance_variable_set(:@config, nil) + end + + describe 'a declared nested attribute' do + it 'is wrapped in the nested policy proxy rather than handed out raw' do + expect(protected_resource.current_contract).to be_a(ProtectedResource) + end + + it 'exposes nested values the nested policy allows' do + expect(protected_resource.current_contract.hours).to eq(37) + end + + context 'when the nested policy allows nothing' do + let(:viewer) { double('Viewer', hours_reader?: false) } # rubocop:disable RSpec/VerifiedDoubles + + it 'withholds the whole value on :hard, rather than a proxy answering nil to everything' do + expect(protected_resource.current_contract).to be_nil + end + + context 'when configured soft' do + let(:reported) { [] } + + before do + ResourcePolicy.config.nested_protection = :soft + ResourcePolicy.config.reporter = ->(event) { reported << event } + end + + it 'hands back the record itself, exactly as the target returned it' do + expect(protected_resource.current_contract).to be(contract) + end + + it 'says nothing, because a declaration is visible in the policy already' do + protected_resource.current_contract + + expect(reported).to be_empty + end + end + end + + context 'when the nested policy declares a :read action' do + let(:contract_policy_class) do + Struct.new(:contract, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :contract + c.action(:read).allowed(if: :readable?) + c.attribute(:hours).allowed(:read) + end + + private + + def readable? + viewer.hours_reader? + end + end + end + + it 'hands the value over when the action allows reading it' do + expect(protected_resource.current_contract).to be_a(ProtectedResource) + end + + context 'when the action denies reading it' do + let(:viewer) { double('Viewer', hours_reader?: false) } # rubocop:disable RSpec/VerifiedDoubles + + it 'withholds the value even though its attributes would allow the read' do + expect(protected_resource.current_contract).to be_nil + end + + context 'when configured soft' do + before do + ResourcePolicy.config.nested_protection = :soft + ResourcePolicy.config.reporter = ->(_event) {} + end + + it 'hands the record over, because soft applies no declaration' do + expect(protected_resource.current_contract).to be(contract) + end + end + end + end + + context 'when the value is a collection' do + it 'wraps every item' do + expect(protected_resource.contracts).to all(be_a(ProtectedResource)) + end + + it 'keeps nested values reachable' do + expect(protected_resource.contracts.map(&:hours)).to eq([37]) + end + + context 'when the nested policy allows nothing' do + let(:viewer) { double('Viewer', hours_reader?: false) } # rubocop:disable RSpec/VerifiedDoubles + + it 'drops the withheld items rather than padding the collection with nils' do + expect(protected_resource.contracts).to eq([]) + end + + context 'when configured soft' do + let(:reported) { [] } + + before do + ResourcePolicy.config.nested_protection = :soft + ResourcePolicy.config.reporter = ->(event) { reported << event } + end + + it 'hands the collection back untouched, items and all' do + expect(protected_resource.contracts).to eq([contract]) + end + + it 'says nothing' do + protected_resource.contracts + + expect(reported).to be_empty + end + end + end + end + + context 'when the value is nil' do + let(:contract) { nil } + + it 'stays nil' do + expect(protected_resource.current_contract).to be_nil + end + end + end + + # The rule which matters during a rollout: adding a `.nested` declaration must not change + # anything until `:hard` is switched on. + describe 'an attribute the nested policy denies' do + let(:contract_policy_class) do + Struct.new(:contract, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :contract + c.action(:read).allowed + c.attribute(:hours).allowed(:read, if: :hours_readable?) + end + + private + + def hours_readable? + viewer.hours_reader? + end + end + end + + let(:viewer) { double('Viewer', hours_reader?: false) } # rubocop:disable RSpec/VerifiedDoubles + + it 'is nil on :hard' do + expect(protected_resource.current_contract.hours).to be_nil + end + + context 'when configured soft' do + before { ResourcePolicy.config.nested_protection = :soft } + + it 'is the raw record, so the value is reachable exactly as it was' do + expect(protected_resource.current_contract.hours).to eq(37) + end + end + end + + describe 'a value which needs no policy' do + it 'is returned untouched' do + expect(protected_resource.name).to eq('John') + end + end + + describe 'an undeclared record' do + let(:policy_class) do + Struct.new(:target, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :target + c.attribute(:current_contract).allowed(:read) + end + end + end + + it 'raises in the default hard mode, naming the attribute' do + expect { protected_resource.current_contract } + .to raise_error(Configuration::UnprotectedNestedValueError, /:current_contract/) + end + + it 'suggests the declaration to add' do + expect { protected_resource.current_contract } + .to raise_error(Configuration::UnprotectedNestedValueError, /\.nested/) + end + + context 'when configured soft' do + let(:reported) { [] } + + before do + ResourcePolicy.config.nested_protection = :soft + ResourcePolicy.config.reporter = ->(event) { reported << event } + end + + it 'still returns the value, so production keeps working' do + expect(protected_resource.current_contract).to be(contract) + end + + it 'reports the attribute it could not vouch for' do + protected_resource.current_contract + expect(reported.map { |event| event.attribute.name }).to eq([:current_contract]) + end + end + + context 'when the attribute is explicitly opted out' do + let(:policy_class) do + Struct.new(:target, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :target + c.attribute(:current_contract).allowed(:read).unprotected(because: 'value object') + end + end + end + + it 'is returned untouched' do + expect(protected_resource.current_contract).to be(contract) + end + end + end + + describe 'a relation-backed collection' do + let(:relation) { relation_class.new([contract], klass: contract_class) } + let(:target) { target_class.new(contract, relation, 'John') } + + context 'when the attribute is opted out' do + let(:policy_class) do + Struct.new(:target, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :target + c.attribute(:contracts).allowed(:read).unprotected(because: 'checked elsewhere') + end + end + end + + it 'keeps the relation, so callers can still chain query methods' do + expect(protected_resource.contracts).to respond_to(:includes) + end + end + + context 'when the attribute is undeclared and the mode is soft' do + let(:policy_class) do + Struct.new(:target, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :target + c.attribute(:contracts).allowed(:read) + end + end + end + + before do + ResourcePolicy.config.nested_protection = :soft + ResourcePolicy.config.reporter = ->(_event) {} + end + + it 'keeps the relation rather than flattening it into an Array' do + expect(protected_resource.contracts).to respond_to(:includes) + end + end + + context 'when the attribute is undeclared and the mode is hard' do + let(:policy_class) do + Struct.new(:target, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :target + c.attribute(:contracts).allowed(:read) + end + end + end + + it 'raises once for the collection, naming the item class' do + expect { protected_resource.contracts } + .to raise_error(Configuration::UnprotectedNestedValueError, /:contracts/) + end + end + + # The decision is taken on every list read on every screen, so it has to cost nothing. + # `loads` counts the times the relation was enumerated. + context 'when deciding what to do with it' do + let(:policy_class) do + Struct.new(:target, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :target + c.attribute(:contracts).allowed(:read) + end + end + end + + before do + ResourcePolicy.config.nested_protection = :soft + ResourcePolicy.config.reporter = ->(_event) {} + end + + it 'reads no row from an undeclared relation' do + protected_resource.contracts + + expect(relation.loads).to eq(0) + end + + context 'when the attribute is opted out' do + let(:policy_class) do + Struct.new(:target, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :target + c.attribute(:contracts).allowed(:read).unprotected(because: 'checked elsewhere') + end + end + end + + it 'reads no row' do + protected_resource.contracts + + expect(relation.loads).to eq(0) + end + end + + context 'when the host app has not switched protection on' do + before { ResourcePolicy.config.protectable_class = Configuration::DEFAULT_PROTECTABLE_CLASS } + + it 'reads no row' do + protected_resource.contracts + + expect(relation.loads).to eq(0) + end + end + + context 'when the attribute declares a nested policy' do + let(:policy_class) do + nested_class = contract_policy_class + + Struct.new(:target, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :target + c.attribute(:contracts) + .allowed(:read) + .nested { |contract| nested_class.new(contract, viewer) } + end + end + end + + it 'reads no row on :soft, because the declaration is not applied' do + protected_resource.contracts + + expect(relation.loads).to eq(0) + end + + it 'reads it once on :hard, because every item has to be wrapped' do + ResourcePolicy.config.nested_protection = :hard + protected_resource.contracts + + expect(relation.loads).to eq(1) + end + end + end + + context 'when the collection is empty' do + let(:relation) { relation_class.new([], klass: contract_class) } + + let(:policy_class) do + Struct.new(:target, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :target + c.attribute(:contracts).allowed(:read) + end + end + end + + it 'is still decided, because a relation knows its item class whether or not it holds rows' do + expect { protected_resource.contracts } + .to raise_error(Configuration::UnprotectedNestedValueError, /:contracts/) + end + end + end + + describe 'an array-backed collection' do + let(:target) { target_class.new(contract, items, 'John') } + + let(:policy_class) do + Struct.new(:target, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :target + c.attribute(:contracts).allowed(:read) + end + end + end + + context 'when it holds records' do + let(:items) { [contract] } + + it 'is decided from the item it already holds in memory' do + expect { protected_resource.contracts } + .to raise_error(Configuration::UnprotectedNestedValueError, /:contracts/) + end + end + + context 'when it is empty' do + let(:items) { [] } + + it 'says nothing about what it would have held, so it is handed back untouched' do + expect(protected_resource.contracts).to eq([]) + end + end + end + + # The question the rollout turns on: with declarations in place, is `:soft` distinguishable + # from an application which has never heard of this gem? + # A protected resource answers the attributes its policy declares and nothing else, which is + # the whole point of it. Nesting therefore hands a narrow object to whatever consumes the + # value, and code which treats it as the record - a decorator, a serialiser, anything + # calling an association or a helper - stops working the moment `:hard` is switched on. + describe 'a nested value handed to code which expects the record' do + let(:contract_class) do + Class.new(record_class) do + def initialize(hours) + super() + @hours = hours + end + + attr_reader :hours + + def to_param + "contract-#{hours}" + end + end + end + + it 'answers the attributes the nested policy declares' do + expect(protected_resource.current_contract.hours).to eq(37) + end + + it 'raises on anything the nested policy does not declare, rather than answering nil' do + expect { protected_resource.current_contract.to_param } + .to raise_error(NoMethodError, /to_param/) + end + + context 'when configured soft' do + before { ResourcePolicy.config.nested_protection = :soft } + + it 'is the record itself, so undeclared methods still work' do + expect(protected_resource.current_contract.to_param).to eq('contract-37') + end + end + end + + # The shape most of an application's nested records actually travel in: a decorator which + # builds the nested policy itself. Declaring `nested` as well protects the value twice - the + # decorator ends up constructing its policy on the proxy, and the policy's own conditions + # read the record - so the declaration to use is `unprotected`, with the reason written down. + describe 'a value a decorator protects downstream' do + let(:decorator_class) do + nested_class = contract_policy_class + viewer_for_decorator = viewer + + Class.new do + define_method(:initialize) { |contract| @contract = contract } + + define_method(:protected_contract) do + nested_class.new(@contract, viewer_for_decorator).protected_resource + end + + def hours + protected_contract.hours + end + end + end + + # A condition which reads the record, exactly as a real policy's conditions do. + let(:contract_policy_class) do + Struct.new(:contract, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :contract + c.action(:read).allowed + c.attribute(:hours).allowed(:read, if: :readable_contract?) + end + + private + + def readable_contract? + !contract.ended? && viewer.hours_reader? + end + end + end + + let(:contract_class) do + Class.new(record_class) do + def initialize(hours) + super() + @hours = hours + end + + attr_reader :hours + + def ended? + false + end + end + end + + context 'when the attribute is declared nested' do + it 'breaks the decorator, which needs the record its own policy reads' do + expect { decorator_class.new(protected_resource.current_contract).hours } + .to raise_error(NoMethodError, /ended\?/) + end + end + + context 'when the attribute is declared unprotected instead' do + let(:policy_class) do + Struct.new(:target, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :target + c.attribute(:current_contract) + .allowed(:read) + .unprotected(because: 'exposed only through the decorator, which applies ContractPolicy') + end + end + end + + it 'hands the record over, so the decorator works' do + expect(decorator_class.new(protected_resource.current_contract).hours).to eq(37) + end + + it 'loses no protection, because the decorator applies the same policy' do + allow(viewer).to receive(:hours_reader?).and_return(false) + + expect(decorator_class.new(protected_resource.current_contract).hours).to be_nil + end + end + end + + describe 'soft against no protection at all' do + let(:relation) { relation_class.new([contract, nil], klass: contract_class) } + let(:target) { target_class.new(contract, relation, 'John') } + + def read_everything + { + single: protected_resource.current_contract, + list: protected_resource.contracts, + plain: protected_resource.name + } + end + + # The baseline is the target itself: what a caller would get with no policy in the way + # at all, which is what the application does today. + it 'hands back the very same objects the target would' do + ResourcePolicy.config.nested_protection = :soft + + expect(read_everything).to eq( + single: target.current_contract, list: target.contracts, plain: target.name + ) + end + + it 'leaves the list a relation, not an array' do + ResourcePolicy.config.nested_protection = :soft + + expect(protected_resource.contracts).to be(relation) + end + + it 'keeps every item in it, including the empty ones' do + ResourcePolicy.config.nested_protection = :soft + + expect(protected_resource.contracts.to_a).to eq([contract, nil]) + end + + it 'never reads a row from it' do + ResourcePolicy.config.nested_protection = :soft + protected_resource.contracts + + expect(relation.loads).to eq(0) + end + + it 'is distinguishable on :hard, which is where the declaration takes effect' do + expect(protected_resource.contracts).to be_a(Array) + end + end + + describe 'declarations made inside a group' do + let(:policy_class) do + nested_class = contract_policy_class + + Struct.new(:target, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :target + c.group(:allowed?) do |g| + g.attribute(:current_contract) + .allowed(:read) + .nested { |contract| nested_class.new(contract, viewer) } + end + end + + private + + def allowed? + true + end + end + end + + it 'survives the group merge' do + expect(protected_resource.current_contract).to be_a(ProtectedResource) + end + end + + describe 'declarations inherited by a subclass' do + let(:subclass) { Class.new(policy_class) } + let(:policy) { subclass.new(target, viewer) } + + it 'survives inheritance' do + expect(protected_resource.current_contract).to be_a(ProtectedResource) + end + end + + describe 'an app which has not opted in' do + before do + ResourcePolicy.instance_variable_set(:@config, nil) + end + + let(:policy_class) do + Struct.new(:target, :viewer) do + include ResourcePolicy::Policy + + policy do |c| + c.policy_target :target + c.attribute(:current_contract).allowed(:read) + end + end + end + + it 'behaves exactly as before, handing the value back' do + expect(protected_resource.current_contract).to be(contract) + end + end + end +end diff --git a/spec/lib/resource_policy/policy/actions_policy/actions_policy_model_spec.rb b/spec/lib/resource_policy/policy/actions_policy/actions_policy_model_spec.rb index 1365f41..156d2e3 100644 --- a/spec/lib/resource_policy/policy/actions_policy/actions_policy_model_spec.rb +++ b/spec/lib/resource_policy/policy/actions_policy/actions_policy_model_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ::ResourcePolicy:: Policy::ActionsPolicy::ActionsPolicyModel do +RSpec.describe ::ResourcePolicy::Policy::ActionsPolicy::ActionsPolicyModel do subject(:actions_policy_model) { described_class.new(model_instance) } let(:policy_model) do diff --git a/spec/lib/resource_policy/policy/actions_policy_spec.rb b/spec/lib/resource_policy/policy/actions_policy_spec.rb index a37c6a6..e30ea70 100644 --- a/spec/lib/resource_policy/policy/actions_policy_spec.rb +++ b/spec/lib/resource_policy/policy/actions_policy_spec.rb @@ -27,13 +27,13 @@ def no let(:actions_policy) { model.new.actions_policy } context 'when action conditions are passing' do - it 'marks actions as allowed ' do + it 'marks actions as allowed' do expect(actions_policy.create).to be_allowed end end context 'when action conditions are failing' do - it 'marks actions as not allowed ' do + it 'marks actions as not allowed' do expect(actions_policy.failing).not_to be_allowed end end diff --git a/spec/lib/resource_policy/policy/policy_configuration_spec.rb b/spec/lib/resource_policy/policy/policy_configuration_spec.rb index b5e4faa..c9e938e 100644 --- a/spec/lib/resource_policy/policy/policy_configuration_spec.rb +++ b/spec/lib/resource_policy/policy/policy_configuration_spec.rb @@ -55,7 +55,7 @@ module ResourcePolicy::Policy end it 'adds action to actions list' do - expect(policy_configuration.actions.keys).to match_array([:read]) + expect(policy_configuration.actions.keys).to contain_exactly(:read) end it 'does not add condition to other actions' do diff --git a/spec/lib/resource_policy/protected_resource_spec.rb b/spec/lib/resource_policy/protected_resource_spec.rb index a6fb1f4..ce9f89b 100644 --- a/spec/lib/resource_policy/protected_resource_spec.rb +++ b/spec/lib/resource_policy/protected_resource_spec.rb @@ -63,20 +63,20 @@ def writable? end it 'returns false' do - expect(protected_resource).not_to be_respond_to(:some_non_policy_attribute) + expect(protected_resource).not_to respond_to(:some_non_policy_attribute) end end context 'when attribute is specified on policy' do it 'returns true' do - expect(protected_resource).to be_respond_to(:some_attribute) + expect(protected_resource).to respond_to(:some_attribute) end end end context 'when method does not exist on target' do it 'returns false' do - expect(protected_resource).not_to be_respond_to(:non_existing_method) + expect(protected_resource).not_to respond_to(:non_existing_method) end end end diff --git a/spec/lib/resource_policy/validators/attributes_validator_spec.rb b/spec/lib/resource_policy/validators/attributes_validator_spec.rb index c56f358..323f076 100644 --- a/spec/lib/resource_policy/validators/attributes_validator_spec.rb +++ b/spec/lib/resource_policy/validators/attributes_validator_spec.rb @@ -33,13 +33,11 @@ def initialize(allowed:) end end - let(:params) { { first_name: 'John' } } let(:policy) { policy_class.new(allowed: is_allowed) } let(:is_allowed) { true } let(:access_level) { :write } - context 'when attribute matches expected access level' do it { is_expected.to be_valid } end diff --git a/spec/resource_policy_spec.rb b/spec/resource_policy_spec.rb index 64037e1..f092358 100644 --- a/spec/resource_policy_spec.rb +++ b/spec/resource_policy_spec.rb @@ -2,6 +2,6 @@ RSpec.describe ResourcePolicy do it 'has a version number' do - expect(ResourcePolicy::VERSION).not_to be nil + expect(ResourcePolicy::VERSION).not_to be_nil end end