Adds role mapping from Idp #532
Conversation
| // Process coursecreator and manager | ||
| $syscontext = context_system::instance(); | ||
| if(in_array($config->saml_role_coursecreator_map,$attributes['Role'])){ | ||
| $creatorrole = $DB->get_record('role', array('shortname'=>'coursecreator'), '*', MUST_EXIST); |
There was a problem hiding this comment.
I'd like for the roles to not be hard coded here, this should be configurable
| */ | ||
| $string['saml_role_map'] = "Role"; | ||
| $string['saml_rolemapping'] = "Role Mapping"; | ||
| $string['saml_rolemapping_head'] = "The IdP can use it's own roles. Set in this section the mapping between IdP and Moodle roles. Accepts multiple valued comma separated. Example: admin,owner,superuser."; |
There was a problem hiding this comment.
I think I'd like to see a more clear mapping and example here, I think something like a set of key value pairs one per line, like
keycloakadmin,admin
keycloakmanager,coursemanager
keycloakstudent,learner
This would make it much more powerful and also easier to see what is actually happening. It should also validate against the configured moodle roles.
There is also the important question of negative constraints, ie if you had the admin role in keycloak and then it was removed, then we should be able to declare that this role is fully managed by saml and it should be removed if needed and not just added.
I'm not sure how to best express this in config but a simple idea is an optional 3rd column which says if this role should be removed if not present:
keycloakadmin,admin,remove
keycloakmanager,coursemanager
keycloakstudent,learner
One last thing, the key name of the 'Role' saml attribute should also configurable too., and it can default to 'Role'
There was a problem hiding this comment.
I see your point using key/value pairs. I haven't seen this feature in the enrollment plugins I've used but will allow for wider Idp configuration (also if the 'Role' field name could be set up).
As for the user roles being removed at Idp side, how do you think it should be done? As per user login in again I assume, as we could add a cron to retrieve users info from Idp but then it will be tied to a single Idp (keycloak in our scenario).
Also, I'm thinking now if the plugin behavior will update user roles modified at Idp side when he logs in again?
There was a problem hiding this comment.
I can't see how this could work via cron, it can only be based on the latest attributes it gets during a particular login. So the idp says these are the roles you have right now, Moodle then takes those and if you don't have any attributes which map to a role which you do have then it removes it on the spot.
Also note we've now branches into MOODLE_39_STABLE can you please point this pr against it?
There was a problem hiding this comment.
We are using this PR in production and we use keycloak as the only user source so our desired behaviour is like you say: it updates de role when the users logs in again if it was modified in keycloak.
Maybe there should be a checkbox to set or unset role updating at each user login.
We will try to point it to your latest branch and update the README next week.
There was a problem hiding this comment.
It's updated to MOODLE_39_STABLE and updated README.
I'll let the improvements you said for next developments but, as we are using it in production, this will provide an starting point that will provide at least the basic role mapping feature.
|
hi @jvinolas this looks really great and this feature is well overdue. A few comments in the pr, and can you please also touch the README.md where it mentions roles not being managed? |
|
Hi @jvinolas, just edit PR (click edit button above near the title) and make it target MOODLE_39_STABLE rather than master. Thanks! |
|
thanks @kabalin - I've just adjusted that - ideally I'd prefer @brendanheywood to come back and finish off the PR, but if not @nhoobin do you want to look at this one? |
|
hmm - I'd add a -1 to merging this with hard-coded role shortnames (coursecreator/manager) - I think we'd need to tidy up the way roles are mapped before merging this. |
| @@ -687,6 +687,9 @@ public function saml_login_complete($attributes) { | |||
| set_config('siteadmins', implode(',', $admins)); | |||
| } | |||
|
|
|||
| // Synchronize IdP roles to moodle | |||
| sync_roles($user, $attributes, $this->config); | |||
There was a problem hiding this comment.
If $attributes['Role'] is not defined, this function should not be called at all. Also, it worth moving it into auth class rather than keeping it in locallib.php, it is more logical given this function operates in context of authentication process and also you would not need to pass $this->config to it.
There was a problem hiding this comment.
With regard to role assigning, I would suggest using approach similar to one in
auth_ldap: https://github.com/moodle/moodle/blob/master/auth/ldap/auth.php#L1838-L1859, so that the actual component that does role allocation is recorded and those roles do not interfere with other roles user may have. This will simplify role unassigning as well (those roles not listed in attribute will be removed as long as they were allocated through this plugin) and you would not need to hardcode roles (any assignable role will work). IMO site admin allocation though this is not a good idea, I would suggest removing this option.
We integrate moodle, nextcloud and wordpress with SAML and we use the admin role to just allow also admin to go from one app to the other and configure it. Is it a problem to just let admin role mapping there as an option if one wants to use it in his deployment? If not mapped it won't be used at all.
There was a problem hiding this comment.
If you want to keep working on this and get it into a state that we could accept it might be cleaner to remove the admin role sync for now, tidy up the role mapping to remove the hard-coded shortnames and then once that is good enough to land you could look at a separate PR to add site-level admin mapping support.
|
With regard to role assigning, I would suggest using approach similar to one in |
We've implemented the admin, course creator and manager role mapping. Tested working against keycloak role mapping.
#405