Drupal 8 only allows one user every 6 hours to register when page caching is enabled — caused by entity UUID in form state

This either needs tests or should enable the page cache for the UserRegistrationTest to make it explicit.

a lot of developers will suffer from this, it happens a lot that you generate stuff and put it into the formstate. Most devs will simply disable page caching in the end for every page with a form that is a bit more complex.

That's fine, no problem with that. The problem is generating a unique identifier, putting that in form state and expecting it to stay unique.

@Berdir yep but a dynamic default value is going to have problems with this too - no?

Right, it is, but that's not different to any other page. You need to either use JS or disable page cache for it. Or if that dynamic value doesn't need to be visible, generate it on submit?

I don't think we we can do anything about it and it's also no different than it ever was?

No but now some people will think this is possible, so it would be good to have an example page of some forms that can't be cached.

start of a test

Removing the no_cache for the user/register route (as added by #606840: Enable internal page cache by default) should be enough to make the existing user registration test fail as well, but an explicit test is likely useful too.

Works on failures

+++ b/core/modules/system/src/Tests/Entity/EntityFieldDefaultValueTest.php
@@ -26,6 +26,9 @@ class EntityFieldDefaultValueTest extends EntityUnitTestBase  {
     parent::setUp();
+    foreach (entity_test_entity_types() as $entity_type) {
+      $this->installEntitySchema($entity_type);
+    }

@@ -48,6 +51,7 @@ public function testDefaultValues() {
   protected function assertDefaultValues($entity_type_id) {
     $entity = entity_create($entity_type_id);
+    $entity->save();

I think we should just remove UUID from this test. It's no longer a default value, so we shouldn't be asserting it here.

sure

Related - see discussion in #2318605-52: uuid, created, changed, language default value implementations need to be updated (comments #52 - #58) :

- EntityStorageBase::create() has code that generates UUIDs
- It seems generic (and is in EntityStorage*Base*), but only actually does something if $this->uuidService is defined. Only ConfigEntityStorage defines it, and ContentEntityStorageBase doesn't, so generation of UUIDs in EntityStorageBase::create() in fact applies to config entities, not for content entities - not exactly intuitive.
- For content entities, UUID generation happens in an entirely different place, at the "field default value" level.
- It is hardcoded in UUIDItem::applyDefaultValue(), and thus applies for *any* field of type 'uuid'. this sucks, because you could want a uuid field that stores an external reference and is not "the primary UUID of *this* entity", and for which we don't want a default value.
- The proposed approach over there was to move "the primary uuid field of a content entity" from
$field['uuid'] = new BaseFieldDefinition('uuid')
to something like :
$field['uuid'] = new BaseFieldDefinition('uuid')->setDefaultValue('***RANDOM***')
with the magic around '***RANDOM***' being coded in UUIDItem::processDefaultValue()

Right, the solution there actually won't help us, but we could do a mix?

Move the generation to Entity*::preSave(), based on the existence of a uuid key, generate it only there, remove the logic from the field item completely? We still need the lazy-creation in uuid(), so we could just have preSave() { // Ensure that a uuid is generated $this->uuid() }?

And then also unify config and content entities, if possible?

[edit: crosspost with #19]

Hm - the approach mentioned in #18 keeps the UUID generation at the "field default value" level, meaning at entity creation time, meaning I guess it would still leave the bug about "same entity is generated once and cached in the form state for user/register", right ?

But the drawback of the current patch is that it keeps the assignment of a generated UUID hardcoded to all fields of type 'uuid', so you can't have a field that just "stores some uuid".

Also, having UUID generated at create time for config entities, but at save time for content entities sounds like a nice WTF :-/

- Any way we can fix the form state cache thing without moving UUID generation at save time ?
- If we find this problem on a form with a content entity, couldn't we have the same problem on a form with a config entity ?
- Thus, if we *have* to move UUID generation to save-time, shouldn't we be doing it for config entities *and* content entities ?
- Whether create time or save time, any way we can re-unite UUID generation in EntityStorageBase, for contant and config entities both, *only* for the primary 'uuid' field of *this* entity, (based on the 'uuid' key in the entity type annotation) ?

Yeah, my suggestions overlap quite a bit with what you are saying :) Which I think is a good sign.

So first, trying to summarize a bit how form state works when combined with page cache, as far as I understand it.

When the page is requested the first time, we generate a new user with all the default values, put it in form state, build the form, store the form state and display it. The form state is identified through the form id and form build id.

Every user that gets the cached page then obviously gets the same build id. When they submit the form, they get the stored form cache and basically "branch off at that point", they either then submit the form or, in case of a validation error or ajax request (since the security fix a while ago), rebuild the form with a new build id. now they have their own form, and are separated from others. In 7.x, we additionally had a line that delete the form state when page caching was disabled, that no longer exists in 8.x but doesn't really change anything.

Anything that is generated on the first form build however is shared between between all users, unless it is explicitly changed again.

That's why I think we must not generate the UUID as a default value but only set it when the entity is saved or we need the UUID for something. And yes, if someone manages to call uuid() in the first build, then it would be broken too. I don't know what we could do about that.

The form doesn't know if it will be cached or not. We're trying to, with the immutable flag, but that will soon only be set in a form alter and is missing a check for max age, because internal and external cache are two separate things now. So we can't really do something differently based on whether we will be cached or not.

@fago asked in IRC if we couldn't find a generic way to fix this problem. But I don't know what that even means. In general, cached entity forms *do* work, just this part doesn't. Even if we find a way to invalidate a form build ID on submission for example, there's still a small risk for a race condition when two submissions happen at the same time, and it might change the expected behavior of other forms, I'm not sure.

To be clear, it's not something new in Drupal 8 that is triggering this. Exactly the same could happen in 7.x with enabled cache too, if you in a form alter or field use a default value that needs to be unique.

Also, this problem is *not* limited to the page cache. I've seen this a few times before when manually creating a few nodes. I never quite got what caused it, but I just found a way to reproduce it:

1. Go to /node/add/article
2. Fill out, make sure to force a server-side validation error (empty space in title works)
3. Fix and save the node.
4. Use the back button
5. Now the browser is so nice to show you the previous page again (you can still the form validation error). Without validation error, the page is loaded again.
6. Try to save the node again.
7. BOOM.

Discussed this with @webchick, @alexpott, and @xjm on our triage call, and yep, this is critical per https://www.drupal.org/core/issue-priority, because:

1. this bug makes the self registration system unusable
2. having more than one user per 6 hour period self-register is not a rare circumstance
3. there's no acceptable workaround (for many sites, disabling page cache is not an acceptable option)

Here's an idea that's not fully baked yet, but wanting to get early feedback on:

The current proposed resolution is basically saying "Since we don't have an API to invalidate persisted form state's by anything other than their expiration time, don't put something into there that can become invalid sooner than that." But what if instead we added an API to invalidate when needed: i.e., add tags to KV stores, just like we have for cache? And then entity forms can add the entity uuid to the form state's tags, and invalidate that tag when the entity is saved?

working on #23

Ok, so worked on a test-only patch first.
But I can't make it fail.
Attached is test-only patch, + removal of page-cache FALSE from routing.
What am I missing?

Same patch without the commented out tests.

1. +++ b/core/modules/user/src/Tests/UserRegistrationTest.php
   @@ -150,6 +150,40 @@ function testRegistrationEmailDuplicates() {
   +    // Don't require email verification and allow registration by site visitors
   +    // without administrator approval.
   ...
   +      ->set('register', USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL)
   

   This comment is now wrong, I changed it to use a different mode to try and make it fail, to no avail

2. +++ b/core/modules/user/src/Tests/UserRegistrationTest.php
   @@ -150,6 +150,40 @@ function testRegistrationEmailDuplicates() {
   +    $this->config('system.performance')
   +      ->set('cache.page.use_internal', TRUE)
   +      ->set('cache.page.max_age', 600)
   +      ->save();
   

   Also, I think this shouldn't be needed, but was trying a few things

Ok so the existing tests fail, but the new ones don't - we can work with that

Will poke at #23 during the week

But what if instead we added an API to invalidate when needed: i.e., add tags to KV stores, just like we have for cache? And then entity forms can add the entity uuid to the form state's tags, and invalidate that tag when the entity is saved?

I do not think it can work that way, as other than cache entries the cached form state cannot be use across all requests ever as long as something like the generated uuid value is in there.

@fago asked in IRC if we couldn't find a generic way to fix this problem. But I don't know what that even means. In general, cached entity forms *do* work, just this part doesn't. Even if we find a way to invalidate a form build ID on submission for example, there's still a small risk for a race condition when two submissions happen at the same time, and it might change the expected behavior of other forms, I'm not sure.

What I've been thinking about is fixing it for all dynamic default values, as it does not seem to be unusual to have a dynamic default value and a cached entity creation form... Shouldn't it be enough to only populate the entity in form state on the first request processing input and skip this when the form is rendered the first time? It do not see why would need entity cached when we cache the initial version of the form.

Here is a proof a concept, not tested - shouldn't something like that work?

But... What... how... interesting ;)

Testbot thinks this works... Can we have a patch that combines that with the test-only patches from above, or just remove the no_cache in user.routing.yml?

I don't think it will work for my node/add scenario in #21, though, it's not the first rebuild there?

I'm wondering if that could have unexpected side effects, when default values change after the first submission. Anything that is displays will be used from the form default values then, maybe something that is hidden..

I'm wondering if that could have unexpected side effects, when default values change after the first submission. Anything that is displays will be used from the form default values then, maybe something that is hidden..

If the default value involves a field that's shown on the form, you form becomes uncachable and you should not cache it. If it's not used like UUID, I do not see a problem as long as there is no configuration change.

Actually, I think my proof of concept might not work in the "form state is cached when rendering" case, but we should be able to use the same approach in both cases. Can re-roll and try tomorrow.

#2263569: Bypass form caching by default for forms using #ajax. and #2421503: SA-CORE-2014-002 forward port only checks internal cache aren't mentioned here yet but root cause seems similar to me.

With the failing test

Ok so it doesn't appear to fix the issue?

ok, fixed that problem but turns out the idea does not work out :-( Problem is that when form caching is active the whole form object is repopulated from cache. So when we the form callbacks were introduced, the form object went into form state and gets re-populated from cache. That means $this->entity IS already updated anyway - what renders the current updating of $this->entity useless. :-(

So to fix that we'd have to have access to the original $entity being passed to the form. Sort of weird that $form_state build info does not do that, what make me think this is a bug and the form object shouldn't be updated from cache? Or at least not the one being the form state callback argument - if we change it that way we can easily use this entity to fix the problem.

Updated patch, with still failing tests :-(

#2263569-22: Bypass form caching by default for forms using #ajax. has some additional information for how $form_state works and what could be done about it. Maybe it is helpful :).

Claiming for dev days.

I've been thinking about this and discussing with others.

First, we found a related issue, that would at least help with the node case. In 7.x, we deleted stored form/form_state data after a submission when page cache is not enabled. That somehow got lost I think, we should open an issue to restore it, and what we can actually do is remove all but immutable forms (which should never be submitted anyway). That will help to keep the form_state key value collection under control as well.

Then, I think that generating a UUID as a default value when it might end up in places like form state caches is wrong. Compare it to the uid, which we also generate manually through sequence. We do that in preSave() as well. It would clash otherwise, and the UUID is not directly going to clash, but conceptually, those things are still pretty related I think. And given that the approach that @fago suggested isn't actually going to work unless we make some changes to how we work with form objects*, I'd suggest we go back to #17 for now.

There has been some feedback/discussion between @yched and me since then, about applying the same approach also to config entities, and remove the confusing injected uuid service/generation. Not sure if we should do that here though, I don't know exactly how that will affect config entities.

* We should look into that anyway, because we're currently doing things that don't make sense, like the process method where we assign the entity again, but it is already the same object.

Starting from #17, applied the suggestion in #19, i.e. moved generating from UuidItem::preSave() to ContentEntityBase::preSave().

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -971,6 +983,18 @@ public function label() {
+    return $this;

I don't think preSave() returns anything?

The patch is also missing the removal of no_cache in user.routing.yml.

And some minor thingie.
I *think* we camelcase UUID when using in method names.
But you should check before you change that ;)

testUUIDFormState ==> testUuidFormState

I *think* we camelcase UUID when using in method names.

I think so too, because coder complains about it. I don't find any mention in https://www.drupal.org/coding-standards#naming but let's change it to camel.

Opened #2473759: Form caches should be deleted after submission for deleting form cache entries.

I must say I'm not so fond of the solution in general and would prefer to fix the form workflow instead. But, if that won't work out fast enough I'm fine with adding this instead.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -971,6 +983,16 @@ public function label() {
+    // Ensure that a uuid is generated.
+    $this->uuid();

However, this is a WTF as I'd not expect a getter like this to change the state and set something. If it needs to generate an UUID earlier, I think it should at least not set it? Also else, this would start to fail again as soon as someone calls $entity->uuid() during entity creation what seems totally valid to me.

I'm fine with setting it explicitly, we could move that part into a helper method.

Also else, this would start to fail again as soon as someone calls $entity->uuid() during entity creation what seems totally valid to me.

Yeah.. but if someone does that *and* uses the UUID somehow, then it will be broken with changing the form workflow too, because on submission, it will be a different UUID :)

1. Created the helper function initUuid().
2. Removed EntityFieldDefaultValueTest::setUp because it is not overriding anything from the parent.

I noticed also that ContentEntityBase::createDuplicate() sets the UUID so we should probably remove that.

Could there be a better pattern for this? We have a property (uuid) that should be initialized for some entity types and not others.

The reality is that if there is a helper function to set this, we are extending the API in these cases.

Yeah.. but if someone does that *and* uses the UUID somehow, then it will be broken with changing the form workflow too, because on submission, it will be a different UUID :)

Right, that's the case whatever what we do. But if you do so, you violate page caching and so you'll have to take care of disabling it.

So the problem I see with #51 is that a getter call changes the state of the object, what is not what you would expect? E.g. it would lead to the following WTF:

$entity = EntityClass::create();
$uuid = $entity>uuid->value; // NULL
$uuid2 = $entity->uuid(); // uuid
if ($uuid != $entity>uuid->value) {
  throw new Exception("WTF is going on here?!")
}

I cannot see how a helper method could help to prevent this?

I do agree that calling a getter should not change any internal stored value, especially it feels a bit odd that we special case UUIDs even the problem might exist in other places.

ok, fixed that problem but turns out the idea does not work out :-( Problem is that when form caching is active the whole form object is repopulated from cache. So when we the form callbacks were introduced, the form object went into form state and gets re-populated from cache. That means $this->entity IS already updated anyway - what renders the current updating of $this->entity useless. :-(

Cross-referencing #2242749: Port Form API security fix SA-CORE-2014-002 to Drupal 8. In that issue we discussed putting the form_state storage into $_SESSION. Also in #2263569: Bypass form caching by default for forms using #ajax. we've discussed never writing out $form_state just due to viewing a form.

I think we should consider postponing this on one or both of those issues - if we 1. only wrote to form state storage when actually necessary 2. tied that to sessions, then this issue should just end up duplicate and not require any changes in itself.

Going ahead and postponing this on #2263569: Bypass form caching by default for forms using #ajax..

.

NOTE: once #2263569: Forms using #ajax generate cached form data for every user on every request lands, this issue will be fixed as well :)

I'd vote for still write explicit test coverage, we really should not run into that regression again :)

Agreed with #64. Additionally, since the user/register form includes a picture upload field, this issue won't be fixed until #2500527: Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache is as well.

Once both #2263569: Bypass form caching by default for forms using #ajax. and #2500527: Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache are committed, this can be demoted to major, since nothing in core will trigger the bug.
However, we need test coverage regardless.
That can be worked on now, before the blockers are in.

That said, contrib or custom code could reintroduce this bug if $form_state->setCached() is called by something on the registration page.
@effulgentsia is opening an issue about removing that possibility once and for all.

Note: #2421503: SA-CORE-2014-002 forward port only checks internal cache will also help with that issue, because it will ensure that the form state will not be cached for those forms ....

Extracted the existing test coverage for this issue.

.

The test looks good to me, I assume it will be green once #2263569: Bypass form caching by default for forms using #ajax. is done, correct?

Depends if under the testing scenario there's a picture upload field. See #65.

Adapted the test coverage a bit to make sense.

So the behaviour is still as expected.

#67 unpostponed for tests which this now has. So, repostponing on #2500527: Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache.

This merges #2500527: Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache with the test only patch.

I think it would be interesting to see whether this passes.

Just adapted the issue summary

Alright, let's reupload with out.

+++ b/core/modules/user/src/Tests/UserRegistrationTest.php
@@ -150,6 +153,72 @@ function testRegistrationEmailDuplicates() {
+
+    // Add a picture field in order to ensure that no form cache is added, which
+    // breaks registration of more than 1 user every 6 hour.
+    $field_storage = FieldStorageConfig::create([
+      'field_name' => 'user_picture',

Should we point out that this is a regression test for #2500527: Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache? That won't be explicit form the git history given this is a separate issue.

Also 'no form cache is added' - change to 'no form cache is written' or 'the form structure is not persistently cached'? We're going to break $form['#cache'] altogether soon.

Addressed it.

+++ b/core/modules/user/src/Tests/UserRegistrationTest.php
@@ -155,13 +155,16 @@ function testRegistrationEmailDuplicates() {
+   * This is a regression test for https://www.drupal.org/node/2500527 to ensure
+   * that the form is not cached on the GET requests.

nit - the GET requests => GET requests

Fixed both of those locally. Great that we fixed this properly by doing the other two issues and didn't need to touch entity forms here.

Committed/pushed to 8.0.x, thanks!