always_issue_new_refresh_token does not work in OAuth2\Server constructor

[PHPGangsta]

Hi,

first: I love your library, it's really easy to create an OAuth2 token controller and application!

I found a small problem: In the docs it says that I can give the config setting 'always_issue_new_refresh_token' to the OAuth\Server constructor, see here:
http://bshaffer.github.io/oauth2-server-php-docs/grant-types/refresh-token/

However, both of these configuration options can be sent in when the server is created using the server's configuration array:

$server = new OAuth2\Server($storage, array(
  'always_issue_new_refresh_token' => true,
  'refresh_token_lifetime'         => 2419200,
));

For me this does not work, and as far as I can see it is not supported in the code. This setting has to be done in the RefreshToken-constructor.

Now there are 2 solutions: correct the docs, or implement this feature. I don't know which is better.

Michael

[bshaffer]

Can you explain what you mean by "For me this does not work"?

I have looked at the code, and we have tests that ensure always_issue_new_refresh_token works, and although we don't have test coverage, you can see in the code here that the refresh_token_lifetime configuration is also passed in from the Server class.

Perhaps you can post the code you use to instantiate the OAuth2\Server object, or submit a PR with a test case that proves this fails, and then I can better address your problem.

[PHPGangsta]

Hi bshaffer,

the test "testValidRefreshTokenWithNoRefreshTokenInResponse" only tests "always_issue_new_refresh_token" when given to the RefreshToken constructor, not to the OAuth\Server constructor.

Please try exactly what is written in the docs, and as quoted above. Try to set "always_issue_new_refresh_token" in the OAuth\Server constructor, you will see that it does not work, you don't get a new refresh token, because it's not merged into the RefreshToken object where it's used.

[bshaffer]

Aha, yes, my mistake. But this also has test coverage here

[PHPGangsta]

If you check the source code here:
https://github.com/bshaffer/oauth2-server-php/blob/develop/src/OAuth2/GrantType/RefreshToken.php#L32
you can see that only the RefreshToken constructor parameter $config is used to configure "always_issue_new_refresh_token". If you set this option in the OAuth2\Server constructor it's not used.

In the docs it says this is possible, but it isn't:

$server = new OAuth2\Server($storage, array(
  'always_issue_new_refresh_token' => true,
));

Maybe you can try it yourself?

[bshaffer]

I've now added this test as well, which is the same as the one referenced above, but setting the always_issue_new_refresh_token config on a server level. The test passes.

So I think there must be something else in your implementation that is different from the tests.

[bshaffer]

I think I know the problem...

The configuration to OAuth2\Server is ONLY used if the refresh token grant type isn't explicitly provided. For instance, if you do this:

$server = new OAuth2\Server($storage, array(
  'always_issue_new_refresh_token' => true,
));

$server->addGrantType(new RefreshToken());

Then the configuration supplied in the constructor for OAuth2\Server will be overwritten by the configuration supplied to RefreshToken.

My guess is this is the problem you're having. This behavior is working as designed, as the OAuth2\Server class is only a convenience class which wraps the other controllers and instantiates defaults. If you override these classes, then this will happen.

I agree this may not be expected behavior. If you have a proposed fix, I will take it into consideration. Otherwise, I'm closing this issue.

[PHPGangsta]

Thanks for the explanation, but I don't really understand, that makes no sense to me... If I don't add the RefreshToken grant type explicitly, I cannot use the refresh token feature...

If I just do this:
$server = new OAuth2\Server($storage, array(
'always_issue_new_refresh_token' => true,
));

then I get:
{"error":"unsupported_grant_type","error_description":"Grant type "refresh_token" not supported"}

That means "always_issue_new_refresh_token" is not used anywhere (because it's only used in the RefreshToken grant type, and that is disabled if I don#'t add it explicitly).
And if I add the grant type explicitly, then it is not used either, then I have to use the option parameter of the RefreshToken constructor...

So I'm not sure in which case this option makes any sense in the OAuth\Server constructor... Maybe you can enlight me?

[bshaffer]

Yeah, no problem. The only reason that grant type wouldn't be added is if 1) your $storage object does not implement OAuth2\Storage\RefreshTokenInterface, or if you explicitly set your grant types in the constructor of the Server class, or if you specify a storage array and map the storages to keys, such as:

$server = OAuth2\Server(array('client_credentials'=> $storage));
—
Brent Shaffer

On Tue, Jul 1, 2014 at 6:22 PM, Michael Kliewe notifications@github.com
wrote:

Thanks for the explanation, but I don't really understand, that makes no sense to me... If I don't add the RefreshToken grant type explicitly, I cannot use the refresh token feature...
If I just do this:
$server = new OAuth2\Server($storage, array(
'always_issue_new_refresh_token' => true,
));
then I get:
{"error":"unsupported_grant_type","error_description":"Grant type "refresh_token" not supported"}
That means "always_issue_new_refresh_token" is not used anywhere (because it's only used in the RefreshToken grant type, and that is disabled if I don#'t add it explicitly).
And if I add the grant type explicitly, then it is not used either, then I have to use the option parameter of the RefreshToken constructor...

So I'm not sure in which case this option makes any sense in the OAuth\Server constructor... Maybe you can enlight me?

Reply to this email directly or view it on GitHub:
#40 (comment)

[PHPGangsta]

I'm sorry, but all 3 things do not apply... Here is my code:

$config = array(
    'access_lifetime'                => 7*86400,    // 7 days
    'refresh_token_lifetime'         => 6*31*86400, // 6 month
    'always_issue_new_refresh_token' => true
);

$storage = new \App\OAuth2\Storage\Pdo();
$server = new OAuth2\Server($storage, $config);
$server->addGrantType(new OAuth2\GrantType\UserCredentials($storage));
$server->handleTokenRequest(OAuth2\Request::createFromGlobals())->send();

My own Pdo class looks like this:

class Pdo implements AccessTokenInterface, ClientCredentialsInterface, UserCredentialsInterface, RefreshTokenInterface

As you can see

1. I'm implementing RefreshTokenInterface in my storage class
2. I don't set the grant types in the constructor of the Server class
3. I don't have a storage array and I'm not mapping the storages to keys

But the RefreshToken grant type is not added automatically, I have to do it explicitly...

Maybe I should not kill your time any longer, it's working if I set the parameter in the constructor of the RefreshToken, that's all I want at the moment. It would be nice if all configuration settings could be in one place (the server constructor), but that's not the case, I'm fine with that at the moment.

[bshaffer]

Try removing the line where you add a user credentials grant type.  If that works, then I think I know the problem…—
Brent Shaffer

On Tue, Jul 1, 2014 at 7:05 PM, Michael Kliewe notifications@github.com
wrote:

I'm sorry, but all 3 things do not apply... Here is my code:
$config = array(
'access_lifetime' => 7_86400, // 7 days
'refresh_token_lifetime' => 6_31*86400, // 6 month
'always_issue_new_refresh_token' => true
);

$storage = new \App\OAuth2\Storage\Pdo();
$server = new OAuth2\Server($storage, $config);
$server->addGrantType(new OAuth2\GrantType\UserCredentials($storage));
$server->handleTokenRequest(OAuth2\Request::createFromGlobals())->send();

My own Pdo class looks like this:
class Pdo implements AccessTokenInterface, ClientCredentialsInterface, UserCredentialsInterface, RefreshTokenInterface
As you can see

1. I'm implementing RefreshTokenInterface in my storage class
2. I don't set the grant types in the constructor of the Server class
3. I don't have a storage array and I'm not mapping the storages to keys
   But the RefreshToken grant type is not added automatically, I have to do it explicitly...

Maybe I should not kill your time any longer, it's working if I set the parameter in the constructor of the RefreshToken, that's all I want at the moment. It would be nice if all configuration settings could be in one place (the server constructor), but that's not the case, I'm fine with that at the moment.

Reply to this email directly or view it on GitHub:
#40 (comment)

[PHPGangsta]

You are right, if I remove the addGrantType() call, then it works as you said...

But in my case I want to only allow the grant_type=password (and refresh_token), and disallow grant_type=client_credentials. To do that I had to add UserCredentials and RefreshToken grant types explicitly. And then I had to move the configuration of "always_issue_new_refresh_token" from the server constructor to the RefreshToken constructor. That was the problem I had, that was not obvious and a bit strange.

Thanks for you help, let's hope nobody else faces this problem ;-)

[bshaffer]

Great! Yes, I agree the convenience of the server class is a double – edge sword. However, I cannot think of a better way to handle this, other than maybe by throwing an exception when this problem occurs. What are your thoughts on this?—
Brent Shaffer

On Tue, Jul 1, 2014 at 7:21 PM, Michael Kliewe notifications@github.com
wrote:

You are right, if I remove the addGrantType() call, then it works as you said...
But in my case I want to only allow the grant_type=password (and refresh_token), and disallow grant_type=client_credentials. To do that I had to add UserCredentials and RefreshToken grant types explicitly. And then I had to move the configuration of "always_issue_new_refresh_token" from the server constructor to the RefreshToken constructor. That was the problem I had, that was not obvious and a bit strange.

Thanks for you help, let's hope nobody else faces this problem ;-)

Reply to this email directly or view it on GitHub:
#40 (comment)