Add multirole support #99
Conversation
Existing authLdap code only supported users having a single role and would cause problems with other plugins (e.g. Members) that exploit WordPress's native support for multiple roles. This update should allow a user to belong to multiple roles and gracefully add roles based on LDAP group, if configured to do so. Simple tests using my specific use case have validated this fix.
Add multirole support
|
Thank you for this PR! From a first glance it looks like a great addition! But I'll have to dig through it and I'm sure I'll have the one or other question. |
authLdap.php
Outdated
| if (!$authLDAPGroupEnable || !$authLDAPGroupOverUser) { | ||
| $role = authLdap_user_role($uid); | ||
| } | ||
| $roles = authLdap_user_roles($uid); |
There was a problem hiding this comment.
We definitely need this if-condition. Otherwise the LDAP-roles will ALWAYS overwrite the Wordpress-roles even though people might not want this plugin to handle the roles.
There was a problem hiding this comment.
All this does is get the current role(s). The point is to not clobber existing roles but to gracefully merge additional ones (unless configured to completely override). This line simply gets the roles as they exist and then adds them as roles at the end of the function. So if LDAP is not configured to override then these are simply added back in and no harm done. What am I missing?
|
Sorry, it became a bit lengthy.... But I'm through now 😉 |
|
Overall it looks like I was a bit hasty in my request. There are things that should clearly be changed about my initial set of updates. But it might be that we'll have to part ways simply based on our difference of opinion pointed out above relative to all WP or all LDAP but not both. Thank you for your time. |
Existing authLdap code only supported users having a single role and would cause problems with other plugins (e.g. Members) that exploit WordPress' native support for multiple roles. This update should allow a user to belong to multiple roles and gracefully add roles based on LDAP group, if configured to do so. Simple tests using my specific use case have validated this change.