register missing push devices at login

[stefan0xC]

I've been looking into why enabling push notifications is not enough to get automatic sync to work and why it requires removing the app (or clearing the app data) and as far as I understand this deep dive that is because the endpoint on vaultwarden that registers the token to Bitwarden's Azure Notification Hub is currently only called when the OnNewToken() method is run. (Which only happens in rare instances, i.e. when installing the app or clearing the app data.)

If we save the push_token even when PUSH_ENABLED=false we could register missing push devices later. I've implemented a crude way of doing it (looking for a user's not yet registered devices on login, once mobile push notifications are enabled).

The push_uuid of a device will only be filled when push is enabled, so we know which push devices have not been registered yet.

Note: I'm not sure if there's a way to register the wrong token (which is why I'm skipping devices that have been added before this patch - and also because I don't know what values would be acceptable here). According to the deep dive the mobile clients (both on Android and iOS) should check daily if their token are still correct and will update them if not but I'm not sure what exactly happens in that case. - I'm currently testing what happens if you register the wrong token (if it will start working after 24 hours or if it will keep being broken). If this does not work this whole approach would probably not be viable.

Also since I don't know the API limits of the Bitwarden push relay and I did not want to abuse it (and maybe there's a better way to call the method) I've limited the number of devices that would be processed to <= 2. I think we could probably improve this a bit but I wanted to get some feedback first if this would be a viable approach. (Instead of calling it on login of a user it migh be better to do that on startup for all unregistered devices. Not really sure what is preferable here...)

[BlackDex]

Any progress on this @stefan0xC?

Did you found anything useful too this?

[stefan0xC]

Registering the wrong token does not seem to do anything as far as I've tested it. So I'm not sure if this would be an issue or if it would still work if there is much time between storing the device token initially and enabling push notifications. Worst case: it does not work and we would have to do the same steps that are necessary right now to get it to work.

I mean, it could also be an issue if you have a lot of stale devices stored that login takes forever with needlessly registering all saved mobile devices once you enable push notifications. So I'm not sure if we should register more than 2 devices at login.

So all in all the patch as it currently is should work so in the future you would not need to clear the app data or reinstall the app after enabling mobile push notifications. However this would only affect devices that are connected after this patch has been applied and with the caveat that it might not work if too much time has passed.

edit: I'll update my PR and mark it ready for review once I'm home.

[BlackDex]

@stefan0xC
Hmm...

I'm not sure what is happening here.
Shouldn't the devices them self provide a push-token, which the server then needs to check-in into the identity/push endpoint with the correct push-uuid?

If that is the case, and someone does not have push enabled, but we do store the push-token to be sure we have it in the future when someone does enable it. We can then also just tell people to logout and login again to make sure there device gets registered at the push servers. That way, we only have to check if that device has a push-token already, but not a push-uuid, and if push is enabled, then register that specific device, instead of registering all the devices.

That way we also do not accidentally register inactive devices, and only the device which the user at the moment actually uses.

I'm still not sure when Bitwarden sends those push-tokens to the server actually. Is that only on a fresh install, are there other triggers, i really do not know, that would help.

Besides this, I'm thinking of making some adjustments to the admin user page for which i was thinking about for a long time already, but now with these individual devices item it makes it a more needed item.
I'm planning on adding a pop-over/modal which shows all devices registered to a user with there individual last usage, but that can now also show if that device has a push-token and a push-uuid of course. That probably helps the users (well admins actually) to check and verify if there device should be able to receive push events.

We might even be able to add a button there to re-register the device upstream if that device was just in the like last x days or week or so, to also prevent devices not being used getting registered.

Those are just some head-spins from me.

[BlackDex]

it could also be sent when you login. If that is the case it would make this a lot easier, indeed.

How did we all have missed that haha.
Looks like you have some new testing to do 😂

[stefan0xC]

Alas, the device token is not set on login so the mobile client will just sent an empty string.

device_push_token: Some("")

[tessus]

Is something still missing in this PR? I have been playing with push notifications and registering the devices upon login could be very useful.

[stefan0xC]

I still have to rewrite the PR so only the device that is used to login is registered (if a device push token has been saved).

[stefan0xC]

Okay, I think I've finished rewriting this PR. I've also added a warning when the device registration will be skipped (because of an empty push token field) so some users would have an indication of what to do.

The only thing I'm currently unsure about would be whether the behavior of the iOS app differs greatly from the Android app and if we should improve put_device_token (i.e. add a check if the transmitted token is the same as the already saved token before unregistering)...

[stefan0xC]

I've added the check I was thinking of so it should be fine now. (The added check to unregister the token might be unnecessary if put_device_token is only called once but just in case it's not this should work now even for iOS.)

[BlackDex]

@stefan0xC, it looks all ok, just needs a rebase i think :).