Connect Cloud auth: review follow-ups (refresh errors, credential selection, keyring, shared retry) - #839
Closed
samperman wants to merge 9 commits into
Closed
Connect Cloud auth: review follow-ups (refresh errors, credential selection, keyring, shared retry)#839samperman wants to merge 9 commits into
samperman wants to merge 9 commits into
Conversation
ConnectCloudClient._attempt_token_refresh logged every failure at debug and returned False, so an expired session or a revoked service account secret surfaced only as the original opaque 401. oauth.py now raises a typed InvalidGrantError when the token endpoint returns error=invalid_grant, carrying the server's error_description. The Cloud client acts on the two credential rejections it can explain: - invalid_grant on the refresh-token path clears connect_cloud_access_token and connect_cloud_refresh_token on the saved servers.json entry (account name/id and nickname are kept) and raises "Your Posit Connect Cloud session has expired and could not be renewed. Authenticate again with `rsconnect add --connect-cloud -n <name> -A <account>`." - invalid_client on the client-credentials path raises a message saying the service account credential was revoked or rotated, pointing at <auth host>/identity/credentials and the `rsconnect add` command with --client-id/--client-secret. The stored entry is left alone. Everything else — network failures, a rejected CLI OAuth client, unexpected responses — still returns False so the original 401 surfaces, but logs at warning instead of debug, matching the Connect refresh path. The servers.json write-back moved into _persist_tokens so the clearing path reuses the field-preserving update. Connect's refresh is unchanged: its generic `except Exception` already covers the new error type.
A saved Connect Cloud entry is a credential, not an account binding: the login behind it can publish to every account its user has rights on. So -A/--account no longer picks which saved credential to use, only where to publish. With one credential saved it is used whatever account is named; with several, -n/--name is now required and the error lists the saved nicknames with the account each publishes to by default. This drops the account-filter branch and its two error paths from ServerStore._get_connect_cloud_server, along with the now-dead account_name argument to get_by_url and resolve. Behavior change: With several saved credentials, -n selects the credential; -A no longer matches against entries and instead always selects the account to publish to, so `-n cred -A other-account` publishes there with that credential.
Connect Cloud tokens and service account client secrets now go to the system keyring, keyed "<url>#<nickname>" because every Connect Cloud entry records the same API URL. `rsconnect add` and token refresh write there when a keyring is available and leave the matching servers.json fields out, which moves the secrets of an entry saved before this change out of the file on its next add or refresh. Reads prefer the keyring and fall back to those fields, so a machine without a usable keyring (a CI runner) keeps working as before, and `rsconnect server remove` deletes the entries for the removed nickname. `rsconnect list` reports which of the two holds the credentials. The keyring helpers in oauth.py now take the entry key explicitly. Posit Connect keeps passing the bare server URL, so its "<url>:access_token" and "<url>:refresh_token" usernames are unchanged and existing logins are untouched. Tests get a conftest fixture that makes the keyring unavailable by default, since the module is installed in the test environment and would otherwise reach the machine's real keychain.
RSConnectClient and ConnectCloudClient each had their own copy of "send the request, on 401 mint a new token, send it once more". Both now inherit it from BearerTokenHTTPServer, which calls the subclass's _attempt_token_refresh to mint and apply the token and asks _can_refresh_token whether there is anything to mint from -- false for an API key, a bootstrap JWT, or a Snowflake token exchange. Connect Cloud gains the seekable-body rewind that only the Connect copy had, so a streamed body is not sent empty on the retry. The minting stays per-target, unchanged: Connect keeps discovery against its registered client and the InvalidClientError re-registration recovery, and Connect Cloud keeps the client-credentials-versus-refresh choice and its typed error handling. The keyring-with-servers.json-fallback load and write-back is already the same code on both sides, differing only in the key it is given; what remains target-specific is Connect's token expiry tracking and Connect Cloud's field-preserving write-back, which it skips for a run with no saved entry. Connect's three copies of "find the entry this server came from" become ServerStore.saved_entry. No behavior change other than the added rewind; every existing test passes unmodified.
|
The stream-body retry tests annotate returns as list[Any], which 3.8 evaluates at class-definition time and rejects. Deferring annotation evaluation with the __future__ import fixes collection for the file.
The autouse no_system_keyring fixture requested monkeypatch, hoisting the shared per-test instance ahead of every test-level fixture. Its undo then ran after those fixtures' cleanup, so a test using monkeypatch.chdir into a TemporaryDirectory had the directory deleted while it was still the working directory, which Windows rejects (WinError 32 in test_git_metadata teardown). The fixture now saves and restores sys.modules itself.
The teardown guard treated a missing sys.modules key the same as the fixture's own None marker, so a test that deleted the entry would raise KeyError during restore. The sentinel default now separates the cases.
Extracts the fixture body into an importable generator and adds tests driving each teardown branch: previous module restored, marker removed when nothing was stored, and a deleted key left deleted. The marker is reinstated through a fixture finalizer so a failing assertion cannot leak state into later tests.
Nothing asserted this raise; the Connect integration suite exercised it only by accident, and the -n/-A test rework there removed even that.
samperman
marked this pull request as draft
August 18, 2026 12:19
This was referenced Aug 18, 2026
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups from the review discussion on #837 (see #837 (comment)). One commit per item, intended to be read commit by commit:
95da59fd— actionable refresh failures ("Refresh failures should be actionable").invalid_granton the refresh-token path clears the dead stored tokens and raises with the exactrsconnect addcommand to re-authenticate (keeping a non-production-sURL and the saved entry's account); a rejected service-account secret raises with the credentials page URL and the re-save command; transient failures still surface the original 401, now logged at warning instead of debug.a3ef2684— nickname-only credential selection ("Is supporting multiple saved credentials for the same URL required?"). Entries are credentials; accounts are publish targets. With one saved credential it is used regardless of-A; with several,-npicks the credential.-Aalways selects the account to publish to, so-n cred -A other-accountpublishes there with that credential. Deployment records remain keyed by URL + account id.159a22ce— keyring-backed credential storage ("Could Connect Cloud reuse the existing keyring-backed OAuth token storage?"). Cloud tokens and stored client secrets live in the system keyring keyed<url>#<nickname>; reads prefer the keyring and fall back toservers.json; a successful keyring write scrubs the plaintext fields, andserver removedeletes the keyring entries. Connect's existing keyring key format is unchanged (frozen with a test). Machines without a usable keyring — package not installed, or no backend (NoKeyringError, e.g. CI) — keep the documentedservers.jsonbehavior.25a7e975— shared 401 → refresh → retry ("CouldConnectCloudClientshare the existing bearer-token refresh machinery?").BearerTokenHTTPServerinhttp_support.pyowns request → 401 → refresh → retry-once, including the seekable-body rewind both clients now share. Token minting stays per target: Connect keeps OAuth discovery, its registered client, andinvalid_clientre-registration; Connect Cloud keeps client-credentials-vs-refresh-token and the typed errors from the first commit.Redeployment target inference from application metadata (the
--set-defaultquestion) is deferred to its own PR: it changes target-resolution precedence for all server types, not just Cloud.Tests: full suite on Python 3.13, 1077 passed / 12 skipped; ruff clean.