first pass at always including current URI in login_url

david-crespo · david-crespo · commit 5c33d76dafaa · 2022-06-30T15:59:17.000-05:00
diff --git 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>) -> String {
+fn get_login_url(state: Option<String>) -> 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,25 +561,21 @@ pub async fn console_index_or_login_redirect(
 ) -> Result<Response<Body>, 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;
         }
     }
 
     // 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
@@ -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");
