diff --git a/backend/windmill-api-integration-tests/tests/login_link.rs b/backend/windmill-api-integration-tests/tests/login_link.rs index 5c3a231a48..3906232075 100644 --- a/backend/windmill-api-integration-tests/tests/login_link.rs +++ b/backend/windmill-api-integration-tests/tests/login_link.rs @@ -115,6 +115,60 @@ async fn login_link_is_single_use_and_same_origin(db: Pool) -> anyhow: Ok(()) } +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn confirmed_login_link_is_spent_by_the_click_not_the_page( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api"); + + let resp = client() + .post(format!("{base}/users/login_links")) + .header("Authorization", "Bearer SECRET_TOKEN") + .json(&json!({"email": "test2@windmill.dev", "confirm": true})) + .send() + .await?; + assert_eq!(resp.status(), 201); + let link = resp.json::().await?; + // The URL handed out is the frontend page, not the API path that signs in on a GET. + let token = link["url"] + .as_str() + .unwrap() + .split_once("/user/login_link?token=") + .expect("confirmation page url") + .1 + .to_string(); + + let confirm = || { + client() + .post(format!("{base}/auth/login_link/{token}")) + .send() + }; + let resp = confirm().await?; + assert_eq!(resp.status(), 200); + assert!(resp + .headers() + .get_all("set-cookie") + .iter() + .any(|c| c.to_str().unwrap().starts_with("token="))); + assert_eq!( + resp.json::().await?["location"], + "/user/workspaces" + ); + + let resp = confirm().await?; + assert_eq!(resp.status(), 200); + assert!(resp.headers().get("set-cookie").is_none()); + assert_eq!( + resp.json::().await?["location"], + "/user/login_link_expired?reason=used" + ); + + Ok(()) +} + #[sqlx::test(migrations = "../migrations", fixtures("base"))] async fn login_link_mint_can_require_a_login_type(db: Pool) -> anyhow::Result<()> { initialize_tracing().await; diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index 2bfac244f5..8e3ed6894a 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -168,7 +168,10 @@ pub fn make_unauthed_service() -> Router { .route("/logout", post(logout).get(logout)) .route("/is_first_time_setup", get(is_first_time_setup)) .route("/request_password_reset", post(request_password_reset)) - .route("/login_link/{token}", get(consume_login_link)) + .route( + "/login_link/{token}", + get(consume_login_link).post(confirm_login_link), + ) .route("/is_smtp_configured", get(is_smtp_configured)) .route( "/is_password_login_disabled", @@ -3215,9 +3218,12 @@ async fn impersonate( } const LOGIN_LINK_DEFAULT_TTL_S: u32 = 600; -const LOGIN_LINK_MAX_TTL_S: u32 = 900; +// Long enough for a link sent by email to still work when it is read. `require_login_type` is +// only checked at mint, so a much longer cap would need re-checking it when the link is opened. +const LOGIN_LINK_MAX_TTL_S: u32 = 7200; const LOGIN_LINK_DEFAULT_RD: &str = "/user/workspaces"; const LOGIN_LINK_EXPIRED_PAGE: &str = "/user/login_link_expired"; +const LOGIN_LINK_CONFIRM_PAGE: &str = "/user/login_link"; #[derive(Deserialize)] pub struct NewLoginLink { @@ -3228,6 +3234,9 @@ pub struct NewLoginLink { /// account it created can require `pending_oauth`, so the link stops working once the /// owner has set a password or signed in with a provider. pub require_login_type: Option, + /// Hand out a page that signs in only when its button is clicked. Mail scanners open links + /// on delivery, and opening the plain link spends it, so a link sent by email sets this. + pub confirm: Option, } #[derive(Serialize)] @@ -3378,11 +3387,12 @@ async fn create_login_link( .await?; tx.commit().await?; - let url = format!( - "{}/api/auth/login_link/{}", - (**BASE_URL.load()).clone(), - token - ); + let base_url = (**BASE_URL.load()).clone(); + let url = if nl.confirm.unwrap_or(false) { + format!("{base_url}{LOGIN_LINK_CONFIRM_PAGE}?token={token}") + } else { + format!("{base_url}/api/auth/login_link/{token}") + }; Ok((StatusCode::CREATED, Json(LoginLink { url, expires_at }))) } @@ -3628,19 +3638,45 @@ async fn consume_login_link( Path(token): Path, Query(query): Query, ) -> Result { - let bounce = |reason: &str| { - Ok(login_link_redirect(format!( - "{LOGIN_LINK_EXPIRED_PAGE}?reason={reason}" - ))) - }; + let location = redeem_login_link(&headers, cookies, &db, &token, query.rd).await?; + Ok(login_link_redirect(location)) +} + +#[derive(Serialize)] +struct LoginLinkLocation { + location: String, +} + +/// The confirmation page's click. It answers with where to go rather than redirecting, and the +/// page navigates there itself. +async fn confirm_login_link( + headers: axum::http::HeaderMap, + cookies: Cookies, + Extension(db): Extension, + Path(token): Path, +) -> JsonResult { + let location = redeem_login_link(&headers, cookies, &db, &token, None).await?; + Ok(Json(LoginLinkLocation { location })) +} + +/// Spends the link and sets the session cookie, returning the post-login destination; or +/// returns the explanation page, with no session, when the link cannot be used. +async fn redeem_login_link( + headers: &axum::http::HeaderMap, + cookies: Cookies, + db: &DB, + token: &str, + requested_rd: Option, +) -> Result { + let bounce = |reason: &str| Ok(format!("{LOGIN_LINK_EXPIRED_PAGE}?reason={reason}")); if token.len() != 32 { return bounce("invalid"); } - let t_hash = hash_token(&token); + let t_hash = hash_token(token); // The account is unknown until the row is read, so only the global and per-IP tiers // apply here; a 32-char random token leaves nothing for the per-account tier to guard. windmill_common::login_rate_limit::check_and_increment_login_attempt( - &headers, + headers, &t_hash[..TOKEN_PREFIX_LEN], )?; @@ -3707,11 +3743,10 @@ async fn consume_login_link( .await?; tx.commit().await?; - let rd = link + Ok(link .rd - .or_else(|| same_origin_rd(query.rd)) - .unwrap_or_else(|| LOGIN_LINK_DEFAULT_RD.to_string()); - Ok(login_link_redirect(rd)) + .or_else(|| same_origin_rd(requested_rd)) + .unwrap_or_else(|| LOGIN_LINK_DEFAULT_RD.to_string())) } #[derive(Deserialize)] diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 39f769fe9a..40f4e8e8d2 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -529,6 +529,30 @@ paths: responses: "302": description: redirected to the post-login destination, or to /user/login_link_expired when the link is used, expired or unknown + post: + security: [] + summary: consume a single-use login link from its confirmation page, set the session cookie and answer where to go + operationId: confirmLoginLink + tags: + - user + parameters: + - name: token + in: path + required: true + schema: + type: string + responses: + "200": + description: the post-login destination, or /user/login_link_expired when the link is used, expired or unknown + content: + application/json: + schema: + type: object + required: + - location + properties: + location: + type: string /auth/reset_password: post: @@ -6753,7 +6777,7 @@ paths: type: string expires_in_s: type: integer - description: link lifetime in seconds, at most 900 (default 600) + description: link lifetime in seconds, at most 7200 (default 600) rd: type: string description: same-origin path the browser lands on after login (default /user/workspaces) @@ -6763,6 +6787,12 @@ paths: mint only while the account still has this login type (for example pending_oauth), so a link stops working once the owner has set a password or signed in with a provider + confirm: + type: boolean + description: >- + return a /user/login_link page that signs in only when its button is + clicked, instead of a link spent by opening it; set it for links sent by + email, which mail scanners open on delivery (default false) responses: "201": description: login link minted diff --git a/docs/auth-surface.md b/docs/auth-surface.md index abeba1fe2b..8b9b265b20 100644 --- a/docs/auth-surface.md +++ b/docs/auth-surface.md @@ -29,8 +29,10 @@ Symbols, not line numbers, are cited: they drift less. the account into a `password` one in the same statement (an account created ahead of its owner gets its first credential that way, or through the OAuth claim below). - **Login links** (`login_link` table, `POST /users/login_links` superadmin-only, - `GET /auth/login_link/{token}` unauthenticated): single-use, ≤15 min, a session cookie and a - 302 to a same-origin `rd`. `require_login_type` on the mint refuses (409) an account whose + `GET /auth/login_link/{token}` unauthenticated): single-use, ≤2 h, a session cookie and a + 302 to a same-origin `rd`. A link minted with `confirm` is the `/user/login_link` page + instead, which spends it only on a click (`POST` to the same path, answering `{location}`), so + a mail scanner opening it does not. `require_login_type` on the mint refuses (409) an account whose `login_type` has moved on — the way a caller re-entering an account it created stops being able to once the owner has a password or a provider. - **Pre-approved trial offer** (`cloud_trial_offer`, cloud-only routes under diff --git a/frontend/src/routes/user/login_link/+page.svelte b/frontend/src/routes/user/login_link/+page.svelte new file mode 100644 index 0000000000..48d1d356fb --- /dev/null +++ b/frontend/src/routes/user/login_link/+page.svelte @@ -0,0 +1,44 @@ + + + + {#if token} + + {/if} + diff --git a/frontend/src/routes/user/login_link_expired/+page.svelte b/frontend/src/routes/user/login_link_expired/+page.svelte index b160985bb2..6c99d35408 100644 --- a/frontend/src/routes/user/login_link_expired/+page.svelte +++ b/frontend/src/routes/user/login_link_expired/+page.svelte @@ -17,7 +17,7 @@