Refresh the OAuth token when it expires #165
Conversation
|
Hold off merging. I see some issues after the rebase to use moshi. |
…esh token when using static auth
|
Works like a dream now. |
|
i think you can update to gradle 2.14.1 as well |
| distributionBase=GRADLE_USER_HOME | ||
| distributionPath=wrapper/dists | ||
| zipStoreBase=GRADLE_USER_HOME | ||
| zipStorePath=wrapper/dists | ||
| distributionUrl=https\://services.gradle.org/distributions/gradle-2.13-bin.zip | ||
| distributionUrl=https\://services.gradle.org/distributions/gradle-2.13-all.zip |
There was a problem hiding this comment.
Why? That downloads sources and docs for Gradle that no one in our case really needs.
There was a problem hiding this comment.
IDE code completion for gradle does not work without that.
There was a problem hiding this comment.
Well yeah, but do we need that?
There was a problem hiding this comment.
I dont see why not. This is not bundled anyway. And its a one time download of a few megs more.
|
whats about a okhttp request interceptor to update the token when expired? |
| @@ -166,7 +166,7 @@ public AuthInfo login(String username, String password) throws LoginFailedExcept | |||
| .addQueryParameter("client_id", CLIENT_ID) | |||
| .addQueryParameter("redirect_uri", REDIRECT_URI) | |||
| .addQueryParameter("client_secret", CLIENT_SECRET) | |||
| .addQueryParameter("grant_type", "refresh_token") | |||
| .addQueryParameter("grant_type", "refreshToken") | |||
There was a problem hiding this comment.
Are you sure this change is required?
There was a problem hiding this comment.
the official client is using refresh_token so will should keep it .addQueryParameter("grant_type", "refresh_token")
|
@FabianTerhorst Dont see how we can do that because we never receive a Token Expired 401. We only receive a response from niantic in the RPC which fails. |
|
@rajulbhatnagar thats true right now this isn´t possible with the current implementation. When you revert the gradle wrapper change. this could be ready to merge. Im only unsure about the spacings at the imports. |
| @@ -129,8 +133,21 @@ public void sendServerRequests(ServerRequest... serverRequests) throws RemoteSer | |||
| lastAuth = responseEnvelop.getAuthTicket(); | |||
| } | |||
|
|
|||
| if (responseEnvelop.getStatusCode() == 102) { | |||
| throw new LoginFailedException(); | |||
| if (responseEnvelop.getStatusCode() == 102 && GoogleLoginSecrets.refresh_token != null) { | |||
There was a problem hiding this comment.
Maybe refactor the Login interface and implement this for Ptc as well, right now, by some bug in their checks of the token, those tokens are valid for a very long time, but I don't expect it to stay that way.
Then we can deprecate logging in with the token for Ptc as well and use some generic token refreshing code here (if refreshToken would be a part of the interface, the implementation for PtcLogin could currently be to return the old token).
There was a problem hiding this comment.
Ya the login code/logic could do with some refactoring but I dont think i'd be able to do it now. I really wanted to implement this feature as it makes google auth kinda useless without it.
My ideal API design would be to pass
an implementation of Login to PokemonGo and let the implementation handle tokens transparently.
RequestManager should just ask the PokemonGo object for AuthInfo when it wants to do a call.
|
@FabianTerhorst The reason I created a new file was to prevent people using the API to shoot themselves on the foot. If the static was inside the GoogleLogin class and someone holds a reference to it, it would cause the login object to leak and in mobile environment it could easily mean leaking a fragment or activity! Because I was separating this out it also made sense to move the constants to the file. We should definitely leave the static refreshToken in a separate file. Other constants can be moved back but I dont see the upside. Ideal handling of this will require quite a bit of code refactoring where the auth provider is passed to the api client instead of just the auth tokenid and the auth provider is then self managing but I think this works just as well. |
|
Have tested it and looks good to me. I think the refactoring should happen after the merge. 👍 |
Changes made: