Details
- Mobile user makes a new thick thread with web user (with new message button)
- Mobile user sends friend request
- Throw some error in fetchUserIdentitiesPromise() so that finding user indentity always fails and make auxUserInfo empty.
- Web user accepts the request
- Before a new thin thread was created. Now no new threads are created, users are friends, because there is already a thick thread between users.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Hmmm... I know I initially proposed doing both D13938 and this, but after reading D13938 I wonder if we need this additional mechanism.
@kamil, do you know if it's possible for the user to see a thick thread in the UI if one of the members doesn't pass the userHasDeviceList check? Specifically in this case, I'm wondering about a PERSONAL thread, like in @tomek's original report in ENG-9509. It seems to me that the thread creation message would have had to come in from an established Olm session, so I would expect that userHasDeviceList would always be true, and the check in D13938 would suffice.
If that's the case, I'd prefer to keep the code simple. Sorry for any misdirection here!
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1903–1907 | What do you think of this notation? | |
Yes, there are some edge cases. The most realistic one is:
- Creating a thick thread with a user A
- A logs out on all its devices using V1 flows (or in the future, has password reset)
- A's device list is an empty array
- Looking at the code, userHasDeviceList will return false, but in theory, the user supports thick threads.
Actually, I feel like using userHasDeviceList is not the best long-term check in useUsersSupportThickThreads - as we shouldn't check deviceListIsNonEmpty but only if the device list exists. If I recall correctly, we decided to do this before launching E2EE DMs because of the limitations of our architecture (no backup), there is no point in creating a thick thread in that case because the other user will never get information about this thread (has empty device list).
It seems to me that the thread creation message would have had to come in from an established Olm session, so I would expect that userHasDeviceList would always be true, and the check-in D13938 would suffice.
This part about the Olm session also can have an edge case, when we receive a session request and thread creation message, we establish a session and process DM Ops but e.g. we fail to process the device list update prior to that, and AuxUserStore can be out-of-date. There is a task tracking this: ENG-9332.
If that's the case, I'd prefer to keep the code simple. Sorry for any misdirection here!
I feel like this could be okay as a temporary solution, but in the future (when having a backup or at least Share missing threads between peers) we should more lean towards updating userHasDeviceList to check only device list existence, not device list length.
I guess a way to reframe what you're saying about useUsersSupportThickThreads is that userHasDeviceList doesn't match findUserIdentities in the case of an empty device list. We end up returning the correct answer anyways, since we check findUserIdentities as a fallback. But it would be better if we were able to get the right answer without having to query the identity service.
I think it makes sense to update useUsersSupportThickThreads to just check for device list existence instead of userHasDeviceList. But I'm not sure I agree with this part:
in the future (when having a backup or at least Share missing threads between peers) we should more lean towards updating userHasDeviceList to check only device list existence, not device list length.
I checked the other two callsites of userHasDeviceList:
- useCreateMessagesToPeersFromDMOp: in this case, it seems like the only change would be that we would stop requesting the device list for this user, which is probably a bad thing. Outside of that, in terms of sending messages to peers, an empty device list is the same as no device list: in both cases, no devices are added to peerUserIDAndDeviceIDs.
- usePotentialMemberItems: in this case, we would end up showing a user with no devices as an option to add to a thick thread. I think that's probably a bad thing too... this user will never see any messages sent to them, so it's better to prevent them from being added.
Separately, I think there's a disconnect about the backup service. You mentioned backup in the last quote, and also this one:
If I recall correctly, we decided to do this before launching E2EE DMs because of the limitations of our architecture (no backup), there is no point in creating a thick thread in that case because the other user will never get information about this thread (has empty device list).
I think this will still be the case even after we launch backup. If there is no device to send the message to, then there is no way to send an encrypted message, right?
Yes, exactly
I think it makes sense to update useUsersSupportThickThreads to just check for device list existence instead of userHasDeviceList. But I'm not sure I agree with this part:
in the future (when having a backup or at least Share missing threads between peers) we should more lean towards updating userHasDeviceList to check only device list existence, not device list length.
I checked the other two callsites of userHasDeviceList:
- useCreateMessagesToPeersFromDMOp: in this case, it seems like the only change would be that we would stop requesting the device list for this user, which is probably a bad thing. Outside of that, in terms of sending messages to peers, an empty device list is the same as no device list: in both cases, no devices are added to peerUserIDAndDeviceIDs.
- usePotentialMemberItems: in this case, we would end up showing a user with no devices as an option to add to a thick thread. I think that's probably a bad thing too... this user will never see any messages sent to them, so it's better to prevent them from being added.
Agree with you, I was thinking about this in general, when we should use thick/thin threads - but in the cases you listed, indeed it's a bad thing, my comment was a misdirection.
Separately, I think there's a disconnect about the backup service. You mentioned backup in the last quote, and also this one:
If I recall correctly, we decided to do this before launching E2EE DMs because of the limitations of our architecture (no backup), there is no point in creating a thick thread in that case because the other user will never get information about this thread (has empty device list).
I think this will still be the case even after we launch backup. If there is no device to send the message to, then there is no way to send an encrypted message, right?
Yes, you're completely right. I just wanted to find an explanation why in the past we decided to check for device list length, not only existence because it was concerning for me... but the idea I came up with was wrong and unconsidered
@ashoat Do I understand correctly that the threads check is not needed and I just need to check for existence of device list instead of using userHasDeviceList?