thephpleague / oauth2-server

A spec compliant, secure by default PHP OAuth 2.0 Server
https://oauth2.thephpleague.com
MIT License
6.51k stars 1.12k forks source link

JSON encoding ScopeEntityInterface #1262

Closed AdeAttwood closed 2 years ago

AdeAttwood commented 2 years ago

Currently, when encoding a ScopeEntityInterface for the JWT, it relies on the jsonSerialize function of the class to return a string of the scope identifier. In one of our apps we are struggling with this a bit as we would like the jsonSerialize function to return an object.

Would you be interested in accepting and PR what would use the getIdentifier method on the ScopeEntityInterface to encoded into the tokens. I think this has the benefits of being able to be typed better rather than relying on the jsonSerialize that is unable to be typed.

I was thinking something like.

array_map(
    function (ScopeEntityInterface $scope): string {
        return $scope->getIdentifier();
    },
    $this->getScopes()
);

Let me know what you think. I am not 100% sure if this would brake BC however I can't see how it would. I think below is the only two places that would need to be change.

Sephster commented 2 years ago

Hello. Cheers for getting in touch. Are you not able to use a custom trait to achieve this instead of the default ScopeTrait provided? I think this might be a better route to go down but if you disagree, please could you provide more context as to why you need the scope to be encoded as an object? I'd be interested in the use-case. Thanks

AdeAttwood commented 2 years ago

For context, we have an API for managing our scopes. Our scope entity has a id and description. Then in our actions we have something like.

return $response->getBody()->write(\json_encode($scope));

This will encode to

{
  "id": 1,
  "identifier": "profie",
  "description": "Scope for viewing the profile"
}

I think this will be a good idea to change to ensure consistency in the package, and remove any ambiguous behaviour. For example, in the BearerTokenResponse.php:36 the scopes are the only one that is interfaced to return ScopeEntity[] all the others return string or int.

$refreshTokenPayload = \json_encode([
    'client_id'  => $this->accessToken->getClient()->getIdentifier(),
    'refresh_token_id' => $this->refreshToken->getIdentifier(),
    'access_token_id' => $this->accessToken->getIdentifier(),
    'scopes'  => $this->accessToken->getScopes(),
    'user_id' => $this->accessToken->getUserIdentifier(),
    'expire_time' => $this->refreshToken->getExpiryDateTime()->getTimestamp(),
]);

By refactor in this to be like below we are using all the interfaced methods on each entity, this will then ensure the correct behaviour and not relying on the untyped jsonSerialize

$scopes = array_map(
    function (ScopeEntityInterface $scope): string {
        return $scope->getIdentifier();
    },
    $this->accessToken->getScopes()
);

$refreshTokenPayload = \json_encode([
    'client_id' => $this->accessToken->getClient()->getIdentifier(),
    'refresh_token_id' => $this->refreshToken->getIdentifier(),
    'access_token_id'  => $this->accessToken->getIdentifier(),
    'scopes' => $scopes,
    'user_id' => $this->accessToken->getUserIdentifier(),
    'expire_time' => $this->refreshToken->getExpiryDateTime()->getTimestamp(),
]);

For now, we have built in a work around that overrides the entity and one for the API and one for this package, so we are all good.

Sephster commented 2 years ago

Thank you for the explanation @AdeAttwood. I'm a bit confused so please bear with me. In the original post you said you wanted jsonSerialize to return the object instead of the identifier but in the code above, it looks like jsonSerialize is being bypassed entirely. Are you using the jsonSerialize function elsewhere?

I see what you mean about types though. In our trait, we specify that we will be returning a string but in the interface, the jsonSerializable states the return will be mixed which isn't ideal as we need this to be a string.

AdeAttwood commented 2 years ago

In the original post you said you wanted jsonSerialize to return the object instead

Sorry if this was not clear.

Are you using the jsonSerialize function elsewhere?

Yea this in our application code, we would like to return as object from jsonSerialize. That was then encoding the object into the JWT and as you can imagine was causing all sorts of problems.

Sephster commented 2 years ago

Hi @AdeAttwood. Thanks for taking the time to explain your problem. I've given this a lot of thought and I think we should be leaving this as is in the library. It has been like this for over 7 years and I don't recall anyone raising issues about this before. We could change this to use _arraymap as you have suggested but this would make the code slightly longer. I believe the original author chose jsonSerialize because it is a neat way to convert our object to scope strings.

I appreciate this will be interfering with your own code but I think the path of least resistance here would be for you to update your function to retrieve the data you need in a different manner rather than ask the lib to change the way we currently use jsonSerialize.

I take on board your point about needing a string to be returned so I might update the interface to be more specific about what we return from jsonSerialize in the next release. I'm sorry we won't be implementing your request at this time but thank you for bringing this issue to our attention