Handling relationship policy checking in replace_fields #51

gnfisher · 2017-03-08T16:00:26Z

FYI: This breaks tests. I haven't had time to fix them, but wanted to just put this code out to start discussion in the mean time.

This will check permissions on new records being added. It uses the same method syntax as @matthias-g's #40 recently merged PR. i.e. add_comment, remove_comment, add_to_users, remove_from_users, etc. It also creates a new method syntax for polymorphic source records.

For example, if comments are polymorphic, in the comment policy you could check if the user has permissions to comment on an article with add_commentable_article?(article). This would cover payloads like:

# POST api/v1/comment
data: {
  type: 'comment,
  attributes: {
    ...
  },
  relationships: {
    commentable: {
      data: { type: 'articles', id: 1 }
      }
    }
...
}

Also, the remove_to_one_relationship method was not updated to match the rest of the endpoints in @matthias-g 's branch - so it seemed to me - and I updated it here.

Again, need to update the test suite to handle this way of doing things and do more tests around polymorphic scenarios, etc. But it's something to start more conversation on perhaps.

Also this is my first non trivial code contribution to an OSS project so please let me know how I can best help out with this! This is the code I was offering to share in issue #30.

Update both the authorizer and the default pundit policy to follow same pattern as the other relationship endpoints (sending the related record as an argument to the policy method).

authorize_replace_fields

valscion · 2017-03-09T12:27:55Z

lib/jsonapi/authorization/default_pundit_authorizer.rb

+      def remove_to_one_relationship(source_record, related_record, relationship_type)
        relationship_method = "remove_#{relationship_type}?"
-        authorize_relationship_operation(source_record, relationship_method)
+        authorize_relationship_operation(source_record, relationship_method, related_record)


Removing an association doesn't need the old records, as it will be available directly from the record inside the Pundit policies. Wasn't that what you discussed with @matthias-g in #40 (comment)?

valscion · 2017-03-09T12:28:45Z

lib/jsonapi/authorization/default_pundit_authorizer.rb

+                   query: add_method_name,
+                   record: records,
+                   policy: policy
+          end


I think you could use the authorize_relationship_operation helper method a lot in here. It would hopefully cut down the duplication in here a lot :)

valscion · 2017-03-09T12:31:33Z

lib/jsonapi/authorization/default_pundit_authorizer.rb

+        when ->(relationship) { relationship.polymorphic }
+          polymorphic_type = data[:records].class.name.downcase
+          "#{prefix}_#{relationship.class_name.downcase}_#{polymorphic_type}?"
+        when ->(relationship) { relationship.class == JSONAPI::Relationship::ToOne }


I think these case-statements could be written like so?

case relationship when relationship.polymorphic # ... when JSONAPI::Relationship::ToOne # ... else # ... end

I haven't seen this sort of lambda usage before myself 😅

In order to call a method on the case object (like .polymorphic and .class) I have to do this via a lambda. when relationship.polymorphic and relationship.class == JSONAPI::Relationship::HasOne always come back false even when they should be true. Perhaps I am missing something here? Please let me know. It only works properly for me when using lambdas. (ref)

Yes, it seems to be so that you have to use lambda to check for the relationshop.polymorphic case. Ruby doesn't seize to surprise me 😄

For the class case, though, you shouldn't need it to be a lambda.

valscion · 2017-03-09T12:34:41Z

Is it so that you are now using the class name of the actual record to figure out the policy method name? If I understood correctly, in @matthias-g's PR we went with using the relation name from the resource class itself as-is instead of figuring out the class name behind the association.

I think if we'd do the same in here, we could make the code much more simpler and wouldn't have to worry about polymorphic associations as much?

valscion · 2017-03-09T12:35:55Z

Thanks for the PR! This feature is what is missing indeed and we'd like to get it to the 1.0 release 😊 .

Let's figure out how to test this with the current test suite once we agree on a direction where this is going :)

gnfisher · 2017-03-09T12:52:41Z

Thanks, ill review asap and clean things up. Will post back soon!

helper, refactorings.

valscion · 2017-03-13T08:13:45Z

I think we will need to call the remove_from_#{relationship}? and remove_#{relationship}? methods for each record that there used to be when replacing fields.

@matthias-g, what do you think about the case when we're PATCHing away an entire relationship?

Here's the setup:

Article.new(id: 1, comments: [comment_with_id_1, comment_with_id_2]).save

And we'd send this JSON API request:

PATCH /articles/1

"data": {
  "type": "articles",
  "id": "1",
  "relationships": {
    "comments": {
      "data": [
        { "type": "comments", "id": "3" }
      ]
    }
  }
}

What authorization methods should such an operation use? If we didn't have any other methods besides the add and remove ones, I think it would have to be like this:

ArticlePolicy#remove_from_comments(comment_with_id_1)
ArticlePolicy#remove_from_comments(comment_with_id_2)
ArticlePolicy#add_to_comments(comment_with_id_3)

Would that make sense?

If we think this is the solution we should use, we'll need to have specs checking that these are the calls that are being made and authorized against.

valscion · 2017-03-13T14:32:46Z

The build is currently failing because of code style issues:

lib/jsonapi/authorization/authorizing_processor.rb:106:9: W: Useless assignment to variable - field_data.
        field_data = params[:data]
        ^^^^^^^^^^
lib/jsonapi/authorization/authorizing_processor.rb:115:9: W: Useless assignment to variable - field_data.
        field_data = params[:data]
        ^^^^^^^^^^
lib/jsonapi/authorization/authorizing_processor.rb:286:38: C: Avoid comma after the last item of a hash.
              records: related_models,
                                     ^
lib/jsonapi/authorization/default_pundit_authorizer.rb:98:43: C: Trailing whitespace detected.
          # Authorize removing old records 
                                          ^
lib/jsonapi/authorization/default_pundit_authorizer.rb:103:29: C: Trailing whitespace detected.
              source_record, 
                            ^
lib/jsonapi/authorization/default_pundit_authorizer.rb:104:59: C: Trailing whitespace detected.
              relationship_method(data, prefix: 'remove'), 
                                                          ^
lib/jsonapi/authorization/default_pundit_authorizer.rb:124:73: C: Trailing whitespace detected.
      # * +related_records_with_context+ - A hash with the relationship, 
                                                                        ^
lib/jsonapi/authorization/default_pundit_authorizer.rb:273:17: W: Shadowing outer local variable - relationship.
        when ->(relationship) { relationship.polymorphic }
                ^^^^^^^^^^^^

I'm sorry that we don't have a nicer way of running the lint with CI yet 😞 . See #34 for some history regarding these lint failures.

gnfisher · 2017-03-13T14:47:28Z

Is it so that you are now using the class name of the actual record to figure out the policy method name? If I understood correctly, in @matthias-g's PR we went with using the relation name from the resource class itself as-is instead of figuring out the class name behind the association.

I think if we'd do the same in here, we could make the code much more simpler and wouldn't have to worry about polymorphic associations as much?

Sorry but I don't quite follow on this. What situation(s) are you talking about specifically?

If we're talking about the replace_fields_with_context I have a feeling I can refactor that down using some of those helper methods that @matthias-g created, but I've been sick and haven't had time/energy to dig deeper into that just yet. I am looking into it though.

gnfisher · 2017-03-13T14:53:13Z

What authorization methods should such an operation use? If we didn't have any other methods besides the add and remove ones, I think it would have to be like this:
ArticlePolicy#remove_from_comments(comment_with_id_1)
ArticlePolicy#remove_from_comments(comment_with_id_2)
ArticlePolicy#add_to_comments(comment_with_id_3)
Would that make sense?

This is how it is handled in this branch.

valscion · 2017-03-13T15:05:33Z

Sorry but I don't quite follow on this. What situation(s) are you talking about specifically?

Hmm I guess I'm just a bit confused yet as to what Pundit policy method will be called when the resource specifies a custom :relation_name with the relationship http://jsonapi-resources.com/v0.8/guide/resources.html#Options

Here's the setup:

Comment.new(id: 1, reviewing_user: user_with_id_2).save

class CommentResource < JSONAPI::Resource
  include JSONAPI::Authorization::PunditScopedResource

  has_many :tags
  has_one :article
  has_one :reviewer, relation_name: "reviewing_user", class_name: "User"
end

And we'd send this JSON API request:

PATCH /comments/1

"data": {
  "type": "articles",
  "id": "1",
  "relationships": {
    "reviewer": {
      "data": {
        "type": "user",
        "id": "3"
      }
    }
  }
}

Will we call CommentPolicy#remove_reviewer? or CommentPolicy#remove_reviewing_user? method? And the same for #add_reviewer? vs. #add_reviewing_user?.

I think we might be better off if we'd just pick the relation name from the relationship definition, not the :relation_name that was given. I.e. call CommentPolicy#remove_reviewer? and forget about the custom :relation_name.

What do you think?

gnfisher · 2017-03-13T15:53:55Z

Will we call CommentPolicy#remove_reviewer? or CommentPolicy#remove_reviewing_user? method? And the same for #add_reviewer? vs. #add_reviewing_user?.

I think we might be better off if we'd just pick the relation name from the relationship definition, not the :relation_name that was given. I.e. call CommentPolicy#remove_reviewer? and forget about the custom :relation_name.

What do you think?

Ok, I will investigate and report back.

matthias-g · 2017-03-13T16:07:28Z

What authorization methods should such an operation use? If we didn't have any other methods besides the add and remove ones, I think it would have to be like this:
ArticlePolicy#remove_from_comments(comment_with_id_1)
ArticlePolicy#remove_from_comments(comment_with_id_2)
ArticlePolicy#add_to_comments(comment_with_id_3)
Would that make sense?

I agree.

But I see two things to consider:

We should make sure that in case data contains also { "type": "comments", "id": "2"} only ArticlePolicy#remove_from_comments(comment_with_id_1) and ArticlePolicy#add_to_comments(comment_with_id_3) are being called (instead of checking if comment_with_id_2 can be removed and added immediately afterwards).
The article record that is used when calling add_to_comments should already have comments 1 and 2 removed. In case the policy checks that the number of comments does not exceed 2 the request should be allowed.

valscion · 2017-03-13T16:27:59Z

The article record that is used when calling add_to_comments should already have comments 1 and 2 removed

I don't think we will ever be at that stage, considering the authorization happens before any changes to models are made.

I'm not quite sure what you're saying, so maybe you could elaborate this second part?

matthias-g · 2017-03-13T17:04:53Z

I'm not quite sure what you're saying, so maybe you could elaborate this second part?

Oh, sorry, that was indeed not clear.
Actually what I thought was that it might not be enough having just the add and remove methods. Using the approach mentioned above and a policy like

class ArticlePolicy
  def add_to_comments?(comments)
    (record.comments.count + comments.count) < 3
  end
end

the PATCH /articles/1 as above would be disallowed because the add_to_comments?(comment_with_id_3) would return false. But a user would expect the request to be allowed because it results in having only one comment for the article.

- Utilizes `authorize_replace_to_many_relationship` and `authorize_replace_to_one_relationship` helpers. - Removes helpers no longer used after refactor.

valscion · 2017-04-04T13:07:57Z

Hmm I think you might need to also call create_to_many_relationship? and create_to_one_relationship? authorizer methods when you're POSTing a new resource? Or am I mistaken?

gnfisher · 2017-04-04T13:11:43Z

My understanding is that all of that logic goes inside of the replace_<resource>? action in the policy. From the docs:

 def replace_comments?(new_comments)
    allowed = record.comments.all? { |comment| new_comments.include?(comment) || add_to_comments?([comment])}
    allowed && new_comments.all? { |comment| record.comments.include?(comment) || remove_from_comments?(comment) }
  end

valscion · 2017-04-04T13:21:16Z

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

Should we call these methods in this case? (NOTE: The names are arbitrary, I don't like these)

ArticlePolicy#create?
ArticlePolicy#create_with_user?(user_1)
ArticlePolicy#create_with_comments?([comment_1, comment_2])

I'm afraid that we haven't actually discussed this case at all. 😕

What should we authorize in this case? 😧

valscion · 2017-04-04T13:24:12Z

The case in the above comment might need new methods to handle the creation of resources because this is basically how the ArticlePolicy will be instantiated when we're creating new resources instead of modifying existing ones:

policy = ArticlePolicy.new(current_user, Article)

Note that we don't yet have an instance of Article model yet at this point, and we cannot have it.

gnfisher · 2017-04-04T13:30:39Z

I don't think a new action is necessarily required but it is an option.

# ArticlePolicy.rb

def initialize(user, record)
  @user = user
  @record = record.is_a? Class ? record.new() || record
end

def replace_comments?(new_comments)
  remove_from_comments?(record.comments)
  add_to_comments?(new_comments)
end

Could we just do this?

valscion · 2017-04-04T13:52:26Z

Could we just do this?

I'm afraid not 😞

We could however modify the scope of this PR to just handle the PATCH operation, though, and figure out what to do with creating resources and authorizing their relationships separately, in order to get this PR merged and to give room to figure this out on its own.

gnfisher · 2017-04-04T13:57:14Z

Ah, I suppose an new instance would respond with an empty array and the remove_from_comments? action might return false? But couldn't you just return true if new_comments.empty? from those type of methods? I am not sure why setting record to a new instance is not a viable solution. What am I missing? :)

Ok, I have no problem with reducing the scope to just replace_fields.

valscion · 2017-04-04T14:03:33Z

But couldn't you just return true if new_comments.empty? from those type of methods?

Yes, but we don't want to put that burden on our users. Nobody likes the NoMethodError: undefined method X for nil:NilClass, and it would be surprising to get one in this case. That most likely would be because of a badly designed API.

gnfisher · 2017-04-04T14:04:57Z

Ok I figured that was the reasoning. I will clean out create_resources and try to take care of tests on the replace_fields then. Meanwhile, we can open a new issue to specifically chew on the create_resource options?

valscion · 2017-04-04T14:08:04Z

Meanwhile, we can open a new issue to specifically chew on the create_resource options?

Done, see #56 :)

gnfisher · 2017-04-05T01:55:55Z

Returned create_resource to its previous state.
Add spec coverage around #replace_fields

One failing spec seems to me to be an issue with the instance double... I'm not quite sure how to resolve it. It's this spec, which is pending anyway, but still.

# ./spec/requests/tricky_operations_spec.rb:132

      context 'limited by Comments policy scope' do
        let(:comments_policy_scope) { Comment.where("id NOT IN (?)", new_comments.map(&:id)) }
        before { allow_operation('replace_fields', article, related_records_with_context) }

        it do
          pending 'DISCUSS: Should this error out somehow?'
          is_expected.to be_not_found
        end
      end

valscion

Thanks for this, the code looks pretty straightforward to me! I left a few questions inline.

Could you also merge the master branch here, as I discovered that CI didn't actually run the specs (:flushed:) and I just merged #57 to make them run again.

valscion · 2017-04-05T07:38:48Z

spec/jsonapi/authorization/default_pundit_authorizer_spec.rb

+    let(:related_records_with_context) do
+      Array.new(1) {
+        Hash[:relation_name, :comments, :relation_type, :to_many, :records, related_records]
+      }


Could this be written using literal array & hash syntax? Like so:

let(:related_records_with_context) do [{ relation_name: :comments, relation_type: :to_many, records: related_records }] end

I don't know about you, but I have a bit of a hard time figuring out how the hash looks like if I use the Hash[] constructor

valscion · 2017-04-05T07:39:00Z

spec/requests/tricky_operations_spec.rb

+    let(:related_records_with_context) do
+      Array.new(1) {
+        Hash[:relation_name, :comments, :relation_type, :to_many, :records, new_comments]
+      }


Same thing in here as in https://github.com/venuu/jsonapi-authorization/pull/51/files#r109847602

valscion · 2017-04-05T07:42:54Z

lib/jsonapi/authorization/authorizing_processor.rb

+              else
+                resource_class = resource_class_for_relationship(assoc_name)
+                primary_key = resource_class._primary_key.to_sym
+                resource_class._model_class.find_by(primary_key => assoc_value)


Hmm is there a reason why this third branch looks so different to the Hash and Array case? I would've expected here to be similar code using resource_class.find_by_key instead of going to the _model_class and using .find_by manually.

No, that was existing code from #related_models I believe that i co-opted.

All changes made.

… into relationship-processing-enhancements

valscion · 2017-04-05T10:59:40Z

Looking great! Do you think this would benefit from having the same specs for the case when the policy classes don't specify the replace_<type>? methods? That is, cases similar to these:

https://github.com/venuu/jsonapi-authorization/blob/master/spec/jsonapi/authorization/default_pundit_authorizer_spec.rb#L315-L332

    context 'where replace_<type>? not defined' do
      # CommentPolicy does not define #replace_article?, so #update? should determine authorization
      let(:source_record) { comments(:comment_1) }
      let(:related_records) { Article.new }
      subject(:method_call) do
        -> { authorizer.replace_to_one_relationship(source_record, related_record, :article) }
      end

      context 'authorized for update? on record' do
        before { allow_action(source_record, 'update?') }
        it { is_expected.not_to raise_error }
      end

      context 'unauthorized for update? on record' do
        before { disallow_action(source_record, 'update?') }
        it { is_expected.to raise_error(::Pundit::NotAuthorizedError) }
      end
    end

We can do that in another PR, though, as this is getting quite long. Thanks for your help!

gnfisher · 2017-04-05T11:02:55Z

Absolutely, I will file a new PR to add them. And then later this week leave my thoughts on #create_resource discussion.

Thank you @valscion for your incredible patience and helping me to get something contributable finished. I learned a lot!

valscion · 2017-04-05T11:03:54Z

Thank you for enduring all the way through this long PR. It's been a pleasure seeing you learn new things and you being able to contribute to this project ☺️

gnfisher added 9 commits March 8, 2017 11:33

Update remove_to_one_relationship

43f73db

Update both the authorizer and the default pundit policy to follow same pattern as the other relationship endpoints (sending the related record as an argument to the policy method).

Add related_models_with_context method helper

933547f

User related_models_with_context for authorize_create_resource and

f0d300a

authorize_replace_fields

Add relationship policy checks to create_resource

d79acfc

Add relationship policy checking to replace_fields

45b8e7e

Add relationship_method helper

44779ef

Merge branch 'master' into relationship-processing-enhancements

ec1926b

Minor styling changes

78c8467

Add missing param and fix typo

34a06ef

valscion reviewed Mar 9, 2017

View reviewed changes

Change back remove_to_one_relationship methods

49d3390

valscion mentioned this pull request Mar 9, 2017

Roadmap for version 1.0 🎉 #48

Closed

12 tasks

Remove old records from helper, use authorize_association_operation

2d8222c

helper, refactorings.

gnfisher force-pushed the relationship-processing-enhancements branch from f1a3461 to 2d8222c Compare March 10, 2017 16:15

gnfisher added 2 commits March 13, 2017 10:15

Fix up case statement

00a179d

Properly handle old record removal validation in replace_fields

5a83560

Fix rubocop complaints

09b26e4

Properly format polymorphic type

061d165

Refactor create_resources and replace_fields

b09fbb3

- Utilizes `authorize_replace_to_many_relationship` and `authorize_replace_to_one_relationship` helpers. - Removes helpers no longer used after refactor.

gnfisher force-pushed the relationship-processing-enhancements branch from c2e459f to b09fbb3 Compare April 4, 2017 01:21

valscion mentioned this pull request Apr 4, 2017

How to authorize relationships when creating a new resource? #56

Closed

Rollback create_resource

4a95aa2

gnfisher force-pushed the relationship-processing-enhancements branch from 6efb32d to a39f903 Compare April 5, 2017 01:54

Add spec coverage for #replace_fields

082d9eb

gnfisher force-pushed the relationship-processing-enhancements branch from a39f903 to 082d9eb Compare April 5, 2017 01:58

valscion changed the title ~~Handling relationship policy checking in create_resources and replace_fields~~ Handling relationship policy checking in replace_fields Apr 5, 2017

valscion reviewed Apr 5, 2017

View reviewed changes

gnfisher added 2 commits April 5, 2017 07:42

Switch to hash and array literal in spec, use resource.find_by_key

41d641c

Merge branch 'master' of https://github.com/venuu/jsonapi-authorization…

51f6130

… into relationship-processing-enhancements

valscion approved these changes Apr 5, 2017

View reviewed changes

valscion merged commit ad9aad2 into venuu:master Apr 5, 2017

gnfisher mentioned this pull request Apr 7, 2017

Add spec for missing #replace_fields scenario #59

Merged

thibaudgg mentioned this pull request Apr 11, 2017

Make compatible with jsonapi-resources 0.9 #52

Merged

valscion mentioned this pull request Jun 3, 2017

Fall back to authorizing update? on related records #70

Merged

Handling relationship policy checking in replace_fields #51

Handling relationship policy checking in replace_fields #51

Uh oh!

Conversation

gnfisher commented Mar 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnfisher Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valscion commented Mar 9, 2017

Uh oh!

valscion commented Mar 9, 2017

Uh oh!

gnfisher commented Mar 9, 2017

Uh oh!

valscion commented Mar 13, 2017

Uh oh!

valscion commented Mar 13, 2017

Uh oh!

gnfisher commented Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnfisher commented Mar 13, 2017

Uh oh!

valscion commented Mar 13, 2017

Uh oh!

gnfisher commented Mar 13, 2017

Uh oh!

matthias-g commented Mar 13, 2017

Uh oh!

valscion commented Mar 13, 2017

Uh oh!

matthias-g commented Mar 13, 2017

Uh oh!

valscion commented Apr 4, 2017

Uh oh!

gnfisher commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valscion commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valscion commented Apr 4, 2017

Uh oh!

gnfisher commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valscion commented Apr 4, 2017

Uh oh!

gnfisher commented Apr 4, 2017

Uh oh!

valscion commented Apr 4, 2017

Uh oh!

gnfisher commented Apr 4, 2017

Uh oh!

valscion commented Apr 4, 2017

Uh oh!

gnfisher commented Apr 5, 2017

Uh oh!

valscion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valscion commented Apr 5, 2017

gnfisher Mar 10, 2017 •

edited

Loading

gnfisher commented Mar 13, 2017 •

edited

Loading

gnfisher commented Apr 4, 2017 •

edited

Loading

valscion commented Apr 4, 2017 •

edited

Loading

gnfisher commented Apr 4, 2017 •

edited

Loading

valscion commented Apr 5, 2017 •

edited

Loading