[native] Clear auth metadata on logout
ClosedPublic
Actions

Authored by tomek on Mon, Apr 8, 6:01 AM.

Details

Reviewers

kamil
inka
varun
ashoat

Commits

rCOMM77893eed7c78: [native] Clear auth metadata on logout

Summary

When a user logs out, we need to clear the auth metadata. This requires adding a function that is responsible for clearing the metadata and emitting new values.

We're setting empty strings as new metadata - which isn't ideal. We're going to improve it as a part of https://linear.app/comm/issue/ENG-7657/improve-auth-metadata-handling.

https://linear.app/comm/issue/ENG-6730/call-clearcommservicesaccesstoken-on-native-logout-and-account

Depends on D11534

Test Plan

Log out and check if an event with empty token was emitted

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tomek created this revision.Mon, Apr 8, 6:01 AM

Herald added a subscriber: ashoat. · View Herald TranscriptMon, Apr 8, 6:01 AM

tomek added inline comments.Mon, Apr 8, 6:04 AM

native/components/access-token-handler.react.js
21 ↗	(On Diff #38890)	Without https://linear.app/comm/issue/ENG-7657/improve-auth-metadata-handling we have to interpret empty string as a missing `accessToken`.
native/data/sqlite-data-handler.js
42–43 ↗	(On Diff #38890)	It should be possible to run these in parallel, but when testing this I noticed that using `Promise.all` here somehow blocks the emission of the metadata.

Harbormaster completed remote builds in B28041: Diff 38890.Mon, Apr 8, 6:17 AM

tomek requested review of this revision.Mon, Apr 8, 6:17 AM

ashoat added inline comments.Mon, Apr 8, 7:12 PM

native/components/access-token-handler.react.js
21 ↗	(On Diff #38890)	I think that in JS the empty string is considered falsey, so the extra checks aren't necessary
native/data/sqlite-data-handler.js
42–43 ↗	(On Diff #38890)	Did you find out why?
native/identity-service/identity-service-context-provider.react.js
73–76 ↗	(On Diff #38890)	I think that in JS the empty string is considered falsey, so the extra checks aren't necessary

ashoat added inline comments.Mon, Apr 8, 7:15 PM

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
1586–1587 ↗	(On Diff #38890)	The `deviceID` we store in `CommSecureStore` appears to be the same as the one in the Olm account. It seems like there is a risk that the two will drift apart, especially since JS is in between. How do we make sure that these don't go out of sync, and why do we need to store the two separately?
native/schema/CommCoreModuleSchema.js
133–137 ↗	(On Diff #38890)	In what scenarios do we use `setCommServicesAuthMetadata` vs. `setCommServicesAccessToken`?
139 ↗	(On Diff #38890)	In what scenarios do we use `clearCommServicesAuthMetadata` vs. `clearCommServicesAccessToken`?

Delete empty string checks. Move clearing the metadata to clearSensitiveData. Stop running secure store functions on the DB thread.

native/components/access-token-handler.react.js
21 ↗	(On Diff #38890)	Right... I got confused.
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
1586–1587 ↗	(On Diff #38890)	why do we need to store the two separately? I don't think it was a conscious decision and we should try to maintain the single source of truth. We probably should remove the one from the secure store. And at this point, these can get out of sync. We can improve that as a part of https://linear.app/comm/issue/ENG-7657/improve-auth-metadata-handling. @kamil
native/data/sqlite-data-handler.js
42–43 ↗	(On Diff #38890)	For some reason `setCommServicesAuthMetadata` creates a job that is run on `GlobalDBSingleton::instance`. `clearSensitiveData` cancels all the other tasks, so when `setCommServicesAuthMetadata` is run during `clearSensitiveData` it gets simply ignored. Going to modify `setCommServicesAuthMetadata` (and similar functions) so that it isn't run on the DB thread. Also, it probably makes more sense to call `clearCommServicesAuthMetadata` as a part of `clearSensitiveData`.
native/schema/CommCoreModuleSchema.js
133–137 ↗	(On Diff #38890)	Currently, there are no usages of `setCommServicesAccessToken`. I can imagine that if the token becomes invalidated we could receive a new one and set only it. But I think that always setting the whole metadata is safer - these three values are always used together. We can consider deleting this method as a part of https://linear.app/comm/issue/ENG-7657/improve-auth-metadata-handling.
139 ↗	(On Diff #38890)	There are no usages of `clearCommServicesAccessToken` and I think that we should always clear the whole metadata: When a token becomes invalidated, we should set the new one instead of clearing the invalid one When we log out / delete an account / etc. we should clear the whole metadata Deleting this method can be considered as a part of https://linear.app/comm/issue/ENG-7657/improve-auth-metadata-handling.

Harbormaster completed remote builds in B28092: Diff 38946.Tue, Apr 9, 6:32 AM

For some reason setCommServicesAuthMetadata creates a job that is run on GlobalDBSingleton::instance

Looks like this code was introduced by @varun in D9224. @varun, can you clarify why you're running this on the DB thread given it doesn't perform any SQLite queries? I'm wondering if it's done that way on purpose to protect against some race condition or something. @tomek changes that logic here, so we should make sure we're not accidentally reverting something that was decided intentionally.

native/cpp/CommonCpp/NativeModules/CommCoreModule.h
166 ↗	(On Diff #38946)	I think overloading this method name is confusing. Maybe we can call this `innerSetCommServicesAuthMetadata`?
172 ↗	(On Diff #38946)	Same here

In D11573#333844, @ashoat wrote:

For some reason setCommServicesAuthMetadata creates a job that is run on GlobalDBSingleton::instance

Looks like this code was introduced by @varun in D9224. @varun, can you clarify why you're running this on the DB thread given it doesn't perform any SQLite queries? I'm wondering if it's done that way on purpose to protect against some race condition or something. @tomek changes that logic here, so we should make sure we're not accidentally reverting something that was decided intentionally.

I think I was under the impression that we needed to do this to ensure atomicity, but I was wrong... we don't need to run this code on the DB thread (cc @marcin )

This revision is now accepted and ready to land.Wed, Apr 10, 10:15 AM

varun mentioned this in D11623: [native] remove unnecessary include.Wed, Apr 10, 10:48 AM

varun mentioned this in rCOMMcf5e7c2e5d1e: [native] remove unnecessary include.Wed, Apr 10, 12:07 PM

Rename

Harbormaster completed remote builds in B28168: Diff 39051.Thu, Apr 11, 5:33 AM

Closed by commit rCOMM77893eed7c78: [native] Clear auth metadata on logout (authored by tomek). · Explain WhyThu, Apr 11, 5:38 AM

This revision was automatically updated to reflect the committed changes.

tomek added a commit: rCOMM77893eed7c78: [native] Clear auth metadata on logout.