OAuth rework#437
Conversation
|
Ran through a number of iterations here and I think this is ready for final review. |
There was a problem hiding this comment.
I took a stab at reviewing this. Most of my comments are style-related, feel free to apply/ignore/fix later/never/etc.
Oh yeah and the date/ are wrong on migrations (note 2026).
2025-01-01, 2025-01-02, 2025-05-18 -> 2026-05-18_000001 etc.
Agent also identified a potential cli deadlock that figured I'd mention, which we're unlikely to hit, but would by definition be difficult to debug (no error message).
deadlock on cli errors
The CLI PKCE callback path can deadlock on error callbacks. The proxy consumes the single-use callback before invoking it, but the handler only sends on token_tx after successful parsing and code exchange. If the browser
returns error=access_denied, a malformed callback, or a CSRF mismatch, the handler returns Err without notifying the waiting login task, and the caller remains blocked on token_rx indefinitely.Suggested fix: make the callback always complete the one-shot, including failure cases. The simplest approach is to catch all callback errors inside the closure and send Err(...) through token_tx before returning an HTTP
response to the browser. Optionally, also handle error callbacks explicitly and show a user-friendly failure page instead of treating them as missing code.
| ) -> Result<Self, AppError> { | ||
| let signers = signers.into_iter().map(Arc::new).collect::<Vec<_>>(); | ||
| let verifiers = verifiers.into_iter().map(Arc::new).collect::<Vec<_>>(); | ||
| additional_permissions.extend_from_slice(&[ |
There was a problem hiding this comment.
style: sort these alphabetically
| /// be modified or deleted via the API. They do not support activation limits, they | ||
| /// fire unconditionally whenever their rule matches. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct EphemeralMapperConfig { |
There was a problem hiding this comment.
style: I'm not sure Ephemeral is the right word here.
Maybe Static / Fixed rather than ephemeral?
I dunno.
There was a problem hiding this comment.
Yeah, struggled trying to find a name for them. Certainly up for renaming. Originally they were static, but I wanted something that could convey that they are never persisted and are only an in memory construct. I'll give this some more thought.
| mappers: Vec<EphemeralMapperConfig>, | ||
| #[cfg(feature = "sagas")] | ||
| saga: Option<(TypedUuid<SagaExecNodeId>, Option<Logger>)>, | ||
| additional_builtin_permissions: Vec<T>, |
There was a problem hiding this comment.
style: Perhaps worth adding a doc comment here.
There was a problem hiding this comment.
Thanks for the note. I need to rework this actually. Given some permission changes, this needs to change as well.
| provider: &impl CliOAuthProviderInfo, | ||
| details: &DeviceAuthorizationResponse, | ||
| ) -> Result<DeviceTokenResponse> { | ||
| let interval = Duration::from_secs(details.interval.unwrap_or(DEFAULT_POLL_INTERVAL_SECS)); |
There was a problem hiding this comment.
nit: What happens if this is zero? Maybe:
let interval = Duration::from_secs(
details.interval.map(NonZeroU64::get).unwrap_or(DEFAULT_POLL_INTERVAL_SECS),
);There was a problem hiding this comment.
I think technically that would poll as fast as it reasonably can, but that is really the responsibility of the server to define. I think per the spec we are supposed to honor the servers request, and use 5 seconds in the even it is omitted.
|
|
||
| // Check expiration - An attempt without an expiration is by default | ||
| // expired. | ||
| // TODO: Change expires_at to be non-optional. A login attempt should always |
There was a problem hiding this comment.
Is now a reasonable time to have a migration that sets a default value (0) for any existing rows and updates the database to be non-nullable and drops the Option<> from expired_at in LoginAttemptModel and LoginAttempt?
There was a problem hiding this comment.
I think I want to put that into a separate PR at least. I'm still not 100% on if this should be an option or not.
2 participants
A bunch of work that restructures how we do OAuth.
v-cli-sdkwith prebuilt commands for authentication and configuration that v-api based CLI applications can embed