Permalink
Browse files

security overview MDL-20834 Improved strings, and 3 lists now: roles,…

… overrides and users (SQL from skodak, thanks!)
  • Loading branch information...
1 parent 676fff3 commit f9e1d25dbbceacd7701d548f084b6a5a490f53b4 @moodler moodler committed Nov 25, 2009
Showing with 82 additions and 49 deletions.
  1. +76 −45 admin/report/security/lib.php
  2. +6 −4 lang/en_utf8/report_security.php
@@ -1110,59 +1110,90 @@ function report_security_check_riskbackup($detailed=false) {
$result->status = null;
$result->link = null;
- if ($roles = get_roles_with_capability('moodle/backup:userinfo', CAP_ALLOW, get_context_instance(CONTEXT_SYSTEM))) {
- // Find all the users who have the potential ability to backup user info. Ignoring the actual backup capability for now
- // because it could easily be ALLOWed later and then these user info caps would suddenly be a problem.
- $sqluserinfo = "SELECT u.id, u.firstname, u.lastname, u.picture, u.imagealt, u.email, ra.contextid, ra.roleid
- FROM (SELECT rcx.*
- FROM {$CFG->prefix}role_capabilities rcx
- WHERE rcx.capability = 'moodle/backup:userinfo' AND rcx.permission = ".CAP_ALLOW.") rc,
- {$CFG->prefix}context c,
- {$CFG->prefix}context sc,
- {$CFG->prefix}role_assignments ra,
- {$CFG->prefix}user u
- WHERE c.id = rc.contextid
- AND (sc.path = c.path OR sc.path LIKE ".sql_concat('c.path', "'/%'")." OR c.path LIKE ".sql_concat('sc.path', "'/%'").")
- AND u.id = ra.userid AND u.deleted = 0
- AND ra.contextid = sc.id AND ra.roleid = rc.roleid AND sc.contextlevel <= ".CONTEXT_COURSE."
- GROUP BY u.id, u.firstname, u.lastname, u.picture, u.imagealt, u.email, ra.contextid, ra.roleid
- ORDER BY u.lastname, u.firstname";
-
- $usercount = count_records_sql("SELECT COUNT('x') FROM ($sqluserinfo) userinfo");
+ $syscontext = get_context_instance(CONTEXT_SYSTEM);
+
+ $systemroles = get_records_sql(
+ "SELECT DISTINCT r.*
+ FROM {$CFG->prefix}role r
+ JOIN {$CFG->prefix}role_capabilities rc ON rc.roleid = r.id
+ WHERE rc.capability = 'moodle/backup:userinfo' AND rc.contextid = $syscontext->id AND rc.permission = ".CAP_ALLOW."");
+
+ $overriddenroles = get_records_sql(
+ "SELECT DISTINCT r.*, rc.contextid
+ FROM {$CFG->prefix}role r
+ JOIN {$CFG->prefix}role_capabilities rc ON rc.roleid = r.id
+ WHERE rc.capability = 'moodle/backup:userinfo' AND rc.contextid <> $syscontext->id AND rc.permission = ".CAP_ALLOW."");
+
+ // list of users that are able to backup personal info
+ // note: "sc" is context where is role assigned,
+ // "c" is context where is role overriden or system context if in role definition
+ $sqluserinfo = "
+ FROM (SELECT rcx.*
+ FROM {$CFG->prefix}role_capabilities rcx
+ WHERE rcx.permission = ".CAP_ALLOW." AND rcx.capability = 'moodle/backup:userinfo') rc,
+ {$CFG->prefix}context c,
+ {$CFG->prefix}context sc,
+ {$CFG->prefix}role_assignments ra,
+ {$CFG->prefix}user u
+ WHERE c.id = rc.contextid
+ AND (sc.path = c.path OR sc.path LIKE ".sql_concat('c.path', "'/%'")." OR c.path LIKE ".sql_concat('sc.path', "'/%'").")
+ AND u.id = ra.userid AND u.deleted = 0
+ AND ra.contextid = sc.id AND ra.roleid = rc.roleid
+ AND sc.contextlevel <= ".CONTEXT_COURSE." AND c.contextlevel <= ".CONTEXT_COURSE."";
+
+ $usercount = count_records_sql("SELECT COUNT('x') FROM (SELECT DISTINCT u.id $sqluserinfo) userinfo");
+ $systemrolecount = empty($systemroles) ? 0 : count($systemroles);
+ $overriddenrolecount = empty($overriddenroles) ? 0 : count($overriddenroles);
+
+ $result->status = REPORT_SECURITY_WARNING; // there is always at least one admin
+ $a = (object)array('rolecount'=>$systemrolecount,'overridecount'=>$overriddenrolecount,'usercount'=>$usercount);
+ $result->info = get_string('check_riskbackup_warning', 'report_security', $a);
- $result->status = REPORT_SECURITY_WARNING;
- $a = (object)array('rolecount'=>count($roles), 'usercount'=>$usercount);
- $result->info = get_string('check_riskbackup_warning', 'report_security', $a);
+ if ($detailed) {
- if ($detailed) {
- // Make a list of roles
- foreach ($roles as $role) {
+ $result->details = ''; // Will be added to later
+
+ // Make a list of roles
+ if ($systemroles) {
+ $links = array();
+ foreach ($systemroles as $role) {
$role->url = "$CFG->wwwroot/$CFG->admin/roles/manage.php?action=edit&amp;roleid=$role->id";
- $rolelinks[] = '<li>'.get_string('check_riskbackup_editrole', 'report_security', $role).'</li>';
+ $links[] = '<li>'.get_string('check_riskbackup_editrole', 'report_security', $role).'</li>';
}
- $rolelinks = '<ul>'.implode($rolelinks).'</ul>';
+ $links = '<ul>'.implode($links).'</ul>';
+ $result->details .= get_string('check_riskbackup_details_systemroles', 'report_security', $links);
+ }
- // Get a list of users as well
- $rs = get_recordset_sql($sqluserinfo);
- $users = array();
- while ($user = rs_fetch_next_record($rs)) {
- $url = "$CFG->wwwroot/$CFG->admin/roles/assign.php?contextid=$user->contextid&amp;roleid=$user->roleid";
- $a = (object)array('fullname'=>fullname($user), 'url'=>$url, 'email'=>$user->email);
- $users[] = '<li>'.get_string('check_riskbackup_unassign', 'report_security', $a).'</li>';
+ // Make a list of overrides to roles
+ $rolelinks2 = array();
+ if ($overriddenroles) {
+ $links = array();
+ foreach ($overriddenroles as $role) {
+ $context = get_context_instance_by_id($role->contextid);
+ $role->contextname = print_context_name($context);
+ $role->url = "$CFG->wwwroot/$CFG->admin/roles/override.php?contextid=$role->contextid&amp;roleid=$role->id";
+ $links[] = '<li>'.get_string('check_riskbackup_editoverride', 'report_security', $role).'</li>';
}
- rs_close($rs);
- $users = '<ul>'.implode($users).'</ul>';
-
- // Combine as one big list of info
- $a = (object)array('roles'=>$rolelinks, 'users'=>$users);
- $result->details = get_string('check_riskbackup_detailswarning', 'report_security', $a);
+ $links = '<ul>'.implode($links).'</ul>';
+ $result->details .= get_string('check_riskbackup_details_overriddenroles', 'report_security', $links);
}
- } else {
- $result->status = REPORT_SECURITY_OK;
- $result->info = get_string('check_riskbackup_ok', 'report_security');
- if ($detailed) {
- $result->details = get_string('check_riskbackup_detailsok', 'report_security');
+ // Get a list of affected users as well
+ $rs = get_recordset_sql("SELECT DISTINCT u.id, u.firstname, u.lastname, u.picture, u.imagealt, u.email, ra.contextid, ra.roleid
+ $sqluserinfo ORDER BY u.lastname, u.firstname");
+
+ $users = array();
+ while ($user = rs_fetch_next_record($rs)) {
+ $context = get_context_instance_by_id($user->contextid);
+ $url = "$CFG->wwwroot/$CFG->admin/roles/assign.php?contextid=$user->contextid&amp;roleid=$user->roleid";
+ $a = (object)array('fullname'=>fullname($user), 'url'=>$url, 'email'=>$user->email,
+ 'contextname'=>print_context_name($context));
+ $users[] = '<li>'.get_string('check_riskbackup_unassign', 'report_security', $a).'</li>';
+ }
+ rs_close($rs);
+ if (!empty($users)) {
+ $users = '<ul>'.implode($users).'</ul>';
+ $result->details .= get_string('check_riskbackup_details_users', 'report_security', $users);
}
}
@@ -138,11 +138,13 @@
$string['check_riskadmin_name'] = 'Administrators';
$string['check_riskbackup_name'] = 'Backup of user data';
-$string['check_riskbackup_warning'] = 'Found $a->rolecount roles and $a->usercount users with the ability to backup user data.';
-$string['check_riskbackup_detailswarning'] = '<p>The following roles currently allow users to include user data in backups. Are you really sure they need to?</p>$a->roles
-<p>Because of the above roles, the following user accounts currently have the capability to make backups containing user data from the whole site. Make sure they are (a) trusted and (b) protected by strong passwords:</p>$a->users';
+$string['check_riskbackup_warning'] = 'Found $a->rolecount roles, $a->overridecount overrides and $a->usercount users with the ability to backup user data.';
+$string['check_riskbackup_details_systemroles'] = '<p>The following system roles currently allow users to include user data in backups. Please make sure this permission is necessary.</p> $a';
+$string['check_riskbackup_details_overriddenroles'] = '<p>These active overrides give users the ability to include user data in backups. Please make sure this permission is necessary.</p> $a';
+$string['check_riskbackup_details_users'] = '<p>Because of the above roles or local overrides, the following user accounts currently have permission to make backups containing private data from any users enrolled in their course. Make sure they are (a) trusted and (b) protected by strong passwords:</p> $a';
$string['check_riskbackup_editrole'] = '<a href=\"$a->url\">$a->name</a>';
-$string['check_riskbackup_unassign'] = '<a href=\"$a->url\">$a->fullname ($a->email)</a>';
+$string['check_riskbackup_editoverride'] = '<a href=\"$a->url\">$a->name in $a->contextname</a>';
+$string['check_riskbackup_unassign'] = '<a href=\"$a->url\">$a->fullname ($a->email) in $a->contextname</a>';
$string['check_riskbackup_ok'] = 'No roles explicitly allow backup of user data';
$string['check_riskbackup_detailsok'] = 'No roles explicitly allow backup of user data. However, note that admins with the \"doanything\" capability are still likely to be able to do this.';

0 comments on commit f9e1d25

Please sign in to comment.