Implementing breaking API changes for FirebaseCredential API #21
Conversation
…ng a GoogleOAuthAccessToken instance which contains expirty info in addition to the token string. Implementing all token caching at the FirebaseApp level.
| /** | ||
| * Returns a Google OAuth2 access token used to authenticate with Firebase services. | ||
| * Returns a Google OAuth2 access token which can be used to authenticate with Firebase services. | ||
| * This method does not cache tokens, and therefore each invocation may trigger a token |
There was a problem hiding this comment.
Perhaps "may trigger a token refresh" => "will fetch a fresh token."
| || !previousTask.isSuccessful() | ||
| || previousTask.getResult().isExpired())); | ||
| } | ||
| abstract GoogleOAuthAccessToken fetchToken(GoogleCredential credential) throws IOException ; |
| * @param expiryTime Time at which the token will expire (milliseconds since epoch) | ||
| * @throws IllegalArgumentException If the token is null or empty | ||
| */ | ||
| public GoogleOAuthAccessToken(String accessToken, long expiryTime) { |
There was a problem hiding this comment.
Since FirebaseCredential interface is public we have to make this constructor public too, so that a custom implementation of the interface can create and return GoogleOauthAccessTokens.
There was a problem hiding this comment.
Ahh, okay. I wasn't sure if this was meant for external creation or not. It probably should have technically been in the API proposal, but I think it's probably okay. :-)
| /** | ||
| * Returns true if the token is already expired, and false otherwise. | ||
| */ | ||
| public boolean isExpired() { |
There was a problem hiding this comment.
We call it from FirebaseApp which is why I made it public. But on the other hand we didn't really have this method in the API proposal. If that's an issue I can just move the method to FirebaseApp. WDYT?
There was a problem hiding this comment.
Given it wasn't in the proposal and it's just a helper, I would move it to FirebaseApp and avoid adding anything unnecessary to our public surface area .
| Tasks.await(credential.getAccessToken(false)); | ||
| Tasks.await(credential.getAccessToken(true)); | ||
| Tasks.await(credential.getAccessToken()); | ||
| Tasks.await(credential.getAccessToken()); |
There was a problem hiding this comment.
Not sure there's any point in doing this twice...
| Tasks.await(credential.getAccessToken(false)); | ||
| Tasks.await(credential.getAccessToken(true)); | ||
| Tasks.await(credential.getAccessToken()); | ||
| Tasks.await(credential.getAccessToken()); |
| Addressed all suggestions made. Just one follow up question. |
| Answered. :) |
2 participants
Returning a GoogleOAuthAccessToken instance which contains expirty info in addition to the token string. Implementing all token caching at the FirebaseApp level.