CRM-20697 - Online pay now anomalies (contribution transfer to new contact) #11578

jitendrapurohit · 2018-01-23T10:46:19Z

Overview

Avoid contribution to be logged under new contact when "pay now" is done anonymously.

Before

Pay now functionality does not contain any authentication on the main page.
Contribution gets recorded under new contact when paid as anonymous.

After

Checksum is added to the pay now link so that it is bounced back when no contact is found on the main page.
As cid is checked and assigned on the main page, contribution is recorded for the correct contact.

CRM-20697: Online pay now anomalies (contribution transfer to new contact)

jitendrapurohit · 2018-01-23T10:47:08Z

@magnolia61 Are you able to test this?

magnolia61 · 2018-01-23T12:23:57Z

Works ok for me. One small thought though: I am not sure the transparent redirect to the homepage in case of a missing cid or checksum is desirable. I think the same about when the payment is already completed. I think it is proper user experience when people get an error message.
Another important thing would be to update the documentation for the syntax of the constructed PayNow link. Also I just noticed the amount shown is the full amount and not the remaining amount in case of partially paid contributions. But I will open another Jira issue for that. Thanks for your work Jitendra!

New issue on 'overcharging' partially paid contributions: https://issues.civicrm.org/jira/browse/CRM-21700

eileenmcnaughton · 2018-01-24T23:57:53Z

CRM/Contribute/Page/UserDashboard.php

@@ -142,6 +142,8 @@ public function run() {
    $invoiceSettings = Civi::settings()->get('contribution_invoice_settings');
    $invoicing = CRM_Utils_Array::value('invoicing', $invoiceSettings);
    $defaultInvoicePage = CRM_Utils_Array::value('default_invoice_page', $invoiceSettings);
+    $cs = CRM_Contact_BAO_Contact_Utils::generateChecksum($this->_contactId);
+    $this->assign('contactChecksum', $cs);


Maybe squash these 2 lines into one?

Also - does that regenerate it - I think if the user is here it is because one of the following is true

they are an admin user

they are the user who is being viewed & are logged in

they are the user who is being viewed & they got here using a checksum - which should be in the url ( TBH I thought once a check sum had been used it was stored to the session)

they are viewing an alternate contact who they have permission to.

Adding the checksum to the url in scenarios 1 & 4 seem risky -in 3 then it would be in the url & can be passed through?

Not sure of I'm right but I think the change in Main.php already solves the problem of anonymous (constructed) PayNow links without a contact id in the url (payment links to be mailed).
The links in the dashboard itself probable do not need the checksum, but I thought it would be clearer to have one uniform url.
BTW It is not possible to reach the Contact Dashboard via a checksum link (since longtime on my wishlist)
So removing the checksum (if possibly required for security reasons you mention?) at least it would be good to leave the Contact id, because without it anonymous use of the link will fail like described in this issue.

Actually maybe the anonymous link (to be mailed) can do without the checksum and only have ccid and cid? So contribution and contact are matched? Would that work?

@magnolia61 I don't think so, cid needs a checksum to be authenticated, else contact id is not assigned while checking permission on loading of the form.

@eileenmcnaughton Use-case mentioned by pradeep could be another reason to have a cid and checksum on pay now link for scenarios 1 & 4 other than having uniform link mentioned by @magnolia61 ?

eileenmcnaughton · 2018-01-24T23:59:59Z

@monishdeb do you want to review this? I thought it made sense, but then came up with some questions....

. The $_ccid property needs to be declared & documented! I think it is

/**
 * Contribution ID.
*
 *  Implicitly implies the form is being used in the context of adding an additional payment to an existing contribution
*
 * @var int
/**
public $_ccid;

pradpnayak · 2018-01-26T06:53:46Z

@jitendrapurohit @eileenmcnaughton I did UI test on this. I am not sure if this use case needs to be handled or not.

Use case:

Logged in as Admin (no contribution assigned to Admin contact).
Created Contact 'ABC' and added new pending contribution.
On user dashboard for Admin user i couldn't see the pay now option.
I appended to dashboard URL as &cid=205 and loaded the contact dashboard for 'ABC'(I was able to see Pay Now)
Clicked on Pay Now button and submitted Payment

Expected Result:
Contribution should be changed from Pending to Completed for Contact 'ABC'.

Actual Result

No contribution listed under contribution tab for contact 'ABC'
Can see same contribution listed under contribution tab for Admin contact
Contribution status changed from Pending to Completed and also civicrm_contribution.contact_id updated to logged in user's contact id.

jitendrapurohit · 2018-01-26T11:41:02Z

I was able to replicate the above scenario mentioned by @pradpnayak which was due to the wrong assignment of contactId on the dashboard. This has been fixed now.

jitendrapurohit · 2018-01-26T11:45:26Z

CRM/Contact/Page/View/UserDashBoard.php

@@ -64,7 +64,7 @@ public function __construct() {
      CRM_Utils_System::redirect(CRM_Utils_System::url('civicrm/dashboard', 'reset=1'));
    }

-    $this->_contactId = CRM_Utils_Request::retrieve('id', 'Positive', $this);
+    $this->_contactId = CRM_Utils_Request::retrieve('cid', 'Positive', $this);


It actually depends on how user dashboard is loaded. This worked fine when url was loaded as civicrm/user?reset=1&id=203. As cid is most common param for contact id, user dashboard is also updated to use this as an identifier now.

pradpnayak · 2018-01-28T11:08:54Z

@jitendrapurohit don't you need to update the url at all the places?

drupal//civicrm_user.inc:213:          'civicrm/user',
CRM//Contact/BAO/Contact.php:3145:        'href' => CRM_Utils_System::url('civicrm/user', "reset=1&id={$contactId}"),
CRM//Contact/Form/RelatedContact.php:77:    $session->pushUserContext(CRM_Utils_System::url('civicrm/user', "reset=1{$rcid}"));
CRM//Contact/Page/View/Relationship.php:228:      $url = CRM_Utils_System::url('civicrm/user',
CRM//Contact/Page/View/UserDashBoard/GroupContact.php:102:      $url = CRM_Utils_System::url('civicrm/user', "reset=1&id={$this->_contactId}");
CRM//Contact/Page/View/UserDashBoard/GroupContact.php:116:      CRM_Utils_System::url('civicrm/user', "reset=1&id={$this->_contactId}"),
CRM//Contact/Page/View/UserDashBoard.php:222:          'url' => 'civicrm/user',
CRM//Contact/Page/View.php:357:      $dashboardURL = CRM_Utils_System::url('civicrm/user',
CRM//Contribute/BAO/ContributionRecur.php:493:            $url = CRM_Utils_System::url('civicrm/user', "reset=1&id={$cid}");
CRM//Core/Block.php:467:          'path' => 'civicrm/user',

eileenmcnaughton · 2018-01-28T22:07:28Z

So I'm a bit worried that if I as admin user view a contact's dashboard (which I can do) I will see a checksum link & if I click on that then logically I should then be logged in as that contact (undesirable) - I think once you get to the dashboard you are validated in some way & shouldn't need a checksum link from there

magnolia61 · 2018-01-29T03:36:26Z

Hi all, in this the discussion in the first commit by Jitendra I already said I could live with only the changes in Main.php (that prevent a contribution made by using a constructed (and mailed) Pay Now link from being logged wrongly. d1ff7c1

The change in Main.php solves the original reason for this issue: prevent issues when a user gets mailed a PayNow link and tries to pay using it: https://issues.civicrm.org/jira/browse/CRM-20697 So thats using the PayNow link functionality while 'logged in' with a checksum in the link, ccid. The change in Main.php enforced the requirement to have a cid in that link also, which solves the issue of stangely transfering contributions.

All other proposed changes and discussion on the paynow links from the dashboard that I see appearing here are more to prevent an admin user that wants to pay using a Pay Now link fro making mistakes. Maybe other solutions are needed there than to include he checksum in all links.

That being said I do think that the problem Pradeep ran into should be solved. When a logged in admin pays using the PayNow button (or mailed link) the paid contribution should not 'move' but keep being attached to the 'owner' of the ccid.

Could a general solution be just to make sure a payment made with either the (mailed) link or dashboard button gets logged as an activity with the logged in user (posibly admin) but stays (changed to paid) under the user that belongs to the ccid (and not the checksum owner or logged in user) ? (and skip including the checksum in the userdashboard paynow links?)

jitendrapurohit · 2018-03-28T04:25:52Z

Sorry for the delayed update on this. I've now updated the PR to include only the changes in Main.php which prevent incorrect contact to pay for the existing contribution in civi.

If an anonymous user wants to complete its pending contribution, the URL should have the cid and the checksum of the contact in order to authenticate the user on the contribution page. Maybe, we should add some statements in the documentation regarding this.

magnolia61 · 2018-03-28T05:23:04Z

Thanks Jitendra, I will test it.

While you're in the Online Pay Now subject would you know a solution for the overcharging / double net amount issue with Online Pay Now payment links? That is an even greater anomaly (impact & reputationwise) https://issues.civicrm.org/jira/browse/CRM-21700

eileenmcnaughton · 2018-06-12T02:06:53Z

@monishdeb are you able to review this one once you are back. I think @JoeMurray will also ask you to look at CRM-21700 which has been updated recently with some good information

monishdeb · 2018-06-15T13:18:58Z

I have tested on my local and it works fine, also checked the issue mentioned by Pradeep and it worked fine. UT cannot be added as there isn't any scope replicate the UI use-case in test env. Merging now.

@jitendrapurohit has submitted a separate PR - #12319 for CRM-21700, will review it later

magnolia61 · 2018-08-20T10:52:44Z

This fix unfortunately does not work when using the Pay Now button from the user dashboard checksum link first (introduced in 5.3.0). Opened an issue for that in gitlab https://lab.civicrm.org/dev/core/issues/346

civicrm-builder added the master label Jan 23, 2018

jitendrapurohit changed the title ~~CRM-20697 - Add checksum to pay now link~~ CRM-20697 - Online pay now anomalies (contribution transfer to new contact) Jan 23, 2018

magnolia61 mentioned this pull request Jan 23, 2018

CRM-21693 show Display Name in online pay now UI #11571

Merged

eileenmcnaughton reviewed Jan 24, 2018

View reviewed changes

jitendrapurohit commented Jan 26, 2018

View reviewed changes

CRM-20697 - Add checksum to pay now link

58b0963

jitendrapurohit force-pushed the CRM-20697 branch from c577989 to 58b0963 Compare March 28, 2018 04:21

monishdeb merged commit ef444b4 into civicrm:master Jun 15, 2018

jitendrapurohit deleted the CRM-20697 branch July 16, 2018 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-20697 - Online pay now anomalies (contribution transfer to new contact) #11578

CRM-20697 - Online pay now anomalies (contribution transfer to new contact) #11578

jitendrapurohit commented Jan 23, 2018 •

edited by civicrm-builder

Loading

jitendrapurohit commented Jan 23, 2018

magnolia61 commented Jan 23, 2018 •

edited

Loading

eileenmcnaughton Jan 24, 2018

eileenmcnaughton Jan 25, 2018 •

edited

Loading

magnolia61 Jan 25, 2018

magnolia61 Jan 25, 2018

jitendrapurohit Jan 26, 2018

jitendrapurohit Jan 26, 2018

eileenmcnaughton commented Jan 24, 2018 •

edited

Loading

pradpnayak commented Jan 26, 2018

jitendrapurohit commented Jan 26, 2018

jitendrapurohit Jan 26, 2018 •

edited

Loading

pradpnayak commented Jan 28, 2018

eileenmcnaughton commented Jan 28, 2018

magnolia61 commented Jan 29, 2018 •

edited

Loading

jitendrapurohit commented Mar 28, 2018 •

edited

Loading

magnolia61 commented Mar 28, 2018 •

edited

Loading

eileenmcnaughton commented Jun 12, 2018

monishdeb commented Jun 15, 2018 •

edited

Loading

magnolia61 commented Aug 20, 2018

CRM-20697 - Online pay now anomalies (contribution transfer to new contact) #11578

CRM-20697 - Online pay now anomalies (contribution transfer to new contact) #11578

Conversation

jitendrapurohit commented Jan 23, 2018 • edited by civicrm-builder Loading

Overview

Before

After

jitendrapurohit commented Jan 23, 2018

magnolia61 commented Jan 23, 2018 • edited Loading

eileenmcnaughton Jan 24, 2018

Choose a reason for hiding this comment

eileenmcnaughton Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

magnolia61 Jan 25, 2018

Choose a reason for hiding this comment

magnolia61 Jan 25, 2018

Choose a reason for hiding this comment

jitendrapurohit Jan 26, 2018

Choose a reason for hiding this comment

jitendrapurohit Jan 26, 2018

Choose a reason for hiding this comment

eileenmcnaughton commented Jan 24, 2018 • edited Loading

pradpnayak commented Jan 26, 2018

jitendrapurohit commented Jan 26, 2018

jitendrapurohit Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

pradpnayak commented Jan 28, 2018

eileenmcnaughton commented Jan 28, 2018

magnolia61 commented Jan 29, 2018 • edited Loading

jitendrapurohit commented Mar 28, 2018 • edited Loading

magnolia61 commented Mar 28, 2018 • edited Loading

eileenmcnaughton commented Jun 12, 2018

monishdeb commented Jun 15, 2018 • edited Loading

magnolia61 commented Aug 20, 2018

jitendrapurohit commented Jan 23, 2018 •

edited by civicrm-builder

Loading

magnolia61 commented Jan 23, 2018 •

edited

Loading

eileenmcnaughton Jan 25, 2018 •

edited

Loading

eileenmcnaughton commented Jan 24, 2018 •

edited

Loading

jitendrapurohit Jan 26, 2018 •

edited

Loading

magnolia61 commented Jan 29, 2018 •

edited

Loading

jitendrapurohit commented Mar 28, 2018 •

edited

Loading

magnolia61 commented Mar 28, 2018 •

edited

Loading

monishdeb commented Jun 15, 2018 •

edited

Loading