Scopes in separate tables

[sheridans]

I've been working with integrating doctrine with zend-expressive-oauth2 which officially uses this library, I have the auth flow pretty much working whereby user has to login and authorise clients & scopes (if present).

During the process due to having a large amount of scopes (and readability) I wanted to drop the [imploded] "scopes" column from the oauth_access_tokens table, create related scopes table, "oauth_token_scopes" which consisted of (token_id,scope_id).

There's one function in the AbstractGrant class which is causing me problems doing this:

private function convertScopesQueryStringToArray($scopes)
{
    return array_filter(explode(self::SCOPE_DELIMITER_STRING, trim($scopes)), function ($scope) {
        return !empty($scope);
    });
}

Throws an exception "trim expects parameter 1 to be a string, object given", understandable as I am no longer using imploded string but array from related table.

I am able to fix and allow scopes in separate table by modifying the above function to:

private function convertScopesQueryStringToArray($scopes)
{
    // parse as string as usual
    if (is_string($scopes)) {
        return array_filter(explode(self::SCOPE_DELIMITER_STRING, trim($scopes)), function ($scope) {
            return !empty($scope);
        });
    }

    // parse array
    $scopes = (array)json_decode(json_encode($scopes), true);
    return array_map('trim', $scopes);
}

Any chance I could submit a suggestion, or PR, to allow this functionality?

[Sephster]

Hi @sheridans. The convertScopesQueryStringToArray() function is called from the validateScopes() function. The scopes are usually passed from the HTTP request or from the decrypted auth code which is a JSON object. This will always be a string as far as I can tell.

I'm wondering if there is a scenario I've missed where we are passing the scopes from the scope repository? If there is a scenario I've missed, could you let me know and we can take a look at this further. Thanks

[sheridans]

I may be wrong and it could be something to do with doctrine entities, seems to be related to the below section of code in AbstractGrant:

    public function validateScopes($scopes, $redirectUri = null)
    {
        if (!\is_array($scopes)) {
            $scopes = $this->convertScopesQueryStringToArray($scopes);
        }
        $validScopes = [];
        foreach ($scopes as $scopeItem) {
            $scope = $this->scopeRepository->getScopeEntityByIdentifier($scopeItem);
            if ($scope instanceof ScopeEntityInterface === false) {
                throw OAuthServerException::invalidScope($scopeItem, $redirectUri);
            }
            $validScopes[] = $scope;
        }
        return $validScopes;
    }

Since convertScopesQueryStringToArray($scopes) is private I managed to overcome the issue by extended the AbstractGrant class and overriding the above function which then allowed me to override the convertScopesQueryStringToArray() function.

Possibly to do with:

if (!\is_array($scopes)) {
            $scopes = $this->convertScopesQueryStringToArray($scopes);
        }

and doctrine collection types I guess as I had OneToMany relationship defined within my AuthTokenEntity class to my ScopesEntity

[philrennie]

I've been hitting this also, where I have scopes as a doctrine entity.

When the code is unpacked in AuthCodeGrant->respondToAccessTokenRequest() line 88 $authCodePayload = json_decode($this->decrypt($encryptedAuthCode)); the scopes property is a single empty stdClass.

I think the problem comes down to when the code is created in completeAuthorizationRequest() $payload = [ ...... 'scopes' => $authCode->getScopes(), ..... ]; although the scopeEntityInterface makes the scopes jsonSerializable the scopes of the payload end up as an empty object once json_encoded.

Have added a PR that I think fixes the issue.

[sheridans]

Good work, glad it wasn't just me having that issue, would be good to get the PR merged. I overcame the issue by extending each of the grant classes in my applications, which is tedious.

[Sephster]

Do either of you have some example code you can share? From @philrennie's pull request it looks like the way you have implemented the getScopes() function is the problem?

Is there a reason you can't just have your version of getScopes return an array of scopes? This would be the correct way to implement the function

[philrennie]

Hi,

I think the underlying issue is the json_encode on line 358 of AuthCodeGrant. The contents of the scopes array shouldn't need to take it in to account.

So in my code getScopes returns a doctrine arrayCollection of scope entities, which implement scopeEntityInterface, which satisfies the constraint for getScopes() to return an array, but doesn't json_encode properly.

/**
     * @var ScopeEntityInterface[]
     *
     * @ORM\ManyToMany(targetEntity="Oauth_scope")
     * @ORM\JoinTable(name="oauth.oauth_authcode_oauth_scope",
     *     joinColumns={@ORM\JoinColumn(name="authcodeid", referencedColumnName="id")},
     *     inverseJoinColumns={@ORM\JoinColumn(name="scopeid", referencedColumnName="id")}
     * )
     */
    private $scopes;

If we look at the auth server code AuthCodeGrant->respondToAccessTokenRequest() calls $this->validateScopes($authCodePayload->scopes) on line 93. validateScopes() expects the scopes to have been extracted from the auth code as an array of strings representing the stored identifiers of the scope entities, and uses these to get the entities from the scopeRepository.

In my PR by calling getIdentifier explicitly for each scope means the scopes property of the authcode is built explicitly as an array of strings regardless of the underlying structure if the scopes array.

This means we don't have to rely of the scopes array having to worry about whatever magic the json_encode on line 358 of AuthCodeGrant does.

[Sephster]

Sorry but I don't think this is an issue the library should be fixing. If I understand correctly, your scopes are not a simple array, but instead a Doctrine arrayCollection which is a wrapper around an array object.

The library does not expect to receive an arrayCollection which I think is what you are trying to work around. Because this is an implementation issue that doesn't affect the majority of implementations, I think it would be best being fixed in your code.

You should change your implementation to provide an array rather than an arrayCollection.

Is there any reason why you can't do this? Thanks for getting back to me with a thorough explanation.

[yovchev]

Throws an exception "trim expects parameter 1 to be a string, object given", understandable as I am no longer using imploded string but array from related table.

@sheridans the function validateScopes is correct it expects string or array as specified in * @param string|array $scopes line 283 and instead you are passing an object that's why it fails with expects parameter 1 to be a string because is not an array where is this object generated can you share the implementation of the AuthCodeEntity in your setup?

@philrennie

Can you confirm your Doctrine ArrayCollection extends JsonSerializable

If you check the src\Entities\ScopeEntityInterface.php extends JsonSerializable

namespace League\OAuth2\Server\Entities;

use JsonSerializable;

interface ScopeEntityInterface extends JsonSerializable
{
    /**
     * Get the scope's identifier.
     *
     * @return string
     */
    public function getIdentifier();
}

[yovchev]

Also as @Sephster mentioned why don't you just override the getScopes function in your AuthCodeEntity implementation

namespace OAuth2ServerExamples\Entities;

use League\OAuth2\Server\Entities\AuthCodeEntityInterface;
use League\OAuth2\Server\Entities\Traits\AuthCodeTrait;
use League\OAuth2\Server\Entities\Traits\EntityTrait;
use League\OAuth2\Server\Entities\Traits\TokenEntityTrait;

class AuthCodeEntity implements AuthCodeEntityInterface
{
    use EntityTrait, TokenEntityTrait, AuthCodeTrait;

    /**
     * Return an array of scopes associated with the token.
     *
     * @return ScopeEntityInterface[]
     */
    public function getScopes()
    {
        $scopes = [];

        foreach ($this->scopes as $scope) {
            $scopes[] = $scope->getIdentifier();
        }

        return $scopes;
    }
}

[Sephster]

Thanks for the input @yovchev. I agree with what you are saying. I think this is an implementation issue that can be resolved by the implementer without needing to change the server. Because of this, and the lack of follow up comments, I'm going to close this issue.

However, if I have misunderstood the underlying issue, please provide me with a concrete example of the issue at hand and I will be happy to investigate and reopen this issue.

Thanks everyone for your input in this issue.