CRM-20892: Prevent cross-editing of mailings between multiple browser/tab instances

[awzilkie]

Overview

CRM-20892: Prevent cross-editing of mailings between multiple browser/tab instances

Before

When the same mailing is open in two different tabs, changes from one can overwrite changes from the other even if the mailing has been submitted!

Steps to recreate:

1. On Tab A: Create a new mailing, add a name, title, recipients, subject and some text. Then click Save Draft or wait until an autosave. Click next, schedule the mailing and submit it.
2. Copy the URL of the mailing, paste it into a new tab (B)
3. On tab B: Change the mailing parameters and click Save Draft or wait for an autosave.
4. Notice the parameters in the database coincide with step 3, those in step 2 were overwritten

This can happen with multiple tabs on the same PC, or different users on different PCs (if two people end up working on the same mailing).

After

Add a new column last_modified to the civicrm_mailing table in the DB to keep track of the last browser instance to change the DB. This id will be based on the number of seconds since the epoch at the time the mailing was created or continued to ensure that it is always unique.

Each browser instance keeps its own unique last modified ID as described above. When a change is made in a browser instance, before it is saved to the DB check if the browser's ID matches that stored in the database for that mailing. If they do not match, bock the change and throw an appropriate error message.

Technical Details

Add last_modified column to the civicrm_mailing table in mysql incremental upgrade file for version 4.7.25

Comments

None

---

• CRM-20892: Same mailing open in two windows can overwrite data on scheduled mailings

[civicrm-builder]

Can one of the admins verify this patch?

[seamuslee001]

Jenkins ok to test

[JKingsnorth]

Thanks for your work on this! Looks good in theory. Just a few thoughts.

Why not call it modified_date to make it consistent with the same field on the contact entity, and store it as the same data type:

civicrm-core/xml/schema/Contact/Contact.xml

Lines 860 to 868 in 23e6588

	<field>
	<name>modified_date</name>
	<type>timestamp</type>
	<comment>When was the contact (or closely related entity) was created or modified or deleted.</comment>
	<required>false</required>
	<export>true</export>
	<default>CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP</default>
	<add>4.3</add>
	</field>

The lock on the contact entity works essentially in the same way: https://github.com/civicrm/civicrm-core/blob/648631cd94799e87fe2347487d465b1a7256aa57/CRM/Contact/Form/Edit/Lock.php

I think it would be better to keep things consistent.

Then - we can update the comments/documentation rather than referring to it as a 'modified ID' it is simply the last modified date.

For completeness it would also be good to add a test for this.

[eileenmcnaughton]

@totten you need to look at this one. I totally agree this is a serious issue and I know of another screen (in that case quickform) where it happens so we need to have some guidelines about best approach/s for this

[totten]

@awzilkie good call in using the modification-time as an optimistic lock mechanism (oplock).

I agree with @JKingsnorth that it should do a column modified_date generated via MySQL. A few reasons:

• It's more consistent with how contact locking works. And there are several other tables do or will use the name modified_date.
• The correctness of an oplock depends on having consistent timestamps/clocks. If you generate the timestamps on the server-side (via SQL or PHP), then the correctness only depends on the configuration of the server's clock. (Yaay!) If you generate them on the client-side (via JS), then the correctness depends on having consistent clock configurations across all clients. (Aarg!)

There is an important difference between contact UI and mailing UI -- the mailing UI goes through APIv3 (no QuickForm). The contact examples don't show how to do oplocking with APIv3 -- and existing integrations don't expect APIv3 to enforce oplocking.

Here's a design you could try -- update the APIv3 contract so support an option.oplock, as in:

// Basic API request which ignores oplocking. Default for backward compatibility.
$response = civicrm_api3('Mailing', 'create', array(
  'id' => 123,
  'modified_date' => 'willbeignored',
   // and other changes...
));

// API request which participates in oplocking.
$response = civicrm_api3('Mailing', 'create', array(
  'options' => array('oplock' => 1),
  'id' => 123,
  'modified_date' => 'the-last-seen-timestamp',
   // and other changes...
));

For implementation, here are some things you could try:

• The JS code would need to send the option.oplock. Also, it may need to read the response for the latest timestamp.
• On the server-side, I think this pseudocode would be a good starting place for supporting 'option.oplock' in APIv3: https://gist.github.com/totten/0219ad30f2b2e4f3a3bb802cf8a09991 However, you'd still need to address a couple FIXMEs and define civicrm_mailing.modified_date column (like John mentioned).

[JKingsnorth]

Thanks for the groundwork, I think this can be closed in favour of #10965 ?

[JoeMurray]

Closing in favour of 10965