Fix Passport Compatibility for V9

[Sephster]

The typing in version 9 has broken some Laravel Passport tests. I think the types have probably been too strict so am relaxing this a little to ease the burden of upgrading to version 9. Many identifiers were originally string only but I've changed this to allow ints or strings and cast where appropriate to strings (usually for the JWT builder).

[hafezdivandari]

@Sephster would you please check this one:

oauth2-server/src/Repositories/ScopeRepositoryInterface.php

Line 42 in 8264a40

string|int|null $userIdentifier = null,

[Sephster]

Good find. Thanks for that. I've fixed now

[hafezdivandari]

@Sephster 6 tests are failing on Passport:

TypeError: League\OAuth2\Server\Grant\AbstractGrant::getClientEntityOrFail(): Argument #1 ($clientId) must be of type string, int given, called in \src\Grant\ClientCredentialsGrant.php on line 39

I believe we can fix them all by casting the $clientId on this line:

oauth2-server/src/Grant/AbstractGrant.php

Line 209 in e4dfdd6

$clientId = $this->getRequestParameter('client_id', $request, $basicAuthUser);

-$clientId = $this->getRequestParameter('client_id', $request, $basicAuthUser); 
+$clientId = (string) $this->getRequestParameter('client_id', $request, $basicAuthUser); 

Also this one:

oauth2-server/src/Entities/UserEntityInterface.php

Line 20 in e4dfdd6

public function getIdentifier(): mixed;

[eugene-borovov]

I`d suggest to make getRequestParameter stricter. All parameters for the library suppose to be string. So why do it return mixed?

protected function getRequestParameter(array $parameter, ServerRequestInterface $request, ?string $default = null): ?string

Current contract should return string or null, but $request->getParsedBody() may return non-string values.

/**
     * Retrieve request parameter.
     *
     * @param string                 $parameter
     * @param ServerRequestInterface $request
     * @param mixed                  $default
     *
     * @return null|string
     */
    protected function getRequestParameter($parameter, ServerRequestInterface $request, $default = null)

[hafezdivandari]

Also these methods can't have mixed $default = null when they are returning ?string:

oauth2-server/src/Grant/AbstractGrant.php

Line 333 in e4dfdd6

protected function getQueryStringParameter(string $parameter, ServerRequestInterface $request, mixed $default = null): ?string

oauth2-server/src/Grant/AbstractGrant.php

Line 341 in e4dfdd6

protected function getCookieParameter(string $parameter, ServerRequestInterface $request, mixed $default = null): ?string

oauth2-server/src/Grant/AbstractGrant.php

Line 349 in e4dfdd6

protected function getServerParameter(string $parameter, ServerRequestInterface $request, mixed $default = null): ?string

I suggest to search for mixed keyword on the package and fix them all at once.

[Sephster]

I've changed the code now to make getRequestParameters more strict and remove as much of the mixed types as I can. Three remain but I think this is ok.

Hopefully we can just change Passport now to cast ID's pulled from the DB to strings. I'm still not entirely convinced that we shouldn't allow ints here. Feels like we are passing too much of the upgrade burden on to the users so would be keen to see what changes are involved to get Passport to work.

Let me know if you need anything else @hafezdivandari and thanks again for your continued support with this

[hafezdivandari]

Thank you @Sephster

We can cast the client_id to string on Passport tests, but if the end-user send the the client_id as an integer, we expect to get OAuthServerException::invalidRequest('client_id') but we get TypeError: League\OAuth2\Server\Grant\AbstractGrant::getRequestParameter(): Return value must be of type array|string|null, int returned instead.

The problem is that we restricted these methods (e.g. getRequestParameter()) to return string then trying to return the exception if it's not string.

[hafezdivandari]

This line is unreachable if the client_id is integer.

[Sephster]

Sorry when this discussion started I was under the assumption that we were all on board with casting to strings being the responsibility of the implementer of the library. This is unavoidable with some of the changes suggested above.

Originally I had the IDs accepting either strings or integers to avoid this problem.

I am not entirely convinced that enforcing strings everywhere is the best approach as it does increase the upgrade burden for users of this library and so, am leaning towards reverting some of the changes here to allow integers for IDs again, and making getRequestParameter a lot looser to reflect how many different types can actually be provided by this function.

Is there a good reason not to revert these changes that I'm missing? Would welcome any thoughts before I revert this as above. Thanks for your continued assistance with this @hafezdivandari and for your input @eugene-borovov

[hafezdivandari]

On Laravel Passport, we pass the request to the oauth-server as is, without any modification:

https://github.com/laravel/passport/blob/8ea1dd41745c89769fd8c4b207c4739eea707e95/src/Http/Controllers/AccessTokenController.php#L52

We can't and I think we shouldn't validate or cast the request's values (on Passport side). However the rfc defines client_id as a unique string. I'll send another PR to Laravel Passport after the current one, to only have uuid for client_id instead of incremental integers.

I agree with making getRequestParameter() return type looser, it totally makes sense.

[Sephster]

My interpretation of the RFC is that when they say string, the just mean we can use a sequence of numbers or characters, rather than making us impose a strict type of string for client IDs. This is the way the oauth2 server has worked up to now. We've allowed both ints and strings as IDs.

I appreciate v9 is a big change with the type system being added and I'm keen to avoid too much friction. If we can't solve ints being sent in without a change in a users implementation, I'd prefer to just allow ints and strings and relax constraints in this area a bit.

It doesn't seem worth the effort or pain for end users to be so prescriptive here and I don't want to hold up the release much longer.

Can you foresee any strong reason not to revert some of the changes? I'm hoping when I do this all Passport tests will pass and we can tag v9 properly. Thanks for your quick replies on this btw! Really appreciate it.

[hafezdivandari]

Sure, let's do it. I'll rerun the Passport tests after this.

[Sephster]

Thanks @hafezdivandari. I will do this tomorrow to give @eugene-borovov time to reply but all being well, will get these changes in place. Thanks!

[eugene-borovov]

Hi @Sephster

I suggested treating identifiers as strings, that is, converting all scalar parameters to a string so that the library does not analyze types. At the time of receiving the parameters, its should be checked and converted to a string. Thus, the end user can specify the parameters as it is convenient for him. Of course, you can force the user to specify all parameters as a string, but then you need to check the types and throw an exception in case of a type mismatch. I think it's best to relax the requirements for input parameters, but convert the values to a string.

Let me illustrate my point.

abstract class AbstractGrant implements GrantTypeInterface
{

    private static function _request(string $parameter, array $data, ?string $default = null): ?string
    {
        $value = $data[$parameter] ?? $default;

        if ($value === null) {
            return null;
        }

        if (\is_scalar($value)) {
            return (string)$value;
        }
        
        throw new \InvalidArgumentException('Unsupported parameter type'); // or return null
    }

    /**
     * Retrieve request parameter.
     *
     */
    protected function getRequestParameter(string $parameter, ServerRequestInterface $request, string $default = null): ?string
    {
        return self::_request($parameter, (array)$request->getParsedBody(), $default);
    }

    /**
     * Retrieve query string parameter.
     */
    protected function getQueryStringParameter(string $parameter, ServerRequestInterface $request, ?string $default = null): ?string
    {
        return self::_request($parameter, $request->getQueryParams(), $default);
    }

    /**
     * Retrieve cookie parameter.
     */
    protected function getCookieParameter(string $parameter, ServerRequestInterface $request, ?string $default = null): ?string
    {
        return self::_request($parameter, $request->getCookieParams(), $default);
    }

    /**
     * Retrieve server parameter.
     */
    protected function getServerParameter(string $parameter, ServerRequestInterface $request, ?string $default = null): ?string
    {
        return self::_request($parameter, $request->getServerParams(), $default);
    }

}

[Sephster]

Sorry for the delay in getting back to this. I was sick this week so just picking this up now. Thanks for your feedback Eugene. Totally get where you are coming from now and I can see the value in this, given all the is_string checks added back in 2021 to try sanitise some of the inputs to the library.

The mixed types coming in via the request object is certainly causing some headaches so a wrapper function like this is appealing.

I've reverted some of the issues that were causing problems for Passport plus my changes making types even stricter. @hafezdivandari please could you check these against PP when you get a chance? If all is well, I will look to see if I can easily implement Eugene's proposal without causing too many headaches.

As long as it doesn't delay the release of v9, I think these are sensible changes to make. Thanks both

[hafezdivandari]

Thank you @Sephster

I re-ran the Passport tests with the latest changes, I'm still getting http 500 when sending integer client_id. As I said on the prev comment (#1402 (comment)), I expect OAuthServerException::invalidRequest('client_id') instead of TypeError.

You may check the tests result here: https://github.com/laravel/passport/actions/runs/8961394067?pr=1734

[Sephster]

Thank you. Sorry, I thought I'd fixed that. Will check asap

[hafezdivandari]

I think we were on the right track here, checking if client_id is string.

[hafezdivandari]

same as above.

[hafezdivandari]

same.

[hafezdivandari]

Suggested change

	* @return mixed[]
	* @return string[]

[hafezdivandari]

Passport tests are passing, thank you.