How to authorize relationships when creating a new resource?

[valscion]

In the new resource case, we don't yet have a record available in the Pundit policies as all we've got is the class.

user_1 = User.new(id: 'user-1').save!
comment_1 = Comment.new(id: 'c-1', content: 'Is this the real life?').save!
comment_2 = Comment.new(id: 'c-2', content: 'Is this just fantasy?').save!

POST /articles

{
  "type": "articles",
  "relationships": {
    "author": {
      "data": {
        "type": "users",
        "id": "user-1"
      }
    },
    "comments": {
      "data": [
        { "type": "comments", "id": "c-1" },
        { "type": "comments", "id": "c-2" }
      ]
    }
  }
}

This will need to be addressed somehow... I'm afraid that we don't actually have any methods we could use for this case, though, as all of the relationship operation methods expect that we already have a record instance! Damn.

What should we call these methods in this case? (NOTE: The names here are arbitrary, they are just meant for a conversion starter)

1. ArticlePolicy#create?
2. ArticlePolicy#create_with_user?(user_1)
3. ArticlePolicy#create_with_comments?([comment_1, comment_2])

This discussion was splitted off from #51

[valscion]

I was thinking about authorizing the relationships in an after hook. That is, the order of authorizations would go like this:

POST /articles

1. Run before callback for create_resource operation:
   1. JSONAPI::Authorization::AuthorizingProcessor#authorize_create_resource
   2. authorizer.create_resource(source_class)
2. The new Article is saved (and we're still inside a transaction)
3. Run after callback for create_resource operation
   1. JSONAPI::Authorization::AuthorizingProcessor#authorize_after_create_resource
   2. authorizer.after_create_resource(source_record, related_models_with_context)
   3. and in after_create_resource, call existing #authorize_related_records method

There's one biiig problem with this, though. That is, we would already have mutated the relations by the time the policy methods are called, so authorize_replace_to_many_relationship would work in a slighly different way and it would lead to weird bugs.

So we can't go the route of using after hooks.

---

We will have to come up with a way that won't have access to the newly created instance. All we've got is the record class, and the existing related models, and we will have to use only them to figure this out.

So whatever solution we choose, will have to work with an API like this:

class ArticlePolicy
  def create_with_comments?(new_comments)
    # NOTE: `record` is the class `Article`, not an instance of `Article` here
    new_comments.all? { |comment| comment.author == user }
  end
end

What do you think about the name create_with_<type>? — is it any good?

[gnfisher]

Maybe this is stupid, but what if we passed in a new instance of the class to the policy instead of the class itself.

def authorize_create_resource
        # How we do it now:
        # source_class = resource_klass._model_class 
        source_record = resource_klass._model_class.new()
        authorizer.create_resource(source_class, related_models)
end

This would prevent any NoMethod on Nil errors in their replace_comments?. I can't think of a situation where having an unpersisted, empty instance would raise any issues.

It would also reduce a bit of code in the DefaultPunditPolicy if we went this route. Like a line. But still. 😄

[valscion]

I can't think of a situation where having an unpersisted, empty instance would raise any issues.

If I had the model in my hands, I'd suppose that the instance would have attributes set, too. Which is not the case :(

class ArticlePolicy
  def replace_comments?(new_comments)
    record.owner == current_user && # ...
  end
end

In this case, the record.owner would not be set and users of this library would get subtle authorization bugs :(

[gnfisher]

Ah ha, that was the catch. Now I remember. 😞

Is it a bridge too far to pass in a mock-like version of the to-be-created object? Not an actual ActiveModel object that might accidently be saved etc in the policy, but a JSONAPI::Authorization::InstanceMock that mimics it and has the to-be-saved attributes set? Not in love with that name but just to share the idea.

[valscion]

Yeah I'm afraid that would just lead to some strange API that would without a doubt create way too big a surface area for bugs. That's very worrisome especially for something as critical as authorization 😥

[gnfisher]

That makes sense. It seems like we have only two options.

1. create_with_comments? type actions. Downside is there are going to be a lot of auth actions in policies. But thats the only one I can think of. In all its still a manageable and reasonable set of action templates we are using, given how how the jsonapi spec works, and we are only adding one more with this implementation - replace_, add_, add_to_, remove_, remove_from, create_with_.

2. Leave it to the user to check record.is_a? Class in their replace_comments? type actions, and handle the case of it being a new record however they wish. Best we could do is make an explicit example in the readme. The advantage of this is it is hands off. It is how we currently handle the case at work.

I can't imagine a third way if we must pass in the class to the policy.

[valscion]

Yeah, I'd be in favor of the first option. Once we've got that solution out, people can build abstractions on top of our naming conventions in their own code or in a separate gem if they want to experiment with needing less policy methods.

[gnfisher]

What is the fallback (if any) when create_with_<type>? is undefined, @valscion?

For all other relationship methods, it falls back to update? on the source_record. With create, we can't do that obviously.

If undefined, just fallback to create? (meaning do nothing, in reality)? Or raise an error and require a create_with_<type>? action in the policy?

[valscion]

For all other relationship methods, it falls back to update? on the source_record. With create, we can't do that obviously.

We currently fallback to update on the related records and it should work for this case, too. The related records already exist at the time of calling the authorization for the new resource creation.

[valscion]

This issue has been obsoleted some time ago already by #60