From 5ea0779d6bdb0c633d1a8d7658542949fdb9c735 Mon Sep 17 00:00:00 2001 From: Stefan Melmuk <509385+stefan0xC@users.noreply.github.com> Date: Sat, 9 Aug 2025 22:18:04 +0200 Subject: [PATCH 1/5] a little cleanup after SSO merge (#6153) * fix some typos * rename scss variable to sso_enabled * refactor is_mobile to device * also mask sensitive sso config options --- .env.template | 2 +- playwright/README.md | 41 ++++++++++--------- playwright/compose/warden/Dockerfile | 2 +- playwright/test.env | 2 +- src/api/core/accounts.rs | 6 +-- src/api/core/organizations.rs | 2 +- src/api/icons.rs | 4 +- src/api/identity.rs | 6 +-- src/api/web.rs | 2 +- src/auth.rs | 2 +- src/config.rs | 5 ++- src/db/models/device.rs | 8 ++-- src/db/models/group.rs | 2 +- src/sso.rs | 6 +-- src/sso_client.rs | 2 +- .../templates/scss/vaultwarden.scss.hbs | 12 +++--- 16 files changed, 55 insertions(+), 49 deletions(-) diff --git a/.env.template b/.env.template index a3e4ef85..75dd181c 100644 --- a/.env.template +++ b/.env.template @@ -485,7 +485,7 @@ # SSO_AUTHORITY=https://auth.example.com ## Authorization request scopes. Optional SSO scopes, override if email and profile are not enough (`openid` is implicit). -#SSO_SCOPES="email profile" +# SSO_SCOPES="email profile" ## Additional authorization url parameters (ex: to obtain a `refresh_token` with Google Auth). # SSO_AUTHORIZE_EXTRA_PARAMS="access_type=offline&prompt=consent" diff --git a/playwright/README.md b/playwright/README.md index 47a1efe6..249108b9 100644 --- a/playwright/README.md +++ b/playwright/README.md @@ -1,18 +1,18 @@ # Integration tests This allows running integration tests using [Playwright](https://playwright.dev/). -\ -It usse its own [test.env](/test/scenarios/test.env) with different ports to not collide with a running dev instance. + +It uses its own `test.env` with different ports to not collide with a running dev instance. ## Install -This rely on `docker` and the `compose` [plugin](https://docs.docker.com/compose/install/). +This relies on `docker` and the `compose` [plugin](https://docs.docker.com/compose/install/). Databases (`Mariadb`, `Mysql` and `Postgres`) and `Playwright` will run in containers. ### Running Playwright outside docker -It's possible to run `Playwright` outside of the container, this remove the need to rebuild the image for each change. -You'll additionally need `nodejs` then run: +It is possible to run `Playwright` outside of the container, this removes the need to rebuild the image for each change. +You will additionally need `nodejs` then run: ```bash npm install @@ -33,7 +33,7 @@ To force a rebuild of the Playwright image: DOCKER_BUILDKIT=1 docker compose --env-file test.env build Playwright ``` -To access the ui to easily run test individually and debug if needed (will not work in docker): +To access the UI to easily run test individually and debug if needed (this will not work in docker): ```bash npx playwright test --ui @@ -42,7 +42,7 @@ npx playwright test --ui ### DB Projects are configured to allow to run tests only on specific database. -\ + You can use: ```bash @@ -62,7 +62,7 @@ DOCKER_BUILDKIT=1 docker compose --profile playwright --env-file test.env run Pl ### Keep services running -If you want you can keep the Db and Keycloak runnning (states are not impacted by the tests): +If you want you can keep the DB and Keycloak runnning (states are not impacted by the tests): ```bash PW_KEEP_SERVICE_RUNNNING=true npx playwright test @@ -86,7 +86,8 @@ DOCKER_BUILDKIT=1 docker compose --profile playwright --env-file test.env run Pl ## Writing scenario -When creating new scenario use the recorder to more easily identify elements (in general try to rely on visible hint to identify elements and not hidden ids). +When creating new scenario use the recorder to more easily identify elements +(in general try to rely on visible hint to identify elements and not hidden IDs). This does not start the server, you will need to start it manually. ```bash @@ -95,7 +96,7 @@ npx playwright codegen "http://127.0.0.1:8000" ## Override web-vault -It's possible to change the `web-vault` used by referencing a different `bw_web_builds` commit. +It is possible to change the `web-vault` used by referencing a different `bw_web_builds` commit. ```bash export PW_WV_REPO_URL=https://github.com/Timshel/oidc_web_builds.git @@ -105,12 +106,13 @@ DOCKER_BUILDKIT=1 docker compose --profile playwright --env-file test.env build # OpenID Connect test setup -Additionally this `docker-compose` template allow to run locally `VaultWarden`, [Keycloak](https://www.keycloak.org/) and [Maildev](https://github.com/timshel/maildev) to test OIDC. +Additionally this `docker-compose` template allows to run locally Vaultwarden, +[Keycloak](https://www.keycloak.org/) and [Maildev](https://github.com/timshel/maildev) to test OIDC. ## Setup This rely on `docker` and the `compose` [plugin](https://docs.docker.com/compose/install/). -First create a copy of `.env.template` as `.env` (This is done to prevent commiting your custom settings, Ex `SMTP_`). +First create a copy of `.env.template` as `.env` (This is done to prevent committing your custom settings, Ex `SMTP_`). ## Usage @@ -125,11 +127,12 @@ keycloakSetup_1 | 74af4933-e386-4e64-ba15-a7b61212c45e oidc_keycloakSetup_1 exited with code 0 ``` -Wait until `oidc_keycloakSetup_1 exited with code 0` which indicate the correct setup of the Keycloak realm, client and user (It's normal for this container to stop once the configuration is done). +Wait until `oidc_keycloakSetup_1 exited with code 0` which indicates the correct setup of the Keycloak realm, client and user +(It is normal for this container to stop once the configuration is done). Then you can access : -- `VaultWarden` on http://0.0.0.0:8000 with the default user `test@yopmail.com/test`. +- `Vaultwarden` on http://0.0.0.0:8000 with the default user `test@yopmail.com/test`. - `Keycloak` on http://0.0.0.0:8080/admin/master/console/ with the default user `admin/admin` - `Maildev` on http://0.0.0.0:1080 @@ -143,7 +146,7 @@ You can run just `Keycloak` with `--profile keycloak`: ```bash > docker compose --profile keycloak --env-file .env up ``` -When running with a local VaultWarden, you can use a front-end build from [dani-garcia/bw_web_builds](https://github.com/dani-garcia/bw_web_builds/releases). +When running with a local Vaultwarden, you can use a front-end build from [dani-garcia/bw_web_builds](https://github.com/dani-garcia/bw_web_builds/releases). ## Rebuilding the Vaultwarden @@ -155,12 +158,12 @@ docker compose --profile vaultwarden --env-file .env build VaultwardenPrebuild V ## Configuration -All configuration for `keycloak` / `VaultWarden` / `keycloak_setup.sh` can be found in [.env](.env.template). +All configuration for `keycloak` / `Vaultwarden` / `keycloak_setup.sh` can be found in [.env](.env.template). The content of the file will be loaded as environment variables in all containers. -- `keycloak` [configuration](https://www.keycloak.org/server/all-config) include `KEYCLOAK_ADMIN` / `KEYCLOAK_ADMIN_PASSWORD` and any variable prefixed `KC_` ([more information](https://www.keycloak.org/server/configuration#_example_configuring_the_db_url_host_parameter)). -- All `VaultWarden` configuration can be set (EX: `SMTP_*`) +- `keycloak` [configuration](https://www.keycloak.org/server/all-config) includes `KEYCLOAK_ADMIN` / `KEYCLOAK_ADMIN_PASSWORD` and any variable prefixed `KC_` ([more information](https://www.keycloak.org/server/configuration#_example_configuring_the_db_url_host_parameter)). +- All `Vaultwarden` configuration can be set (EX: `SMTP_*`) ## Cleanup -Use `docker compose --profile vaultWarden down`. +Use `docker compose --profile vaultwarden down`. diff --git a/playwright/compose/warden/Dockerfile b/playwright/compose/warden/Dockerfile index 93d12b3b..afa33858 100644 --- a/playwright/compose/warden/Dockerfile +++ b/playwright/compose/warden/Dockerfile @@ -1,6 +1,6 @@ FROM playwright_oidc_vaultwarden_prebuilt AS prebuilt -FROM node:18-bookworm AS build +FROM node:22-bookworm AS build ARG REPO_URL ARG COMMIT_HASH diff --git a/playwright/test.env b/playwright/test.env index 4524fcb6..89dd6651 100644 --- a/playwright/test.env +++ b/playwright/test.env @@ -43,7 +43,7 @@ KEYCLOAK_ADMIN_PASSWORD=${KEYCLOAK_ADMIN} KC_HTTP_HOST=127.0.0.1 KC_HTTP_PORT=8081 -# Script parameters (use Keycloak and VaultWarden config too) +# Script parameters (use Keycloak and Vaultwarden config too) TEST_REALM=test DUMMY_REALM=dummy DUMMY_AUTHORITY=http://${KC_HTTP_HOST}:${KC_HTTP_PORT}/realms/${DUMMY_REALM} diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index dedd6c9b..c14bcef2 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -342,11 +342,11 @@ async fn post_set_password(data: Json, headers: Headers, mut co let mut user = headers.user; if user.private_key.is_some() { - err!("Account already intialized cannot set password") + err!("Account already initialized, cannot set password") } - // Check against the password hint setting here so if it fails, the user - // can retry without losing their invitation below. + // Check against the password hint setting here so if it fails, + // the user can retry without losing their invitation below. let password_hint = clean_password_hint(&data.master_password_hint); enforce_password_hint_setting(&password_hint)?; diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 0478821d..b78bf128 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -2310,7 +2310,7 @@ struct OrgImportData { users: Vec, } -/// This function seems to be deprected +/// This function seems to be deprecated /// It is only used with older directory connectors /// TODO: Cleanup Tech debt #[post("/organizations//import", data = "")] diff --git a/src/api/icons.rs b/src/api/icons.rs index ebb87e07..df340e77 100644 --- a/src/api/icons.rs +++ b/src/api/icons.rs @@ -641,9 +641,9 @@ async fn stream_to_bytes_limit(res: Response, max_size: usize) -> Result sso::OIDCCodeWrapper, conn: &DbConn, ) -> ApiResult { - let state = sso::deocde_state(base64_state)?; + let state = sso::decode_state(base64_state)?; let code = sso::encode_code_claims(wrapper(state.clone())); let nonce = match SsoNonce::find(&state, conn).await { Some(n) => n, - None => err!(format!("Failed to retrive redirect_uri with {state}")), + None => err!(format!("Failed to retrieve redirect_uri with {state}")), }; let mut url = match url::Url::parse(&nonce.redirect_uri) { diff --git a/src/api/web.rs b/src/api/web.rs index 8a29a2c2..d8e35009 100644 --- a/src/api/web.rs +++ b/src/api/web.rs @@ -61,7 +61,7 @@ fn vaultwarden_css() -> Cached> { "mail_enabled": CONFIG.mail_enabled(), "sends_allowed": CONFIG.sends_allowed(), "signup_disabled": CONFIG.is_signup_disabled(), - "sso_disabled": !CONFIG.sso_enabled(), + "sso_enabled": CONFIG.sso_enabled(), "sso_only": CONFIG.sso_enabled() && CONFIG.sso_only(), "yubico_enabled": CONFIG._enable_yubico() && CONFIG.yubico_client_id().is_some() && CONFIG.yubico_secret_key().is_some(), }); diff --git a/src/auth.rs b/src/auth.rs index 1a602a5c..a4a0b22c 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -1174,7 +1174,7 @@ impl AuthTokens { let access_claims = LoginJwtClaims::default(device, user, &sub, client_id); - let validity = if DeviceType::is_mobile(&device.atype) { + let validity = if device.is_mobile() { *MOBILE_REFRESH_VALIDITY } else { *DEFAULT_REFRESH_VALIDITY diff --git a/src/config.rs b/src/config.rs index 9a45298c..0337fd52 100644 --- a/src/config.rs +++ b/src/config.rs @@ -283,6 +283,9 @@ macro_rules! make_config { "smtp_host", "smtp_username", "_smtp_img_src", + "sso_client_id", + "sso_authority", + "sso_callback_path", ]; let cfg = { @@ -1139,7 +1142,7 @@ fn validate_config(cfg: &ConfigItems) -> Result<(), Error> { fn validate_internal_sso_issuer_url(sso_authority: &String) -> Result { match openidconnect::IssuerUrl::new(sso_authority.clone()) { - Err(err) => err!(format!("Invalid sso_authority UR ({sso_authority}): {err}")), + Err(err) => err!(format!("Invalid sso_authority URL ({sso_authority}): {err}")), Ok(issuer_url) => Ok(issuer_url), } } diff --git a/src/db/models/device.rs b/src/db/models/device.rs index 91cc1e18..005d942d 100644 --- a/src/db/models/device.rs +++ b/src/db/models/device.rs @@ -70,6 +70,10 @@ impl Device { pub fn is_cli(&self) -> bool { matches!(DeviceType::from_i32(self.atype), DeviceType::WindowsCLI | DeviceType::MacOsCLI | DeviceType::LinuxCLI) } + + pub fn is_mobile(&self) -> bool { + matches!(DeviceType::from_i32(self.atype), DeviceType::Android | DeviceType::Ios) + } } pub struct DeviceWithAuthRequest { @@ -353,10 +357,6 @@ impl DeviceType { _ => DeviceType::UnknownBrowser, } } - - pub fn is_mobile(value: &i32) -> bool { - *value == DeviceType::Android as i32 || *value == DeviceType::Ios as i32 - } } #[derive( diff --git a/src/db/models/group.rs b/src/db/models/group.rs index b9f91171..310576c4 100644 --- a/src/db/models/group.rs +++ b/src/db/models/group.rs @@ -135,7 +135,7 @@ impl CollectionGroup { // If both read_only and hide_passwords are false, then manage should be true // You can't have an entry with read_only and manage, or hide_passwords and manage // Or an entry with everything to false - // For backwards compaibility and migration proposes we keep checking read_only and hide_password + // For backwards compatibility and migration proposes we keep checking read_only and hide_password json!({ "id": self.groups_uuid, "readOnly": self.read_only, diff --git a/src/sso.rs b/src/sso.rs index 4f7ed86a..8e746114 100644 --- a/src/sso.rs +++ b/src/sso.rs @@ -151,7 +151,7 @@ fn decode_token_claims(token_name: &str, token: &str) -> ApiResult ApiResult { +pub fn decode_state(base64_state: String) -> ApiResult { let state = match data_encoding::BASE64.decode(base64_state.as_bytes()) { Ok(vec) => match String::from_utf8(vec) { Ok(valid) => OIDCState(valid), @@ -316,7 +316,7 @@ pub async fn exchange_code(wrapped_code: &str, conn: &mut DbConn) -> ApiResult EmptyResult { + pub async fn check_validity(access_token: String) -> EmptyResult { let client = Client::cached().await?; match client.user_info(AccessToken::new(access_token)).await { Err(err) => { diff --git a/src/static/templates/scss/vaultwarden.scss.hbs b/src/static/templates/scss/vaultwarden.scss.hbs index 143a1599..b031404d 100644 --- a/src/static/templates/scss/vaultwarden.scss.hbs +++ b/src/static/templates/scss/vaultwarden.scss.hbs @@ -21,21 +21,21 @@ a[href$="/settings/sponsored-families"] { } /* Hide the sso `Email` input field */ -{{#if sso_disabled}} +{{#if (not sso_enabled)}} .vw-email-sso { @extend %vw-hide; } {{/if}} /* Hide the default/continue `Email` input field */ -{{#if (not sso_disabled)}} +{{#if sso_enabled}} .vw-email-continue { @extend %vw-hide; } {{/if}} /* Hide the `Continue` button on the login page */ -{{#if (not sso_disabled)}} +{{#if sso_enabled}} .vw-continue-login { @extend %vw-hide; } @@ -43,7 +43,7 @@ a[href$="/settings/sponsored-families"] { /* Hide the `Enterprise Single Sign-On` button on the login page */ {{#if (webver ">=2025.5.1")}} -{{#if sso_disabled}} +{{#if (not sso_enabled)}} .vw-sso-login { @extend %vw-hide; } @@ -71,7 +71,7 @@ app-root ng-component > form > div:nth-child(1) > div > button[buttontype="secon /* Hide the or text followed by the two buttons hidden above */ {{#if (webver ">=2025.5.1")}} -{{#if (or sso_disabled sso_only)}} +{{#if (or (not sso_enabled) sso_only)}} .vw-or-text { @extend %vw-hide; } @@ -83,7 +83,7 @@ app-root ng-component > form > div:nth-child(1) > div:nth-child(3) > div:nth-chi {{/if}} /* Hide the `Other` button on the login page */ -{{#if (or sso_disabled sso_only)}} +{{#if (or (not sso_enabled) sso_only)}} .vw-other-login { @extend %vw-hide; } From 7fc94516ce7916a808016c3bfee68451e3c94601 Mon Sep 17 00:00:00 2001 From: Timshel Date: Sat, 9 Aug 2025 22:20:03 +0200 Subject: [PATCH 2/5] Fix link to point to the wiki (#6157) --- src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.rs b/src/config.rs index 0337fd52..545d7dce 100644 --- a/src/config.rs +++ b/src/config.rs @@ -712,7 +712,7 @@ make_config! { sso_master_password_policy: String, true, option; /// Use SSO only for auth not the session lifecycle |> Use default Vaultwarden session lifecycle (Idle refresh token valid for 30days) sso_auth_only_not_session: bool, true, def, false; - /// Client cache for discovery endpoint. |> Duration in seconds (0 or less to disable). More details: https://github.com/dani-garcia/vaultwarden/blob/sso-support/SSO.md#client-cache + /// Client cache for discovery endpoint. |> Duration in seconds (0 or less to disable). More details: https://github.com/dani-garcia/vaultwarden/wiki/Enabling-SSO-support-using-OpenId-Connect#client-cache sso_client_cache_expiration: u64, true, def, 0; /// Log all tokens |> `LOG_LEVEL=debug` or `LOG_LEVEL=info,vaultwarden::sso=debug` is required sso_debug_tokens: bool, true, def, false; From 4a5516e150d4d44d42fa841c6d65f5c29540f078 Mon Sep 17 00:00:00 2001 From: Daniel Date: Sat, 9 Aug 2025 23:20:23 +0300 Subject: [PATCH 3/5] Fix Email 2FA for mobile apps (#6156) --- src/api/core/two_factor/email.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/api/core/two_factor/email.rs b/src/api/core/two_factor/email.rs index 46ff4599..f895efa1 100644 --- a/src/api/core/two_factor/email.rs +++ b/src/api/core/two_factor/email.rs @@ -24,6 +24,7 @@ pub fn routes() -> Vec { #[derive(Deserialize)] #[serde(rename_all = "camelCase")] struct SendEmailLoginData { + #[serde(alias = "DeviceIdentifier")] device_identifier: DeviceId, #[allow(unused)] From 8fd0ee421123e7f5aab2cede351267ef304cd928 Mon Sep 17 00:00:00 2001 From: Daniel Date: Sat, 9 Aug 2025 23:21:09 +0300 Subject: [PATCH 4/5] Update Rust to 1.89.0 (#6150) - also raise MSRV to 1.87.0 --- Cargo.toml | 2 +- docker/DockerSettings.yaml | 2 +- docker/Dockerfile.alpine | 8 ++++---- docker/Dockerfile.debian | 2 +- rust-toolchain.toml | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 0c4a511d..0e83dc50 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,7 +6,7 @@ name = "vaultwarden" version = "1.0.0" authors = ["Daniel García "] edition = "2021" -rust-version = "1.86.0" +rust-version = "1.87.0" resolver = "2" repository = "https://github.com/dani-garcia/vaultwarden" diff --git a/docker/DockerSettings.yaml b/docker/DockerSettings.yaml index ffecb1ad..860bbc25 100644 --- a/docker/DockerSettings.yaml +++ b/docker/DockerSettings.yaml @@ -5,7 +5,7 @@ vault_image_digest: "sha256:f6ac819a2cd9e226f2cd2ec26196ede94a41e672e9672a11b5f3 # We use the linux/amd64 platform shell scripts since there is no difference between the different platform scripts # https://github.com/tonistiigi/xx | https://hub.docker.com/r/tonistiigi/xx/tags xx_image_digest: "sha256:9c207bead753dda9430bdd15425c6518fc7a03d866103c516a2c6889188f5894" -rust_version: 1.88.0 # Rust version to be used +rust_version: 1.89.0 # Rust version to be used debian_version: bookworm # Debian release name to be used alpine_version: "3.22" # Alpine version to be used # For which platforms/architectures will we try to build images diff --git a/docker/Dockerfile.alpine b/docker/Dockerfile.alpine index a1e724e6..3a242c3a 100644 --- a/docker/Dockerfile.alpine +++ b/docker/Dockerfile.alpine @@ -32,10 +32,10 @@ FROM --platform=linux/amd64 docker.io/vaultwarden/web-vault@sha256:f6ac819a2cd9e ########################## ALPINE BUILD IMAGES ########################## ## NOTE: The Alpine Base Images do not support other platforms then linux/amd64 ## And for Alpine we define all build images here, they will only be loaded when actually used -FROM --platform=linux/amd64 ghcr.io/blackdex/rust-musl:x86_64-musl-stable-1.88.0 AS build_amd64 -FROM --platform=linux/amd64 ghcr.io/blackdex/rust-musl:aarch64-musl-stable-1.88.0 AS build_arm64 -FROM --platform=linux/amd64 ghcr.io/blackdex/rust-musl:armv7-musleabihf-stable-1.88.0 AS build_armv7 -FROM --platform=linux/amd64 ghcr.io/blackdex/rust-musl:arm-musleabi-stable-1.88.0 AS build_armv6 +FROM --platform=linux/amd64 ghcr.io/blackdex/rust-musl:x86_64-musl-stable-1.89.0 AS build_amd64 +FROM --platform=linux/amd64 ghcr.io/blackdex/rust-musl:aarch64-musl-stable-1.89.0 AS build_arm64 +FROM --platform=linux/amd64 ghcr.io/blackdex/rust-musl:armv7-musleabihf-stable-1.89.0 AS build_armv7 +FROM --platform=linux/amd64 ghcr.io/blackdex/rust-musl:arm-musleabi-stable-1.89.0 AS build_armv6 ########################## BUILD IMAGE ########################## # hadolint ignore=DL3006 diff --git a/docker/Dockerfile.debian b/docker/Dockerfile.debian index 6bd0cdfc..bdcc9721 100644 --- a/docker/Dockerfile.debian +++ b/docker/Dockerfile.debian @@ -36,7 +36,7 @@ FROM --platform=linux/amd64 docker.io/tonistiigi/xx@sha256:9c207bead753dda9430bd ########################## BUILD IMAGE ########################## # hadolint ignore=DL3006 -FROM --platform=$BUILDPLATFORM docker.io/library/rust:1.88.0-slim-bookworm AS build +FROM --platform=$BUILDPLATFORM docker.io/library/rust:1.89.0-slim-bookworm AS build COPY --from=xx / / ARG TARGETARCH ARG TARGETVARIANT diff --git a/rust-toolchain.toml b/rust-toolchain.toml index e09e3cf7..8729fb13 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,4 +1,4 @@ [toolchain] -channel = "1.88.0" +channel = "1.89.0" components = [ "rustfmt", "clippy" ] profile = "minimal" From 2a5489a4b28e25a1170e0f5f61dc9b9d2224f5ec Mon Sep 17 00:00:00 2001 From: Mathijs van Veluw Date: Sat, 9 Aug 2025 23:06:16 +0200 Subject: [PATCH 5/5] Fix several more multi select push issues (#6151) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix several more multi select push issues There were some more items which would still overload the push endpoint. This PR fixes the remaining items (I hope). I also encountered a missing endpoint for restoring multiple ciphers from the trash via the admin console. Overall, we could improve a lot of these items in a different way. Like bundle all SQL Queries etc... But that takes more time, and this fixes overloading the Bitwarden push servers, and speeds up these specific actions. Signed-off-by: BlackDex * Update src/api/core/ciphers.rs Co-authored-by: Daniel García --------- Signed-off-by: BlackDex Co-authored-by: Daniel García --- src/api/core/ciphers.rs | 98 ++++++++++++++++++++++++----------- src/db/models/auth_request.rs | 2 +- src/db/models/cipher.rs | 51 +++++++++++++++--- 3 files changed, 112 insertions(+), 39 deletions(-) diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs index 546b9d7d..d8e622f2 100644 --- a/src/api/core/ciphers.rs +++ b/src/api/core/ciphers.rs @@ -78,6 +78,7 @@ pub fn routes() -> Vec { restore_cipher_put, restore_cipher_put_admin, restore_cipher_selected, + restore_cipher_selected_admin, delete_all, move_cipher_selected, move_cipher_selected_put, @@ -318,7 +319,7 @@ async fn post_ciphers_create( // or otherwise), we can just ignore this field entirely. data.cipher.last_known_revision_date = None; - share_cipher_by_uuid(&cipher.uuid, data, &headers, &mut conn, &nt).await + share_cipher_by_uuid(&cipher.uuid, data, &headers, &mut conn, &nt, None).await } /// Called when creating a new user-owned cipher. @@ -920,7 +921,7 @@ async fn post_cipher_share( ) -> JsonResult { let data: ShareCipherData = data.into_inner(); - share_cipher_by_uuid(&cipher_id, data, &headers, &mut conn, &nt).await + share_cipher_by_uuid(&cipher_id, data, &headers, &mut conn, &nt, None).await } #[put("/ciphers//share", data = "")] @@ -933,7 +934,7 @@ async fn put_cipher_share( ) -> JsonResult { let data: ShareCipherData = data.into_inner(); - share_cipher_by_uuid(&cipher_id, data, &headers, &mut conn, &nt).await + share_cipher_by_uuid(&cipher_id, data, &headers, &mut conn, &nt, None).await } #[derive(Deserialize)] @@ -973,11 +974,16 @@ async fn put_cipher_share_selected( }; match shared_cipher_data.cipher.id.take() { - Some(id) => share_cipher_by_uuid(&id, shared_cipher_data, &headers, &mut conn, &nt).await?, + Some(id) => { + share_cipher_by_uuid(&id, shared_cipher_data, &headers, &mut conn, &nt, Some(UpdateType::None)).await? + } None => err!("Request missing ids field"), }; } + // Multi share actions do not send out a push for each cipher, we need to send a general sync here + nt.send_user_update(UpdateType::SyncCiphers, &headers.user, &headers.device.push_uuid, &mut conn).await; + Ok(()) } @@ -987,6 +993,7 @@ async fn share_cipher_by_uuid( headers: &Headers, conn: &mut DbConn, nt: &Notify<'_>, + override_ut: Option, ) -> JsonResult { let mut cipher = match Cipher::find_by_uuid(cipher_id, conn).await { Some(cipher) => { @@ -1018,7 +1025,10 @@ async fn share_cipher_by_uuid( }; // When LastKnownRevisionDate is None, it is a new cipher, so send CipherCreate. - let ut = if data.cipher.last_known_revision_date.is_some() { + // If there is an override, like when handling multiple items, we want to prevent a push notification for every single item + let ut = if let Some(ut) = override_ut { + ut + } else if data.cipher.last_known_revision_date.is_some() { UpdateType::SyncCipherUpdate } else { UpdateType::SyncCipherCreate @@ -1517,7 +1527,7 @@ async fn delete_cipher_selected_put_admin( #[put("/ciphers//restore")] async fn restore_cipher_put(cipher_id: CipherId, headers: Headers, mut conn: DbConn, nt: Notify<'_>) -> JsonResult { - _restore_cipher_by_uuid(&cipher_id, &headers, &mut conn, &nt).await + _restore_cipher_by_uuid(&cipher_id, &headers, false, &mut conn, &nt).await } #[put("/ciphers//restore-admin")] @@ -1527,7 +1537,17 @@ async fn restore_cipher_put_admin( mut conn: DbConn, nt: Notify<'_>, ) -> JsonResult { - _restore_cipher_by_uuid(&cipher_id, &headers, &mut conn, &nt).await + _restore_cipher_by_uuid(&cipher_id, &headers, false, &mut conn, &nt).await +} + +#[put("/ciphers/restore-admin", data = "")] +async fn restore_cipher_selected_admin( + data: Json, + headers: Headers, + mut conn: DbConn, + nt: Notify<'_>, +) -> JsonResult { + _restore_multiple_ciphers(data, &headers, &mut conn, &nt).await } #[put("/ciphers/restore", data = "")] @@ -1555,35 +1575,47 @@ async fn move_cipher_selected( nt: Notify<'_>, ) -> EmptyResult { let data = data.into_inner(); - let user_id = headers.user.uuid; + let user_id = &headers.user.uuid; if let Some(ref folder_id) = data.folder_id { - if Folder::find_by_uuid_and_user(folder_id, &user_id, &mut conn).await.is_none() { + if Folder::find_by_uuid_and_user(folder_id, user_id, &mut conn).await.is_none() { err!("Invalid folder", "Folder does not exist or belongs to another user"); } } - for cipher_id in data.ids { - let Some(cipher) = Cipher::find_by_uuid(&cipher_id, &mut conn).await else { - err!("Cipher doesn't exist") - }; - - if !cipher.is_accessible_to_user(&user_id, &mut conn).await { - err!("Cipher is not accessible by user") + let cipher_count = data.ids.len(); + let mut single_cipher: Option = None; + + // TODO: Convert this to use a single query (or at least less) to update all items + // Find all ciphers a user has access to, all others will be ignored + let accessible_ciphers = Cipher::find_by_user_and_ciphers(user_id, &data.ids, &mut conn).await; + let accessible_ciphers_count = accessible_ciphers.len(); + for cipher in accessible_ciphers { + cipher.move_to_folder(data.folder_id.clone(), user_id, &mut conn).await?; + if cipher_count == 1 { + single_cipher = Some(cipher); } + } - // Move cipher - cipher.move_to_folder(data.folder_id.clone(), &user_id, &mut conn).await?; - + if let Some(cipher) = single_cipher { nt.send_cipher_update( UpdateType::SyncCipherUpdate, &cipher, - std::slice::from_ref(&user_id), + std::slice::from_ref(user_id), &headers.device, None, &mut conn, ) .await; + } else { + // Multi move actions do not send out a push for each cipher, we need to send a general sync here + nt.send_user_update(UpdateType::SyncCiphers, &headers.user, &headers.device.push_uuid, &mut conn).await; + } + + if cipher_count != accessible_ciphers_count { + err!(format!( + "Not all ciphers are moved! {accessible_ciphers_count} of the selected {cipher_count} were moved." + )) } Ok(()) @@ -1764,6 +1796,7 @@ async fn _delete_multiple_ciphers( async fn _restore_cipher_by_uuid( cipher_id: &CipherId, headers: &Headers, + multi_restore: bool, conn: &mut DbConn, nt: &Notify<'_>, ) -> JsonResult { @@ -1778,15 +1811,17 @@ async fn _restore_cipher_by_uuid( cipher.deleted_at = None; cipher.save(conn).await?; - nt.send_cipher_update( - UpdateType::SyncCipherUpdate, - &cipher, - &cipher.update_users_revision(conn).await, - &headers.device, - None, - conn, - ) - .await; + if !multi_restore { + nt.send_cipher_update( + UpdateType::SyncCipherUpdate, + &cipher, + &cipher.update_users_revision(conn).await, + &headers.device, + None, + conn, + ) + .await; + } if let Some(org_id) = &cipher.organization_uuid { log_event( @@ -1814,12 +1849,15 @@ async fn _restore_multiple_ciphers( let mut ciphers: Vec = Vec::new(); for cipher_id in data.ids { - match _restore_cipher_by_uuid(&cipher_id, headers, conn, nt).await { + match _restore_cipher_by_uuid(&cipher_id, headers, true, conn, nt).await { Ok(json) => ciphers.push(json.into_inner()), err => return err, } } + // Multi move actions do not send out a push for each cipher, we need to send a general sync here + nt.send_user_update(UpdateType::SyncCiphers, &headers.user, &headers.device.push_uuid, conn).await; + Ok(Json(json!({ "data": ciphers, "object": "list", diff --git a/src/db/models/auth_request.rs b/src/db/models/auth_request.rs index 2a14787e..31e7e66e 100644 --- a/src/db/models/auth_request.rs +++ b/src/db/models/auth_request.rs @@ -6,7 +6,7 @@ use macros::UuidFromParam; use serde_json::Value; db_object! { - #[derive(Debug, Identifiable, Queryable, Insertable, AsChangeset, Deserialize, Serialize)] + #[derive(Identifiable, Queryable, Insertable, AsChangeset, Deserialize, Serialize)] #[diesel(table_name = auth_requests)] #[diesel(treat_none_as_null = true)] #[diesel(primary_key(uuid))] diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs index f8d60505..8cbad4b7 100644 --- a/src/db/models/cipher.rs +++ b/src/db/models/cipher.rs @@ -783,7 +783,12 @@ impl Cipher { // true, then the non-interesting ciphers will not be returned. As a // result, those ciphers will not appear in "My Vault" for the org // owner/admin, but they can still be accessed via the org vault view. - pub async fn find_by_user(user_uuid: &UserId, visible_only: bool, conn: &mut DbConn) -> Vec { + pub async fn find_by_user( + user_uuid: &UserId, + visible_only: bool, + cipher_uuids: &Vec, + conn: &mut DbConn, + ) -> Vec { if CONFIG.org_groups_enabled() { db_run! {conn: { let mut query = ciphers::table @@ -821,7 +826,14 @@ impl Cipher { if !visible_only { query = query.or_filter( users_organizations::atype.le(MembershipType::Admin as i32) // Org admin/owner - ); + ); + } + + // Only filter for one specific cipher + if !cipher_uuids.is_empty() { + query = query.filter( + ciphers::uuid.eq_any(cipher_uuids) + ); } query @@ -850,11 +862,18 @@ impl Cipher { .or_filter(users_collections::user_uuid.eq(user_uuid)) // Access to collection .into_boxed(); - if !visible_only { - query = query.or_filter( - users_organizations::atype.le(MembershipType::Admin as i32) // Org admin/owner - ); - } + if !visible_only { + query = query.or_filter( + users_organizations::atype.le(MembershipType::Admin as i32) // Org admin/owner + ); + } + + // Only filter for one specific cipher + if !cipher_uuids.is_empty() { + query = query.filter( + ciphers::uuid.eq_any(cipher_uuids) + ); + } query .select(ciphers::all_columns) @@ -866,7 +885,23 @@ impl Cipher { // Find all ciphers visible to the specified user. pub async fn find_by_user_visible(user_uuid: &UserId, conn: &mut DbConn) -> Vec { - Self::find_by_user(user_uuid, true, conn).await + Self::find_by_user(user_uuid, true, &vec![], conn).await + } + + pub async fn find_by_user_and_ciphers( + user_uuid: &UserId, + cipher_uuids: &Vec, + conn: &mut DbConn, + ) -> Vec { + Self::find_by_user(user_uuid, true, cipher_uuids, conn).await + } + + pub async fn find_by_user_and_cipher( + user_uuid: &UserId, + cipher_uuid: &CipherId, + conn: &mut DbConn, + ) -> Option { + Self::find_by_user(user_uuid, true, &vec![cipher_uuid.clone()], conn).await.pop() } // Find all ciphers directly owned by the specified user.