Add ability to temporarily override logged in user

[colemanw]

Overview

This adds a function in CRM_Core_Session to temporarily masquerade as a different user, which api4 can use to specify the acting user for certain api calls.

Before

Current user taken from session, permissions always checked against current user.

After

Current user can be overridden, permission checks respect override, e.g.

// Elevate privileges
$contactId = 300;
$handle = $session->overrideCurrentUser($contactId);

// ... Do something as $contactId, like
printf("Am I am an admin? %s",
  CRM_Core_Permission::check('administer CiviCRM') ? 'yes' : 'no');

// Restore privileges
$session->restoreCurrentUser($handle);

If the given $contactId does not have a corresponding user account (with permissions), then permissions will match the anonymous user.

Comments

Added a simple E2E test for the overrides.

This patch does not directly change the value of userID in the CRM_Core_Session - it does not intend to persistently change the active the user. Rather, this provides a momentary escalation. To accomplish this, it:

• Adds an additional static variable+function (CRM_Core_Session::getOverriddenUser()) which may be consulted on a bespoke basis to determine if there is an active override, and
• Modifies the behavior of get('userID') so to respect this value.

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

[seamuslee001]

@michaelmcandrew this might be of interest to you, @colemanw guessing this could be used especially for re-calculating recipients for a mailing right?

[michaelmcandrew]

thanks @seamuslee001!

I've made a note (see reference above).

[seamuslee001]

also paging @pfigel in case you have any thoughts on this

[pfigel]

Makes sense to me. 👍

[eileenmcnaughton]

So this change basically permits 0 to be passed in - why is it moved below the other?

[colemanw]

@eileenmcnaughton the problem was that this function wasn't differentiating NULL from 0 and they are very different.

0 = no user
null = current user

I fixed that and then moved the block down because CRM_Core_Permission::check already handles NULL and 0 correctly so there was no need to mess with the $contactID value before calling that function.

[kcristiano]

ping @christianwach can you look at this as well?

[totten]

1. Hmm, if there's going to be a static listing, then probably use Civi::$statics?

2. It seems like this query ought to be invoked any time there's a transition of the active user ID, regardless of who/how/why it's transitioning.

3. Maybe this function should fire some kind of "set user" event? It feels a little brittle for any one party to enumerate all the things that happen when the user changes.

[totten]

TBH, part of me thinks that the whole of CRM_Core_Config::singleton() and Civi::$statics should be rebuilt/swapped whenever one switches the active user (to categorically prevent leaking data via caches). On the flip side, that's probably slower (esp if there are many user changes within the PHP request-lifespan) and rather complicated, and those leak bugs are hypothetical. It's equally plausible from my POV that cache-leak vulnerabilities turn into long-term whackamole... or never happen at all...

However, I'm very much on the fence about that - just want to verbalize to see what others think.

[colemanw]

The only one I've looked at is the contact ACL cache, which is on a per-user basis so it works fine with these overrides. But you're right there may be other spots in the system where things are cached assuming the logged in user id won't change.

My original use case for this is pretty limited though. I wanted an anonymous user, authenticated with a checksum, to be able to submit an afform as that contact. So the afform.submit call internally authenticates the checksum and then sets "acting_user" param on subsequent api calls. So that's not nearly as demanding a scenario as actually "masquerading" as a different user and clicking around the site.

[totten]

I'm a little surprised PHP 7 doesn't complain about dispatching a static-function on an object. Learn something new...

[colemanw]

It's not really different from doing self::fooParentFunction() from within the object.

[christianwach]

ping @christianwach can you look at this as well?

Thanks for the heads-up @kcristiano.

I'm not sure I understand the need for this given that we already have hook_civicrm_permission_check available to escalate permissions when necessary. Am I missing something?

[seamuslee001]

@christianwach so this is about actually having the ACL rules applied as if your another user not the current user either an annon or another logged in user

[totten]

Part 1, General

I confess to vacillating a lot while digesting this, and it seems to me that there's a continuum.

• On one end, we can try to pick one specific one thing that involve permissions/identity -- e.g. just userID or just hook_civicrm_permission_check -- and make that work with masquerading
• On the other end, we can broadly audit the system for layers/options/services that are influenced directly/indirectly by permissions/identity. I took a stab at drafting a list of services/fields/layers that should (not) be toggled when (de)masquerading (although I imagine it's not perfect and would change with discussion).

The two ends of the continuum feel like a trade-off between cost/feasibility and quality/bugs:

• The narrow approach is much easier to do. But it's sort of a gamble on whether (practically-speaking) we think people will actual have problems because the ancillary data-structures grew stale when switching users.
• The broad approach is more risk-averse. It ought to prevent various bug-reports/support-issues, but it is harder.

I think it might be good to use a phone-call to:

• Talk through the other services/fields/layers - to see how important they actually are.
• Talk about how important it is to switch users within the page-request - versus changing the authentication-mode/session-initialization at the start of the request. (I've lost track of this a bit.)

Part 2, An Experiment

For the session things... I played around with generalizing the "userID override" and making a "subsession", wherein:

• A subsession may be a fork that inherits content from the main session - or it can be a blank slate.
• A subsession may be modified, and it will not affect the main session. Data in the subsession is ephemeral.

That experiment has some bits that I liked+disliked:

• 👍 - Easier to set or clean multiple identity fields (userID + ufID + ufUniqID). Easier to set a presumption of "do-no-share" when it comes to all the random unknown session data (authSrc, qfPrivateKey, profileParams, transaction.userID, etc).
• 👍 - Conceptually, it feels more accurate to say that "a subrequest with a different user should have its own (ephemeral) session" than to say "a subrequest with a different user should re-use the session and overload a specific variable".
• 😕 - I started a pivot toward stricter usage of a "stack"-style contract instead of "handle"-style contract, but my exploratory branch still mixes up the concepts... and I've changed my view. I'm back on board with Coleman's original preference for a "handle"-style contract. However, I think the design of this layer should be strictly "handles" XOR "stack". As a reader/consumer, my brain breaks down when the metaphors are mixed. In a pure-"handle" metaphor, you'd have something like the following - with explicit switching and explicit deallocation:

  $session = CRM_Core_Session::singleton();
  $alice = $session->createSubsession(['userID' => 100]);
  $bob = $session->createSubsession(['userID' => 200]);
  $carol = $session->createSubsession(['userID' => 300]);
  
  $session->setSubsession($alice);
  civicrm_api3('Note', 'create', ['subject' => 'I approve of this!'...]);
  
  $session->setSubsession($bob);
  civicrm_api3('Note', 'create', ['subject' => 'I disapprove of this!']);
  
  $session->setSubsession($alice);
  civicrm_api3('Note', 'create', ['subject' => 'But I approve most strongly!'...]);
  
  $session->setSubsession($carol);
  civicrm_api3('Note', 'create', ['subject' => 'I do not have an opinion!!!!']);
  
  $session->closeSession($bob);
  $session->closeSession($alice);
  $session->closeSession($carol);

• 👎 👍 - I'm not wild about the class-model (treating subsessions as different modes of a single session object), but it's not hugely prominent, and this is livable.
• 👎 - It needs a clearer way

[eileenmcnaughton]

Ok so in terms of how to proceed - today's decision is 'do we give up on getting this into the rc & target 5.19 instead.

The above reads like a 'yes'. However I would note that mitigating the risk / in argument of merging this my take on it is that this would be non-regressive & the only risks are for people attempting to use a new feature - in which case it could be merged & if necessary iterated on

Either way we should merge or punt & get 5.18 cut once the current MOP PRs have passed tests

[colemanw]

I'm fine with punting this to the next release while we discuss.

[eileenmcnaughton]

@colemanw @totten do we still want this merged?

[jaapjansma]

Betty and I are reviewing PR's and we came across this one. As there is no reaction on @eileenmcnaughton requests of whether this pr needs to be merged. We think it is wise to close this one.
Feel free to reopen again if you think it still needs to be merged.

[eileenmcnaughton]

re-closing as not currently on the go

[totten]

The main impetus behind this was to allow hyperlinks to Afform with an authentication token. Coleman, Seamus, and I talked this through a bit further and realized the same goal could be addressed by changing the way the authentication token works. For this, the spiritual successor is #19590.

There could be other cases where one needs to override the user temporarily, but these should be rare. Here's a sketch of how one might achieve that once the other PR is done (details subject to change):

// Make an authentication token to elevate privileges
$token = Civi::service('crypto.jwt')->encode([
  'exp' => time() + 60, // Token is only valid for the moment we need it
  'civi.cid' => 1234, // The user we want to run as
]);

// Send a sub-request with this authentication token
$handler = HandlerStack::create();
$handler->unshift(\CRM_Utils_GuzzleMiddleware::url(), 'civi_url');
$http = new \GuzzleHttp\Client(['handler' => $handler]);
$http->post('civicrm/ajax/rest', [
  'headers' => ['Authorization' => "Bearer $token"],
  'form_params' => ...,
]);