Skip to content

[CFX-7608] Classify server errors on the dr auth check legs and split 403 from 401 - #911

Open
chasdr wants to merge 4 commits into
mainfrom
chas/CFX-7608-auth-check-legs
Open

[CFX-7608] Classify server errors on the dr auth check legs and split 403 from 401#911
chasdr wants to merge 4 commits into
mainfrom
chas/CFX-7608-auth-check-legs

Conversation

@chasdr

@chasdr chasdr commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

PR 2 of 3: #776 was PR 1, #912 is PR 3

Summary

The .env and stored-profile legs of dr auth check blamed the API token for any non-200 from GET /version/. A 503, 404, or 429 told the user their token was invalid, and on the stored profile the gate reopened the browser login flow, when the instance was what failed. PR #776 fixed this for the DATAROBOT_ENDPOINT/DATAROBOT_API_TOKEN pair; this extends the same classification to the other two legs.

It also splits 403 out from 401. A 403 on /version/ authenticates the key, then the account or agreement refuses it (a deactivated account, or an unsigned clickthrough agreement), so a fresh login mints another key that 403s the same way. A 403 now reports that the account lacks access and stops the relaunch. 401 keeps its exact wording and still opens login.

Output

Stored profile, against local instances returning each status:

$ dr auth check
❌ No valid API key found in CLI config.          # 401: token rejected, relaunch login
Run dr auth login to authenticate.

$ dr auth check
❌ http://127.0.0.1:8972 accepted your credentials but your account lacks API access (HTTP 403).
Check that your account is activated and any required agreement is signed.

$ dr auth check
❌ http://127.0.0.1:8973 answered HTTP 503, so the CLI could not verify your credentials.
Check the configured DataRobot endpoint, and the instance's status if it persists.

Technical Changes

  • internal/auth/auth.go: fprintServerStatus splits 403 (reports lack of access, returns true so the gate stops relaunching) from 401 (returns false, relaunch). 401 wording unchanged.
  • cmd/auth/check/cmd.go: checkCLICredentials and verifyDotenvToken defer to ReportUnjudged; the dotenv print helpers take an io.Writer.
  • docs/development/authentication.md, docs/commands/auth.md: flow and example output.
  • Tests: per-status tables for both check legs, 403 flips in the auth suite, new stored-profile 403 gate test.

Breakdown

  • code: +58 / -58
  • tests: +124 / -40
  • docs: +8 / -3

Note

Medium Risk
Changes when the auth gate starts login and what users see for stored profile, .env, and env credentials—security-sensitive paths, but behavior is narrow and heavily covered by new tests.

Overview
Extends HTTP status classification to the stored-profile and .env legs of dr auth check, matching the env-var behavior from the prior PR: only 401 is treated as an invalid token; 503/404/etc. blame the instance, and 403 is handled separately.

403 is no longer lumped with 401. A forbidden response means the key authenticated but the account lacks API access, so messaging says to check activation/agreements and EnsureAuthenticated does not reopen browser login (a new key would 403 the same way). 401 still prompts dr auth login / token refresh wording.

checkCLICredentials and verifyDotenvToken now delegate diagnostics to auth.ReportUnjudged; dotenv error helpers write to an io.Writer. Fresh installs with no stored token keep the login hint instead of endpoint parse noise. Docs and table-driven tests cover both check legs plus the stored-profile 403 gate.

Reviewed by Cursor Bugbot for commit fac5234. Configure here.

Why:
A 403 from GET /version/ authenticated the key, then the account or agreement
refused it (deactivated account, or an unsigned clickthrough agreement). A fresh
login mints another key that 403s the same way, so relaunching login is futile.
Before this, a 403 blamed the token like a 401 and reopened the browser login flow.

Changes:
- fprintServerStatus: 401 alone returns false (relaunch). 403 reports that the
  account lacks access and returns true, so the gate stops the relaunch through
  ReportUnjudged. 401 wording unchanged.
- Tests: 403 row flips in TestReportUnjudged and TestReportEnvCredentialsError;
  new TestEnsureAuthenticated_StoredProfile403.
- docs: authentication.md flow and auth.md example output.
Why:
The .env and stored-profile legs of dr auth check blamed the token for any
non-200. A 404, 429, or 5xx told the user their token was bad when the instance
was what failed. PR #776 fixed this for the DATAROBOT_ENDPOINT/DATAROBOT_API_TOKEN
pair; this extends the same classification to the other two legs.

Changes:
- checkCLICredentials and verifyDotenvToken defer to auth.ReportUnjudged; only a
  real 401 falls through to the dr auth login / dr dotenv update advice.
- verifyDotenvToken and the dotenv print helpers take an io.Writer for testing.
- Tests: per-status tables for both legs (401/403/404/503).
@datarobot-pr-review-router

Copy link
Copy Markdown

🎫 Jira: CFX-7608 — dr still blames credentials for server errors on the .env and stored-profile paths

@chasdr
chasdr requested a lite review from Copilot September 10, 2026 22:34
@github-actions github-actions Bot added the go Pull requests that update go code label Sep 10, 2026
@chasdr
chasdr marked this pull request as ready for review September 10, 2026 22:35
@chasdr
chasdr requested a review from a team as a code owner September 10, 2026 22:35

This comment was marked as resolved.

@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/auth/check/cmd.go
…th check

Why:
Routing every GetAPIKey failure through ReportUnjudged sent a fresh install
(empty stored endpoint and token) into unusableEndpoint, which reported "missing
URL scheme" and dropped the "Run dr auth login" advice. EnsureAuthenticated
guards its ReportUnjudged call on a stored token being present; checkCLICredentials
did not.

Changes:
- Skip ReportUnjudged when no token is stored; print the login advice instead.
- Test: a fresh install keeps the login advice and shows no missing-URL-scheme.
The old contract said false means the instance rejected the credentials, but a
403 is a verdict that now returns true to suppress the login relaunch. Restate
what true and false mean so a caller does not read true as unjudged-only.

Copilot AI 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.

🟢 Approval recommended

No unresolved issues were identified.

Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@chasdr

chasdr commented Sep 10, 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 fac5234. Configure here.

@datarobot-pr-review-router

Copy link
Copy Markdown

Code Ownership

Cli Maintainers

  • cmd/auth/check/cmd.go
  • cmd/auth/check/cmd_test.go
  • docs/commands/auth.md
  • docs/development/authentication.md
  • internal/auth/auth.go
  • internal/auth/auth_test.go

Review requested from the teams above. Labels will be removed automatically upon approval.

@chasdr

chasdr commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

/approve-smoke-tests

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

good stuff. 🚢

Out of scope for this PR: the new 403 line prints the endpoint through hostOrEndpoint, and SchemeHostOnly preserves URL.User, so an endpoint with embedded userinfo (https://user:pass@host) puts the password on stderr (internal/auth/auth.go:260). This should be handled as part of CFX-7608 rather than this PR — the ticket already tracks the family (the timeout and could-not-connect lines have leaked it since #751), and the fix belongs at the hostOrEndpoint print site or in the remaining set-url PR, since SchemeHostOnly is shared with set-url/export/VerifyEnvCredentials and those need the URL as written.

Comment on lines +55 to +57
{"401 blames the token", http.StatusUnauthorized, "No valid API key found", ""},
{"403 reports lacking access", http.StatusForbidden, "lacks API access", "No valid API key found"},
{"503 blames the instance", http.StatusServiceUnavailable, "answered HTTP 503", "No valid API key found"},

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.

[P3] Add the stored-profile 404 row the commit message claims

The commit says per-status tables for both legs (401/403/404/503), but this table stops at 401/403/503 — the .env table below has all four. This is the leg the PR is adding (checkCLICredentials' new ReportUnjudged wiring), so the missing row is exactly the new code path's coverage gap.

Suggested change
{"401 blames the token", http.StatusUnauthorized, "No valid API key found", ""},
{"403 reports lacking access", http.StatusForbidden, "lacks API access", "No valid API key found"},
{"503 blames the instance", http.StatusServiceUnavailable, "answered HTTP 503", "No valid API key found"},
{"401 blames the token", http.StatusUnauthorized, "No valid API key found", ""},
{"403 reports lacking access", http.StatusForbidden, "lacks API access", "No valid API key found"},
{"404 blames the instance", http.StatusNotFound, "answered HTTP 404", "No valid API key found"},
{"503 blames the instance", http.StatusServiceUnavailable, "answered HTTP 503", "No valid API key found"},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

00 - Ready for Review go Pull requests that update go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants