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 7 commits
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
1 change: 1 addition & 0 deletions firebase-installations/api.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ package com.google.firebase.installations.local {
ctor public PersistedFid(@NonNull FirebaseApp);
method @NonNull public boolean clear();
method @NonNull public boolean insertOrUpdatePersistedFidEntry(@NonNull com.google.firebase.installations.local.PersistedFidEntry);
method @NonNull public boolean isFidRegistered(@Nullable com.google.firebase.installations.local.PersistedFidEntry);
method @Nullable public com.google.firebase.installations.local.PersistedFidEntry readPersistedFidEntryValue();
}

Expand Down
55 changes: 40 additions & 15 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 @@ -89,7 +89,7 @@ public class FirebaseInstallationsInstrumentedTest {
.setFirebaseInstallationId(TEST_FID_1)
.setAuthToken(TEST_AUTH_TOKEN)
.setRefreshToken(TEST_REFRESH_TOKEN)
.setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_2)
.setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_1)
.setExpiresInSecs(TEST_TOKEN_EXPIRATION_TIMESTAMP)
.setRegistrationStatus(PersistedFid.RegistrationStatus.REGISTERED)
.build();
Expand All @@ -99,7 +99,7 @@ public class FirebaseInstallationsInstrumentedTest {
.setFirebaseInstallationId(TEST_FID_1)
.setAuthToken(TEST_AUTH_TOKEN)
.setRefreshToken(TEST_REFRESH_TOKEN)
.setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_2)
.setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_1)
.setExpiresInSecs(TEST_TOKEN_EXPIRATION_TIMESTAMP_2)
.setRegistrationStatus(PersistedFid.RegistrationStatus.REGISTERED)
.build();
Expand All @@ -109,7 +109,7 @@ public class FirebaseInstallationsInstrumentedTest {
.setFirebaseInstallationId(TEST_FID_1)
.setAuthToken("")
.setRefreshToken("")
.setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_2)
.setTokenCreationEpochInSecs(TEST_CREATION_TIMESTAMP_1)
.setExpiresInSecs(0)
.setRegistrationStatus(PersistedFid.RegistrationStatus.UNREGISTERED)
.build();
Expand Down Expand Up @@ -190,7 +190,7 @@ public void testGetId_PersistedFidOk_BackendOk() throws Exception {
mockClock, executor, firebaseApp, backendClientReturnsOk, persistedFid, mockUtils);

// No exception, means success.
assertWithMessage("getId Task fails.")
assertWithMessage("getId Task failed")
.that(Tasks.await(firebaseInstallations.getId()))
.isNotEmpty();
PersistedFidEntry entryValue = persistedFid.readPersistedFidEntryValue();
Expand All @@ -217,7 +217,7 @@ public void testGetId_multipleCalls_sameFIDReturned() throws Exception {
mockClock, executor, firebaseApp, backendClientReturnsOk, persistedFid, mockUtils);

// No exception, means success.
assertWithMessage("getId Task fails.")
assertWithMessage("getId Task failed")
.that(Tasks.await(firebaseInstallations.getId()))
.isNotEmpty();
PersistedFidEntry entryValue = persistedFid.readPersistedFidEntryValue();
Expand Down Expand Up @@ -290,6 +290,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 failed")
.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 @@ -418,18 +448,13 @@ public void testGetAuthToken_serverError_failure() throws Exception {
@Test
public void testGetAuthToken_multipleCallsDoNotForceRefresh_fetchedNewTokenOnce()
throws Exception {
// Using mockPersistedFid to ensure the order of returning persistedFidEntry to 2 tasks
// 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);
// Update local storage with fid entry that has auth token expired. When 2 tasks are triggered
// simultaneously, Task2 waits for Task1 to complete. On Task1 completion, task2 reads the
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "reads the" has too many (two) spaces

// UPDATED_AUTH_TOKEN_FID_ENTRY by Task1 on execution.
persistedFid.insertOrUpdatePersistedFidEntry(EXPIRED_AUTH_TOKEN_ENTRY);
FirebaseInstallations firebaseInstallations =
new FirebaseInstallations(
mockClock, executor, firebaseApp, backendClientReturnsOk, mockPersistedFid, mockUtils);
mockClock, executor, firebaseApp, backendClientReturnsOk, persistedFid, mockUtils);

// Call getAuthToken multiple times with DO_NOT_FORCE_REFRESH option
Task<InstallationTokenResult> task1 =
Expand Down
2 changes: 1 addition & 1 deletion ...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 @@ -35,5 +35,5 @@ public final class FisAndroidTestConstants {
public static final long TEST_TOKEN_EXPIRATION_TIMESTAMP_2 = 2000L;

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_2 = 2L;
}
114 changes: 62 additions & 52 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 @@ -123,8 +123,7 @@ public Task<String> getId() {
private Task<String> getId(AwaitListener awaitListener) {
return Tasks.call(executor, this::getPersistedFid)
.continueWith(orElse(this::createAndPersistNewFid))
.onSuccessTask(
persistedFidEntry -> registerFidIfNecessary(persistedFidEntry, awaitListener));
.onSuccessTask(unused -> registerFidIfNecessary(awaitListener));
Copy link
Contributor

Choose a reason for hiding this comment

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

both this and the continueWith need an executor or they'll run on the main thread

}

/**
Expand Down Expand Up @@ -179,18 +178,14 @@ String getName() {
*/
private PersistedFidEntry getPersistedFid() throws FirebaseInstallationsException {
PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue();
if (persistedFidMissingOrInErrorState(persistedFidEntry)) {
if (persistedFidEntry == null
|| persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTER_ERROR) {
throw new FirebaseInstallationsException(
"Failed to get existing fid.", FirebaseInstallationsException.Status.CLIENT_ERROR);
}
return persistedFidEntry;
}

private static boolean persistedFidMissingOrInErrorState(PersistedFidEntry persistedFidEntry) {
return persistedFidEntry == null
|| persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTER_ERROR;
}

@NonNull
private static <F, T> Continuation<F, T> orElse(@NonNull Supplier<T> supplier) {
return t -> {
Expand Down Expand Up @@ -235,50 +230,62 @@ 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);
}
private Task<String> registerFidIfNecessary(AwaitListener listener) {
synchronized (persistedFid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if you're going to start synchronizing these instead of piping the values through a Task chain then you should add @GuardedBy annotations http://errorprone.info/bugpattern/GuardedBy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I am locking on persistedFid and not persistedFidEntry( was piping it earlier on this value through task).

Also, I cannot synchronize on persistedFid throughout this class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, my point was that synchronization isn't necessarily required if you're just piping them through a Task call stack.

Why are we synchronizing on it here if we can't do it consistently? What benefit does it provide here that it doesn't provide elsewhere? Can we document that more explicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PENDING state was introduced to avoid multiple network calls for getId(). Registration status changes from UNREGISTERED to PENDING before triggering the network call.

Need for synchronization: Reading the existing registration status and updating it to PENDING should be atomic. That's the block i tried to synchronize.

I think piping wont solve the atomicity issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

ugh, this is too complicated

I see two similar potential issues here:

  1. synchronizing this PENDING write doesn't do any good if the reads are happening in an unsynchronized block on a different thread because there's no guarantee that the readers will see writes from a separate thread go/ej3e-78

    If we can show that everything that touches the PersistedFid class is running in the same thread (not currently the case) then I can see why we wouldn't need synchronization on this class, but as it is this looks really dangerous to me

  2. the PersistedFid class is vulnerable to the same issues because the read method doesn't hold a lock on the shared prefs so there's no guarantee that a write isn't going to get applied halfway through reading the old values, leaving you with a super corrupt / nonsensical PersistedFidEntry. SharedPrefs reads are atomic with writes, but we're reading like 6 fields so there's nothing stopping us reading the first 3 fields and then a write changing it underneath us

(also sidenote: given that getAuthToken is jumping straight to a worker thread it seems weird to have it synchronized, esp. since it's the only block synchronizing on this)

PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue();
String fid = persistedFidEntry.getFirebaseInstallationId();

// If FID registration status is PENDING, return the persisted fid. This avoids multiple
// network calls to FIS Servers.
if (persistedFidEntry.getRegistrationStatus() == RegistrationStatus.PENDING) {
return Tasks.forResult(fid);
}

return Tasks.forResult(fid);
}
// If FID registration status is REGISTERED and auth token is valid, return the
// persisted fid.
if (persistedFid.isFidRegistered(persistedFidEntry)
&& !isAuthTokenExpired(persistedFidEntry)) {

private void updateAwaitListenerIfRegisteredFid(
PersistedFidEntry persistedFidEntry, AwaitListener listener) {
if (listener != null
&& persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTERED) {
listener.onSuccess();
}
}
// Update the listener if awaiting
if (listener != null) {
listener.onSuccess();
}
return Tasks.forResult(fid);
}

/**
* Registers the FID with FIS servers in a background thread and updates the listener on
* completion.
*/
private void executeFidRegistration(PersistedFidEntry persistedFidEntry, AwaitListener listener) {
Task<Void> task = Tasks.call(executor, () -> registerAndSaveFid(persistedFidEntry));
if (listener != null) {
task.addOnCompleteListener(listener);
}
}
// Update FID registration status to PENDING to avoid multiple network calls to FIS Servers.
persistedFid.insertOrUpdatePersistedFidEntry(
persistedFidEntry.toBuilder().setRegistrationStatus(RegistrationStatus.PENDING).build());

// If fid registration for this firebase installation is incomplete without an expired auth
// token, execute Fid registration.
if (!isAuthTokenExpired(persistedFidEntry)) {
Task<Void> fidRegistrationTask =
Tasks.call(executor, () -> registerAndSaveFid(persistedFidEntry));

// Update the listener if awaiting
if (listener != null) {
fidRegistrationTask.addOnCompleteListener(listener);
}
return Tasks.forResult(fid);
}

// If auth token is expired, refresh FID auth token with FIS servers in a background thread
// and updates the listener on completion
Task<InstallationTokenResult> refreshAuthTokenTask =
Tasks.call(executor, () -> refreshAuthTokenIfNecessary(FORCE_REFRESH));

private void updatePersistedFidWithPendingStatus(String fid) {
persistedFid.insertOrUpdatePersistedFidEntry(
PersistedFidEntry.builder()
.setFirebaseInstallationId(fid)
.setRegistrationStatus(RegistrationStatus.PENDING)
.build());
// Update the listener if awaiting
if (listener != null) {
refreshAuthTokenTask.addOnCompleteListener((unused) -> listener.onSuccess());
}
return Tasks.forResult(fid);
}
}

/** Registers the created Fid with FIS servers and update the shared prefs. */
Expand Down Expand Up @@ -320,7 +327,9 @@ private InstallationTokenResult refreshAuthTokenIfNecessary(int authTokenOption)

PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue();

if (!isPersistedFidRegistered(persistedFidEntry)) {
if (persistedFidEntry == null
|| (persistedFidEntry.getRegistrationStatus() != RegistrationStatus.REGISTERED
&& persistedFidEntry.getRegistrationStatus() != RegistrationStatus.PENDING)) {
throw new FirebaseInstallationsException(
"Firebase Installation is not registered.",
FirebaseInstallationsException.Status.SDK_INTERNAL_ERROR);
Expand Down Expand Up @@ -353,11 +362,6 @@ private InstallationTokenResult getValidAuthToken(PersistedFidEntry persistedFid
.build();
}

private boolean isPersistedFidRegistered(PersistedFidEntry persistedFidEntry) {
return persistedFidEntry != null
&& persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTERED;
}

/** Calls the FIS servers to generate an auth token for this Firebase installation. */
private InstallationTokenResult fetchAuthTokenFromServer(PersistedFidEntry persistedFidEntry)
throws FirebaseInstallationsException {
Expand All @@ -382,6 +386,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 Expand Up @@ -409,7 +419,7 @@ private Void deleteFirebaseInstallationId() throws FirebaseInstallationsExceptio

PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue();

if (isPersistedFidRegistered(persistedFidEntry)) {
if (persistedFid.isFidRegistered(persistedFidEntry)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't right, is it? if persistedFidEntry is null then the isFidRegistered method will return true and then we'll call persistedFidEntry.getFirebaseInstallationId() right below which will NPE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if persistedFidEntry is null then the isFidRegistered method will return false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes this.

// Call the FIS servers to delete this firebase installation id.
try {
serviceClient.deleteFirebaseInstallation(
Expand Down
6 changes: 6 additions & 0 deletions ...ase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,4 +134,10 @@ public boolean clear() {
private String getSharedPreferencesKey(String key) {
return String.format("%s|%s", persistenceKey, key);
}

@NonNull
public boolean isFidRegistered(@Nullable 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.

imo this should go on PersistedFidEntry or some sort of static util class... Otherwise it feels like we're just putting it here for convenience

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. PTAL.

return persistedFidEntry != null
&& persistedFidEntry.getRegistrationStatus() == RegistrationStatus.REGISTERED;
}
}