Details
-
Type:
Improvement
-
Status:
Closed
-
Priority:
Minor
-
Resolution: Fixed
-
Affects Version/s: 1.9
-
Fix Version/s: 1.9.5
-
Component/s: Course, Libraries, Roles / Access
-
Labels:None
-
Affected Branches:MOODLE_19_STABLE
-
Fixed Branches: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
| This issue is a clone of: | ||||
| MDL-11943 | CLONE -Removing [moodle/role:assign] as Requisite for Switch Roles |
|
|
|
| This issue will help resolve: | ||||
| MDL-10216 | [moodle/role:switchroles] permission needs/forces [moodle/role:assign] to be granted |
|
|
|
| This issue has a non-specific relationship to: | ||||
| MDL-18132 | New switchrole matrix and remove the role assign requirement only in HEAD |
|
|
|
Activity
- All
- Comments
- History
- Activity
- Source
- Test Sessions
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.
) 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. 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.
.
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. I have committed imporved strings proposed by Helen, feel free to further improve it
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!