Skip to content

Send login_hint with the device authorization request - #841

Merged
jeremy merged 2 commits into
mainfrom
device-login-hint
Sep 3, 2026
Merged

Send login_hint with the device authorization request#841
jeremy merged 2 commits into
mainfrom
device-login-hint

Conversation

@jeremy

@jeremy jeremy commented Sep 2, 2026

Copy link
Copy Markdown
Member

RFC 8628 §3.1 lets a device client name the user it expects to approve the code (login_hint). Basecamp's device verification page is growing support for it, so basecamp auth login --login-hint <email> can steer a shared machine — or a person with several identities — to the right sign-in. The server never treats the hint as authentication; approval still comes from whoever holds the session.

What

  • oauth.WithDeviceLoginHint(hint) — a DeviceOption that adds login_hint to the device authorization POST. Unset leaves the form exactly as it was.
  • SPEC §16 requestDeviceAuthorization gains the optional loginHint? parameter, marked Go-only for now: no other SDK has a caller asking for it, and the option is additive, so parity can follow when one does.

Consumer

basecamp-cli wires --login-hint through LoginOptions.LoginHint and appends this option once it bumps to an SDK that carries it (basecamp/basecamp-cli — "auth login --with-token" PR).

Verification

  • cd go && go vet ./pkg/basecamp/oauth && go test ./pkg/basecamp/oauth — green. New TestRequestDeviceAuthorization_SendsLoginHintWhenSet covers both the sent form and the omitted-when-unset form.
  • gofmt -l clean. make lint did not run locally (golangci-lint on this machine predates the Go 1.27 export-data format and fails typecheck on every package, main included); CI's lint is the gate.

Summary by cubic

Adds oauth.WithDeviceLoginHint so oauth.RequestDeviceAuthorization can include Basecamp’s optional RFC 8628 login_hint, steering shared or multi-identity machines to the intended sign-in. Without the option, requests are unchanged; the hint never authenticates, and approval still comes from whoever holds the session.

Spec

  • Documents optional loginHint? support for device authorization and device login as Go-only until another SDK needs it.

Written for commit f9426ed. Summary will update on new commits.

Review in cubic

Copilot AI balanced review requested due to automatic review settings September 2, 2026 19:53
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-02T20:17:49.086625Z f9426ed New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions github-actions Bot added the go label Sep 2, 2026
RFC 8628 §3.1 lets a device client tell the authorization server which user
it expects to approve the code. Basecamp's device verification page is
growing support for it, so basecamp-cli can steer a shared machine (or a
person with several identities) to the right sign-in without the server
ever treating the hint as authentication.

WithDeviceLoginHint adds the parameter to the device authorization POST;
unset leaves the form unchanged. Go only for now — the other SDKs have no
caller asking for it yet, and SPEC §16 records the parameter as optional.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The specification and public Go documentation incorrectly attribute login_hint to RFC 8628.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds optional login_hint support to Go device authorization requests.

Changes:

  • Adds WithDeviceLoginHint.
  • Sends and tests optional login_hint.
  • Updates the device-flow specification.

[!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.

File summaries
File Description
SPEC.md Documents the optional parameter.
go/pkg/basecamp/oauth/device.go Implements the option and request field.
go/pkg/basecamp/oauth/device_test.go Tests inclusion and omission.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread SPEC.md Outdated
Comment thread go/pkg/basecamp/oauth/device.go Outdated
jeremy added a commit to basecamp/basecamp-cli that referenced this pull request Sep 2, 2026
A bot's Basecamp identity should never come from a browser sign-in on a
shared machine: whoever's session the browser holds is who the profile
quietly becomes, and nothing checks. Basecamp now mints personal access
tokens, so the CLI can take one from a secret store and refuse to keep it
unless it authenticates as the expected identity.

`basecamp auth login --with-token -P <profile> --account <id>` reads the
token from stdin (a terminal is refused, with the `op read … |` shape
shown), stores it under the profile as a non-expiring bc5 credential —
ExpiresAt 0 is the path AccessToken already never refreshes — and creates
the profile when --account names its account. The profile entry is written
only after /my/profile.json and the authorization endpoint have answered
for the token; a rejected token leaves the profile exactly as it was found,
previous credential included. Profile registration is factored out of
`profile create` so both paths write the same entry, and the root pre-run
lets a may-create command name a profile that does not exist yet.

`--expect-identity <id>` makes every login assertive — browser and device
flows included — discarding the new credential on a mismatch. `--json`
returns the envelope (profile, account, identity, person, oauth_type,
scope, expires_at null), and the interactive flows now refuse machine
output modes up front instead of printing prose and blocking (the
machine-mode half of #669).

`BASECAMP_OAUTH_ISSUER` pins the BC5 authorization server and skips
discovery, so a device login reaches a server that is piloting the client
while its metadata is still dark. It is a temporary escape hatch and is
documented as one. `--login-hint <email>` is wired through LoginOptions;
the pinned SDK cannot put it on the wire yet (basecamp/basecamp-sdk#841
adds the option), so this build tells the user the hint instead of sending
it, and Launchpad ignores it.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed

You’re at about 97% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread SPEC.md Outdated
Comment thread SPEC.md
RFC 8628 §3.1 defines only client_id and scope for the device authorization
request and leaves room for extension parameters; login_hint is Basecamp's,
named after the OpenID Connect parameter. Say so in the option's doc and in
SPEC §16, and thread loginHint through performDeviceLogin's signature there,
since Go's PerformDeviceLogin forwards the option.
@jeremy
jeremy merged commit 47e7ca3 into main Sep 3, 2026
44 of 45 checks passed
@jeremy
jeremy deleted the device-login-hint branch September 3, 2026 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants