search

ktbartholomew / saml-20-single-sign-on

Wordpress plugin that makes a Wordpress site act as a SAML service provider
GNU General Public License v2.0
37 stars 22 forks source link

Bug that was introduced in PR#4 #7

Closed phille97 closed 8 years ago

phille97 commented 8 years ago

Set the role to the first role it matches.

WP orders the roles from Admin -> Subscriber. So if a user belongs to two AD groups that's mapped to administrator and subscriber, then it's most likely that the user belongs to the administrator role.

Previously when it looped through the WP roles it set the role to the last one in the list, because there were no check if the role already were set.

ktbartholomew commented 8 years ago

@phille97 How do custom roles fit into that order? If custom roles are added to the end of the role list, then it's increasingly possible that those roles couldn't be assigned because another role had already been matched.

It would be nice to assign users to the role with the most expansive capabilities, although the decoupling of roles and capabilities makes that tricky to define. Honestly though, establish a group system that doesn't have conflicts like this should be an exercise for the AD admin, not something for this plugin to guess at.

I'm fine with using the first matched role, but we should also include a small note in the UI explaining the way this works.

phille97 commented 8 years ago

WordPress wp_roles()->roles Is fetched using get_option( $this->role_key ) as seen in the source of WP_Roles. This would mean that the roles will be in the order of the array when it's stored.

So in theory new roles will be appended to the end of that array, but this should maybe not be trusted as the array has a possibility to be sorted in a different way.

I agree that it would be favorable for admins to choose the order of which role should be mapped to a AD group. If someone implements that I also think it's also a good idea for admins to have the ability to map multiple AD groups to one WP role.

Source of add_role

But for now I think that this simpler approach works fine. I can add a note in the UI, and add it to this PR but that would be tomorrow.

ktbartholomew commented 8 years ago

I can add a note in the UI, and add it to this PR but that would be tomorrow.

👍

phille97 commented 8 years ago

I've added the notes and created an issue #8 so we don't forget this issue in the future.