support for setDefaultScope

[corbosman]

Hi, the rfc says (https://tools.ietf.org/html/rfc6749#page-23) that when a client does not pass a scope parameter, the server can choose to either return a default set of scopes, or a scope error. Laravel kind of does neither. When I omit the scope parameter laravel simply doesn't set a scope, and doesn't return a scope error.

The league server does support a setDefaultScope method on the AuthorizationServer specifically for this, but unfortunately right now the only way to be able to set that is through the PassportServiceProvider in the registerAuthorizationServer singleton. When you enable a grant, the oauth2 server can set a previously set default scope.

see: https://github.com/thephpleague/oauth2-server/blob/master/src/Grant/AuthCodeGrant.php#L259

It would be nice if we could do a Passport::setDefaultScope([..]) through the AuthServiceProvider, and then have passport set that on the AuthorizationServer before enabling the grants. I implemented that locally as a test and that works great. Would a PR be welcome for this? Or is this not something laravel would want to support?

[driesvints]

Ping @Sephster. Is this something that we could add here on Passport?

[Sephster]

Hey @driesvints, it probably should be something that is added to Passport. By default, the scope set on the AuthorisationServer is a blank string. When this change was introduced, if you didn't set a default scope, and didn't specify any scopes in a request, the request would fail as @corbosman notes.

However, we didn't introduce this change in a major version and it caused some Passport instances to fail where they had previously worked #583

An update was issued to stop requests failing but I will probably revert this for version 8: thephpleague/oauth2-server@6.1.0...6.1.1

The AbstractGrant class has a public function called setDefaultScope() which can be used to set the scope. Passport should call that function.

[driesvints]

@Sephster thanks for the detailed explanation! :)

[corbosman]

@Sephster Wouldnt it be better to call the public method setDefaultScope() on the AuthorizationServer? It in turn calls setDefaultScope on all the grants in the enableGrantType() method. Then laravel can just call setDefaultScope on the server before enabling all the grants.

See:

https://github.com/thephpleague/oauth2-server/blob/master/src/AuthorizationServer.php#L229
https://github.com/thephpleague/oauth2-server/blob/master/src/AuthorizationServer.php#L131
https://github.com/laravel/passport/blob/7.0/src/PassportServiceProvider.php#L95

In the PassportServiceProvider laravel could just call $server->setDefaultScope(..).

[corbosman]

@driesvints I think this commit would add the necessary code to support this enhancement.

corbosman@97132b9

It should be fully backwards compatible with the current code in 7.0 so that's why I committed it to 7.0. If you don't set a default scope, it's simply going to be exactly how it is now, ie laravel won't fail and returns a token without scopes. If you do set a default scope, and no scope parameter was passed in the request, you'll get the default scope. You set the default scope similarly to tokensCan() in the AuthServiceProvider like so:

Passport::setDefaultScopes([...]);

I named it 'setDefaultScopes' and not setDefaultScope to be more consistent with how laravel seems to be naming arrays of scopes. If you'd prefer setDefaultScope i can simply change that, not a problem.

I've been thinking about how to create a test for this, but I'm not sure how to approach this as most of the heavy lifting is actually done inside oauth2-server, not in laravel. In laravel you basically only configure this. So you wouldn't be able to mock the AuthorizationServer, as it handles the logic.

Any comments on this change? If you prefer I go to discord to discuss this, also fine.

[corbosman]

Come to think of it, i guess the Passport::$defaultScope could just be a space delimited set of scopes just like oauth2-server expects, then there's no need for an array. Any preferences?

[driesvints]

@corbosman yeah I think just a string would be fine. You might also do an is_array check inside the setDefaultScopes method. And then simple save the scopes as a string on the object.