Problem/Motivation

Private tempstore should store a value that persists with the user's session.

However, if the user is anonymous, then this is broken, because saving to tempstore doesn't start a session.

You can see the problem by running this code (eg in Devel's exec PHP form). Each execution causes a new row in key_value_expire:

\Drupal::service('user.private_tempstore')->get('test')->set('test', 'test value!');

The expected behaviour is that after the first row is inserted, subsequent executions just update it.

Proposed resolution

Ensure that an anonymous user has a session created.
There is no current way to make initialise a session other than storing something in it, so best option is:

if ($this->currentUser->isAnonymous()) {
  $this->requestStack->getCurrentRequest()->getSession()->set('forced', TRUE);
}

This snippet will be placed as first thing in \Drupal\Core\TempStore\PrivateTempStore::set(), so when getOwner() will be called - by createkey() - a session always exists and its ID will be returned and used.

Remaining tasks

  • Prove the bug exists with a valid failing test #41
  • Work on a fix #45
  • Understand if the session enforcement need to happen on creating the tempstore instance
  • Understand if we need a Browser tests too
  • Review && RTBC

User interface changes

No.

API changes

No. Setting a private tempstore value now force a session to be opened. However this should be the expected behaviour, so that is not technically a change?

Data model changes

No.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

joachim created an issue. See original summary.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joachim’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Active » Needs review
FileSize
658 bytes

Here's a patch.

Going by http://drupal.stackexchange.com/questions/163142/what-is-the-right-way-t..., the best way to forcibly start a session for an anon user is to store something in it.

gambry’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

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?

joachim’s picture

This is just a quick hack; I think if we're adding test coverage it would be better done as part of providing a cleaner way of doing this: #2865991: provide an API to forcibly start a session.

wesleymusgrove’s picture

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.

DedMoroz’s picture

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

rackberg’s picture

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.

rackberg’s picture

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

agoradesign’s picture

Status: Needs work » Needs review

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

The last submitted patch, 8: drupal-anonymous_session-2743931-8-D8.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: drupal-anonymous_session-2743931-8-D8.patch, failed testing.

Yogesh Pawar’s picture

Status: Needs work » Needs review
FileSize
659 bytes

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

agoradesign’s picture

Thanks, just wanted to do the same :)

Dinesh18’s picture

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

stijndmd’s picture

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

mrkdboyd’s picture

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

joachim’s picture

Status: Needs review » Reviewed & tested by the community

Ok, that's plenty of positive reviews -- let's set this to RTBC so it can get attention from a maintainer.

agoradesign’s picture

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

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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

harsha012’s picture

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

harsha012’s picture

Status: Needs work » Needs review
alexej_d’s picture

Status: Needs review » Needs work

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

$this->requestStack->getCurrentRequest()->getSession()->set('forced', TRUE);
agoradesign’s picture

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

harsha012’s picture

Status: Needs work » Needs review
FileSize
702 bytes

@alexej_d,

Thanks for clarification.

added patch for that.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ricovandevin’s picture

Patch from #26 applies fine and works.

Filozofer’s picture

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'));
joachim’s picture

I don't understand what your code is trying to do.

Rob C’s picture

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

joachim’s picture

> So this will now always start a session for anonymous?

Yes, it should.

> (Also when not desired?)

If you don't want to start a session, then why try to store something for an anon user that will not persist to the next page load?

mrupsidown’s picture

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?

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

acbramley’s picture

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

aaronbauman’s picture

Status: Needs work » Needs review
gbirch’s picture

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.

gambry’s picture

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:

collection name value expire
tempstore.private.anonymous_private_temp_store :foo O:8:"stdClass":3:{s:5:"owner";s:43:"xB95NwU_Qqd5OLJbE_enFPrMf6m6_x68phnJQIm63Uo";s:4:"data";s:3:"bar";s:7:"updated";i:1519058497;} 1519663297
tempstore.private.anonymous_private_temp_store xB95NwU_Qqd5OLJbE_enFPrMf6m6_x68phnJQIm63Uo:foo O:8:"stdClass":3:{s:5:"owner";s:43:"xB95NwU_Qqd5OLJbE_enFPrMf6m6_x68phnJQIm63Uo";s:4:"data";s:3:"bar";s:7:"updated";i:1519058497;} 1519663297

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?

The last submitted patch, 39: 2743931-39.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

aaronbauman’s picture

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.

aaronbauman’s picture

fixed version of #41

The last submitted patch, 41: 2743931-41-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 42: 2743931-42-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

aaronbauman’s picture

#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()

agoradesign’s picture

Status: Needs work » Needs review
gambry’s picture

Issue summary: View changes

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.

aaronbauman’s picture

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.

agoradesign’s picture

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

aaronbauman’s picture

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)

agoradesign’s picture

right

alexej_d’s picture

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?

agoradesign’s picture

I haven't observed any problems with caching so far

aaronbauman’s picture

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.

nortmas’s picture

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