Add method to facilitate BearerTokenValidator override when you want to append data to the jwt token

[maximebeaudoin]

Currently, it's easy to implement the convertToJWT method in the AccessTokenEntityInterface implementation to add data to the JWT. However, i don't see any solution, except creating a new implementation of BearerTokenValidator, to add additional attributes from the token, to the returning ServerRequestInterface object.

I have take a look at #497 and i don't see the solution to resolve this.

I think this PR can provide a way to simply override the BearerTokenValidator:: appendAttributesFromToken() method and pass the custom BearerTokenValidator instance to the ResourceServer instance.

[simonhamp]

I like the concept. My concern with this implementation is that overriding could make for some unwanted code duplication in a hurry and some unexpected behaviour if you fail to instate the default attributes.

Perhaps appendAttributesFromToken() can sub out to another method that is used purely for custom attributes, e.g. appendCustomAttributes. The default implementation of which can just return the $request or even be empty.

That way you get the defaults pre-/post-appended with room for greater logic on how the custom ones get handled internally and your custom attributes method can be overridden with less danger of unintended side effects.

Thoughts?

[ragboyjr]

Why don't we just pass the entire token instance into the request? Then if you'd like to attach more request attributes, you can do it in a middleware down stream.

[maximebeaudoin]

Sorry for the delay guys, i'm little busy ! I'll take a look at this.

[maximebeaudoin]

@ragboyjr Not sure if it's a good practice to return the access token as a attribute in the request. However, i think you are right, not having access to the token object after the authorization validation look like the main issue. You can't access token data other then oauth_access_token_id, oauth_client_id, oauth_user_id, oauth_scopes.

Personally, i ended parsing the token again with Lcobucci\JWT\Parser to extract the custom claim from the token.

$token = (new \Lcobucci\JWT\Parser())->parse($jwt);

$tenantId = $token->getClaim('tenant_id');

[ragboyjr]

@maximebeaudoin interesting. I feel like a request attribute is most appropriate for things like a parsed representation of the JWT. I could be wrong.

Giving the entire token in the request would allow for a lot of customizations that wouldn't require updates to the library, but just a simple middleware downstream.

[simonhamp]

@ragboyjr I agree in principle, but I'm keen to get feedback from more implementors on this. It strikes me that it should be one way or the other, not both, but passing only a parsed token around forces writing the logic of extracting the most commonly used claim attributes onto implementors and feels like the wrong move.

Also, I'm not yet sure about the implications of lugging around a parsed JWT with the request through all the middleware.

One approach I've been thinking of is to simplify the attribute-to-claim mapping to a couple of string arrays, something like:

protected $defaultClaimAttributes = [
    'oauth_access_token_id' => 'jti',
    'oauth_client_id' => 'aud',
];

// Can be added in a subclass without any function definitions
protected $customClaimAttributes = [
    'custom_attribute' => 'custom_claim',
];

protected function appendAttributesFromToken(ServerRequestInterface $request, Token $token)
{
    $claimAttributes = array_merge($this->defaultClaimAttributes, $this->customClaimAttributes);

    foreach ($claimAttributes as $attr => $claim) {
        $request->withAttribute($attr, $token->getClaim($claim));
    }

    return $request;
}

[Sephster]

I think this is probably something we do need to overcome, especially if we are to make it easier to add custom claims to JWTs as has been mentioned in another PR.

This PR has exposed a slight architectural problem here. We've added a method to the bearerTokenValidator to extract the claims but I think this functionality is probably outside the scope of what a validator should be doing.

Arguably it would be nice to just have the validator verify the token but then have a separate function to determine what to do with the token once it has been validated to keep everything nice and modularised.

The problem I see at the moment is if we were to do this, where would this extraction code be house? No obvious answer springs to mind for me. Off the top of the head we might need a new response type for the resource server which we can use to provide such extensions but I haven't really thought this through so there might be complications with that which aren't immediately obvious

[simonhamp]

this functionality is probably outside the scope of what a validator should be doing.

I completely agree and I understand the points you're making, but there's no need as yet to over-engineer a solution. Why not just extract a method that can be overridden in a subclass more easily?

[Sephster]

While having a new method in the validator to create the http response would be the easiest solution, I don't believe it is the correct solution as it will muddy the responsibilities of our classes. I think pursuing an alternative approach would be better for the longevity and maintainability of the code base.

The class is currently generating a response but I think if we are going to adjust this, we should look to improve the class when we do so, and separating the concerns to make extensibility easier would be my preferred option.

[maximebeaudoin]

@Sephster Agree with you, i think we should find a alternative approach for this. I don't think doing a quick fix will be a good solution. For now, it's not impossible to use custom claim but you have to handle this outside of the package completely and you need to bypass the validation.

[simonhamp]

The class is currently generating a response

Are you sure? I think what's happening is it's just extracting token claims and adding them to the $request object for easy access during the request pipeline via middleware.

I do agree that it's still different concerns and definitely needs separating at some point.

[Sephster]

Sorry yep, I'd misread there :)