rename membership and adopt newtype pattern

[stefan0xC]

I've been working on improving Vaultwarden's legibility by renaming the UsersOrganizations to simply Membership and making it clearer when something is referring to the User and when to a Membership relation.

After doing that I was motivated to try my hands on using the newtype pattern so that the Rust type system can be used to check on compile time if we always call the right uuid because there are a lot of Strings that can be used interchangeably otherwise.

For example I noticed that we are passing the wrong id here:

vaultwarden/src/api/core/organizations.rs

Line 910 in ed4ad67

let mut group_entry = GroupUser::new(String::from(group), user.uuid.clone());

Because in contrast to CollectionUser the GroupUser::new actually expects a different id:

vaultwarden/src/db/models/group.rs

Line 122 in ed4ad67

pub fn new(groups_uuid: String, users_organizations_uuid: String) -> Self {

This was probably overlooked because it's not a major issue when a user is not correctly invited to a group and it only shows up as a small warning in the logs:

[2024-12-22 21:23:47.022][request][INFO] POST /api/organizations/152bd996-19f9-40ee-b991-e52333a7e719/users/invite
[2024-12-22 21:23:47.031][vaultwarden::db::models::group][WARN] User could not be found!
[2024-12-22 21:23:48.082][response][INFO] (send_invite) POST /api/organizations/<org_id>/users/invite => 200 OK

[dani-garcia]

Love this, I've been meaning for a while to do something about all the untyped string ids and never put much time into it as it seemed like a huge task. Thanks for the great work @stefan0xC!

Gave it a quick look and it looks pretty good, just have a couple of questions

[dani-garcia]

Every newtype is implementing their own copy of from_param with the same validation as far as I can see, would it make sense to extract the newtype creation into a macro or something?

[stefan0xC]

Well, I just copied the implementation for SafeString because of the first error message I got which I hadn't realized was only used by

vaultwarden/src/api/web.rs

Line 162 in ef4bff0

async fn attachments(uuid: SafeString, file_id: SafeString, token: String) -> Option<NamedFile> {

and

vaultwarden/src/api/core/sends.rs

Line 560 in ef4bff0

async fn download_send(send_id: SafeString, file_id: SafeString, t: &str) -> Option<NamedFile> {

Luckily the created UUIDs seem to have the same (if not more) restrictions, so this worked as expected.

Regarding the newtype creation via a macro: Not sure I know how do that or if that makes sense because they vary a bit in what they need to implement (they might not even all need everything I've derived for them). I've implemented a custom derive procedural macro for those that are used as Rocket parameter but not sure this is an improvement.

[stefan0xC]

Added a second derive macro to parse UUIDs which should be safe at least for those Ids because they all seem to be created via

vaultwarden/src/util.rs

Lines 358 to 360 in ef4bff0

	pub fn get_uuid() -> String {
	uuid::Uuid::new_v4().to_string()
	}

[dani-garcia]

Now that we're changing the ID's representations, would it also be a good idea to use uuid::Uuid as the internal type? It would mean we don't need to do input validation, plus we'd be able to derive Copy on the newtype, which would allow us to remove some clones.

[BlackDex]

Not sure how that would affect Diesel, since it doesn't support Uuid by default. Which can be solved too of course, but that would cause multiple castings of type i think.

[dani-garcia]

Ah yeah I forgot about that part and was just thinking about the rocket parsing, that's true

[stefan0xC]

I've planned to look into that too and maybe use uuid::Uuid::parse_str for validating the Strings that Rocket receives where appropriate. I mean I am not against also using them as UUIDs internally and convert them to Strings where needed but I think this might be a more radical change.

And yeah, at the moment diesel supports Uuid only for the postgres_backend right now. So for the other backends we'd had to either write custom sql types (if we want to store them efficiently as 16 byte blobs in SQLite and MySQL) or we keep saving them as Text (36 characters) to keep them more aligned? (I imagine #3796 would be harder to implement.)

[BlackDex]

Having different types per backend would be an issue then yes.
Unless we use a custom type of course, but indeed it might be a bigger change.

[dfunkt]

Encountered this when trying to build:

0.831 error: failed to load manifest for workspace member `/app/macros`
0.831 referenced by workspace at `/app/Cargo.toml`
0.831 
0.831 Caused by:
0.831   failed to read `/app/macros/Cargo.toml`
0.831 
0.831 Caused by:
0.831   No such file or directory (os error 2)

I needed to add:

diff --git a/.dockerignore b/.dockerignore
index 05c2a8e3..e11cb4ba 100644
--- a/.dockerignore
+++ b/.dockerignore
@@ -11,5 +11,6 @@
 !build.rs
 !Cargo.lock
 !Cargo.toml
+!macros
 !rustfmt.toml
 !rust-toolchain.toml
diff --git a/docker/Dockerfile.alpine b/docker/Dockerfile.alpine
index bc69fd7e..5341f1fd 100644
--- a/docker/Dockerfile.alpine
+++ b/docker/Dockerfile.alpine
@@ -81,6 +81,7 @@ RUN source /env-cargo && \

 # Copies over *only* your manifests and build files
 COPY ./Cargo.* ./rust-toolchain.toml ./build.rs ./
+COPY ./macros ./macros

 ARG CARGO_PROFILE=release

diff --git a/docker/Dockerfile.debian b/docker/Dockerfile.debian
index 896adf85..075d3759 100644
--- a/docker/Dockerfile.debian
+++ b/docker/Dockerfile.debian
@@ -120,6 +120,7 @@ RUN source /env-cargo && \

 # Copies over *only* your manifests and build files
 COPY ./Cargo.* ./rust-toolchain.toml ./build.rs ./
+COPY ./macros ./macros

 ARG CARGO_PROFILE=release

diff --git a/docker/Dockerfile.j2 b/docker/Dockerfile.j2
index 797501ed..66db34eb 100644
--- a/docker/Dockerfile.j2
+++ b/docker/Dockerfile.j2
@@ -147,6 +147,7 @@ RUN source /env-cargo && \

 # Copies over *only* your manifests and build files
 COPY ./Cargo.* ./rust-toolchain.toml ./build.rs ./
+COPY ./macros ./macros

 ARG CARGO_PROFILE=release

[stefan0xC]

@dfunkt Thanks for catching that.