credentials->getValidTokensForAppAndUser() returns all tokens when nobody is logged in

[bencresty]

When nobody is currently logged in on Craft (for example when only working frontend) getValidTokensForAppAndUser() always returns ALL tokens. This is not what I would expect as this obviously is prone to security issues. I would expect NONE of the tokens to be returned, so an empty list.

When looking in the code of the plugin in Credentials.php the default value for the user parameter of the method is set to null. Inside the method it does this:

$userId = null;

if ($user instanceof User) {
    $userId = $user->getId();
} elseif (is_int($user)) {
    $userId = $user;
}

$query = $app->getTokenRecordQuery();

if ($userId !== null) {
    $query->andWhere(['userId' => $userId]);
}

So if the user parameter of the getValidTokensForAppAndUser() method isn't used it will default to null for user/userID and the query will not be filtered and so returns ALL tokens. Which should be fine.

But if we DO enter a userID when there's no user logged in on Craft, so Craft set the userID to null, we suddenly get ALL tokens from all users because the query isn't filtered by ANY user/userID! Like this:

$currentUser = Craft::$app->getUser();
$oAuthAppHandle = 'google';
$tokens = $oAuthClientPlugin->credentials->getValidTokensForAppAndUser(
                $oAuthAppHandle, 
                $currentUser->id // == null when nobody is logged in
); 

// $tokens now is set to ALL tokens if no user is logged in

This is a security issue if we want to provide data from an external OAuth API by using a route via our own module backend (for better security as than we have the back channel via Craft). As we don't want our own API to become a public gateway to the external Resource server, but limit it to only users of Craft.

Of course we can make sure it never reaches the getValidTokensForAppAndUser() method when nobody is logged in.
For instance like so

$tokens = $oAuthClientPlugin->credentials->getValidTokensForAppAndUser(
                $oAuthAppHandle, $currentUser->id !== null ? $currentUser->id : -1);

But that's easy to forget and when new to the module may not even be known. I would expect this to also be checked in the getValidTokensForAppAndUser() as then we only need to check if there are tokens for this user and don't need to include an extra step just for this that is easy to forget (making the API vulnarable without us realising).

Please, if nobody is logged in so a userID is set to null, don't return all tokens, but return no tokens at all. Especially because the name implies that it's always only returning tokens from a single user. This is easy to change in the method inside Credentials.php code by using a different user/userID parameter default (like -999 or whatever that's not a legal userID nor user object)