[native] commCoreModule methods for getting, setting, and clearing the commServicesAccessToken
Needs ReviewPublic
Actions

Authored by varun on Mon, Sep 18, 2:38 PM.

Details

Reviewers

bartek
kamil

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

Branch

securestore

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

varun created this revision.Mon, Sep 18, 2:38 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptMon, Sep 18, 2:38 PM

Harbormaster completed remote builds in B22700: Diff 31235.Mon, Sep 18, 2:56 PM

varun requested review of this revision.Mon, Sep 18, 2:56 PM

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

@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.
@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: Necessary for authentication at the same time. 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

bartek added inline comments.Tue, Sep 19, 9:09 AM

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

address feedback

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

varun edited the test plan for this revision. (Show Details)Tue, Sep 19, 8:03 PM

Harbormaster completed remote builds in B22747: Diff 31290.Tue, Sep 19, 8:14 PM

@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