Steps to replicate:

- enable ctools_wizard_test
- Open one browser, let's say Firefox, log in as admin, and navigate to /ctools/wizard
- Enter "foo" on form one and click next
- Open another browser, e.g. Chrome, not logged into that site, and navigate to /ctools/wizard
- Observe "foo" is prepopulated

Steps to fix:

I believe this can be simply fixed by swapping uses of SharedTempStoreFactory for PrivateTempStoreFactory.

The best illustration of this is the attached screenshot from my database. I have been testing this with two wizards: WizardTest is the ctools_wizard_test module, otherwise unaltered, offerdoc is something a bit more elaborate I have extended from FormWizardBase.
screenshot from my database

You can immediately see that with tempstore.shared only a single row is created per wizard, with the name *wizard_machine_name*, whereas with tempstore.private, one row is created per-wizard-per-user, name *user_id:wizard_machine_name*, which, unless I very thoroughly misunderstand the intention of the WizardBase classes, is clearly the expected and desired behaviour. (1:WizardTest is me-as-admin, uid 1; the long hashes represent me-as-anon sessions in my second browser)

Although the tempstore.shared collections do include an 'owner' key there is only one store for everyone. When user t1lD8O... comes along and the Wizard code requests the contents of shared tempstore with the id WizardTest, they are provided with the contents of tempstore as provided by user 1. Although that tempstore data contains the key owner:1, per the comments at the top of SharedTempStore.php in core,

"It is the responsibility of the implementation to decide when and whether one owner can use or update another owner's data."

The ctools / wizard code does not seem to take on this responsibility. It could, for example, use $tempstore->getIfOwner() and corresponding setter to ensure that user t1lD8O... does not see/set the contents of the tempstore previously set by user 1, but we see e.g. ->set rather than ->setIfOwner on line 287 of FormWizardBase. In other words, all users completing the same wizard will share the same cache of values. As they progress through the wizard, they will merrily read and overwrite each others cached values.

So far simply changing Shared to Private within the Wizard code seems to have created the desired behaviour of giving each user their own set of cached values, however as yet I haven't changed TempstoreConverter.php and I suspect sooner or later I will run into a mismatch between classes here. However I'm less confident changing them there because I have no idea what other aspects of ctools besides wizards (which is all I've looked at so far) are relying on it, and perhaps they definitely need Shared tempstores. So perhaps this needs to be refactored to handle either/both...

This is where I would ideally just attach a .patch file, however I cannot conveniently do that because it needs to fix "only this issue and nothing but this issue". However meanwhile all the uses of SharedTempStoreFactory are from the deprecated \User\ namespace and not the \Core\ namespace. As per

https://www.drupal.org/project/ctools/issues/2936263#comment-12726840

I have patched this, however that would mean any new patch I make for this issue would included changes not strictly relating to this issue (change of namespace for all TempStores including those not relating to Wizards), OR, if I rolled back those changes and made a patch that only changed Shared to Private without addressing the deprecated namespace, I would be making a patch that introduced deprecated code, OR, if I both Shared->Private and de-deprecated in this patch but for Wizards only I would be knowingly ignoring all the other non-Wizard deprecations, and that is also silly, so basically... the deprecations need replacing first, be that via my other patch, or someone else's better job of it, and then I can meaningfully roll a patch for this one.

Comments

stevekeiretsu created an issue. See original summary.

daniel korte’s picture

Status: Active » Needs review
StatusFileSize
new3.06 KB

It appears the module no longer uses the deprecated \User\ namespace.

daniel korte’s picture

StatusFileSize
new5.76 KB

Applied to WizardFormController.php and EntityFormWizardBase.php too.

thalles’s picture

Issue summary: View changes

How can I enable ctools_wizard_test?

gonssal’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

This is actually a severe bug that causes any multi-step form extending the wizard to behave absolutely erratically when being used by more than one user at the same time.

Not only that, but this has the potential to leak private information if you are showing information posted on earlier steps during the process, because you can see data posted by other anonymous users.

Should be fixed ASAP.

Tested the patch and works, thanks for it.

thalles’s picture

Status: Reviewed & tested by the community » Needs review
StatusFileSize
new6.79 KB
new1.31 KB

Follow a new patch replace the services.

thalles’s picture

StatusFileSize
new1.31 KB
thalles’s picture

gonssal’s picture

@thalles why did you set it to Needs review again? The intial patch works for me, if it's not working for you maybe you should explain why? I tried your patches and I get fatal errors out of both of them.

berdir’s picture

Status: Needs review » Needs work

#2 had test fails too, so that isn't ready either.

osman’s picture

Status: Needs work » Needs review
StatusFileSize
new9.47 KB

Here's my attempt to address this issue.

daniel korte’s picture

Status: Needs review » Reviewed & tested by the community

#11 looks good to me and tests pass!

sagannotcarl’s picture

#11 didn't apply for me because of a change in EntityFormWizardBase->getParameters(). Here is another patch with that fixed.

Sorry my mistake. I was using the stable version not dev.

sagannotcarl’s picture

chris matthews’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
Checking patch src/Controller/WizardFormController.php...
Checking patch src/Wizard/EntityFormWizardBase.php...
Checking patch src/Wizard/FormWizardBase.php...
error: while searching for:
use Drupal\ctools\Ajax\OpenModalWizardCommand;
use Drupal\ctools\Event\WizardEvent;
use Drupal\Core\TempStore\PrivateTempStoreFactory;
use Drupal\Core\TempStore\SharedTempStoreFactory;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;

error: patch failed: src/Wizard/FormWizardBase.php:14
error: src/Wizard/FormWizardBase.php: patch does not apply
Checking patch src/Wizard/FormWizardInterface.php...
Checking patch tests/modules/ctools_wizard_test/src/Form/ExampleConfigEntityExternalForm.php...
baikho’s picture

baikho’s picture

Status: Needs work » Needs review
sagannotcarl’s picture

Status: Needs review » Reviewed & tested by the community

#16 tests pass and fixes the original problem for me.

sagannotcarl’s picture

Issue tags: -Needs reroll
akalata’s picture

Status: Reviewed & tested by the community » Needs work

Work needed to fix test failures.

japerry’s picture

Status: Needs work » Needs review
StatusFileSize
new10.52 KB

Lets try this re-roll and see what tests do.

japerry’s picture

Status: Needs review » Fixed

Since 8.x-3.x doesn't support 9.4 from a testing perspective, this is good.

  • japerry committed f243a85 on 8.x-3.x
    Issue #2992362 by thalles, Daniel Korte, sagannotcarl, baikho, osman,...

  • japerry committed b551bd7 on 4.x
    Issue #2992362 by thalles, Daniel Korte, sagannotcarl, baikho, osman,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

rudi teschner’s picture

This change is causing an error in page_manager that makes in unable to edit pages. And it probably affects more then just that module, so I'm adding that information in this issue.

Error: Call to a member function label() on null in Drupal\page_manager_ui\Controller\PageManagerController->editPageTitle() (line 110 of modules/contrib/page_manager/page_manager_ui/src/Controller/PageManagerController.php).

https://www.drupal.org/project/page_manager/issues/3052342

japerry’s picture

Status: Closed (fixed) » Needs work

Re-opening issue, and reverting in the 3.x branch. Need to have a larger discussion about private vs shared tempstore. Honestly though, shared tempstore is what we want to support here, similar to views. What is missing is the 'locking' mechanism that warns users that someone else is in the form.

  • japerry committed d9f3228 on 8.x-3.x
    Revert "Issue #2992362 by thalles, Daniel Korte, sagannotcarl, baikho,...

  • joelpittet committed d8b60dd on 4.0.x authored by japerry
    Revert "Issue #2992362 by thalles, Daniel Korte, sagannotcarl, baikho,...

  • joelpittet committed 7f8b4d5 on 4.0.x
    Partial revert to align with 8.x-3.x "Issue #2992362 by thalles, Daniel...
vmarchuk’s picture

New patch compatible with D9.3 and Ctools 8.3.7

rszrama’s picture

(Just a note that's Ctools 8.x-3.7, which is the version currently installed a project of ours.)

japerry’s picture

Category: Bug report » Feature request
StatusFileSize
new10.54 KB

This is a not a bug, but rather a feature request. However, there is a legitimate need for Private Tempstore, as well as a locking mechanism for the shared tempstore. This ticket should be morphed into supporting both use cases.

In the meantime, for projects needing private tempstore, this is a reroll against 8.x-3.13. The previous patch doesn't apply to the latest version of ctools.

baikho’s picture

Status: Needs work » Needs review

  • japerry committed 190227f2 on 4.x
    Revert "Issue #2992362 by thalles, Daniel Korte, sagannotcarl, baikho,...

  • japerry committed a72103e9 on 4.x
    Issue #2992362 by thalles, Daniel Korte, sagannotcarl, baikho, osman,...

  • japerry committed 190227f2 on 4.1.x
    Revert "Issue #2992362 by thalles, Daniel Korte, sagannotcarl, baikho,...

  • japerry committed a72103e9 on 4.1.x
    Issue #2992362 by thalles, Daniel Korte, sagannotcarl, baikho, osman,...