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
2025-09-09 18:25:58 +03:00 · 2025-08-09 22:18:04 +02:00
parent a133d4e90c
commit 5ea0779d6b
16 changed files with 55 additions and 49 deletions
--- 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"
--- 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`.
--- 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
--- 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}
--- a/src/api/core/accounts.rs
+++ b/src/api/core/accounts.rs
@@ -342,11 +342,11 @@ async fn post_set_password(data: Json<SetPasswordData>, 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)?;

--- a/src/api/core/organizations.rs
+++ b/src/api/core/organizations.rs
@@ -2310,7 +2310,7 @@ struct OrgImportData {
    users: Vec<OrgImportUserData>,
 }

-/// 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/<org_id>/import", data = "<data>")]
--- 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<Bytes,
    let mut buf = BytesMut::new();
    let mut size = 0;
    while let Some(chunk) = stream.next().await {
-        // It is possible that there might occure UnexpectedEof errors or others
+        // It is possible that there might occur UnexpectedEof errors or others
        // This is most of the time no issue, and if there is no chunked data anymore or at all parsing the HTML will not happen anyway.
-        // Therfore if chunk is an err, just break and continue with the data be have received.
+        // Therefore if chunk is an err, just break and continue with the data be have received.
        if chunk.is_err() {
            break;
        }
--- a/src/api/identity.rs
+++ b/src/api/identity.rs
@@ -293,7 +293,7 @@ async fn _sso_login(
        }
    };

-    // We passed 2FA get full user informations
+    // We passed 2FA get full user information
    let auth_user = sso::redeem(&user_infos.state, conn).await?;

    if sso_user.is_none() {
@@ -1060,12 +1060,12 @@ async fn oidcsignin_redirect(
    wrapper: impl FnOnce(OIDCState) -> sso::OIDCCodeWrapper,
    conn: &DbConn,
 ) -> ApiResult<Redirect> {
-    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) {
--- a/src/api/web.rs
+++ b/src/api/web.rs
@@ -61,7 +61,7 @@ fn vaultwarden_css() -> Cached<Css<String>> {
        "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(),
    });
--- 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
--- 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<openidconnect::IssuerUrl, Error> {
    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),
    }
 }
--- 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(
--- 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,
--- a/src/sso.rs
+++ b/src/sso.rs
@@ -151,7 +151,7 @@ fn decode_token_claims(token_name: &str, token: &str) -> ApiResult<BasicTokenCla
    }
 }

-pub fn deocde_state(base64_state: String) -> ApiResult<OIDCState> {
+pub fn decode_state(base64_state: String) -> ApiResult<OIDCState> {
    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<U
        user_name: user_name.clone(),
    };

-    debug!("Authentified user {authenticated_user:?}");
+    debug!("Authenticated user {authenticated_user:?}");

    AC_CACHE.insert(state.clone(), authenticated_user);

@@ -443,7 +443,7 @@ pub async fn exchange_refresh_token(
                err_silent!("Access token is close to expiration but we have no refresh token")
            }

-            Client::check_validaty(access_token.clone()).await?;
+            Client::check_validity(access_token.clone()).await?;

            let access_claims = auth::LoginJwtClaims::new(
                device,
--- a/src/sso_client.rs
+++ b/src/sso_client.rs
@@ -203,7 +203,7 @@ impl Client {
        }
    }

-    pub async fn check_validaty(access_token: String) -> 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) => {
--- 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;
 }