Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adds role mapping from Idp #532

Open
wants to merge 1 commit into
base: MOODLE_39_STABLE
Choose a base branch
from
Open

Adds role mapping from Idp #532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ Features
* Automatic certificate creation
* Optionally auto create users
* Support for multiple identity providers
* Role mapping for admin, manager and course_creator system roles
* Idp initiated flow / IdP first flow / IdP unsolicited logins, eg:

http://idp.local/simplesaml/saml2/idp/SSOService.php?spentityid=http://moodle.local/auth/saml2/sp/metadata.php&RelayState=http://moodle.local/course/view.php?id=2
Expand All @@ -67,7 +68,6 @@ http://idp.local/simplesaml/saml2/idp/SSOService.php?spentityid=http://moodle.lo
Features not yet implemented:

* Enrolment - this should be an enrol plugin and not in an auth plugin
* Role mapping - not yet implemented

Branches
--------
Expand Down
3 changes: 3 additions & 0 deletions classes/auth.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

@kabalin kabalin Jun 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Make sure all user data is fetched.
$user = get_complete_user_data('username', $user->username);

Expand Down
10 changes: 10 additions & 0 deletions lang/en/auth_saml2.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,3 +218,13 @@
$string['regeneratepath'] = 'Certificate path path: {$a}';
$string['regenerateheader'] = 'Regenerate Private Key and Certificate';
$string['regeneratesuccess'] = 'Private Key and Certificate successfully regenerated';

/*
* Role mapping
*/
$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.";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@jvinolas jvinolas Jun 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$string['saml_role_siteadmin_map'] = "Site administrators";
$string['saml_role_manager_map'] = "Manager";
$string['saml_role_coursecreator_map'] = "Course creator";
29 changes: 29 additions & 0 deletions locallib.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,3 +543,32 @@ function auth_saml2_admin_nav($title, $url) {
$PAGE->set_heading(get_string('pluginname', 'auth_saml2') . ': ' . $title);
$PAGE->set_title(get_string('pluginname', 'auth_saml2') . ': ' . $title);
}

/**
* Map user roles from Roles array
*
*/
function sync_roles($user,$attributes,$config) {
global $CFG, $DB;

// Process siteadmin (special, they are stored at mdl_config)
if(in_array($config->saml_role_siteadmin_map,$attributes['Role'])){
$siteadmins = explode(',', $CFG->siteadmins);
if (!in_array($user->id, $siteadmins)) {
$siteadmins[] = $user->id;
$newAdmins = implode(',', $siteadmins);
set_config('siteadmins', $newAdmins);
}
}

// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like for the roles to not be hard coded here, this should be configurable

role_assign($creatorrole->id, $user->id, $syscontext);
}
if (in_array($config->saml_role_manager_map, $attributes['Role'])) {
$managerrole = $DB->get_record('role', array('shortname'=>'manager'), '*', MUST_EXIST);
role_assign($managerrole->id, $user->id, $syscontext);
}
}
36 changes: 36 additions & 0 deletions settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,42 @@
$authplugin->get_ssp_version()
));

// Role mapping
$name = 'auth_saml2/field_map_role';
$title = get_string('saml_role_map', 'auth_saml2');
$description = '';
$default = '';
$setting = new admin_setting_configtext($name, $title, $description, $default, PARAM_ALPHANUMEXT);
$settings->add($setting);

$settings->add(
new admin_setting_heading(
'auth_saml2/saml_rolemapping',
new lang_string('saml_rolemapping', 'auth_saml2'),
new lang_string('saml_rolemapping_head', 'auth_saml2')
)
);

$name = 'auth_saml2/saml_role_siteadmin_map';
$title = get_string('saml_role_siteadmin_map', 'auth_saml2');
$description = '';
$default = '';
$setting = new admin_setting_configtext($name, $title, $description, $default, PARAM_ALPHANUMEXT);
$settings->add($setting);

$name = 'auth_saml2/saml_role_manager_map';
$title = get_string('saml_role_manager_map', 'auth_saml2');
$description = '';
$default = '';
$setting = new admin_setting_configtext($name, $title, $description, $default, PARAM_ALPHANUMEXT);
$settings->add($setting);

$name = 'auth_saml2/saml_role_coursecreator_map';
$title = get_string('saml_role_coursecreator_map', 'auth_saml2');
$description = '';
$default = '';
$setting = new admin_setting_configtext($name, $title, $description, $default, PARAM_ALPHANUMEXT);
$settings->add($setting);

// Display locking / mapping of profile fields.
$help = get_string('auth_updatelocal_expl', 'auth');
Expand Down