Clamp TTL on device token confirmed with existing device token #9411
Conversation
| /// a device token. This is a slightly awkward fit but is included here | ||
| /// because we need to use this to clamp the expiration time when device | ||
| /// tokens are confirmed using an existing device token. | ||
| device_token_expiration: Option<DateTime<Utc>>, |
There was a problem hiding this comment.
This is the sad part. I considered doing a proper scheme: AuthnScheme struct here like AuthnScheme::DeviceToken { time_expires: Option<DateTime<Utc>> } but I think that makes the abstraction-leaking here even worse. We don't want app-layer code to be doing conditional logic based on the authn scheme. Authz policy is generally meant to cover that. The device token confirmation logic in this PR is a special case.
| max and the current token's expiration time.", | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
new validation logic for explicit TTL request
| (None, Some(token_exp)) => Some(token_exp), | ||
| (None, None) => None, | ||
| } | ||
| }; |
There was a problem hiding this comment.
new clamping logic for no explicit TTL
| // If the user does not request a specific TTL, we do not error out. | ||
| // We calculate the token TTL as min(silo max TTL, current token TTL | ||
| // if present). Token confirm requests authenticated with a console | ||
| // session can get device tokens with TTLs up to the silo max. |
There was a problem hiding this comment.
We discussed this clamping logic in chat and decided that while it was more user-friendly to error out when TTL is specified, it is too easy to get the error when not specifying a TTL, so we clamp instead. Users confirming a token through a console session will never hit any of this logic anyway.
pietroalbini
left a comment
pietroalbini
left a comment
There was a problem hiding this comment.
Code looks correct to me! Left a minor nit and a few suggestions to improve the tests, but I'm also happy for the test changes to be done in a followup PR if we want to get 17.1 out ASAP.
| SchemeResult::Authenticated(Details { | ||
| actor: _, | ||
| device_token_expiration: _ | ||
| }) |
There was a problem hiding this comment.
Nit:
| SchemeResult::Authenticated(Details { | |
| actor: _, | |
| device_token_expiration: _ | |
| }) | |
| SchemeResult::Authenticated(Details { .. }) |
| testctx, | ||
| session_auth_response.device_code, | ||
| client_id, | ||
| AuthnMode::Session(session_token.clone()), |
There was a problem hiding this comment.
This request doesn't have to be authenticated. I lean slightly towards only authenticating requests that actually need it.
| } | ||
|
|
||
| #[nexus_test] | ||
| async fn test_device_token_cannot_extend_expiration( |
There was a problem hiding this comment.
In general, can we wrap the three device flow requests into a function? That'd made the test easier to understand (as we'd check whether the flow as a whole passes or fails with some params, instead of a wall of requests).
https://github.com/oxidecomputer/customer-support/issues/662