Reuse existing Iid as Fid #924

ankitaj224 · 2019-10-16T22:06:48Z

Reading iid from "com.google.android.gms.appid" shared pref

….appid" shared pref

…sdk into iid-migration

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

ciarand · 2019-10-16T22:32:00Z

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

+
+  public IidStore(@NonNull FirebaseApp firebaseApp) {
+    // Different FirebaseApp in the same Android application should have the same application
+    // context and same dir path


this comment is true, but it's not obvious why it's here? are we doing something differently because of this fact?

Updated. PTAL.

ciarand · 2019-10-16T22:33:08Z

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

+public class IidStore {
+  private static final String IID_SHARED_PREFS_NAME = "com.google.android.gms.appid";
+  private static final String STORE_KEY_PUB = "|S||P|";
+  private static final String STORE_KEY_ID = "|S|id";


both of these are missing the subtype part of the key format so they'll lose any subtype'd IDs

Can you please provide some context on subtype.

I saw that SUBTYPE_DEFAULT = ""; in
FirebaseInstanceId.java and this value is used for getting id from store.

I don't honestly know much about this topic. Rayo will have more info on subtypes and whether they're important, I just want to know that we're explicitly ok with dropping non-default subtypes

@andirayo Can you please take a look at this.

Thanks for pointing it out @ciarand

Background
The Instance-ID Service revolves around Registrations (identified by IID-Tokens). Registrations are authorizations, mainly authorizations to Cloud/Firebase projects (identified by project-number, called "Sender-ID") to send (FCM) notifications to an App-Instance (identified by Android-ID, Package-Name, and if necessary other parameters).
For some applications, particularly browsers (e.g. Chrome), the application needs additional information how to route incoming notifications. For this purpose, the field "subtype" was introduced. In the case of browsers, the field "subtype" holds the URL of the browser-tab or web-app that a notification is intended for. Every web-app (i.e. every different value of "subtype") will create a separate registration.
Nevertheless, the Instance-ID for all these registrations is always the same. That means, 1 App-Instance of a browser-app can have hundreds of registrations (and IID-Tokens), but only 1 Instance-ID.

IID Storage on Android
Unfortunately, I'm not familiar with the details of how the FirebaseInstanceId SDK stores data on an Android device, but the value of the field "subtype" should be irrelevant for getting the Instance-ID of an App-Instance.
However, maybe it is necessary to know one value for "subtype" to get the Instance-ID - I don't know.

When I looked at the IID Android code: this was the subtype default SUBTYPE_DEFAULT = ""; Hence, I skipped adding subtype.

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

ankitaj224 · 2019-10-16T23:31:52Z

/test new-smoke-tests

andirayo

pretty cool stuff!

andirayo · 2019-10-21T22:14:18Z

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

+    assertThat(entryValue).hasFid(TEST_INSTANCE_ID_1);
+
+    // Waiting for Task that registers FID on the FIS Servers
+    executor.awaitTermination(500, TimeUnit.MILLISECONDS);


Why does this have to wait half a second?
Are we actually sending a request to FIS here?

This test is for getId(). As you know, getId() return fid immediately but registers fid asynchronously. Tests have to wait for half a second while we mock registration. We dont send an actual request to FIS in tests.

I would recommend to add this explanation to the comment in the code.
Also, 500ms sounds excessive, but can't hurt maybe... :)

andirayo · 2019-10-21T22:25:54Z

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

+public class IidStore {
+  private static final String IID_SHARED_PREFS_NAME = "com.google.android.gms.appid";
+  private static final String STORE_KEY_PUB = "|S||P|";
+  private static final String STORE_KEY_ID = "|S|id";


Background
The Instance-ID Service revolves around Registrations (identified by IID-Tokens). Registrations are authorizations, mainly authorizations to Cloud/Firebase projects (identified by project-number, called "Sender-ID") to send (FCM) notifications to an App-Instance (identified by Android-ID, Package-Name, and if necessary other parameters).
For some applications, particularly browsers (e.g. Chrome), the application needs additional information how to route incoming notifications. For this purpose, the field "subtype" was introduced. In the case of browsers, the field "subtype" holds the URL of the browser-tab or web-app that a notification is intended for. Every web-app (i.e. every different value of "subtype") will create a separate registration.
Nevertheless, the Instance-ID for all these registrations is always the same. That means, 1 App-Instance of a browser-app can have hundreds of registrations (and IID-Tokens), but only 1 Instance-ID.

IID Storage on Android
Unfortunately, I'm not familiar with the details of how the FirebaseInstanceId SDK stores data on an Android device, but the value of the field "subtype" should be irrelevant for getting the Instance-ID of an App-Instance.
However, maybe it is necessary to know one value for "subtype" to get the Instance-ID - I don't know.

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

andirayo · 2019-10-21T23:01:47Z

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

+      String id = iidPrefs.getString(STORE_KEY_ID, /* defaultValue= */ null);
+
+      if (id != null) {
+        return id;
+      }


(Feel free to ignore this)
How about outsourcing this into its own method, even though its only one line:

private Optional<String> readInstanceIdFromLocalStorage() {
return Optional.ofNullable(iidPrefs.getString(STORE_KEY_ID, /* defaultValue= */ null));
}

(after reading my comments below)
You can then rewrite this method like this:

return readInstanceIdFromLocalStorage().orElse(this:: readPublicKeyFromLocalStorageAndCalculateInstanceIdFromIt);

andirayo · 2019-10-21T23:02:46Z

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

+        String fid = iidStore.readIid();
+        if (fid == null) {
+          fid = utils.createRandomFid();
+        }


(please see my comments at the bottom)
Assuming you switch the IidStore class to avoid NULL values, you can rewrite this as
String fid = iidStroe.readIid().orElse(utils::createRandomFid);

andirayo · 2019-10-23T20:50:31Z

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

+    assertThat(entryValue).hasFid(TEST_INSTANCE_ID_1);
+
+    // Waiting for Task that registers FID on the FIS Servers
+    executor.awaitTermination(500, TimeUnit.MILLISECONDS);


I would recommend to add this explanation to the comment in the code.
Also, 500ms sounds excessive, but can't hurt maybe... :)

andirayo · 2019-10-23T20:51:38Z

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

@@ -59,6 +61,20 @@
          .setResponseCode(ResponseCode.OK)
          .build();

+  public static final InstallationResponse TEST_INSTALLATION_RESPONSE_WITH_IID =
+      InstallationResponse.builder()
+          .setUri("/projects/" + TEST_PROJECT_ID + "/installations/" + TEST_INSTANCE_ID_1)


you could use a constant for this as well:
FIS_URI = "/projects/%s/installations/%s";

but not important.

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

ankitaj224 · 2019-10-24T17:11:45Z

/test smoke-tests

ankitaj224 · 2019-10-24T20:21:54Z

/test smoke-tests

Reusing existing Iid as Fid. Reading iid from "com.google.android.gms…

2382cc1

….appid" shared pref

ankitaj224 requested review from ciarand and andirayo October 16, 2019 22:06

googlebot added the cla: yes Override cla label Oct 16, 2019

google-oss-bot added the size/L label Oct 16, 2019

ankitaj224 changed the title ~~Reusing existing Iid as Fid~~ Reuse existing Iid as Fid Oct 16, 2019

ankitaj224 added 2 commits October 16, 2019 15:19

Reusing existing Iid as Fid. Reading iid from "com.google.android.gms…

7d7beed

….appid" shared pref

Merge branch 'iid-migration' of github.com:firebase/firebase-android-…

e6ba1d7

…sdk into iid-migration

ciarand reviewed Oct 16, 2019

View reviewed changes

Addressing ciaran's comments.

8446e9e

ankitaj224 requested a review from ciarand October 16, 2019 22:56

Fixing NPE.

5b86dca

andirayo approved these changes Oct 21, 2019

View reviewed changes

ankitaj224 added 3 commits October 22, 2019 11:30

Addressing Rayo's comments.

bec50b1

Fix verifyformat`

bce4059

Reading iid store only for default firebase app

a5ffdd3

andirayo approved these changes Oct 23, 2019

View reviewed changes

Addressed Rayo's comments.

ee514e8

Add FisError to the persisted installation entry. (#931)

d209690

ankitaj224 merged commit 98b2aac into fis_sdk Oct 25, 2019

ankitaj224 deleted the iid-migration branch October 25, 2019 18:43

firebase locked and limited conversation to collaborators Nov 25, 2019

Reuse existing Iid as Fid #924

Reuse existing Iid as Fid #924

Uh oh!

Conversation

ankitaj224 commented Oct 16, 2019

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!

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!

ankitaj224 commented Oct 16, 2019

Uh oh!

andirayo left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ankitaj224 commented Oct 24, 2019

Uh oh!

ankitaj224 commented Oct 24, 2019

Uh oh!

Uh oh!