Skip to content

Refresh Auth token if expired during getId call. #809

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.

Sign up for GitHub

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

Closed
wants to merge 14 commits into from
Closed

Refresh Auth token if expired during getId call. #809

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
18cc029
Refresh Auth token if expired during getId call.
ankitaj224 Sep 17, 2019
c0580b1
Addressing Di's comments.
ankitaj224 Sep 17, 2019
8558f73
Refractoring the code to replace small methods with inline code.
ankitaj224 Sep 17, 2019
0fb2666
Addressing Ciaran's comments and refractored the code for better
ankitaj224 Sep 18, 2019
b727b35
Merge branch 'fis_sdk' of github.com:firebase/firebase-android-sdk in…
ankitaj224 Sep 18, 2019
60f4023
Adding isFidRegistered method.
ankitaj224 Sep 18, 2019
63da567
Fixing api-information check.
ankitaj224 Sep 18, 2019
c26adc7
Restructuring the synchronized block of registerFidIfNeccesary
ankitaj224 Sep 18, 2019
4afc7e5
Addressed ciaran'c comments.
ankitaj224 Sep 18, 2019
86e3728
Added helper methods for registration status.
ankitaj224 Sep 18, 2019
4f64c09
Merge branch 'fis_sdk' of github.com:firebase/firebase-android-sdk in…
ankitaj224 Sep 18, 2019
62098a7
Introducing new registration status - NOT_GENERATED as default to avo…
ankitaj224 Sep 18, 2019
f7457d3
Cleaning up the code for redudant checks.
ankitaj224 Sep 19, 2019
fe2577c
Merge branch 'fis_sdk' of github.com:firebase/firebase-android-sdk in…
ankitaj224 Sep 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 34 additions & 6 deletions ...oidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static com.google.firebase.installations.FisAndroidTestConstants.TEST_AUTH_TOKEN_4;
import static com.google.firebase.installations.FisAndroidTestConstants.TEST_CREATION_TIMESTAMP_1;
import static com.google.firebase.installations.FisAndroidTestConstants.TEST_CREATION_TIMESTAMP_2;
import static com.google.firebase.installations.FisAndroidTestConstants.TEST_CREATION_TIMESTAMP_3;
import static com.google.firebase.installations.FisAndroidTestConstants.TEST_FID_1;
import static com.google.firebase.installations.FisAndroidTestConstants.TEST_PROJECT_ID;
import static com.google.firebase.installations.FisAndroidTestConstants.TEST_REFRESH_TOKEN;
Expand All @@ -32,6 +33,7 @@
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -116,7 +118,7 @@ public class FirebaseInstallationsInstrumentedTest {
.setFirebaseInstallationId(TEST_FID_1)
.setAuthToken(TEST_AUTH_TOKEN_2)
.setRefreshToken(TEST_REFRESH_TOKEN)
.setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_2)
.setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_3)
.setExpiresInSecs(TEST_TOKEN_EXPIRATION_TIMESTAMP)
.setRegistrationStatus(PersistedFid.RegistrationStatus.REGISTERED)
.build();
Expand Down Expand Up @@ -277,6 +279,36 @@ public void testGetId_PersistedFidError_BackendOk() throws InterruptedException
}
}

@Test
public void testGetId_expiredAuthToken_refreshesAuthToken() throws Exception {
// Update local storage with fid entry that has auth token expired.
persistedFid.insertOrUpdatePersistedFidEntry(EXPIRED_AUTH_TOKEN_ENTRY);
FirebaseInstallations firebaseInstallations =
new FirebaseInstallations(
mockClock, executor, firebaseApp, backendClientReturnsOk, persistedFid, mockUtils);

assertWithMessage("getId Task fails.")
.that(Tasks.await(firebaseInstallations.getId()))
.isNotEmpty();
PersistedFidEntry entryValue = persistedFid.readPersistedFidEntryValue();
assertWithMessage("Persisted Fid doesn't match")
.that(entryValue.getFirebaseInstallationId())
.isEqualTo(TEST_FID_1);

// Waiting for Task that generates auth token with the FIS Servers
executor.awaitTermination(500, TimeUnit.MILLISECONDS);

// Validate that registration is complete with updated auth token
PersistedFidEntry updatedFidEntry = persistedFid.readPersistedFidEntryValue();
assertWithMessage("Persisted Fid doesn't match")
.that(updatedFidEntry)
.isEqualTo(UPDATED_AUTH_TOKEN_FID_ENTRY);
verify(backendClientReturnsOk, never())
.createFirebaseInstallation(TEST_API_KEY, TEST_FID_1, TEST_PROJECT_ID, TEST_APP_ID_1);
verify(backendClientReturnsOk, times(1))
.generateAuthToken(TEST_API_KEY, TEST_FID_1, TEST_PROJECT_ID, TEST_REFRESH_TOKEN);
}

@Test
public void testGetAuthToken_fidDoesNotExist_successful() throws Exception {
FirebaseInstallations firebaseInstallations =
Expand Down Expand Up @@ -409,11 +441,7 @@ public void testGetAuthToken_multipleCallsDoNotForceRefresh_fetchedNewTokenOnce(
// triggered simultaneously. Task2 waits for Task1 to complete. On Task1 completion, task2 reads
// the UPDATED_AUTH_TOKEN_FID_ENTRY by Task1 on execution.
when(mockPersistedFid.readPersistedFidEntryValue())
.thenReturn(
EXPIRED_AUTH_TOKEN_ENTRY,
EXPIRED_AUTH_TOKEN_ENTRY,
EXPIRED_AUTH_TOKEN_ENTRY,
UPDATED_AUTH_TOKEN_FID_ENTRY);
.thenReturn(EXPIRED_AUTH_TOKEN_ENTRY, UPDATED_AUTH_TOKEN_FID_ENTRY);
FirebaseInstallations firebaseInstallations =
new FirebaseInstallations(
mockClock, executor, firebaseApp, backendClientReturnsOk, mockPersistedFid, mockUtils);
Expand Down
1 change: 1 addition & 0 deletions ...tions/src/androidTest/java/com/google/firebase/installations/FisAndroidTestConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,5 @@ public final class FisAndroidTestConstants {

public static final long TEST_CREATION_TIMESTAMP_1 = 2000L;
public static final long TEST_CREATION_TIMESTAMP_2 = 2000L;
public static final long TEST_CREATION_TIMESTAMP_3 = 2L;
}
57 changes: 43 additions & 14 deletions ...-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,31 +235,45 @@ private void persistFid(String fid) throws FirebaseInstallationsException {
}

/**
* Registers the FID with FIS servers if FID is in UNREGISTERED state.
* Registers the FID with FIS servers if FID is in UNREGISTERED state. Also, refreshes auth token
* if expired.
*
* <p>Updates FID registration status to PENDING to avoid multiple network calls to FIS Servers.
*/
private Task<String> registerFidIfNecessary(
PersistedFidEntry persistedFidEntry, AwaitListener listener) {
String fid = persistedFidEntry.getFirebaseInstallationId();

// Check if the fid is unregistered
if (persistedFidEntry.getRegistrationStatus() == RegistrationStatus.UNREGISTERED) {
updatePersistedFidWithPendingStatus(fid);
executeFidRegistration(persistedFidEntry, listener);
} else {
updateAwaitListenerIfRegisteredFid(persistedFidEntry, listener);
if (!isFidUnregisteredOrAuthTokenExpired(persistedFidEntry)) {
return updateAwaitListenerIfRegisteredFid(persistedFidEntry, listener);
}

updatePersistedFidWithPendingStatus(persistedFidEntry);
executeFISCalls(persistedFidEntry, listener);
return Tasks.forResult(fid);
}

private void updateAwaitListenerIfRegisteredFid(
private boolean isFidUnregisteredOrAuthTokenExpired(PersistedFidEntry persistedFidEntry) {
return persistedFidEntry.getRegistrationStatus() == RegistrationStatus.UNREGISTERED
|| isAuthTokenExpired(persistedFidEntry);
}

private void executeFISCalls(PersistedFidEntry persistedFidEntry, AwaitListener listener) {

if (!isAuthTokenExpired(persistedFidEntry)) {
executeFidRegistration(persistedFidEntry, listener);
return;
}
refreshAuthToken(listener);
}

private Task<String> updateAwaitListenerIfRegisteredFid(
PersistedFidEntry persistedFidEntry, AwaitListener listener) {
if (listener != null
&& persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTERED) {
listener.onSuccess();
}
return Tasks.forResult(persistedFidEntry.getFirebaseInstallationId());
}

/**
Expand All @@ -273,12 +287,20 @@ private void executeFidRegistration(PersistedFidEntry persistedFidEntry, AwaitLi
}
}

private void updatePersistedFidWithPendingStatus(String fid) {
/**
* Refreshes FID auth token with FIS servers in a background thread and updates the listener on
* completion.
*/
private void refreshAuthToken(AwaitListener listener) {
Task<InstallationTokenResult> task = Tasks.call(executor, () -> refreshAuthTokenIfNecessary(1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never pass the raw value for an @intdef constant; treat it like an enum and always use the predefined constants

(Gradle has a check for this afair, but I don't know if this repo is configured to run them)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much. Updated. Any examples on how do I configure it?

if (listener != null) {
task.addOnCompleteListener((unused) -> listener.onSuccess());
}
}

private void updatePersistedFidWithPendingStatus(PersistedFidEntry persistedFidEntry) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A general comment : I feel like you are doing functionalization too much in this class.

Functionalization is useful when you want to hide a chunk of detailed logic, or to avoid duplicated code. But for example for updatePersistedFidWithPendingStatus, this function is only used in one place(doesn't avoid duplicated code), and it's not hiding any detailed logic (use one line to replace another line)

A downside of over functionalization is, with too many small method, it's really hard to find the relationship between them.

Above all, for these cases, I prefer making the code inline than making as a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed explanation. Updated the code. PTAL.

persistedFid.insertOrUpdatePersistedFidEntry(
PersistedFidEntry.builder()
.setFirebaseInstallationId(fid)
.setRegistrationStatus(RegistrationStatus.PENDING)
.build());
persistedFidEntry.toBuilder().setRegistrationStatus(RegistrationStatus.PENDING).build());
}

/** Registers the created Fid with FIS servers and update the shared prefs. */
Expand Down Expand Up @@ -355,7 +377,8 @@ private InstallationTokenResult getValidAuthToken(PersistedFidEntry persistedFid

private boolean isPersistedFidRegistered(PersistedFidEntry persistedFidEntry) {
return persistedFidEntry != null
&& persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTERED;
&& (persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTERED
|| persistedFidEntry.getRegistrationStatus() == RegistrationStatus.PENDING);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed now, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. We need this check. With this new flow - both getId and getAuthToken need this.

getAuthToken uses it with persistedFid in REGISTERED state.
getId uses it with persistedFid in PENDING state.

}

/** Calls the FIS servers to generate an auth token for this Firebase installation. */
Expand All @@ -382,6 +405,12 @@ private InstallationTokenResult fetchAuthTokenFromServer(PersistedFidEntry persi

return tokenResult;
} catch (FirebaseInstallationServiceException exception) {
persistedFid.insertOrUpdatePersistedFidEntry(
persistedFidEntry
.toBuilder()
.setRegistrationStatus(RegistrationStatus.REGISTER_ERROR)
.build());

throw new FirebaseInstallationsException(
"Failed to generate auth token for a Firebase Installation.",
FirebaseInstallationsException.Status.SDK_INTERNAL_ERROR);
Expand Down