Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds support for rules at the attribute/method level #474

Conversation

phaedryx
Copy link
Contributor

@phaedryx phaedryx commented Dec 14, 2017

Allows you to add attribute-level rules, e.g.

can :read, User, :first_name
can :read, User, :last_name
cannot :read, User, :api_key

@phaedryx
Copy link
Contributor Author

phaedryx commented Dec 14, 2017

@coorasse something I would like you to weigh in on.

For the can? method Ryan added a third, attribute parameter here:
https://github.com/ryanb/cancan/blob/2.0/lib/cancan/ability.rb#L47

Unfortunately, in the main code base, he also added an extra_args parameter here:
ryanb/cancan@69f7a65?diff=unified#diff-d4e0a70d43eb4c389e623945f417d6e0R43
in response to this issue: ryanb/cancan#48

Which is part of the code base now. I thought it might be an undocumented or unused feature that could be replaced, but there are clearly tests for it here: https://github.com/CanCanCommunity/cancancan/blob/develop/spec/cancan/ability_spec.rb#L273

Because extra_args could be anything, the only way to reconcile is to change the can? method signature to def can?(action, subject, attribute, *extra_args) which would be a breaking change.

Thoughts?

@phaedryx phaedryx force-pushed the incorporate-ryanbs-attribute-code branch 2 times, most recently from 58b8a85 to c4345d8 Compare Jan 3, 2018
  This is based on Ryan Bate's Cancan 2.0 code, but modified to match
  modern Cancancan. Tests are included.

  I also updated the rubocop config to target Ruby to version 2.2.0 to
  match the required version in the gemspec and to avoid end of life
  warnings.
@phaedryx phaedryx force-pushed the incorporate-ryanbs-attribute-code branch from c4345d8 to 4ac0b1b Compare Jan 4, 2018
@coorasse
Copy link
Member

coorasse commented Jan 16, 2018

Good point. I also didn't remember of the extra_args feature honestly. 😄
I think is very useful and should be documented properly!

This brings me to a fixed point: we should preserve that feature.
Said that we have two options in front of us on how we could introduce the attributes feature: add a new parameter and break changes, add them to the extra_args.
in your example, if I use the extra_args I would end up with:

 can? :update, book, nil, [2, 3]

to have [2, 3] as extra args, which sucks.
Why don't we integrate the attributes directly in the extra_args?
e.g.

 can? :update, book, attribute: :title

this would not be a breaking change. what do you think?

@coorasse
Copy link
Member

coorasse commented Jan 16, 2018

From my point of view we could also have a breaking change and be more open to future implementations. changing the syntax to:
can? :update, book, attribute: :title, extra_args: {...}

.rubocop.yml Outdated Show resolved Hide resolved
lib/cancan/conditions_matcher.rb Outdated Show resolved Hide resolved
spec/cancan/ability_spec.rb Outdated Show resolved Hide resolved
@CyborgMaster
Copy link

CyborgMaster commented Jan 19, 2018

I'm very interested in having fields in our CanCan permissions and would love to see them as first class citizens.

@coorasse, I agree, having to pass nil as a third parameter to get to the extra args is annoying. This doesn't look very natural:

 can? :update, book, nil, [2, 3]

However, I'm not fond of having to write attributes: every time we check a permission. If I fold this into our project, I'll be doing that a ton and it feels a little clunky. It makes attributes feel tacked on and I see them as being just as important as action and subject.

What about going in between your two suggestions and making extra_args a named parameter and attributes an optional positional one. That way you could have calls like this:

 can? :update, book, extra_args: [2, 3]

or this:

 can? :update, book, [:title, :author], extra_args: [2, 3]

because extra_args is a named parameter and the attributes are optional, both would work. This way the three main parameters, action, subject, and attributes are all positional and the tack on, extra_args is named.

@coorasse, what do you think?
@phaedryx, would you be willing to go this way?

@phaedryx
Copy link
Contributor Author

phaedryx commented Jan 19, 2018

@CyborgMaster I like the suggestion. I'll explore some code around it.

I'm interested in this because I'm working on a GraphQL API and would love to be able to easily define attribute-level permissions in a simple, straightforward way.

@coorasse I like the symmetry of can :action, Subject, :attribute and can? :action, Subject, :attribute. Would your solution be to add attributes: to both?

@phaedryx
Copy link
Contributor Author

phaedryx commented Jan 20, 2018

@coorasse When I run the tests locally they all pass. I'm not sure how to solve the pg gem error.

@phaedryx
Copy link
Contributor Author

phaedryx commented Jan 20, 2018

I have removed the nils from the tests where they weren't needed and wrote a test to cover two instances when they are.

That is, the nil is only needed when you don't have attributes for a rule but you do have additional arguments that look like attributes, e.g. an array of symbols [:foo, :bar]. I expect this is a rare edge case.

@pedrocarmona
Copy link

pedrocarmona commented Jan 22, 2018

I also need this PR. Thx @phaedryx 👍

@phaedryx
Copy link
Contributor Author

phaedryx commented Jan 22, 2018

Note: because of how I handled the initialize method of Rule, the change isn't as breaking as I initially thought it would be. The only incompatibility, like I mentioned above, is if you are passing a symbol or array of symbols as extra arguments (passing conditions works the same as before). The extra arguments functionality is barely remembered and undocumented, so I assume it is rarely used, let alone with an array of symbols. Adding a nil in this case doesn't seem like a big problem. All that said, I don't think I will add the attributes:.

Currently, the latest version of pg is 1.0.0, but activerecord 5.1.0 is
not compatible with it (see https://goo.gl/L14fPa for an example). It is
better to pin to a known compatible version.
@phaedryx
Copy link
Contributor Author

phaedryx commented Feb 6, 2018

This adds a permitted_attributes method. I imagine it will be used like this: params.require(:user).permit(*current_ability.permitted_attributes(:update, User))

@coorasse coorasse added this to the 3.0 milestone Feb 8, 2018
@phaedryx
Copy link
Contributor Author

phaedryx commented Feb 8, 2018

I've been trying to make extra_args work as a named parameter and I've been having some difficulty. The primary problem is how mix conditions hashes without {} (which is common) with a named parameter and handle them correctly.

@phaedryx
Copy link
Contributor Author

phaedryx commented Feb 9, 2018

@coorasse
I think it is outside of the scope of this pull request, but as a follow-up I think, at a minimum, this method and this method should be renamed. Really, how strong parameters are handle and code like this and this should be reconsidered.

@phaedryx
Copy link
Contributor Author

phaedryx commented Apr 3, 2018

@stalniy this refactor addresses your comments

@coorasse
Copy link
Member

coorasse commented Apr 9, 2018

this is proceeding well. it will take still some time but I have an eye on it 👍

@phaedryx
Copy link
Contributor Author

phaedryx commented Apr 16, 2018

This branch does everything I need it to do and I've fixed all of the bugs I can think of. I consider it complete.

@phaedryx phaedryx changed the title Incorporate ryanbs attribute code Adds support for rules at the attribute/method level May 21, 2018
@coorasse coorasse merged commit 0886db9 into CanCanCommunity:feature/3.0.0 Jun 22, 2018
1 check was pending
@seanmsmith23
Copy link

seanmsmith23 commented May 13, 2020

Is there any reason not to use some of these features @phaedryx ? I don't think I see them documented in the wiki, but they would be useful for a project I'm working on.

@coorasse
Copy link
Member

coorasse commented May 14, 2020

The feature is available and can be used. Is not well documented because we are moving the wiki at the moment. Please use it! 👍

@seanmsmith23
Copy link

seanmsmith23 commented Jun 11, 2020

Great, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants