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?

[aaronbhansen]

On a one-to-many, a has-one, or many-to-many you can always go back to the the belongs-to relationship reference. If the model has a belongs-to relationship, it is a child of a parent.

In the one-to-many example, the parent would be the one who has the relationship you're trying to add.

User 
  email
Article
  text
  user (parent)

A user has many articles. Article belongs to a user (parent). Check on the User policy if the relationship is allowed.

In the has one example, you still can know the parent.

Product
  name
  (has-one product_details)
Product Details
  description
  product (belongs-to product)

A product has one product_details, a product_details belongs to one product (parent).

I think its easiest to just give responsibility to the product and have it decide if the product_detail is allowed to exist as a relationship. Since the has-one relationship is a relationship pointer anyways with no FK's on product.

In the many-to-many example, it seems more complex as there isn't a model with a belongs to represented in code, but there still is a belongs to relationship implied. If its a has-many-and-belongs-to-many with just a mapping table and no model representing the intermediate table, there is still a database table that has a belongs to relationship to both sides.

User
UsersCompanies
Company

A user has many companies, and a company has many users. The users_companies table isn't represented as a model in rails, but the users_companies still has a belongs-to to a user and a belongs-to to a company. Since the users_companies tables isn't represented in rails code as a model, you would need to check the user policy if the company relationship can be added, and the inverse, check the company policy if the user relationship can be added.

I also think that many-to-many tables are the exception, and most fall under has-many, and belongs-to.

If the users / companies many-to-many was a has-many-through relationship, then when creating the has-many-through relationship, its more clear that you should still check the parents.

You create a new users_company relationship and it has

UsersCompanies
   user
   company
   role
   active

In this situation a belongs to exists as a relationship on the model in rails. User has many users-companies, and users-companies belongs to user.

All it comes down to is, anytime you are creating a new record, if it has a belongs-to FK, that record is a child of the parent it belongs to. Go check if the child can be created from the parent it references.

If you take the example of an e-commerce ordering system. You have the following tables laid out, you have two ways of checking if that association is allowed. One you check the parent, or two you check on each child. Let me lay out both.

The table relationships

Order
  (has many) order_lines

Order Line
   order_id (belongs to)
   (has many) order_selections

Order Selection
   order_id (belongs to)
   order_line_id (belongs to)
   (has many) order_selection_questions 

Order Selection Questions
   order_id (belongs to)
   order_line_id (belongs to)
   order_selection_id (belongs_to)

In the original examples, if you wanted to check if you could create child objects under the parents you would need to add the following methods.

Order Policy
  (none)

Order Lines Policy 
  associate_with_order?

Order Selection Policy
  associate_with_order? (duplicate)
  associate_with_order_line?

Order Selection Question Policy
  associate_with_order? (duplicate)
  associate_with_order_line? (duplicate)
  associate_with_order_selection?

As you can see from that example, as you have nested tables, you have to add the same methods over and over. If you decided to change whats allowed, you need to make sure each policy method is updated to ensure the same results. If you had the parent take responsibility for checking.

Order Policy
  allow_association? (check if order_id reference is allowed)

Order Lines Policy 
  allow_association? (check if order_line_id reference is allowed)

Order Selection Policy
  allow_association? (check if order_selection_id reference is allowed)

Order Selection Question Policy
  (no children, no association checks)

In this example, you have 3 methods which cover any child being added. Instead of re-implementing the same method up to 3 times in the previous example.

Hopefully that clarifies my thought process on why I think the parent should be responsible for checking authorization of a child.

[NuckChorris]

Given the same scenario as in #30 (comment), my proposal (#30 (comment)) would look like this, without the downside listed in (D) of requiring transactions:

policy(comment_1).create?

CommentPolicy merely needs to validate its belongs_to associations. Which, for most models, will be one. No trouble there.

So let's try a more complex scenario. Given the following models (the resources should be fairly obvious):

class Article
  has_many :comments, as: 'topic'
end
class Review
  has_many :comments, as: 'topic'
end
class Comment
  belongs_to :topic, polymorphic: true
end

And this request

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

1. Grab any has_many associations
2. Find policy via type attribute in JSON
3. Attempt to find inverse via AR Reflection. If this fails, stop and call Review#associate_with_comments? or some other fallback method on the Review model. Or maybe call it on the other side? I dunno, we can discuss the most logical fallback. If you find the inverse, continue with ⬇️
4. Load associated records
5. Fill the parent record's ID into the associated record's inverse field (which is usually the Rails default of #{model}_id
6. Run 'em each through CommentPolicy#update?

This way, the default is to just ask the model declaring belongs_to (the child, the side that has the foreign key) to authorize an #update? to their foreign key. Everything should just work™ when you're using ActiveRecord.

When you aren't using ActiveRecord (or if AR Reflection fails us, though you should declare an inverse_of in that case), we just avoid the whole setting-and-calling thing because there's a risk of mutating something and we don't want to accidentally do that. In that case, we would just call a fallback method which authorizes the association specifically. Now, that fallback method might even just call on another policy's #update? method, but it's not a linkage we can automatically determine, so we have to make it explicit.

[NuckChorris]

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.

This seems backwards. The User shouldn't know whether they can create a Comment, the Comment should know whether it can be created by a User. Otherwise, when User can create 700 things, the UserPolicy is gonna be huge while the CommentPolicy is 5LOC

If you want to defer to the User to determine this, it should be explicit, because you're very much going against the grain on separation.

[aaronbhansen]

This seems backwards. The User shouldn't know whether they can create a Comment, the Comment should know whether it can be created by a User. Otherwise, when User can create 700 things, the UserPolicy is gonna be huge while the CommentPolicy is 5LOC

If you want to defer to the User to determine this, it should be explicit, because you're very much going against the grain on separation.

I disagree. Why would the user, article, etc not know if you can add a relationship.

If the article isn't published, then you shouldn't be able to publish a component. That is a restriction of the article, not the comment. The comment shouldn't check itself for article permissions, the article should be the one that gives permission to the comment to publish.

If a user wants to publish a comment, and the user isn't active, the comment shouldn't check itself for user permissions, the user should know if its in a valid state to allow that comment through.

My point was, if you have a user on 4 different tables, Article, Review, Comment, History, etc and you want to perform the same user checks on each one, why write the same code over and over. The user policy could have overrides as I specified for specific relationships, but there is a base permission check that is shared when adding any relationship. A modified example:

# User Policy
def allow_relationship_article?(article)
  unless @user.is_publisher? || @user.admin?
    return false
  end

  allow_relationship?(article)
end

def allow_relationship?(related_model)
    if @user.admin?
      return true
    end

    if @user.id != related_model.user_id
      return false
    end   

    unless @user.active?
      return false
    end

    true
end

Why would you want to have those same checks on each of the child policies that have a user relationship instead of going to the parent (user) to do some global checks? You have the same permissions you need to check thats global for any relationship being added as a user belongs-to.

• If the current user is an admin, let it pass
• If the user relation doesn't match the current user, don't let any child associations be created. Not an admin role, so stop.
• If the user isn't active, don't let any child associations be created. Needs to be an activated user to perform actions.

Then you could add specific overrides for child relationships like article.

• If the user isn't a publisher, they don't have the rights to publish. Only admins and publishers can create articles

[NuckChorris]

if you have a user on 4 different tables, Article, Review, Comment, History, etc and you want to perform the same user checks on each one

Because it's an action on the Comment, not on the Article. The Article doesn't necessarily have a has_many, but the Comment definitely has a belongs_to.

You're suggesting that the User should have responsibility for checking whether a Comment can be created? That's patently absurd.

If you want to share the code, don't just try and shove the responsibility onto the wrong object — create a generic method on the User such as User#active?, and apply that in each CommentPolicy, ArticlePolicy, etc.

It's one line, and it's 100% logical to do si.

If the user isn't active, don't let any child associations be created. Needs to be an activated user to perform actions.

This is probably not a good idea in any situation, because suddenly when you add another model named Setting that belongs to User, you're gonna go "wait why can't I create this?" and then realize that your SettingPolicy doesn't control the case of setting a Setting... the UserPolicy does. Why? Because you put that task in the hands of the wrong object!

If you put it in the correct object (the SettingPolicy), it would be easy: the CommentPolicy and ArticlePolicy have this:

def update?
  return false unless record.user.active?
end

And then SettingsPolicy has this:

def update?
  !!current_user
end

Super simple, it's very clear what's going on, and there's no possibility of confusion by POV.

[aaronbhansen]

You're suggesting that the User should have responsibility for checking whether a Comment can be created? That's patently absurd.

No, I'm suggesting that there are still permission that need to be checked if you can create a comment, and that is contained entirely within the scope of the CommentPolicy.create?, except for relationships. If you want to associate a comment with a user, the UserPolicy should take responsibility for allowing that relationship or not.

It works for any situation, has-many, has-one, many to many. Its a relational operation, everything has a relation and a responsibility. You're saying that with a parent / child relation, you should let the children decide to do whatever they want? If the Parent has no responsibility at that point, why is it a parent relationship.

From a security stand point, adding security on the child models in an opt-in process, you have to ensure that you opt-in to any policy relationship permission. You must create relationship permission checks on each policy you create. If you forgot to opt-int you now have created a security hole.

If the parent is responsible, its an opt-out process. Everything has the same security requirements unless you specifically opt-out.

Yes, you could separate the concerns into a module and include it on each child policy, but if you forget to add a association permission on a new policy you now are defaulting to the parents update? permission anyway. So it goes back to the parent by default, why not start and maintain relationship security checks on the parent.

Even from your own comment, you spell out what I'm trying to avoid.

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?,

Yes that is correct, CommentPolicy.create? is called when trying to create a comment, but also UserPolicy.update? because of the user relationship, which is the problem as it exists today.

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.

Exactly one of the problems I'm trying to avoid. Rather than re-implement the same methods over and over with possibly differences, have a single source of truth. Continuing on:

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

The rest of your solution is to try and fall back to the parent relationship dynamically and check if update? is allowed. Your whole comment is what I'm suggesting except instead of calling update?, which is the entirely the wrong method to check in the first place, call an association permission check.

Why is calling the parent relationship policy update? permission the entirely wrong method to check. It's because the current user may not have access to update? an Article, but they can add an associated comment to that article, hence the allow_relationship? check on article. Why would you default back to update? when its clearly the wrong solution. Updating an article and creating a relationship are two different concerns.

Even your last comment aren't even targeting the right security settings in your example.

If you put it in the correct object (the SettingPolicy), it would be easy: the CommentPolicy and ArticlePolicy have this:

# CommentPolicy, ArticlePolicy
def update?
  return false unless record.user.active?
end

From your previous comments, you said that the CommentPolicy or ArticlePolicy should check back to the inverse relationship and check the update? permission, and if the inverse can't be found then check the association method. Your CommentPolicy and ArticlePolicy don't even make sense. If you are creating a new Comment as JSON authorization stands today, it would check the comment policy for create? and then would call ArticlePoilcy for update? You aren't checking the article policies user to see if that user is active to create a comment, you need to check your own comments user relationship. It doesn't matter about the articles user, only thing that matters is your user.

The baseline change that needs to happen is to try and avoided calling update? on a parent relation when creating a child model. There is a clear difference between being able to update? an article and being able to create a relationship off of article.

[NuckChorris]

Our proposals are quite opposite, in fact: you're proposing the association be handled by the associated side while I propose it's handled on the side with the foreign key.

My proposal is merely that a foreign key is naught but another attribute to check — if you want to build your solution as an abstraction on top of that, be my guest.

The simplest and most Pundit-like API should be the one used by default in JA, if you ask me. And that is, quite clearly, my solution.

Associations are not some magical thing which needs to be treated differently. I understand that you feel that the parent is the real decision-maker here (and I don't disagree, generally I defer to a method on the parent when checking this), but this risks ballooning the parent's concerns to be huge, and leaving the child completely emaciated.

I mean, what happens when you have seven different has_many associations on a User? Suddenly you have seven codepaths in that policy and none in the child policies.

Let's not talk about knowledge (because we both agree that's in the User model), let's talk about responsibility. When a User wants to create a Comment, is the User model responsible for validating that Comment?

Hell no it isn't, so why should it be responsible for checking if the Comment is doable, just because it happens that the authorization for a Comment calls on the User? By that logic maybe I should just straight take it to the Role model, since the User model is just gonna call on the Role model anyways. It's abundantly clear that this approach does not scale beyond a simple project.

And what happens when you have a has_many :through? There's now two parents, so which one gets called? Both? None? Who even knows! With my approach, you would call on the :through model because it's the child of both. It solves all relationships except HABTM cleanly. HABTM will need to be a special case in pretty much any scenario.

The entire point of Pundit is to say "given X object, can Y user do Z?", and you're throwing that out the window because you want to simplify your own solution (which honestly doesn't even make sense, because you would be authorizing the current_user, not the associated user!)

[aaronbhansen]

The simplest and most Pundit-like API should be the one used by default in JA, if you ask me. And that is, quite clearly, my solution.

Pundit works well in the non api world, but not in an api one. You can do so many more things through an api call. When its not a structured json api controller, you have specific methods for specific operations which pundit can account for. create_user, make_admin, etc, etc. Each having its own policy authorization.

Associations are not some magical thing which needs to be treated differently. I understand that you feel that the parent is the real decision-maker here (and I don't disagree, generally I defer to a method on the parent when checking this), but this risks ballooning the parent's concerns to be huge, and leaving the child completely emaciated.

This is wrong, they do need to be specifically checked. What if you call the api with curl and pass in a different user_id. You now have created an object for a different user.

I mean, what happens when you have seven different has_many associations on a User? Suddenly you have seven codepaths in that policy and none in the child policies.

This is a total valid authorization. You need to call the parent for each of those children. This doesn't mean you'll have zero responsibilities in child policies as I show below.

And what happens when you have a has_many :through? There's now two parents, so which one gets called? Both? None? Who even knows! With my approach, you would call on the :through model because it's the child of both. It solves all relationships except HABTM cleanly. HABTM will need to be a special case in pretty much any scenario.

As i explained earlier, there is always a has_many_through table, something still belongs to something. The has_many table has two belongs-to references to both parent tables.

Json authorization has two responsibilities it needs to handle, I'll try to make them both abundantly clear.

1. There is the responsibility for viewing/ creating / updating / deleting objects
2. There is the responsibility for authorizing relationships.

Without one of those responsibilities, its half an authorization system.

You have a User Object with 3 roles Admin, Publisher, User

You have an Articles, Comment, and Users table.

Lets create the three policies and point out where is responsibility needs to happen.

#ArticlesPolicy
def create? / update?
   # responsibility 1, what can a user create / update
   if @user.is_admin? || @user.is_publisher?
      true
   end
   false
end

def allow_relationship(related)
   # responsibility 2, can a user associate their create/ update with this
   if @model.published?
     true
   end 

   false
end

#CommentsPolicy
def create?
  # responsibility 1, anyone logged in can create a comment
  current_user.present?
end

def update?
   # responsibility 1, if admin, allow updating
   if @user.is_admin?
     return true
   end

   # make sure the user updating the comment, created it
   if @current_user.id == @record.user_id
     return true
   end 

   false
end 

def allow_relationship(related)
  # responsibility 2, nothing else is a child of comment, lets default to false for now
  false
end

UserPolicy
def create? / update?
   # responsibility 1, if admin, allow creating / updating
   if @user.is_admin?
     true
   end

   # responsibility 1, check non relationship properties
   # make sure they didn't try to make themselves an admin
   if @record.is_user?
     true
   end 

   false
end

def allow_relationship(related)
   # responsibility 2, if admin, allow association
   if @user.is_admin?  
     true
   end

   # responsibility 2, if user matches, allow association
   if @current_user.id == related.id
     true
   end

   false 
end

Now that we've defined the policies, lets play through some scenarios.

Scenarios:

1. User with the role of user, wants to create an article.
   • They do a post to /articles, responsibility 1 (ArticlePolicy.create?) stops them. Authorization failed because the user isn't an admin or publisher.
2. User with a role of user, wants to create a comment
   • The do a post to /comments, and responsibility 1 (CommentPolicy.create?) says they can create a comment.
   • Responsibility 2 (UserPolicy.allow_relationship?), checks to ensure they can associate that comment with the user_id passed in. It is allowed, comment is created.
3. User with a role of user, wants to create a comment for User X. Say the user X said something they didn't
   • The do a post to /comments, and responsibility 1 (CommentPolicy.create?) says they can create a comment.
   • Responsibility 2 (UserPolicy.allow_relationship?), checks to ensure they can associate that comment with the user_id passed in. The user isn't an admin and the user doesn't match the current user. Authorization failed because the User can't post as someone else.
4. An admin wants to post an article on behalf of user Y because they don't have internet access
   • Admin does a post to /articles, and responsibility 1 (ArticlePolicy.create?) says they can create an article because they are an admin.
   • Responsibility 2 (UserPolicy.allow_relationship?) is called, and the authorization is allowed since the user making the authorization call is an admin.

In your example, you are totally missing Responsibility 2. Checking relationships is as much of a authorization as Responsibility 1

[NuckChorris]

Pundit works well in the non api world, but not in an api one. You can do so many more things through an api call. When its not a structured json api controller, you have specific methods for specific operations which pundit can account for. create_user, make_admin, etc, etc. Each having its own policy authorization.

Actually, no. Pundit works great in the API world too, as long as you're not trying to find a problem for your solution.

To suggest that Pundit policies should be (or were) traditionally coupled with the controller is absurd. That breaks separation of concerns on so many levels. The job of a policy is to say "Can X do Y to Z?" which does not take context into account. If your policies have special cases like that, you're using it wrong.

Pundit handles CRUD perfectly, it's really 👌 and there's no reason for JA to be different, except that it is a little tough to automatically find which policy should be validating an association, which in traditional Pundit would be exactly as I suggest — the child model!

I proposed a solution to this: we attempt to find the child side of any has_many or has_many :through association, and then do some trickery in AR to get things into a consistent state before we check the policy, and if that fails we fallback to a less-awesome API. It fails safe, and it leaves the Comment to validate the Comment, not the User to validate the Comment.

This is wrong, they do need to be specifically checked. What if you call the api with curl and pass in a different user_id. You now have created an object for a different user.

def update?
  return false unless user == record.user
end
alias-method :create?, :update?

As i explained earlier, there is always a has_many_through table, something still belongs to something. The has_many table has two belongs-to references to both parent tables.

Yes, there's a clear child and two clear parents. You don't answer which parent gets called to validate the child, and that's a very serious problem. Do both parents get called? How do we rig that? Why? Why not use my solution?

Json authorization has two responsibilities it needs to handle, I'll try to make them both abundantly clear.

1. There is the responsibility for viewing/ creating / updating / deleting objects
2. There is the responsibility for authorizing relationships.

And I keep saying these are one and the same. Associations are naught but attributes, and any differences should be hidden from the user.

Where do you even draw the line, you're calling @user in your examples and that's okay but calling up onto the associations is not?

And no, this isn't the role of JA. JA exists to hook Pundit with JR, and nothing more. We should not be opinionated about how you structure your Pundit stuff. If you want to build it this weird way that you want to, you have every right to call out to the other objects or even build your own abstraction. JA's job is to try and fit JR's operations onto Pundit authorizations as accurately as possible

But Pundit handles everything we need just fine.

You have a User Object with 3 roles Admin, Publisher, User

You have an Articles, Comment, and Users table.

Lets create the three policies and point out where is responsibility needs to happen.

def allow_relationship(related)
   # responsibility 2, if admin, allow association
   if @user.is_admin?  
     true
   end
   # responsibility 2, if user matches, allow association
   if @current_user.id == related.id
     true
   end
   false 
end

This is not a reasonable solution. What happens when you have 7 child associations on User? Because we already have that on Hummingbird. And they all need different logic. Yeah, doesn't work too well.

So we would need something like this:

def allow_relationship(related)
  case related
  when Comment then !(record.user.banned? && record.type == 'comment')
  # ...
  end

And so on. For 7 different associations. In a fucking case statement. No. Just no. This is not a good solution, at all.

Meanwhile, in my solution:

class CommentPolicy
  def create?
    return false if record.user.banned? && record.type == 'comment'
  end
  alias_method :update?, :create?
end

Because, as it turns out, updating a foreign key isn't special or magical. It's just normal.

[aaronbhansen]

At this point we aren't to convince the other. We are also the only ones commenting, so the point is somewhat mute. It's not worth discussing further. I think @valscion sound pick a solution and move forward.

If he does what I suggested in my original ticket, separate pundit and the authorization methods into an abstract class, then that can be set in the config for JSON Api Authorization like many of the services in JSON Resources, anyone can implement their own services.

It would be great to see Pundit and the authorization methods put behind an abstract service that can be configured and changed in your projects config file. Pick a solution from here and implement it as the default. It would be even nicer to have the pundit requirement be optional so you could install only the configurations you wanted, one for the generic authorization service, and then the authorization implementation.

[lime]

Well, that was some unpleasant escalation to wake up to. 😕

People, it should be perfectly possible to discuss the pros and cons of any given solution, without being disrespectful to your peers. This kind of aggressive back-and-forth only serves to muddy the waters, and takes us further away from finding a solution.

I encourage everyone to take a deep breath, and read through the proposals once more. They all have shortcomings, that's why we have this discussion. Rather than defending your own solution with tooth and nail, see if you can find something you like among the rest.

[aaronbhansen]

@lime I agree. I was merely trying to explain a custom built system that we already have in place, but doesn't exist in JSON API Authorizations today. It probably has shortcomings, but we haven't run into them yet. It provides clear testing of permissions and full sanitization. It provides not only operation level permissions (create / update / view, etc), but attribute and relational sanitization. We we love to use it in JSON API Authorizations, but the abstraction to do so don't exist. I was trying to share our domain knowledge in how we fixed the current problem, which is what this ticket is about.

Again, I don't care what solution is picked, my original ticket was to make things abstract enough in JSON Api Authorizations so we could have our own security implementations. I was only trying to fully explain how our custom implementation worked in production today through examples.

Our working process is, sanitize absolutely as much data as you can when you receive an input operation (create, update, delete), then when you receive an output operation (view / index), you can trust your existing data and have very minimal output sanitization. Since input operations are only a minor portion of your sites operations in terms of processing power / requests, it makes sense to do as much work on input as possible, and minimize work on output since viewing data is far more common. Maximize server resources for viewing by trusting that your input was valid. I'm completely fine if others don't agree that there are different levels of security that need to happen (permission, attribute, and relational).

[rob-mcgrail]

Can I confirm that there still isn't any way to create a relationship with a resource unless you're also allowed to update the target of the relationship?

That seems like an essential feature, that must come up in every non-trivial application. Could we not just have an option to skip the update? check on certain relationships?

[valscion]

Yeah, unfortunately such an option is not yet implemented.

Could we not just have an option to skip the update? check on certain relationships?

The problem comes from what does "just have an option to skip the update" look like, and that it hasn't yet been implemented. This is precisely what we have been trying to discuss in this issue :)

[rob-mcgrail]

Yes I went to patch it myself, and can see that it's not that simple. Necessary, but non-obvious how to make it nicely configurable.

For my own purposes I've just added this to my initializers:

module JSONAPI
  module Authorization
    class DefaultPunditAuthorizer
      def create_resource(source_class, related_records)
        ::Pundit.authorize(user, source_class, 'create?')

        related_records.each do |record|
          ::Pundit.authorize(user, record, 'relate?')
        end
      end
    end
  end
end

And in application_policy.rb:

  def relate?
    update?
  end

And then in the policy of the resource where it's become an issue for me:

  def relate?
    true
  end

But that's no kind of solution. Adding it here in case someone really lazy just wants to sidestep that particular validation in a hurry.

[valscion]

That's definitely an improvement over just creating a custom class where no authorization is ran for the related records.

Did you see that you can actually create a completely custom authorizer class for your purposes and configure jsonapi-authorization in your application to use it?

require 'jsonapi-authorization'

class MyCustomAuthorizer < JSONAPI::Authorization::DefaultPunditAuthorizer
  def create_resource(source_class, related_records)
    ::Pundit.authorize(user, source_class, 'create?')

    related_records.each do |record|
      ::Pundit.authorize(user, record, 'relate?')
    end
  end
end

# ...and in an initializer, do this:

JSONAPI::Authorization.configure do |config|
  config.authorizer = MyCustomAuthorizer
end

[valscion]

I'm taking a stab at testing out different approaches to these suggestions in here.

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

• There won't be a generic allow_relationship? method.
• Using only update? will not be enough

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 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...

[valscion]

I just updated this issue's description to highlight the direction I'd like this to go.

Resolving this issue is not a high priority for our team yet, as our API is currently only used internally, and we're perfectly fine with just authorizing for update? always. If some of you are blocked by this issue, I'd be happy to help a bit with guiding your collaboration for this feature into jsonapi-authorization.

The principle of JA is basically: Prefer being overly restrictive rather than too permissive by accident. What follows is that we want to have:

1. Whitelist over blacklist -approach for authorization
2. Fall back on a more strict authorization

[matthias-g]

I've thought about this issue and I've come up with a proposal that looks (too?) simple. For easier discussion of the changes I've submitted a PR (#40) instead of posting the code here.

[gnfisher]

Ok so, after thinking this over, here are my thoughts on this.

The JSONAPI spec basically treats relationships as resources themselves. This is why we get into trouble with Pundit coverage for those pesky associations. Usually, in a standard rails app the manipulating of those relationships is just within a controller action that we can authorize for. Here, they are WITHOUT. If that makes sense...

I feel like Solution (G) from the above is the best route to go.

• Separate policies for every relationship, i.e. ArticleCommentPolicy
• Alphabetical order, camelcased of class names in relationship.
• They might look something like this

class ArticleCommentPolicy
  attr_reader :user, :article, :comment

  def initialize(user, article, comment)
    @user = user
    @article = article
    @comemnt = comment
  end

  def create
    # logic..
  end
  def destroy
    # logic..
  end
end

Probably would need a scope as well, but whatever. These seem like the only two actions we need on them. For PATCH requests, you would pass in the OLD related records through #destroy then the NEW related records through CREATE.

As far as I know, the entire request is wrapped in a transaction by JR, so if auth fails at some point everything will rollback and an error returned to the client.

I have put together a working version of (C) in our application but I dislike immensely having TWO separate policies to check the same relationship. It's just asking for trouble eventually. I think I will try to put something together on this to see how it works out. I'll report back.

[rob-mcgrail]

This makes a lot of sense to me - authorising associations correctly is important enough (and, often, confusing enough) that giving it it's own policy file seems appropriate.

[arcreative]

Begrudgingly, I think I like it too. "Begrudgingly" because 1000 policies, but "like" because it does seem to give the requisite flexibility. I'm currently in nightmare land where I'm throwing Pundit exceptions in the resource because I just can't get access to context or params, which is needed in a lot of cases for my application.

The only problem I see is that you would still need the inverse policy as well...right? If you have Post and Comment, wouldn't you need both PostCommentPolicy as well as CommentPostPolicy? Who's your daddy and what does he do? 😆

[gnfisher]

Regarding a dual policy, its one relationship, so one policy ought to cover it fine. Its just a POV difference from one model to the other.

[rob-mcgrail]

Yeah presumably it would depend how they were defined?

module V1
  class CatResource < BaseResource
       relationship :mouse, to: :one

would imply a CatMousePolicy

But you wouldn't want a MouseCatPolicy until you do:

module V1
  class MouseResource < BaseResource
       relationship :cat, to: :one

[arcreative]

Seems good to me then. A problem I'm running into just today is regarding a model where creation requires that either the sender or recipient is the current user, but not both, and not neither. The both can be handled by a validator, but I'm having trouble deciding where to do the rest of it. Lack of access to context somewhat necessitates that it's done from the resource, but I keep running into edge (but all too often) where Pundit's opinions are too restrictive.

In my case, the relationships don't only need to know about the two records, but also about each other, which opens up a whole new can of worms. I can chalk this up to being uncommon/unorthodox, but I would love to have a solution that just feels beautiful like [most of] the rest of Rails.

[valscion]

Yes, I think that option G does seem to be the most powerful option that allows any abstraction to be built on top of it.

I think we'll want to test out how G would look like in a real-world application and what challenges it entails. I also envision that as it is such a new concept compared to the basic pundit classes, we'd want to make it opt-in with a configuration option -- similar to how you can now override the class used for authorization. Or we could make it configurable on the resource level, too.

This would give us hopefully the best of both worlds -- the default authorizer gets you up and running quickly and safely while the relationship policy classes would allow you to consolidate your relationship authorizations to one, conventionally named pundit-ish class.

This way you wouldn't have to define a huge number of new policy classes unless you think it would clean up your application.

In our application for instance, we don't allow a relationship to be mutated from both sides of the relation. This way we'll likely only need to authorize those relationship changes only in one policy class instead of two.

I feel that the best way to go forward will be for me and @lime to draft a roadmap issue soon to open up a discussion of what the future could look like and to allow comments to be consolidated in one place

[matthias-g]

I like the idea behind solution (G) to interpret relationships as resources themselves.
However it's not clear to me how a good naming of these policies would look like.

What if we have a setup with two relationships that relate the same classes?

class Concert < ActiveRecord::Base
  has_many :attendees, class_name: 'User'
  has_many :performers, class_name: 'User'
end

class User < ActiveRecord::Base
  has_many :concerts_as_attendee, class_name: 'Concert'
  has_many :concerts_as_performer, class_name: 'Concert'
end

(An alternative would be to use a single relationship like has_many :users, through: attendances with a boolean Attendance.as_performer. But this wouldn't allow for API requests like POST /concerts/1/relationships/attendees. Instead you would need to model the relationship as a resource in the API: POST /attendances
In the API I prefer resources to be resources and relationships to be relationships. 😉)

Would the policies be called ConcertAttendeePolicy and ConcertPerformerPolicy? How would they be found for the other direction?

[valscion]

That's a very good question 🤔 . Definitely a use case worth considering before committing to any particular direction.

[gnfisher]

It seems like we should be able to figure things out via reflection on the models, then build the class name, regardless of which end of the relationship we're doing the calculations.

However I sympathize a lot with the idea @valscion put forth about this being a sort of opt-in, more complex layer. I think @matthias-g covers most things well enough, and I've been able to do something similar in POST and PATCH routes, just hitting those same policy actions (add/replace/remove)... the caveat being making sure policies covering the same relationship match.

I'm going to spend the weekend thinking about it!

[valscion]

Sound like a good idea! Do you think it would be more clear to discuss the G case specifically in a new issue? I'm afraid that this issue might get cluttered if we want to dive deeper to one solution, and having a new issue might allow us to have more constructive dialogue ☺️

[matthias-g]

I have put together a working version of (C) in our application but I dislike immensely having TWO separate policies to check the same relationship. It's just asking for trouble eventually.

I just want to bring in a way to handle this with the current approach of JA. It's rather a design pattern than a clear solution.
Instead of explaining it I'll show you an example for the scenario that I mentioned above:

policies
├── application_policy.rb
├── concerns
│   └── concert_user_relationship.rb
├── concert_policy.rb
└── user_policy.rb

module ConcertUserRelationship extend ActiveSupport::Concern
included do
  def allow_add_attendee_to_concert?(attendee, concert)
    concert.published && (user == attendee || user.admin?)
  end

  def allow_remove_attendee_from_concert?(attendee, concert)
    user == attendee || user.admin?
  end

  # also allow_add_performer_to_concert? and allow_remove_performer_from_concert?
end
end

class ConcertPolicy < ApplicationPolicy
  include ConcertUserRelationship

  def add_to_attendees?(users)
    users.all? { |user| allow_add_attendee_to_concert?(user, record) }
  end

  def remove_from_attendees?(user)
    allow_remove_attendee_from_concert?(user, record)
  end

  # also add_to_performers? and remove_from_performers?
end

class UserPolicy < ApplicationPolicy
  include ConcertUserRelationship

  def add_to_concerts_as_attendee?(concerts)
    concerts.all? { |concert| allow_add_attendee_to_concert?(record, concert) }
  end

  def remove_from_concerts_as_attendee?(concert)
    allow_remove_attendee_from_concert?(record, concert)
  end

  # also add_to_concerts_as_performer? and remove_from_concerts_as_performer?
end

[gnfisher]

I like that. Seems like something we should have already thought about when confronting the 'make sure both policies have the same logic' ... In fact I think I'll refactor our app a touch to use this.

[valscion]

Such a design pattern could easily work out OK for most of the time. We might be better off delaying the implementation of an association policy class and only implement one when we have discovered real world usage pains, instead of just ones we might have.

It might also make sense to document such a pattern in the README under the association policy caveats :)

[valscion]

What do you think of the following which is in the roadmap issue #48 ?

• Look into the API of DefaultPunditAuthorizer#replace_fields and figure out if it could be implemented similarly to Authorize relationship actions with custom policy methods #40
  • The replace operation could even call the new replace/remove relationship authorization methods
  • If this wouldn't be done, we might need to bump to v2.0.0 after implementing as it would be a major breaking change in the API... so better look into it sooner than later

Do you think it would be a good first step to implement the #replace_fields authorization similarly to what was done in #40 before releasing a v1.0? Right now it seems oddly out of place, as all other relationship-affecting methods get the relation_name but the #replace_fields only gets the new records :/

[gnfisher]

I will try my best to put a PR together before the end of the week, next week at the latest - with the code from our project for replace_fields and create_resource (uses same routes as @matthias-g 's #40). I am a little bit crazy with work so maybe not until weekend/next week!

[valscion]

Relationship authorization has been implemented and this issue can be closed. https://github.com/venuu/jsonapi-authorization/blob/v1.0.0.beta2/docs/relationship-authorization.md