dev/core#3671 - CRM_Utils_Recent - Alternate fix for handling unauthorized records

[totten]

(@stesi561 @demeritcowboy @eileenmcnaughton @colemanw - This is a proposed revision/alternative for #23836.)

Use cases

1. You have a user with lesser privileges who makes a request with escalated privileges (eg civicrm_api4('Contact', 'create', ['checkPermisisons'=>FALSE])). For example, submitting a webform.

   This action triggers (by way of hook or BAO) the creation of a new RecentItem. But the user doesn't have permission for this item.

   (Note that this is usually academic -- such users generally don't actually see 'Recent Items', because the webform is targeted at constituents while 'Recent Items' is targeted at staffers.)

2. Log in as a user with permission to access CiviCRM but with only limited visibility to specific contacts. Use the console to add a RecentItem that you should not have access to.

   CRM.api4('RecentItem', 'create', {values: {"entity_type":"Contact", "entity_id":172}})

Before (last release)

• (Use case 1) Creating the RecentItem fails because it cannot figure out the name/title. The whole request fails.

• (Use case 2) Creating the RecentItem fails because it cannot figure out the name/title. The AJAX call fails.

Before (current/unreleased head):

• (Use case 1) The RecentItem is created. The name/title is populated, even if you don't have permission to view it. (Unverified by r-run, but strongly expected: The link in RecentItems is broken.)

• (Use case 2) The RecentItem is created. The name/title is populated, even if you don't have permission to view it. You can use this to poll for the names of all contacts.

After (this patch):

• (Use case 1) The RecentItem is created. The name/title is a dummy placeholder. (Unverified by r-run, but strongly expected.)

• (Use case 2) The RecentItem is created. The name/title is a dummy placeholder.

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

[colemanw]

Whoops, this catch-all at the end ought to have been

Suggested change

	return $title ?? (CoreUtil::getInfoItem($entityType, 'label_field'));
	return $title ?? (CoreUtil::getInfoItem($entityType, 'title'));

[totten]

@colemanw Hrm, did I position the ts('Unknown record') poorly? I didn't actually read this line while drafting the other bits.

I guess L193 is saying: "If we didn't figure out a $title, then just use the name of the entity-type ('Contact', 'Activity', etc)." But as written, maybe it's kinda hard to get a NULL value of $title? (They're both fallbacks - just different.)

I haven't tested all these edges... but perhaps both L191 and L193 should be changing? eg

      $title = $record[$labelField] ?? NULL;
    }
    return $title ?? (CoreUtil::getInfoItem($entityType, 'title'));

Or

      $title = $record[$labelField] ?? NULL;
    }
    return $title ?? ts('%1 #%2', [
      1 => CoreUtil::getInfoItem($entityType, 'title'),
      2 => $entityId,
    ]);

[colemanw]

@totten AFAIK the line I commented on is not reachable except maybe in odd circumstances such as the record having been deleted.

The logic here all feels a little incongruent because ostensibly this is a simple function to fetch a title, but it winds up being a permission checker too.

How about checking permissions before we call this function, and actually following the $checkPermissions parameter in doing so? Like this: #23876