VPN-5932: Change where we save device model

[mcleinman]

Description

This bug existed for the following situation:

• Have an account with five devices.
• Sign in on a new device
• Remove a device from the account as part of the sign in flow
• End up on main screen
• Force kill app
• Reopen it
• Note that you need to sign in again - not what we expected

The list of valid devices comes from the Guardian, and is saved in local settings (as the device list JSON). Upon launch, we check if the current device is in those devices, and if not, we ask the user to sign in again.

When we sign into the app when an account already has max devices, we retrieve the device list from Guardian twice - once upon initial launch, and a second time after we remove a device and add our current one. However, we were only saving it to settings after that first retrieval. Thus, on subsequent launch, the current device wasn't in the saved list, signing the user out.

I updated where we save the device JSON to local settings - It's now before we check the current device. checkCurrentDevice returns false here, because the state is StateDeviceLimit - but we want to save the new devices anyway.

Note that we save the user data in the same spot, and I moved it as well.

Reference

VPN-5932

Checklist

• My code follows the style guidelines for this project
• I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
• I have performed a self review of my own code
• I have commented my code PARTICULARLY in hard to understand areas
• I have added thorough tests where needed

[vinocher]

For my understanding, could you please explain the call sequence? When the user signs in, I think MozillaVPN::completeAuthentication calls DeviceModel::fromJson to add the user's other devices. Then MozillaVPN::accountChecked calls DeviceModel::fromJson with all devices, including the current device.

AFAIK, MozillaVPN::maybeStateMain sets the StateDeviceLimit state, which is checked by checkCurrentDevice. But MozillaVPN::maybeStateMain seems to be called after MozillaVPN::accountChecked. If so, I'm not sure how this change fixes the issue. I'm likely misunderstanding something, so would appreciate some clarification.

[mcleinman]

For my understanding, could you please explain the call sequence? When the user signs in, I think MozillaVPN::completeAuthentication calls DeviceModel::fromJson to add the user's other devices. Then MozillaVPN::accountChecked calls DeviceModel::fromJson with all devices, including the current device.

AFAIK, MozillaVPN::maybeStateMain sets the StateDeviceLimit state, which is checked by checkCurrentDevice. But MozillaVPN::maybeStateMain seems to be called after MozillaVPN::accountChecked. If so, I'm not sure how this change fixes the issue. I'm likely misunderstanding something, so would appreciate some clarification.

While DeviceModel::fromJSON is where we get the data from the server, we save it to the device with a call to m_deviceModel.writeSettings();. If the current device is not in the list of devices saved to settings, we sign the user out. The bug is coming from not saving the list of devices correctly. (We're loading them from the server correctly.)

When a user already has 5 devices when signing in, they're prompted to remove a device. After the user selects which device to remove, TaskAccount is run. This, in turn, hits accountChecked. In accountChecked, we correctly get the account's devices (m_private->m_deviceModel.fromJson(keys(), json)). However, before we can save these devices (line 754: m_private->m_deviceModel.writeSettings();), we hit checkCurrentDevice. Since the current state is still StateDeviceLimit, checkCurrentDevice returns false, causing accountChecked to do an early return right before we would have written the device settings. Everything else continues fine on this run, as the device model in memory is perfect - it's the device model on disk that has the wrong state. This state bites us on the next launch - when we load the saved devices, find out the current device isn't there, and sign the user out.

Please let me know if any of this didn't make sense - happy to explain more/better.

[vinocher]

Thanks for the explanation!

[vinocher]

A comment here may be useful to explain why the device model is being written before checkCurrentDevice.

[mcleinman]

Good call, thanks. Just added some comments, with a link to the full explanation here.