Discovered in #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only.
\Drupal\jsonapi\Controller\EntityResource::createIndividual():
$entity->save();
// Build response object.
$response = $this->getIndividual($entity, $request, 201);
…
return $response;
and
\Drupal\jsonapi\Controller\EntityResource::patchIndividual():
$entity->save();
return $this->getIndividual($entity, $request);
with \Drupal\jsonapi\Controller\EntityResource::getIndividual() doing this:
$entity_access = $entity->access('view', NULL, TRUE);
if (!$entity_access->isAllowed()) {
throw new EntityAccessDeniedHttpException($entity, $entity_access, '/data', 'The current user is not allowed to GET the selected resource.');
}
…
which means that even if you're allowed to POST or PATCH an entity, you still need explicit GET permission for this entity type.
This is confusing.
Core's REST module uses a different approach: when you can POST or PATCH an entity, you also without exception receive the serialization of the entity you just created or modified.
Arguably, JSON API's approach is safer/stricter … but it's important that it behaves similarly to core's REST module. On top of that, the current implementation violates the spec (http://jsonapi.org/format/#crud-creating), because even though the resource (entity) was created, you are not receiving it in the response!
Do not check GET (view) access after successfully PATCHing or POSTing.
None.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 2942255-2.patch | 858 bytes | wim leers |
Comments
Comment #2
wim leersComment #3
wim leersComment #4
gabesulliceNice catch.
Comment #5
wim leersThis is now hard-blocking
Commenttest coverage in #2930028-50: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only because it'sPOSTingan unpublished comment (which perhaps needs to be approved by a site admin first), and due to this bug the POST request receives a 403 response because only published comments can be read.In other test cases, I could work around it in #2930028 because I could just grant extra permissions. I can't in this case, because it's not a matter of permissions, but entity data!
So, going to commit this now…
Comment #7
wim leersSee #2930028-51: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only and #2930028-52: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only— a re-test there should now pass tests, which means that this does have test coverage: over at #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only!
Comment #8
e0ipsoI'm sorry I'm late to the party, but I think that the previous behavior was the classic mailbox behavior. You can put, but you cannot see the result of the put. This is common in file sharing systems. Is that not something we should honor?
Comment #9
wim leersI understand your reasoning, but the spec is pretty clear about it. Let me repeat what I wrote in the IS:
Comment #10
e0ipsoWhile I agree on the principle, I wouldn't like this to be a law.
The spec also says that you should get a response with
/data/typeto a GET request. Implicit to that it is that if an error occurs, you need to handle it. I am confident that the spec doesn't mean to say that you need to return inaccessible resources when you do a patch/post request.If you are not convinced by this, I suggest opening a support request in the JSON API forum, and hold off a release of this commit until we get more clarity.
Comment #11
wim leersAbsolutely!
Can you rephrase this? I don't understand the point you're trying to make, sorry :(
Hm, you're right that the spec is generally very bad about anything access-related. But isn't it logical that you can access the entity that you just created? You know what data was sent, right? And if there's any computed fields with potentially sensitive data, field access would still prevent those from being included in the response!
Comment #12
e0ipsoAll I meant is that the spec assumes that you need to handle errors and exceptions when they occur, even if they are not explicitly mentioned in the particular operation. http://jsonapi.org/format/#errors-processing hints that:
Comment #13
e0ipsoYou can access whatever the access system says. If the system says you cannot view an entity after creating it… oh well. In that case we can either throw a 403 (because the post-create operation is not accessible) or respond with a 204.
Comment #14
gabesulliceI see no wiggle room here. The MUST is definitive. Either we have a client generated ID or we don't. If we don't have a client-generated ID, we must return the resource per our own documentation:
We may omit it only when we have a client-generated ID. We could omit the resource if access is denied at that point, but then we'd have to pass around that information. So we might as well just respond with
201and call it a day.A 403 implies a bad request. Assuming we have a client-generated ID, returning a 403 where the exact same request with new credentials would result in a 409 conflict breaks HTTP semantics. Which we must not do:
Finally, if we are to interpret access denied as an "error", then we must undo the successful save and return 403. A request cannot partially succeed:
Comment #15
wim leersLet's move this back to so we don't accidentally forget to reach a conclusion here.
Comment #16
wim leersis probably more accurate.
Comment #17
wim leersMore than a month has passed. How are we feeling about this?
Comment #18
wim leersNo feedback for >2 months. I'm going to close this.