Optional Refresh Tokens

[filecage]

This PR resolves #649 by allowing a RefreshTokenRepository to return null on getNewRefreshToken().

I chose this way of implementing the optional Refresh Token because from what I've seen it's the only way of doing it without breaking BC - I'd actually prefer not passing a RefreshTokenRepository at all when I don't want to issue a refresh token, but that would mean having a nullable type hint and that would break support for PHP 7.0.

I'm looking forward to your feedback!

[filecage]

@Sephster The PHPStan and Style CI checks have incosistent behaviour. Declaring return null; makes Style CI remove the null and making it a void return which is then failing in Travis CI due to PHPStan.

Please advise.

[Sephster]

I think it is better to be explicit so I would favour removing the StyleCI check here.

[filecage]

I agree, thank you! Changed and pushed.

[filecage]

@Sephster please tell me whether you'd like me to remove the simplified_null_return style CI fixer to make all the checks pass. I have no experience with style CI and cannot tell whether removing it will break anything else.

[filecage]

@Sephster short reminder that this PR can not pass the checks if you don't give me advice whether to change the style CI settings.

Your feedback is very much appreciated.

[Sephster]

Sorry @filecage I've been busy on another project which is now coming to a close so will have some time to look at this this weekend. I had a quick glance to see if it was obvious what needs to be changed but I can't see the rule that is being triggered. I don't think it is the simplified_null_return as in the docs it states that this rule:

A return statement wishing to return void should not return null.

The function in question isn't explicitly returning void so I'm not clear if this is linked. It definitely seems to be the most likely candidate from naming alone though.

If you are happy to, go ahead and remove this in your branch as it shouldn't cause any issues and we will the definitely know if it is this rule triggering the conflict. Cheers

[filecage]

@Sephster StyleCI seems to focus on return statements and is not aware of the function or method context. Removing the option made the check pass.

If there is nothing else left that you would like to change, feel free to merge.

[Sephster]

Thanks @filecage - I've made some minor adjustments to the changelog and formatting but otherwise looks good. I think long term this will be a stop gap and likely reverted when version 8 comes along as I'd like to eventually implement Alex's proposed plan of just not passing the refresh repo to grants if you don't want to issue a refresh token.

At the moment though, this is a good solution for people that need to have optional refresh tokens now.

[filecage]

@Sephster thank you for your feedback. I also think that the proposal of not passing a refresh token repository is a cleaner way of implementing this but in terms of BC this one is the safer solution. So it makes absolute sense to me to revert this in a 8.x release.

[Sephster]

No worries. Thanks for your contribution and patience while I reviewed this.

[ambrous]

Hi there!

If I understand right Optional Refresh Token is only when you try to get Access Token second and next times. How it is noted on the scheme here https://tools.ietf.org/html/rfc6749#page-11 you obtain Refresh Token always when you do the first request to get Access Token, however, then you are able to get a new Refresh Token or not.

Moreover, I have tested the version (7.4.0) with that fix and it looks like it does not work with https://github.com/thephpleague/oauth2-client (2.4.1) . I get an error of Required parameter not passed: "refresh_token"

[ambrous]

I think the best way to do support of Optional Refresh Token is to change src/Grant/RefreshTokenGrant.php so that we are able to send a flag to the constructor to have capacity to set that behavior up and change method respondToAccessTokenRequest so that we are able to return old refresh token instead of issue of a new one. However, in that case we have to change Access Token of the old our Refresh Token, and because of this we will have orphan access tokens.

But, to do thiswe have to add getByIdentifier method into RefreshTokenRepository including adding that method into the RefreshTokenRepositoryInterface what breaks backward compatibility.