Passport: Client Grant requires users table

[sasa-b]

Currently, the __Client Grant__ implementation requires the presence of the users table, otherwise if the table is not present and we try to authorize we get the following exception:

{ 
    "message": "SQLSTATE[42S02]: Base table or view not found: 1146 Table 'my_db.users' doesn't exist (SQL: select * from `users` where `id` =  limit 1)",
    "exception": "Illuminate\\Database\\QueryException",
    ...
}

Since the Client Grant should be used for machine to machine communication the users table should not be mandatory.

The issue happens in the Laravel\Passport\Guards\TokenGuard:

$user = $this->provider->retrieveById(
      $psr->getAttribute('oauth_user_id')
 );

if (! $user) {
     return;
}

The provider tries to fetch the user and everything blows up. We could easily circumvent this with an additional if statement, namely, if a user_id/oauth_user_id is null in the oauth_tokens table, don't try to fetch the user. Something like this:

$userId = $psr->getAttribute('oauth_user_id');

if ($userId) {
     $user = $this->provider->retrieveById($userId);

      if (! $user) {
          return;
      }
}

I'd be happy to make a pull request if that's OK with the maintainers?

[driesvints]

I'm not really sure how this can happen. A client credentials grant shouldn't attempt to retrieve a user in the first place I believe? Or am I missing something?

[sasa-b]

@driesvints Well it shouldn't, but here is the catch. Laravel requires the auth.php config driver and provider to be set. If you use passport as a driver or even any other, when you use the \Laravel\Passport\Http\Middleware\CheckClientCredentials::class for the Client Access Token the Laravel\Passport\Guards\TokenGuard is used and then happens what I described previously.

I'll make a PR, hope that is OK with you?

[driesvints]

I'm a bit concerned with edge cases and what this will break in people's setups. I think we need to investigate this more first.

[sasa-b]

I don't think it will break anything, because when we use the Client Grant there is no user_id associated with the token, and guard provider should not try to fetch the user.

In Client Grant scenario we are only concerned if the token is valid, e.g. if it has not expired or if it has not been revoked etc.

When we use other scenarios, the user_id is set on the tokens and the provider should and will try to fetch the user.

Then again, I might have jumped the gun, I was just eager to make my first open source contribution :D

I hope I am right and it gets merged :)

[driesvints]

You gotta realize that this method is used for all grant types not just client credentials.

[sasa-b]

Yes, I am aware of that, but in all other cases the user_id is not null and when we "encode" the token the user_id becomes part of the token, the sub (subject) claim of the JWT.

In the case of __Client Grant__ user_id is null so the sub will be null and everytime we "decode" the JWT we will get null, so there is no need to try and fetch the user. I will try to go through the flow with you.

This method returns an instance of the AccessTokenEntity.

AccessTokenRepository::getNewToken(ClientEntityInterface $clientEntity, array $scopes, $userIdentifier = null)

As you can see the $userIdentifier is nullable.

AccessTokenEntity uses the AccessTokenEntityTrait which has the method that turns the token in its string representation:

trait AccessTokenTrait
{
    /**
     * Generate a JWT from the access token
     *
     * @param CryptKey $privateKey
     *
     * @return Token
     */
    public function convertToJWT(CryptKey $privateKey)
    {
        return (new Builder())
            ->setAudience($this->getClient()->getIdentifier())
            ->setId($this->getIdentifier(), true)
            ->setIssuedAt(time())
            ->setNotBefore(time())
            ->setExpiration($this->getExpiryDateTime()->getTimestamp())
            ->setSubject($this->getUserIdentifier())
            ->set('scopes', $this->getScopes())
            ->sign(new Sha256(), new Key($privateKey->getKeyPath(), $privateKey->getPassPhrase()))
            ->getToken();
    }
}

That's the flow in a nutshell when we create tokens.

When we are authenticating the token in the TokenGuard::authenticateViaBearerToken($request) we call the getPsrRequestViaBearerToken() method and a few others down the stack and finally the BearerTokenValidator::validateAuthorization(ServerRequestInterface $request) which validates the token and returns a request object up the stack:

// Return the request with additional attributes
            return $request
                ->withAttribute('oauth_access_token_id', $token->getClaim('jti'))
                ->withAttribute('oauth_client_id', $token->getClaim('aud'))
                ->withAttribute('oauth_user_id', $token->getClaim('sub'))
                ->withAttribute('oauth_scopes', $token->getClaim('scopes'));

In __Client Grant__ the sub will be null, in all other grants it won't. So here is where my changes come to play. I've changed the TokenGuard from this:

$user = $this->provider->retrieveById(
      $psr->getAttribute('oauth_user_id')
 );

if (! $user) {
     return;
}

To this:

 $userId = $psr->getAttribute('oauth_user_id') ?: null;

 if ($userId) {
     $user = $this->provider->retrieveById($userId);

      if (! $user) {
           return;
      }
 }

In all other grant types we will always get the user id since it is the sub claim of JWT so there is no need to worry.

I hope I made it more clear. :)

[driesvints]

I still think this won't work. This way people can send tokens without the sub claim and pass authentication which could lead to security issues.

Btw there's a call to the $user variable a bit further down which also needs to be addressed.

@sephster do you have any insights here?

[sasa-b]

I don't think so because that would result in different hashes and this method would fail

 public function verify(Signer $signer, $key)
    {
        if ($this->signature === null) {
            throw new BadMethodCallException('This token is not signed');
        }

        if ($this->headers['alg'] !== $signer->getAlgorithmId()) {
            return false;
        }

        return $this->signature->verify($signer, $this->getPayload(), $key);
    }

[sasa-b]

@driesvints Totally missed the $user at the end of the method. I've investigated the code a bit more and came to the conclusion that this method returns a token authenticated user so we should actually change it to this:

if (! $userId = $psr->getAttribute('oauth_user_id')) {
    return;
}

$user = $this->provider->retrieveById($userId);

if (! $user) {
    return;
}

This way the __Client Grant__ works without the users table and your concerns are mitigated. Would this be acceptable for a PR?

[driesvints]

I still don't know if this is the best approach. Don't you need to provide full compatibility with OAuth2 which means providing support for all OAuth2 grants? You still need a users table for the other grants. If you leave it away the other grant types won't work anymore?

[Sephster]

Hi @driesvints - apologies for the delay in responding. As you have intimated, the sub must be set to be compliant with OAuth specs.

For the majority of grants, it is usually set to the resource owner which would be the user ID in most cases. However, the client credentials grant is the only exception to this rule. As there is no resource owner, the sub should be set to the client ID instead. It should never be set to null.

@sasa-b is right that we shouldn't need to check for a user ID but the assertion that the sub will be null isn't correct.

I hope this helps. If I get a chance over the weekend I will see if I can find a quick win solution to this if nothing has been resolved. Happy to help however I can. Cheers

[driesvints]

@Sephster thanks for replying! It seems to me that we first need to retrieve the client, check if it's a client credentials grant and if so just skip the user check? In other cases we'll do need to check the user.

[michael-sbs]

Hi @driesvints,

I am wondering if the issue was introduced as part of this commit: https://github.com/laravel/passport/pull/854/commits/404b345775c91cd04443e77e8d6164b9727a3448

I have not had time to test it yet, but I have been able to use the client credentials grant in the past without requiring a user table, but after starting a new project I have now discovered the issue.

I can run a few more tests tomorrow in the hope of identifying the issue.

[michael-sbs]

After a few tests, I don't believe it is due to the commit mentioned above.

I will continue digging into it and hopefully find why the issue has suddenly started happening.

[driesvints]

@Sephster Are you sure the sub can't be empty? This is the part of the BearerTokenValidator that sets the oauth attributes on the request:

            return $request
                ->withAttribute('oauth_access_token_id', $token->getClaim('jti'))
                ->withAttribute('oauth_client_id', $token->getClaim('aud'))
                ->withAttribute('oauth_user_id', $token->getClaim('sub'))
                ->withAttribute('oauth_scopes', $token->getClaim('scopes'));

There's already an oauth_client_id attribute which conforms to the aud claim so it would seem weird to me that the aud and the sub claim contained the same value?

[driesvints]

@sasa-b @michael-sbs I've tried digging into this issue but I've come to the conclusion that you shouldn't be using the TokenGuard class at all when using the client credentials grant. Can you please post some more code like routes etc. Where exactly is this problem happening? Can you create a repo which recreates this behavior?

[Sephster]

Hi @driesvints, sorry, in work just now so posting this quickly but I think this is correct. There has been dome debate over our usage of the aud claim. It is most likely incorrectly setting it to the client ID when it should instead be setting it to the resource server that would be consuming the JWT. There is a discussion open for it here https://github.com/thephpleague/oauth2-server/issues/857

[driesvints]

@Sephster I see. Thanks and keep us posted :)

[driesvints]

@sasa-b @michael-sbs going to close this off then. Feel free to respond if you ever find time.