API proposal for authorizing relationships

[valscion]

UPDATE on 2016-10-29:

The approach I want to take on authorizing relationships relies on always using a whitelist instead of blacklist. This means that:

• There won't be a generic allow_relationship? method that would handle all relationships.
• Using update? after trying to set a foreign key won't work, as it is a blacklist approach.

So even at the cost of some policies becoming huge, I want to keep each method as small and unambiguous as possible. That means methods named like this:

class CommentPolicy
  def allow_relationship_article?(article)
    # ...
  end
end

I haven't yet made up my mind on which side of the association the authorization should happen, but I'd like that choice to be abstracted away from the AuthorizingProcessor. It could live inside DefaultPunditAuthorizer, if it would make sense, or we could come up with a new place for that logic to live in.

See my comment about this for more details, and to continue the discussion.

Open this to read the old proposal that kicked off the conversation

For a create request that looks like this:

{
  "data": {
    "id": "post-1",
    "type": "posts",
    "relationships": {
      "author": {
        "data": {
          "id": "user-1", "type": "user"
        }
      },
      "comments": {
        "data": [
          { "id": "comment-1", "type": "comments" },
          { "id": "comment-2", "type": "comments" }
        ]
      },
      "tags": {
        "data": [
          { "id": "tag-1", "type": "tags" },
          { "id": "tag-2", "type": "tags" }
        ]
      }
    }
  }
}

We authorize with these methods:

1. PostPolicy.new(post_1, current_user).create?
2. PostPolicy.new(post_1, current_user).associate_with_author(user_1)?
3. PostPolicy.new(post_1, current_user).associate_with_comments([comment_1, comment_2])?
4. PostPolicy.new(post_1, current_user).associate_with_tags([tag_1, tag_2])?

For this request, no matter if it's an update or a create call, we are able to only use PostPolicy to authorize for every association.

I can't see there being any other way as we ​_cannot_​ change the UserPolicy#associate_with_comments call to these without a huge scope creep:

1. CommentPolicy.new(comment_1, user).associate_with_post(post_1)?
2. CommentPolicy.new(comment_2, user).associate_with_post(post_1)?

Who knows, the Comment resource might even call that association with a completely different name than post underneath!

If that request would be an update request, it would only change the PostPolicy#create? call to be PostPolicy#update? and the association checks would remain the same.

I know this goes against Pundit architecture, but I can't really see there being any other way because we aren't able to call post_1.author = author_1 nor post_1.comments = [comment_1, comment_2] before authorizing as it would implicitly save the association immediately. So we aren't able to just use the post_1.author value in PostPolicy#update? to check for authorization, as it isn't set yet.

• has_one association reference —  When are objects saved? (Ruby on Rails Guides)

  When you assign an object to a has_one association, that object is automatically saved (in order to update its foreign key). In addition, any object being replaced is also automatically saved, because its foreign key will change too.

• has_many association reference — When are objects saved? (Ruby on Rails Guides)

  When you assign an object to a has_many association, that object is automatically saved (in order to update its foreign key). If you assign multiple objects in one statement, then they are all saved.

[valscion]

Pinging people that are affected by this: @venuu/core @NuckChorris @aaronbhansen @vevix @DNA @billxinli @GRardB @arcreative @RSSchermer

Please let me know what you think of this proposal! Any input would be greatly appreciated 💞 (even a +1 reaction to the proposal is enough, if you agree with it 😄)

[valscion]

Also @alyssais and @barelyknown from https://github.com/togglepro/pundit-resources if you have had to work around this same issue I'm super interested to hear how you've managed to solve it!

[alyssais]

I don't know off the top of my head what pundit-resources does in this circumstance. It's entirely possible that it handles it incorrectly. I don't know if it's something we've come across.

[NuckChorris]

With this proposal, the authorization can differ by point of view. For example, creating a comment model directly (POST /comments) with a user_id would call CommentPolicy#create?, but if I try to associate a Comment with a User from the User resource (PUT /users/12345 with an entry in the comments association), that calls UserPolicy#associate_with_comments?. These methods are obviously not guaranteed to return the same answer.

If we wanted them to return the same answer, we would probably do something like this:

class UserPolicy
  def associate_with_comments?(comments)
    comment.map { |comment| CommentPolicy.new(comment, user).update? }
  end
end

But that should be really simple to generate, so why do we even write it? Oh, right, because...

I can't see there being any other way as we ​cannot​ change the UserPolicy#associate_with_comments call to these without a huge scope creep:

1. CommentPolicy.new(comment_1, user).associate_with_post?(post_1)
2. CommentPolicy.new(comment_2, user).associate_with_post?(post_1)

Who knows, the Comment resource might even call that association with a completely different name than post underneath!

But it turns out to be quite easy to get the inverse of an association in ActiveRecord: just model.reflect_on_assocation(association).inverse_of.class_name will get us the model of the other side (replace class_name with name to get the name of the inverse association), thanks to their kickass reflection. Sadly, this would lock users into ActiveRecord (not that you can really avoid that in JR) and doesn't even work for polymorphic has_many associations since there is no singular inverse. Well, that sucks.

What if we generated the Policy name based on the type field from the JSON? That would let us get the correct Policy, but how do we then use that associated policy, updating the instance and then calling #update?? We obviously don't know the inverse of the association (unless we use AR reflection), and we can't set them into the parent instance's association to get them updated by Rails. The records should already be loaded into RAM, but we can't modify them using traditional Rails means and we don't know what field (the inverse) to set.

So, what if we just made a best-effort attempt to find the inverse? Attempt to use AR reflection to get the name of the associated record, and otherwise fall back to the less-awesome User#associate_with_comment? if that fails (which can then explicitly defer to whatever Policy you like), so common cases are covered but rare cases are still simple enough.

I guess the code would look something like this (warning: do not use this code without a whitelist on the constantize):

# Assuming 'record' accesses the main record of the request
# And 'policy' accesses that class's policy
# And 'current_user' accesses the current user
relationships_json.each do |key, associated|
  associated_class = associated[:type].classify.constantize
  associated_record = associated_class.find(associated[:id])
  associated_policy = Pundit::PolicyFinder.new(associated_class).policy
  inverse = associated_class.try(:reflect_on_association, key)&.inverse_of
  if inverse.present?
    associated_record[inverse.name] = record
    unless associated_policy.new(associated_record, current_user).update?
      raise 'Not allowed'
    end
  else
    unless policy.new(record, current_user).send("associate_with#{key}?", associated_record)
      raise 'Not allowed'
    end
  end
end

I'm not certain this works for all associations (I just wrote it right now in the GitHub comment box) but I suspect it could be made to cover all cases fairly easily.

My view is that JA should do whatever it can (even if it means adding complexity to JA itself) to enforce a consistent view of authorization, to hew as closely as possible to the existing Pundit techniques, and abstract the request away from the policy. Obviously, this won't be perfect, since json:api has a lot more surface area than the traditional server-rendered apps that Pundit was originally designed for, but I don't think adding more methods is necessary in most cases.

[valscion]

Just asking whether I understood correctly:

1. The associated_record in your case reflects to the author, comment or tag in the original request
2. We'd set the the association foreign key directly on the belongs_to side (e.g. using comment.post_id = 'post-1')
3. Then we'd test for e.g. CommentPolicy#update? to verify the user is authorized to do this

So the way it differs from the current behavior of JA is the step 2 above. For the unfortunate case where we wouldn't be able to query for the inverse assocation, we'd fallback to the associate_with_X method.

How would this translate to the case, where you want to authorize for sending a comment to a post as a normal user? Clearly you wouldn't be authorized to update the post so what would be called in that case?

{
  "data": {
    "id": "comment-1",
    "type": "comments",
    "relationships": {
      "post": {
        "data": {
          "id": "post-1", "type": "post"
        }
      }
    }
  }
}

Would the answer be in this case that we'd somehow set comment.post_id = 'post-1' and then only authorize for CommentPolicy#create? (or CommentPolicy#update?) and stop at that?

---

You're absolutely right in that the current proposal risks having different behavior depending on from which side the association is authorized from 😕. I'm not sure anymore on how much we should try to shield us away from the surprising mutations of ActiveRecord.

What would happen if we'd just rely on transactions always being able to rollback the changes and just handle the authorization in an after hook, thereby allowing us to just use the original create? and update? checks on the origin record policy as they'd now have access to the upcoming situation?

Here would be an example case where users aren't allowed to comment on draft posts. Does this make sense?

class CommentPolicy
  attr_reader :user, :record

  def initialize(user, record)
    @user = user
    @record = record
  end

  def create?
    !record.post.draft?
  end
end

[valscion]

I'm getting quite confused here. Could we try to identify some approaches that are flying around here and in our heads more concisely and give examples on how they'd look like?

Say we have two models, Article and Comment.

class Article < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :article
end

And we have our resources:

class ArticleResource < BaseResource
  has_many :comments
end

class CommentResource < BaseResource
  attribute :body
  has_one :article
end

We have an Article record already available and we'd like to add a comment to it.

POST /comments:

{
  "data": {
    "id": "comment-uuid-1",
    "type": "comments",
    "attributes": { "body": "foobar" },
    "relationships": {
      "article": {
        "data": {
          "id": "article-1",
          "type": "article"
        }
      }
    }
  }
}

(A) Current approach ¹

policy(comment_1).create?
policy(article_1).update?

(B) associate_with_#{association_name}? ²

policy(comment_1).create?
policy(comment_1).associate_with_article?(article_1)

(C) Try (B) but fallback to (A) if method is missing ³

policy(comment_1).create?
if policy_exists?(comment_1, :associate_with_article?)
  policy(comment_1).associate_with_article?(article_1)
else
  policy(article_1).update?
end

(D) Set associations and authorize afterwards ⁴

policy(comment_1).create?

(E) Authorize with (B) on both sides of the association ⁵

Note: The name of article_1.comments << comment_1 operation authorization is not relevant now and can be bikeshed later.

policy(comment_1).create?
policy(comment_1).associate_with_article?(article_1)
# Naming gets a bit difficult for this, let's not bikeshed it here.
policy(article_1).associate_with_comment?(comment_1)

(F) Try (E) but fallback to (A) if methods are missing ⁶

Note: The name of article_1.comments << comment_1 operation authorization is not relevant now and can be bikeshed later.

policy(comment_1).create?
if (
  policy_exists?(comment_1, :associate_with_article?) && 
  policy_exists?(article_1, :associate_with_comment?)
)
  policy(comment_1).associate_with_article?(article_1)
  # Naming gets a bit difficult for this, let's not bikeshed it here.
  policy(article_1).associate_with_comment?(comment_1)
else
  policy(article_1).update?
end

(G) "Association policies" ⁷

association_policy(comment_1, article_1).associate?

---

¹ Approach A causes issues, as even in this example case it doesn't allow users to add comments on any article they can't edit. This is the reason why this issue exists.

² Approach B is what I tried to ask about in the original post.

³ Approach C is an opt-in approach to B where the app developers could use finer grained permissions when the overly restrictive approach A causes them trouble.

⁴ Approach D forces us to use transactions and has a risk of having security holes as it is a whitelist rather than a blacklist approach. You'd need to authorize for every changeable association in CommentPolicy#create?. You most likely also don't use it similarly in your regular controllers if you have normal Rails app alongside your API as well.

⁵ Approach E has the same qualities as B but authorizes for all records such that it wouldn't matter which record is the origin of the request. It also needs to be bikeshed for the authorization method name of article_1.comments << comment_1 operation.

⁶ Approach F is starting to look like a monster but has the same qualities as B but also the mirroring qualities of E.

⁷ Approach G has a good part that it is a single source of truth. The bad part is naming — how on earth would we create a consistent naming convention? ArticleAndCommentPolicy? CommentAndArticlePolicy? ArticleCommentAssociationPolicy?!?? 💀

---

Is there any other approaches besides these that you were thinking of @NuckChorris ?

[valscion]

We could opt-in to the approach G above and not force any naming convention to the users by letting them supply the association policy class name themselves.

class ArticleResource < BaseResource
  has_many :comments

  authorize_relationship :comments, with: 'ArticleCommentAssociationPolicy'
end

class CommentResource < BaseResource
  has_one :article

  authorize_relationship :article, with: 'ArticleCommentAssociationPolicy'
end

class ArticleCommentAssociationPolicy
  def initialize(user, article:, comment:)
    @user = user
    @article = article
    @comment = comment
  end

  def associate?
    ArticlePolicy.new(@user, @article).show?
  end
end

1. Forcing usage of keyword arguments in association policy class allows us to sanity check their names and remove the need of deciding on whether article or comment should come first.

2. Developers can can opt-out of supplying authorize_relationship in a resource, in which case the strict, current approach takes hold.

3. We can raise helpful errors when the association policy classes don't match between both sides of the relationship

4. We can allow the association policy classes to differ by opting in to that logic, too:

   class CommentResource < BaseResource
     has_one :article
   
     authorize_relationship :article,
                            with: 'DifferentlyNamedCommentArticleAssociationPolicy',
                            allow_different_policy_class: true
   end

[aaronbhansen]

Going back to my original ticket, what I was hoping to do is have some simple permission checking to let me have a hook into creating the association instead of calling update? on the related model. I think the solutions above are trying to solve this, but add a lot of extra methods and complexity.

I would rather have most the examples above reversed. Taking option C above as an example

policy(comment_1).create?
if policy_exists?(comment_1, :associate_with_article?)
  policy(comment_1).associate_with_article?(article_1)
else
  policy(article_1).update?
end

I feel like it makes more sense to reverse how you authorize the creation of a relationship. In the example above, if you have a single relationship point, an article has multiple comments, the example makes sense. But when you have something like a user object that is on multiple relationships (article, comment, tag, etc), you know have to implement that same check over and over on all the child policies.

My original idea was to do something like this.

policy(comment_1).create?
policy(article_1).allow_relationship?(comment_1)

While its similar to the example above, I think having the parent check the relationship makes the most sense. What if I had a user object on the article and comment, then I have to implement the same method on each child policy.

policy(comment_1).associate_with_user?(user_1)
policy(article_1).associate_with_user?(user_1)
policy(tag_1).associate_with_user?(user_1)

Where in all the examples I have locally, I know from the user object if that association is allowed.

policy(user_1).allow_relationship?(comment_1)
policy(user_1).allow_relationship?(article_1)
policy(user_1).allow_relationship?(tag_1)

One parent association point to check all the related classes. And if you had a base pundit class, you could still default allow_relationship? back to update? until the method is implemented.

Maybe others can better define more use cases, but for me the state of the parent object usually defined if I allowed child objects to be related. If the user wasn't a Author for example, then they shouldn't be able to create an article or tag. If they are a user or an author then they can create comments.

You could allow a cascading type of check

# Check if a specific model hook was created
if policy_exists?(user_1, :allow_relationship_comment?)
  policy(user_1).allow_relationship_comment?(comment_1)
else
  policy(user_1).allow_relationship?(comment_1)
end

# internally allow_relationship on the base template defaults back to the current update?
def allow_relationship?(relationship)
   policy(relationship).update?
end

Mostly I want to avoid trying to implement the same policy over 7 different models, when I know from the parent relationship if that relationship can be created. The user object is even easier to show this as you could just check if the user_id on the association is the same as the current user and then allow the model.

# user policy
def allow_relationship?(relationship)
   # they are allowed to create as an admin
   if @user.is_admin?
     return true
   end 

   # otherwise, ensure the relationship user is equal to the current user logged in
   if relationship.user != @user
      return false
   end 

   # Otherwise make sure the current user isn't disabled
   if @user.disabled?
      return false
   end

   true
end

[lime]

While its similar to the example above, I think having the parent check the relationship makes the most sense.

Could you clarify what you mean by "parent" here? It's not immediately obvious to me.

Is it possible to, without knowing the business logic of the app, look at a one-to-one relationship between Foo and Bar and deduce which one is the parent? One-to-many? Many-to-many?