GrantInterface::createAuthorizationResponse does not accept `null` for $client

[basz]

Trying to get 100% coverage I run into a possible issue.

For grants that allowPublicClients() and where the request does not contain a client_id a null is return by the AuthorizationServer ::getClient method.

That null client is then fed into GrantInterface::createAuthorizationResponse, but it that interface doesn't allow for $client to be null.

https://github.com/zf-fr/zfr-oauth2-server/blob/master/src/AuthorizationServer.php#L171-L173 https://github.com/zf-fr/zfr-oauth2-server/blob/master/src/Grant/GrantInterface.php#L55

[prolic]

We could return a NullObject perhabs?

[basz]

Perhaps, but i'm curious as to of this path is meant to be taken. @bakura10

[bakura10]

Hmm... Seems an error in the interface, as client less authentication is definitely something allowed. Simply modify the interface? It would also be on par with this one: https://github.com/zf-fr/zfr-oauth2-server/blob/master/src/Grant/GrantInterface.php#L67

[basz]

Are you sure? As https://github.com/zf-fr/zfr-oauth2-server/blob/master/src/Grant/AuthorizationGrant.php#L104 definitely expects a $client and I think a implicit grant flow also does.

Looking at http://brentertainment.com/oauth2/ I also see a client (demoapp) in the url...

edit: additionally https://tools.ietf.org/html/rfc6749#section-4.1.1 and https://tools.ietf.org/html/rfc6749#section-4.2.1 specifically defines a client_id as required

[basz]

therefore I propose that instead of changing the interface we should add something like;

if (null === $client) {
    throw OAuth2Exception::invalidClient('No client could be authenticated');
}

at

[bakura10]

I'm not sure to understand. Some grant do not require client (https://github.com/zf-fr/zfr-oauth2-server/blob/master/src/Grant/PasswordGrant.php) - actually all grants that allow public authentication -.

So I'd say changing the interface to allow null AND checking on grants that require client as you did.

Considering the sematnic that some grants require client and some other does not, it seems more logical to me to modify the interface to allow null (until we can use 7.1 with nullable typehints), and check for those that need it, rather than the opposite?

The only thing that bother me is the spec requiring a client_id... It's been a long time since I've read this spec, but how do they expect public grants to handle this issue? Do they assume that a client ID should ALWAYS be given (like a hard-coded one for public grants)?

[basz]

I think the confusion comes from two things

1. the createAuthorizationResponse method is only used be the AuthorsationGrant type (and the ImplicitGrant when implemented). see PasswordGrant ClientCredentialsGrant RefreshTokenGrant
2. a public client does not mean there is no client, just that it is public.

The only thing that bother me is the spec requiring a client_id... It's been a long time since I've read this spec, but how do they expect public grants to handle this issue? Do they assume that a client ID should ALWAYS be given (like a hard-coded one for public grants)?

I think so yes, without a secret.

Because of 1 I argue the interface doesn't need to be changed

[bakura10]

That makes sense :). So goes on with throwing an exception if client is not there :).

[basz]

Done, thank you