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.

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

Added Multi-Role implementation #161

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nick-perry14
Copy link

@nick-perry14 nick-perry14 commented Aug 26, 2024

Added backward compatible multi-role implementation. An example of a modified snippet which would take advantage of this is:

function update_authorizer_custom_role( $default_role, $user_data ) {
	// Don't change role for administrators.
	if ( 'administrator' === $default_role ) {
		return $default_role;
	}
	$add_role = [];
	$remove_role = [];
	// Assign custom roles to users.
	if ( isset( $user_data['oauth2_attributes']['groups'] ) ) {
		// Mapping for OAuth groups to WordPress roles. If a user has
		// multiple groups, the first one in the array below takes primary role and all others are assigned as secondary roles.
		$group_to_role_mapping = array(
			'Lead Developer' => 'lead-developer',
			'Developer' => 'developer',
			// Add more mappings here, if needed, in this format:
			// 'uh-grouping' => 'wordpress_role',
		);
		$changedRole = False;
		foreach ( $group_to_role_mapping as $group => $role ) {
			if (
				$group === $user_data['oauth2_attributes']['groups'] ||
				(
					is_array( $user_data['oauth2_attributes']['groups'] ) &&
					array_search( $group, $user_data['oauth2_attributes']['groups'] ) !== false
				)
			) {
				if(!$changedRole) {
					$default_role = $role;
					$changedRole = True;
				}
				else
				array_push($add_role, $role);
			}
			else{
				array_push($remove_role, $role);
			}
		}
	}

	return [$default_role, $add_role, $remove_role];
}
add_filter( 'authorizer_custom_role', 'update_authorizer_custom_role', 10, 2 );

Also want to commend you guys for creating this. I didn't want to get nickled and dimed by miniorange and this works perfectly. Thank you to both the university and maintainers for creating something awesome!

@figureone
Copy link
Member

Thanks! We'll review this with the goal of including it in the next version. I think it might need to be more comprehensive, since there are some other functions in the codebase that handle syncing roles between Authorizer and wp_usermeta, such as if the role is edited in the WordPress User screen instead of in Authorizer Settings. Also an extra layer for multisite support.

Can you let us know if you use another 3rd party plugin for multi-role support? We can review that as well to make sure this method uses the proper hooks.

We'll also let you know if there are any changes that will affect your example hook. For example I think it's likely that we'll use named parameters on the return value, something like:

	return array(
		'role' => $default_role,
		'roles_to_add' => $add_role,
		'roles_to_remove' => $remove_role,
	);

For context, there are two reasons we haven't implemented this yet:

  1. Core WP_User::set_role() still doesn't support multiple roles: https://developer.wordpress.org/reference/classes/wp_user/set_role/
  2. There are potential security issues with neglecting to remove roles from a user being demoted. For example, in your example hook above, if a privileged role like administrator isn't included in your $group_to_role_mapping, it won't be removed from the WordPress user logging in, even though on the surface you would expect someone who only has Developer group to be demoted to the developer role. Do you think we should remove any WordPress roles the user has that aren't explicitly in the add_roles parameter, even if they aren't listed in remove_roles?

@nick-perry14
Copy link
Author

Hi there.

  1. From my understanding, multi-role is not necessarily a plugin. I use the standard UREPro to manage roles, but I believe WP has functionality for multi roles built in via the add_role and remove_role functions.
  2. Feel free to make all the edits you want. I work mostly in python so PHP has been a while so I'm sure I messed up somewhere :)
  3. That's a good nuance, though I'm not sure what the correct answer is. In your example, let's say you want to assign someone a role that's not in the LDAP, ouath, etc. (i.e. TA, or give a student with disability extra access to materials, or materials early). That role assignment would be more manual and therefore would not want to be affected by logging back in. Those kinds of cases (I'm using this plugin in a non-academic setting) drove me to only 'Manage roles' that were explicitly defined in the hook.

@figureone
Copy link
Member

Thanks! For reference in (1), the issue with WordPress core is that if you edit a user's role in the admin dashboard (either editing their profile, or using the bulk action), the single role you choose there will overwrite any multi-roles.
Screenshot 2024-09-24 at 3 16 46 PM
Screenshot 2024-09-24 at 3 16 30 PM

I believe 3rd party plugins (like URE Pro) modify this behavior. The issue for us is that we try to sync Authorizer roles with any changes made via those core interfaces, and we use the set_user_role hook that fires, and it currently only supports a single role (confusingly, though, it has a parameter that supports multiple $old_roles).
https://developer.wordpress.org/reference/hooks/set_user_role/

Regardless, we'll try to find some time to inspect the free version of User Role Editor and see if we can find some common ground to support your use case.
https://www.role-editor.com/download-plugin/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants