Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to temporarily override logged in user #15151

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions CRM/ACL/API.php
Original file line number Diff line number Diff line change
Expand Up @@ -113,18 +113,18 @@ public static function whereClause(
}
}

if (!$contactID) {
$contactID = CRM_Core_Session::getLoggedInContactID();
}
$contactID = (int) $contactID;

// first see if the contact has edit / view all permission
if (CRM_Core_Permission::check('edit all contacts', $contactID) ||
($type == self::VIEW && CRM_Core_Permission::check('view all contacts', $contactID))
) {
return $deleteClause;
}

if (is_null($contactID)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

$contactID = CRM_Core_Session::getLoggedInContactID();
}
$contactID = (int) $contactID;

$whereClause = CRM_ACL_BAO_ACL::whereClause($type,
$tables,
$whereTables,
Expand Down
7 changes: 7 additions & 0 deletions CRM/Core/Permission.php
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,20 @@ public static function getPermission() {
*
* @param int $contactId
* Contact id to check permissions for. Defaults to current logged-in user.
* Pass 0 for no user (anonymous)
*
* @return bool
* true if contact has permission(s), else false
*/
public static function check($permissions, $contactId = NULL) {
$permissions = (array) $permissions;
// If the logged in contact id is being overridden, use the substitute contactId
$contactId = $contactId ?? CRM_Core_Session::getOverriddenUser();
$userId = CRM_Core_BAO_UFMatch::getUFId($contactId);
// If contact has no associated user, set to 0 for anonymous (logged-out)
if ($contactId === 0 || ($contactId && !$userId)) {
$userId = 0;
}

/** @var CRM_Core_Permission_Temp $tempPerm */
$tempPerm = CRM_Core_Config::singleton()->userPermissionTemp;
Expand Down
8 changes: 2 additions & 6 deletions CRM/Core/Permission/Backdrop.php
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,10 @@ class CRM_Core_Permission_Backdrop extends CRM_Core_Permission_DrupalBase {
*
* @param string $str
* The permission to check.
*
* @param int $userId
* NULL = current user; 0 = anonymous (logged-out)
*
* @return bool
* true if yes, else false
*/
public function check($str, $userId = NULL) {
$str = $this->translatePermission($str, 'Drupal', [
Expand All @@ -85,10 +84,7 @@ public function check($str, $userId = NULL) {
return TRUE;
}
if (function_exists('user_access')) {
$account = NULL;
if ($userId) {
$account = user_load($userId);
}
$account = is_numeric($userId) ? user_load($userId) : NULL;
return user_access($str, $account);
}
return TRUE;
Expand Down
8 changes: 2 additions & 6 deletions CRM/Core/Permission/Drupal.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,10 @@ class CRM_Core_Permission_Drupal extends CRM_Core_Permission_DrupalBase {
*
* @param string $str
* The permission to check.
*
* @param int $userId
* NULL = current user; 0 = anonymous (logged-out)
*
* @return bool
* true if yes, else false
*/
public function check($str, $userId = NULL) {
$str = $this->translatePermission($str, 'Drupal', [
Expand All @@ -84,10 +83,7 @@ public function check($str, $userId = NULL) {
return TRUE;
}
if (function_exists('user_access')) {
$account = NULL;
if ($userId) {
$account = user_load($userId);
}
$account = is_numeric($userId) ? user_load($userId) : NULL;
return user_access($str, $account);
}
return TRUE;
Expand Down
8 changes: 2 additions & 6 deletions CRM/Core/Permission/Drupal6.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,10 @@ class CRM_Core_Permission_Drupal6 extends CRM_Core_Permission_DrupalBase {
*
* @param string $str
* The permission to check.
*
* @param int $userId
* NULL = current user; 0 = anonymous (logged-out)
*
* @return bool
* true if yes, else false
*/
public function check($str, $userId = NULL) {
$str = $this->translatePermission($str, 'Drupal6', [
Expand All @@ -84,10 +83,7 @@ public function check($str, $userId = NULL) {
return TRUE;
}
if (function_exists('user_access')) {
$account = NULL;
if ($userId) {
$account = user_load($userId);
}
$account = is_numeric($userId) ? user_load($userId) : NULL;
return user_access($str, $account);
}
return TRUE;
Expand Down
4 changes: 2 additions & 2 deletions CRM/Core/Permission/Drupal8.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ class CRM_Core_Permission_Drupal8 extends CRM_Core_Permission_DrupalBase {
*
* @param string $str
* The permission to check.
*
* @param int $userId
* NULL = current user; 0 = anonymous (logged-out)
*
* @return bool
*/
Expand All @@ -59,7 +59,7 @@ public function check($str, $userId = NULL) {
if ($str == CRM_Core_Permission::ALWAYS_ALLOW_PERMISSION) {
return TRUE;
}
$acct = $userId ? \Drupal\user\Entity\User::load($userId) : \Drupal::currentUser();
$acct = is_numeric($userId) ? \Drupal\user\Entity\User::load((int) $userId) : \Drupal::currentUser();
return $acct->hasPermission($str);
}

Expand Down
8 changes: 3 additions & 5 deletions CRM/Core/Permission/Joomla.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,14 @@ class CRM_Core_Permission_Joomla extends CRM_Core_Permission_Base {
* @param string $str
* The permission to check.
* @param int $userId
* NULL = current user; 0 = anonymous (logged-out)
*
* @return bool
* true if yes, else false
*/
public function check($str, $userId = NULL) {
$config = CRM_Core_Config::singleton();
// JFactory::getUser does strict type checking, so convert falesy values to NULL
if (!$userId) {
$userId = NULL;
}
// JFactory::getUser does strict type checking, so convert non-numeric values to NULL
$userId = is_numeric($userId) ? (int) $userId : NULL;

$translated = $this->translateJoomlaPermission($str);
if ($translated === CRM_Core_Permission::ALWAYS_DENY_PERMISSION) {
Expand Down
15 changes: 7 additions & 8 deletions CRM/Core/Permission/WordPress.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ class CRM_Core_Permission_WordPress extends CRM_Core_Permission_Base {
* @param string $str
* The permission to check.
* @param int $userId
* NULL = current user; 0 = anonymous (logged-out)
*
* @return bool
* true if yes, else false
*/
public function check($str, $userId = NULL) {
// Generic cms 'administer users' role tranlates to users with the 'edit_users' capability' in WordPress
Expand All @@ -72,20 +72,19 @@ public function check($str, $userId = NULL) {
require_once ABSPATH . WPINC . '/pluggable.php';

// for administrators give them all permissions
if (!function_exists('current_user_can')) {
if (!function_exists('current_user_can') && !is_numeric($userId)) {
return TRUE;
}

$user = $userId ? get_userdata($userId) : wp_get_current_user();

if ($user->has_cap('super admin') || $user->has_cap('administrator')) {
return TRUE;
}
$user = is_numeric($userId) ? get_userdata((int) $userId) : wp_get_current_user();

// Make string lowercase and convert spaces into underscore
$str = CRM_Utils_String::munge(strtolower($str));

if ($user->exists()) {
if ($user && $user->exists()) {
if ($user->has_cap('super admin') || $user->has_cap('administrator')) {
return TRUE;
}
// Check whether the logged in user has the capabilitity
if ($user->has_cap($str)) {
return TRUE;
Expand Down
68 changes: 68 additions & 0 deletions CRM/Core/Session.php
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,13 @@ class CRM_Core_Session {
*/
static private $_singleton = NULL;

/**
* Stack of contact ids when overriding current user
*
* @var array
*/
static private $userOverride = [];

/**
* Constructor.
*
Expand Down Expand Up @@ -250,6 +257,11 @@ public function get($name, $prefix = NULL) {
// create session scope
$this->createScope($prefix, TRUE);

if (!$prefix && $name == 'userID' && self::$userOverride) {
end(self::$userOverride);
return current(self::$userOverride);
}

if (empty($this->_session) || empty($this->_session[$this->_key])) {
return NULL;
}
Expand Down Expand Up @@ -573,4 +585,60 @@ public function isEmpty() {
return empty($_SESSION);
}

/**
* Temporarily masquerade as a different user.
*
* Note: the override is stored in a static variable so does not persist past this page request.
*
* Returns a handle to identify this override so you can revert it later.
*
* @param int $contactId
* Contact id to spoof as current user.
* @param string|int $handle
* Specify your own handle if you wish.
* @return bool|string|int
* Handle if anything was done, FALSE otherwise
*/
public function overrideCurrentUser($contactId, $handle = NULL) {
if ($contactId == $this->get('userID')) {
// Current user is already $contactId; nothing to do
return FALSE;
}
$handle = $handle ?? uniqid();
self::$userOverride[$handle] = $contactId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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.

Copy link
Member

@totten totten Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return $handle;
}

/**
* Reverts $this->overrideCurrentUser
*
* @param string|int $handle
* Optionally specify the override to revert, else it will just
* pop the latest override off the stack.
*
* @return bool
* Whether or not anything was done.
*/
public function restoreCurrentUser($handle = NULL) {
if (self::$userOverride) {
$handle = $handle ?? array_reverse(array_keys(self::$userOverride))[0];
if (array_key_exists($handle, self::$userOverride)) {
unset(self::$userOverride[$handle]);
return TRUE;
}
}
return FALSE;
}

/**
* If the contact id of the logged-in user has been overridden, return it.
*
* Otherwise return null to indicate the current user is not being overridden.
*
* @return int|null
*/
public static function getOverriddenUser() {
return self::$userOverride ? self::getLoggedInContactID() : NULL;
}

}
52 changes: 52 additions & 0 deletions tests/phpunit/E2E/Core/UserOverrideTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php

namespace E2E\Core;

/**
* Class UserOverrideTest
*
* Check that overriding session user behaves as expected.
*
* @package E2E\Core
* @group e2e
*/
class UserOverrideTest extends \CiviEndToEndTestCase {

protected function setUp() {
parent::setUp();
}

protected function tearDown() {
while (\CRM_Core_Session::singleton()->restoreCurrentUser()) {
// Loop until all overrides are cleared
}
}

public function testOverride() {
$session = \CRM_Core_Session::singleton();
$originalUser = $session::getLoggedInContactID();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Set user CID to 2
$o1 = $session->overrideCurrentUser(2);
$this->assertEquals(2, $session::getLoggedInContactID());
// Set user CID to 3
$o2 = $session->overrideCurrentUser(3);
$this->assertEquals(3, $session::getLoggedInContactID());
// Set user CID to 4
$o3 = $session->overrideCurrentUser(4);
// Clear the second override
$this->assertTrue($session->restoreCurrentUser($o2));
// Latest override should still stand
$this->assertEquals(4, $session::getLoggedInContactID());
// Clear the last override, should revert to the one remaining override
$this->assertTrue($session->restoreCurrentUser());
$this->assertEquals(2, $session::getLoggedInContactID());
$this->assertEquals(2, $session->getOverriddenUser());
// Clear the final override
$this->assertTrue($session->restoreCurrentUser());
$this->assertEquals($originalUser, $session::getLoggedInContactID());
// Assert there are no overrides left to clear
$this->assertNull($session->getOverriddenUser());
$this->assertFalse($session->restoreCurrentUser());
}

}