Updating related resources does not check against the related records

When updating resources e.g. PATCH /articles/:id/relationships/comments or PATCH /articles/:id with related resources that the user does not have access to, I would expect either a 403 or 404 response instead of a 2xx.

https://github.com/venuu/jsonapi-authorization/blob/c7b69079be557848083e7aab77627f95a266f22e/spec/requests/relationship_operations_spec.rb#L179-L182 is marked as a pending spec with a not found response, but it currently returns a 204 ~and updates the article with comments that the user does not have access to.~ edit: this was a problem with the spec, not the code itself

https://github.com/venuu/jsonapi-authorization/blob/c7b69079be557848083e7aab77627f95a266f22e/spec/requests/tricky_operations_spec.rb#L212 explicitly checks that if a user sends a PATCH to an article with comments that the user does not have access to, that a successful response is returned (and though the spec doesn't test it, the article now includes those comments that the user doesn't have access to.

I've confirmed that even when writing a custom ArticlePolicy#replace_comments? method, the comments that are passed in are already limited to those that are in scope by https://github.com/venuu/jsonapi-authorization/blob/c7b69079be557848083e7aab77627f95a266f22e/lib/jsonapi/authorization/authorizing_processor.rb#L335 so the method never has a chance to raise an error or return false.

I believe this is a bug, and that it started with https://github.com/venuu/jsonapi-authorization/commit/ad9aad277d6d262b2b1a2268d18c056362d75294 when related_models_with_context was added and used find_by_keys instead of _model_class.where(...). I'd change it back in a PR but am uncertain what other ramifications that may have.

Thoughts?

Thank you for the detailed report. This does seem like a bug.

Checking the spec you highlighted more closely:

https://github.com/venuu/jsonapi-authorization/blob/c7b69079be557848083e7aab77627f95a266f22e/spec/requests/relationship_operations_spec.rb#L173-L183

It should assert that replace_to_many_relationship was called with all the new comments and not an empty array. Is that not the case? I'm no longer certain if the keywords must all match for allow_operation, but it seems like they must or else the authorization stubbing would not work?

https://github.com/venuu/jsonapi-authorization/blob/c7b69079be557848083e7aab77627f95a266f22e/spec/support/authorization_stubs.rb#L4-L9

That is the problem, that replace_to_many_relationship was called with new_comments (which is comprised solely of comments that the user does not have access to) and the stub forced the authorizer not to raise an error. Because it did not return an error, the spec continued on to attach those comments to the article.

In a non-stubbed test, the authorizer would have checked if the user had update? permissions on the comments (or replace_comments? on the policy) and raised an error at that time. (Side note: It feels like there is a massive over-reliance on stubs in all these request specs)

To clarify: https://github.com/venuu/jsonapi-authorization/blob/c7b69079be557848083e7aab77627f95a266f22e/spec/requests/relationship_operations_spec.rb#L173-L183 is just a broken spec (whether pending or not). The behavior when hitting PATCH /articles/:id/relationships/comments seems correct and raises a 403 (in prod code without stubs) when the user does not have update capabilities on the given comments.

The bigger problem is: https://github.com/venuu/jsonapi-authorization/blob/c7b69079be557848083e7aab77627f95a266f22e/spec/requests/tricky_operations_spec.rb#L212 indicates that the behavior of PATCH /articles/:id is broken when comments are included in the relationships, as it does not raise any errors and continues to update relationships that the user should not have any permissions to see, much less modify.

The difference: Hitting the relationships endpoints goes to https://github.com/venuu/jsonapi-authorization/blob/c7b69079be557848083e7aab77627f95a266f22e/lib/jsonapi/authorization/authorizing_processor.rb#L186-L194 which uses a find, and hitting the main resource endpoint goes to https://github.com/venuu/jsonapi-authorization/blob/c7b69079be557848083e7aab77627f95a266f22e/lib/jsonapi/authorization/authorizing_processor.rb#L116-L125 which uses related_models_with_context.

I believe that creation may also be impacted since it uses related_models_with_context, but I haven't had a chance to verify that yet.

Thank you so much for the digging in extensively. This helps clarify the potential impact this issue has a lot.

So it seems that all the places where we end up calling:

resource_class.find_by_keys

method, can return less records than they should, ending up in a situation where the authorization never sees the records the user should not be able to see in the first place. Yikes 😬

(Side note: It feels like there is a massive over-reliance on stubs in all these request specs)

Yeah, that's true. We didn't come up with another easy-enough alternative back when the specs were done that would've afforded the separation of DefaultPunditAuthorizer from Pundit policies itself.

I don't have any ideas on what to do about those tests now anymore. Converting them to something else than stubs seems like a daunting task.

Would it be possible to assert that the stubbed authorization methods are indeed called with the given parameters and error out otherwise? It seems we wrote those stubs as an expectation of sorts in the spec, but my memory about the details is fuzzy.

Would you be able to start a PR that would replace the find_by_keys calls with _model_class.where(...) as you proposed in your original message? We could figure out together what to do about the specs that would be failing after that change.

Any improvements that would also help us avoid accidents like this would also be huge.

venuu / jsonapi-authorization

Updating related resources does not check against the related records #118