make everything less ugly

Author: david-crespo

end-to-end-tests/src/bin/bootstrap.rs

@@ -115,7 +115,7 @@ async fn main() -> Result<()> {
         deserialize_byte_stream(
             ctx.client
                 .device_auth_request()
-                .body(DeviceAuthRequest { client_id })
+                .body(DeviceAuthRequest { client_id, ttl_seconds: None })
                 .send()
                 .await?,
         )

nexus/db-model/src/device_auth.rs

@@ -32,7 +32,9 @@ pub struct DeviceAuthRequest {
     pub user_code: String,
     pub time_created: DateTime<Utc>,
     pub time_expires: DateTime<Utc>,
-    pub requested_ttl_seconds: Option<i64>,
+
+    /// TTL requested by the user
+    pub token_ttl_seconds: Option<i64>,
 }
 
 impl DeviceAuthRequest {
@@ -108,7 +110,7 @@ impl DeviceAuthRequest {
             time_created: now,
             time_expires: now
                 + Duration::seconds(CLIENT_AUTHENTICATION_TIMEOUT),
-            requested_ttl_seconds: requested_ttl_seconds.map(|ttl| ttl as i64),
+            token_ttl_seconds: requested_ttl_seconds.map(i64::from),
         }
     }
 

nexus/db-model/src/schema_versions.rs

@@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock};
 ///
 /// This must be updated when you change the database schema.  Refer to
 /// schema/crdb/README.adoc in the root of this repository for details.
-pub const SCHEMA_VERSION: Version = Version::new(146, 0, 0);
+pub const SCHEMA_VERSION: Version = Version::new(147, 0, 0);
 
 /// List of all past database schema versions, in *reverse* order
 ///
@@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock<Vec<KnownVersion>> = LazyLock::new(|| {
         // |  leaving the first copy as an example for the next person.
         // v
         // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
+        KnownVersion::new(147, "device-auth-request-ttl"),
         KnownVersion::new(146, "silo-settings-token-expiration"),
         KnownVersion::new(145, "token-and-session-ids"),
         KnownVersion::new(144, "inventory-omicron-sled-config"),

nexus/db-schema/src/schema.rs

@@ -1360,7 +1360,7 @@ table! {
         device_code -> Text,
         time_created -> Timestamptz,
         time_expires -> Timestamptz,
-        requested_ttl_seconds -> Nullable<Int8>,
+        token_ttl_seconds -> Nullable<Int8>,
     }
 }
 

nexus/src/app/device_auth.rs

@@ -53,7 +53,7 @@ use nexus_db_queries::context::OpContext;
 use nexus_db_queries::db::model::{DeviceAccessToken, DeviceAuthRequest};
 
 use anyhow::anyhow;
-use nexus_types::external_api::params::DeviceAccessTokenRequest;
+use nexus_types::external_api::params;
 use nexus_types::external_api::views;
 use omicron_common::api::external::{
     CreateResult, DataPageParams, Error, ListResultVec,
@@ -77,15 +77,17 @@ impl super::Nexus {
     pub(crate) async fn device_auth_request_create(
         &self,
         opctx: &OpContext,
-        client_id: Uuid,
-        requested_ttl_seconds: Option<u32>,
+        params: params::DeviceAuthRequest,
     ) -> CreateResult<DeviceAuthRequest> {
         // TODO-correctness: the `user_code` generated for a new request
         // is used as a primary key, but may potentially collide with an
         // existing outstanding request. So we should retry some (small)
         // number of times if inserting the new request fails.
+
+        // Note that we cannot validate the TTL here against the silo max
+        // because we do not know what silo we're talking about until verify
         let auth_request =
-            DeviceAuthRequest::new(client_id, requested_ttl_seconds);
+            DeviceAuthRequest::new(params.client_id, params.ttl_seconds);
         self.db_datastore.device_auth_request_create(opctx, auth_request).await
     }
 
@@ -117,43 +119,29 @@ impl super::Nexus {
             .silo_auth_settings_view(opctx, &authz_silo)
             .await?;
 
-        // Validate the requested TTL against the silo's max TTL
-        if let Some(requested_ttl) = db_request.requested_ttl_seconds {
-            if let Some(max_ttl) = silo_auth_settings.device_token_max_ttl_seconds {
-                if requested_ttl > max_ttl {
-                    return Err(Error::invalid_request(&format!(
-                        "Requested TTL {} exceeds maximum allowed TTL {} for this silo",
-                        requested_ttl, max_ttl
-                    )));
-                }
+        let silo_max_ttl = silo_auth_settings.device_token_max_ttl_seconds;
+        let requested_ttl = db_request.token_ttl_seconds;
+
+        // validate the requested TTL against silo max if both present
+        if let (Some(requested), Some(max)) = (requested_ttl, silo_max_ttl) {
+            if requested > max {
+                return Err(Error::invalid_request(&format!(
+                    "Requested TTL {} seconds exceeds maximum allowed TTL {} seconds for this silo",
+                    requested, max
+                )));
             }
         }
 
-        // Create an access token record.
-        let token_ttl = match db_request.requested_ttl_seconds {
-            Some(requested_ttl) => {
-                // Use the requested TTL, but cap it at the silo max if there is one
-                let effective_ttl =
-                    match silo_auth_settings.device_token_max_ttl_seconds {
-                        Some(max_ttl) => std::cmp::min(requested_ttl, max_ttl),
-                        None => requested_ttl,
-                    };
-                Some(Utc::now() + Duration::seconds(effective_ttl))
-            }
-            None => {
-                // Use the silo max TTL if no specific TTL was requested
-                silo_auth_settings
-                    .device_token_max_ttl_seconds
-                    .map(|ttl| Utc::now() + Duration::seconds(ttl))
-            }
-        };
+        let time_expires = requested_ttl
+            .or(silo_max_ttl)
+            .map(|ttl| Utc::now() + Duration::seconds(ttl));
 
         let token = DeviceAccessToken::new(
             db_request.client_id,
             db_request.device_code,
             db_request.time_created,
             silo_user_id,
-            token_ttl,
+            time_expires,
         );
 
         if db_request.time_expires < Utc::now() {
@@ -252,7 +240,7 @@ impl super::Nexus {
     pub(crate) async fn device_access_token(
         &self,
         opctx: &OpContext,
-        params: DeviceAccessTokenRequest,
+        params: params::DeviceAccessTokenRequest,
     ) -> Result<Response<Body>, HttpError> {
         // RFC 8628 §3.4
         if params.grant_type != "urn:ietf:params:oauth:grant-type:device_code" {

nexus/src/external_api/http_entrypoints.rs

@@ -7854,9 +7854,8 @@ impl NexusExternalApi for NexusExternalApiImpl {
                 }
             };
 
-            let model = nexus
-                .device_auth_request_create(&opctx, params.client_id, params.ttl_seconds)
-                .await?;
+            let model =
+                nexus.device_auth_request_create(&opctx, params).await?;
             nexus.build_oauth_response(
                 StatusCode::OK,
                 &model.into_response(rqctx.server.using_tls(), host),

nexus/tests/integration_tests/device_auth.rs

@@ -5,8 +5,8 @@
 use std::num::NonZeroU32;
 
 use chrono::Utc;
-use dropshot::ResultsPage;
 use dropshot::test_util::ClientTestContext;
+use dropshot::{HttpErrorResponseBody, ResultsPage};
 use nexus_auth::authn::USER_TEST_UNPRIVILEGED;
 use nexus_db_queries::db::fixed_data::silo::DEFAULT_SILO;
 use nexus_db_queries::db::identity::{Asset, Resource};
@@ -55,6 +55,8 @@ async fn test_device_auth_flow(cptestctx: &ControlPlaneTestContext) {
         .expect("failed to reject device auth start without client_id");
 
     let client_id = Uuid::new_v4();
+    // note that this exercises ttl_seconds being omitted from the body because
+    // it's URL encoded, so None means it's omitted
     let authn_params = DeviceAuthRequest { client_id, ttl_seconds: None };
 
     // Using a JSON encoded body fails.
@@ -426,72 +428,68 @@ async fn test_device_token_request_ttl(cptestctx: &ControlPlaneTestContext) {
     let testctx = &cptestctx.external_client;
 
     // Set silo max TTL to 10 seconds
-    let _settings: views::SiloSettings = object_put(
-        testctx,
-        "/v1/settings",
-        &params::SiloSettingsUpdate {
-            device_token_max_ttl_seconds: NonZeroU32::new(10),
-        },
-    )
-    .await;
-
-    let client_id = Uuid::new_v4();
+    let settings = params::SiloSettingsUpdate {
+        device_token_max_ttl_seconds: NonZeroU32::new(10).into(),
+    };
+    let _: views::SiloSettings =
+        object_put(testctx, "/v1/settings", &settings).await;
 
     // Test 1: Request TTL above the max should fail at verification time
-    let authn_params_invalid = DeviceAuthRequest { 
-        client_id, 
-        ttl_seconds: Some(20) // Above the 10 second max
+    let invalid_ttl = DeviceAuthRequest {
+        client_id: Uuid::new_v4(),
+        ttl_seconds: Some(20), // Above the 10 second max
     };
 
-    let auth_response: DeviceAuthResponse =
+    let auth_response = NexusRequest::new(
         RequestBuilder::new(testctx, Method::POST, "/device/auth")
-            .allow_non_dropshot_errors()
-            .body_urlencoded(Some(&authn_params_invalid))
-            .expect_status(Some(StatusCode::OK))
-            .execute()
-            .await
-            .expect("failed to start client authentication flow")
-            .parsed_body()
-            .expect("client authentication response");
+            .body_urlencoded(Some(&invalid_ttl))
+            .expect_status(Some(StatusCode::OK)),
+    )
+    .execute_and_parse_unwrap::<DeviceAuthResponse>()
+    .await;
 
-    let confirm_params = DeviceAuthVerify { user_code: auth_response.user_code };
+    let confirm_params =
+        DeviceAuthVerify { user_code: auth_response.user_code };
 
-    // Confirmation should fail because requested TTL exceeds max
-    let confirm_response = NexusRequest::new(
+    // Confirmation fails because requested TTL exceeds max
+    let confirm_error = NexusRequest::new(
         RequestBuilder::new(testctx, Method::POST, "/device/confirm")
             .body(Some(&confirm_params))
             .expect_status(Some(StatusCode::BAD_REQUEST)),
     )
     .authn_as(AuthnMode::PrivilegedUser)
-    .execute()
-    .await
-    .expect("confirmation should fail for TTL above max");
+    .execute_and_parse_unwrap::<HttpErrorResponseBody>()
+    .await;
 
     // Check that the error message mentions TTL
-    let error_body = String::from_utf8_lossy(&confirm_response.body);
-    assert!(error_body.contains("TTL") || error_body.contains("ttl"));
+    assert_eq!(confirm_error.error_code, Some("InvalidRequest".to_string()));
+    assert_eq!(
+        confirm_error.message,
+        "Requested TTL 20 seconds exceeds maximum allowed TTL 10 seconds for this silo"
+    );
 
     // Test 2: Request TTL below the max should succeed and be used
-    let authn_params_valid = DeviceAuthRequest { 
-        client_id: Uuid::new_v4(), // New client ID for new flow
-        ttl_seconds: Some(5) // Below the 10 second max
+    let valid_ttl = DeviceAuthRequest {
+        client_id: Uuid::new_v4(),
+        ttl_seconds: Some(3), // Below the 10 second max
     };
 
-    let auth_response: DeviceAuthResponse =
+    let auth_response = NexusRequest::new(
         RequestBuilder::new(testctx, Method::POST, "/device/auth")
-            .allow_non_dropshot_errors()
-            .body_urlencoded(Some(&authn_params_valid))
-            .expect_status(Some(StatusCode::OK))
-            .execute()
-            .await
-            .expect("failed to start client authentication flow")
-            .parsed_body()
-            .expect("client authentication response");
+            .body_urlencoded(Some(&valid_ttl))
+            .expect_status(Some(StatusCode::OK)),
+    )
+    .execute_and_parse_unwrap::<DeviceAuthResponse>()
+    .await;
 
     let device_code = auth_response.device_code;
     let user_code = auth_response.user_code;
     let confirm_params = DeviceAuthVerify { user_code };
 
+    // this time will be pretty close to the now() used on the server when
+    // calculating expiration time
+    let t0 = Utc::now();
+
     // Confirmation should succeed
     NexusRequest::new(
         RequestBuilder::new(testctx, Method::POST, "/device/confirm")
@@ -506,44 +504,39 @@ async fn test_device_token_request_ttl(cptestctx: &ControlPlaneTestContext) {
     let token_params = DeviceAccessTokenRequest {
         grant_type: "urn:ietf:params:oauth:grant-type:device_code".to_string(),
         device_code,
-        client_id: authn_params_valid.client_id,
+        client_id: valid_ttl.client_id,
     };
 
     // Get the token
-    let token: DeviceAccessTokenGrant = NexusRequest::new(
+    let token_grant = NexusRequest::new(
         RequestBuilder::new(testctx, Method::POST, "/device/token")
             .allow_non_dropshot_errors()
             .body_urlencoded(Some(&token_params))
             .expect_status(Some(StatusCode::OK)),
     )
     .authn_as(AuthnMode::PrivilegedUser)
-    .execute()
-    .await
-    .expect("failed to get token")
-    .parsed_body()
-    .expect("failed to deserialize token response");
+    .execute_and_parse_unwrap::<DeviceAccessTokenGrant>()
+    .await;
 
-    // Verify the token has the correct expiration (5 seconds from now)
+    // Verify the token has roughly the correct expiration time. One second
+    // threshold is sufficient to confirm it's not getting the silo max of 10
+    // seconds. Locally, I saw diffs as low as 14ms.
     let tokens = get_tokens_priv(testctx).await;
-    let our_token = tokens.iter().find(|t| t.time_expires.is_some()).unwrap();
-    let expires_at = our_token.time_expires.unwrap();
-    let now = Utc::now();
-    
-    // Should expire approximately 5 seconds from now (allow some tolerance for test timing)
-    let expected_expiry = now + chrono::Duration::seconds(5);
-    let time_diff = (expires_at - expected_expiry).num_seconds().abs();
-    assert!(time_diff <= 2, "Token expiry should be close to requested TTL");
+    let time_expires = tokens[0].time_expires.unwrap();
+    let expected_expires = t0 + Duration::from_secs(3);
+    let diff_ms = (time_expires - expected_expires).num_milliseconds().abs();
+    assert!(diff_ms <= 1000, "time diff was {diff_ms} ms. should be near zero");
 
     // Token should work initially
-    project_list(&testctx, &token.access_token, StatusCode::OK)
+    project_list(&testctx, &token_grant.access_token, StatusCode::OK)
         .await
         .expect("token should work initially");
 
     // Wait for token to expire
-    sleep(Duration::from_secs(6)).await;
+    sleep(Duration::from_secs(4)).await;
 
-    // Token should be expired now
-    project_list(&testctx, &token.access_token, StatusCode::UNAUTHORIZED)
+    // Token is expired
+    project_list(&testctx, &token_grant.access_token, StatusCode::UNAUTHORIZED)
         .await
         .expect("token should be expired");
 }

nexus/types/src/external_api/params.rs

@@ -2382,7 +2382,8 @@ impl TryFrom<String> for RelativeUri {
 #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
 pub struct DeviceAuthRequest {
     pub client_id: Uuid,
-    /// Optional TTL for the access token in seconds. If not specified, the silo's max TTL will be used.
+    /// Optional lifetime for the access token in seconds. If not specified, the
+    /// silo's max TTL will be used (if set).
     pub ttl_seconds: Option<u32>,
 }
 

openapi/nexus.json

@@ -16658,6 +16658,13 @@
           "client_id": {
             "type": "string",
             "format": "uuid"
+          },
+          "ttl_seconds": {
+            "nullable": true,
+            "description": "Optional lifetime for the access token in seconds. If not specified, the silo's max TTL will be used (if set).",
+            "type": "integer",
+            "format": "uint32",
+            "minimum": 0
           }
         },
         "required": [

schema/crdb/dbinit.sql

@@ -2815,7 +2815,7 @@ CREATE TABLE IF NOT EXISTS omicron.public.device_auth_request (
     time_created TIMESTAMPTZ NOT NULL,
     time_expires TIMESTAMPTZ NOT NULL,
     -- requested TTL for the token in seconds (if specified by the user)
-    requested_ttl_seconds INT8 CHECK (requested_ttl_seconds > 0)
+    token_ttl_seconds INT8 CHECK (token_ttl_seconds > 0)
 );
 
 -- Access tokens granted in response to successful device authorization flows.
@@ -5696,7 +5696,7 @@ INSERT INTO omicron.public.db_metadata (
     version,
     target_version
 ) VALUES
-    (TRUE, NOW(), NOW(), '146.0.0', NULL)
+    (TRUE, NOW(), NOW(), '147.0.0', NULL)
 ON CONFLICT DO NOTHING;
 
 COMMIT;