This differential implements react component that attempts to clear SQLite and secure store when user logs out or deletes account. Component attempts deletion when 'currentUserInfo' of redux-state changes to 'anonymous' which indicates that log out succeded or account was succesfully deleted
Details
Build the app and repeat log in and log out action several times switching users. Additionally some accounts might also be deleted. Application should work without any difference no matter whether it is kept foregrounded, backgrounded or closed by the user between subsequent log-in and log-out actions. Additionally database size should be monitored in XCode/Android studio after each log out and log in to ensure it is actually deleted and recreated empty.
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Adding @ashoat because this is a really important code.
native/data/sensitive-data-cleaner.react.js | ||
---|---|---|
12 ↗ | (On Diff #15262) | This can be simplified |
24 ↗ | (On Diff #15262) | Just to clarify: we attempt to delete the db every time a user is anonymous instead of doing that only during log out process. To check if user is logging out, we would need to compare anonymous with previous state and check if it changed. In this case this is intentional. We would like to handle the case when the app was force quitted during during logging out but before the db was deleted. Restarting the app would result in a state when a user is anonymous but the db contains the old data. So every time a user is anonymous we attempt to delete the db and not only when it's changing. |
As @tomek suggests, we should track past state and only clean when the relevant state changes. Separately, we should clear data not just if the CurrentUserInfo is anonymous, but also if there is no CurrentUserInfo.
So we want something like:
const userIsLoggedIn = useSelector(state => !!(state.currentUserInfo && !state.currentUserInfo.anonymous)); const userWasLoggedIn = React.useRef(undefined); React.useEffect(() => { if (userIsLoggedIn === userWasLoggedIn.current) { return; } userWasLoggedIn.current = userIsLoggedIn; if (userIsLoggedIn) { return; } try { global.CommCoreModule.clearSensitiveData(); } catch (e) { if (__DEV__) { throw e; } else { console.log(e.message); ExitApp.exitApp(); } } }, [userIsLoggedIn]);
If userIsLoggedIn indeed is the only thing in the dep list, you might not need userWasLoggedIn actually... the whole thing can probably be simplified to:
const userIsLoggedIn = useSelector(state => !!(state.currentUserInfo && !state.currentUserInfo.anonymous)); React.useEffect(() => { if (userIsLoggedIn) { return; } try { global.CommCoreModule.clearSensitiveData(); } catch (e) { if (__DEV__) { throw e; } else { console.log(e.message); ExitApp.exitApp(); } } }, [userIsLoggedIn]);
I assume the main reason behind requesting those changes was the fact that previous code didn't handle the case when currentUserInfo is null. It only attempted database deletion when currentUserInfo contained anonymous field. I agree it is probably safer to delete database as soon as possible (when currentUserInfo becomes null) but I did not handle this case previously on purpose. When I was working with flipper I never observed currentUserInfo to become permanently null. It always eventually ended up containing anonymous field. But as stated previously I am not against deletion database as soon as we know that user logged out/account was deleted.
Not just that... you were also running the effect more often than necessary since it would run whenever currentUserInfo changes, which would result in SensitiveDataCleaner being run in a potential future case where currentUserInfo changes without the logged in user changing. (For instance, if we introduced settings to LoggedOutUserInfo`.)
It only attempted database deletion when currentUserInfo contained anonymous field. I agree it is probably safer to delete database as soon as possible (when currentUserInfo becomes null) but I did not handle this case previously on purpose. When I was working with flipper I never observed currentUserInfo to become permanently null. It always eventually ended up containing anonymous field. But as stated previously I am not against deletion database as soon as we know that user logged out/account was deleted.
While it's great that you tested, it's not always possible to test all conditions. It is indeed possible for the user to be logged out without an anonymous cookie being provided (it is an edge case) and we should consider that case.
native/data/sensitive-data-cleaner.react.js | ||
---|---|---|
9–11 ↗ | (On Diff #15354) | There are easier ways to case to boolean, eg. Boolean(something) or !!something |
10 ↗ | (On Diff #15354) | It's currently not possible for currentUserInfo to change from one logged-in user to another. But if in the future we introduce something like that, I think the way this code is written it would not call clearSensitiveData. Sorry for not considering this earlier! I wonder if we should look at something like currentlyLoggedInUserID rather than userIsLoggedIn. And then if currentlyLoggedInUserID changes at all we would run clearSensitiveData. What do you think? |
17 ↗ | (On Diff #15354) | Are we okay with this function being run when the app is opened and the user is not logged in? Or do we only want it to run when the user logs out after being logged in? |
native/data/sensitive-data-cleaner.react.js | ||
---|---|---|
9–11 ↗ | (On Diff #15354) | The problem here is not about casting to boolean, but about accessing .anonymous field of currentUserInfo. currentUserInfo does not have to contain this field so flow complained about it. using the ternary operator here was the least verbose way to satisfy flow. |
10 ↗ | (On Diff #15354) | I also didn't consider this case. It is good that you mention it. If transition between logged user is expected to go through some intermediate state when currentUserInfo is either null or contains anonymous field then the code above will work. But if we wish that this transition will be direct then we probably we will need to introduce changes. To summarise we want to fire clearSensitiveData everytime:
Probably 1 and 3 can be merged together to something like: currentUserInfo becomes null or changes its id field. |
17 ↗ | (On Diff #15354) |
In the comment above Tomek explains why it is essential that we fire this function when the app is opened and the user is not logged in. In clearSensitiveData three things happen in C++ layer (in the following order):
If 1 or 2 fails we are left with undeleted database and/or secure store - that is a safety threat. If 3) fails we are left with running application with deleted database, so it would eventually crash itself. So we intentionally kill the app if any of 1) - 3) fails and restart the process from the beginning when the user opens the app. That is why we want to run this function if nobody is logged in but the app is opened since we might need to recover from the failure of one of 1 - 3. |
native/data/sensitive-data-cleaner.react.js | ||
---|---|---|
10 ↗ | (On Diff #15354) | I will be currently working on new boolean conditions that will correctly handle those and only those cases. |
native/data/sensitive-data-cleaner.react.js | ||
---|---|---|
17 ↗ | (On Diff #15354) | I was asking more, does running clearSensitiveData like this cause any problems? Is clearSensitiveData able to handle being run when the app is opened and the user is not logged in? Has this case been tested? |
Use new logic based on user id kept in the database to address sensitive data deletion upon user log out/account deletion
native/data/sensitive-data-cleaner.react.js | ||
---|---|---|
13–14 ↗ | (On Diff #15508) | I don't think the Boolean conversion is necessary here anymore... if you want to keep it, is it maybe possible to simplify it now? (eg. by casting to Boolean or using double negation !!) |
16–18 ↗ | (On Diff #15508) | I think these two conditions can be combined. Isn't the first condition a subset of the second condition? |
32 ↗ | (On Diff #15508) | I don't see why it's necessary to pass the whole currentUserInfo in here. Inside the effect we only need to look at currentUserInfo?.id, right? |
native/data/sensitive-data-cleaner.react.js | ||
---|---|---|
13–14 ↗ | (On Diff #15508) | As in previous version of this diff, it is not the case of casting to boolean. Flow complains if I try to access anonymous attribute since currentUserInfo might not have it. This is how I was able to satisfy flow. Another possibility was lambda with if statement that checks whether currentUserInfo as anonymous attribute. This one is less verbose. |
16–18 ↗ | (On Diff #15508) |
Not exactly, second condition assumes currentUserInfo is not null. But they can be simplified to: |
32 ↗ | (On Diff #15508) | We want to run this effect also when currentUserInfo becomes null. But I am not react expert and might not know that passing currentUserInfo?.id instead will also cause this effect to be triggered if currentUserInfo changes to null. |
17 ↗ | (On Diff #15354) |
This function is being executed when user opens the app but nobody is logged in. Previous comment in this thread should have explained this here:
This case is mentioned in the test plan for this diff:
I wouldn't put this diff on phabricator without executing steps and seeing the outcome as described in the test plan. |
I don't understand your responses as to why this code needs to be so complicated. I'm still not convinced. If you are sure it needs to be this complicated, please try explaining again in a clear, concise way. It might help to run your explanation by @tomek to make sure it makes sense to him before replying again
native/data/sensitive-data-cleaner.react.js | ||
---|---|---|
17 ↗ | (On Diff #15604) | See comment below. I don't think this needs to be so complicated |
31 ↗ | (On Diff #15604) | Still don't think we need all of currentUserInfo here. I think we just need: const currentLoggedInUserID = useSelector(state => state.currentUserInfo.anonymous ? null : state.currentUserInfo.id); I'm pretty sure that's the only thing you need to pass into this effect. Can you please simplify the logic? |
I agree the logics of if statements and dependency list here was too complicated, and your suggestion on how to simplify it was right. Let me explain step by step what actually happens in this differential:
const currentLoggedInUserID = useSelector(state => state.currentUserInfo?.anonymous ? null : state.currentUserInfo?.id, );
If there is a user currently logged in we get his id. Otherwise we get null
const databaseCurrentUserInfoID = global.CommCoreModule.getCurrentUserID();
Line above retrieves ID of currently logged user from the database. We duplicate ID of currently logged user ID (if there is not logged user we get empty string) between database and redux so that we can detect discrepancies stemming from unsuccessful database deletion after user logs out or transitions for another user (not currently supported but we prepare ourselves in advance for such a case).
if ( databaseCurrentUserInfoID && databaseCurrentUserInfoID !== currentLoggedInUserID ) { global.CommCoreModule.clearSensitiveData(); }
If user ID from database is not empty string and it is not equal to currently logged user ID from redux it means we have discrepancy - database deletion failed or we didn't attempt it at all. We are left with database containing data of previously logged user.
else if (currentLoggedInUserID && !databaseCurrentUserInfoID) { global.CommCoreModule.setCurrentUserID(currentLoggedInUserID); }
If currently logged user id from redux is not null and there is not user id in the database we want to set it so that next time user logs out/deletes account we have a reference id to compare with.
native/data/sensitive-data-cleaner.react.js | ||
---|---|---|
15–22 ↗ | (On Diff #15639) | If databaseCurrentUserInfoID exists but is not the same as currentLoggedInUserID, it seems like we should first clear clearSensitiveData and then setCurrentUserID. But I think the code as-written will just clearSensitiveData. Is this a concern? |
native/data/sensitive-data-cleaner.react.js | ||
---|---|---|
15–22 ↗ | (On Diff #15639) | It is good that you pointed it out. I didn't notice it since it is not actually an issue not to do so. In most cases we will delete database after logout so currentUserInfo is null and we cannot call setCurrentUserID. But once we support direct transition from one user to another we should update this data in the database. So instead of placing a direct call to setCurrentUserID after clearSensitiveData I think it is best to convert the else if branch into separate if statement. Like this: if (currentLoggedInUserID && currentLoggedInUserID !== databaseCurrentUserInfoID) { global.CommCoreModule.setCurrentUserID(currentLoggedInUserID); } It could be simplified to: if (currentLoggedInUserID) { global.CommCoreModule.setCurrentUserID(currentLoggedInUserID); } But at the cost that setCurrentUserID will be called every time user opens the app while being already logged in, which will not be harmful but redundant. |
native/data/sensitive-data-cleaner.react.js | ||
---|---|---|
15–23 ↗ | (On Diff #15660) | Will there be any issues first calling clearSensitiveData and then setCurrentUserID? I'm worried about a potential race condition / deadlock type issue. I'm guessing it's fine, but would be good to test this scenario |
native/data/sensitive-data-cleaner.react.js | ||
---|---|---|
15–23 ↗ | (On Diff #15660) | I don't think there are going to be any threading issues here. Both calls are implemented as blocking and running on the main thread on the C++ side. |
Looks good
native/data/sensitive-data-cleaner.react.js | ||
---|---|---|
15–23 ↗ | (On Diff #15660) | Is there any benefit to both changes being included in a single DB transaction so they're made atomically? Can't think of a specific reason, but I also don't have the same context. |
native/data/sensitive-data-cleaner.react.js | ||
---|---|---|
15–23 ↗ | (On Diff #15660) | I am not sure whether it is possible. The first one (global.CommCoreModule.clearSensitiveData()) does not execute any database query. It uses file deletion to clear database, then updates secure store with new encryption key, finally re-creates new empty database. See here: https://phab.comm.dev/D4728. The next global.CommCoreModule.setCurrentUserID(currentLoggedInUserID) is a regular database query. I don't think we can tie file system operations and SQLite query into a SQLite transaction. |
native/data/sensitive-data-cleaner.react.js | ||
---|---|---|
15–23 ↗ | (On Diff #15660) | Ah sorry didn't realize clearSensitiveData() was just a file operation... feel free to disregard what I said. |