Refresh Auth token if expired during getId call. #809

ankitaj224 · 2019-09-17T17:11:28Z

Addressing Rayo's comment on another PR which was marked as a TODO.

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

diwu-arete · 2019-09-17T20:40:58Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+    }
+  }
+
+  private void updatePersistedFidWithPendingStatus(PersistedFidEntry persistedFidEntry) {


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.

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

diwu-arete · 2019-09-17T20:41:12Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

@@ -355,7 +373,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);


This can be removed now, right?

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.

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

diwu-arete · 2019-09-17T21:36:46Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

    }

+    // If auth token is expired, refresh FID auth token with FIS servers in a background thread and
+    // updates the listener on


nit : in one line.

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

...llations/src/androidTest/java/com/google/firebase/installations/FisAndroidTestConstants.java

ciarand · 2019-09-17T21:20:35Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

-      executeFidRegistration(persistedFidEntry, listener);
-    } else {
-      updateAwaitListenerIfRegisteredFid(persistedFidEntry, listener);
+    if (persistedFidEntry.getRegistrationStatus() != RegistrationStatus.UNREGISTERED


Can you add a helper for this? PersistedFidEntry#isRegistered

Added, as this method could be reused in getId and delete APIs.

ciarand · 2019-09-17T21:24:36Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+   * completion.
+   */
+  private void refreshAuthToken(AwaitListener listener) {
+    Task<InstallationTokenResult> task = Tasks.call(executor, () -> refreshAuthTokenIfNecessary(1));


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)

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

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

diwu-arete · 2019-09-17T21:42:13Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+
+    // Update FID registration status to PENDING to avoid multiple network calls to FIS Servers.
+    persistedFid.insertOrUpdatePersistedFidEntry(
+        persistedFidEntry.toBuilder().setRegistrationStatus(RegistrationStatus.PENDING).build());


you need to keep "reading cache status" and "updating cache status" in one atomic section, to avoid duplicated network calls.

Updated. PTAL.

readability.

…to minor_updates

ankitaj224 · 2019-09-18T18:52:47Z

/test device-check-changed

ankitaj224 · 2019-09-18T18:53:03Z

/test device-check-changed

google-oss-bot · 2019-09-18T18:55:58Z

The public api surface has changed for the subproject firebase-installations:
error: Added method com.google.firebase.installations.local.PersistedFid.isFidRegistered(com.google.firebase.installations.local.PersistedFidEntry) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

ciarand

I am worried about our use of null values in this code. Given we already have a state machine sorta thing, could we eliminate the use of null by adding a NOT_EVEN_GENERATED_YET_LOL (w/e) state? That way everything could assume non-null and the state machine describes the whole state

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

ciarand · 2019-09-18T21:09:58Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

-      updateAwaitListenerIfRegisteredFid(persistedFidEntry, listener);
-    }
+  private Task<String> registerFidIfNecessary(AwaitListener listener) {
+    synchronized (persistedFid) {


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

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.

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?

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.

ugh, this is too complicated

I see two similar potential issues here:

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

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)

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

ciarand · 2019-09-18T21:13:39Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

@@ -409,7 +419,7 @@ private Void deleteFirebaseInstallationId() throws FirebaseInstallationsExceptio

    PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue();

-    if (isPersistedFidRegistered(persistedFidEntry)) {
+    if (persistedFid.isFidRegistered(persistedFidEntry)) {


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

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

Changes this.

ciarand · 2019-09-18T21:14:51Z

firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java

@@ -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) {


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

Updated. PTAL.

…to minor_updates

…id null checks.

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

ciarand · 2019-09-18T23:39:06Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

-      updateAwaitListenerIfRegisteredFid(persistedFidEntry, listener);
-    }
+  private Task<String> registerFidIfNecessary(AwaitListener listener) {
+    synchronized (persistedFid) {


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?

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

ciarand · 2019-09-18T23:54:35Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

-            .build());
+    // If fid registration for this firebase installation is incomplete without an expired auth
+    // token, execute Fid registration.
+    if (!isAuthTokenExpired(updatedPersistedFidEntry)) {


isn't this being checked twice in the same body?

Yes, it is. First time its checked to evaluate if we need network calls. Second check helps to decide what type of network call is required.

Network call should be made : If UNREGISTERED or AuthToken Expired
Before triggering the call, we update the status to PENDING.

So, we rely on AuthToken Expired check.

If AuthToken not Expired: REGISTER Fid
else: Refresh Auth Token

I'm having a really really hard time understanding the flow of this method. I don't know if it's just that I lack the domain knowledge or familiarity with the terms, but the multiple return points, double checking in and out of the synch block, and sometimes-async kickoffs are breaking my brain.

ciarand · 2019-09-18T23:55:51Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+
+    // Update the listener if awaiting
+    if (listener != null) {
+      refreshAuthTokenTask.addOnCompleteListener((unused) -> listener.onSuccess());


you're turning an onComplete into an onSuccess, this probably needs an explanation

changed it to onSuccess. This is to update the listener asap on success instead of making it wait for 10s.

…to minor_updates

ciarand · 2019-09-19T18:50:08Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

@@ -123,8 +123,7 @@ public static FirebaseInstallations getInstance(@NonNull FirebaseApp app) {
  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));


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

ciarand · 2019-09-19T18:52:17Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+
+    // Update the listener if awaiting
+    if (listener != null) {
+      refreshAuthTokenTask.addOnSuccessListener((unused) -> listener.onSuccess());


needs an executor or a comment explaining why it's chill that we're calling this on the main thread

ciarand · 2019-09-19T18:53:10Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+
+      // Update the listener if awaiting
+      if (listener != null) {
+        fidRegistrationTask.addOnCompleteListener(listener);


needs an executor or a comment explaining why it's ok to call this on the main thread

ciarand · 2019-09-19T19:08:56Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

-      updateAwaitListenerIfRegisteredFid(persistedFidEntry, listener);
-    }
+  private Task<String> registerFidIfNecessary(AwaitListener listener) {
+    synchronized (persistedFid) {


ugh, this is too complicated

I see two similar potential issues here:

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

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)

ciarand · 2019-09-20T20:39:32Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

-            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


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

ciarand · 2019-09-20T20:40:15Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

@@ -508,7 +535,9 @@ public void testDelete_registeredFID_successful() throws Exception {
    Tasks.await(firebaseInstallations.delete());

    PersistedFidEntry entryValue = persistedFid.readPersistedFidEntryValue();
-    assertWithMessage("Persisted Fid Entry is not null.").that(entryValue).isNull();
+    assertWithMessage("Persisted Fid Entry is not null.")


the "is not null" message isn't correct any more

ciarand · 2019-09-20T20:41:55Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

-            .build());
+    // If fid registration for this firebase installation is incomplete without an expired auth
+    // token, execute Fid registration.
+    if (!isAuthTokenExpired(updatedPersistedFidEntry)) {


I'm having a really really hard time understanding the flow of this method. I don't know if it's just that I lack the domain knowledge or familiarity with the terms, but the multiple return points, double checking in and out of the synch block, and sometimes-async kickoffs are breaking my brain.

andirayo

Removing this from my list of review-requests.

Refresh Auth token if expired during getId call.

18cc029

ankitaj224 requested review from ciarand, andirayo and diwu-arete September 17, 2019 17:11

googlebot added the cla: yes Override cla label Sep 17, 2019

google-oss-bot added the size/M label Sep 17, 2019

diwu-arete reviewed Sep 17, 2019

View reviewed changes

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java Outdated Show resolved Hide resolved

Addressing Di's comments.

c0580b1

ankitaj224 requested a review from diwu-arete September 17, 2019 20:37

diwu-arete reviewed Sep 17, 2019

View reviewed changes

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java Outdated Show resolved Hide resolved

diwu-arete reviewed Sep 17, 2019

View reviewed changes

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java Outdated Show resolved Hide resolved

diwu-arete reviewed Sep 17, 2019

View reviewed changes

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java Outdated Show resolved Hide resolved

diwu-arete reviewed Sep 17, 2019

View reviewed changes

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java Show resolved Hide resolved

diwu-arete self-requested a review September 17, 2019 20:55

Refractoring the code to replace small methods with inline code.

8558f73

diwu-arete reviewed Sep 17, 2019

View reviewed changes

ciarand suggested changes Sep 17, 2019

View reviewed changes

diwu-arete reviewed Sep 17, 2019

View reviewed changes

Addressing Ciaran's comments and refractored the code for better

0fb2666

readability.

google-oss-bot added size/L and removed size/M labels Sep 18, 2019

ankitaj224 requested review from ciarand and diwu-arete September 18, 2019 18:08

ankitaj224 added 2 commits September 18, 2019 11:45

Merge branch 'fis_sdk' of github.com:firebase/firebase-android-sdk in…

b727b35

…to minor_updates

Adding isFidRegistered method.

60f4023

Fixing api-information check.

63da567

ciarand suggested changes Sep 18, 2019

View reviewed changes

ankitaj224 added 2 commits September 18, 2019 14:37

Restructuring the synchronized block of registerFidIfNeccesary

c26adc7

Addressed ciaran'c comments.

4afc7e5

ankitaj224 requested a review from ciarand September 18, 2019 22:09

ankitaj224 added 3 commits September 18, 2019 15:24

Added helper methods for registration status.

86e3728

Merge branch 'fis_sdk' of github.com:firebase/firebase-android-sdk in…

4f64c09

…to minor_updates

Introducing new registration status - NOT_GENERATED as default to avo…

62098a7

…id null checks.

diwu-arete reviewed Sep 18, 2019

View reviewed changes

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java Outdated Show resolved Hide resolved

diwu-arete approved these changes Sep 18, 2019

View reviewed changes

ciarand reviewed Sep 18, 2019

View reviewed changes

Cleaning up the code for redudant checks.

f7457d3

ankitaj224 requested a review from ciarand September 19, 2019 17:32

Merge branch 'fis_sdk' of github.com:firebase/firebase-android-sdk in…

fe2577c

…to minor_updates

ciarand suggested changes Sep 20, 2019

View reviewed changes

andirayo reviewed Mar 9, 2023

View reviewed changes

rlazo closed this Jan 30, 2024

firebase locked and limited conversation to collaborators Mar 1, 2024

Refresh Auth token if expired during getId call. #809

Refresh Auth token if expired during getId call. #809

Uh oh!

Conversation

ankitaj224 commented Sep 17, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankitaj224 commented Sep 18, 2019

/test device-check-changed

Uh oh!

ankitaj224 commented Sep 18, 2019

Uh oh!

google-oss-bot commented Sep 18, 2019

Uh oh!

ciarand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment