Saving to the private tempstore doesn't start a session for anonymous users

This is a quite critical issue as developers assume by default user.private_tempstore works for anonymous by storing something on the $_SESSION (as mentioned on several page either on d.org or stack*.com), but by using it they figure out something is wrong.

Manually tested and patch on #4 works.

We may need test coverage to prove problem is real and patch fixes it?

I'm running into this issue trying to store a value in an anonymous visitor's `user.private_tempstore` from within a custom `hook_form_submit` handler. My purpose is to respond to a form being submitted by firing off some tracking events to Google Analytics, but I only want these events to fire once after the success URL has been reached.

My initial thought was to store this value in $form_state like:

<?php
$form_state->setValue('hidden_field', 'some value');
?>

However the $_POST body doesn't persist because I'm using the core Contact Form and specifying a redirect URL (this is also true for Webforms that redirect). After the form submits and redirects to the success URL, the $_POST values aren't available to me.

<?php
\Drupal::request()->request->get('hidden_field'); // form post is null after redirect
?>

So that's why I was opting to store this value in user.private_tempstore, like this:

<?php
function example_form_submit($form, \Drupal\Core\Form\FormStateInterface $form_state) {
  $input = $form_state->getUserInput();
  if (isset($input) && !empty($input['name']) && !empty($input['mail'])) {
    $tempstore = \Drupal::service('user.private_tempstore')->get('example_tempstore');
    $tempstore->set('form_submitted', TRUE);
  }
}
?>

And then printing out some tracking events in $page_bottom from a template like this:

<?php
function example_page_bottom(array &$page_bottom) {
  $tempstore = \Drupal::service('user.private_tempstore')->get('example_tempstore');

  //This is NULL because a session hasn't been started for the anonymous user.
  if ($form_submitted = $tempstore->get('form_submitted')) {
    $page_bottom['tracking_events'] = array(
      '#theme' => 'tracking_events',
      '#form_submitted' => $form_submitted,
    );

    //Unset this flag after this template is rendered the first time to prevent duplicate tracking events.
    $tempstore->set('form_submitted', FALSE);
    $tempstore->delete('form_submitted');
  }
}

function example_theme($existing, $type, $theme, $path) {
  return array(
    'tracking_events' => array(
      'variables' => array(
        'form_submitted' => NULL,
      ),
      'template' => 'tracking-events-twig-template',
    ),
  );
}
?>

The problem is just what this issue is about... When I check for the value in private temp store in the `hook_page_bottom`, it's NULL because a session hasn't yet been started for the anonymous user who submitted the form.

#4 patch can not be applied after Drupal update to 8.3.1.
Here is reroll patch.

I tested the patch of comment #8 using drupal 8.3.2 and can confirm that i am now able to use the private temporary storage as an anonymous user.

Uploaded the patch again, trying to start the testing process for the patch.

@rackberg: to start the testing process, you have to set the status to "Needs review"

Re-roll of #10 because it's failed to apply on 8.4.x

Thanks, just wanted to do the same :)

Reviewed the patch #14. Looks good to me. +1 to RTBC

This patch doesn't fix the issue for me. I'm trying to do something very similar to what wesleymusgrove is doing in #7.

Patch in #14 worked for me on Drupal 8.3.2. +1 to RTBC

Good idea, hope this helps that this patch lands in 8.4

+++ b/core/modules/user/src/PrivateTempStore.php
@@ -125,6 +125,13 @@ public function set($key, $value) {
+      $_SESSION['forced'] = TRUE;

We've worked actively in the Drupal 8 cycle to remove uses of the superglobals.

The correct way to do this, in a fashion that can be unit tested etc, is to use \Symfony\Component\HttpFoundation\Session\SessionInterface::set, which can be called on the Session object returned from calling getSession on the current request, which can be obtained by calling the getCurrentRequest method on the request_stack service.

As this service already has the request stack injected, we should definitely be using that approach over the superglobals.

Also, needs tests still

Test not added. update the code as per the comment. please review

@harsha012 I believe lerowlan meant using the injected service like so:

$this->requestStack->getCurrentRequest()->getSession()->set('forced', TRUE);

@alexej_d is right. The patch from #22 can't work at all, impossible

@alexej_d,

Thanks for clarification.

added patch for that.

Patch from #26 applies fine and works.

Patch from #26 not working for me on Drupal 8.4.x.

Just trying to do this one line in my controller as an anonymous user:

 \Drupal::service('user.private_tempstore')->get('my_module')->set('ReturnToUrl', $returnTo);

When I refresh the page and do this, I get null:

 dump(\Drupal::service('user.private_tempstore')->get('my_module')->get('ReturnToUrl'));

So this will now always start a session for anonymous? (Also when not desired?)

I am on 8.4.2 and have applied patch from #26 but I am still unable to set and retrieve a variable from the user.tempstore for an anonymous user.

I am setting a variable in my controller (triggered by an AJAX call on a button click):

$tempstore = \Drupal::service('user.private_tempstore')->get('my_module');
$tempstore->set('start_lock', TRUE);

Then on page load, I am trying to get that variable again:

$tempstore = \Drupal::service('user.private_tempstore')->get('my_module');
$start_lock = $tempstore->get('start_lock');

This will return TRUE when I am logged in, and NULL when I am not. Am I missing something?

#26 works for me when manually testing with logged in and logged out users.

Doesn't the new code in patch #26 need to be invoked before the call to createkey()? createkey() calls getOwner(), which in turn grabs the session id for anonymous users and returns nothing if session is not started. Or maybe just put it in getOwner()?

Otherwise, has anyone considered whether there's any way to make this data persist across sessions, e.g. while transitioning from anonymous to logged-in? That seems like the whole point of having a database backend - you don't need to rely on session id. But solving it securely is tricky - while it's a reasonably common use case, the proposition that actions by anonymous users can cause data to be stored in the backend that might affect the experience of logged in users is pretty scary.

Apologies, you could use SharedTempStore for the anonymous -> logged in transition.

Attached a reroll for 26 (without test) + an approach to test.

Test is setting a collection key/value twice, but it fails the first time and I'm wondering if it fails because my test approach is wrong, or it fails because we made a mistake somewhere in here and we haven't noticed the issue still existing on first set() even with current fix.

The status of key_value_expire when running the test is:

Basically because we force the session to start on set(), createkey() calculates the key from an empty/unstarted session. The key is then ":foo" for the first set() call, "xB95NwU_Qqd5OLJbE_enFPrMf6m6_x68phnJQIm63Uo:foo" for all the other calls.

Is that my test or it's actually a problem?

This test is nearly the right approach, but doesn't sufficiently test the condition we are concerned with: the metadata->owner property.
Specifically, this test will currently pass against core, since owner is empty for anonymous session.

Attached test ensures that ->owner is not empty, and that owner matches between subsequent calls.
This test will fail against core, and all the previous patches in this thread.

My next comment will include a patch which satisfies this test.

fixed version of #41

#42-test-only failed as expected against unpatched core.

This patch includes test from #42, and passes it by calling force-session first in PrivateTempStore::set()

This test is nearly the right approach, but doesn't sufficiently test the condition we are concerned with: the metadata->owner property.

Yep, that's was intentional as current fix was still producing a failing test.

I can see you moved the enforcement on set() before anything else. That should be the optimal solution then!

I've got three questions:

1. Do we need to move the session enforcement somewhere else, i.e. in getOwner()? Currently if the user doesn't trigger a set(), but he/she does trigger get() or delete() the query will run against key/value pairs without a owner (with name :key_name)
2. Do we need an upgrade path, removing any key/value without the getOwner() part in the key name?
3. Do we need a Browser test to fully validate this in a proper journey

Updating the Issue summary.

1. Do we need to move the session enforcement somewhere else, i.e. in getOwner()? Currently if the user doesn't trigger a set(), but he/she does trigger get() or delete() the query will run against key/value pairs without a owner (with name :key_name)

if set() has not been called, there will be nothing to get() or delete().
I guess it could go into the constructor, but I don't know - is that a better approach?

2. Do we need an upgrade path, removing any key/value without the getOwner() part in the key name?

I don't think so, because those entries will get cleared out when their dates expire anyway.

3. Do we need a Browser test to fully validate this in a proper journey?

Tests could be improved by initiating multiple anonymous sessions, and ensuring each cannot access the other's data: ie. ensuring unique owner metadata. If that requires a browser test, then yes. If we can do it in kernel test, then we can update this test method.

thank you so much @aaronbauman!

Now, that we have a test, we should hopefully get this committed soon :)

btw, shouldn't we reset the version to 8.5.x-dev?? Because it's a bug, not a new feature

I think since the bug persists to 8.6.x, the protocol is to fix it there, followed by a backport to supported versions (8.5 and 8.4)

right

Sorry to chime in like that, but did anyone test setting the private tempstore for anonymous users with page cache activated? I can recall that my previous tests showed it is impossible to set and read from private tempstore under there circumstances. One would need to introduce a new cache context to vary by. Or is this topic not concerned with caching?

I haven't observed any problems with caching so far

It's possible that page cache or another cache could prevent a request from reaching whatever controller is calling the private temp store.
That can't be part of this issue though, since the private temp store doesn't share any dependencies with the cache layer.

Hey, guys, this is an alternative solution for now:
https://www.drupal.org/project/session_based_temp_store

2. Do we need an upgrade path, removing any key/value without the getOwner() part in the key name?

I don't think so, because those entries will get cleared out when their dates expire anyway.

I was wondering if current core PrivateTempStore was creating for anonymous user keys prefixed by '0:'. But it doesn't.
Key are prefixed with a session ID, but session is not maintained so each request has different session IDs.

Updating the IS to mention this bit.

I contributed at the code so I don't feel being a good candidate for final review. :(

Now that I'm watching the test, I think we are using the wrong approach.

When I try #42 locally it work. In fact what test coverage so far does is setting and getting values in the same request.
It's good to have this aspect covered, however what this issue is trying to achieve is having PrivateTempStore maintained for anonymous users across their requests/session.

So we still need to cover the bit where anonymous user PrivateTempStore is set from a request, updated in a new one and this doesn't create 2 different key_value_expired records (unless session is cleared).

Setting Needs Work for this reason.

Yes, i suppose it makes sense to test across requests.
The easiest way to accomplish this test is probably with a webtest then.
Not sure if i'll be able to get to this soon, so anyone please feel free to jump in.

Thank you for updating the summary - much more clear now

OK, I've identified an appropriate place to inject a web test for this:

Node preview relies on private temp store, and straight up doesn't work for anonymous users.
This use case is not tested.

I'm working on new test for coverage of node preview for anonymous to demonstrate this bug.

Found this older issue #2703247: Previewing a node as an anonymous user results in a page not found error
Closing this as a dupe and will post another update there.

On second thought - this thread has more contributors and a more recent activity.
Closed other thread and re-opened this one.

Here are all the core features that rely on PrivateTempStore, which will fail for anonymous users (from most critical to least):
- Node Preview
- Quickedit
- Delete multiple (from admin/content)
- Cancel user multiple (from admin/people)

I've confirmed myself in browser that node preview and quickedit fail.
Oddly, I can't get the webtest to fail.

I assume this has to do with some internal mechanism of the testing framework, but haven't had time to delve into that.
I'm posting the test I've got so far, which fails in an interactive user session but passes in simpletest.
Maybe it will work (ie. expected fail) on testbot???

Update: it passed on testbot, even though we expected it to fail.
Something odd is happening here which requires deeper knowledge of the testing framework and session management.

Thanks to @berdir i figured out the problem - the dependency on node_test module was causing a session to be created, which forced the private temp store into working, invalidating the observed behavior.

Attached patch is another test-only, and expected to fail.
After this, i'll post test + a patch that satisfies it.

In this patch:
- The fix for PrivateTempStore to address failing test in #64
- The webtest from #64
- The kernel test from #42 + #45
- Remove extraneous use statements from AnonymousPrivateTempStoreTest

interdiff is between this patch and #45

To address this task:

Understand if the session enforcement need to happen on creating the tempstore instance

I think forcing session creation on set() is sufficient, and we do not need to enforce on __construct()
If set() has not been called, there will be nothing to get() or delete().
Furthermore, moving session enforcement to __construct() would create noise for any session which relies on the temp store service, but doesn't actually set any value. By keeping it in set(), we're only saving session data when requested explicitly.

Also updated summary.

+++ b/core/modules/node/src/Tests/PagePreviewAnonymousTest.php
@@ -0,0 +1,58 @@
+    $edit = [];
+    $edit[$title_key] = '<em>' . $this->randomMachineName(8) . '</em>';
+    $edit[$body_key] = $this->randomMachineName(16);

This can be simplified to setting the keys during the initialisation of the var.

Also not sure why we need the <em> tags around the title?

+++ b/core/modules/node/src/Tests/PagePreviewAnonymousTest.php
@@ -0,0 +1,58 @@
+class PagePreviewAnonymousTest extends NodeTestBase {

This test should probably be extending \Drupal\Tests\node\Functional\NodeTestBase instead since this one is being deprecated.

OK, actually quite a bit of cruft to cleanup here, and I moved the test into the non-deprecated set of test in the Functional directory.
Since this implied a good chunk of changes, I'm re-submitting the test only to show that it fails.
Then i'll re-submit the with test + patch

- Basically the same test but namespace is now Drupal\Tests\node\Functional (this breaks the interdiff)
- Updated comments
- Removed extra 'text' module dependency
- Removed unnecessary 'edit content' permission
- Updated all assertion methods appropriately

Rolled #68 without the tests against 8.4.5

Patch from #26 not working for me on Drupal 8.5.x.

Just trying to do this one line in my controller as an anonymous user:

\Drupal::service('user.private_tempstore')->get('my_module')->set('ReturnToUrl', $returnTo);

When I refresh the page and do this, I get null:

dump(\Drupal::service('user.private_tempstore')->get('my_module')->get('ReturnToUrl'));

Integrating patch from https://www.drupal.org/project/drupal/issues/2703247

Hey @akalam , I'm not sure what your patch is trying to integrate?
If you mean to add anything to #68 please elaborate a bit more and may be add an interdiff?
That would help other people trying to understand the contribution a bit more. :)

Otherwise/Currenlty correct patch is still the one in #68.

Hey, so, I'm kinda new to contributing, so I apologize if I'm breaking the rules :-| ... or if this doesn't work 8-[

I was kinda in the same boat as #70 - stuck on 8.4.x until I could get this resolved - so here's a patch based on the 8.4.x patch from #26 to get Drupal 8.5.x to work. It doesn't address any of the concerns since comment #26.

@koryp: the current version of the patch is #68 (or #69, if you wanna skip the new test), so why try #26?

You should try the newest one first. If that doesn't apply, which is possible of course, then you'd help us the most, if you would re-roll patch #68 instead. thanks! :)

@agoradesign It looks like the patch in #69 targets 8.4.5 (though didn't include the test) and is very similiar to #26, but it appears to initiate the session in the "get" method instead of the "set", so it's a different approach ... not sure if that would work in my case.

The other patches since #26 (including #68) have targeted 8.6.x and excluded the 8.5.x tests... A quick glance at the recent 8.6.x patches show the addition of a couple new test files. I don't know if those are compatible with 8.5.x, but assumed they were not since the 8.5.x test was not included. TBH, I didn't try them and assumed from your comment in #49 and also the more recent comment from #70 that others had done so with no success. I apologize if the latest 8.6.x patch does indeed work for 8.5.x and I've led people astray...

Also, sorry, I assumed my uploaded patch file would be auto-renamed to align with my post # ... don't know if there's a way to rename it now... or, if it turns out #68 or #71(!?) work for 8.5.x, maybe my post #73 can just be wiped from existence... ;-p

#68 will apply cleanly to >= 8.5.0, but will need to be backported for < 8.5.0 (assuming this gets committed while 8.4 is still in LTS)
PrivateTempStore class namespace was moved from User to core between 8.4 and 8.5, and the service id changed as well.

Generally, the backporting gets done only after the patch against latest dev is committed, so that we're not trying to maintain N different versions of the same patch in parallel

I just want to point out that 8.4 is not an LTS. There are no ordinary bugfix releases (the kind that would include a normal priority bug fix) of 8.4.x scheduled.

Let’s get back the current patch needing review.

Reviewing #68. Can we try to keep the patch for 8.5.x/8.6.x as the most current since that is the one that will be committed. I think this is a major bug. It is possible to configure Drupal in a way that breaks and you get data loss.

1. +++ b/core/modules/node/tests/src/Functional/NodePreviewAnonymousTest.php
   @@ -0,0 +1,63 @@
   +    // Create Basic page node type.
   +    if ($this->profile != 'standard') {
   +      $this->drupalCreateContentType([
   +        'type' => 'page',
   +        'name' => 'Basic page',
   +        'display_submitted' => FALSE,
   +      ]);
   +    }
   

   You this can be

         $this->drupalCreateContentType([
           'type' => 'page',
           'name' => 'Basic page',
           'display_submitted' => FALSE,
         ]);
   

   there's no need for the profile check. The tests that have this are legacy tests from back when we used the standard profile for testing.

2. +++ b/core/tests/Drupal/KernelTests/Core/TempStore/AnonymousPrivateTempStoreTest.php
   @@ -0,0 +1,60 @@
   +   * Tests anonymous can get/set to his PrivateTempStore.
   

   No need for gendered language. Could be
   Tests anonymous users can use the PrivateTempStore.

Needs work for #79

re-roll of #68 per #79

@aaronbauman thanks for rolling the patch looks good! Any chance of a test only patch too because whilst you added one before (nice) - it didn't have the kernel test.

Kernel test-only is in #42
Hasn't changed except for the comment

For good measure, here are both tests from #81 in a test-only patch.
These should both fail.

re-posting #81 so that this issue can be back in needs-review

removing needs-tests since tests are now included

Let's fix this

Argh, I spent few hours before finding this issue.
Patch #87 works fine for me.

Same for us ! (Patch #87 works fine)

+++ b/core/lib/Drupal/Core/TempStore/PrivateTempStore.php
@@ -117,6 +117,13 @@ public function get($key) {
+      $this->requestStack->getCurrentRequest()->getSession()->set('forced', TRUE);

Sorry I should have spotted this earlier. This is quite a generic key. Let's namespace it a bit just to be sure that there are no unintended clashes. I suggest core.tempstore.private

Makes sense.
Re-roll attached, and interdiff against #81

+++ b/core/lib/Drupal/Core/TempStore/PrivateTempStore.php
@@ -117,6 +117,18 @@ public function get($key) {
+      // start session API rather than setting an arbitrary value directly.

Real nit but according to https://www.drupal.org/docs/develop/coding-standards/api-documentation-a... this line needs to be indented.
So

//   start session API rather than setting an arbitrary value directly.

That's a nit alright.
I guess I pulled from one of the many instances in core that ignore this standard.

re-rolled, interdiff against 93

@joachim the majority of inline @todos I know of follow this standard /me shrugs.

Crediting reviewers who left comments that altered the direction of the patch.

Adding people who worked on the duplicate issue #2703247: Previewing a node as an anonymous user results in a page not found error

Committed 9fbba7e and pushed to 8.6.x. Thanks! Marking for backport to 8.5.x

Fixed some more coding standards on commit.

FILE: ...e/modules/node/tests/src/Functional/NodePreviewAnonymousTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 27 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found
    |       |     5
 61 | ERROR | [x] The closing brace for the class must have an empty
    |       |     line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

diff --git a/core/modules/node/tests/src/Functional/NodePreviewAnonymousTest.php b/core/modules/node/tests/src/Functional/NodePreviewAnonymousTest.php
index 6f7e4ab2ac..66660f9507 100644
--- a/core/modules/node/tests/src/Functional/NodePreviewAnonymousTest.php
+++ b/core/modules/node/tests/src/Functional/NodePreviewAnonymousTest.php
@@ -24,7 +24,7 @@ class NodePreviewAnonymousTest extends BrowserTestBase {
    * {@inheritdoc}
    */
   protected function setUp() {
-     parent::setUp();
+    parent::setUp();
     // Create Basic page node type.
     $this->drupalCreateContentType([
       'type' => 'page',
@@ -58,4 +58,5 @@ public function testAnonymousPagePreview() {
     $this->assertSession()->responseContains($edit[$body_key]);
     $this->assertSession()->titleEquals($edit[$title_key] . ' | Drupal');
   }
+
 }

#95 applies cleanly to 8.5.x, but here's a reroll with the coding standards from #105

interdiff against #95

Thanks @aaronbauman - I’ll cherry pick the 8.6.x commit once 8.5.x is open - it is frozen atm because of the recent security fix

@catch has already cherry-picked #2932606: Use PHPUnit 6 for PHP 7.0 / 7.1 testing and #2912353: Handle menu_items related to Drupal 6 and 7 node translations with different IDs to 8.5.x earlier today so I don't think it is frozen anymore.

What version will this patch be rolled into? I have 8.5.3 installed and it still doesn't seem to be fixed. Will this be in 8.5.4?

I'm running into issues in drush with null sessions just today, attempting to track down the commit and I'm using 8.6.x-dev so I think it's part of this issue @marcaddeo

This is included in 8.5.4
https://github.com/drupal/drupal/blob/8.5.4/core/lib/Drupal/Core/TempSto...

sorry if I disturb the thread with a backport, but I need this patch for CI (8.4.x)