Add mobile push device filter to non-null push uuid

[quexten]

Partially fixes #3577.

Over time, old devices accumulate in the SQL device table. Even when the device push uuid is NULL, a push request is sent for them when mobile push is active. For my server, this lead to ~100 requests to the Bitwarden push servers without a single active mobile push client, and a request time of ~15 seconds.

This requests adds a filter to prevent mobile push notifications from being sent to clients who did not even register for mobile push.

[GeekCornerGH]

I actually made a function filtring push devices, you'll want to use this

Thanks for the catch tho

[BlackDex]

This probably also needs to be done for the push_logout.
And using a function which runs the query that doesn't even returns those records would be better indeed.

[quexten]

Ah, nice. I'll look into using that function.

[BlackDex]

@quexten , if you check my comments on the other PR, it might be better to merge the two and move the send_to_push_relay(data).await call either outside of the for loop, and send a vec![data], so that the send_to_push_relay function can handle all the calls within the same spawned call.

[quexten]

Thank you both for the helpful suggestions. Updated the PR to have send_to_push_relay take a Vec, and updated all of the calls to send_to_push_relay to be wrapped in a spawned task. Furthermore, as per @GeekCornerGH's suggestion I used the find_push_device_by_user where possible, so that the SQL query does not even return those devices.

On my local system, there is now no noticeable difference in the time the PUT request takes (4-5 ms total whether it's enabled or not) with the full ~100 old devices from my database.

[BlackDex]

@quexten Could you rebase your PR on the current main branch please :)

[quexten]

@quexten Could you rebase your PR on the current main branch please :)

Done.

[quexten]

Ah actually, seems like I need to do some more changes.

[BlackDex]

Well, there are some clippy changes needed.

But i just tested it quickly, and it doesn't seem to update all the devices for me for some reason.

[quexten]

Well, there are some clippy changes needed.

Yeah, was working on that at the moment.

But i just tested it quickly, and it doesn't seem to update all the devices for me for some reason.

Okay, I will do some testing, good catch.

[BlackDex]

I am using the v2023.5.0 version of the app on Android. And it looks like it doesn't send the device id's anymore.

I have not tested the v2023.4.0 again yet. But that might be a thing

[quexten]

I am using the v2023.5.0 version of the app on Android. And it looks like it doesn't send the device id's anymore.

Do you mean the android app itself does not send the ids to the server, whereas before it did?

Does that also mean it does not currently work with the vaultwarden main branch for you?

[BlackDex]

@quexten @GeekCornerGH
Ok, i have figured it out i think by some debugging.

Instead of pushing to all those items, it in fact should be one, BUT, all the data in the payload should contain the information of the client creating the change.

You only need to check if there are devices which have a push token for that user, if that is the case, then call the Bitwarden push server with the data from the cipher and the data from the acting_device.

I probably need to check some other stuff, like organizations for example.

This also explains why it worked for me in the beginning, because i probably had multiple devices with different device id's and push_tokens already and the code back then didn't in time matched one of the devices i was using. Because there wasn't a match for the specific Android device, the clients processed the request.

The clients skip a call to refresh if they see there own device id in the call, and that is what probably is the clue here.

[quexten]

So, deviceId and identifier should both be the UUID of the acting device? In that case why are there even different push uuids saved in the DB? As far as I can tell, if we use the acting UUID in the push notification, we would then only use the push UUID's in the registration, though I haven't studied the protocol closely.

I'll try this later today.

[BlackDex]

So, deviceId and identifier should both be the UUID of the acting device? In that case why are there even different push uuids saved in the DB? As far as I can tell, if we use the acting UUID in the push notification, we would then only use the push UUID's in the registration, though I haven't studied the protocol closely.

I'll try this later today.

Well it was an oversight i think. We still need to store them for user/device specific actions maybe like logout and the push token storage.

[GeekCornerGH]

So, deviceId and identifier should both be the UUID of the acting device? In that case why are there even different push uuids saved in the DB? As far as I can tell, if we use the acting UUID in the push notification, we would then only use the push UUID's in the registration, though I haven't studied the protocol closely.

I'll try this later today.

My best bet would be that Bitwarden is using uuids as a way to uniquely identify any device, instead of searching per mail then per identifier (as they call them)

[BlackDex]

So, deviceId and identifier should both be the UUID of the acting device? In that case why are there even different push uuids saved in the DB? As far as I can tell, if we use the acting UUID in the push notification, we would then only use the push UUID's in the registration, though I haven't studied the protocol closely.
I'll try this later today.

My best bet would be that Bitwarden is using uuids as a way to uniquely identify any device, instead of searching per mail then per identifier (as they call them)

That is what i think to, and we still need it during register

[BlackDex]

@quexten , did you tested multiple devices? And what where you results?

[quexten]

@quexten , did you tested multiple devices? And what where you results?

I tested with 2 mobile devices, in one case with making the changes in my web extension, in one case on one of the mobile devices, and they did get updated correctly (for editing, deleting, adding a cipher).

I recommend to test the newest commit to see if your case is also resolved.

[BlackDex]

Ok, I did some more testing. While these latest changes seem to be in line with what Bitwarden it self sends out, it still doesn't work with multiple devices for me.

But, if i test this on a Self-Hosted or even on there SaaS system, that doesn't even work.
It seems it round-robins the push requests, so, one update goes to device 1, then next update goes to device 2 etc.. etc...

When i force a logout, it seems to only reach one device.
So, how hard we will try, it will probably never get work no matter what we adjust on our side 😄 .

But since i think these current changes are better in line into upstream, and we are not spamming the Push server with these changes, that would be better.

If someone has other thoughts, please let me know.

[tessus]

When I look at the current code in this PR, it calls find_push_device_by_user which returns a Vec, but then there is no loop, but just one call to send_to_push_relay. So maybe I am missing something here, but this seems weird.

Also, if this doesn't work with bw's self-hosted option, shouldn't there already be an issue open for this?

Update:

Shouldn't this:

if !Device::find_push_device_by_user(user_uuid, conn).await.is_empty() {

be rather something like:

vec = find_push_device_by_user(user_uuid, conn).await;
iterate over vec and push to all devices in vec

Additionally, shouldn't the function be called find_push_devices_by_user? Isn't the function supposed to find all push devices for a given user?

[quexten]

Ok, I did some more testing. While these latest changes seem to be in line with what Bitwarden it self sends out, it still doesn't work with multiple devices for me.

But, if i test this on a Self-Hosted or even on there SaaS system, that doesn't even work. It seems it round-robins the push requests, so, one update goes to device 1, then next update goes to device 2 etc.. etc...

When i force a logout, it seems to only reach one device. So, how hard we will try, it will probably never get work no matter what we adjust on our side smile .

But since i think these current changes are better in line into upstream, and we are not spamming the Push server with these changes, that would be better.

If someone has other thoughts, please let me know.

Weird, for me it still consistently works with both mobile devices, with the android one being 2-3 seconds slower than the iOS device which is nearly instant. I tested modifying ciphers and de-authorizing sessions. Is the behavior for you inconsistent, i.e does the device the changes reach change?

When I look at the current code in this PR, it calls find_push_device_by_user which returns a Vec, but then there is no loop, but just one call to send_to_push_relay. So maybe I am > missing something here, but this seems weird.

Also, if this doesn't work with bw's self-hosted option, shouldn't there already be an issue open for this?

Update:
Shouldn't this:

if !Device::find_push_device_by_user(user_uuid, conn).await.is_empty() {

be rather something like:

vec = find_push_device_by_user(user_uuid, conn).await;
iterate over vec and push to all devices in vec

Additionally, shouldn't the function be called find_push_devices_by_user? Isn't the function supposed to find all push devices for a given user?

That was what was initially implemented, yes, but as @BlackDex points out ( #3578 (comment) ), that does not appear to be what the official server is doing? It seems it is just one push notification with the acting device, and the push server itself then sends the message to all? devices of the user. (I have not looked closely at the official server implementation yet).

I should probably look into changing

find_push_device_by_user which returns a Vec,
this to just returning true/false though if we only need to know whether the user has a push device.

[BlackDex]

When I look at the current code in this PR, it calls find_push_device_by_user which returns a Vec, but then there is no loop, but just one call to send_to_push_relay. So maybe I am missing something here, but this seems weird.

My question was more technical in the sens of the workings. But this it would be much better if it would just return a bool, or int, by using a COUNT for the query for example, that is much quicker, and safes resources.

Also, if this doesn't work with bw's self-hosted option, shouldn't there already be an issue open for this?

Update:

Shouldn't this:

if !Device::find_push_device_by_user(user_uuid, conn).await.is_empty() {

be rather something like:

vec = find_push_device_by_user(user_uuid, conn).await;
iterate over vec and push to all devices in vec

We determined that this was not the case, and that was reverted because of that.

Additionally, shouldn't the function be called find_push_devices_by_user? Isn't the function supposed to find all push devices for a given user?

As mentioned above, it should probably be renamed, or maybe a new function should be created.

But, what i wanted to establish here is that this is just the way it seems to work. At least for me currently for Vaultwarden, Self-Hosted and Bitwarden Hosted instances.

If that is the case, then I'm generally ok with the workings of this PR. And then i can review it as is with some of the above mentioned items :)

[quexten]

As mentioned above, it should probably be renamed, or maybe a new function should be created.

It seems the function is required by the de-auth functionality in admin.rs so I have just renamed it to mention devices in plural. Additionally I added a database query to just check whether there are any push devices for a user with a boolean return value. Deploying to my test server now to see if everything works as it should.

[quexten]

Okay, it seems to work on the latest build, however I also encountered an issue with not syncing. After a forced deauth, I signed my devices back in, but no changes were pushed because they did not have a push token / uuid anymore. The de-auth had cleared their push tokens in the database (as it should) but no re-registration happened. When I completely cleared the app-data and signed back in, the registration happened again and push worked again.

I suspect (but have not yet investigated) there is a problem somewhere in re-registration. Maybe just dropping all device entries for the user on a forced de-auth would be enough? Would this cause any other issues?

Device::delete_all_by_user(&user.uuid, &mut conn).await?;

There is this line in the deauth-function, but for some reason for me the table still contains my devices after a de-auth...

[BlackDex]

It did removed all devices for me.
It also logged out at least one mobile device.

But, as mentioned, it looks like it's a strange issue with the push server. It might even be our registration or something. But it's hard to debug.
But since it also happens for me on vault.bitwarden.com, it probably is not something we can fix server side.

[quexten]

Ok, well aside from this weird issue, I'm happy with the state of the PR, so feel free to review when you find the time :)

[BlackDex]

There are some small items to address i think.

Also, i noticed that all the payload keys are starting with a capital letter.
I'm not seeing that in the calls and code from Bitwarden it self.

Maybe best that we change those to lowercase also?

[quexten]

FYI I have tested on bitwarden.com and sends are also not syncing there. I have created an upstream bug report in case they are not aware (there was no other issue tracking this).

[quexten]

Also, i noticed that all the payload keys are starting with a capital letter.
I'm not seeing that in the calls and code from Bitwarden it self.

Changed them to lower-case now.

[BlackDex]

I think it all looks good.
Just one item which needs to be removed.

And, if possible if you can squash this PR that would be best i think.
Probably best way would be to run this i think.

# First reset to the beginning
git reset --soft HEAD~13

# Remove the bw_web_builds folder
rm -rf bw_web_builds

# Check if all is ok and do a force push by using a + sign infront of your branch-name
git push origin +my_branch_name

[quexten]

Sorry for the confusion, I think it should be correctly reset now?

[BlackDex]

Sorry for the confusion, I think it should be correctly reset now?

Looks ok indeed.

[GeekCornerGH]

I guess that's good, didn't tried yet, but changes seems alright

[tessus]

@BlackDex ok, so let me try to summarize the architecture for push notifications

1. the device is registered at the push server with user_id and device_id (but when?)
2. if there is a notification to be pushed, the device sends a ping to the push server with user_id and device_id
3. the push server searches for all device_ids registered for user_id
4. the push server sends out a ping to all previously found device_ids to refresh their data, except to the one device that sent the ping

Is this the current workflow?

If so, what do I have to do to get the devices registered after I upgrade to the code that includes this PR? De-auth all devices? logout/login?

[BlackDex]

@BlackDex ok, so let me try to summarize the architecture for push notifications

1. the device is registered at the push server with user_id and device_id (but when?)

During login at the device.

2. if there is a notification to be pushed, the device sends a ping to the push server with user_id and device_id

The server sends a notification. So it will send a web-socket message to all web-socket clients and a message to the push server of Bitwarden.

3. the push server searches for all device_ids registered for user_id
4. the push server sends out a ping to all previously found device_ids to refresh their data, except to the one device that sent the ping

This part is not something i have looked into that deeply. As this is more under Bitwarden control. And it doesn't seem that it will search and notify all devices which contain the users-id. Because for me multiple devices didn't received the update message. But this is the same for vault.bitwarden.com.

Is this the current workflow?

If so, what do I have to do to get the devices registered after I upgrade to the code that includes this PR? De-auth all devices? logout/login?

For me, during testing, i needed to fully clear the old devices and also clear the data on the mobile device and login again.

I know that Bitwarden is looking into new ways to handle these notifications, because the currently way is stressing there current infrastructure.

[tessus]

Btw, the PR still shows that changes are requested. Isn't it ready to be merged?

[BlackDex]

Btw, the PR still shows that changes are requested. Isn't it ready to be merged?

I want to test it first. Didn't had time yet.