bitwarden_rs: 1.9.1 -> 1.13.0 #74927

Br1ght0ne · 2019-12-03T18:11:42Z

Motivation for this change

https://github.com/dani-garcia/bitwarden_rs/releases/tag/1.13.0
MySQL and (initial) PostgreSQL support have been added, and are accomodated in this PR. The default DB is still SQLite (to preserve backwards compatibility).

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)

/nix/store/rvmzjirrbdmy9n042ssk72qkami10yij-bitwarden_rs-1.9.1	  47.2M

Ensured that relevant documentation is up to date
Fits CONTRIBUTING.md.

Notify maintainers

cc @msteen

This change is

msteen · 2019-12-03T20:38:37Z

Thank you for updating bitwarden_rs! I have not yet tested the changes myself, but I have looked over the code changes and they look good to me. However what are the implications on the corresponding bitwarden_rs module? Doesn't the backup script and its service PATH need to change depending on the database backend used?

Ma27

Building bitwarden_rs breaks for me with the following failure:

bitwarden_rs>    Compiling bitwarden_rs v1.0.0 (/build/source)
bitwarden_rs>    Compiling subtle v2.2.2
bitwarden_rs> error: You need to enable one DB backend. To build with previous defaults do: cargo build --features sqlite
bitwarden_rs>   --> build.rs:12:5
bitwarden_rs>    |
bitwarden_rs> 12 |     compile_error!("You need to enable one DB backend. To build with previous defaults do: cargo build --features sqlite");
bitwarden_rs>    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
bitwarden_rs>    Compiling data-encoding v2.1.2
bitwarden_rs>    Compiling quoted_printable v0.4.1
bitwarden_rs> error: aborting due to previous error
bitwarden_rs> error: could not compile `bitwarden_rs`.
bitwarden_rs> warning: build failed, waiting for other jobs to finish...
bitwarden_rs> error: build failed
bitwarden_rs> builder for '/nix/store/znfdg3299j5mhq87y1p6mdxlc92l41vg-bitwarden_rs-1.13.0.drv' failed with exit code 101

Br1ght0ne · 2019-12-05T02:06:13Z

@msteen I updated the module (didn't implement alternative backup strategies yet).

nixos/modules/services/security/bitwarden_rs/default.nix

Co-authored-by: msteen <msteen@users.noreply.github.com> Co-authored-by: Daniel Løvbrøtte Olsen <daniel.olsen99+GitHub@gmail.com>

Rolling out initial support for: - mysql (1.10.0) https://github.com/dani-garcia/bitwarden_rs/releases/tag/1.10.0 - postgresql (1.11.0) https://github.com/dani-garcia/bitwarden_rs/releases/tag/1.11.0 As current backup strategy is sqlite-specific, more work is needed. For now, an assertion to make users know those wont work.

Ma27 · 2019-12-30T15:38:38Z

Ah sorry, I totally forgot about that one! (next time feel free to ping me!).

Will review/test it locally now :)

msteen

Since we are hardcoding the use of sqlite for now, it is better not to commit the work in progress done in the module, none of those changes are necessary at the moment.

Ma27

The module currently breaks with the following error:

error: anonymous function at /home/ma27/Projects/nixpkgs/pkgs/tools/security/bitwarden_rs/default.nix:1:1 called with unexpected argument 'dbBackend', at /home/ma27/Projects/nixpkgs/lib/customisation.nix:69:16
(use '--show-trace' to show detailed location information)

Ma27 · 2019-12-30T15:57:44Z

pkgs/tools/security/bitwarden_rs/default.nix

  nativeBuildInputs = [ pkgconfig ];
  buildInputs = [ openssl ] ++ stdenv.lib.optionals stdenv.isDarwin [ Security CoreServices ];

  RUSTC_BOOTSTRAP = 1;

-  cargoSha256 = "0p39gqrqdmgqhngp1qyh6jl0sp0ifj5n3bxfqafjbspb4zph3ls4";
+  cargoSha256 = "03szl83r9mjn08mqkwah8fmzpszci8ay7ki69yx1rgv3zx7npnxk";
+  cargoBuildFlags = [ "--features sqlite" ];


I'd keep the dbBackend flag (as mentioned above, this currently breaks eval as well) and validate the option's value using types.enum (this means that you can see all supported backends when looking e.g. at configuration.nix(5)) and in the package (to ensure that people get an evaluation error for invalid backends during eval when using e.g. bitwarden_rs from Nix on a non-NixOS system).

Also I'm wondering if it makes sense to provide e.g. a bitwarden_rs-full/bitwarden_rs-minimal, but I haven't checked the final closure sizes, so I'm not sure how many variants we actually want to add.

Ma27 · 2019-12-30T16:00:55Z

nixos/modules/services/security/bitwarden_rs/default.nix

@@ -69,6 +84,11 @@ in {
  };

  config = mkIf cfg.enable {
+    assertions = [{
+      assertion = cfg.backupDir != null -> elem cfg.dbBackend supportedBackupBackends;


The second part of the assertion is always true atm as the only allowed value (which is also the default) is sqlite which makes the condition always true.

Ma27 · 2019-12-30T16:15:45Z

pkgs/tools/security/bitwarden_rs/default.nix

+  cargoBuildFlags = [ "--features sqlite" ];
+
+  # FIXME: The rust check phase seems broken due to passing the flags after --.
+  doCheck = false;


When I set doCheck to true, I currently get the following errors:

bitwarden_rs> error: You need to enable one DB backend. To build with previous defaults do: cargo build --features sqlite bitwarden_rs> --> build.rs:12:5 bitwarden_rs> | bitwarden_rs> 12 | compile_error!("You need to enable one DB backend. To build with previous defaults do: cargo build --features sqlite"); bitwarden_rs> | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ bitwarden_rs> Compiling dotenv v0.15.0

Not sure if I'm misunderstanding your comment, but isn't it possible to somehow add some additional feature flags (e.g. for a default backend for testing) to the check-phase to ensure that the test builds with a db-backend?

Well, the right fix would be to fix checkPhase instead, because it does not really make sense to add flags after --. Apparently some flags can be added after -- (seems a weird API design choice to me) as can be seen in the cargo test manpage (e.g. cargo test -- --nocapture), but the documented flags have to be added before --, like --features. Maybe it can be fixed by adding checkArgs to differentiate between flags and arguments.

Just passing all cargo flags to cargo test could solve this issue.

Mic92 · 2019-12-31T15:28:46Z

This is what I did to implement postgresql/mysql as well: Mic92@86031cd

I had to switch to rust nightly in my fork so.

Ma27 · 2019-12-31T15:30:09Z

I had to switch to rust nightly in my fork so.

Do we actually need nightly features or is it sufficient to set RUSTC_BOOTSTRAP?

Mic92 · 2019-12-31T15:31:09Z

I had to switch to rust nightly in my fork so.

Do we actually need nightly features or is it sufficient to set RUSTC_BOOTSTRAP?

Ah. This might be the problem.

Mic92 · 2019-12-31T15:43:44Z

This variant no longer needs nightly: 30c49ee7ba3c43b13f1e26ae8dc7ef6b03e3d13a

Br1ght0ne · 2020-01-28T08:12:25Z

Closing in favor of #78615.

ofborg bot added 11.by: package-maintainer 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Dec 3, 2019

Ma27 requested changes Dec 4, 2019

View reviewed changes

Br1ght0ne force-pushed the bitwarden_rs-1.13.0 branch 2 times, most recently from 046feb0 to a4faba1 Compare December 5, 2019 02:05

ofborg bot added 6.topic: nixos 8.has: module (update) labels Dec 5, 2019

Br1ght0ne force-pushed the bitwarden_rs-1.13.0 branch from a4faba1 to b7860cf Compare December 5, 2019 10:06

Br1ght0ne requested a review from Ma27 December 5, 2019 10:06

Ma27 reviewed Dec 6, 2019

View reviewed changes

nixos/modules/services/security/bitwarden_rs/default.nix Outdated Show resolved Hide resolved

Br1ght0ne force-pushed the bitwarden_rs-1.13.0 branch from b7860cf to 45d1db8 Compare December 7, 2019 12:37

Br1ght0ne requested a review from Ma27 December 7, 2019 12:38

msteen mentioned this pull request Dec 30, 2019

bitwarden_rs: 1.9.1 -> 1.13.0 #76673

Closed

10 tasks

Br1ght0ne and others added 3 commits December 30, 2019 13:20

bitwarden_rs: 1.9.1 -> 1.13.0

6c1a498

Co-authored-by: msteen <msteen@users.noreply.github.com> Co-authored-by: Daniel Løvbrøtte Olsen <daniel.olsen99+GitHub@gmail.com>

bitwarden_rs: add filalex77 to maintainers

a14051d

Br1ght0ne force-pushed the bitwarden_rs-1.13.0 branch from 45d1db8 to d611666 Compare December 30, 2019 11:21

msteen suggested changes Dec 30, 2019

View reviewed changes

Ma27 reviewed Dec 30, 2019

View reviewed changes

msteen mentioned this pull request Jan 27, 2020

Merge pull request #78615 from msteen/bitwarden_rs #78615

Merged

10 tasks

Br1ght0ne closed this Jan 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bitwarden_rs: 1.9.1 -> 1.13.0 #74927

bitwarden_rs: 1.9.1 -> 1.13.0 #74927

Br1ght0ne commented Dec 3, 2019 •

edited

msteen commented Dec 3, 2019

Ma27 left a comment

Br1ght0ne commented Dec 5, 2019

Ma27 commented Dec 30, 2019

msteen left a comment

Ma27 left a comment

Ma27 Dec 30, 2019

Ma27 Dec 30, 2019

Ma27 Dec 30, 2019

msteen Dec 30, 2019

Mic92 Dec 31, 2019

Mic92 commented Dec 31, 2019

Ma27 commented Dec 31, 2019

Mic92 commented Dec 31, 2019

Mic92 commented Dec 31, 2019

Br1ght0ne commented Jan 28, 2020

bitwarden_rs: 1.9.1 -> 1.13.0 #74927

bitwarden_rs: 1.9.1 -> 1.13.0 #74927

Conversation

Br1ght0ne commented Dec 3, 2019 • edited

Motivation for this change

Things done

Notify maintainers

msteen commented Dec 3, 2019

Ma27 left a comment

Choose a reason for hiding this comment

Br1ght0ne commented Dec 5, 2019

Ma27 commented Dec 30, 2019

msteen left a comment

Choose a reason for hiding this comment

Ma27 left a comment

Choose a reason for hiding this comment

Ma27 Dec 30, 2019

Choose a reason for hiding this comment

Ma27 Dec 30, 2019

Choose a reason for hiding this comment

Ma27 Dec 30, 2019

Choose a reason for hiding this comment

msteen Dec 30, 2019

Choose a reason for hiding this comment

Mic92 Dec 31, 2019

Choose a reason for hiding this comment

Mic92 commented Dec 31, 2019

Ma27 commented Dec 31, 2019

Mic92 commented Dec 31, 2019

Mic92 commented Dec 31, 2019

Br1ght0ne commented Jan 28, 2020

Br1ght0ne commented Dec 3, 2019 •

edited