Fix permission check on Contribution form, clarify underlying functions #27013

eileenmcnaughton · 2023-08-06T23:34:27Z

Overview

This is a reviewer response to #22951

In that PR it seeks to set UserID to something derived from a function that is doing a lot of work. I dug into that function which someone (ahem) wrote many years ago and teased out some of the underlying chunks & cleaned up the variable names for clarity along with a doc block cleanup.

I decided that rather than set _userID in contributionBase we should remove it (it is only referred to in 4 places) and call the more specific function at each usage.

Otherwise we change the meaning of userID that is used somewhat differently later.

Also I rather prefer functions that will perform the same whenever called over relying on a variable having been set correctly the first time

Before

It is not possible to load from the mid on the contribution page if checksum authentication has been used

After

If the checksum user can view a membership it can be loaded

Technical Details

->_userID only used in a test with these changes

Comments

civibot · 2023-08-06T23:34:29Z

Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷

Introduction for new contributors

If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers

eileenmcnaughton · 2023-08-06T23:36:29Z

CRM/Core/Form.php

-        CRM_Core_Resources::singleton()->addVars('coreForm', ['contact_id' => (int) $tempID]);
-        CRM_Core_Resources::singleton()->addVars('coreForm', ['checksum' => $userChecksum]);
-        return $tempID;
+    if ($requestedContactID === $this->getAuthenticatedContactID()) {


Note that $this->getAuthenticatedContactID() is always an int so will not match NULL here

eileenmcnaughton · 2023-08-06T23:38:26Z

CRM/Core/Form.php

-    elseif ($tempID && CRM_Contact_BAO_Contact_Permission::allow($tempID)) {
-      CRM_Core_Resources::singleton()->addVars('coreForm', ['contact_id' => (int) $tempID]);
-      return $tempID;
+    if ($requestedContactID && CRM_Contact_BAO_Contact_Permission::allow($requestedContactID)) {


So this only allows permissioning another user by the logged in user. I wondered about that - but since really only the cid in the url can be check-sum authenticated there hypothetical of a checksum user needed permissioned access to another user does not apply

wasn't there always the "contribute as someone else" on front end forms at some point?

@seamuslee001 yep - but you can combine that with logged in user permissions, but not checksum authentication permissions from my read on the before code (presumably cos cid has to be in the url to be validated with checksum)

mlutfy · 2023-08-11T13:52:09Z

I did a fair amount of testing and it seems to work well.

Tested renewing "on behalf of", as per initial use-case
Tested to make sure that "not you? then cid=0" still works

Also had a look at the code, and it's definitely not fun code, but I definitely appreciate having a clear getAuthenticatedContactID function, because previously every form kind of had to do their own check, and I'm sure I've done it wrong half the time.

Thank you @eileenmcnaughton !

cc @mattwire this might interest you

mlutfy · 2023-08-12T00:24:12Z

jenkins, test this please

mlutfy · 2023-08-12T00:32:55Z

I had done a bit of a mess while trying to solve the conflict because of #27053, but it should be fine now.

Diff of the PR, before/after:

$ diff /tmp/membership.patch /tmp/membership3.patch 
1c1
< From 3d960a758ac55c2153e7d37ba9e31613e2069060 Mon Sep 17 00:00:00 2001
---
> From bdfb2fae6f26331f59b63b479f74db9403441197 Mon Sep 17 00:00:00 2001
27c27
< index 36422a48f3b..396f19579d5 100644
---
> index 82767a072cd..b423a4b41ca 100644
61c61
<                $inheritedRelTypes = implode(CRM_Utils_Array::explodePadded($membershipType->relationship_type_id), ',');
---
>                $inheritedRelTypes = implode(',', CRM_Utils_Array::explodePadded($membershipType->relationship_type_id));

eileenmcnaughton · 2023-08-12T01:26:10Z

@mlutfy I haven't done it at this point - but I have started to mark functions which feel 'settled & consistent' on forms with @api to indicate they are supported for extension usage. Perhaps you have just made the case for getAuthenticatedContactID to be marked as such

mlutfy · 2023-08-12T19:52:57Z

@eileenmcnaughton oh, I like that idea! Can you do it as part of this PR, or want me to do it?

This is a reviewer response to civicrm#22951 In that PR it seeks to set UserID to something derived from a function that is doing a lot of work. I dug into that function which someone (ahem) wrote many years ago and teased out some of the underlying chunks & cleaned up the variable names for clarity along with a doc block cleanup. I decided that rather than set _userID in contributionBase we should remove it (it is only referred to in 4 places) and call the more specific function at each usage. Otherwise we change the meaning of userID that is used somewhat differently later. Also I rather prefer functions that will perform the same whenever called over relying on a variable having been set correctly the first time

eileenmcnaughton · 2023-08-12T21:55:29Z

@mlutfy - Ok I've pushed in a test + comment

eileenmcnaughton · 2023-08-12T22:01:13Z

I also added a note https://docs.civicrm.org/dev/en/latest/hooks/#pitfalls-of-hooks

eileenmcnaughton · 2023-08-12T22:06:32Z

& also here https://lab.civicrm.org/documentation/docs/dev/-/commit/3bb209085be01545bf68d55a6926e1f4de239638

mlutfy · 2023-08-14T20:12:31Z

@eileenmcnaughton you rock - thank you!

civibot bot added the master label Aug 6, 2023

eileenmcnaughton commented Aug 6, 2023

View reviewed changes

eileenmcnaughton mentioned this pull request Aug 6, 2023

Contribution Pages: alias userID to contactID, fixes org renewal with checksum and mid #22951

Closed

mlutfy mentioned this pull request Aug 11, 2023

Fix PHP 8.0 implode on ContributionBase #27053

Merged

mlutfy added the merge ready PR will be merged after a few days if there are no objections label Aug 11, 2023

mlutfy force-pushed the auth branch 3 times, most recently from 6f76735 to bdfb2fa Compare August 12, 2023 00:31

eileenmcnaughton added 2 commits August 13, 2023 09:28

Add test + external support for getAuthenticatedContactID()

a4bbe01

eileenmcnaughton force-pushed the auth branch from bdfb2fa to a4bbe01 Compare August 12, 2023 21:54

mlutfy merged commit 44efaf5 into civicrm:master Aug 14, 2023

eileenmcnaughton deleted the auth branch August 14, 2023 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix permission check on Contribution form, clarify underlying functions #27013

Fix permission check on Contribution form, clarify underlying functions #27013

eileenmcnaughton commented Aug 6, 2023 •

edited

Loading

civibot bot commented Aug 6, 2023

eileenmcnaughton Aug 6, 2023

eileenmcnaughton Aug 6, 2023

seamuslee001 Aug 7, 2023

eileenmcnaughton Aug 7, 2023 •

edited

Loading

mlutfy commented Aug 11, 2023

mlutfy commented Aug 12, 2023

mlutfy commented Aug 12, 2023

eileenmcnaughton commented Aug 12, 2023

mlutfy commented Aug 12, 2023

eileenmcnaughton commented Aug 12, 2023

eileenmcnaughton commented Aug 12, 2023

eileenmcnaughton commented Aug 12, 2023

mlutfy commented Aug 14, 2023

Fix permission check on Contribution form, clarify underlying functions #27013

Fix permission check on Contribution form, clarify underlying functions #27013

Conversation

eileenmcnaughton commented Aug 6, 2023 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Aug 6, 2023

eileenmcnaughton Aug 6, 2023

Choose a reason for hiding this comment

eileenmcnaughton Aug 6, 2023

Choose a reason for hiding this comment

seamuslee001 Aug 7, 2023

Choose a reason for hiding this comment

eileenmcnaughton Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

mlutfy commented Aug 11, 2023

mlutfy commented Aug 12, 2023

mlutfy commented Aug 12, 2023

eileenmcnaughton commented Aug 12, 2023

mlutfy commented Aug 12, 2023

eileenmcnaughton commented Aug 12, 2023

eileenmcnaughton commented Aug 12, 2023

eileenmcnaughton commented Aug 12, 2023

mlutfy commented Aug 14, 2023

eileenmcnaughton commented Aug 6, 2023 •

edited

Loading

eileenmcnaughton Aug 7, 2023 •

edited

Loading