Page MenuHomePhabricator

Implement react component that clears database anbd secure store when user logs out or deletes account
ClosedPublic

Authored by marcin on Aug 3 2022, 5:58 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 10:12 PM
Unknown Object (File)
Fri, Nov 22, 6:59 PM
Unknown Object (File)
Fri, Nov 22, 6:56 PM
Unknown Object (File)
Tue, Nov 5, 10:59 AM
Unknown Object (File)
Oct 19 2024, 7:07 PM
Unknown Object (File)
Oct 10 2024, 3:16 PM
Unknown Object (File)
Oct 10 2024, 3:16 PM
Unknown Object (File)
Oct 10 2024, 3:16 PM

Details

Summary

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

Test Plan

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
Branch
marcin/eng-552
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
marcin requested review of this revision.Aug 3 2022, 6:14 AM
tomek added a reviewer: ashoat.

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.

ashoat requested changes to this revision.Aug 4 2022, 10:09 PM

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]);
This revision now requires changes to proceed.Aug 4 2022, 10:09 PM

Use selector for entire condition governing whether to delete database

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.

ashoat requested changes to this revision.Aug 5 2022, 9:30 AM

I assume the main reason behind requesting those changes was the fact that previous code didn't handle the case when currentUserInfo is null.

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

There are easier ways to case to boolean, eg. Boolean(something) or !!something

10

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

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?

This revision now requires changes to proceed.Aug 5 2022, 9:30 AM
native/data/sensitive-data-cleaner.react.js
9–11

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

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:

  1. currentUserInfo becomes null or gets anonymous field set to true.
  2. User opens the app and currentUserInfo is null or has anonymous field set to true.
  3. currentUserInfo.id changes.

Probably 1 and 3 can be merged together to something like: currentUserInfo becomes null or changes its id field.

17

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.

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):

  1. Database is deleted (on condition it does exist).
  2. Encryption key is overwritten.
  3. New database is created - it is essential, since there are components that try to access database even if the user is logged out.

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

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

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

ashoat requested changes to this revision.Aug 10 2022, 6:56 AM
ashoat added inline comments.
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?

This revision now requires changes to proceed.Aug 10 2022, 6:56 AM
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)

Isn't the first condition a subset of the second condition?

Not exactly, second condition assumes currentUserInfo is not null. But they can be simplified to:
databaseCurrentUserInfoID && (userLoggedOut || currentUserInfo?.id !== databaseCurrentUserInfoID)

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

Is clearSensitiveData able to handle being run when the app is opened and the user is not logged in?

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:

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.

Has this case been tested?

This case is mentioned in the test plan for this diff:

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.

I wouldn't put this diff on phabricator without executing steps and seeing the outcome as described in the test plan.

Simplify database deletion condition

ashoat requested changes to this revision.Aug 12 2022, 10:08 AM

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?

This revision now requires changes to proceed.Aug 12 2022, 10:08 AM

Simplify effect logics and dependency list

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.

ashoat requested changes to this revision.Aug 16 2022, 5:11 AM
ashoat added inline comments.
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?

This revision now requires changes to proceed.Aug 16 2022, 5:11 AM
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.
Keeping in mind that this code is already quite complicated I will opt for the second option.

Make call to setCurrentUserID independent from call to clearSensitiveData

ashoat added inline comments.
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.

This revision is now accepted and ready to land.Aug 17 2022, 10:14 AM
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.

Update component so that it uses asynchronous C++ API