Moodle
  1. Moodle
  2. MDL-11313

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

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9
    • Fix Version/s: 1.9.5
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      36722

      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.

      1. MDL-11313.patch
        4 kB
        Jérôme Mouneyrac
      2. moode_chg_tmprole.txt
        3 kB
        Dirk Winning
      1. switch_role_to_dropdown.png
        3 kB

        Issue Links

          Activity

          Hide
          Hans de Zwart added a comment -

          This problem actually leads to another bug: when I give the role "Non-editing Teacher" the capability to do a role switch ( via moodle/role:switchroles) the appropriate dropdown does not appear (the one that is displayed in the attachment).

          This needs to be fixed!

          Show
          Hans de Zwart added a comment - This problem actually leads to another bug: when I give the role "Non-editing Teacher" the capability to do a role switch ( via moodle/role:switchroles) the appropriate dropdown does not appear (the one that is displayed in the attachment). This needs to be fixed!
          Hide
          Eric Bollens added a comment -

          Non-editing teacher does not have the moodle/role:assign capability, nor does it have any mdl_role_allow_assign values. Hence, moodle/role:switchrole shall have no effect until both problems are fixed. My solution, unfortunately, still requires mdl_role_allow_assign values to be set (we could populate an additional table possibly), but it removes the moodle/role:assign requisite.

          Show
          Eric Bollens added a comment - Non-editing teacher does not have the moodle/role:assign capability, nor does it have any mdl_role_allow_assign values. Hence, moodle/role:switchrole shall have no effect until both problems are fixed. My solution, unfortunately, still requires mdl_role_allow_assign values to be set (we could populate an additional table possibly), but it removes the moodle/role:assign requisite.
          Hide
          Dirk Winning added a comment -

          I just implemented this solution (before finding this bug-entry ) in exactly the same way in our production-env.
          We have no assign-cap on at all, so it's a fine solution for us to rely on the assignment-table, but only for use with switchroles.
          Works well up to now.

          Show
          Dirk Winning added a comment - I just implemented this solution (before finding this bug-entry ) in exactly the same way in our production-env. We have no assign-cap on at all, so it's a fine solution for us to rely on the assignment-table, but only for use with switchroles. Works well up to now.
          Hide
          Dirk Winning added a comment -

          addition to above post
          greets

          Show
          Dirk Winning added a comment - addition to above post greets
          Hide
          Martin Dougiamas added a comment -

          I agree it seems to make more sense to just look at the role_allow_assign table, especially since we have moodle/role:switchroles (I think that was added later, actually).

          Show
          Martin Dougiamas added a comment - I agree it seems to make more sense to just look at the role_allow_assign table, especially since we have moodle/role:switchroles (I think that was added later, actually).
          Hide
          Martin Dougiamas added a comment -

          Jerome can you come up with a MOODLE_19_STABLE patch for this?

          Show
          Martin Dougiamas added a comment - Jerome can you come up with a MOODLE_19_STABLE patch for this?
          Hide
          Petr Škoda added a comment -

          I do not agree with this proposal for STABLE branch, the problem is that it will encourage changes in role assign matrix, but it would have consequences for safe and normal overrides too - not really a good idea for stable branch imho.

          My +1 to add new switchrole matrix and remove the role assign requirement only in HEAD.

          Show
          Petr Škoda added a comment - I do not agree with this proposal for STABLE branch, the problem is that it will encourage changes in role assign matrix, but it would have consequences for safe and normal overrides too - not really a good idea for stable branch imho. My +1 to add new switchrole matrix and remove the role assign requirement only in HEAD.
          Hide
          Eric Bollens added a comment -

          Petr, I agree that ultimately, a new role_allow_switch would be ideal, since the two processes are fairly different. For your concerns with STABLE, though, realize that in its current state, switch roles is useless if you've denied moodle/role:assign, which US institutions often have to do because of an interpretation of FERPA (since teachers don't have a need to know to see every student in the school listed).

          Show
          Eric Bollens added a comment - Petr, I agree that ultimately, a new role_allow_switch would be ideal, since the two processes are fairly different. For your concerns with STABLE, though, realize that in its current state, switch roles is useless if you've denied moodle/role:assign, which US institutions often have to do because of an interpretation of FERPA (since teachers don't have a need to know to see every student in the school listed).
          Hide
          Jérôme Mouneyrac added a comment -

          I'm writting a little comment here in order to make this issue alive again. I'm asking some other opinions at Moodle HQ as this issue needs to be resolved (old issue + lots of votes).

          Petr suggested to put a new system matrix on 2.0. I'll write soon a new issue for 2.0.

          A decision still needs to be done for 1.9... keep in tunes

          Show
          Jérôme Mouneyrac added a comment - I'm writting a little comment here in order to make this issue alive again. I'm asking some other opinions at Moodle HQ as this issue needs to be resolved (old issue + lots of votes). Petr suggested to put a new system matrix on 2.0. I'll write soon a new issue for 2.0. A decision still needs to be done for 1.9... keep in tunes
          Hide
          Jérôme Mouneyrac added a comment -

          I created a new issue for 2.0: http://tracker.moodle.org/browse/MDL-18132

          Show
          Jérôme Mouneyrac added a comment - I created a new issue for 2.0: http://tracker.moodle.org/browse/MDL-18132
          Hide
          Tim Hunt added a comment -

          By default, Teacher and Admin have both moodle/role:assign and moodle/role:switchroles.

          Therefore, if we just remove the lines

          if (!has_capability('moodle/role:assign', $context))

          { return array(); }

          from the function get_assignable_roles_for_switchrole in lib/accesslib.php, then moodle/role:assign and moodle/role:switchroles become independent, which is what people seem to expect. And in a default Moodle install, no behaviour changes.

          Why don't we just do that?

          And then in HEAD only, add a database table / settings page page of checkboxes 'Allow role switching', to go with 'Allow roles assignments' and 'Allow role overrides'.

          By the way, this statement above is incorrect: "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()."

          You need both moodle/role:assign and the role_allow_assign table because the capability lets you control which contexts a user may assign roles in, while the table controls which roles they may assign there.

          Show
          Tim Hunt added a comment - By default, Teacher and Admin have both moodle/role:assign and moodle/role:switchroles. Therefore, if we just remove the lines if (!has_capability('moodle/role:assign', $context)) { return array(); } from the function get_assignable_roles_for_switchrole in lib/accesslib.php, then moodle/role:assign and moodle/role:switchroles become independent, which is what people seem to expect. And in a default Moodle install, no behaviour changes. Why don't we just do that? And then in HEAD only, add a database table / settings page page of checkboxes 'Allow role switching', to go with 'Allow roles assignments' and 'Allow role overrides'. By the way, this statement above is incorrect: "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()." You need both moodle/role:assign and the role_allow_assign table because the capability lets you control which contexts a user may assign roles in, while the table controls which roles they may assign there.
          Hide
          Petr Škoda added a comment -

          The main concern here is security - nothing else. We have to prevent userfrom getting accidentally more capabilities with switch role feature.

          Show
          Petr Škoda added a comment - The main concern here is security - nothing else. We have to prevent userfrom getting accidentally more capabilities with switch role feature.
          Hide
          Tim Hunt added a comment -

          Sure, but that will only happen with non-default configurations, and it should be easy to add the appropriate tests to the security overview report. (Not that I am voluteering .

          It makes the interface more intuitive to separate the two capabilities.

          It makes it very hard to know what RISKS to associate with the switch-role capability, but I guess it is the same as assign now.

          I know why you are concerned, but I think we have to let people do this. We just have to make it easy for people to do it securely, and hard for people to shoot themselves in the foot. Although people will anyway.

          Show
          Tim Hunt added a comment - Sure, but that will only happen with non-default configurations, and it should be easy to add the appropriate tests to the security overview report. (Not that I am voluteering . It makes the interface more intuitive to separate the two capabilities. It makes it very hard to know what RISKS to associate with the switch-role capability, but I guess it is the same as assign now. I know why you are concerned, but I think we have to let people do this. We just have to make it easy for people to do it securely, and hard for people to shoot themselves in the foot. Although people will anyway.
          Hide
          Martin Dougiamas added a comment -

          Regarding http://tracker.moodle.org/browse/MDL-11313?focusedCommentId=64300&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_64300 I think that makes sense, as long as it's optional. ie an admin would need to flick a CFG switch to get rid of that check. It could have appropriate warnings.

          I don't think the security consequences are too dire .... if you can switch roles then you can generally edit the course ... and all this is only at the course level anyway - even an admin at course level is not dangerous, really.

          My votes:

          In 1.9, a switch called "Allow users to switch to roles that they can't assign" (default off) that will disables those first three lines on get_assignable_roles_for_switchrole. It should have clear warnings.

          In 2.0, a new matrix for role_switch

          Show
          Martin Dougiamas added a comment - Regarding http://tracker.moodle.org/browse/MDL-11313?focusedCommentId=64300&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_64300 I think that makes sense, as long as it's optional. ie an admin would need to flick a CFG switch to get rid of that check. It could have appropriate warnings. I don't think the security consequences are too dire .... if you can switch roles then you can generally edit the course ... and all this is only at the course level anyway - even an admin at course level is not dangerous, really. My votes: In 1.9, a switch called "Allow users to switch to roles that they can't assign" (default off) that will disables those first three lines on get_assignable_roles_for_switchrole. It should have clear warnings. In 2.0, a new matrix for role_switch
          Hide
          Petr Škoda added a comment -

          yep, I like this proposal
          +1

          Show
          Petr Škoda added a comment - yep, I like this proposal +1
          Hide
          James Williamson added a comment -

          +1
          Cheers

          Show
          James Williamson added a comment - +1 Cheers
          Hide
          Jérôme Mouneyrac added a comment -

          I attached a patch, I changed as well course/view.php file (otherwise the switchrole has no effect).
          I added a new setting to "User Policies" page.

          Show
          Jérôme Mouneyrac added a comment - I attached a patch, I changed as well course/view.php file (otherwise the switchrole has no effect). I added a new setting to "User Policies" page.
          Hide
          Jérôme Mouneyrac added a comment -

          Except quick review today, as it's a sensitive patch, I'll commit this fix this Wednesday afternoon (after Moodle build).

          Show
          Jérôme Mouneyrac added a comment - Except quick review today, as it's a sensitive patch, I'll commit this fix this Wednesday afternoon (after Moodle build).
          Hide
          Jérôme Mouneyrac added a comment -

          committed in 1.9

          Show
          Jérôme Mouneyrac added a comment - committed in 1.9
          Hide
          Tim Hunt added a comment -

          Reviewed the code. The fix looks good. And tested with the new setting both on and off and it work. Good to see this finally fixed.

          Show
          Tim Hunt added a comment - Reviewed the code. The fix looks good. And tested with the new setting both on and off and it work. Good to see this finally fixed.
          Hide
          Petr Škoda added a comment -

          reopening - the string is incorrect - fixing...

          Show
          Petr Škoda added a comment - reopening - the string is incorrect - fixing...
          Hide
          Petr Škoda added a comment -

          also we need to merge the strings into HEAD

          Show
          Petr Škoda added a comment - also we need to merge the strings into HEAD
          Hide
          Petr Škoda added a comment -

          I have committed imporved strings proposed by Helen, feel free to further improve it

          Show
          Petr Škoda added a comment - I have committed imporved strings proposed by Helen, feel free to further improve it
          Hide
          Helen Foster added a comment -

          Eric, thanks for your report and thanks for everyone's comments.

          Documentation added: http://docs.moodle.org/en/User_policies

          Show
          Helen Foster added a comment - Eric, thanks for your report and thanks for everyone's comments. Documentation added: http://docs.moodle.org/en/User_policies

            People

            • Votes:
              12 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: