Page MenuHomePhabricator

[native] commCoreModule methods for getting, setting, and clearing the commServicesAccessToken
ClosedPublic

Authored by varun on Sep 18 2023, 2:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 23, 7:02 AM
Unknown Object (File)
Wed, May 22, 10:17 PM
Unknown Object (File)
Tue, May 21, 1:19 PM
Unknown Object (File)
Tue, May 21, 1:19 PM
Unknown Object (File)
Tue, May 21, 1:19 PM
Unknown Object (File)
Tue, May 21, 1:19 PM
Unknown Object (File)
Tue, May 21, 1:14 PM
Unknown Object (File)
Sat, May 18, 7:04 PM

Details

Summary

we need the access token to be available in the SecureStore on native for NSE.

Test Plan
 varun  ~  Code  comm  native  git diff                                                                                                      ST 218   securestore 
diff --git a/native/selectors/account-selectors.js b/native/selectors/account-selectors.js
index a9b478e54..77e2bff26 100644
--- a/native/selectors/account-selectors.js
+++ b/native/selectors/account-selectors.js
@@ -25,6 +25,24 @@ const nativeLogInExtraInfoSelector: (
     calendarActive: boolean,
   ) => {
     const loginExtraFuncWithIdentityKey = async () => {
+      const nada = await commCoreModule.getCommServicesAuthMetadata();
+      console.log('nothing here yet: ', JSON.stringify(nada));
+      await commCoreModule.setCommServicesAuthMetadata('123', 'abc', 'cowboy');
+      const metadata = await commCoreModule.getCommServicesAuthMetadata();
+      console.log('got auth metadata: ', JSON.stringify(metadata));
+      await commCoreModule.clearCommServicesAccessToken();
+      const updatedMetadata =
+        await commCoreModule.getCommServicesAuthMetadata();
+      console.log(
+        'got metadata, no token present: ',
+        JSON.stringify(updatedMetadata),
+      );
+      await commCoreModule.setCommServicesAccessToken('bebop');
+      const finalMetadata = await commCoreModule.getCommServicesAuthMetadata();
+      console.log(
+        'got metadata, new token present: ',
+        JSON.stringify(finalMetadata),
+      );
       await commCoreModule.initializeCryptoAccount();
       const { blobPayload, signature } =
         await commCoreModule.getUserPublicKey();

on log out, confirmed that user id, device id, and access token were cleared

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun requested review of this revision.Sep 18 2023, 2:56 PM
marcin added subscribers: kamil, bartek.

I am leaving for vacation so I don't want to block this diff. I am adding two reviewers:

  1. @kamil for changes related to data clearance. I didn't add him as a blocking reviewer since he might be absent for a longer time. But if he is back quickly I would appreciate his review on the data clearance.
  2. @bartek for changes related to putting entire authentication payload in Secure Store at once.
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
972 ↗(On Diff #31235)

As explained here: https://linear.app/comm/issue/ENG-4390/authenticate-blob-service-requests, commAccessToken is not enough to authenticate. We also need deviceID (ed25519) and userID (I recall it is obtained from identity service). Unfortunately deviceID and userID are neither accessible from NotificationServiceExtension so they have to be persisted in Secure Store as well. Perhaps you plan to implement separate setters and getters for those two but since they are:

  1. Necessary for authentication at the same time.
  2. Present in an app at the same time

they should be persisted in Secure Store at the same time.
Perhaps we should stringify and base64 encode the entire Bearer payload and put one string in Secure Store.
cc @bartek

998–1003 ↗(On Diff #31235)

Are we sure this is the reason to throw an error? An error should be thrown in case accessing Secure Store throws (catch statement is executed). In case there is no token present we should probably return undefined so that the JS side has a way to check if the token is present or not.

1024 ↗(On Diff #31235)

As far as I am concerned commServicesAccessToken is considered to be a part of sensitive data so it should be deleted in clearSensitiveData which is implemented in DatabaseManager.

cc @kamil

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
972 ↗(On Diff #31235)

Perhaps we should stringify and base64 encode the entire Bearer payload and put one string in Secure Store.

We shouldn't. Maybe it's convenient for HTTP clients to access them this way, but for gRPC clients (and all other possible cases), it's better to have them available separately

I ended up adding a new setCommServicesAuthMetadata method because we will set user id, device id, and access token all at once initially, but subsequently may need to update just the access token.
also changed getCommServicesAccessToken to getCommServicesAuthMetadata since we will need to retrieve all three at once

bartek added 1 blocking reviewer(s): kamil.

@kamil for changes related to data clearance. I didn't add him as a blocking reviewer since he might be absent for a longer time. But if he is back quickly I would appreciate his review on the data clearance.

Looks good to me but if @kamil is back soon, it would be good if he looks at this too.
Otherwise, we can always address potential feedback later in subsequent diff to avoid blocking this for too long

Accepting to unblock, but please look at my comment about moving clearance code to DatabaseManager::clearSensitiveData()

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
868–870 ↗(On Diff #31290)

this is clearSensitiveData called by the JS, and only in this case.

There is a case when we find some issues with the database, delete it, and cause the app to re-loggin' the user. This is handled DatabaseManager::clearSensitiveData() which was supposed to be a method for clearing anything no matter of reason, so I think it's safer to put this code there.

native/cpp/CommonCpp/NativeModules/CommCoreModule.h
29 ↗(On Diff #31290)

Shouldn't we do something about the old deviceID concept? (getDeviceID(), setDeviceID).

I think there are some unnecessary computations right now, and after this, the naming could be confusing so we can e.g. create a task to discuss this.

This revision is now accepted and ready to land.Sep 22 2023, 7:26 AM
varun requested review of this revision.Oct 5 2023, 12:48 PM
varun added 1 blocking reviewer(s): kamil.
varun added inline comments.
native/cpp/CommonCpp/Tools/CommSecureStore.h
13–15 ↗(On Diff #31697)

@kamil wanted to check if this is idiomatic... i originally wanted to make these static members, but that meant defining them in multiple places for iOS/Android.

requesting review again because i have a question inline and also want to make sure i've addressed @kamil 's feedback correctly

Thanks @varun, looks good

native/cpp/CommonCpp/Tools/CommSecureStore.h
13–15 ↗(On Diff #31697)

Probably this is okay for now, keeping in mind there is no better place for this.

I would only add key suffix, like commServicesAccessTokenKey to distinguish from actual value

This revision is now accepted and ready to land.Oct 6 2023, 5:49 AM
native/cpp/CommonCpp/Tools/CommSecureStore.h
13–15 ↗(On Diff #31697)

I considered adding the key suffix, but felt like it might lead to more confusion since we already have deviceIDKey in some places referring to the "public key which serves as the device ID"

Gonna keep this as-is for now and land the diff. Can make changes later if you feel strongly about this.

native/cpp/CommonCpp/Tools/CommSecureStore.h
13–15 ↗(On Diff #31697)

makes sense, thanks for explanation