Fix: permitted_attributes ignores cannot and default-deny rules#882
Open
tmaier wants to merge 1 commit into
Open
Fix: permitted_attributes ignores cannot and default-deny rules#882tmaier wants to merge 1 commit into
permitted_attributes ignores cannot and default-deny rules#882tmaier wants to merge 1 commit into
Conversation
The `permitted_attributes` method was not correctly handling cases where an action was denied by a `cannot` rule or by default. It would still return a list of attributes from any `can` rules defined for the model, instead of an empty array. This commit adds a guard clause to check `can?(action, subject)` at the beginning of the `permitted_attributes` method. If the action is not allowed, it now correctly returns an empty array, respecting the principle of default-deny. This ensures that attribute sanitization is consistent with the user's defined abilities.
tmaier
force-pushed
the
tmaier-fix-permitted-attributes
branch
from
b43059e to
22fe2e2
Compare
tmaier
changed the title
permitted_attributes ignores cannot and default-deny rules
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes a bug in
permitted_attributeswhere it would return a list of attributes for an action, even when that action was explicitly forbidden by acannotrule or denied by default. The expected behavior in a "default-deny" system is for it to return an empty array.For example, given the ability:
The call
ability.permitted_attributes(:update, Project)would incorrectly return all model attributes, instead of the expected[]. This also occurred in default-deny scenarios where no ability was defined at all for a given action.The Fix
The fix is to add a guard clause at the beginning of the
permitted_attributesmethod. It now uses the corecan?method to check if the user is authorized to perform the action before collecting the attributes. Ifcan?returnsfalse,permitted_attributeswill now correctly return an empty array.This ensures that the behavior of
permitted_attributesis consistent with the rest of the library's authorization logic.I considered an alternative which changes the logic of the method completely.
The idea was to collect all forbidden attributes and subtract then the allowed.
And then to return all attributes minus the forbidden attributes.