Drafts for anonymous users

Hello :)

Yes, I'll offer a patch and you will have the final word on it :) Though I'll defend my position fiercely :)

Let me actually start defending it:

• Where does exactly your security concern go? My proposal is based on the session ID, so each anonymous draft will have a session ID and will be visible only to the client with the same session ID (or to users with "view any webform submission".. or any other "super user" permission)
• Massive amounts of data and DB performance are valid concerns, of course. But I think at least some subset of users will benefit from this feature whereas those who will suffer from DB performance always can disable it and things will be back to normal. And as in #1932560: “Save draft” form button for anonymous users suggested, probably the next logical feature request is to implement auto-purging

• Using a temporary storage table for anonymous drafts could help. Temp table could store serialized data.

  I am rather biased towards using standard WebformSubmissionStorage facilities, i.e. {webform_submission} and {webform_submission_data} that way I do not have to worry about actually saving (storing in a permanent storage) drafts, and instead will simply reuse an already existing solution. Additionally, in the face of coming views integration, there are additional benefits of reusing the existing storage machinery. Additionally, if the registered user drafts are saved that way, why not saving the anonymous drafts too? I think keeping both drafts aligned will save some pain in the future since the storage implementation will be equal.

• The token column in the webform_submission_data table is intended to store a secure ID similar to a session ID

  Thanks for the heads up, I'll check whether it's possible to leverage those tokens.

• Might be able to implement this using a WebformHandler, which means this could be a dedicated module.

  As I said, I would prefer going through the WebformSubmissionStorage route, but otherwise yes, probably a new handler is the best alternative option.

I am not a real fan of TDD, so I'll rather try hammering out the functionality, but when the functionality is pretty stable and I have more or less your approval/support on the implemented approach, I'll see into the tests :) Actually, I also have a rule of thumb of not releasing a stable (anything above *-rc) code without reasonable test coverage in the modules I maintain :)

Hello!

Let's see what I got so far. In fact the patch is simple as ~ a dozen of line changes.

The changes are the following:

1. Change the logic of WebformSubmissionForm::draftEnabled() so now anonymous users are allowed to save a draft, i.e. show that "save draft" button on the form UI.
2. In WebformSubmission::preSave(), if a) it's a draft b) the owner is anonymous user then pull up user private temporary storage (it's a Drupal service that seems to execute the function of temporarily storing some adhoc data on per-user basis) and save the webform submission token into it. This way we kind of mark "this user" and "this webform" as belonging together.
3. In WebformSubmissionStorage::loadDraft(), if we are loading the draft for anonymous user, pull up the private storage and fetch all previous drafts (tokens of those drafts). Then simply add one extra condition on the query to filter by those tokens.

And that's about it. Pretty simple and elegant, hehe, at least it appears this way to me.

Some things that I find worthwhile to bring up:

• Apparently, since in the case of anonymous users the whole process is "cooked" on session ID, we can only load and save drafts for "current drupal user", since we need the session ID (and not only UID) to get going.
• This user private temporary storage works fine with anonymous users, i.e. it does distinguish 2 different anonymous sessions - I've tested it myself.
• While coding, I felt like my current solution is rather "dirty", because the inner logic of storing "this user and this draft" data is not encapsulated anywhere - you will find very similar code related to user private temporary storage both in WebformSubmissionStorage and in WebformSubmissionForm, which I find to be ugly. The only "beautiful" solution that I can think of is to encapsulate this crap into WebformSubmissionStorage class. I would introduce a couple of methods: AddAnonymousDraft(WebformsubmissionInterface $entity) and getAnonymousDrafts(). These 2 methods would "manage" actual storage of "this user - this submission" data via the very same user private temporary storage, but now it would be encapsulated into 1 single class and only into 2 methods. Then Webformsubmission won't suffer any change (against current -dev) and I would just add extra logic necessary upon webform submission saving into WebformSubmissionStorage::doPreSave() or ::doSave().
• Also, seems like the same logic can be leveraged afterwords for something like "allow anonymous users view their own submissions" kind of permission, since we will already have all the necessary machinery already in-place. Just a thought, hehe.

I did some rather simple testing and it worked pretty nice. Let's see what testbot says (as I told you in another issue, for some reason I can't run webform tests locally). Also, a question about tests: do you mind if I try writing the tests to cover this new feature based on Drupal\Tests\BrowserTestBase instead of Drupal\simpletest\WebTestBase? The former comes from PHPUnit whereas the latter is SimpleTest-based and Drupal 8 seems to prefer PHPUnit over Simpletest (https://www.drupal.org/node/2469723)

Just a small additional iteration on top of previous patch after some testing. I explicitly set condition to be IN when loading drafts of anonymous users.

I couldn't find issues about the anonymous draft saving with the limited testing I've executed. So I "clean up" the patch by doing necessary dependency injections.

I am trying to come back to this issue now and to wrap it up too.

With the #2843400: Automatic purging of webform submissions being committed, should we consider the "We need to allow admins to delete unfinished anonymous forms and more." sub-task from this ticket finished? Or you want to do something more about webform submission purging in the context of anonymous drafts?

I have a theoretical question about "We may need to migrate anonymous submissions to logged in ones." What exact behavior are we to code? Let's consider a case:

• Anonymous guy opens a webform and saves an anonymous draft
• He keeps surfing the website
• At some point he logs in as some registered user (uid 10)
• [Are we supposed to make his anonymous draft from above now associated with UID 10? - The logical answer is "yes".] Well, it does sound quite obvious but I prefer to double-check it with you :)

I started working on your points (+ I rerolled the patch from #10). Do not look into it unless you're really curious, it's still work-in-progress.

This is a worthwhile patch. It has the following features:

• no drafts
• only logged in
• both logged in & anonymous drafts
• the anonymous drafts are "carried out" to the logged in user upon a user login, i.e. when I saved a draft in anonymous session and then logged in, now my draft is associated with my UID

A note: eventually I have to discard usage of PrivateTempStorage (a fancy tool for storing some temp data related to a specific user) because it was impossible to carry drafts through login - upon login Drupal regenerates session ID and PrivateTempStorage uses session ID as an identifier identifier so after login it was impossible to recognize which drafts were created in the same session as the session was actually destroyed and replaced with a brand new one. I've included a comment on this matter in the source code. So I decided to use pure-bone $_SESSION, the good old one :) and no fancy wrappers.

Known issues:

• the anonymous page cache seems to conflict - I still have to investigate it in further details, but seems like Drupal's anon page cache creates a cache of the webform form page and the anonymous user cannot "get through" that cache in order to see his draft pre-filled in into the form.
• Needs some tests to cover this new feature.
• Needs hook_update() to migrate all existing *.yml webform configs to the new setting of drafts (it used to be a boolean and now it's a 3-way string)

From now on I'll be posting interdiffs since most of this feature is now coded.

The problem with page cache is solved for anonymous drafts. Finally I've had a good opportunity to get to know the dynamic caching of D8 in details.

The problem can be described as following: the webform submission form must property report its cache tags and contexts. Webform submission storage, in theory, is an interface (though hardly anyone will ever do another implementation of it), nonetheless, since it's an abstract interface, the WebformSubmissionForm may not make any specific assumptions about how drafts are actually stored and based on that data to update it's cacheability metadata.

Therefore I've introduced a new method in the WebformSubmissionStorageInterface, it's ::draftCacheabilityMetadata() and it returns cache metadata that should report how draft storage varies.

The actual implementation of webform submission storage stores the anonymous drafts in $_SESSION so we return a session context in the implementation of that method.

Now the WebformSubmissionForm, if drafts are enabled, can invoke WebformSubmissionStorageInterface::draftCacheabilityMetadata() and merge the result with its own cacheability metadata. As a result the webform submission form additionally varies by session which is the desired outcome of this whole architecture I am describing now.

As a bonus... I've adjusted WebformSubmissionStorage::postLoad() so all anonymous draft entities contain their full cacheability metadata (all anonymous drafts cacheability metadata is enriched by session context).

Tested manually in different set ups and all of it worked just fine.

Now I'd like to ask you to review the patch. I basically believe the functionality is written and I can proceed to the automated tests, but before I invest my time into it, I'd like to verify with you whether the implementation looks good (and anyway another pair of eyes won't do hart to make sure I did not miss anything).

.. just a quick note for myself, while I have it in mind: the same draft may be saved more than once, so we must make sure $_SESSION contains only unique submission tokens.

jrockowitz, yes, of course I'll review and will enrich it with some extra tests :) I just need some time to get straight my things and I'll come back here.

Yes, I see your logic. Basically we'd like to retain as much of control of this abstract storage for tokens as possible, so we can remove the tokens of purged drafts. Don't take it as I argue against it, just let me share a few cents about the dirty and naked reality.

Originally I wanted to use the PrivateTempStorage. I believe with some hacks we could remove tokens of purged drafts if we used that storage. But it has one problem, the one I described above. With this storage we can't pull an anonymous draft through a log in and make it then belong to the logged in user. Basically, due to security concerns upon login Drupal regenerates session ID and then simply copies old (anonymous) $_SESSION into the new (logged in) $_SESSION. The private temp storage uses session ID as its bin identifier, so we effectively lose all the connection between anonymous PrivateTempStorage and logged in PrivateTempStorage due to this session regeneration although it's the same HTTP client. I've studied the Drupal core around log in operation and couldn't find a way to connect those 2 session IDs. If we could recognize that the session has just regenerated, we could take the content of old bin and put it into the new bin. I've studied the Drupal code around log in operation and couldn't figure out any "pretty" way to do it. Maybe you can find it?

As far as I know PrivateTempStorage is the only suitable kind of storage that Drupal offers out of the box. Of course, we could write some kind of custom storage for this purpose, but I see it as overkill and simply unreasonable amount of work for the benefits (and the cost of its future maintenance) it implies.

So I "downgraded" the solution to storing tokens in $_SESSION out of these reasons. As I said, I believe I understand what you mean when you say "seems wrong". After all, it's your module, so you decide and you assume all the results of the decision as a module maintainer.

My thinking regarding $_SESSION ending up to contain too many useless tokens:

• Yes, it sucks. No way it wouldn't.
• At least we do not waste HTTP traffic sending back and forth the "useless" tokens since they are stored in session and not in cookies or browser's local storage or anywhere else on the client.
• Hardly one single session will contain more than hundreds of "useless" tokens - hardly a legitimate anonymous user will make more then a few hundreds of drafts on a website. If it's a spam bot or some kind of machine, then we can also say that it's not our problem - such Drupal instance should leverage CAPTCHAs or some other kind of defense and we could assume such concerns are out of the scope of the webform module.
• Since we are talking about the scale hundreds, DB won't suffer any significant performance downgrade when looking up drafts. Nor will PHP.
• After all, there is also session garbage collector in PHP. So we are kind of backed on this side too - we will not leave "long tails" of dead sessions and thus dead tokens.

I am trying to say while this is not the best possible solution, it seems to me "just good enough". I tried to encapsulate the storage for anonymous draft tokens in the code as much as possible so if tomorrow there is a better alternative, it should be fairly easy to change the storage from $_SESSION to the new one.

Thank you for your work! I will say that in webform_views I also have guys requesting for "allow anonymous users see own submissions" kind of permission. So I support moving in that direction.

Sorry I couldn't assist further with this ticket last few days. I am kind of overbooked with my own stuff in life. But whenever I have a spare moment be sure I'll invest it into webforms or webform_views.

Just some ideas for thinking about this dilemma of "session" vs "custom token". I think both have their pros and cons. I think the advantage of "session" over "custom token" you've already seen since you're considering to switch away from the latter to the former. But custom token can also come handy - imagine an anonymous user has submitted a webform and received an email notification. The email notification has a link to edit his webform submission (or do any other private action with the webform). And the link is tokenized with "custom token". Then if the guy tries to edit his webform from another device (say he's submitted it from a desktop and then from his cell phone he opens the link from his email inbox), if you just use session then he'd get an access denied. But if the authentication scheme was through custom tokens, he'd be allowed through.

I am not saying one is better over another. Just a use case where your original approach is deemed beneficial.

I am glad to see this big feature become available in the webform :)