[PS-1201] Allow bw sync without unlocking

[TomiBelan]

Type of change

- [ ] Bug fix
- [X] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

The CLI command bw sync will run even if the vault is locked. You only need to be logged in. This makes it easier to use bw sync in automated scripts.

As far as I can tell, bw sync doesn't use the master password at any point, so there is no compelling reason to ask for it.

Potentially related or similar issues: #2739, bitwarden/cli#91, bitwarden/cli#423

Code changes

• program.ts: Replace the unlock check.

I think that's it. It seems to me that sync.command.ts and sync.service.ts don't need changing.

(I noticed sync.service.ts calls cryptoService, but apparently not to decrypt anything.)

Rationale

• As far as I can tell, the Bitwarden browser extensions (apps/browser/src/background/main.background.ts) and desktop app (apps/desktop/src/app/app.component.ts + apps/desktop/src/main/messaging.main.ts) are already able to sync in the background when the vault is locked. If that is correct, then this is nothing new, it just adds the same power to the CLI.
• Edited to add from comment: Another reason why bw sync should work without a password is that you can already do it without a password using bw serve + curl -X POST http://localhost:8087/sync.
  This is because the /sync handler in serve.command.ts does not call errorIfLocked. bw sync should be made consistent with bw serve and also work without unlocking.

Before you submit

- [X] I have checked for **linting** errors (`npm run lint`) (required)
- [ ] I have added **unit tests** where it makes sense to do so (encouraged but not required)
- [ ] This change requires a **documentation update** (notify the documentation team)
- [ ] This change has particular **deployment requirements** (notify the DevOps team)

[CLAassistant]

All committers have signed the CLA.

[TomiBelan]

Can someone please review this PR?

I'm guessing @MGibson1 or @eliykat might be the right reviewer for this area. If not, please redirect.

By the way, another reason why bw sync should work without a password is that you can already do it without a password using bw serve + curl -X POST http://localhost:8087/sync.

[dbosompem]

Hi @TomiBelan , thank you for the contribution, and apologies for the delay in reaching out. We've added this to our internal community PR board for review and we will revert soon.

[TomiBelan]

Any updates? It's been 40 days. :(

[dbosompem]

Hi @TomiBelan, contributor PRs do pass through an internal process when submitted. Whiles I can admit that this has taken a bit long, kindly be rest assured it will be attended to, very soon. Thank you for your patience.

[TomiBelan]

Thanks for replying. I'll look forward to it.

I'll also note the patch is just 1 line long, although of course that's not a perfect indicator. :)

[TomiBelan]

Hello again. Today it's two months since I sent this trivial single line patch. What's going on???

I'm just confused: the repo is clearly quite active, and internal PRs are reviewed and merged smoothly. But no community PRs were merged in almost a month. @dbosompem mentioned a process for community PRs --- did the process get bogged down somehow? Or is there some other reason for such extreme delays? I'd like to at least understand what's the bottleneck.

[dbosompem]

Hi @TomiBelan, it's rather unfortunate we haven't merged in your PR yet, though we've had a few community PRs merged in the past months. Yes I mentioned earlier that we do review PRs internally. The code change with this PR is quite small but will require a high QA load and so we want to make sure we get QA capacity to be able to see this through, successfully. Apologies we haven't updated you since, but we'd try and expedite the process. Thank you for your patience!

[TomiBelan]

Thanks for replying and for explaining that the issue is with QA capacity. It sounds like someone did look at the PR at least to determine that. I wish they'd comment at the time. I got frustrated by the lack of updates.

If you don't mind me asking, what exactly about this feature needs QA capacity? Isn't the test plan simply: "1. run bw sync without giving a --session parameter or BW_SESSION variable. 2. check that it succeeded."?
The docs can also stay as is. And sync without unlock was already available with bw serve so it's just easier access to existing functionality. It's possible there is complexity I'm not aware of, so I'm curious to hear about it.

Just to clarify, I searched other community PRs on this page.

[MGibson1]

@TomiBelan, you're right. I made a mistake in estimating the impact of this change. I had forgotten that syncs in CLI are strictly manual and was thinking it was tightly coupled to many other commands when it's not.

Thanks for pointing that out and for the contribution -- this is definitely closer to how sync should work for the CLI.

[TomiBelan]

Yay! Thanks @MGibson1.