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

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

Details

Reviewers

bartek
kamil

Commits

rCOMM13d2c8066a70: [native] commCoreModule methods for getting, setting, and clearing the…

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

No Lint Coverage

Unit

No Test Coverage

Event Timeline

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

Herald added subscribers: tomek, ashoat. · View Herald TranscriptSep 18 2023, 2:38 PM

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

varun requested review of this revision.Sep 18 2023, 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.Sep 19 2023, 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)Sep 19 2023, 8:03 PM

Harbormaster completed remote builds in B22747: Diff 31290.Sep 19 2023, 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

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

address feedback

native/cpp/CommonCpp/NativeModules/CommCoreModule.h
29 ↗	(On Diff #31290)	https://linear.app/comm/issue/ENG-5167/remove-userid-deviceid-getterssetters-from-commcoremodule

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

Harbormaster completed remote builds in B23017: Diff 31697.Oct 5 2023, 1:02 PM

Thanks @varun, looks good

native/cpp/CommonCpp/Tools/CommSecureStore.h
13–15	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

varun added inline comments.Oct 6 2023, 8:05 AM

native/cpp/CommonCpp/Tools/CommSecureStore.h
13–15	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.