feat: Implement Push Notifications sync

[GeekCornerGH]

Implements the Push Notification feature

Resolves #2469

Push-demo.webm

Special thanks to @samb-devel for your help

How to test:

Get your keys from the Bitwarden website
Set environment variables to the required values:

PUSH_ENABLED=true
PUSH_INSTALLATION_ID=CHANGEME
PUSH_INSTALLATION_KEY=CHANGEME

PUSH_RELAY_URI is already configured to use default Bitwarden server.
Start the server, then test.

This was my first shot at Rust.

[stefan0xC]

Thanks for the contribution. I have not looked at the changes yet but can you run cargo fmt and fix the errors reported by cargo clippy so the automatic checks won't fail?

[GeekCornerGH]

Thanks for the contribution. I have not looked at the changes yet but can you run cargo fmt and fix the errors reported by cargo clippy so the automatic checks won't fail?

Sure thing, will do it later

[GeekCornerGH]

Removed pretty much every error.
If someone can suggest changes to remove the last 2 ones, feel free to do so, not sure how to resolve them (removing return throw an error about response type for instance)

[BlackDex]

I'm looking at the code right now.
At a first glance the code looks ok, but have not looked detailed at it.

First things that i notice are the following.
There are two migrations done on the database, while that could be done in one go, which would make a down.sql a bit easier I think, and probably wanted.

Also, i have not checked what the effects are yet of this change, since there is a primary key on the old UUID and USER_UUID combined. I don't know if that will break stuff with this rename.
I still tend do keep uuid as is, and use push_token as a new column. Even though that deviates from upstream Bitwarden. But we can put that into the comments at the object.

I also see a lot of extra calls being done in regards to pushing these notifications.
Now every function which has a nt.send... also has a push_... in there.
Can't we merge this in some way and just have one entry point for sending out changes?
Like, and the push:: to the notifications.rs file?
That would solve future notification updates or additions to just add one, instead of two calls every time.

[BlackDex]

Some more reviews from my side.
I haven't fully checked everything detailed though.

[BlackDex]

@GeekCornerGH , my apologies already for my changes, hope it won't be to hard to merge hehe.

[tessus]

The SQL statements are wrong. I am not sure how they ever ran successfully during a migration.

The proper statement is ALTER TABLE bla ADD COLUMN ...

[GeekCornerGH]

Wooops, I guess git didn't caught the changes 😂 I'll rebase anyways

[GeekCornerGH]

@tessus since GitHub won't allow me to request 2 reviewers at one time, I just tag you haha, is everything fine now?

[tessus]

I think there are still 2 points outstanding, from other reviewers:

1. don't add an option in config.rs for PUSH_RELAY_URI (or at least it should not be public for now)
2. merge the function calls

ad 1) currently there is only one push server and setting it to anything other than the bw address won't work. It is too error prone to allow users to change this value. However, in the code the variable should still be used, since bw might introduce other servers or allow customers to setup their own push server (which would also require a change in he client code).

ad 2) currently there are always 2 fuction calls, as an example:

send_folder_update(...)
push_folder_update(...)

If I understood BlackDex's comment correctly, the second call should be added to the first one:

fn send_folder_update(...) {
    original code;
    
    if (PUSH_ENABLED) {
        push_folder_update(...);
    }
}

But I am not the right person to review the entire PR. Someone who actually knows Rust (I only dabble with it) and the code base will hvae to do a proper review.

[samb-devel]

Thank you @tessus for your remarks.

@BlackDex : could you please confirm that the code suggested by @tessus is what you inteded? Can we simply call push_folder_update(..) from within send_folder_update which is inside the impl WebSocketUsers block without any problem?

Thank you in advance for your feedback

[BlackDex]

Thank you @tessus for your remarks.

@BlackDex : could you please confirm that the code suggested by @tessus is what you inteded? Can we simply call push_folder_update(..) from within send_folder_update which is inside the impl WebSocketUsers block without any problem?

Thank you in advance for your feedback

My main point is i don't like to have two functions being called at all those functions while using the same arguments.
I actually have not yet looked into the best way to do this. Maybe we need to create an overlay on both, so that in the future it could be easy to add a new way. (Maybe even support HA notifications between instances for example)

But i do think the nt can be used for both to simplify and consolidate this.

[samb-devel]

@BlackDex thanks for the answer. I get your point. My main concern is that we're not very deep into rust development, We're happy to learn and do, we just need a bit of guidance on the best practices and on how Vaultwarden may develop.

Does it make sense to keep push calls and websocket notifications in two separate files, or should we merge them push.rs and notifications.rs?
Do you see any possible issue in calling the push_folder_update from within the impl WebSocketUsers block, or are there issues of any kind and that should be avoided?

[tessus]

Does it make sense to keep push calls and websocket notifications in two separate files, or should we merge them push.rs and notifications.rs?

I think this is up to you. Everyone has their preference. I don't see an issue to have 2 files. I suspect other reviewers would have already objected otherwise.

Do you see any possible issue in calling the push_folder_update from within the impl WebSocketUsers block, or are there issues of any kind and that should be avoided?

Personally I'd just go for it. It's just a bit of refactoring. But BlackDex also mentioned that a wrapper might be beneficial. However, as a first step, I'd just try adding it to the impl block and see what happens. ;-)

[tessus]

@GeekCornerGH It seems you are merging main into your PR. You need to rebase it. Currently your PR has so many changes that do not even belong to this PR that it makes reviewing it a real mess. e.g. you added 2 commits. None of them had anything to do with BlackDex's review. But I had to read all of it to find that out, and I might have missed somthing since BlackDex had quite a few comments.

[GeekCornerGH]

@GeekCornerGH It seems you are merging main into your PR. You need to rebase it. Currently your PR has so many changes that do not even belong to this PR that it makes reviewing it a real mess. e.g. you added 2 commits. None of them had anything to do with BlackDex's review. But I had to read all of it to find that out, and I might have missed somthing since BlackDex had quite a few comments.

I'm in fact using rebase, not merge. Also I missed BlackDex's comments, as my phone didn't showed the reviews themselves. Checking this rn

Edit: I guess the multiple commits are shown if you had the pull request page open and GitHub caught up the changes.

[tessus]

I'm in fact using rebase, not merge.

This is very strange then. Maybe I clicked on the wrong button. I think I clicked on Compare instead of the Files changed button at the top.

In that case my apologies. My bad.

[GeekCornerGH]

Latest changes were tested against 2 cloned AVD, running the latest apk 2023.05.0

[BlackDex]

Looks nice, some final tweaks i think.

[tetsumaki]

I think you should delete EXPOSE 3012 from many Dockerfile here : https://github.com/dani-garcia/vaultwarden/tree/main/docker

[BlackDex]

I think you should delete EXPOSE 3012 from many Dockerfile here : https://github.com/dani-garcia/vaultwarden/tree/main/docker

For now we don't, since we need a transition from the old to the new way of websockets. And we don't want to break current installations.