Skip to content

Commit

Permalink
MDL-64971 access: Ensure that the capability exists when fetching
Browse files Browse the repository at this point in the history
  • Loading branch information
@andrewnicols
andrewnicols committed Mar 6, 2019
1 parent 9074071 commit 8a2e954
Show file tree
Hide file tree
Showing 5 changed files with 145 additions and 13 deletions.
2 changes: 1 addition & 1 deletion admin/tool/lp/tests/externallib_test.php
View file
Expand Up @@ -116,7 +116,7 @@ protected function setUp() {
$this->userrole = create_role('User role', 'lpuserrole', 'learning plan user role description');

assign_capability('moodle/competency:competencymanage', CAP_ALLOW, $this->creatorrole, $syscontext->id);
assign_capability('moodle/competency:competencycompetencyconfigure', CAP_ALLOW, $this->creatorrole, $syscontext->id);
assign_capability('moodle/competency:coursecompetencyconfigure', CAP_ALLOW, $this->creatorrole, $syscontext->id);
assign_capability('moodle/competency:planmanage', CAP_ALLOW, $this->creatorrole, $syscontext->id);
assign_capability('moodle/competency:planmanagedraft', CAP_ALLOW, $this->creatorrole, $syscontext->id);
assign_capability('moodle/competency:planmanageown', CAP_ALLOW, $this->creatorrole, $syscontext->id);
Expand Down
2 changes: 1 addition & 1 deletion competency/tests/external_test.php
View file
Expand Up @@ -139,7 +139,7 @@ protected function setUp() {
$this->userrole = create_role('User role', 'userrole', 'learning plan user role description');

assign_capability('moodle/competency:competencymanage', CAP_ALLOW, $this->creatorrole, $syscontext->id);
assign_capability('moodle/competency:competencycompetencyconfigure', CAP_ALLOW, $this->creatorrole, $syscontext->id);
assign_capability('moodle/competency:coursecompetencyconfigure', CAP_ALLOW, $this->creatorrole, $syscontext->id);
assign_capability('moodle/competency:competencyview', CAP_ALLOW, $this->userrole, $syscontext->id);
assign_capability('moodle/competency:planmanage', CAP_ALLOW, $this->creatorrole, $syscontext->id);
assign_capability('moodle/competency:planmanagedraft', CAP_ALLOW, $this->creatorrole, $syscontext->id);
Expand Down
49 changes: 43 additions & 6 deletions lib/accesslib.php
View file
Expand Up @@ -330,6 +330,7 @@ function get_role_definitions_uncached(array $roleids) {
$sql = "SELECT ctx.path, rc.roleid, rc.capability, rc.permission
FROM {role_capabilities} rc
JOIN {context} ctx ON rc.contextid = ctx.id
JOIN {capabilities} cap ON rc.capability = cap.name
WHERE rc.roleid $sql
ORDER BY ctx.path, rc.roleid, rc.capability";
$rs = $DB->get_recordset_sql($sql, $params);
Expand Down Expand Up @@ -1144,7 +1145,17 @@ function is_safe_capability($capability) {
*/
function get_local_override($roleid, $contextid, $capability) {
global $DB;
return $DB->get_record('role_capabilities', array('roleid'=>$roleid, 'capability'=>$capability, 'contextid'=>$contextid));

return $DB->get_record_sql("
SELECT rc.*
FROM {role_capabilities} rc
JOIN {capability} cap ON rc.capability = cap.name
WHERE rc.roleid = :roleid AND rc.capability = :capability AND rc.contextid = :contextid", [
'roleid' => $roleid,
'contextid' => $contextid,
'capability' => $capability,

]);
}

/**
Expand Down Expand Up @@ -1290,6 +1301,11 @@ function assign_capability($capability, $permission, $roleid, $contextid, $overw
$context = context::instance_by_id($contextid);
}

// Capability must exist.
if (!$capinfo = get_capability_info($capability)) {
throw new coding_exception("Capability '{$capability}' was not found! This has to be fixed in code.");
}

if (empty($permission) || $permission == CAP_INHERIT) { // if permission is not set
unassign_capability($capability, $roleid, $context->id);
return true;
Expand Down Expand Up @@ -1337,6 +1353,11 @@ function assign_capability($capability, $permission, $roleid, $contextid, $overw
function unassign_capability($capability, $roleid, $contextid = null) {
global $DB;

// Capability must exist.
if (!$capinfo = get_capability_info($capability)) {
throw new coding_exception("Capability '{$capability}' was not found! This has to be fixed in code.");
}

if (!empty($contextid)) {
if ($contextid instanceof context) {
$context = $contextid;
Expand Down Expand Up @@ -1391,6 +1412,7 @@ function get_roles_with_capability($capability, $permission = null, $context = n
FROM {role} r
WHERE r.id IN (SELECT rc.roleid
FROM {role_capabilities} rc
JOIN {capabilities} cap ON rc.capability = cap.name
WHERE rc.capability = :capname
$contextsql
$permissionsql)";
Expand Down Expand Up @@ -2177,6 +2199,9 @@ function update_capabilities($component = 'moodle') {

$DB->insert_record('capabilities', $capability, false);

// Flush the cached, as we have changed DB.
cache::make('core', 'capabilities')->delete('core_capabilities');

if (isset($capdef['clonepermissionsfrom']) && in_array($capdef['clonepermissionsfrom'], $existingcaps)){
if ($rolecapabilities = $DB->get_records('role_capabilities', array('capability'=>$capdef['clonepermissionsfrom']))){
foreach ($rolecapabilities as $rolecapability){
Expand Down Expand Up @@ -2230,9 +2255,6 @@ function capabilities_cleanup($component, $newcapdef = null) {
if (empty($newcapdef) ||
array_key_exists($cachedcap->name, $newcapdef) === false) {

// Remove from capabilities cache.
$DB->delete_records('capabilities', array('name'=>$cachedcap->name));
$removedcount++;
// Delete from roles.
if ($roles = get_roles_with_capability($cachedcap->name)) {
foreach($roles as $role) {
Expand All @@ -2241,6 +2263,13 @@ function capabilities_cleanup($component, $newcapdef = null) {
}
}
}

// Remove from role_capabilities for any old ones.
$DB->delete_records('role_capabilities', array('capability' => $cachedcap->name));

// Remove from capabilities cache.
$DB->delete_records('capabilities', array('name' => $cachedcap->name));
$removedcount++;
} // End if.
}
}
Expand Down Expand Up @@ -2305,10 +2334,12 @@ function role_context_capabilities($roleid, context $context, $cap = '') {
}

$sql = "SELECT rc.*
FROM {role_capabilities} rc, {context} c
FROM {role_capabilities} rc
JOIN {context} c ON rc.contextid = c.id
JOIN {capabilities} cap ON rc.capability = cap.name
WHERE rc.contextid in $contexts
AND rc.roleid = ?
AND rc.contextid = c.id $search
$search
ORDER BY c.contextlevel DESC, rc.capability DESC";

$capabilities = array();
Expand Down Expand Up @@ -3024,6 +3055,7 @@ function get_switchable_roles(context $context) {
SELECT r.id, r.name, r.shortname, rn.name AS coursealias
FROM (SELECT DISTINCT rc.roleid
FROM {role_capabilities} rc
$extrajoins
$extrawhere) idlist
JOIN {role} r ON r.id = idlist.roleid
Expand Down Expand Up @@ -3286,6 +3318,7 @@ function get_users_by_capability(context $context, $capability, $fields = '', $s
$params = array_merge($params, $params2);
$sql = "SELECT rc.id, rc.roleid, rc.permission, rc.capability, ctx.path
FROM {role_capabilities} rc
JOIN {capabilities} cap ON rc.capability = cap.name
JOIN {context} ctx on rc.contextid = ctx.id
WHERE rc.contextid $incontexts AND rc.capability $incaps";

Expand Down Expand Up @@ -4381,6 +4414,7 @@ function get_roles_with_cap_in_context($context, $capability) {
$sql = "SELECT rc.id, rc.roleid, rc.permission, ctx.depth
FROM {role_capabilities} rc
JOIN {context} ctx ON ctx.id = rc.contextid
JOIN {capabilities} cap ON rc.capability = cap.name
WHERE rc.capability = :cap AND ctx.id IN ($ctxids)
ORDER BY rc.roleid ASC, ctx.depth DESC";
$params = array('cap'=>$capability);
Expand Down Expand Up @@ -4500,6 +4534,7 @@ function prohibit_is_removable($roleid, context $context, $capability) {
$sql = "SELECT ctx.id
FROM {role_capabilities} rc
JOIN {context} ctx ON ctx.id = rc.contextid
JOIN {capabilities} cap ON rc.capability = cap.name
WHERE rc.roleid = :roleid AND rc.permission = :prohibit AND rc.capability = :cap AND ctx.id IN ($ctxids)
ORDER BY ctx.depth DESC";

Expand Down Expand Up @@ -4543,6 +4578,7 @@ function role_change_permission($roleid, $context, $capname, $permission) {
$sql = "SELECT ctx.id, rc.permission, ctx.depth
FROM {role_capabilities} rc
JOIN {context} ctx ON ctx.id = rc.contextid
JOIN {capabilities} cap ON rc.capability = cap.name
WHERE rc.roleid = :roleid AND rc.capability = :cap AND ctx.id IN ($ctxids)
ORDER BY ctx.depth DESC";

Expand Down Expand Up @@ -7224,6 +7260,7 @@ function get_with_capability_join(context $context, $capability, $useridcolumn)
$defs = array();
$sql = "SELECT rc.id, rc.roleid, rc.permission, ctx.path
FROM {role_capabilities} rc
JOIN {capabilities} cap ON rc.capability = cap.name
JOIN {context} ctx on rc.contextid = ctx.id
WHERE rc.contextid $incontexts AND rc.capability $incaps";
$rcs = $DB->get_records_sql($sql, array_merge($cparams, $capsparams));
Expand Down
103 changes: 99 additions & 4 deletions lib/tests/accesslib_test.php
View file
Expand Up @@ -1696,6 +1696,101 @@ public function test_has_capability_and_friends() {
$this->assertFalse(has_all_capabilities($sca, $coursecontext, 0));
}

/**
* Test that assigning a fake cap does not return.
*/
public function test_fake_capability() {
global $DB;

$this->resetAfterTest();

$course = $this->getDataGenerator()->create_course();
$coursecontext = context_course::instance($course->id);
$teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher'), '*', MUST_EXIST);
$teacher = $this->getDataGenerator()->create_user();

$fakecapname = 'moodle/fake:capability';

role_assign($teacherrole->id, $teacher->id, $coursecontext);
$admin = $DB->get_record('user', array('username' => 'admin'));

// Test a capability which does not exist.
// Note: Do not use assign_capability because it will not allow fake caps.
$DB->insert_record('role_capabilities', (object) [
'contextid' => $coursecontext->id,
'roleid' => $teacherrole->id,
'capability' => $fakecapname,
'permission' => CAP_ALLOW,
'timemodified' => time(),
'modifierid' => 0,
]);

// Check `has_capability`.
$this->assertFalse(has_capability($fakecapname, $coursecontext, $teacher));
$this->assertDebuggingCalled("Capability \"{$fakecapname}\" was not found! This has to be fixed in code.");
$this->assertFalse(has_capability($fakecapname, $coursecontext, $admin));
$this->assertDebuggingCalled("Capability \"{$fakecapname}\" was not found! This has to be fixed in code.");

// Check `get_with_capability_sql` (with uses `get_with_capability_join`).
list($sql, $params) = get_with_capability_sql($coursecontext, $fakecapname);
$users = $DB->get_records_sql($sql, $params);

$this->assertFalse(array_key_exists($teacher->id, $users));
$this->assertFalse(array_key_exists($admin->id, $users));

// Check `get_users_by_capability`.
$users = get_users_by_capability($coursecontext, $fakecapname);

$this->assertFalse(array_key_exists($teacher->id, $users));
$this->assertFalse(array_key_exists($admin->id, $users));
}

/**
* Test that assigning a fake cap does not return.
*/
public function test_fake_capability_assign() {
global $DB;

$this->resetAfterTest();

$course = $this->getDataGenerator()->create_course();
$coursecontext = context_course::instance($course->id);
$teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher'), '*', MUST_EXIST);
$teacher = $this->getDataGenerator()->create_user();

$capability = 'moodle/fake:capability';

role_assign($teacherrole->id, $teacher->id, $coursecontext);
$admin = $DB->get_record('user', array('username' => 'admin'));

$this->expectException('coding_exception');
$this->expectExceptionMessage("Capability '{$capability}' was not found! This has to be fixed in code.");
assign_capability($capability, CAP_ALLOW, $teacherrole->id, $coursecontext);
}

/**
* Test that assigning a fake cap does not return.
*/
public function test_fake_capability_unassign() {
global $DB;

$this->resetAfterTest();

$course = $this->getDataGenerator()->create_course();
$coursecontext = context_course::instance($course->id);
$teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher'), '*', MUST_EXIST);
$teacher = $this->getDataGenerator()->create_user();

$capability = 'moodle/fake:capability';

role_assign($teacherrole->id, $teacher->id, $coursecontext);
$admin = $DB->get_record('user', array('username' => 'admin'));

$this->expectException('coding_exception');
$this->expectExceptionMessage("Capability '{$capability}' was not found! This has to be fixed in code.");
unassign_capability($capability, CAP_ALLOW, $teacherrole->id, $coursecontext);
}

/**
* Test that the caching in get_role_definitions() and get_role_definitions_uncached()
* works as intended.
Expand Down Expand Up @@ -2815,7 +2910,7 @@ public function test_permission_evaluation() {
assign_capability('moodle/site:accessallgroups', CAP_ALLOW, $CFG->defaultfrontpageroleid, $frontpagepagecontext, true);
assign_capability('mod/page:view', CAP_PREVENT, $allroles['guest'], $frontpagepagecontext, true);
assign_capability('mod/page:view', CAP_ALLOW, $allroles['user'], $frontpagepagecontext, true);
assign_capability('moodle/page:view', CAP_ALLOW, $allroles['student'], $frontpagepagecontext, true);
assign_capability('mod/page:view', CAP_ALLOW, $allroles['student'], $frontpagepagecontext, true);

assign_capability('moodle/site:accessallgroups', CAP_ALLOW, $CFG->defaultuserroleid, $frontpagecontext, true);
assign_capability('moodle/site:accessallgroups', CAP_ALLOW, $CFG->defaultfrontpageroleid, $frontpagecontext, true);
Expand Down Expand Up @@ -3443,10 +3538,10 @@ public function test_get_with_capability_sql() {
$this->assertFalse(array_key_exists($guest->id, $users));

// Test role override.
assign_capability('moodle/site:backupcourse', CAP_PROHIBIT, $teacherrole->id, $coursecontext, true);
assign_capability('moodle/site:backupcourse', CAP_ALLOW, $studentrole->id, $coursecontext, true);
assign_capability('moodle/backup:backupcourse', CAP_PROHIBIT, $teacherrole->id, $coursecontext, true);
assign_capability('moodle/backup:backupcourse', CAP_ALLOW, $studentrole->id, $coursecontext, true);

list($sql, $params) = get_with_capability_sql($coursecontext, 'moodle/site:backupcourse');
list($sql, $params) = get_with_capability_sql($coursecontext, 'moodle/backup:backupcourse');
$users = $DB->get_records_sql($sql, $params);

$this->assertFalse(array_key_exists($teacher->id, $users));
Expand Down
2 changes: 1 addition & 1 deletion mod/data/tests/externallib_test.php
View file
Expand Up @@ -274,7 +274,7 @@ public function test_view_database_not_enrolled_user() {
public function test_view_database_no_capabilities() {
// Test user with no capabilities.
// We need a explicit prohibit since this capability is allowed for students by default.
assign_capability('mod/data:viewpage', CAP_PROHIBIT, $this->studentrole->id, $this->context->id);
assign_capability('mod/data:view', CAP_PROHIBIT, $this->studentrole->id, $this->context->id);
accesslib_clear_all_caches_for_unit_testing();

$this->expectException('moodle_exception');
Expand Down

0 comments on commit 8a2e954

Please sign in to comment.