Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-11313

Removing [moodle/role:assign] as Requisite for Switch Roles

    XMLWordPrintable

Details

    • MOODLE_19_STABLE
    • MOODLE_19_STABLE

    Description

      Moodle currently requires the [moodle/role:assign] capability for anyone to perform a role_switch(). This does not make too much sense because Moodle has a separate capability [moodle/role:switchroles] which is also required for role switching. As Jim Williamson pointed out in MDL-10216, "these two very different uses of roles [should] be separated, if possible. One use is required by the system for security reasons, the other is required by end-users such as Instructor and Course Creators for display and information purposes."

      Yu Zhang explained that we should only allow users to switch to roles they have the ability to assign - otherwise, we could end up with security issues. However, the ability to assign and the [moodle/role:assign] capability are not necessarily the same thing. The mdl_role_allow_assign table is populated even when [moodle/role:assign] is set PROHIBIT, so we could use that data even without the [moodle/role:assign] capability.

      It appears that [moodle/role:assign] may have become required almost simply because of the desire to recycle the existing functions get_assignable_roles() and user_can_assign().

      role_switch() & switchroles_form() >> get_assignable_roles() >> user_can_assign() >> has_capability()

      I propose the creation of two new functions get_switchable_roles() and user_can_switch(). We could simply add an extra parameter and pass it through the functional chain, but it seems cleaner simply to create two new functions in accesslib instead.

      function get_switchable_roles ($context, $field="name") {

      $options = array();

      if ($roles = get_all_roles()) {
      foreach ($roles as $role) {
      if (user_can_switch($context, $role->id)) {
      $options[$role->id] = strip_tags(format_string($role->{$field}, true));
      }
      }
      }
      return $options;
      }

      function user_can_switch($context, $targetroleid) {

      // pull out all active roles of this user from this context(or above)
      if ($userroles = get_user_roles($context)) {
      foreach ($userroles as $userrole) {
      // if any in the role_allow_override table, then it's ok
      if (get_record('role_allow_assign', 'roleid', $userrole->roleid, 'allowassign', $targetroleid))

      { return true; }

      }
      }

      return false;
      }

      Then, we simply need to modify role_switch() in accesslib and switchroles_form() in weblib.

      In role_switch():

      FROM:

      if (!$roles = get_assignable_roles($context))

      { return false; }

      TO:

      if($CFG->no_assign_req_for_switch_roles){
      if (!$roles = get_switchable_roles($context))

      { return false; }
      }else{
      if (!$roles = get_assignable_roles($context)) { return false; }

      }

      In switchroles_form():

      FROM:

      if (!$roles = get_assignable_roles($context))

      { return ''; // Nothing to show! }

      TO:

      if($CFG->no_assign_req_for_switch_roles){
      if (!$roles = get_switchable_roles($context))

      { return ''; // Nothing to show! }
      }else{
      if (!$roles = get_assignable_roles($context)) { return ''; // Nothing to show! }

      }

      Implemented this on a DEV system without any negative responses so far.

      An added reason for implementing this is that [moodle/role:assign] actually proves a FERPA/privacy violation for schools in the United States due to the right-side list of all users and email in the system. Thus we've had to turn it off. However, teachers find the switch roles tool extremely useful, so they still want that ability. As mdl_role_allow_assign does not remove entries when prohibit [moodle/role:assign], we can get away with using these functions instead and just skipping the has_capability() check.

      Attachments

        Issue Links

          Activity

            People

              jerome Jérôme Mouneyrac
              ebollens Eric Bollens
              Tim Hunt Tim Hunt
              Votes:
              12 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:
                13/May/09