Problem/Motivation
When trying to update a comment via REST using:
- method: PATCH
- URL:
/comment/1
Content-Type: application/json
{
"langcode":[{"value":"en"}],
"subject":[{"value":"New Subject"}],
"uid":[{"target_id":"1","url":"/discasaurus.com/user/1"}],
"status":[{"value":"1"}],
"comment_type":[{"target_id":"comment"}],
"default_langcode":[{"value":"1"}],
"comment_body":[{"value":"<p>New body...</p>","format":"basic_html"}]
}
A 403 response is sent, with the following body:
{"error":"Access denied on updating field 'comment_type'."}
Removing the comment_type
from the request body results in a 400 response with the following body:
{"error":"A string must be provided as a bundle value."}
Proposed resolution
In \Drupal\rest\Plugin\rest\resource\EntityResource::patch()
(for updating entities via REST), retrieve an entity type's bundle key, and ignore any fields that set it.
Once that is done, you'll get an exception like this though:
LogicException: The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\rest\ResourceResponse. in Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (line 159 of /var/www/discasaurus.com/drupal/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php).
This is due to rdf_comment_storage_load()
generating a URL that causes cacheability metadata to be bubbled. So a minor change/fix to rdf_comment_storage_load()
is also necessary.
Remaining tasks
TBD
User interface changes
None.
API changes
TBD
Data model changes
TBD
Comment | File | Size | Author |
---|---|---|---|
#53 | interdiff-2631774-47-53.txt | 1.29 KB | tedbow |
#53 | 2631774-53.patch | 10.5 KB | tedbow |
#47 | interdiff.txt | 2.52 KB | dawehner |
#47 | 2631774-47.patch | 10.51 KB | dawehner |
#40 | 2631774-40.png | 47.06 KB | marthinal |
Comments
Comment #2
tyler.frankenstein CreditAttribution: tyler.frankenstein commentedComment #4
Wim LeersConfirmed.
Comment #5
Wim LeersComment #6
Wim LeersThis is what I wrote on IRC, shortly after I discovered this issue:
Had a first stab at updating the issue summary. Will try to push forward @tyler.frankenstein's proposed solution.
Comment #7
Wim LeersOption 0: the patch in #2
The patch doesn't work (tests fail), and only fixes the problem in the particular case of the author because the access restrictions are simply removed. That's not a proper solution.
Option 1: naïve
I would say we do something like this:
… but that means we also won't get errors anymore when trying to change the UUID, i.e. no more errors like:
… even though you actually are changing the UUID field.
Option 2: generalize
But, really, we do something similar for the
langcode
field already. So why not simply generalize it? That would look like this:But, the same problem: no complains when trying to change the UUID field. This time around, we are not actually changing the UUID field anymore though: any entity field that is also an entity key is simply ignored. This is also in compliance with the general rule https://en.wikipedia.org/wiki/Robustness_principle.
:Option 3: special case the
bundle
field just like we special case thelangcode
fieldOption 4: ?
I don't think there's a significantly different option? We can't modify
\Drupal\serialization\Normalizer\EntityNormalizer::denormalize()
to not need the bundle in any case.Comment #8
Wim LeersOption 2 actually doesn't make sense, because e.g. entity label can definitely be updated for most entities, yet option 2 would no longer allow for that.
Given all that, I think something like option 3 makes most sense: it fulfills the requirements for the
EntityNormalizer
to work correctly, and gets in the way as little as possible! (It only gets in the way in that you won't be getting a{"error":"Access denied on updating field 'comment_type'."}
error anymore, but that was a directly conflicting requirement anyway.)Comment #9
Wim LeersSo #8 allows you to update a comment (or any entity with a bundle field that you're not allowed to change). But I can confirm the problem that the original poster mentioned:
This fixes that.
Comment #10
Wim LeersIS updated with the proposed solution.
Comment #11
Wim LeersNot every entity type has bundles.
Comment #12
Wim LeersComment #13
Wim Leers#2405091: Cannot create user entities - {"error":"Access denied on creating field pass"} solved similar problems for creating entities. It also vastly expanded
\Drupal\rest\Tests\CreateTest()
, just like we should expand\Drupal\rest\Tests\UpdateTest
here.Comment #14
neclimdulRe: Option 1
This was the approach I expected. But this code:
I assumed this would look like:
This would still give us that error because if the values aren't the same we fall back on the access check.
Comment #15
Wim LeersThat indeed makes sense. Let's do that.
Comment #16
Wim LeersThere, did that. And since the special handling for the langcode field is just another special case of the more general entity key field special handling we're introducing here, we can (and should) refactor that while doing this, to keep the code understandable.
Comment #18
Wim LeersComment #20
Wim LeersOops.
This snuck in from another issue. And it also caused the sole failure.
Comment #21
Wim LeersManually edited patch.
Comment #22
neclimdulWorks for me. Preemptive RTBC assuming this would should actually come back green.
Comment #23
larowlanelse if (isset($entity_keys['langcode']) && $field_name === $entity_keys['langcode'] && $field->isEmpty()) { + continue;
This will never evaluate TRUE because you now call array values on the keys
Also, why the rdf changes? I think they might break unsaved entity preview, there is a similar issue for users and terms #2314509: RDF module prevents viewing of unsaved users - might need a try/catch?
Comment #24
Wim LeersWhy RDF changes: see IS, explained there. Does not break anything, we merely prevent URL generation from bubbling metadata.
Comment #25
Wim Leers#23: nice catch! Will fix that.
Finally wrote the test coverage I said we'd need in #8. We don't ever want to find out this PITA again!
In doing so, I also noticed that this problem only occurs for the JSON normalization/serialization, not for HAL+JSON! The fact that pretty much all our test coverage for REST is only testing HAL+JSON therefore does not help inspire confidence… Even worse: the
$entity->set($field_name, NULL)
-style removal of values that are read-only that the existingUpdateTest
does only work for HAL+JSON! For JSON, we have to resort toarray_diff_key()
, because it's not actually possible to unsetcid
, for example. So, great, we'd standardize onarray_diff_key()
, right? Nope! Wrong again! Doesn't work because then we'll still end up with children of_links
in HAL+JSON referring to fields we're not allowed to update. These differences are because HAL uses its own normalizer.I've now got less faith in REST test coverage than I already had.
So, I present to you, a piece of very solid test coverage, that exposes this problem in the first place, but that also shows the crazy normalizer-dependent omissions you have to do when building PATCH requests. See
\Drupal\rest\Tests\UpdateTest::assertPatchEntity()
.No interdiff, because this is pure test coverage to prove that HEAD is broken. In the next reroll, I will add #21.
Comment #26
Wim LeersAlso, please note the difference in read-only fields when comparing HAL+JSON and JSON:
hal
module's normalizer)serialization
module's normalizer)Comment #27
Wim LeersSo, here's the fix merged in again (i.e. #21 ===
interdiff.txt
).Note that because the test coverage is so comprehensive, this will fail!
The test coverage starts with the given entity, tries to PATCH it, but gets a 403 because it's trying to modify read-only fields. Then it removes the first item in the list of read-only fields, tries again. Now it gets a 403 for the next read-only field. And so on. So, we comprehensively test which fields you're allowed to modify. Should this list change, then that will be a significant difference for REST API users, so it's important that we do exercise that.
Now, the patch (#21) actually makes it so that any entity key can be sent without complaints. So we won't get an error anymore for the UUID and CID fields either.
Comment #28
Wim LeersThis patch will be green.
It no longer removes entity key fields, because those are now safe to send (see the explanation in #27).
Comment #29
dawehnerJust a quick driveby review.
Ideally we should typehint $comment, because then we don't have to typehint $entity, as its type follows automatically.
If possible it always seems better to test with the entity_test entity type and not a special snowflake like node.
Comment #32
Wim Leers\Drupal\rest\Tests\RESTTestBase::setUp()
and\Drupal\rest\Tests\CreateTest
already do. But I think you're right of course. Fixed.Comment #33
dawehnerLet's explain why. The code explains that they need special treatments already.
No worries. I just try to push for not using node all the time, just because it seems to be more convenient.
Comment #34
Wim LeersDone.
+1. Thanks for pushing for that :)
Comment #36
gabesulliceTriaging with valthebald.
Comment #37
gabesulliceConfirmed #34 applies and resolves the issue.
Comment #38
larowlanrtbc +1 - thanks all
Comment #39
catchWhy's this necessary? And what's the difference? Could use a comment to explain if it really is.
Why is the array_values() necessary?
Should this be:
'Unchanged values for entity keys don't need access checking'?
Comment #40
marthinal CreditAttribution: marthinal commented1) IMHO we need to fix #2626298: REST module must cache only responses to GET requests and then we don't need this change. In this hook (
rdf_comment_storage_load()
) we only need to obtain the url... and well, a patch request should not be cached anyway. I've tested manually applying that patch and the Exception disappears.2) There's an empty value (see the attached image) and we only avoid to check some values. To be honest I'm not sure 100% if makes sense this option or maybe simply verify langcode + bundle_key.
3) Done.
Comment #41
dawehnerConceptually it would be still nice to do that. Giving good examples to people is the right thing to do. On the other hand your are right about that. The other patch fixes most proabably the problem.
For me this is ready for it.
Comment #42
alexpottSo given @catch's comment and the responses don't we need an @todo here?
Comment #43
dawehnerWhat about adding something like
// We are in a storage load hook, which is outside of a render context, so we should avoid to bubble metadata up, which <code>$entity->url()
would.?
Comment #44
xjm(Updating issue credit for the major triage sprint. Thanks @gabesullice for helping triage this!)
Comment #45
valthebaldWas triaging the issue during DrupalCon NOLA Friday sprint with @gabesullice
Comment #46
xjm(Added credit.)
Comment #47
dawehnerLet's just fix that. I also changed the "assert" function to use a normal function. There is no clear abstraction happening in this function but rather a set of operations and some assertions in there.
Comment #48
effulgentsia CreditAttribution: effulgentsia at Acquia commentedDiscussed with @alexpott, @catch, @xjm, @webchick, and @Cottser, and we agreed this is Major priority, so tagging. As per the recently updated bullet points in https://www.drupal.org/core/issue-priority#major-bug, this is a "feature unusable with no workaround".
Comment #49
Wim LeersI think all feedback has been addressed.
Comment #50
alexpottI still don't get the need for array_values() here. Since using it does not remove the empty value. If we want to do that we should use array_filter. Not sure it is worth it. Plus I'd change the in_array to use the strict comparison option just to be type safe.
Comment #51
alexpottDo we need to protect against empty arrays here? And fallback to the default format if so?
Comment #52
Wim Leers#50: oops, sorry, I thought that was addressed. I looked at the various patches here. Turns out I proposed this in #7. AFAICT I did it originally just to stress we only want the entity keys for a particular entity type. But keeping the keys would be totally harmless:
So, +1 to remove
array_values()
.#51: the reason is that we want to keep BC for
enableService()
for custom/contrib tests. It used to only allow a single format to be passed in, and otherwise default to "the default format" (for a particular test). This patch is making it possible to specify multiple formats.We don't need to protect against empty arrays: if somebody specifies that, then it's intentional (and probably wrong).
IOW: we don't need to babybsit crappy/broken tests, but we need to make sure we retain BC.
Comment #53
tedbowFixed #50
Comment #54
dawehnerNice, better code!
Comment #55
alexpottI think this fix is BC compatible and therefore should be committed to 8.1.x as well since this is a major bug fix. Committed 829fe8a and pushed to 8.1.x and 8.2.x. Thanks!
Coding standards fix on commit.
Comment #58
Wim LeersYAY! Took almost 4 months, but so happy to finally see this in. This was a huge, show-stopping REST bug, IMO bordering on critical.
Thanks for also committing it to Drupal 8.1!
Comment #59
Wim LeersFor the brittleness we saw here wrt formats and test coverage, as described in #25 in particular, I opened #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.
Comment #61
webchickThis sounds like it allows developers to use comments as first-class citizens, so worthy of release note mention.
Comment #62
kalinchernev CreditAttribution: kalinchernev as a volunteer commentedDraft change record created.
Edit: I was following the guide and I'm unsure whether draft is correct when the patch is already merged.
Comment #63
pranilkochar CreditAttribution: pranilkochar commentedDrupal 8 supports OAuth 2.0 as na authentication mechanism?
Comment #64
Wim LeersI just noticed the CR was never published. Just did that: https://www.drupal.org/node/2778563.
Comment #65
jhonatanfdez CreditAttribution: jhonatanfdez commentedProposed resolution
method: PATCH
URL: drupal8/comment/1
Content-Type: application/json
note: the user must have the administrator role