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

User role in auth_settings_access_users_approved not updated #149

Closed
mdebski opened this issue Mar 27, 2024 · 10 comments
Closed

User role in auth_settings_access_users_approved not updated #149

mdebski opened this issue Mar 27, 2024 · 10 comments

Comments

@mdebski
Copy link

mdebski commented Mar 27, 2024

I'm using authorizer with generic oauth & using custom authorizer_custom_role filter (basically mapping role from some oauth attribute). When I change the roles in the oauth server, this does not fully get reflected in wordpress, resulting in a somewhat funny behaviour when the user gets either old role or new role every second login.

I believe the core issue is that auth_settings_access_users_approved is only saved when the user is initially approved, so when the role changes, it does not get updated.

Or maybe something is wrong here - I observe that $default_role and $approved_role are both the new role, but $user_info['role'] is the old role (from auth_settings_access_users_approved, from when the user was created), and that's the one that ends up being used.

@figureone
Copy link
Member

Thanks for digging into that.

Another problem might be that we are too aggressively syncing Authorizer user roles with the current role of their WordPress account. So perhaps you'll update the role (in your custom filter) to match the role on your oauth server, but then Authorizer will detect it's different than the WordPress user role, and switch it back.

We'll work on tracing the specific code path, but for now, can you try manually updating the WordPress user's role in the authorizer_custom_role filter? Something like:

$wp_user = get_user_by( 'email', $user_data['email'] );
if ( ! empty( $wp_user ) ) {
	$wp_user->set_role( 'your-custom-role' );
}

@mdebski
Copy link
Author

mdebski commented May 3, 2024

That didn't work unfortunately.
I used the following snippet in the filter (note the extra array subscript on emails):

  $wp_user = get_user_by('email', $user_data['email'][0]);
  if (!empty($wp_user)) {
    error_log("Setting role: " . $role);
    $wp_user->set_role($role);
  }

And every time I see:

[03-May-2024 12:06:25 UTC] Setting role: editor

yet the user is still logged in as the default "subscriber" role every second time.

@mdebski
Copy link
Author

mdebski commented May 3, 2024

BTW the quick fix I used was:

--- wp-content/plugins/authorizer/src/authorizer/class-authorization.php	2024-04-29 21:14:13.701436439 +0000
+++ wp-content/plugins/authorizer/src/authorizer/class-authorization.php	2024-05-03 11:00:48.518966738 +0000
@@ -225,7 +225,7 @@
 				// If this user's role was modified above (in the
 				// authorizer_custom_role filter), use that value instead of
 				// whatever is specified in the approved list.
-				if ( $user_info['role'] !== $approved_role ) {
+				if ( $default_role !== $approved_role ) {
 					$user_info['role'] = $approved_role;
 				}

It worked for me, but of course I'm not sure if it's correct in all use cases.

@figureone
Copy link
Member

Thanks for all the details here. Definitely an issue since in this specific case we have the role in 3 different places:

  1. The actual WordPress user ($default_role)
  2. The value coming from the oauth backend via authorizer_custom_role filter ($approved_role)
  3. The value stored in the Authorizer approved list ($user_info['role'])

I expect to nail this down next week, just need to make sure we don't inadvertently affect other flows. In the meantime, can you let us know if you're running in multisite? There are extra edge cases that are multisite-specific so I want to know if we need to dig into those as well.

@figureone
Copy link
Member

And just one more comment, I believe this is the place where we are reverting back to the role stored in the approved list, instead of respecting the one coming in from authorizer_custom_role:
https://github.com/uhm-coe/authorizer/blob/master/src/authorizer/class-authorization.php#L391-L394

@mdebski
Copy link
Author

mdebski commented May 6, 2024

I'm not using multisite. Thanks!

@figureone
Copy link
Member

Thanks again for your help fixing this longstanding bug! We have a fix that will land in the next released version:
8be7cbc

It's a little different than your one-line fix above: we instead sync a new role coming in from the authorizer_custom_role filter to that user's entry in the Authorizer approved list. (This addresses a discrepancy where the database would always store the old/original role, but it would never show in the approved list UI because that would always show the WordPress user role instead.)

That said, you can leave your one-line hotfix in place until the next version is released; it will work fine in the interim!

@mdebski
Copy link
Author

mdebski commented May 8, 2024

Thanks! I verified the fix, works perfectly!

@figureone
Copy link
Member

Aloha, heads up, version 3.8.2 (just released) has a tweak to the fix for this issue, since it was inadvertently catching new user logins and assigning them the default role instead of the role defined in the approved list.

Please test and make sure the tweak didn't revert the behavior for your use case. Thanks!
940bbf1

@mdebski
Copy link
Author

mdebski commented May 23, 2024

Still works, thanks!

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

No branches or pull requests

2 participants