doy / rbw

unofficial bitwarden cli
https://git.tozt.net/rbw
Other
581 stars 83 forks source link

Implement basic websocket support #115

Closed quexten closed 1 year ago

quexten commented 1 year ago

This PR adds basic support for Websocket notifications. It still far from ready for merging but I'm opening it as a draft PR in case anyone has comments.

Related issue: https://github.com/doy/rbw/issues/112

The PR currently does a full sync whenever an item changes (though the parsing part of which item changed is not too difficult, so this could be improved later).

Furthermore, it also implements the logout message, sent when de-authorizing sessions in the web UI.

The code is structured, such that adding more message types (passwordless login request) should not be too difficult.

If the server does not support Websockets, it silently fails. The connection is tried when starting the agent, and when a login is successful.

9ary commented 1 year ago

High level comment from a quick look at the code: it seems you only attempt to connect to the websocket at agent startup and after a successful login, discard all errors, and never attempt to reconnect. Is that right?

quexten commented 1 year ago

Yes, your understanding is correct.

I tested disconnection by cutting off the network connection on my laptop for a minute, and then re-enabling it, and after a few seconds it seemed to resume the subscription. I.e by default there does not seem to be a timeout. I'm not sure what would happen if the connection is closed on the server side though.

The read future of the websocket never returned for me from timeout by default. After looking into it, I think I need to create a TCP stream, set the timeout on it and pass it to tungstenite, which should make the read future close after the timeout, at which point re-connection should be attempted.

By the way, I'm open to changing the connection behaviour. Currently, as you point out, it is on startup of the agent, and on login. But if the Websocket connection initially fails, should it be reattempted every x minutes?

9ary commented 1 year ago

By the way, I'm open to changing the connection behaviour. Currently, as you point out, it is on startup of the agent, and on login. But if the Websocket connection initially fails, should it be reattempted every x minutes?

I think exponential backoff (which resets after a successful sync) should work. A config option to turn websocket functionality off entirely would also be good in combination with that. A heuristic could also be used to detect when sync is possible but the websocket is not available.

I tested disconnection by cutting off the network connection on my laptop for a minute, and then re-enabling it, and after a few seconds it seemed to resume the subscription. I.e by default there does not seem to be a timeout. I'm not sure what would happen if the connection is closed on the server side though.

This may not be enough to kill a TCP connection unfortunately. As it turns out, those can survive a long time as long as addresses stay the same, among other things.

doy commented 1 year ago

maybe a useful thing to do would be to hook into the existing timer mechanism - instead of just ignoring sync errors from the timer entirely, check to see if the timer-based sync succeeded, and if it did and the websocket is not currently connected, try reconnecting the websocket?

quexten commented 1 year ago

maybe a useful thing to do would be to hook into the existing timer mechanism - instead of just ignoring sync errors from the timer entirely, check to see if the timer-based sync succeeded, and if it did and the websocket is not currently connected, try reconnecting the websocket?

The latest commit implements this.

quexten commented 1 year ago

this overall looks pretty good! just a few minor fixes. can you also run cargo fmt?

Formatted in latest commit.

doy commented 1 year ago

thanks!