dev/core#314 Event self-service Transfer picks up the deleted contact ID basically transferring to the wrong contact

[aniesshsethh]

Overview

When transferring a event registration, it picks up the first email id in the email table and get the corresponding contact id, which in some cases can be the deleted contact.

Before

It's picking up the wrong contact to attach the registration to.

After

It's now picking up the correct contact to attach the registration to.

Technical Details

Just a simple Join and checking for deleted records.

Comments

---

https://lab.civicrm.org/dev/core/issues/314

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

@davejenx @monishdeb @jitendrapurohit I never let a good deed go unpunished & you 3 seem to be the only semi-recent contributors to this code so I'm hoping one of you might review this

[davejenx]

@eileenmcnaughton Looks like it was Dave G, not me but I've commented anyway. :-)

[eileenmcnaughton]

@davejenx any Dave will do

[eileenmcnaughton]

@davejenx I don't see your comment

[monishdeb]

Hmm necessary improvement to find the valid contact but additional improvement would be:

- $query = "select contact_id from civicrm_email left join civicrm_contact on civicrm_contact.id=civicrm_email.contact_id where email='" . $params['email'] . "' and is_deleted=0";
-      $dao = CRM_Core_DAO::executeQuery($query);
-      while ($dao->fetch()) {
-      $contact_id  = $dao->contact_id;
-  }
+  $contactID =  CRM_Core_DAO::singleValueQuery("select contact_id from civicrm_email left join civicrm_contact on civicrm_contact.id=civicrm_email.contact_id where email='" . $params['email'] . "' and is_deleted=0 LIMIT 1  ");
+  if (!$contactID) {
+   CRM_Core_Error::statusBounce(ts('Contact does not exist.'));
+  }

[davejenx]

Why left join here? Looks like the WHERE clause will ensure only civicrm_email records with a matching civicrm_contact record having is_deleted = 0 will be retrieved. And foreign key constraints should ensure you can't have a civicrm_email.contact_id that doesn't match a civicrm_contact.id . So INNER JOIN seems appropriate to me.

Also there looks to be an existing issue in the code: it's putting a parameter in a SQL query by concatenation rather than passing a parameters array to CRM_Core_DAO::executeQuery(). Would be good to fix that if altering this code, in my opinion.

[davejenx]

@eileenmcnaughton Oops, hadn't hit the button a second time to submit review comment.
P.S. Me and my mate Dave take exception to your blatantly Davist comment.

[eileenmcnaughton]

@davejenx that's so funny because I've had to face down allegations of 'Jackism' this week. Apparently a key symptom of Jackism is people named 'Jack' get asked to empty the dishwasher

[aniesshsethh]

Thanks Guys for the review, I have made the changes suggested with the exception of the variable name from contactID to contact_id as it is been used in the script below

[pradpnayak]

Can we use api? So that one can alter the result using hook.

Thoughts?

[aniesshsethh]

actually I have had a case where due to some special chars, API not only fails to respond, but basically sends a very non standard response, I want to catch and fix that one first before using API in regards to email anywhere else. Please correct me if I am wrong

[eileenmcnaughton]

@aniesshsethh We do have some tests on the api with special characters - there are some cases where on some server configs on some fields some characters are ignored. I would still go with the api as we use it by choice in general & this doesn't seem like the corner of the code to reject it over a really edge case bug

[aniesshsethh]

okay so done @eileenmcnaughton

[pradpnayak]

Hi @aniesshsethh

You can also do it as :

diff --git a/CRM/Event/Form/SelfSvcTransfer.php b/CRM/Event/Form/SelfSvcTransfer.php
index 7452ccb..4076655 100644
--- a/CRM/Event/Form/SelfSvcTransfer.php
+++ b/CRM/Event/Form/SelfSvcTransfer.php
@@ -337,10 +337,16 @@ class CRM_Event_Form_SelfSvcTransfer extends CRM_Core_Form {
     }
     else {
       //cancel 'from' participant row
-      $query = "select contact_id from civicrm_email where email = '" . $params['email'] . "'";
-      $dao = CRM_Core_DAO::executeQuery($query);
-      while ($dao->fetch()) {
-        $contact_id  = $dao->contact_id;
+      try {
+        $contact_id = civicrm_api3('Email', 'getvalue', [
+          'return' => 'contact_id',
+          'email' => $params['email'],
+          'options' => ['limit' => 1],
+          'contact_id.is_deleted' => 0,
+        ]);
+      }
+      catch (CiviCRM_API3_Exception $e) {
+        CRM_Core_Error::statusBounce(ts('Contact does not exist.'));
       }
     }
     $from_participant = $params = array();

The reason why i sued email api is because Contact api searches for primary emails but in your case i believe its not mandatory to be a primary email.

Note: The above snippet is not tested, i just created blindly.

HTH

Pradeep

[aniesshsethh]

Hi @pradpnayak

I tried implementing this, but it is failing some of the tests. But regardless, I do not think this is required since the bug I am mentioning does not result in Exception, but a unstandard response, so a try catch in this scope would actually not fix this

[mattwire]

@pradpnayak @davejenx What's the status with this PR?

[mattwire]

@aniesshsethh What is the current state of this PR?

[aniesshsethh]

@mattwire I am fine with the current patch if others are

[mattwire]

@davejenx @pradpnayak Could you please confirm if you are happy with this PR now? So we can merge.

@lcdservices @alifrumin Do either of you have capacity to give this a quick test and confirm if it is ok to merge?

[alifrumin]

To test this I tried:

1. Going to an Event Settings-> Online Registration tab and checking the "Allow self-service cancellation or transfer?" box for the Event.
2. Deleting a contact that has a first name, last name and email address (so they are in the trash not permanently deleted) call this contact Bob.
3. Thru the event registration front end form registering a contact for the event (call this contact Cat)
4. Clicking the Transfer link in the email confirmation for Cat
5. Trying to transfer the the event registration to the deleted Bob by entering his first name, last name and email.

On a site with this pr the result was that a new Bob gets created and the event registration is assigned to him, no "Contact does not exist." error was thrown.

Perhaps I am missing a step in how to replicate this bug.

[aniesshsethh]

Hi @alifrumin

Sorry for the late response. This bug can be reproduced like this. Create a contact A in the system, delete it, again create that contact A so that it has a new contact id, but the old contact id should still be in the system. Now Register as contact B, and Try to transfer the ticket to contact A, it will assign that ticket to the deleted contact A instead of the valid contact A

  _$query = "select contact_id from civicrm_email where email = '" . $params['email'] . "'";_

The basic issue was this query is returning even the deleted contact_id, When we are calling it through the API, only the valid contact ID is returned

I will check your steps though to reproduce the error you said, because as you said, the contact didn't already existed in the system so I guess that is why it threw the error, I will add code to handle that

[eileenmcnaughton]

@alifrumin @aniesshsethh what is the status here?

[alifrumin]

Hey @eileenmcnaughton thanks for following up on this.

I just tested on a site with this pr and a site without the pr by:

1. Creating a contact A in the system,
2. Pressing "Delete Contact" on the Contact Summary page for contact A (putting the contact in the trash not deleting it permanently)
3. again creating contact A so that it has a new contact id
4. Going to an existing Event registration for an unrelated contact and selecting more ->Transfer or Cancel
5. Selecting Transfer on the "Self-service Registration Update"
6. On the "Self-service Registration Transfer" form entering the first, last and email for contact A

RESULT:
both with and without this pr the event registration was transferred to the contact A that is not in the trash.

@aniesshsethh, perhaps I am misinterpreting "Create a contact A in the system, delete it, again create that contact A so that it has a new contact id, but the old contact id should still be in the system." thoughts?

[aniesshsethh]

na you are right , Did you create contact A again with the exact same primary email as the original contact?

[alifrumin]

I double checked that I used the same first name, last name and primary email address by searching in the api. As you can see in the screen shot of the api explorer below the count is 2 when searching on the first name, last name and email :

[eileenmcnaughton]

I took a look through this & it's a shame this code is in core rather than an extension - it doesn't seem to have much love & less tests.

However, on balance I'm going to merge this change - logically reading the code it should work, reading @alifrumin's comments it doesn't but it doesn't make it worse. However, it does make the code more consistent and readable & removes some sql interpolation - which is a bad security practice even though it may not be exploitable.