Skip to content

Commit

Permalink
MDL-67748 admin: Improve get_missing_capabilities_by_users()
Browse files Browse the repository at this point in the history 
The previous implementation falsely reported all implicit capabilities
inherited from the authenticated user archetype. That caused a lot of
capabilities reported as missing, even if they were correctly granted.

This new implementation uses a different logic. Instead of seeking for
explicitly assigned capabilities, it searches for capabilities that are
not assigned to any of the user's role across the system.

Please refer to the inline documentation. This should be still used for
informative reports only, not for actual permissions evaluation. The
context has been ignored here, as well as all the overrides etc. This
patch just makes it a lesser evil.
  • Loading branch information
@mudrd8mz
mudrd8mz committed Mar 15, 2021
1 parent cd7c805 commit 2a189a9
Show file tree
Hide file tree
Showing 3 changed files with 150 additions and 28 deletions.
113 changes: 85 additions & 28 deletions webservice/lib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -623,11 +623,16 @@ public function get_service_required_capabilities($serviceid) {
* as the front end does not display it itself. In pratice,
* admins would like the info, for more info you can follow: MDL-29962
*
* @deprecated since Moodle 3.11 in MDL-67748 without a replacement.
* @todo MDL-70187 Please delete this method completely in Moodle 4.3, thank you.
* @param int $userid user id
* @return array
*/
public function get_user_capabilities($userid) {
global $DB;

debugging('webservice::get_user_capabilities() has been deprecated.', DEBUG_DEVELOPER);

//retrieve the user capabilities
$sql = "SELECT DISTINCT rc.id, rc.capability FROM {role_capabilities} rc, {role_assignments} ra
WHERE rc.roleid=ra.roleid AND ra.userid= ? AND rc.permission = ?";
Expand All @@ -640,45 +645,97 @@ public function get_user_capabilities($userid) {
}

/**
* Get missing user capabilities for a given service
* WARNING: do not use this "broken" function. It was created in the goal to display some capabilities
* required by users. In theory we should not need to display this kind of information
* as the front end does not display it itself. In pratice,
* admins would like the info, for more info you can follow: MDL-29962
* Get missing user capabilities for the given service's functions.
*
* @param array $users users
* @param int $serviceid service id
* @return array of missing capabilities, keys being the user ids
* Every external function can declare some required capabilities to allow for easier setup of the web services.
* However, that is supposed to be used for informational admin report only. There is no automatic evaluation of
* the declared capabilities and the context of the capability evaluation is ignored. Also, actual capability
* evaluation is much more complex as it allows for overrides etc.
*
* Returned are capabilities that the given users do not seem to have assigned anywhere at the site and that should
* be checked by the admin.
*
* Do not use this method for anything else, particularly not for any security related checks. See MDL-29962 for the
* background of why we have this - there are arguments for dropping this feature completely.
*
* @param array $users List of users to check, consisting of objects, arrays or integer ids.
* @param int $serviceid The id of the external service to check.
* @return array List of missing capabilities: (int)userid => array of (string)capabilitynames
*/
public function get_missing_capabilities_by_users($users, $serviceid) {
public function get_missing_capabilities_by_users(array $users, int $serviceid): array {
global $DB;
$usersmissingcaps = array();

//retrieve capabilities required by the service
$servicecaps = $this->get_service_required_capabilities($serviceid);
// The following are default capabilities for all authenticated users and we will assume them granted.
$commoncaps = get_default_capabilities('user');

// Get the list of additional capabilities required by the service.
$servicecaps = [];
foreach ($this->get_service_required_capabilities($serviceid) as $service => $caps) {
foreach ($caps as $cap) {
if (empty($commoncaps[$cap])) {
$servicecaps[$cap] = true;
}
}
}

if (empty($servicecaps)) {
return [];
}

//retrieve users missing capabilities
// Prepare a list of user ids we want to check.
$userids = [];
foreach ($users as $user) {
//cast user array into object to be a bit more flexible
if (is_array($user)) {
$user = (object) $user;
if (is_object($user) && isset($user->id)) {
$userids[$user->id] = true;
} else if (is_array($user) && isset($user['id'])) {
$userids[$user['id']] = true;
} else {
throw new coding_exception('Unexpected format of users list in webservice::get_missing_capabilities_by_users().');
}
$usercaps = $this->get_user_capabilities($user->id);
}

//detect the missing capabilities
foreach ($servicecaps as $functioname => $functioncaps) {
foreach ($functioncaps as $functioncap) {
if (!array_key_exists($functioncap, $usercaps)) {
if (!isset($usersmissingcaps[$user->id])
or array_search($functioncap, $usersmissingcaps[$user->id]) === false) {
$usersmissingcaps[$user->id][] = $functioncap;
}
}
}
// Prepare a matrix of missing capabilities x users - consider them all missing by default.
foreach (array_keys($userids) as $userid) {
foreach (array_keys($servicecaps) as $capname) {
$matrix[$userid][$capname] = true;
}
}

list($capsql, $capparams) = $DB->get_in_or_equal(array_keys($servicecaps), SQL_PARAMS_NAMED, 'paramcap');
list($usersql, $userparams) = $DB->get_in_or_equal(array_keys($userids), SQL_PARAMS_NAMED, 'paramuser');

$sql = "SELECT c.name AS capability, u.id AS userid
FROM {capabilities} c
JOIN {role_capabilities} rc ON c.name = rc.capability
JOIN {role_assignments} ra ON ra.roleid = rc.roleid
JOIN {user} u ON ra.userid = u.id
WHERE rc.permission = :capallow
AND c.name {$capsql}
AND u.id {$usersql}";

$params = $capparams + $userparams + [
'capallow' => CAP_ALLOW,
];

$rs = $DB->get_recordset_sql($sql, $params);

foreach ($rs as $record) {
// If there was a potential role assignment found that might grant the user the given capability,
// remove it from the matrix. Again, we ignore all the contexts, prohibits, prevents and other details
// of the permissions evaluations. See the function docblock for details.
unset($matrix[$record->userid][$record->capability]);
}

$rs->close();

foreach ($matrix as $userid => $caps) {
$matrix[$userid] = array_keys($caps);
if (empty($matrix[$userid])) {
unset($matrix[$userid]);
}
}

return $usersmissingcaps;
return $matrix;
}

/**
Expand Down
59 changes: 59 additions & 0 deletions webservice/tests/lib_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,65 @@ public function test_update_token_lastaccess() {
$this->assertEquals($before + 60, $token->lastaccess);
}

/**
* Tests for the {@see webservice::get_missing_capabilities_by_users()} implementation.
*/
public function test_get_missing_capabilities_by_users() {
global $DB;

$this->resetAfterTest(true);
$wsman = new webservice();

$user1 = $this->getDataGenerator()->create_user();
$user2 = $this->getDataGenerator()->create_user();
$user3 = $this->getDataGenerator()->create_user();

// Add a test web service.
$serviceid = $wsman->add_external_service((object)[
'name' => 'Test web service',
'enabled' => 1,
'requiredcapability' => '',
'restrictedusers' => false,
'component' => 'moodle',
'downloadfiles' => false,
'uploadfiles' => false,
]);

// Add a function to the service that does not declare any capability as required.
$wsman->add_external_function_to_service('core_webservice_get_site_info', $serviceid);

// Users can be provided as an array of objects, arrays or integers (ids).
$this->assertEmpty($wsman->get_missing_capabilities_by_users([$user1, array($user2), $user3->id], $serviceid));

// Add a function to the service that declares some capability as required, but that capability is common for
// any user. Here we use 'core_message_delete_conversation' which declares 'moodle/site:deleteownmessage' which
// in turn is granted to the authenticated user archetype by default.
$wsman->add_external_function_to_service('core_message_delete_conversation', $serviceid);

// So all three users should have this capability implicitly.
$this->assertEmpty($wsman->get_missing_capabilities_by_users([$user1, $user2, $user3], $serviceid));

// Add a function to the service that declares some non-common capability. Here we use
// 'core_group_add_group_members' that wants 'moodle/course:managegroups'.
$wsman->add_external_function_to_service('core_group_add_group_members', $serviceid);

// Make it so that the $user1 has the capability in some course.
$course1 = $this->getDataGenerator()->create_course();
$this->getDataGenerator()->enrol_user($user1->id, $course1->id, 'editingteacher');

// Check that no missing capability is reported for the $user1. We don't care at what actual context the
// external function call will evaluate the permission. We just check that there is a chance that the user has
// the capability somewhere.
$this->assertEmpty($wsman->get_missing_capabilities_by_users([$user1], $serviceid));

// But there is no place at the site where the capability would be granted to the other two users, so it is
// reported as missing.
$missing = $wsman->get_missing_capabilities_by_users([$user1, $user2, $user3], $serviceid);
$this->assertArrayNotHasKey($user1->id, $missing);
$this->assertContains('moodle/course:managegroups', $missing[$user2->id]);
$this->assertContains('moodle/course:managegroups', $missing[$user3->id]);
}

/**
* Utility method that tests the parameter type of a method info's input/output parameter.
*
Expand Down
6 changes: 6 additions & 0 deletions webservice/upgrade.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@ information provided here is intended especially for developers.

This information is intended for authors of webservices, not people writing webservice clients.

=== 3.11 ===

* The method webservice::get_user_capabilities() is deprecated now without a replacement. It has been used
internally only to populate the list of missing capabilities. That functionality has been improved so that
it no longer needs this standalone method.

=== 3.10 ===

* The class externallib_advanced_testcase, used in unit tests, has a new function called "configure_filters" to easily configure filters for external functions testing.
Expand Down

0 comments on commit 2a189a9

Please sign in to comment.