dev/core#3133 - Extend Authx functionality to support validation of externally generated JWTs

Migrate legacy rest to appropriate place

Remove legacy auth as not needed due to it self registering now

authx - Simplify service declaration

Merge pull request #25952 from seamuslee001/dev_core_3133

dev/core#3133 - Extend Authx functionality to support validation of e…

…xternally generated JWTs

Overview

This adds support for using externally generated JWTs e.g. by Auth0 to authenticate users using Authx

Before

Authx,when processing a Bearer token, expects the scope claim to be "authx" and the sub claim to be "cid:{thecontactid}", as generated by civicrm itself.

After

Other extensions can override the handling of checking of the credential in an example such as this extesnion https://github.com/australiangreens/civiauth0jwt

Technical Details

This changes to fire a new event civi.authx.checkCredential to allow extensions to validate if an authentication credential should be accepted or not.

This is a re-submit of https://github.com/civicrm/civicrm-core/pull/23019 which some minor modifications to make it work with the current Authx code

Yeah, I don't have any more to add, and I think it's had a decent pause. Let's merge it.

That sounds like a good idea to me, @jensschuppe

Before commenting, I ran a couple reports to measure the various metadata fields. Since the PR speaks to settings and APIv4, that gives two main subtopics:

• Setting Metadata: Report (de facto usage), Docs (de jure usage)
• APIv4 Field Metadata: Report (de facto usage)

I don't exactly agree or believe that the setting/is_multiple/serialize/Array question is major obstacle that needs to block all other work involving settings. In terms of the overall system (as measured my local dmaster), there are ~220 settings -- of which ~7 use serialize and ~13 use html_attributes.multiple. (That's probably overlapping - haven't checked.) Of course, that's a subjective lens. (For this report, I scanned little or nothing from contrib -- and I don't see the settings that are missing/needed/blocked by the limitation.). It may still be important. I just don't intuitively recognize it.

But... if you're looking at the input_attrs / html_attributes topic, then the multiplicity indicator is an interesting case-study. Here's what I mean:

• We have a common model (*.setting.php) that can be consumed by different page-stacks (eg SettingTrait/QF, APIv4 Explorer/Angular).
• The common model is augmented with quirky bits like html_attributes.multiple and html_attributes.class. AFAICS, there is no meaningful spec that tells you what can be used here. You have to try something and then see if it works on your page.
• The effect of the PR (integrating html_attributes/input_attrs) is that you're crossing more quirky bits between the different page-stacks.
• I think we had the idea that html_attributes/input_attrs was ontologically different from the other parts of the metadata. But on reflection, I'm not so sure. We have to break down examples:
  • multiple (13 settings): A multi-value field differs from a single-value field on many levels. It's stored differently; validated differently; displayed differently. FK's are enforced differently. Sprinkling in an HTML attribute changes the appearance, but it doesn't make the system work end-to-end.
  • maxlength (37 settings): This should be enforced regardless of how you put the data in. It's not just an HTML thing.
  • rows,cols (1 setting): OK, yes, this is totally an HTML/view thing.
  • class (35),style (3): The semantics are pretty open-ended. They are technically HTML-view-layer things. But the semantics can be tricky. But do you really expect classes and styles to look the same in different renderers?
• Put another way:
  • Is it proper to pass html_attributes.multiple or html_attributes.class (as used in SettingTrait/QF) over to input_attrs.multiple or input_attrs.class (as used in Explorer/Angular)?
  • If, for example, you believe that multiple should be normalized and well-supported, then you would probably judge that:
    • A good course-of-action is to update both systems to support multiple as primary metadata.
    • A bad course-of-action is to integrate the open-ended html_attributes/input_attrs, deal with the fallout/impedence-mismatches organically, and then later update both systems to support multiple as primary metadata.

Import templates Set created_id, expires_date appropriately

This tweaks just-merged userJob functionality.

When creating an import job it is not correct to use the created_id or expires_date from the template - but these should be set appropriately, rather than left blank

Merge pull request #25957 from eileenmcnaughton/import_template

Import templates Set created_id, expires_date appropriately

This tweaks just-merged userJob functionality.

When creating an import job it is not correct to use the created_id or expires_date from the template - but these should be set appropriately, rather than left blank

@eileenmcnaughton It gives me a chance to try out the template mechanism now. :)

I played with it using a couple user accounts (admin for original import; demo for follow-up), and I can confirm that each UserJob was attributed sensibly (ie demo's import was attributed to demo).

On usability - the only thing that took me a moment was to figure out that the link (You can re-use this import configuration [here]) would be displayed for Import Contributions but not Import Contacts.

A small code-style observation - it looks like the constant +1 week (for expires_date) matches-up with the +1 week in createUserJob(). We don't really have an snappy playbook for pulling out that into a const or setting, but if you were so inclined, then I'd be +1. (Heh... +1 about a +1 constant...) Regardless, this is consistent with the style+behavior currently there.

In any event, looks like the PR works. Tests pass. Merging.

Sounds pretty sensible. The recent work that it's updating is 2b8037052e5724858c864084123c4af0c0020f0a. I'll give a quick run.

Seems like a good idea @colemanw.

The test failure in CRM_Group_Page_AjaxTest does seem to be related.

Overview

TAP is a format for listing test results. It's handy for displaying large test-jobs -- because you can monitor progress and see when errors arise. We've been using it for sometime. But it requires a phpunit plugin.

ping @seamuslee001

https://lab.civicrm.org/dev/core/-/issues/4188

Before

You can use Civi\Test\TAP with phpunit8 (and some older ones):

CIVICRM_UF=UnitTests phpunit8 --printer '\Civi\Test\TAP' tests/phpunit/CRM/Utils/StringTest.php

After

You can additionally use Civi\Test\TAP with phpunit9:

CIVICRM_UF=UnitTests phpunit9 --printer '\Civi\Test\TAP' tests/phpunit/CRM/Utils/StringTest.php

Also, the file-layout for ./Civi/Test/TAP*.php is a bit more consistent. (Every implementation of TAP has a full file of its own.)

tools/scripts/phpunit - Enable tap for phpunit9

1. So in the eventmessages example, it does the same transform on several more fields. And it's similar to this PR in that both are focused on ICS/ICAL generation. @jensschuppe @yashodha For the ical scenario, how many fields should we be concerned about?
2. It definitely hurts my brain to track down the proper encoding for data in getCompleteInfo() -- i.e. there are multiple callers (CRM_Core_Block, CRM_Event_ICalendar, CRM_Event_Page_List, and the various extensions). There are multiple output media (HTML, ICal). There are several text fields (title, summary, description) with different encodings on-disk.
3. Part of me wonders if some of these callers should be hitting Event.get API instead.
4. As a general matter, throwing in extra decodes randomly is liable to create security issues (e.g. if a user-supplied < becomes a literal < in an HTML output, then sanitized user-content can become unsanitized JS commands. But if one looks at the specific callers+fields, you might find mitigating factors (e.g. one caller displays to a non-HTML medium; another displays HTML but calls purify).
5. I'm guessing that nobody else has done (or wants to do) the due-diligence of re-validating the security of all the callers with this change.
6. If the main issue is how the data appears in ICS/ICAL, then perhaps a safer approach would be:
   • Hold steady the data-format returned by getCompleteInfo() (for whoever calls it now - they're not gonna break)
   • Update the ICal.tpl view to properly render the format returned by getCompleteInfo()

authx - Simplify service declaration

Overview

These tests failed on phpunit9 because upstream swapped around the semantics of assertContains().

Same issue as #25947 but applied to the phpunit-crm suite.

Overview

Same issue as #25947, but applied to the phpunit-api3 suite. These tests failed on phpunit9 because upstream swapped around the semantics of assertContains().

The specific issue is whether an array like ["1","2","3"] contains the value 2.

Before

• assertContains() in phpunit8 says yes => tests pass.
• assertContains() in phpunit9 says no => tests fail.

After

• assertContainsEquals() in both phpunit8+phpunit9 says yes => tests pass.