Callable for customizing JWT payload

[afilippov1985]

Even easier way for customizing JWT payload

Issue #750
Issue #793
PR #795

Usage:

$myapp = .... // $myapp may be instance of your application or Service Container
$server = new OAuth2\Server($storage, array(
    'use_jwt_access_tokens' => true,
    'jwt_extra_payload' => function($client_id, $user_id, $scope) use ($myapp) {
        $data = $myapp->getSomeData();
        return array(
            'custom_param' => $data['something_important']
        );
    }
));

one more example:

$myapp = .... // $myapp may be instance of your application or Service Container
$server = new OAuth2\Server($storage, array(
    'use_jwt_access_tokens' => true,
    'jwt_extra_payload' => array($myapp, 'getSomeData')
));

[svycka]

I would add $payload to the callable arguments instead of this array_merge

[afilippov1985]

Do you mean that callable must be defined like this? (pay attention that $payload passed by reference)
function(&$payload, $client_id, $user_id, $scope) {}

I think that standard JWT claims should be protected from external changes. That's why I put $payload last in array_merge()

[svycka]

no, I man instead of:

$extra = call_user_func($this->config['jwt_extra_payload'], $client_id, $user_id, $scope);
if (is_array($extra)) {
    $payload = array_merge($extra, $payload);
}

do this:

$payload = call_user_func($this->config['jwt_extra_payload'], $payload);

[svycka]

I don't see why can't change those if we need. specification does not say they are required and even if this lib does require some when we override them here it is most likely we will handle this change in other places as well.
@bshaffer what do you think?

[afilippov1985]

Doing so callable may break JWT. Standard JWT claims should be protected from external changes.

[svycka]

this feature is to generate those claims for example if you would want increase jwt token expire time for some user you could not. But if you only want to add new parameters then yes its ok

[bluebaroncanada]

I don't like any of this. It violates the paradigms the library was built on. I love doing callbacks like this, but it ought to be consistent. If it's coming down to just an array merge, I think #795 is the place to do this.

[svycka]

if it is not empty and not callable maybe better throw exception because it is most likely something wrong with implementation and should be fixed instead of silently ignore this.

[afilippov1985]

I agree

[svycka]

maybe better use call_user_func() instead of variable?

[afilippov1985]

Yes! My mistake.

[svycka]

because of if (isset($this->config['jwt_extra_payload'])) will throw exception on empty values like null, '', 0 or false

[svycka]

I think this is new feature and requires tests.

[svycka]

I don't think this is required

[bshaffer]

Yes it is, otherwise the array_merge below will throw an error.

[svycka]

@bshaffer this is about array_merge error vs custom error? result will be the same :D but maybe yes more clear error message is better

[bshaffer]

I would prefer these exceptions be handled like this:

if (!is_callable($this->config['jwt_extra_payload']))) {
    throw new \InvalidArgumentException("config['jwt_extra_payload'] is not callable");
}

$extra = call_user_func($this->config['jwt_extra_payload'], $client_id, $user_id, $scope);

if (!is_array($extra)) {
    throw new \InvalidArgumentException("config['jwt_extra_payload'] callable must return array");
}

$payload = array_merge($extra, $payload);

[svycka]

@bshaffer I still don't like the idea that it is impossible to change values in $payload.

[svycka]

since I dont like nesting I would change this to

if (!isset($this->config['jwt_extra_payload'])) {
    return $payload;
}
//...

[afilippov1985]

Let's vote:

• Defensive programming
  $payload = array_merge($extra, $payload); // critical data can't be changed

OR

• Flexibility
  $payload = array_merge($payload, $extra); // everything may be changed

[bshaffer]

This is looking great. We need to add the parameter here also!

[afilippov1985]

How about to change parameter name to jwt_extra_payload_callable to clarify its purpose?
May be some other name?

[bshaffer]

@afilippov1985 yes, I think jwt_extra_payload_callable works for me.

[bshaffer]

Everything looks great, two minor suggestions. Sorry this has taken so long... let's get this baby merged!!

[bshaffer]

nit, but I'd rather this just be "jwt_extra_payload_callable is not callable"

[bshaffer]

Why don't we just pass this function $payload and expect the returned array to be the new payload, e.g.:

$payload = call_user_func($this->config['jwt_extra_payload_callable'], $payload);

[afilippov1985]

This may break proper work of JWT
See our conversation with @svycka - https://github.com/bshaffer/oauth2-server-php/pull/804/files/86bd64e745ee34fa03bfb3456e5d2a2ab0a1c07e#r98169436
I agree to replace this
$payload = array_merge($extra, $payload);
to this
$payload = array_merge($payload, $extra);

[afilippov1985]

then, we should also rename the parameter to jwt_override_payload_callable

[bshaffer]

I'm convinced. Ty for the explanation.