Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refresh token in oauth2 auth_code grant #48

Open
hudecsamuel opened this issue Jul 11, 2017 · 4 comments
Open

Refresh token in oauth2 auth_code grant #48

hudecsamuel opened this issue Jul 11, 2017 · 4 comments
Labels

Comments

@hudecsamuel
Copy link

hudecsamuel commented Jul 11, 2017

Current situation:

How we authorize with auth_code grant:

authorization

Finding out our access_token expired on form submit.

401_current

Problem:

The situation described is quite common: User works in app for long period of time and his access_token expired during that and we need to make full auth_code redirect flow. This is not that much of a problem if he is just reads static pages that he get with GET method, but is quite disruptive during POST/PATCH request such as form submission.

Solution

In oauth2 specification there is also refresh_token in auth_code grant type. Although it is optional it's quite common to see it with widespread oauth2 providers and would be perfect fit for our problem.

"refresh_token" - A token you can use to get a new access_token without requiring user interaction.

Auth flow with refresh token:

auth_mergado_refresh

Refreshing access_token on form submission (example):

refresh_proposed

I tried to figure out other possible solutions to this kind of problem, like storing apps state in localStorage with some indicator of action the user intended to take, so when it could be repeated after we have obtained new token, but this is very unpractical (and might have a few catches also). This is also linked to issue #47 (problem with localStorage in Mergado apps).

Conclusion:

If you would implement refresh_token into auth_code oauth2 grant, that would be great.

@hudecsamuel
Copy link
Author

I would add that we can still do something like this with offline_token, but I would consider that quite bad practice.

@hudecsamuel
Copy link
Author

Also curently refresh_token grant in Mergado is implemented as something described as "Offline Mode" . AFAK this is not really what oauth2 refresh_token was intended for and should have been some custom grant such as offline_token from the begining.

@smuuf
Copy link
Member

smuuf commented Jul 27, 2017

Agreed with the last comment (and pretty much agree also with the proposal). Again, if we were to do this, I can't currently come up with a pretty solution that wouldn't be a huge B/C break.

Pretty solution, IMO, would be to rename the current offline mode refresh_token to be some sort of offline mode offline_token and at the same time implement a classic, online mode called refresh_token to act as a proper refresh token for online access tokens.

Which would be a B/C break, because apps would expect the current implementation under refresh_token grant.

How should we go about this, if we were to "fix" this? On the other hand, this (implementing a proper refresh_token) is not really something "The World of Apps" depends on, is it?

@hudecsamuel
Copy link
Author

I'm aware that this has no "pretty" solution without breaking B/C, but I thought this is something we ought to talk about.

My suggestion:

  1. Announcement that current refresh_token grant is deprecated at the same time releasing alternative offline_token grant. It would take some time for developers to implement the change, although it should be trivial if offline_token grant works as current refresh_token.
  2. Get in touch with developers (there is not that many of them) and tell them of the changes, guide them though the process of changing it if necessary. It might be a good idea to monitor the usage of refresh_token grant on your side.
  3. After some period of time I would replace the old (deprecated) refresh_token grant with this proposed refresh+access token grant. Would also note that before making the change I would double check with developers (email, check the usage of refresh_token grant in last few days)

Speaking for Reglendo: We use mergado/mergado-api-client-php and it already works with "offline_token" grant the same as with "refresh_token" (it just rewrites it to "refresh_token")

$offlineToken = $this->provider->getAccessToken("refresh_token", [ //works with "offline_token"
    "entity_id" => $entityId
]);

So on our side it changing just the name of that grant would be as simple as changing it along with some minor tweaks in MergadoProvider in mergado/mergado-api-client-php and running composer update in our apps

Implementing the new refresh_token would be something else, but that is not realy as much of a problem as moving from the old one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants