Skip to content

Handle OAuth denial and deduplicate login output - #260

Merged
rgarcia merged 5 commits into
mainfrom
hypeship/oauth-denial-message
Sep 17, 2026
Merged

rgarcia merged 5 commits into
mainfrom
hypeship/oauth-denial-message

Conversation

@rgarcia

@rgarcia rgarcia commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Summary

OAuth consent denial returns error=access_denied without a code. The CLI previously treated that valid error response as a malformed success callback, showing “missing authorization code” in the browser. Native login also emitted duplicate terminal statuses and reported success before credentials were persisted.

  • Handle OAuth errors after state validation and before requiring a code. Denial returns a distinct error without entering token exchange.
  • Reuse the callback page layout for a clear branded denial page without a success checkmark. Provider-supplied error descriptions are not reflected.
  • Stop and erase the waiting spinner without printing a second status. The root renderer now emits the single meaningful denial, cancellation, or authentication error with a nonzero exit.
  • Classify cancellation from the authentication error itself so a racing context cancellation cannot mask an unrelated failure.
  • Print one success only after credentials are saved. Credential-save failures retain the storage error and reauthentication guidance and now exit nonzero.
  • Preserve existing stored credentials on denial.
  • Extract the callback and root error handlers for focused rendered-output regression coverage.

Validation

  • go test ./pkg/auth ./cmd -count=1
  • go test -race ./pkg/auth -count=1
  • go vet ./pkg/auth ./cmd
  • go build ./...
  • Focused output tests cover success, consent denial, generic authentication failure, cancellation races, credential-save failure, and the real spinner erase boundary.
  • Earlier built-binary loopback fixture: simulated browser launch and denial callback produced one clear terminal error, exit code 1, zero token requests, and unchanged fixture credentials. This used isolated fixture configuration with keyring access disabled, not a live provider login.
  • Chromium rendering at desktop and mobile widths: denial text, embedded favicon, no success checkmark, and no horizontal overflow. Existing callback layout and colors are retained.
  • Build, test, Semgrep, Socket Security, and BugBot checks pass on d99e4da9b708e119211d76dec6e5047305d8858f.

Released as v0.39.1 from merge commit 76daab28eabfc97bdb729d3fa579d9e2bbe99ecb. A live provider denial retest is still pending; the loopback fixture above was local only.

@cursor cursor 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.

Stale Bugbot comment from a previous run.

Comment thread cmd/login.go
@rgarcia rgarcia changed the title Handle OAuth consent denial without missing-code errors Handle OAuth denial and deduplicate login output Sep 17, 2026

@masnwilliams masnwilliams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

approved. this is a focused improvement: OAuth denial is handled after state validation, terminal output now has one owner, success is reported only after credentials are persisted, and the new failure paths have solid coverage. the callback refactor also leaves the files easier to navigate.

left two non-blocking follow-ups around cancellation classification and coverage of the real spinner behavior.

Comment thread cmd/login.go Outdated
Comment thread cmd/login_test.go
@rgarcia

rgarcia commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor 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.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit d99e4da. Configure here.

@rgarcia
rgarcia merged commit 76daab2 into main Sep 17, 2026
8 checks passed
@rgarcia
rgarcia deleted the hypeship/oauth-denial-message branch September 17, 2026 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants