MDL-18258 fixed legacy type and improved risky default course role hints

diff --git a/admin/report/security/lib.php b/admin/report/security/lib.php
index 12fa3a18e340d3fd4d2d545d5d59aba45c047cdc..1830fc54359480e3e1a45a3eb76e95a86307d3ed 100644 (file)
--- a/admin/report/security/lib.php
+++ b/admin/report/security/lib.php
@@ -901,6 +901,19 @@ function report_security_check_courserole($detailed=false) {
 
     $roleids = array_keys($student_roles);
 
+    $sql = "SELECT DISTINCT rc.roleid
+              FROM {role_capabilities} rc
+             WHERE (rc.capability = :coursecreator OR rc.capability = :admin OR rc.capability = :teacher OR rc.capability = :editingteacher)
+                   AND rc.permission = ".CAP_ALLOW."";
+    $params = array('coursecreator'  => 'moodle/legacy:coursecreator',
+                    'admin'          => 'moodle/legacy:admin',
+                    'teacher'        => 'moodle/legacy:teacher',
+                    'editingteacher' => 'moodle/legacy:editingteacher');
+
+    $riskyroleids = $DB->get_records_sql($sql, $params);
+    $riskyroleids = array_keys($riskyroleids);
+
+
     // first test if do anything enabled - that would be really crazy!!!!!!
     list($inroles, $params) = $DB->get_in_or_equal($roleids, SQL_PARAMS_NAMED, 'r0', true);
     $params = array_merge($params, array('doanything'=>'moodle/site:doanything', 'capallow'=>CAP_ALLOW));
@@ -927,43 +940,43 @@ function report_security_check_courserole($detailed=false) {
     }
     $rs->close();
 
-    // risky caps in any level - usually very dangerous!!
+    // any XSS legacy cap does not make any sense here!
     list($inroles, $params) = $DB->get_in_or_equal($roleids, SQL_PARAMS_NAMED, 'r0', true);
-    $params = array_merge($params, array('capallow'=>CAP_ALLOW));
-    $sql = "SELECT rc.roleid, rc.contextid
-              FROM {role_capabilities} rc
-              JOIN {capabilities} cap ON cap.name = rc.capability
-             WHERE ".$DB->sql_bitand('cap.riskbitmask', (RISK_XSS | RISK_CONFIG | RISK_DATALOSS))." <> 0
-                   AND rc.permission = :capallow
-                   AND rc.roleid $inroles
-          GROUP BY rc.roleid, rc.contextid
-          ORDER BY rc.roleid, rc.contextid";
-    $rs = $DB->get_recordset_sql($sql, $params);
-    foreach($rs as $res) {
-        $roleid    = $res->roleid;
-        $contextid = $res->contextid;
-        if ($contextid == SYSCONTEXTID) {
-            $a = "$CFG->wwwroot/$CFG->admin/roles/define.php?action=view&amp;roleid=$roleid";
-        } else {
-            $a = "$CFG->wwwroot/$CFG->admin/roles/override.php?contextid=$contextid&amp;roleid=$roleid";
+    $sql = "SELECT DISTINCT c.id, c.shortname
+              FROM {course} c
+             WHERE c.defaultrole $inroles
+          ORDER BY c.sortorder";
+    if ($courses = $DB->get_records_sql($sql, $params)) {
+        foreach ($courses as $course) {
+            $a = (object)array('url'=>"$CFG->wwwroot/course/edit.php?id=$course->id", 'shortname'=>$course->shortname);
+            $problems[] = get_string('check_courserole_riskylegacy', 'report_security', $a);
         }
-        $problems[] = get_string('check_courserole_risky', 'report_security', $a);
     }
-    $rs->close();
 
-    // course creator or administrator does not make any sense here!
-    list($inroles, $params) = $DB->get_in_or_equal($roleids, SQL_PARAMS_NAMED, 'r0', true);
-    $params = array_merge($params, array('capallow'=>CAP_ALLOW, 'creator'=>'moodle/legacy:coursecreator', 'admin'=>'moodle/legacy:admin'));
-    $sql = "SELECT DISTINCT rc.roleid
-              FROM {role_capabilities} rc
-             WHERE (rc.capability = :creator OR rc.capability = :admin)
-                   AND rc.permission = :capallow
-                   AND rc.roleid $inroles";
-    if ($legacys = $DB->get_records_sql($sql, $params)) {
-        foreach ($legacys as $roleid=>$unused) {
-            $a = "$CFG->wwwroot/$CFG->admin/roles/define.php?action=view&amp;roleid=$roleid";
-            $problems[] = get_string('check_defaultcourserole_legacy', 'report_security', $a);
+    // risky caps in any level - usually very dangerous!!
+    if ($checkroles = array_diff($roleids, $riskyroleids)) {
+        list($inroles, $params) = $DB->get_in_or_equal($checkroles, SQL_PARAMS_NAMED, 'r0', true);
+        $params = array_merge($params, array('capallow'=>CAP_ALLOW));
+        $sql = "SELECT rc.roleid, rc.contextid
+                  FROM {role_capabilities} rc
+                  JOIN {capabilities} cap ON cap.name = rc.capability
+                 WHERE ".$DB->sql_bitand('cap.riskbitmask', (RISK_XSS | RISK_CONFIG | RISK_DATALOSS))." <> 0
+                       AND rc.permission = :capallow
+                       AND rc.roleid $inroles
+              GROUP BY rc.roleid, rc.contextid
+              ORDER BY rc.roleid, rc.contextid";
+        $rs = $DB->get_recordset_sql($sql, $params);
+        foreach($rs as $res) {
+            $roleid    = $res->roleid;
+            $contextid = $res->contextid;
+            if ($contextid == SYSCONTEXTID) {
+                $a = "$CFG->wwwroot/$CFG->admin/roles/define.php?action=view&amp;roleid=$roleid";
+            } else {
+                $a = "$CFG->wwwroot/$CFG->admin/roles/override.php?contextid=$contextid&amp;roleid=$roleid";
+            }
+            $problems[] = get_string('check_courserole_risky', 'report_security', $a);
         }
+        $rs->close();
     }
 
 

diff --git a/lang/en_utf8/report_security.php b/lang/en_utf8/report_security.php
index efb88b1481400d5e5e82a03d79115604287f42a6..9e9b7c96ab379516e4ddbe37f9dcf52b45638233 100644 (file)
--- a/lang/en_utf8/report_security.php
+++ b/lang/en_utf8/report_security.php
@@ -32,7 +32,7 @@ $string['check_courserole_anything'] = 'The \"doanything\" capability must not b
 $string['check_courserole_details'] = '<p>Each course has one default enrolment role specified. Please make sure no risky capabilities are allowed for this role.</p>
 <p>The only supported legacy type for the default course role is <em>Student</em>.</p>';
 $string['check_courserole_error'] = 'Incorrectly defined default course roles detected!';
-$string['check_courserole_legacy'] = 'Unsupported legacy type detected in the <a href=\"$a\">role</a>.';
+$string['check_courserole_riskylegacy'] = 'Risky legacy type detected in <a href=\"$a->url\">$a->shortname</a>.';
 $string['check_courserole_name'] = 'Default roles (courses)';
 $string['check_courserole_notyet'] = 'Used only default course role.';
 $string['check_courserole_ok'] = 'Default course role definitions is OK.';
@@ -42,7 +42,7 @@ $string['check_defaultcourserole_anything'] = 'The \"doanything\" capability mus
 $string['check_defaultcourserole_details'] = '<p>The default student role for course enrolment specifies the default role for courses. Please make sure no risky capabilities are allowed in this role.</p>
 <p>The only supported legacy type for default role is <em>Student</em>.</p>';
 $string['check_defaultcourserole_error'] = 'Incorrectly defined default course role \"$a\" detected!';
-$string['check_defaultcourserole_legacy'] = 'Unsupported legacy type detected.';
+$string['check_defaultcourserole_legacy'] = 'Risky legacy type detected.';
 $string['check_defaultcourserole_name'] = 'Default course role (global)';
 $string['check_defaultcourserole_notset'] = 'Default role is not set.';
 $string['check_defaultcourserole_ok'] = 'Site default role definition is OK.';