Add hook for validating checksum

[mattwire]

Overview

Checksums are not stored anywhere in the database (they are calculated at time of creation and time of use). This causes problems because there is no way to "invalidate" a checksum that was made available by mistake (eg. hardcoded in an email).

This PR proposes adding a hook that can be called to override/modify the checksum validation.

Before

No way to override checksum validation.

After

Can be overridden via hook - eg.

function example_civicrm_invalidateChecksum($contactID, $checksum, &$invalid) {
  // These checksums sent out hardcoded via mailing on 14th July (valid for 30 days)
  if (in_array($checksum, ['fdsfsdfdsf_sfsd_123', 'fsa34f30fsfs_sf34f_123'])) {
    $invalid = TRUE;
    \Civi::log()->warning('Invalidated checksum was used: ' . $checksum . ' for contact ID ' . $contactID);
    // Optionally trigger a redirect to another page explaining why it was invalid
    // CRM_Utils_System::redirect('https://example.org/invalidchecksumlandingpage');
  }
}

Technical Details

Comments

@seamuslee001 @petednz @demeritcowboy What do you think?

[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).

[demeritcowboy]

Sounds good. A couple thoughts:

• Changing the contact record's hash should invalidate it, but I suppose would invalidate ALL checksums for that contact.
• Hooks like this are usually based on drupal's where you can have multiple consumers and some might say it's valid and some might say it's not, so is_valid could get overwritten which is almost like "if ANY allow then allow it" whereas you want "if ANY deny then deny it". So remove the is_valid param and instead the hook implementation would return array(FALSE) and that gets merged into a combined array for you automatically, then you loop thru all results and if ANY are false then it's not valid.
• As a new feature it should have a unit test - looks like there should be lots of examples grep -r setHook tests\phpunit although I don't immediately see a "validate" type of hook and they all look like "alter" types. But same principle, just return an array value instead of altering.

[seamuslee001]

Yeh I tend to agree with Dave D here and his assessment of this, I have just been thinking that this potentially could open a security hole if someone just caused it to return valid for any checksum no matter what. So my thinking is that we should be only allowing an extension to invalidate a checksum rather than overriding the ttl / other checksum validation stuff. On the other hand that would be kind of a user beware of what your adding to your system issue if we did allow extensions to just by-pass the validation of the checksum.

[mattwire]

@seamuslee001 @demeritcowboy My use-case is a blacklist of "compromised" checksums so a "invalidate-only" hook would be fine for my use-case. The only other hook that "returns" a value is https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_findDuplicates/ and (disclaimer) I implemented that one too.

So we could just pass a $invalid parameter that's set to FALSE by default and the hook implementation can choose to set that to TRUE?

[demeritcowboy]

I think you're both right. So then the hook should be called "invalidateChecksum" or "invalidChecksumList" or something clear that that's all you can do.

[seamuslee001]

@mattwire @demeritcowboy I think that makes most sense to me, and if the value is true then return an error back without doing the additional validation

[mattwire]

Docs here https://lab.civicrm.org/documentation/docs/dev/-/merge_requests/960

[demeritcowboy]

See unit test at #20973