OAuth rework#437
Conversation
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.
| 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.
|
|
||
| // 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