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

Make sure we don't return the token of the wrong user #95

Merged
merged 11 commits into from
Sep 14, 2023
43 changes: 40 additions & 3 deletions src/Storage/AuthorizationCodeStorage.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,16 @@ private function getUserIdByCode( $code ) {
return null;
}

$key = self::META_KEY_PREFIX . '_client_id_' . $code;
ashfame marked this conversation as resolved.
Show resolved Hide resolved

$users = get_users(
array(
'number' => 1,
// Specifying blog_id does nothing for non-MultiSite installs. But for MultiSite installs, it allows you
// to customize users of which site is supposed to be available for whatever sites
// this plugin is meant to be activated on.
'blog_id' => apply_filters( 'oidc_auth_code_storage_blog_id', get_current_blog_id() ),
// phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_key
'meta_key' => self::META_KEY_PREFIX . '_client_id_' . $code,
'meta_key' => $key,
// Using a meta_key EXISTS query is not slow, see https://github.com/WordPress/WordPress-Coding-Standards/issues/1871.
'meta_compare' => 'EXISTS',
)
Expand All @@ -42,7 +43,43 @@ private function getUserIdByCode( $code ) {
return null;
}

return absint( $users[0]->ID );
if ( count( $users ) > 1 ) {
// This should never happen.
// If it does, something is wrong, so it's best to not return any user.
ashfame marked this conversation as resolved.
Show resolved Hide resolved
$debug_log = "[CONCURRENTLOGINS] more than 1 user found for code: $code.";
$found = 0;
$found_user_id = 0;
psrpinto marked this conversation as resolved.
Show resolved Hide resolved
foreach ( $users as $index => $user ) {
ashfame marked this conversation as resolved.
Show resolved Hide resolved
if ( '' === get_user_meta( $user->ID, $key, true ) ) {
$debug_log .= " ($user->ID empty meta) ";
} else {
++$found;
$found_user_id = $user->ID; // only used when $found is 1, so overwrite is fine.
$debug_log .= " ($user->ID meta exists) ";
}
}

if ( 1 === $found ) {
$debug_log .= ' RECOVERED ';
// phpcs:ignore WordPress.PHP.DevelopmentFunctions
error_log( $debug_log . print_r( $users, true ) );
return $found_user_id;
}

$debug_log .= " FAILED (found:$found)";
// phpcs:ignore WordPress.PHP.DevelopmentFunctions
error_log( $debug_log . print_r( $users, true ) );
return null;
}

$user = $users[0];
ashfame marked this conversation as resolved.
Show resolved Hide resolved

// Double-check that the user actually has the meta key.
if ( '' === get_user_meta( $user, $key, true ) ) {
return null;
}

return absint( $user->ID );
}

public function getAuthorizationCode( $code ) {
Expand Down
Loading