Fix user role mapping #91
Conversation
* Ensures role mapping occurs on each login in case of updates * Fixes network role mapping where both sites and network level settings are supplied * Uses `add_user_to_blog()` so that the current blog is properly restored and other defaults and caches are created
|
@mattheu possibly breaking for some use cases or instances, however that's ok for minor releases below a 1.0.0 release. We're not promising 100% stability yet. I will bump to 0.6.0 with this though, so properly scoped composer dependencies should continue to use the appropriate version. |
| @@ -3,7 +3,7 @@ | |||
| Plugin Name: WP Simple SAML | |||
| Description: Integrate SAML 2.0 IDP without the hassle | |||
| Author: Shady Sharaf, Human Made | |||
| Version: 0.1 | |||
| Version: 0.6.0 | |||
There was a problem hiding this comment.
Why the very large version update? The version in master is 0.4.1, why the jump?
There was a problem hiding this comment.
The latest release tag was 0.5.1, this introduces a new action so I opted for a minor release. Semantic versioning of a plugin below 0.x allows for this.
There was a problem hiding this comment.
This file now says 0.6.0 (which is a major release under 0.x), but the actual plugin version is in plugin.php not this file, which is 0.4.1:
Line 6 in 83961f3
Seems like we need to remove it from this file, fix the main one, and make sure we keep those in sync.
| * @param \WP_User $user User object | ||
| * @param array $attributes SAML Attributes passed from IdP | ||
| */ | ||
| do_action( 'wpsimplesaml_user_matched', $user, $attributes ); |
There was a problem hiding this comment.
What's the functional difference in this PR? It's not clear to me why the existing action is not good enough here.
There was a problem hiding this comment.
This one acts on each authentication, the other action only works when a user account is initially created.
Changes to settings in active directory once a user has logged in once will never be reflected in WordPress unless the roles mapping is checked again on each authentication.
There was a problem hiding this comment.
This seems like a fairly major change in behaviour that might have subtle effects; is there an issue where we can discuss that behaviour in more depth?
| } | ||
|
|
||
| return $network_roles ?? (array) $roles; | ||
| return $roles; |
There was a problem hiding this comment.
Why is this change here? There shouldn't be any functional difference?
There was a problem hiding this comment.
It was just weirdly written, $network_roles is only defined in the if block above this return statement, and $roles is already cast to an array.
Yeah I just commented didn't approve. |
|
Sorry I thought you'd approved it Matt. My fault. |
| foreach ( $roles['network'] as $role ) { | ||
| add_user_to_blog( (int) $site_id, $user->ID, $role ); |
There was a problem hiding this comment.
In terms of #90, the minimal change here I think would have been to restore_current_blog(), although add_user_to_blog() might make sense overall. Need to check the difference here.
Also, the previous code skips the first role in $roles['network'], why the difference here?
There was a problem hiding this comment.
OK, I think this breaks the current behaviour if you have multiple roles.
Internally, add_user_to_blog() does a few things this code did not do, including setting the user's primary site and clearing caches, which I think is good. However, internally it calls ->set_role() not ->add_role(), so using it in a loop like this will wipe previous roles.
However, I do think the current behaviour is probably also kind of broken in that it never removes roles they shouldn't have if the upstream changes. That's a fairly significant change though, so might warrant an option of some sort - as some users may want to allow adding roles purely in WP.
Let's open an issue to discuss.
There was a problem hiding this comment.
Created #94 for the follow up discussion.
add_user_to_blog()so that the current blog is properly restored and other defaults and caches are created