Authorize relationship actions with custom policy methods

[matthias-g]

This PR is to discuss a proposal for how the authorization of relationships could be handled (see #30).

[matthias-g]

Given the following scenario:

class User < ActiveRecord::Base
  has_and_belongs_to_many :projects
end

class Project < ActiveRecord::Base
  has_and_belongs_to_many :users
end

and

class UserResource < BaseResource
  has_many :projects
end

class ProjectResource < BaseResource
  has_many :users
end

A user can be added to a project through
POST /projects/1/relationships/users

"data": [
  {
    "type": "users",
    "id": 1
  }
]

Now you can allow this action independently from allowing to update the project using allow_relationship_users?:

class ProjectPolicy < ApplicationPolicy
  def allow_relationship_users? users
    true
  end

  # but
  def update?
    false
  end
end

[valscion]

Yeah, that could work to allow for the relationship operations first to define more specific authorizations 🤔. That way we could first implement the relationship operations and only afterwards think about implementing the same thing in the generic update.

I think I was too bogged down in thinking how to allow PATCH calls directly to /projects/1 URL and didn't think of how it would look like for the relationship operations specifically.

We'd need tests to avoid breaking this functionality in the future and to make sure it works properly. The current tests are a bit... unwieldy right now, sorry about that 😳. They are meant to be request-level tests testing the actual JSON API payloads and ensuring authorization is ran properly and that all cases are covered.

Is there anything I could help you with?

[valscion]

The build is currently failing because Rubocop would like this piece of code to look something like this:

allowed =
  if policy.respond_to?(relationship_method)
    policy.send(relationship_method, new_related_records)
  else
    policy.update?
  end

[valscion]

Did you know that you could use this, too?

relationship_method = "allow_relationship_#{relationship_type}?"

It might make it more easier to add dynamic stuff together to form a string.

[matthias-g]

Thanks for the feedback! I've just updated the code according to your reviews and added some tests.

[valscion]

Thanks! Do you want to keep on going with this approach for the other relationship operation types, too? I think we could take this as the first proper approach for handling the complex cases and then later look into what it would mean if we'd get it done for the common PATCH /comments/1 case, too.

If you do want to keep on going for the other relationship types, too, I guess you could modify the pull request title to really say how amazing things are happening here 😄

[valscion]

That is, what do you want the scope of this pull request to be? Do you think this PR should be merged at some point, and if so, when do you feel like this PR is done? What things would you like to see implemented here?

[matthias-g]

Yeah, I think the other relationship operation types should be implemented as well before the PR is mergeable. I'd say that the result of this PR should be that as a user of JA you can define policies for relationship operations on a per relationship level (instead of relying just on update?).

I'm still in the process of trying out though and I don't have a clear picture of how the result should look like. As a next step I've implemented the remove_to_many_relationship method.
I think this should use another policy method than create_to_many_relationship because a user might be allowed to create a relationship but not to delete it or the other way around. In the example above a user might not be allowed to add himself to a project but to delete himself from a project.
I've called the new method remove_from_<relationship_name>?. What do you think about renaming allow_relationship_<relationship_name>? to add_to_<relationship_name>?? To me that would seem more intuitive.

What I noticed and wanted to document here is that a user of JA needs to ensure that allow_relationship_users? and allow_relationships_projects? return the same results. This is not done using the current implementation and I don't see a good way to do this in JA but them returning different results could lead to unwanted behaviour (in case POST /projects/1/relationships/users is forbidden but POST /users/1/relationships/projects is allowed although they do the same).

[valscion]

Yeah, I think the other relationship operation types should be implemented as well before the PR is mergeable. I'd say that the result of this PR should be that as a user of JA you can define policies for relationship operations on a per relationship level (instead of relying just on update?).

👏 Yes, great that you think so, too! I'll modify the PR title accordingly.

I think that we can leave out the entire update? method for now and figure that out later, if at all possible. We should be able to reuse the same policy methods for the added/removed associations in there later, too.

I'm still in the process of trying out though and I don't have a clear picture of how the result should look like

Yeah I figured that only after testing out actually implementing these, would we discover things that cannot easily be seen upfront. The final solution can be something different than the original proposal idea, as long as the principles I outlined back in #30 stand:

Prefer being overly restrictive rather than too permissive by accident.

I really appreciate you communicating your ideas and I'm very open in rethinking a solution together 😌

As a next step I've implemented the remove_to_many_relationship method.
I think this should use another policy method than create_to_many_relationship because a user might be allowed to create a relationship but not to delete it or the other way around.

👍 Sounds reasonable, and this gem should allow for such a scenario to be handled, too.

I've called the new method remove_from_<relationship_name>?

I like that name 👍

What do you think about renaming allow_relationship_<relationship_name>? to add_to_<relationship_name>?? To me that would seem more intuitive.

👍 This makes sense, feel free to do so. That has a nice symmetry with the remove method.

What I noticed and wanted to document here is that a user of JA needs to ensure that allow_relationship_users? and allow_relationships_projects? return the same results. This is not done using the current implementation and I don't see a good way to do this in JA but them returning different results could lead to unwanted behaviour (in case POST /projects/1/relationships/users is forbidden but POST /users/1/relationships/projects is allowed although they do the same).

Yes, this was in fact a big thing discussed over at #30 and was one of the reasons why the discussion got a bit heated.

There were talk about inspecting the relationship itself and then, with some logic, decide which of the two policies should be called. I liked the idea @aaronbhansen posted in #30 (comment), but I can't yet really form a proper opinion on the validity of that approach before I see some code implementing that feature.

Anyway, I'm a bit wary of going down that rabbit hole yet. Do you think it could be reasonable to solve that "both policies need to look the same" issue after we've got the primitive API working in this PR? I fear that tackling that issue might open a can of worms where we must start to add swathes of tests to cover different types of relationships. And oh boy, there are many. It might be too soon to start figuring it out in this PR already.

I did hack around a bit to see if that would even remotely be possible, and I found out that yes, jsonapi-resources does have the information of relationships on the level that would make solving this double-the-logic-across-two-policies problem. I commented on it at #30 (comment):

I haven't yet decided on which side of the association should handle the authorization. I'll have to restructure the spec dummy application a lot to include different kinds of relations so that I can be sure each case is tested properly 💦.

Based on my initial testings, it should be possible to get a nice approach for these authorization checks. Relationship objects coming from JR do know who owns the foreign key and allows jumping between the associated resources with relative ease.

This just might be quite big of an effort...

---

I'm really grateful for your efforts here! Would you like to get added as a contributor to this project once we get this out? ☺️

[valscion]

Hiya @matthias-g! What's the status with this PR? Anything I can do to help you move forwards?

[matthias-g]

Hey, thanks for asking. I'm sorry for being quiet the last weeks - I've just been busy with other work.
But I've already started this week working on the 'replace_to_many' and I think I'll be able to write the tests for that and commit them tomorrow. And to reply to your comment above. ;-)

[valscion]

Yay, great to hear! If you need help bouncing around ideas or anything, we can book a https://room.co/ video chat (just open your browser and it works, no software install required) to talk things through if that would help not getting stuck with this :)

[matthias-g]

Anyway, I'm a bit wary of going down that rabbit hole yet. Do you think it could be reasonable to solve that "both policies need to look the same" issue after we've got the primitive API working in this PR?

Definitely. I'm not even sure if solving this on the side of JA is the best way. Maybe a design pattern for the policies could work as well. Like creating a concern per relationship that includes methods which are being called by the respective policy methods. But it's a good idea to discuss that later. 😉

I'm really grateful for your efforts here! Would you like to get added as a contributor to this project once we get this out?

Thanks, that would be nice - once this has been merged.

---

For the recent commits I want to document two thoughts:

1. My first idea was to avoid creating a new policy method replace_users? by reusing remove_from_users? and add_to_users? instead (calling remove_from_users? for all old users and calling add_to_users? for the new users). But this doesn't work well enough as both methods are being called before any changes happen, meaning that add_to_users? is being called before the old users have been removed. This is a problem in the following scenario:
   add_to_users? only returns true if the given users are not yet in the users relation. User A is in users and replace_to_many is called with new users including user A.
   This would fail because user A can't be added, although the replace action is valid. That's why I used a new replace_users? method.

2. There was a TODO at replace_to_many_relationship in the authorizer: "Should probably take old records as well"
   I've deleted this because the old records are available for the policy through source_record.relationship_name (like project.users). So it shouldn't be necessary to pass them to the policy.

[valscion]

But it's a good idea to discuss that later. 😉

👍 Glad we're on the same page on this one 😅

Thanks, that would be nice - once this has been merged.

We'll be glad to have you with us ☺️

I really like the way you went with the replace_users? type of policy method. I also figured out the TODO comment being obsolete now as I read your commit and was happy to see you explain it more 😄. It does feel a bit cleaner to be able to look at the old relationship models in the policy via normal means, just like with any other model. Love it 👍.

Also, it makes sense to provide a separate replace_X? policy method. For example, it is usually desirable to prevent people from replacing all the comments on a Post and only allow adding to them. Having a separate policy method for the replace case seems like a good choice 👍

[matthias-g]

So, I've just pushed the implementations for the missing replace_to_one and remove_to_one.
To avoid inconsistencies I've changed the behavior slightly to calling replace_X? only in case an actual replacement happens. If you replace with null remove_X? is being called.

[valscion]

👍 Good call

[valscion]

Huh, sorry, I hadn't figured out that this is already functionally complete! Sorry 😅

Would you like to try out documenting this in the README somehow?

[matthias-g]

Huh, sorry, I hadn't figured out that this is already functionally complete! Sorry 😅

No problem. I didn't stress that because I was still thinking about how the new policy methods could be useful for the generic update. But I didn't find a good way and I actually think that it is not that important as you can use the specific relationship operations if update? is too permissive.

Would you like to try out documenting this in the README somehow?

Sure. I've just added a paragraph to the README. What do you think about it?

[valscion]

This is looking super great, thank you! We'll look at this PR with @lime soon and reply back to you and maybe even merge this as it is 😊

[valscion]

All in all this looks very good and we really appreciate your effort! We have some ideas on what we'd like to see here. I'll create a PR against your PR based on our review to get this shipped ☺️

[valscion]

Here's some suggestions we came up with @lime as we reviewed this PR. What do you think?

• Add some helping comments about the fallbacking to update? in specs
  • Right now it isn't very obvious why these cases don't have the policy method defined
  • Maybe just a comment like this above each of the test cases could do?

    context 'where remove_<type>? not defined' do
      # CommentPolicy does not define #remove_article? method, so test using it
      let(:source_record) { comments(:comment_1) }
      subject(:method_call) do
        -> { authorizer.remove_to_one_relationship(source_record, :article) }
      end

• The Pundit::NotAuthorizedError is missing its positional arguments:
  1. query parameter, e.g. add_to_comments?
  2. record
  3. policy
  • These are used in the authorization error messages, so they should be set
• README.md could talk about many-to-many relationships instead of M:N
  • People are more familiar with the term many-to-many than M:N, I think

---

EDIT: The things below were originally cluttered in the tasks above, but it might make more sense to focus on these after we've got this PR done ☺️

We were also thinking of doing these, but it might be a better idea to do them at a later point in time. I'm just writing these here for posterity:

• Add tests for every scenario of the relationship operations
  • This is done already in another branch and I'll open up a PR for it once we get this PR done.

Merging this could be the first step to getting to 1.0 release. It might be a good idea to create a "Roadmap to 1.0" issue that documents what needs to be done until we're happy with a major version release. Before that, though, we could release a 1.0.0-alpha.1 version soon-ish. One thing that such an issue would contain would be this:

• The API of DefaultPunditAuthorizer methods could be changed to use keyword arguments
  • Currently the API change is backwards-incompatible in a way that could be very confusing,
    namely as replace_to_one_relationship has the same amount of parameters but are very
    different than what there was before
  • Keyword arguments are supported ever since Ruby 2.0 so we can use them

[gnfisher]

It looks like including relationships in the payload on a POST or PATCH to a resource still just defaults to checking permissions via update? on the associated record?

I have done a bit of work on these based on the work @valscion did in his proto-finer-grained relationships branch that I think can be tweaked to use the same add_to_*, 'replace_andremove_` style policy actions. That would cover all scenarios, I think.

Or are those routes outside the scope of this PR?

EDIT:

Just another nitpick.

The remove_to_one_relationship callback doesn't pass the record to the policy method. In my apps situation, where a Group has a Parent Group, it would be ideal to have access to the record being removed as well, so I can check the user has permission to modify that parent, too.

[gnfisher]

I am also curious what is thought to be a better way of handling has_many related records: sending them as an array to the relationship method (like in this PR) or iterating through the array of records in the DefaultPunditAuthorizer class and passing one record at a time to the relationship method?

Basically, add_to_users?(users) or add_to_users?(user)?

[matthias-g]

Thanks for your constructive feedback, @valscion, @lime!

Add some helping comments about the fallbacking to update? in specs

Comments added. ✔️

The Pundit::NotAuthorizedError is missing its positional arguments

Thanks, I had overlooked that. ✔️

README.md could talk about many-to-many relationships instead of M:N

It does now. ✔️

[matthias-g]

@gnfisher Thanks for reviewing and providing feedback as well!

It looks like including relationships in the payload on a POST or PATCH to a resource still just defaults to checking permissions via update? on the associated record?

Yes, that is true. And I prefer an implementation that uses the relationship policy actions, too.
However I'd suggest to put this in a separate pull request, because this has potential for much discussion (as you can see in #30 😉).

The remove_to_one_relationship callback doesn't pass the record to the policy method. In my apps situation, where a Group has a Parent Group, it would be ideal to have access to the record being removed as well, so I can check the user has permission to modify that parent, too.

This is because the other relationship callbacks don't pass the current related records as well.
Isn't it possible to access the Parent Group through something like record.parent in your GroupPolicy?

Basically, add_to_users?(users) or add_to_users?(user)?

That is a good question. And the current implementation is even inconsistent: It's add_to_comments?(comments) but remove_from_comments?(comment).
Currently I don't see that one option is better. So I decided to copy the behavior of JR. There the inconsistency will be fixed in version 0.9.

[gnfisher]

This is because the other relationship callbacks don't pass the current related records as well.
Isn't it possible to access the Parent Group through something like record.parent in your GroupPolicy?

Of course. 🤦‍♂️ Sorry!

That is a good question. And the current implementation is even inconsistent: It's add_to_comments?(comments) but remove_from_comments?(comment).
Currently I don't see that one option is better. So I decided to copy the behavior of JR. There the inconsistency will be fixed in version 0.9.

Ok. yes, I spoke to Larry in the JR chat about some other topic and I do remember him saying that in the next release it will pass all records versuses going through the processor for every record in the array. Awesome!

[valscion]

Looks great to me, thank you! I will follow-up later with more PRs and the 1.0 roadmap issue until releasing a new version of this gem.

I expect to have time for this next week. We are tracking progress on getting this done on our internal Kanban board at work so this shouldn't be forgotten 😄