From d3b9e369b1656945f426b17a738a78c162d9417c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 30 Jun 2022 12:54:46 -0500 Subject: [PATCH 1/5] serve console index at /settings/* --- nexus/src/external_api/console_api.rs | 48 ++++++++++++++++++--------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index d9fba343afc..68d6359ceb2 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -558,22 +558,8 @@ pub async fn session_me( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } -// Dropshot does not have route match ranking and does not allow overlapping -// route definitions, so we cannot have a catchall `/*` route for console pages -// and then also define, e.g., `/api/blah/blah` and give the latter priority -// because it's a more specific match. So for now we simply give the console -// catchall route a prefix to avoid overlap. Long-term, if a route prefix is -// part of the solution, we would probably prefer it to be on the API endpoints, -// not on the console pages. Conveniently, all the console page routes start -// with /orgs already. -#[endpoint { - method = GET, - path = "/orgs/{path:.*}", - unpublished = true, -}] -pub async fn console_page( +async fn console_page_inner( rqctx: Arc>>, - _path_params: Path, ) -> Result, HttpError> { let opctx = OpContext::for_external_api(&rqctx).await; @@ -599,6 +585,38 @@ pub async fn console_page( .body("".into())?) } +// Dropshot does not have route match ranking and does not allow overlapping +// route definitions, so we cannot have a catchall `/*` route for console pages +// and then also define, e.g., `/api/blah/blah` and give the latter priority +// because it's a more specific match. So for now we simply give the console +// catchall route a prefix to avoid overlap. Long-term, if a route prefix is +// part of the solution, we would probably prefer it to be on the API endpoints, +// not on the console pages. Conveniently, all the console page routes start +// with /orgs already. +#[endpoint { + method = GET, + path = "/orgs/{path:.*}", + unpublished = true, +}] +pub async fn console_page( + rqctx: Arc>>, + _path_params: Path, +) -> Result, HttpError> { + console_page_inner(rqctx).await +} + +#[endpoint { + method = GET, + path = "/settings/{path:.*}", + unpublished = true, +}] +pub async fn console_settings_page( + rqctx: Arc>>, + _path_params: Path, +) -> Result, HttpError> { + console_page_inner(rqctx).await +} + /// Fetch a static asset from `/assets`. 404 on virtually all /// errors. No auth. NO SENSITIVE FILES. #[endpoint { From 6db573cf7dfc2239aee63404678cd2ff81f61927 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 30 Jun 2022 13:23:47 -0500 Subject: [PATCH 2/5] use full helper for /device/verify too --- nexus/src/external_api/console_api.rs | 6 +++--- nexus/src/external_api/device_auth.rs | 22 ++-------------------- 2 files changed, 5 insertions(+), 23 deletions(-) diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index 68d6359ceb2..33ae00b5f6c 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -558,7 +558,7 @@ pub async fn session_me( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } -async fn console_page_inner( +pub async fn console_index_or_login_redirect( rqctx: Arc>>, ) -> Result, HttpError> { let opctx = OpContext::for_external_api(&rqctx).await; @@ -602,7 +602,7 @@ pub async fn console_page( rqctx: Arc>>, _path_params: Path, ) -> Result, HttpError> { - console_page_inner(rqctx).await + console_index_or_login_redirect(rqctx).await } #[endpoint { @@ -614,7 +614,7 @@ pub async fn console_settings_page( rqctx: Arc>>, _path_params: Path, ) -> Result, HttpError> { - console_page_inner(rqctx).await + console_index_or_login_redirect(rqctx).await } /// Fetch a static asset from `/assets`. 404 on virtually all diff --git a/nexus/src/external_api/device_auth.rs b/nexus/src/external_api/device_auth.rs index c0a2a373b06..92747c54135 100644 --- a/nexus/src/external_api/device_auth.rs +++ b/nexus/src/external_api/device_auth.rs @@ -9,7 +9,7 @@ //! are for requesting access tokens that will be managed and used by //! the client to make other API requests. -use super::console_api::{get_login_url, serve_console_index}; +use super::console_api::console_index_or_login_redirect; use super::views::{DeviceAccessTokenGrant, DeviceAuthResponse}; use crate::context::OpContext; use crate::db::model::DeviceAccessToken; @@ -21,7 +21,6 @@ use http::{header, Response, StatusCode}; use hyper::Body; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use serde_urlencoded; use std::sync::Arc; use uuid::Uuid; @@ -128,24 +127,7 @@ pub async fn device_auth_verify( rqctx: Arc>>, params: Query, ) -> Result, HttpError> { - // If the user is authenticated, serve the console verification page. - if let Ok(opctx) = OpContext::for_external_api(&rqctx).await { - if opctx.authn.actor().is_some() { - return serve_console_index(rqctx.context()).await; - } - } - - // Otherwise, redirect for authentication. - let params = params.into_inner(); - let state_params = serde_urlencoded::to_string(serde_json::json!({ - "user_code": params.user_code - })) - .map_err(|e| HttpError::for_internal_error(e.to_string()))?; - let state = Some(format!("/device/verify?{}", state_params)); - Ok(Response::builder() - .status(StatusCode::FOUND) - .header(http::header::LOCATION, get_login_url(state)) - .body("".into())?) + console_index_or_login_redirect(rqctx).await } /// Confirm an OAuth 2.0 Device Authorization Grant From 92b870aad99952e072c4585887f29c686df95044 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 30 Jun 2022 14:13:05 -0500 Subject: [PATCH 3/5] also do /device/success --- nexus/src/external_api/device_auth.rs | 13 ++++++++++++- nexus/src/external_api/http_entrypoints.rs | 1 + 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/nexus/src/external_api/device_auth.rs b/nexus/src/external_api/device_auth.rs index 92747c54135..00132cc078c 100644 --- a/nexus/src/external_api/device_auth.rs +++ b/nexus/src/external_api/device_auth.rs @@ -125,7 +125,18 @@ pub struct DeviceAuthVerify { }] pub async fn device_auth_verify( rqctx: Arc>>, - params: Query, + _params: Query, +) -> Result, HttpError> { + console_index_or_login_redirect(rqctx).await +} + +#[endpoint { + method = GET, + path = "/device/success", + unpublished = true, +}] +pub async fn device_auth_success( + rqctx: Arc>>, ) -> Result, HttpError> { console_index_or_login_redirect(rqctx).await } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 82a3fff3ed5..5384b9f412a 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -228,6 +228,7 @@ pub fn external_api() -> NexusApiDescription { api.register(device_auth::device_auth_request)?; api.register(device_auth::device_auth_verify)?; + api.register(device_auth::device_auth_success)?; api.register(device_auth::device_auth_confirm)?; api.register(device_auth::device_access_token)?; From 5c33d76dafaa9154ae1dcb2f4fce7f1af48736c2 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 30 Jun 2022 15:47:21 -0500 Subject: [PATCH 4/5] first pass at always including current URI in login_url --- nexus/src/external_api/console_api.rs | 26 ++++++++------------ nexus/tests/integration_tests/console_api.rs | 5 +++- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index 33ae00b5f6c..07011daf68d 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -496,17 +496,15 @@ pub struct LoginUrlQuery { /// Generate URL to IdP login form. Optional `state` param is included in query /// string if present, and will typically represent the URL to send the user /// back to after successful login. -pub fn get_login_url(state: Option) -> String { +fn get_login_url(state: Option) -> String { // assume state is not URL encoded, so no risk of double encoding (dropshot // decodes it on the way in) let query = match state { Some(state) if state.is_empty() => None, - Some(state) => Some( + Some(state) => { serde_urlencoded::to_string(LoginUrlQuery { state: Some(state) }) - // unwrap is safe because query.state was just deserialized out - // of a query param, so we know it's serializable - .unwrap(), - ), + .ok() // in the hard-to-imagine event it's not serializable, no query + } None => None, }; // Once we have IdP integration, this will be a URL for the IdP login page. @@ -563,15 +561,7 @@ pub async fn console_index_or_login_redirect( ) -> Result, HttpError> { let opctx = OpContext::for_external_api(&rqctx).await; - // if authed, serve HTML page with bundle in script tag - - // HTML doesn't need to be static -- we'll probably find a reason to do some - // minimal templating, e.g., putting a CSRF token in the page - - // amusingly, at least to start out, I don't think we care about the path - // because the real routing is all client-side. we serve the same HTML - // regardless, the app starts on the client and renders the right page and - // makes the right API requests. + // if authed, serve console index.html with JS bundle in script tag if let Ok(opctx) = opctx { if opctx.authn.actor().is_some() { return serve_console_index(rqctx.context()).await; @@ -579,9 +569,13 @@ pub async fn console_index_or_login_redirect( } // otherwise redirect to idp + + // put the current URI in the query string to redirect back to after login + let uri = rqctx.request.lock().await.uri().clone(); + Ok(Response::builder() .status(StatusCode::FOUND) - .header(http::header::LOCATION, get_login_url(None)) + .header(http::header::LOCATION, get_login_url(Some(uri.to_string()))) .body("".into())?) } diff --git a/nexus/tests/integration_tests/console_api.rs b/nexus/tests/integration_tests/console_api.rs index 5c3d8fc5ee7..685bf358d4e 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/nexus/tests/integration_tests/console_api.rs @@ -116,7 +116,10 @@ async fn test_console_pages(cptestctx: &ControlPlaneTestContext) { // request to console page route without auth should redirect to IdP let _ = RequestBuilder::new(&testctx, Method::GET, "/orgs/irrelevant-path") .expect_status(Some(StatusCode::FOUND)) - .expect_response_header(header::LOCATION, "/spoof_login") + .expect_response_header( + header::LOCATION, + "/spoof_login?state=%2Forgs%2Firrelevant-path", + ) .execute() .await .expect("failed to redirect to IdP on auth failure"); From 1ef808a3ea928e0a43191b5c548f2ce26b372997 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 30 Jun 2022 16:15:59 -0500 Subject: [PATCH 5/5] improve code slightly, add big comment --- nexus/src/external_api/console_api.rs | 34 +++++++++++++++++---------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index 07011daf68d..e5e898b27c5 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -496,25 +496,33 @@ pub struct LoginUrlQuery { /// Generate URL to IdP login form. Optional `state` param is included in query /// string if present, and will typically represent the URL to send the user /// back to after successful login. +// TODO this does not know anything about IdPs, and it should. When the user is +// logged out and hits an auth-gated route, if there are multiple IdPs and we +// don't known which one they want to use, we need to send them to a page that +// will allow them to choose among discoverable IdPs. However, there may be ways +// to give ourselves a hint about which one they want, for example, by storing +// that info in a browser cookie when they log in. When their session ends, we +// will not be able to look at the dead session to find the silo or IdP (well, +// maybe we can but we probably shouldn't) but we can look at the cookie and +// default to sending them to the IdP indicated (though if they don't want that +// one we need to make sure they can get to a different one). If there is no +// cookie, we send them to the selector page. In any case, none of this is done +// here yet. We go to /spoof_login no matter what. fn get_login_url(state: Option) -> String { - // assume state is not URL encoded, so no risk of double encoding (dropshot - // decodes it on the way in) + // assume state is not already URL encoded let query = match state { - Some(state) if state.is_empty() => None, - Some(state) => { + Some(state) if !state.is_empty() => { serde_urlencoded::to_string(LoginUrlQuery { state: Some(state) }) - .ok() // in the hard-to-imagine event it's not serializable, no query + .ok() // in the strange event it's not serializable, no query } - None => None, + _ => None, }; // Once we have IdP integration, this will be a URL for the IdP login page. // For now we point to our own placeholder login page. - let mut url = "/spoof_login".to_string(); - if let Some(query) = query { - url.push('?'); - url.push_str(query.as_str()); + match query { + Some(query) => format!("/spoof_login?{query}"), + None => "/spoof_login".to_string(), } - url } /// Redirect to IdP login URL @@ -571,11 +579,11 @@ pub async fn console_index_or_login_redirect( // otherwise redirect to idp // put the current URI in the query string to redirect back to after login - let uri = rqctx.request.lock().await.uri().clone(); + let uri = rqctx.request.lock().await.uri().to_string(); Ok(Response::builder() .status(StatusCode::FOUND) - .header(http::header::LOCATION, get_login_url(Some(uri.to_string()))) + .header(http::header::LOCATION, get_login_url(Some(uri))) .body("".into())?) }