Add for_all_actions composite matcher to permit/forbid_mass_assignment_of

[chrisalley]

The readme current states the following:

Warning: Currently, Pundit Matchers does not automatically check if the attribute is permitted by a permitted_attributes_for_#{action} method, so even if you include a forbid_mass_assignment_of(:attribute) expectation in the policy spec, it's entirely possible that the attribute is being permitted through a permitted_attributes_for_#{action} method that is tested separately. For this reason, you should always explicitly test all implemented permitted_attributes_for_#{action} methods, as demonstrated in the example.

However if an attribute is permitted or forbidden in all permitted_attributes methods in the policy, the spec code can become quite long winded. Here's an example from the readme:

 it { is_expected.to permit_mass_assignment_of(:slug) }
 it { is_expected.to permit_mass_assignment_of(:slug).for_action(:create) }
 it { is_expected.to permit_mass_assignment_of(:slug).for_action(:update) }

It would be useful to check do this test in a single statement. I propose that we add a for_all_actions compose matcher to do just that:

 it { is_expected.to permit_mass_assignment_of(:slug).for_all_actions }

This would also work for forbid_mass_assignment_of.

[chrisalley]

In 2018 I proposed adding a composite matcher for_all_actions. However, I've been thinking about this some more since then. It may be safer if we check all actions by default, rather than leaving it to the developer to remember to add for_all_actions. From a security point of view, this would be consistent with the comprehensiveness used by permit_only_actions, our new default approach to testing actions.

Testing permitted attributes for all actions by default would be a breaking change though, which could be disruptive. If there is value in checking all actions by default, perhaps we could consider adding new matchers such as permit_attributes and permit_only_attributes?

[jasonkarns]

Some related thoughts as this feature is worked through:

Given that Pundit internally falls back from an permitted_attributes_for_{action}-specific method to generic permitted_attributes; I think pundit-matchers should reflect that capability.

That is:

Given a policy that implements def permitted_attributes = [:slug] And a test that matches via is_expected.to permit_mass_assignment_of(:slug).for_action(:create)

It should pass. Why? Because that configuration of a pundit policy truly does permit the slug attribute for create actions. The current matchers fail because they don't implement the same fallback rules that pundit actually does.

[jasonkarns]

I'd also add some additional caution/concern regarding the potential implementation of for_all_actions. Note that any value can be provided to a policy as the action. So however for_all_actions is implemented, it must not presume the actions to be limited to create|update|etc. (Which means the implementation would theoretically need to iterate over all policy methods that match permitted_attributes_for_*)

(I have a vested interest in this capability because we're using Pundit to authorize our Graphiti api, so the "actions" for attribute guards are :read, :write, :filter, :sort, and :schema)

[chrisalley]

@jasonkarns Yes, I think your reasoning is solid for us to change the behaviour of permit_mass_assignment_of to iterate over permitted_attributes_for_* policy methods. This would be a behaviour change requiring a new major version of Pundit Matchers as it may break some existing usage of the matcher, but would make the API clearer than creating a new matcher.

A question remains: do we need a fallback composite matcher for testing the default permitted_attributes method? E.g. permit_mass_assignment_of(:publish).for_default_perrmitted_attributes_method_only. If we're considering these as purely behavioural tests (rather than method tests), then I would argue that the answer is "no". The permitted_attributes methods can be considered an implementation detail of the behaviour of permitting particular actions. However, we should add some details about which method did or did not permit the attribute to the error message displayed when a test fails.