Page MenuHomePhabricator

Merge SensitiveDataHandler functionality into SQLiteContextProvider
ClosedPublic

Authored by marcin on Nov 9 2022, 4:53 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 21, 3:18 PM
Unknown Object (File)
Wed, Nov 20, 9:07 PM
Unknown Object (File)
Thu, Nov 14, 6:15 PM
Unknown Object (File)
Tue, Nov 5, 11:32 AM
Unknown Object (File)
Tue, Nov 5, 11:32 AM
Unknown Object (File)
Tue, Nov 5, 11:32 AM
Unknown Object (File)
Tue, Nov 5, 11:20 AM
Unknown Object (File)
Tue, Nov 5, 11:04 AM
Subscribers

Details

Summary

This differential merges SensitiveDataHandler functionality into SQLiteContextProvider. There is a race condition between those two components which might lead to hard to debug issue. Therefore in this differential we make sure that functionalities privided by those two components are always executed in deterministic order.

Test Plan

Test plan for this differential should include test plans for those two differentials: https://phab.comm.dev/D4730, https://phab.comm.dev/D5444 since SensitiveDataHandler functionality should remain. Additionally we should temporarily add logging inside handleSensitiveData and before SQLiteContextProvider queries SQLite and updates redux. We should ensure the order of logs is deterministic on every app run.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin requested review of this revision.Nov 9 2022, 5:07 AM
ashoat added 2 blocking reviewer(s): tomek, atul.

I'm concerned about a potential scenario where the user is logged in when the app starts, but then quickly becomes logged out.

In this scenario, the commCoreModule data fetch will start (since the user is logged in when the app starts), but then the logout will trigger handleSensitiveData.

I could imagine this leading to a situation where the data fetch gets bad results because of handleSensitiveData, and then commits those bad results into Redux.

My suggestion to address this: the Redux action should have a field on it that says what the userID was when the fetch was initiated. Then the reducer can check if this matches the current userID, and if not it will ignore the data. This approach is similar to how invalidSessionDowngrade works.

That said, I think that should be done as part of ENG-2137. This diff I think is a clear improvement, so we can probably land it now.

I'd still like to get @tomek's and @atul's perspectives, however.

I'm concerned about a potential scenario where the user is logged in when the app starts, but then quickly becomes logged out.

In this scenario, the commCoreModule data fetch will start (since the user is logged in when the app starts), but then the logout will trigger handleSensitiveData.

I could imagine this leading to a situation where the data fetch gets bad results because of handleSensitiveData, and then commits those bad results into Redux.

My suggestion to address this: the Redux action should have a field on it that says what the userID was when the fetch was initiated. Then the reducer can check if this matches the current userID, and if not it will ignore the data. This approach is similar to how invalidSessionDowngrade works.

That said, I think that should be done as part of ENG-2137. This diff I think is a clear improvement, so we can probably land it now.

I'd still like to get @tomek's and @atul's perspectives, however.

Sounds like a valid concern and I agree that the solution will fix the issue. Also, landing this as is, is a good idea.

This revision is now accepted and ready to land.Nov 9 2022, 6:00 PM

Rebase on master before landing