BC5 OAuth: device flow (RFC 8628, 5 SDKs)#370
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2890db6e63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Implements the OAuth 2.0 Device Authorization Grant (RFC 8628) contract in SPEC.md and rolls it out across the TypeScript, Python, Ruby, Go, and Kotlin SDKs, including deterministic tests and end-user documentation.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Changes:
- Adds RFC 8628 device-flow primitives (
requestDeviceAuthorization,pollDeviceToken) plus an orchestrator (performDeviceLogin) with capability guarding and typed terminal errors. - Implements RFC 8628 polling behavior (sustained
slow_down, timeout backoff, monotonic deadline / injectable clock, cancellation awareness) with per-SDK test coverage. - Updates SDK READMEs and exports to document and expose the new device-flow APIs.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| typescript/tests/oauth/device.test.ts | Adds deterministic Vitest coverage for device auth, polling, cancellation, and error mapping. |
| typescript/src/oauth/types.ts | Introduces DeviceAuthorization type used by device-flow APIs. |
| typescript/src/oauth/index.ts | Exports device-flow APIs, types, and error types from the oauth entrypoint. |
| typescript/src/oauth/device.ts | Implements device authorization request + token polling loop for RFC 8628. |
| typescript/src/oauth/device-login.ts | Adds performDeviceLogin orchestrator and capability guard. |
| typescript/src/oauth/device-errors.ts | Defines DeviceFlowError + reason→category mapping for terminal device-flow outcomes. |
| typescript/README.md | Documents how to use device flow and the derived error taxonomy. |
| SPEC.md | Adds the cross-SDK device-flow contract (RFC 8628) and terminal-outcome mapping. |
| ruby/test/basecamp/oauth_device_test.rb | Adds deterministic Minitest coverage for request/poll/orchestrate behavior. |
| ruby/README.md | Documents device authorization usage and error taxonomy. |
| ruby/lib/basecamp/oauth/device_flow.rb | Implements request/poll/orchestrate device flow for Ruby. |
| ruby/lib/basecamp/oauth/device_flow_error.rb | Adds Ruby DeviceFlowError with derived parent error type. |
| ruby/lib/basecamp/oauth/device_authorization.rb | Defines Ruby DeviceAuthorization value object for the device/user code pair. |
| ruby/lib/basecamp/oauth.rb | Exposes device-flow functions as public OAuth entrypoints. |
| python/tests/oauth/test_device.py | Adds pytest coverage for request/poll/orchestrate behavior and error mapping. |
| python/src/basecamp/oauth/errors.py | Adds Python DeviceFlowError and reason→parent-category mapping (incl. usage). |
| python/src/basecamp/oauth/device.py | Implements synchronous device-flow request/poll/orchestrate functions. |
| python/src/basecamp/oauth/device_authorization.py | Defines Python DeviceAuthorization dataclass. |
| python/src/basecamp/oauth/init.py | Re-exports device-flow APIs and types from the oauth package. |
| python/README.md | Documents device flow usage and terminal error mapping. |
| kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/OAuthDeviceTest.kt | Adds deterministic coroutine/virtual-time tests for device flow. |
| kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/ErrorTest.kt | Extends error classification tests to include the new DeviceFlow exception type. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/oauth/Exchange.kt | Makes refreshToken client secret optional for public clients. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/oauth/Device.kt | Implements device flow in Kotlin (request/poll/orchestrate + TimeSource-based deadline). |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/BasecampException.kt | Introduces BasecampException.DeviceFlow with reason-derived taxonomy and defaults. |
| kotlin/README.md | Documents Kotlin device authorization flow usage and behavior. |
| go/README.md | Documents Go device flow usage, capability guard, and error taxonomy behavior. |
| go/pkg/basecamp/oauth/device.go | Implements device flow for Go with bounded reads, backoff, deadline, and cancellation semantics. |
| go/pkg/basecamp/oauth/device_test.go | Adds deterministic Go tests for request/poll/orchestrate behavior including malformed 2xx token responses. |
| go/pkg/basecamp/oauth/device_errors.go | Adds DeviceFlowError type with reason-derived taxonomy and retryability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
3 issues found across 30 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="typescript/src/oauth/types.ts">
<violation number="1" location="typescript/src/oauth/types.ts:82">
P2: Consumers using the documented `@37signals/basecamp` entrypoint cannot name the new `DeviceAuthorization` type because it is only exported from `@37signals/basecamp/oauth`. Re-exporting it from the root entrypoint would keep the public device-flow API usable without a subpath import.</violation>
<violation number="2" location="typescript/src/oauth/types.ts:92">
P2: Malformed device responses with fractional `expires_in` or `interval` values are accepted and used to calculate polling deadlines and delays. The response validation should require finite integer values before constructing this type.
(Based on your team's feedback about rejecting non-integral semantic integer fields.) [FEEDBACK_USED].</violation>
</file>
<file name="typescript/README.md">
<violation number="1" location="typescript/README.md:238">
P2: This example does not compile because the package root does not export `performDeviceLogin` or `DeviceFlowError`. Import them from `@37signals/basecamp/oauth` (or add root re-exports) so the documented example is usable.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
b012173 to
336df9d
Compare
2890db6 to
b8d4cbb
Compare
Review round 2 — pushed (history folded, 6 commits/branch preserved)Addressed the genuine blockers from the automated + manual review. Fixes are folded into their owning per-SDK commits (discovery-layer → PR #369, device-layer → PR #370); both branch tips pass full Discovery (PR #369):
Device flow (PR #370):
Dismissed — false positive: the Ruby IPv6 claim is incorrect under the project Ruby — Trivy (Go SDK) check: the failure is a scanner-version/DB discrepancy on an unchanged Still under the agreed merge hold pending bc3 #9471. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8d4cbb813
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
336df9d to
64e2ef3
Compare
b8d4cbb to
7207f74
Compare
httpx's timeout is per-read (it resets on every received chunk) and httpx has no total-request timeout, so a peer slow-dripping a VALID response byte-by-byte — each read under the timeout — could hold a device request open far past it (verified: a 0.5s timeout took ~20s under a 0.3s drip). The prior monotonic- deadline checks only ran between chunks and could not bound the blocking header read; closing the client from a watchdog does not interrupt a blocked read either. Run the request on a daemon worker and bound the CALLER with join(timeout): a stalled/drip request now returns within the timeout as a retryable transport failure, and the abandoned daemon worker never blocks exit. Adds a real-socket drip test asserting the bound. Also harden _normalize_timeout to validate its operation default (extract _finite_positive_timeout): a caller passing an invalid default can no longer disable both bounds — it falls back to the discovery constant.
- parse_device_authorization parsed JSON before checking status, so a non-2xx with a non-JSON body (common for 500/502) surfaced as a parse error instead of 'failed with status …'. Check status first (matching discovery and the Python request path); the token poll still parses non-2xx bodies for authorization_pending. - request_device_authorization / poll_device_token now normalize the timeout ONCE at operation entry (outside the poll loop) and pass the SAME value to build_client and every post_form, so an invalid input can't leave the default-built client's socket timeout unbounded. post_form no longer re-normalizes.
…fication_uri_complete (TS) - requestDeviceAuthorization and pollDeviceToken hard-coded 30000 in their parameter defaults while DEFAULT_DEVICE_TIMEOUT_MS existed — reference the constant so the omitted-default and normalized-invalid paths can't diverge. - RawDeviceAuthorization.verification_uri_complete is typed string | null to reflect that a JSON null on the wire is accepted as absent (normalized to undefined), matching the Go/Kotlin decoders.
… wait The daemon-worker bound (previous commit) bounded the CALLER but could not terminate the leaked worker/socket under a sustained drip, and a huge finite timeout overflowed thread join. Two fixes: - Run the request as a coroutine under asyncio.wait_for, which CANCELS it and closes the socket at the deadline (verified: caller bounded AND no leaked worker — thread count unchanged, unlike a sync thread that can't be killed). httpx has no total-request timeout and its per-read timeout resets on drip. - Cap the request timeout at _MAX_DEVICE_REQUEST_TIMEOUT (3600s) via a maximum= arg on _normalize_timeout so a huge value falls back to the default instead of overflowing the wall-clock wait primitive. Also make the poll wait interruptible: time.sleep is not cancellable, so a cancel set mid-wait was not seen until the whole (possibly grown slow_down) interval elapsed. _wait_cancellable polls should_cancel every _CANCEL_POLL_INTERVAL when a probe is set (single sleep otherwise, preserving the schedule) — matching the ctx/AbortSignal/coroutine cancellation Go/TS/Kotlin already have. Adds tests.
A plain sleep is not cancellable, so a cancellation set during the wait was not observed until the whole (possibly grown slow_down) interval elapsed. Add wait_cancellable: with the default no-op probe a single sleep preserves the exact wait schedule; with a real cancelled probe it polls every CANCEL_POLL_INTERVAL_SECONDS and raises promptly — matching Go/TS/Kotlin's cancellable waits. Adds a prompt-cancellation test.
asyncio.run() raises RuntimeError when the sync device helpers are invoked from code that already has a running loop (Jupyter/FastAPI/async CLI), leaking a raw error before any request is made. Run the asyncio.wait_for-bounded coroutine in a DEDICATED thread with its own event loop instead: it works from any context, and because wait_for cancels the request at the deadline the thread always completes within ~timeout (joining it never blocks; no leaked worker). Typed OAuthError/DeviceFlowError/ReadTimeout are re-raised on the caller thread. Also clamp the final _normalize_timeout fallback to a caller-supplied maximum below _DISCOVERY_TIMEOUT, so an impossible maximum can't be bypassed. Adds the inclusive-boundary and sub-default-maximum tests.
Inserting wait_cancellable left the POST doc-comment stranded above it; move the POST contract back directly above post_form and keep wait_cancellable's own doc with wait_cancellable. Comment-only.
… (Python) - The bounded coroutine now runs on a DAEMON worker joined with a small grace (timeout + _WORKER_JOIN_GRACE): asyncio.wait_for cancels the request at the deadline so the worker normally finishes well within it, but if the async cancellation/cleanup itself stalls, the caller returns a timeout rather than block on an unbounded join — the daemon never blocks interpreter exit. - Re-sync backoff to the grown interval after slow_down: the reset used the pre-increment value, so a following timeout doubled the stale interval (5→10) instead of the grown one (10→20), polling too aggressively under combined throttling + timeouts. Adds a slow_down-then-timeout schedule test ([5,10,20]).
After slow_down raised the interval, a following poll timeout doubled the stale pre-increment backoff (5→10) instead of the grown interval (10→20). Re-sync the backoff to the grown interval. Adds a timeout-on-attempt transport + a slow_down-then-timeout schedule test asserting [5, 10, 20].
Re-sync backoff to the grown interval after slow_down so a following timeout doubles from the new interval, not the stale pre-increment one.
Re-sync backoffSeconds to the grown interval after slow_down so a following timeout doubles from the new interval, not the stale pre-increment one.
The signal could abort between the initial aborted-check and attaching the
listener; with { once } that abort event is already spent, so the listener never
fires and the promise settles only at the full timeout. Re-check signal.aborted
after attaching and handle it manually, dropping the now-dead listener.
…(TypeScript)
- Device-auth and token-poll paths read the response body BEFORE checking status,
so a non-2xx / suppressed-3xx response that slowly streamed (or never finished)
its body could time out mid-read and be misclassified: a hard api_error surfaced
as a retryable transport failure (device-auth), or a redirecting token endpoint
kept the poll loop backing off and retrying until expiry instead of failing.
Check status first — a non-2xx (device-auth) or 3xx (token) whose body is unused
fails by status immediately; a 4xx token body is still read for
authorization_pending/slow_down.
- Validate a token as a non-empty string: a non-string (e.g. {"error":123})
no longer becomes the OAuth error code — fall back to http_<status>, matching the
other SDKs. Adds tests.
data.get("error") or f"http_{status}" accepted any truthy non-string (dict/number)
as the OAuth error code. Treat a non-string/empty error as absent and fall back to
http_<status>, matching the other SDKs. Adds a test.
RequestDeviceAuthorization drained the body BEFORE checking status, and the token poll read the body before classifying a 3xx. A non-2xx/3xx that slowly streamed its body could hit the request timeout mid-read and be misclassified — a hard api_error as a retryable transport failure, or a redirecting token endpoint kept the poll backing off and retrying until expiry. Check status first: a non-2xx device-auth and a 3xx token response (whose bodies are unused) fail by status immediately; a 4xx token body is still read for authorization_pending/slow_down. Adds an oversized-3xx test asserting it fails by status, not the size cap.
…cript) The status-first throw branches (non-2xx device-auth, 3xx token) left response.body undrained, so under repeated failures the response stream could retain sockets / connection-pool resources. Cancel it non-blockingly before throwing; the status error still surfaces immediately.
The token poll read the whole body before the status branches, so a redirect that slowly streamed its body could time out mid-read and be retried by the poll loop until expiry. Return the redirect api_error by status BEFORE readBoundedText; a 4xx body is still read for the OAuth error. Device-auth already checked status first.
_post_form_bounded read the whole body before the caller checked status, so a slow non-2xx (device-auth) or 3xx (token) body could time out mid-read and be misclassified as transport / retried until expiry. Add a read_body(status) predicate — evaluated once headers arrive — so a caller returns without draining a body it does not use (device-auth reads only 2xx; the poll skips 3xx, still reads 4xx). Adds an oversized-3xx test asserting it fails by status, not the size cap.
Faraday streams the body during the synchronous request, so a slow non-2xx/3xx body could time out (→ transport / poll retry-until-expiry) before post_form checked status. Faraday passes env.status to on_data once headers arrive: a new skip_status predicate raises Fetcher::SkipBody from on_data for a status whose body the caller does not use (device-auth non-2xx, token 3xx), and post_form returns it empty for the caller to classify. Adds a streaming-adapter test asserting an oversized 302 body fails by status, not the size cap.
PollDeviceToken is an exported entry point: a non-positive expiresIn built a deadline in the past, and an oversized one overflowed the time.Duration(seconds) * time.Second math into a garbage deadline. Reject both up front as a usage error, mirroring the TS pollDeviceToken guard. The PerformDeviceLogin path is unaffected: it polls via pollDeviceTokenUntil with already-validated, clamped values.
poll_device_token is a public entry point: a non-finite expires_in (inf) built a deadline that never passes — an unbounded poll loop — and zero/negative/ oversized/bool durations are not schedulable waits. Reject them up front as a usage error, mirroring the TS/Go caller guards. perform_device_login is unaffected: it passes the server-validated interval and a positive fractional remaining lifetime, both within the shared ceiling. Also align the worker-join comment with the is_alive backstop it sits above, note the 3.11+ TimeoutError aliasing the isinstance check relies on, and complete the docstring raise list (usage, api_error).
…adapter (Ruby) The on_data SkipBody fast-path only fires when the adapter streams AND passes env (Faraday >= 2.5). A buffered adapter that ignores on_data, an older Faraday (2.0-2.4) that omits env, or a header-only response reached post_form with the skip-status body un-skipped, so an oversized 3xx tripped the size cap instead of the redirect api_error. Re-apply skip_status to the completed response.status as a backstop, and state the one genuinely unreachable case honestly in place: Faraday has no headers-time callback, so a body that stalls PAST the read timeout surfaces as a bounded transport timeout (never followed, never unbounded); exact headers-first semantics for that case belong to the transport primitive shared with discovery, tracked as a pre-go-live hardening follow-up. Also reject out-of-range caller durations at the public poll entry as usage (nil interval raised NoMethodError; an infinite expires_in built a deadline that never passes), mirroring the Go/TS/Python guards; coerce integer-valued Float expires_in/interval to Integer, matching build_token and the declared types; and fold build_client into Fetcher.build_client, which it duplicated.
DeviceLoginOptions.signal is deliberately scoped to the polling loop — the device-code request is bounded by its own timeout and the display hook is caller code; say so on the option. defaultClock: name the Date.now() branch for what it is, a wall-clock last resort for runtimes without performance, so the MonotonicClock label cannot be read as a guarantee the fallback does not keep.
…tlin)
A 4xx token body of {"error":""} decodes cleanly (error is a required non-null
String, so no SerializationException fires) and surfaced a dangling message;
normalize it to http_<status> via ifEmpty, matching the coercion Go, TS, Python,
and Ruby already apply.
Reject out-of-range interval/expiresIn at the exported pollDeviceToken entry as
a usage error: an oversized Long saturates Duration to infinite, building a
deadline that never passes — an unbounded poll loop — and non-positive values
are not schedulable. performDeviceLogin is unaffected (it passes
server-validated values and its own issuance-anchored deadline).
The token 3xx early return needs no explicit channel cancel: HttpStatement
.execute runs cleanup in its finally, which cancels the raw content channel
(verified against ktor-client-core 3.5.1); state that guarantee in place.
Complete the BasecampException KDoc when-example with the Ambiguous branch.
…Python)
Two guard escapes on the public entries:
The caller-duration guard ran math.isfinite before the range checks, and
isfinite converts its argument to float — so an astronomically large int
(10**400) raised OverflowError out of the guard instead of the usage error.
Range checks now run first (int comparisons never convert; NaN compares False
on both and falls through to the isfinite reject).
max_body_bytes was forwarded unnormalized, so an invalid runtime cap
(float("inf"), None, a negative) disabled the streaming memory bound —
total > inf never trips — despite the documented 1 MiB guard. Normalize at
both public entries via _normalize_body_cap, now parameterized with an
operation-specific default (validated too), mirroring _normalize_timeout.
max_body_bytes was forwarded unnormalized into bounded_reader and the buffered fallback, so an invalid runtime cap (Float::INFINITY, nil, a negative) on a direct DeviceFlow call disabled the size bound the methods advertise. Add Fetcher.normalize_body_cap (non-negative Integer, validated default — the same discipline the discovery initializer applies) and normalize once at the request/poll entries alongside the existing timeout normalization.
…ice flow (Ruby) Discovery#initialize and Resource#initialize carried inline copies of the cap normalization that Fetcher.normalize_body_cap now implements; route both through the shared helper so the three consumers cannot drift.
BasecampException.Api defaults retryable to httpStatus in 500..599, so a completed 5xx from the device-auth or token endpoint read as retryable — but in the device flow only the transport reason is retryable, and the other four SDKs surface these as a non-retryable api_error (Python/Ruby default false, Go zero value, TS unset). Pass retryable = false at the three 5xx-capable raise sites: the device-auth non-2xx classification, the token PollResult.Other fault, and the shared readBoundedText size cap (device polls read 5xx bodies; an oversized body is a completed fault in discovery too).
| var raw struct { | ||
| AccessToken string `json:"access_token"` | ||
| RefreshToken string `json:"refresh_token"` | ||
| TokenType *string `json:"token_type"` | ||
| ExpiresIn *float64 `json:"expires_in"` | ||
| Scope string `json:"scope"` | ||
| } | ||
| if err := json.Unmarshal(body, &raw); err != nil { | ||
| return pollResult{kind: pollInvalidResponse, status: resp.StatusCode, err: fmt.Errorf("parsing device token response: %w", err)} | ||
| } | ||
| if raw.AccessToken == "" { | ||
| return pollResult{kind: pollInvalidResponse, status: resp.StatusCode, err: errors.New("device token response missing access_token")} | ||
| } | ||
| if raw.TokenType != nil && *raw.TokenType == "" { | ||
| return pollResult{kind: pollInvalidResponse, status: resp.StatusCode, err: errors.New("device token response token_type must be a non-empty string")} | ||
| } | ||
| token := Token{ | ||
| AccessToken: raw.AccessToken, | ||
| RefreshToken: raw.RefreshToken, | ||
| TokenType: "Bearer", | ||
| Scope: raw.Scope, | ||
| } |
Device flow (RFC 8628)
Why this exists
Not every client can open a browser to sign in. Command-line tools, CI tasks, and input-constrained or headless devices need a login flow that works with nothing but a code and a URL the user visits elsewhere. This PR adds the OAuth 2.0 Device Authorization Grant (RFC 8628) to every SDK, giving those clients a first-class, browserless path to a Basecamp access token.
It is the second half of Basecamp's OAuth modernization: #369 taught the SDKs to discover the right authorization server; this PR teaches them to authenticate against it without a browser.
What you get — one uniform API across TypeScript, Python, Go, Ruby, and Kotlin
requestDeviceAuthorization(...)— begin the grant; returns the user code, verification URL, and polling parameters.pollDeviceToken(...)— poll to completion, honoring the server's pacing (authorization_pending, sustainedslow_down), a monotonic expiry deadline, backoff on transient transport failure, and caller cancellation.performDeviceLogin(...)— the end-to-end convenience path: takes an already-discovered config, guards that the server actually supports the device grant, surfaces the user code/URL through a display hook, and polls until the user approves.Terminal outcomes are a single typed
DeviceFlowErrorwhose category is derived from the reason (denied/expired → auth, transport → retryable network, unsupported → validation, cancelled → native cancellation), uniform across all five languages.Compatibility & rollout
device_codegrant. Today's Launchpad metadata advertises neither, soperformDeviceLoginreportsunavailableagainst it by design — the capability lights up when the first-party server does, with no client change required.basecamp-cliclient (no client secret, grantsdevice_code+refresh_token).Design intent
slow_downincrements; anchors expiry to a real deadline rather than a running counter; and never overflows timer arithmetic. Durations are validated uniformly across languages (positive whole seconds within a safe bound; integer-valued floats accepted, fractional rejected).api_error— never a raw crash or a misclassified retryable transport error.Verification
make check— all five SDKs (plus Swift) and the conformance/lint gates — passes at the branch tip.Summary by cubic
Adds the OAuth 2.0 device authorization flow (RFC 8628) to TypeScript, Python, Go, Ruby, and Kotlin so
basecamp-clican log in without a browser. Codifies and hardens the device-flow contract inSPEC.md§16 and updates SDK READMEs.New Features
requestDeviceAuthorization,pollDeviceToken,performDeviceLogin; plusDeviceAuthorizationandDEVICE_CODE_GRANT_TYPE.device_authorization_endpointandgrant_types_supported∋device_code(Launchpad-only configs returnunavailable).authorization_pendingand sustainedslow_down(+5s), uses a monotonic expiry deadline, is cancellation-aware, enforces HTTPS with no redirects, and separates server interval from timeout backoff.Bug Fixes
errormust be a non-empty string (elsehttp_<status>);expires_in(when present) is a positive whole number ≤ 2_147_483_647;token_typedefaults to Bearer when absent/null; explicit emptytoken_typeisapi_error; all malformed-field errors carryhttpStatus.httpStatuson all validation errors; acceptverification_uri_completenull as absent; Go models it as optional (*string).timeoutMs; Kotlin marks completed 5xx as non-retryable.usage); capability guard requires real array membership (no substring matches).Written for commit e9f1414. Summary will update on new commits.