Address ENG-9528.
Added a hook that performs authoritative keyserver and visual logout. It should be called when Identity is already logged out.
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/actions/user-actions.js | ||
|---|---|---|
| 283 ↗ | (On Diff #45862) | Leftover |
Comment Actions
I have two concerns about this diff:
- Inline comment
- When this hook is called, the device is immediately logged out without notice. This might be a bad user experience, so I thought about displaying an alert/modal saying sth like "This device has been externally logged out. Please log in again". On the other hand, when this device was logged out by the primary device, the user is expecting all devices to log out, so maybe the UX is not that bad.
| lib/actions/user-actions.js | ||
|---|---|---|
| 282–294 ↗ | (On Diff #45862) | The hook is supposed to do:
Visual logout is the most important, so I was wondering about doing try {
await callKeyserverLogout()
} catch {}to always succeed. However I don't understand impact of the action's return value (LogOutResult type) and I don't know what to return when keyserver logout fails. |
Comment Actions
When this hook is called, the device is immediately logged out without notice. This might be a bad user experience, so I thought about displaying an alert/modal saying sth like "This device has been externally logged out. Please log in again". On the other hand, when this device was logged out by the primary device, the user is expecting all devices to log out, so maybe the UX is not that bad.
Hmm... that's an interesting question. I'm not immediately sure what would be better. Can you clarify in what scenarios (both currently as well as in the future) that we'll trigger UnauthorizedDevice? If it's always initiated by the user on their primary, then I don't think we need an alert. But if there's other cases where the user might not already have context, then I think it might be worth implementing an alert. (We could also consider differentiating these two cases in some way, such as using different error codes.)
| lib/actions/user-actions.js | ||
|---|---|---|
| 282–294 ↗ | (On Diff #45862) |
Visual logout actually begins on logOutActionTypes.started, which flips dataLoaded.
While visual logout will be initiated either way since it's based on logOutActionTypes.started, it's still important that was guarantee that logOutActionTypes.success gets called. This is why the other actions all swallow errors like you've described.
The significance is that it becomes the payload for the logOutActionTypes.success action dispatched on the next line. Since this is primarily a keyserver logout, have you tried modelling it after the keyserverLogOut function? |
| 284 ↗ | (On Diff #45862) | The other logouts all have a Promise.race to guarantee that the logOutActionTypes.success action eventually gets dispatched. Why'd you skip it in your new one? |
Comment Actions
There are two scenarios:
- User issued this on primary device
- Primary device logout
- Primary backup restore protocol
- Logout the secondary device via device list UI menu
- Password reset by Comm staff (performed upon user's request)
In my opinion, in all these cases the user is aware that the device should now be logged out.
| lib/actions/user-actions.js | ||
|---|---|---|
| 282–294 ↗ | (On Diff #45862) |
That's right, I confused these two.
I mean, I'm aware that return value is the action payload. I didn't understand the impact of the payload in reducers where it's used. Either way, as stated in my above comment, my concern is solved because keyserverLogOut already swallows errors. |
| 284 ↗ | (On Diff #45862) | In useLogOut() The Promise.race is called for Identity promise, while keyserver promise is ran in Promise.all. But I didn't notice that callKeyserverLogOut() already swallows errors in the keyserverLogOut call, so it never fails here |