mozilla-vpn-client
mozilla-vpn-client copied to clipboard
mozilla-mobile
Acknowledge and update client once device deletion push message is received
This is the companion ticket for https://mozilla-hub.atlassian.net/browse/PSP-406.
Note: it is part of this ticket to instrument this new feature so it can be monitored and validated after release.
Open questions:
- How are device deletions currently handled by the app? Are there any UI changes? Should we involve design here otherwise?
- Should we keep the polling as a fallback for device list update while this feature has not been validated?
┆Issue is synchronized with this Jira Task ┆author: Beatriz Rizental Machado
➤ Beatriz Rizental Machado commented:
- How are device deletions currently handled by the app? Are there any UI changes? Should we involve design here otherwise?
Currently, there are no UI changes. The user is logged out automatically.
[UPDATE]
Actually I just noticed this shows an “Authentication failed” alert on logout, which is not the most descriptive.
➤ Beatriz Rizental Machado commented:
{quote}Should we keep the polling as a fallback for device list update while this feature has not been validated?{quote}
There is no specific task that checks exclusively for device list. How it happens currently, if a certain device is deleted, TaskAccount will fail (here ( https://github.com/mozilla-mobile/mozilla-vpn-client/blob/main/src/tasks/account/taskaccount.cpp#L28 )), which will trigger an AuthenticationError which will reset the application.
Since TaskAccount is responsible for updating many other states other than device list, we cannot remove it by just implementing push for device deletion.
This mean that we will keep polling for this information anyways, so no worries for now.
➤ Beatriz Rizental Machado commented:
Instructions for QA
This ticked implemented a handler on the client for messages sent from the server with regards to device deletions on a given account. Take for example the following scenario:
- I have three (or more) different devices logged in to the same VPN account.
- I go on the “Device List” view in one of those devices and delete the one of the other devices.
- The device that was deleted should be immediatelly logged out, no error messages shown.
- The device that was not deleted should have it’s device list immediatelly updated.
➤ Bianca Hidecuti commented:
Beatriz Rizental Machado , verified this on the latest 2.10 builds and the following were observed:
Stage server: if being logged in on multiple devices with the same FXA account, and deleting one of them:
- while the VPN is OFF/ON on the device that was deleted, the user is immediatelly logged out, but the “Authentication error Try again” error message is displayed - attaching a video and logs as well. This happens on both mobile and desktop;
- the device that was not deleted has it’s device list immediatelly updated;
[^MozillaVPN-logs.txt]
!original-E97EEBDD-E65B-4FC9-B191-C5F28C6D52C5.mp4|width=444,height=960!
Production server: if being logged in on multiple devices with the same FXA account, and deleting one of them:
- while the VPN is OFF on the device that was deleted, the user not logged out until the VPN is turned ON or “My devices” screen is opened. After that, the “Authentication error Try again” error message is displayed. This happens on both mobile and desktop;
- while the VPN is ON on the device that was deleted, the user is immediatelly logged out, but the “Authentication error Try again” error message is displayed. This happens on both mobile and desktop;
- the device that was not deleted has it’s device list immediatelly updated;
Also, while testing the 2.10 builds, we encountered the following issue: https://mozilla-hub.atlassian.net/browse/VPN-2643 ( https://mozilla-hub.atlassian.net/browse/VPN-2643|smart-link ). Beatriz Rizental Machado , do you think it could be related with the changes made for this ticket?
Giving the above, I am reopening this issue until further details are provided.
➤ Beatriz Rizental Machado commented:
Bianca Hidecuti , thank you so much for such a quick verification. From the logs it seems the websockets feature is not turned on. It is totally my fault for no adding that as an instruction to my write up earlier. Would you mind checking if it is turned on and if not turn it on and run the checks again? Thanks!
➤ Beatriz Rizental Machado commented:
Actually, Bianca Hidecuti I have just filed a ticked to turn that feature on by default wich should land between today or tomorrow. If you’d prefer to wait, here is the ticket https://mozilla-hub.atlassian.net/browse/VPN-2646 ( https://mozilla-hub.atlassian.net/browse/VPN-2646|smart-link )
➤ Bianca Hidecuti commented:
Beatriz Rizental Machado , no worries. The websocket feature is indeed unchecked. Will recheck again tomorrow.
Thank you!
➤ Bianca Hidecuti commented:
Beatriz Rizental Machado , reverified this ticket with the WebSocket feature enabled.
On stage server, the user is immediatelly logged out, but the “Authentication error Try again” red banner is still displayed, on both mobile and desktop.
However, on production server, the user is not logged out until the VPN is turned ON (if initially was OFF, or if it was ON, it needs to be turned OFF and then ON again in order to be logged out) or until “My devices” screen is opened. The banner is still displayed as well. Beatriz Rizental Machado , should we file a new issue for this behavior or should we track it here? Thank you.
Attaching some logs as well for the banner being displayed on stage and the user not being logged out on production.
[^device removal prod.txt] [^MozillaVPN_Logs_2022-39-03-9-39-23.txt]
Reopening this issue until further details are provided.
➤ Beatriz Rizental Machado commented:
Thank you Bianca Hidecuti , that is weird behaviour. Thank you for reopening the issue. I will investigate this tomorrow.
➤ Beatriz Rizental Machado commented:
Alright, I took longer than I initially planned to to get to this. However, I see from the logs that your development VPN client was never able to connect to the Guardian websocket, that would mean this is a server issue and not a client issue. I have filed a bug to look into that and will block this one until that is fixed.
With regards to the production issue, this is not expected to work in production so that is an unrelated bug. It will probably be fixed passively by this work, but it is definitely worth filing an issue just in case.
➤ Valentina Virlics commented:
Beatriz Rizental Machado We tracked the production behavior here https://mozilla-hub.atlassian.net/browse/VPN-2666 ( https://mozilla-hub.atlassian.net/browse/VPN-2666|smart-link ) . We will follow up on it after this work is done. Thank you!
➤ Beatriz Rizental Machado commented:
Hey Bianca Hidecuti and Valentina Virlics with https://mozilla-hub.atlassian.net/browse/PSP-497 ( https://mozilla-hub.atlassian.net/browse/PSP-497|smart-link ) fixed this is once again unblocked for QA 🙂 Note that this feature is not expected to work on production, so it should only be tested on the staging environment.
➤ Bianca Hidecuti commented:
Hi Beatriz Rizental Machado ,
Verified this across platforms on the latest 2.10 builds, and the following were observed:
- On MacOS / Windows - if the device removal is made from another desktop device, the user is logged out and no banner is displayed. However, if the device removal is made from a mobile device, the “Authentication” error is sometimes displayed;
- On iOS / Android - the user is logged out and the “Authentication” error message is displayed 2 out of 5 times;
Beatriz Rizental Machado , should we reopen this issue or is this working as expected? Thank you!
Attaching some logs as well.
[^mozillavpn authentication errortxt.txt]
➤ Beatriz Rizental Machado commented:
Hi Bianca, yes this is expected to work most, but not all of the time. There is a race condition between the two implementations of device deletion. For now, that is acceptable.
Nevertheless, I will file a ticket to see if we can lower the occurances of it for 2.10. That would not be a blocker for release though 🙂 So I’ll go ahead and close this one as Done. Thanks a lot for the quick turnout here!
➤ Bianca Hidecuti commented:
Beatriz Rizental Machado , thanks for confirming.
Marking this verified as fixed.