From 95da59fda860cc54a4ad17ab91ad02cd14fa3644 Mon Sep 17 00:00:00 2001 From: Sam Perman Date: Fri, 14 Aug 2026 14:05:49 -0400 Subject: [PATCH 1/9] fix: make Connect Cloud token refresh failures actionable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 -A `." - invalid_client on the client-credentials path raises a message saying the service account credential was revoked or rotated, pointing at /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. --- rsconnect/api.py | 107 +++++++++++++++++++------ rsconnect/oauth.py | 27 +++++-- tests/test_connect_cloud.py | 150 +++++++++++++++++++++++++++++++++++- tests/test_oauth.py | 15 ++++ 4 files changed, 269 insertions(+), 30 deletions(-) diff --git a/rsconnect/api.py b/rsconnect/api.py index 6d4d7ba0..515da889 100644 --- a/rsconnect/api.py +++ b/rsconnect/api.py @@ -12,6 +12,7 @@ import json import os import re +import shlex import sys import tarfile import time @@ -2947,10 +2948,15 @@ def _attempt_token_refresh(self) -> bool: Uses the client credentials grant when a service account credential is stored, and the refresh token otherwise. Returns whether a new token was obtained. + + A credential the auth server rejects outright raises instead, since no + retry will fix it and the caller would otherwise report only the opaque + 401. Transient failures still return False so the original 401 surfaces. """ - from .metadata import ServerStore + from .oauth import InvalidClientError, InvalidGrantError server = self._server + service_account = bool(server.client_id and server.client_secret) try: if server.client_id and server.client_secret: tokens = connect_cloud.login_client_credentials( @@ -2960,12 +2966,32 @@ def _attempt_token_refresh(self) -> bool: tokens = connect_cloud.refresh(server.refresh_token, server.environment) else: return False + except InvalidClientError as exc: + if not service_account: + # This CLI's own OAuth client, not the user's credential. + logger.warning("Posit Connect Cloud token refresh failed: %s" % exc) + return False + raise RSConnectException( + "The Posit Connect Cloud service account credential was rejected — it has been revoked or " + "rotated. Create a new one at %s/identity/credentials, then save it with `%s`." + % (server.urls().auth, self._add_command(service_account=True)) + ) from exc + except InvalidGrantError as exc: + if service_account: + logger.warning("Posit Connect Cloud token refresh failed: %s" % exc) + return False + self._persist_tokens(None, None) + raise RSConnectException( + "Your Posit Connect Cloud session has expired and could not be renewed. " + "Authenticate again with `%s`." % self._add_command() + ) from exc except RSConnectException as exc: - logger.debug("Posit Connect Cloud token refresh failed: %s" % exc) + logger.warning("Posit Connect Cloud token refresh failed: %s" % exc) return False access_token = tokens.get("access_token") if not access_token: + logger.warning("Posit Connect Cloud returned no access token when refreshing the credential.") return False server.access_token = access_token @@ -2973,30 +2999,67 @@ def _attempt_token_refresh(self) -> bool: # so keep the existing one rather than clearing it. server.refresh_token = tokens.get("refresh_token") or server.refresh_token self._apply_authorization() + self._persist_tokens(server.access_token, server.refresh_token) + return True + + def _add_command(self, service_account: bool = False) -> str: + """The `rsconnect add` invocation that would re-save this credential.""" + from .metadata import ServerStore + + server = self._server + # Re-adding must keep the nickname pointed at the saved entry's account, + # not this run's, which may be publishing to a different one via -A. + account = server.account_name if server.server_name: - store = ServerStore() - entry = store.get_by_name(server.server_name) + entry = ServerStore().get_by_name(server.server_name) if entry: - # A refresh persists the new tokens and nothing else. Every other field - # is taken from the saved entry alone — never from this run, which may - # be publishing to a different account on the same login, or carrying - # service-account credentials from the environment that must not be - # grafted onto an interactively created entry. `rsconnect add` is what - # changes those. This is how the R client's withTokenRefreshRetry() - # writes back too. ServerStore.set writes the file itself. - store.set( - server.server_name, - server.url, - connect_cloud_account_name=entry.get("connect_cloud_account_name") or server.account_name, - connect_cloud_account_id=entry.get("connect_cloud_account_id"), - connect_cloud_client_id=entry.get("connect_cloud_client_id"), - connect_cloud_client_secret=entry.get("connect_cloud_client_secret"), - connect_cloud_access_token=server.access_token, - connect_cloud_refresh_token=server.refresh_token, - ) + account = entry.get("connect_cloud_account_name") or account + parts = ["rsconnect add --connect-cloud"] + if server.environment != connect_cloud.DEFAULT_ENVIRONMENT: + # Without the URL, add would authenticate against production. + parts.append("-s %s" % shlex.quote(server.url)) + parts.append("-n %s" % (shlex.quote(server.server_name) if server.server_name else "")) + parts.append("-A %s" % (shlex.quote(account) if account else "")) + if service_account: + parts.append("--client-id --client-secret ") + return " ".join(parts) + + def _persist_tokens(self, access_token: Optional[str], refresh_token: Optional[str]) -> None: + """Write the tokens back to the saved entry, or clear them when both are None. + + Does nothing for a run with no saved entry behind it: a credential + override or a one-shot deploy. + """ + from .metadata import ServerStore - return True + server = self._server + if not server.server_name: + return + + store = ServerStore() + entry = store.get_by_name(server.server_name) + if not entry: + return + + # A refresh persists the new tokens and nothing else. Every other field + # is taken from the saved entry alone — never from this run, which may + # be publishing to a different account on the same login, or carrying + # service-account credentials from the environment that must not be + # grafted onto an interactively created entry. `rsconnect add` is what + # changes those. This is how the R client's withTokenRefreshRetry() + # writes back too. ServerStore.set writes the file itself, and omits + # the token fields when they are None. + store.set( + server.server_name, + server.url, + connect_cloud_account_name=entry.get("connect_cloud_account_name") or server.account_name, + connect_cloud_account_id=entry.get("connect_cloud_account_id"), + connect_cloud_client_id=entry.get("connect_cloud_client_id"), + connect_cloud_client_secret=entry.get("connect_cloud_client_secret"), + connect_cloud_access_token=access_token, + connect_cloud_refresh_token=refresh_token, + ) def get_current_user(self) -> JsonData: response = self.get("/users/me") diff --git a/rsconnect/oauth.py b/rsconnect/oauth.py index bf03b8c7..260a7945 100644 --- a/rsconnect/oauth.py +++ b/rsconnect/oauth.py @@ -37,15 +37,31 @@ def __init__(self) -> None: super().__init__("OAuth client_id is invalid or has been deleted on the server.") +class InvalidGrantError(RSConnectException): + """Raised when the OAuth server returns an invalid_grant error. + + The grant presented — usually a refresh token — has expired, been revoked, + or was issued to another client. Callers decide what to do about it; the + server's ``error_description`` is kept in ``description`` when it sends one. + """ + + def __init__(self, description: Optional[str] = None) -> None: + self.description = description + detail = f": {description}" if description else "." + super().__init__(f"The OAuth grant is invalid, expired, or has been revoked{detail}") + + def _check_oauth_error_response(response: HTTPResponse) -> None: """Check an HTTPResponse for OAuth error codes and raise appropriately.""" if response.json_data and isinstance(response.json_data, dict): - error = response.json_data.get("error", "") + error = str(response.json_data.get("error") or "") + description = str(response.json_data.get("error_description") or "") if error == "invalid_client": raise InvalidClientError() - description = response.json_data.get("error_description", error) - if description: - raise RSConnectException(f"OAuth error: {description}") + if error == "invalid_grant": + raise InvalidGrantError(description or None) + if description or error: + raise RSConnectException(f"OAuth error: {description or error}") def _unwrap_json_response(response: Any) -> dict[str, Any]: @@ -442,7 +458,8 @@ def refresh_access_token( """Refresh an OAuth access token using a refresh token. Returns the new token response dict. Raises InvalidClientError if the - client_id has been deleted server-side. + client_id has been deleted server-side, or InvalidGrantError if the refresh + token has expired or been revoked. """ params = { "grant_type": "refresh_token", diff --git a/tests/test_connect_cloud.py b/tests/test_connect_cloud.py index b2f459ea..581818e7 100644 --- a/tests/test_connect_cloud.py +++ b/tests/test_connect_cloud.py @@ -25,6 +25,7 @@ from rsconnect.main import cli from rsconnect.metadata import AppStore, ServerData, ServerStore from rsconnect.models import AppModes +from rsconnect.oauth import InvalidClientError, InvalidGrantError from rsconnect.validation import validate_connection_options ENV = ParameterSource.ENVIRONMENT @@ -720,12 +721,30 @@ def _get_user_with_refresh(self, server, mock_target, token_response): client.get_current_user() return refresher - def _save_entry(self, **fields): - """Persist a "cloud" entry and point the client's write-back store at it.""" + def _get_user_with_refresh_failure(self, server, mock_target, error): + """Serve a 401 with the named rsconnect.connect_cloud function raising `error`. + + Returns the exception that reached the caller, so a test can tell an + actionable refresh failure from the original 401 passing through. + """ + httpretty.register_uri( + httpretty.GET, + f"{API}/users/me", + responses=[httpretty.Response(body="", status=401), _json_response({"id": "u1"})], + ) + client = ConnectCloudClient(server) + with mock.patch(f"rsconnect.connect_cloud.{mock_target}", side_effect=error): + with client: + with self.assertRaises(RSConnectException) as raised: + client.get_current_user() + return raised.exception + + def _save_entry(self, name="cloud", **fields): + """Persist a saved entry and point the client's write-back store at it.""" self._base_dir = tempfile.mkdtemp() store = ServerStore(base_dir=self._base_dir) fields.setdefault("connect_cloud_account_name", "acme") - store.set("cloud", API, **fields) + store.set(name, API, **fields) store.save() # The client imports ServerStore inside the function, so patch it at its source. patch = mock.patch("rsconnect.metadata.ServerStore", lambda: ServerStore(base_dir=self._base_dir)) @@ -851,6 +870,131 @@ def test_a_refresh_does_not_persist_client_credentials_from_the_environment(self self.assertEqual(entry["connect_cloud_client_secret"], "saved-secret") self.assertEqual(entry["connect_cloud_access_token"], "fresh") + @httpretty.activate(verbose=True, allow_net_connect=False) + def test_an_expired_refresh_token_clears_the_stored_tokens_and_says_how_to_reauthenticate(self): + self._save_entry( + connect_cloud_account_id="acct-1", + connect_cloud_access_token="stale", + connect_cloud_refresh_token="rt", + ) + server = ConnectCloudServer("acme", access_token="stale", refresh_token="rt", server_name="cloud") + exception = self._get_user_with_refresh_failure(server, "refresh", InvalidGrantError("token expired")) + + self.assertIn("session has expired", exception.message) + self.assertIn("rsconnect add --connect-cloud -n cloud -A acme", exception.message) + entry = self._stored_entry() + self.assertNotIn("connect_cloud_access_token", entry) + self.assertNotIn("connect_cloud_refresh_token", entry) + # Only the tokens go: the entry is still the credential to re-authenticate. + self.assertEqual(entry["name"], "cloud") + self.assertEqual(entry["connect_cloud_account_name"], "acme") + self.assertEqual(entry["connect_cloud_account_id"], "acct-1") + + @httpretty.activate(verbose=True, allow_net_connect=False) + def test_an_expired_refresh_token_is_reported_without_a_saved_entry(self): + server = ConnectCloudServer("acme", access_token="stale", refresh_token="rt") + with mock.patch("rsconnect.metadata.ServerStore") as store: + exception = self._get_user_with_refresh_failure(server, "refresh", InvalidGrantError()) + + store.assert_not_called() + self.assertIn("session has expired", exception.message) + self.assertIn("rsconnect add --connect-cloud -n -A acme", exception.message) + + @httpretty.activate(verbose=True, allow_net_connect=False) + def test_the_reauthentication_command_names_the_saved_entrys_account(self): + # The run may be publishing to another account on the same login; + # re-adding must not repoint the nickname at it. + self._save_entry(connect_cloud_account_name="alice", connect_cloud_refresh_token="rt") + server = ConnectCloudServer("team-x", access_token="stale", refresh_token="rt", server_name="cloud") + exception = self._get_user_with_refresh_failure(server, "refresh", InvalidGrantError()) + + self.assertIn("-A alice", exception.message) + self.assertNotIn("team-x", exception.message) + + @httpretty.activate(verbose=True, allow_net_connect=False) + def test_the_reauthentication_command_keeps_a_non_production_server(self): + # Without the URL, the suggested add would authenticate against production. + staging_api = "https://api.staging.connect.posit.cloud/v1" + httpretty.register_uri(httpretty.GET, f"{staging_api}/users/me", body="", status=401) + server = ConnectCloudServer("acme", access_token="stale", refresh_token="rt", url=staging_api) + client = ConnectCloudClient(server) + + with mock.patch("rsconnect.connect_cloud.refresh", side_effect=InvalidGrantError()): + with client: + with self.assertRaises(RSConnectException) as raised: + client.get_current_user() + + self.assertIn("-s %s" % staging_api, raised.exception.message) + + @httpretty.activate(verbose=True, allow_net_connect=False) + def test_the_reauthentication_command_quotes_values_that_need_it(self): + self._save_entry(name="my cloud", connect_cloud_account_name="acme", connect_cloud_refresh_token="rt") + server = ConnectCloudServer("acme", access_token="stale", refresh_token="rt", server_name="my cloud") + exception = self._get_user_with_refresh_failure(server, "refresh", InvalidGrantError()) + + self.assertIn("-n 'my cloud' -A acme", exception.message) + + @httpretty.activate(verbose=True, allow_net_connect=False) + def test_a_rejected_service_account_secret_is_reported_and_the_entry_is_left_alone(self): + self._save_entry( + connect_cloud_client_id="cid", + connect_cloud_client_secret="csecret", + connect_cloud_access_token="stale", + ) + server = ConnectCloudServer( + "acme", access_token="stale", client_id="cid", client_secret="csecret", server_name="cloud" + ) + exception = self._get_user_with_refresh_failure(server, "login_client_credentials", InvalidClientError()) + + self.assertIn("service account credential was rejected", exception.message) + self.assertIn("https://login.posit.cloud/identity/credentials", exception.message) + self.assertIn( + "rsconnect add --connect-cloud -n cloud -A acme --client-id --client-secret ", + exception.message, + ) + entry = self._stored_entry() + self.assertEqual(entry["connect_cloud_client_secret"], "csecret") + self.assertEqual(entry["connect_cloud_access_token"], "stale") + + @httpretty.activate(verbose=True, allow_net_connect=False) + def test_a_transient_refresh_failure_leaves_the_original_401_and_warns(self): + self._save_entry(connect_cloud_access_token="stale", connect_cloud_refresh_token="rt") + server = ConnectCloudServer("acme", access_token="stale", refresh_token="rt", server_name="cloud") + + with self.assertLogs("rsconnect", level="WARNING") as captured: + exception = self._get_user_with_refresh_failure( + server, "refresh", RSConnectException("Could not connect to https://login.posit.cloud") + ) + + self.assertIn("401", exception.message) + self.assertIn("token refresh failed", "\n".join(captured.output)) + self.assertEqual(self._stored_entry()["connect_cloud_refresh_token"], "rt") + self.assertEqual(len(httpretty.latest_requests()), 1) + + @httpretty.activate(verbose=True, allow_net_connect=False) + def test_a_rejected_cli_oauth_client_is_not_reported_as_an_expired_session(self): + # invalid_client on the refresh-token path is about this CLI's own OAuth + # client, not a credential the user can re-save. + self._save_entry(connect_cloud_access_token="stale", connect_cloud_refresh_token="rt") + server = ConnectCloudServer("acme", access_token="stale", refresh_token="rt", server_name="cloud") + + with self.assertLogs("rsconnect", level="WARNING"): + exception = self._get_user_with_refresh_failure(server, "refresh", InvalidClientError()) + + self.assertIn("401", exception.message) + self.assertEqual(self._stored_entry()["connect_cloud_refresh_token"], "rt") + + @httpretty.activate(verbose=True, allow_net_connect=False) + def test_invalid_grant_from_a_client_credentials_grant_leaves_the_original_401(self): + server = ConnectCloudServer("acme", access_token="stale", client_id="cid", client_secret="csecret") + + with self.assertLogs("rsconnect", level="WARNING"): + exception = self._get_user_with_refresh_failure( + server, "login_client_credentials", InvalidGrantError("no grant") + ) + + self.assertIn("401", exception.message) + class TestConnectCloudAdd(CliTestCase): """CLI-level tests for `rsconnect add -s connect.posit.cloud`.""" diff --git a/tests/test_oauth.py b/tests/test_oauth.py index 7bfc2e22..cda9a275 100644 --- a/tests/test_oauth.py +++ b/tests/test_oauth.py @@ -12,6 +12,7 @@ from rsconnect.metadata import ServerData from rsconnect.oauth import ( InvalidClientError, + InvalidGrantError, _exchange_code_for_token, _poll_for_device_token, discover_oauth_metadata, @@ -290,6 +291,20 @@ def test_invalid_client(self, mock_http_server: MagicMock): with pytest.raises(InvalidClientError): refresh_access_token(FAKE_METADATA, "bad-client", "old-rt") + def test_invalid_grant(self, mock_http_server: MagicMock): + mock_http_server.request.return_value = _make_response( + 400, {"error": "invalid_grant", "error_description": "refresh token expired"} + ) + with pytest.raises(InvalidGrantError, match="refresh token expired") as raised: + refresh_access_token(FAKE_METADATA, "client-1", "old-rt") + assert raised.value.description == "refresh token expired" + + def test_invalid_grant_without_a_description(self, mock_http_server: MagicMock): + mock_http_server.request.return_value = _make_response(400, {"error": "invalid_grant"}) + with pytest.raises(InvalidGrantError, match="revoked") as raised: + refresh_access_token(FAKE_METADATA, "client-1", "old-rt") + assert raised.value.description is None + class TestKeyringIntegration: def test_store_success(self): From a3ef2684cb31b04de6697d705d0a7d9455680b4d Mon Sep 17 00:00:00 2001 From: Sam Perman Date: Fri, 14 Aug 2026 14:36:09 -0400 Subject: [PATCH 2/9] feat: select Posit Connect Cloud credentials by nickname only 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. --- docs/CHANGELOG.md | 11 +-- rsconnect/api.py | 24 ++++--- rsconnect/main.py | 7 +- rsconnect/metadata.py | 63 +++++++---------- rsconnect/validation.py | 39 ++++++----- tests/test_connect_cloud.py | 134 +++++++++++++++++++++++++++--------- 6 files changed, 175 insertions(+), 103 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 4d9bf052..6c0b4604 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -19,17 +19,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `--connect-cloud` (or `-s connect.posit.cloud`) with `-A ` (or the `CONNECT_CLOUD_ACCOUNT` environment variable; a `SHINYAPPS_ACCOUNT` variable exported for shinyapps.io is ignored here), or `-n ` for a saved - account. Supported content types: Shiny (Python - and R), Streamlit, Dash, Bokeh, Jupyter notebooks, Quarto, R Markdown, and - static content. + credential — which publishes to the account it was saved with, or to another + account of the same login when `-A` is given as well. Supported content types: + Shiny (Python and R), Streamlit, Dash, Bokeh, Jupyter notebooks, Quarto, + R Markdown, and static content. - `rsconnect add` now reports invalid option combinations and unreadable certificate files as plain error messages. Previously these surfaced as raw Python tracebacks. - Deploying with a nickname (`-n`) no longer fails when `SHINYAPPS_ACCOUNT`, `SHINYAPPS_TOKEN`, or `SHINYAPPS_SECRET` happen to be set in the environment. Those values are ignored for nickname deploys — the saved credential is what - the nickname means. Shinyapps options typed on the command line still - conflict with `-n`. + the nickname means. A typed `-T/--token` or `-S/--secret` still conflicts with + `-n`. - Credentials are now redacted from verbose (`-v`) log output and from error messages that quote a request URL. This covers authorization and signing headers, OAuth tokens and client secrets in request and response bodies, the diff --git a/rsconnect/api.py b/rsconnect/api.py index 515da889..32792faf 100644 --- a/rsconnect/api.py +++ b/rsconnect/api.py @@ -1577,12 +1577,12 @@ def setup_remote_server( server_data = ServerData(None, None, False) else: try: - server_data = store.resolve(name, url, account_name) + server_data = store.resolve(name, url) except RSConnectException: - # Several saved Connect Cloud entries can make the lookup ambiguous, + # Several saved Connect Cloud credentials make the lookup ambiguous, # but a complete supplied credential pair with an account is its own # identity (the one-shot/CI shape) and needs nothing from the store. - # A matching single entry still resolves above and keeps its + # A single saved credential still resolves above and keeps its # token-reuse and write-back behavior. if supplied_client_id and supplied_client_secret and account_name and not name: server_data = ServerData(None, None, False) @@ -1653,11 +1653,19 @@ def setup_remote_server( # Deferred from validate_connection_options: a nickname or default # server is only known not to be Connect Cloud here, after resolution. validation.validate_connect_cloud_credential_options(ctx, supplied_client_id, supplied_client_secret) - # Also deferred: a lone -A was allowed through when a default server - # might have resolved to Connect Cloud. It did not, so the shinyapps - # all-or-nothing rule applies after all -- judged on the pre-merge - # values, or a default shinyapps entry's token and secret would - # satisfy it and deploy the typed account with borrowed credentials. + # Also deferred: a lone -A was allowed through when a nickname or + # default server might have resolved to Connect Cloud. It did not, so + # -A means the shinyapps account again -- which a nickname already + # names, and which the all-or-nothing rule below governs otherwise. + # Both are judged on the pre-merge values, or a default shinyapps + # entry's token and secret would satisfy the rule and deploy the typed + # account with borrowed credentials. + if name and supplied_account_name: + raise RSConnectException( + "-n/--name cannot be specified in conjunction with -A/--account, unless the nickname \ +names a Posit Connect Cloud credential, where -A selects the account to publish to. \ +See command help for further details." + ) if supplied_account_name and not (supplied_token and supplied_secret): raise RSConnectException( "-A/--account, -T/--token, and -S/--secret must all be provided \ diff --git a/rsconnect/main.py b/rsconnect/main.py index c9faa975..e371ef8a 100644 --- a/rsconnect/main.py +++ b/rsconnect/main.py @@ -287,7 +287,9 @@ def cloud_shinyapps_args(func: Callable[P, T]) -> Callable[P, T]: envvar=["SHINYAPPS_ACCOUNT"], help="The shinyapps.io or Posit Connect Cloud account name. (Also settable via the \ SHINYAPPS_ACCOUNT environment variable for shinyapps.io, or CONNECT_CLOUD_ACCOUNT for \ -Posit Connect Cloud; each applies only to its own target.)", +Posit Connect Cloud; each applies only to its own target.) For Posit Connect Cloud this \ +is the account to publish to, and may accompany -n/--name to publish to an account other \ +than the one the credential was saved with.", ) @click.option( "--token", @@ -358,7 +360,8 @@ def connect_cloud_account_arg(func: Callable[P, T]) -> Callable[P, T]: "--account", "-A", help="The Posit Connect Cloud account to deploy to. (Also settable via the \ -CONNECT_CLOUD_ACCOUNT environment variable.)", +CONNECT_CLOUD_ACCOUNT environment variable.) May accompany -n/--name to publish to an \ +account other than the one the credential was saved with.", ) @functools.wraps(func) def wrapper(*args: P.args, **kwargs: P.kwargs): diff --git a/rsconnect/metadata.py b/rsconnect/metadata.py index c54e1869..4bcc185a 100644 --- a/rsconnect/metadata.py +++ b/rsconnect/metadata.py @@ -370,28 +370,26 @@ def get_by_name(self, name: str): """ return self._get_by_key(name) - def get_by_url(self, url: str, account_name: Optional[str] = None): + def get_by_url(self, url: str): """ Get the server information for the given URL.. :param url: the Connect URL of the server to get information for. A short name such as "connect.posit.cloud" is translated to the API URL entries are stored under. - :param account_name: the Posit Connect Cloud account to select, for the case - where one URL covers several saved servers. - :raises RSConnectException: if several Posit Connect Cloud servers share the - URL and the account does not single one out. + :raises RSConnectException: if several Posit Connect Cloud credentials share + the URL, since only a nickname can tell them apart. """ target = resolve_server_alias(url) if connect_cloud.is_connect_cloud_url(target): - return self._get_connect_cloud_server(target, account_name) + return self._get_connect_cloud_server(target) return self._get_by_value_attr("url", target) def has_connect_cloud_account(self, url: Optional[str]) -> bool: """Whether any Posit Connect Cloud credential is saved for this URL. Callers use this to decide whether the account has to be supplied on the - command line or can come from a saved server. + command line or can come from a saved credential's default target. """ if not url: return False @@ -408,45 +406,32 @@ def _connect_cloud_servers(self, url: str) -> list[ServerDataDict]: key=lambda entry: entry.get("name") or "", ) - def _get_connect_cloud_server(self, url: str, account_name: Optional[str]): - """Pick one of the Posit Connect Cloud servers saved for an API URL. + def _get_connect_cloud_server(self, url: str): + """Pick the Posit Connect Cloud credential saved for an API URL. Every Connect Cloud entry records the same API URL, so unlike Posit Connect - the URL does not identify one server; the account does. With a single saved - login an explicit account still selects what to publish to rather than which - credential to use, since that login can publish to every account it has - access to. Anything more ambiguous is reported rather than guessed at. + the URL does not identify one entry. An 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 selects the publish target and only the nickname selects + the credential. With several saved the choice is reported rather than guessed + at, because they may hold different credentials (an interactive login and a + service account, or two identities) which refresh differently. """ candidates = self._connect_cloud_servers(url) - if account_name: - matches = [e for e in candidates if e.get("connect_cloud_account_name") == account_name] - if len(matches) == 1: - return matches[0] - if len(matches) > 1: - # The entries may hold different credentials (interactive vs - # service account); picking one silently would decide which gets - # used and refreshed. - nicknames = ", ".join('"%s"' % e.get("name") for e in matches) - raise RSConnectException( - 'Several saved Posit Connect Cloud credentials are for account "%s": %s. ' - "Use -n/--name to pick one." % (account_name, nicknames) - ) if not candidates: return None if len(candidates) == 1: return candidates[0] + # The account each credential publishes to by default, so the nicknames can + # be recognized. saved = ", ".join( - '%s (nickname "%s")' % (entry.get("connect_cloud_account_name"), entry.get("name")) for entry in candidates + '"%s" (account %s)' % (entry.get("name"), entry.get("connect_cloud_account_name")) for entry in candidates ) - if account_name: - raise RSConnectException( - 'No saved Posit Connect Cloud credential is for account "%s", and there are several to choose ' - "from: %s. Use -n/--name to choose one, or run `rsconnect add` for that account." - % (account_name, saved) - ) raise RSConnectException( - "Several Posit Connect Cloud accounts are saved: %s. Use -A/--account or -n/--name to pick one." % saved + "Several Posit Connect Cloud credentials are saved: %s. Use -n/--name to choose one, adding " + "-A/--account to publish to a different account, or pass --client-id and --client-secret to " + "use a service account credential directly." % saved ) def get_all_servers(self): @@ -590,8 +575,8 @@ def remove_by_url(self, url: str): arbitrary one. :param url: the Connect URL of the server to remove. - :raises RSConnectException: if several Posit Connect Cloud entries share - the URL. + :raises RSConnectException: if several Posit Connect Cloud credentials + share the URL. """ entry = self.get_by_url(url) if entry is None: @@ -620,7 +605,7 @@ def update_oauth_tokens( updated.pop("oauth_token_expiry", None) # type: ignore[misc] self._set(name, updated) - def resolve(self, name: Optional[str], url: Optional[str], account_name: Optional[str] = None) -> ServerData: + def resolve(self, name: Optional[str], url: Optional[str]) -> ServerData: """ This function will resolve the given inputs into a set of server information. It assumes that either `name` or `url` is provided. @@ -638,8 +623,6 @@ def resolve(self, name: Optional[str], url: Optional[str], account_name: Optiona :param name: the nickname to look for. :param url: the Connect server URL to look for. - :param account_name: the Posit Connect Cloud account to look for, which is - what identifies one of several servers sharing the Connect Cloud API URL. :return: the information needed to interact with the resolved server and whether it came from the store or the arguments. """ @@ -648,7 +631,7 @@ def resolve(self, name: Optional[str], url: Optional[str], account_name: Optiona if not entry: raise RSConnectException('The nickname, "%s", does not exist.' % name) elif url: - entry = self.get_by_url(url, account_name) + entry = self.get_by_url(url) else: entry = self.get_default() if entry is None and self.count() == 1: diff --git a/rsconnect/validation.py b/rsconnect/validation.py index bab2aabf..43791b9d 100644 --- a/rsconnect/validation.py +++ b/rsconnect/validation.py @@ -184,15 +184,17 @@ def validate_connection_options( # `rsconnect add` is unaffected: there -n names the entry being created, and # add does not pass it to this function. # - # Typed shinyapps options contradict a nickname the same way, but + # A typed -T/--token or -S/--secret contradicts a nickname the same way, but # environment-sourced ones (SHINYAPPS_ACCOUNT/TOKEN/SECRET exported for CI # elsewhere) are just the environment and must not block a nickname deploy; # the executor drops them before resolution so they cannot merge into the - # entry either. + # entry either. -A/--account is not judged here at all: a nickname may name a + # Posit Connect Cloud credential, where -A selects the account to publish to. + # The executor raises the conflict once the nickname is known not to be one. options_mutually_exclusive_with_name = {"-s/--server": url, "--connect-cloud": connect_cloud} present_options_mutually_exclusive_with_name = _get_present_options( options_mutually_exclusive_with_name, ctx - ) + _get_present_options(shinyapps_options, ctx, ignore_sources=("ENVIRONMENT",)) + ) + _get_present_options({"-T/--token": token, "-S/--secret": secret}, ctx, ignore_sources=("ENVIRONMENT",)) if name and present_options_mutually_exclusive_with_name: name_source = get_parameter_source_name_from_ctx("name", ctx) @@ -278,25 +280,28 @@ def validate_connection_options( if not name and not (has_default_server and not url): validate_connect_cloud_credential_options(ctx, client_id, client_secret) - # A lone -A alongside a default server cannot be judged yet: the default may - # resolve to Connect Cloud, where -A selects the account to publish to. The - # conflict and all-or-nothing rules below are deferred for this case; the - # executor re-raises the all-or-nothing error after resolution when the - # target turns out not to be Connect Cloud. A token or secret is unambiguous - # shinyapps intent, so those still fail fast here. - lone_account_with_default = bool( - account_name and not token and not secret and has_default_server and not name and not url + # A lone -A alongside a nickname or a default server cannot be judged yet: + # either may resolve to Connect Cloud, where -A selects the account to publish + # to. The conflict and all-or-nothing rules below are deferred for this case; + # the executor raises after resolution when the target turns out not to be + # Connect Cloud. A token or secret is unambiguous shinyapps intent, so those + # still fail fast here. + lone_account_with_saved_server = bool( + account_name and not token and not secret and (has_default_server or name) and not url ) # In the deferred case only *typed* Connect options conflict: an exported # CONNECT_API_KEY or CONNECT_CA_CERTIFICATE is just the environment, and the # default may not even be a Connect server. Cloud targets re-check typed # options after resolution (validate_connect_cloud_incompatible_options). - connect_conflicts = ( - _get_present_options(connect_options, ctx, ignore_sources=("ENVIRONMENT",)) - if lone_account_with_default - else present_connect_options - ) + # With a nickname, not even a typed one is judged here: its -A may be a + # Connect Cloud publish target, and this rule would report the mistake as a + # shinyapps.io conflict. That case fails after resolution too, from the Cloud + # check above or the executor's -n/-A conflict. + if lone_account_with_saved_server: + connect_conflicts = [] if name else _get_present_options(connect_options, ctx, ignore_sources=("ENVIRONMENT",)) + else: + connect_conflicts = present_connect_options if connect_conflicts and present_shinyapps_options: raise RSConnectException( f"Connect options ({', '.join(connect_conflicts)}) may not be passed \ @@ -318,7 +323,7 @@ def validate_connection_options( ) if present_shinyapps_options: - if len(present_shinyapps_options) != len(shinyapps_options) and not lone_account_with_default: + if len(present_shinyapps_options) != len(shinyapps_options) and not lone_account_with_saved_server: raise RSConnectException( "-A/--account, -T/--token, and -S/--secret must all be provided \ for shinyapps.io. See command help for further details." diff --git a/tests/test_connect_cloud.py b/tests/test_connect_cloud.py index 581818e7..2ca0d99d 100644 --- a/tests/test_connect_cloud.py +++ b/tests/test_connect_cloud.py @@ -1404,12 +1404,69 @@ def test_env_shinyapps_credentials_do_not_block_or_merge_into_a_nickname_deploy( ) self.assertEqual(server.account_name, "acme") - def test_a_typed_shinyapps_option_still_conflicts_with_a_nickname(self): + def test_a_typed_shinyapps_token_still_conflicts_with_a_nickname(self): with self.assertRaises(RSConnectException) as context: - _setup_remote_server(ctx=_ctx(account=TYPED), name="cloud", account_name="typed-acct") + _setup_remote_server(ctx=_ctx(token=TYPED), name="cloud", token="typed-token") self.assertIn("cannot be specified in conjunction", str(context.exception)) +class TestNicknameWithATypedAccount(unittest.TestCase): + """-A alongside -n is only meaningful for Connect Cloud, where the nickname + names the credential and -A the account to publish to. For every other target + the nickname already names the account, so the combination is a conflict -- + judged after resolution, since only the store knows which target it is.""" + + def test_a_typed_account_selects_the_target_account_of_a_cloud_nickname(self): + server = _cloud_server( + ctx=_ctx(account=TYPED), + resolve=_cloud_entry(connect_cloud_account_id="acct-acme", connect_cloud_access_token="at"), + name="cloud", + account_name="typed-acct", + ) + self.assertEqual(server.account_name, "typed-acct") + self.assertEqual(server.access_token, "at") + # The saved id belongs to the saved account, so publishing elsewhere resolves + # the name against the server instead. + self.assertIsNone(server.account_id) + + def test_a_typed_account_still_conflicts_with_a_connect_nickname(self): + store = ServerStore(base_dir=tempfile.mkdtemp()) + store.set("prod", "https://connect.example.com", api_key="key") + with self.assertRaises(RSConnectException) as context: + _setup_remote_server(ctx=_ctx(account=TYPED), store=store, name="prod", account_name="typed-acct") + self.assertIn("cannot be specified in conjunction", str(context.exception)) + + def test_a_typed_account_cannot_borrow_a_shinyapps_nicknames_credentials(self): + # The nickname's token and secret belong to its own account, so a typed -A + # must not deploy somewhere else with them. + with self.assertRaises(RSConnectException) as context: + _setup_remote_server( + ctx=_ctx(account=TYPED), + resolve=ServerData( + "shiny", + "https://api.shinyapps.io", + True, + account_name="saved-acct", + token="saved-token", + secret="saved-secret", + ), + name="shiny", + account_name="other-acct", + ) + self.assertIn("cannot be specified in conjunction", str(context.exception)) + + def test_connect_options_with_a_cloud_nickname_are_reported_against_connect_cloud(self): + with self.assertRaises(RSConnectException) as context: + _setup_remote_server( + ctx=_ctx(account=TYPED, insecure=TYPED), + resolve=_cloud_entry(connect_cloud_access_token="at"), + name="cloud", + account_name="typed-acct", + insecure=True, + ) + self.assertIn("alongside Posit Connect Cloud", str(context.exception)) + + class TestDefaultServerAccountDeferral(unittest.TestCase): """With a default server, -A cannot be judged until the default resolves: it selects the Connect Cloud account when the default is Cloud, and is the @@ -2601,8 +2658,8 @@ def test_flag_selects_connect_cloud_in_the_executor(self): class TestConnectCloudSameAccountAmbiguity(unittest.TestCase): - """Several entries for the same account may hold different credentials, so - -A must not silently pick one of them.""" + """Several credentials may share a default account (an interactive login and a + service account, say), so the account cannot pick one of them.""" def _store(self): store = ServerStore(base_dir=tempfile.mkdtemp()) @@ -2610,10 +2667,10 @@ def _store(self): store.set(name, API, connect_cloud_account_name="acme", connect_cloud_access_token="at-" + name) return store - def test_lookup_by_account_with_several_matches_is_rejected(self): + def test_lookup_by_url_with_several_credentials_is_rejected(self): store = self._store() with self.assertRaises(RSConnectException) as context: - store.get_by_url("connect.posit.cloud", "acme") + store.get_by_url("connect.posit.cloud") message = str(context.exception) self.assertIn('"cloud-a"', message) self.assertIn('"cloud-b"', message) @@ -2664,7 +2721,7 @@ def test_lookup_by_variant_urls_finds_the_saved_entry(self): "HTTPS://API.CONNECT.POSIT.CLOUD/v1", "connect.posit.cloud/", ): - entry = store.get_by_url(variant, "acme") + entry = store.get_by_url(variant) assert entry is not None, variant self.assertEqual(entry["name"], "cloud", variant) @@ -2809,9 +2866,9 @@ def test_a_saved_staging_server_is_used_when_staging_is_selected(self): class TestConnectCloudAccountSelection(unittest.TestCase): - """Every Connect Cloud server is saved under the same API URL, so the account is - what picks one out. Mirrors how the R client's findAccount() keys on - (account, server) and refuses to guess between several.""" + """A saved entry is a credential, not an account binding: -A/--account says where + to publish and only -n/--name picks the credential. A single saved credential is + used whatever the account; several are ambiguous until a nickname names one.""" def setUp(self): env_patch = mock.patch.dict(os.environ, {}, clear=True) @@ -2833,6 +2890,14 @@ def _two(self): def _server(self, store, **kwargs): return _cloud_server(store=store, **kwargs) + def test_nothing_saved_resolves_to_no_credential(self): + self.assertIsNone(self._store().get_by_url(connect_cloud.SERVER_NAME)) + + def test_one_saved_credential_is_found_by_url(self): + entry = self._one().get_by_url(connect_cloud.SERVER_NAME) + assert entry is not None + self.assertEqual(entry["name"], "cloud") + def test_the_account_is_not_required_when_one_server_is_saved(self): server = self._server(self._one(), use_connect_cloud=True) self.assertEqual(server.account_name, "sam") @@ -2852,35 +2917,42 @@ def test_add_still_requires_the_account_when_a_server_is_saved(self): self.assertEqual(result.exit_code, 1, result.output) self.assertIn("-A/--account is required", result.output) - def test_the_account_selects_among_several_saved_servers(self): - server = self._server(self._two(), use_connect_cloud=True, account_name="acme-team") - self.assertEqual(server.access_token, "ci-token") - self.assertEqual(server.server_name, "ci") - - server = self._server(self._two(), use_connect_cloud=True, account_name="sam") - self.assertEqual(server.access_token, "sam-token") - self.assertEqual(server.server_name, "personal") + def test_the_account_does_not_select_among_several_credentials(self): + # The account a credential was saved with is its default publish target, not + # its scope, so naming an account cannot say which credential to use. + for account in ("acme-team", "sam", "stranger"): + with self.assertRaises(RSConnectException) as context: + self._server(self._two(), use_connect_cloud=True, account_name=account) + self.assertIn("-n/--name", str(context.exception)) - def test_several_saved_servers_and_no_account_is_an_error(self): + def test_several_saved_credentials_are_listed_with_their_accounts(self): with self.assertRaises(RSConnectException) as context: self._server(self._two(), use_connect_cloud=True) message = str(context.exception) - self.assertIn("Several Posit Connect Cloud accounts are saved", message) - self.assertIn('acme-team (nickname "ci")', message) - self.assertIn('sam (nickname "personal")', message) - - def test_several_saved_servers_and_an_unknown_account_is_an_error(self): - with self.assertRaises(RSConnectException) as context: - self._server(self._two(), use_connect_cloud=True, account_name="stranger") - message = str(context.exception) - self.assertIn('No saved Posit Connect Cloud credential is for account "stranger"', message) - self.assertIn('acme-team (nickname "ci")', message) + self.assertIn("Several Posit Connect Cloud credentials are saved", message) + self.assertIn('"ci" (account acme-team)', message) + self.assertIn('"personal" (account sam)', message) def test_a_nickname_selects_a_server_without_the_account(self): server = self._server(self._two(), name="ci") self.assertEqual(server.account_name, "acme-team") self.assertEqual(server.access_token, "ci-token") + def test_a_nickname_publishes_to_another_account_of_the_same_credential(self): + store = self._two() + store.set( + "personal", + API, + connect_cloud_account_name="sam", + connect_cloud_account_id="acct-sam", + connect_cloud_access_token="sam-token", + ) + server = self._server(store, name="personal", environ={"CONNECT_CLOUD_ACCOUNT": "acme-team"}) + + self.assertEqual(server.access_token, "sam-token") + self.assertEqual(server.account_name, "acme-team") + self.assertIsNone(server.account_id) + def test_one_saved_login_publishes_to_another_of_its_accounts(self): # A Connect Cloud token belongs to a user, who can publish to every account # they have access to, so an explicit account picks the target rather than @@ -2896,14 +2968,14 @@ def test_remove_reports_the_ambiguity_instead_of_deleting_one(self): result = runner.invoke(cli, ["remove", "-s", connect_cloud.SERVER_NAME]) self.assertEqual(result.exit_code, 1, result.output) # `remove` reports through cli_feedback, so the message lands on stdout. - self.assertIn("Several Posit Connect Cloud accounts are saved", result.output) + self.assertIn("Several Posit Connect Cloud credentials are saved", result.output) self.assertIsNotNone(store.get_by_name("personal")) self.assertIsNotNone(store.get_by_name("ci")) def test_a_connect_server_url_is_unaffected(self): store = ServerStore(base_dir=tempfile.mkdtemp()) store.set("prod", "https://connect.example.com", api_key="key") - entry = store.get_by_url("https://connect.example.com", account_name="ignored") + entry = store.get_by_url("https://connect.example.com") assert entry is not None self.assertEqual(entry["name"], "prod") From 159a22ce881daec3ce1079588dfd7f64dc12a388 Mon Sep 17 00:00:00 2001 From: Sam Perman Date: Fri, 14 Aug 2026 15:42:05 -0400 Subject: [PATCH 3/9] feat: keep Posit Connect Cloud credentials in the system keyring Connect Cloud tokens and service account client secrets now go to the system keyring, keyed "#" 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 ":access_token" and ":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. --- docs/deploying.md | 6 + rsconnect/api.py | 35 ++++- rsconnect/connect_cloud.py | 85 ++++++++++- rsconnect/main.py | 33 ++++- rsconnect/metadata.py | 44 +++++- rsconnect/oauth.py | 136 ++++++++++++++---- tests/conftest.py | 15 ++ tests/test_connect_cloud.py | 279 ++++++++++++++++++++++++++++++++++++ tests/test_oauth.py | 95 ++++++++++++ 9 files changed, 682 insertions(+), 46 deletions(-) create mode 100644 tests/conftest.py diff --git a/docs/deploying.md b/docs/deploying.md index 6b05a26a..079a0917 100644 --- a/docs/deploying.md +++ b/docs/deploying.md @@ -553,6 +553,12 @@ Stored information files are stored in a platform-specific directory: Remembered server information is stored in the `servers.json` file in that directory. +OAuth credentials — the tokens from `rsconnect login` for Posit Connect, and the +tokens and service account client secret for Posit Connect Cloud — are stored in the +system keyring when one is available. On a machine without a usable keyring, such as a +CI runner, they are kept in `servers.json` instead, which is written with owner-only +permissions. + ### Deployment Data After a deployment is completed, information about the deployment is saved diff --git a/rsconnect/api.py b/rsconnect/api.py index 32792faf..0201064b 100644 --- a/rsconnect/api.py +++ b/rsconnect/api.py @@ -3034,7 +3034,11 @@ def _add_command(self, service_account: bool = False) -> str: return " ".join(parts) def _persist_tokens(self, access_token: Optional[str], refresh_token: Optional[str]) -> None: - """Write the tokens back to the saved entry, or clear them when both are None. + """Write the tokens back to the saved credential, or clear them when both are None. + + Prefers the system keyring, falling back to the tokens' fields in + servers.json. A keyring write also scrubs those fields, which is how an + entry saved before keyring support moves its tokens out of the file. Does nothing for a run with no saved entry behind it: a credential override or a one-shot deploy. @@ -3050,6 +3054,27 @@ def _persist_tokens(self, access_token: Optional[str], refresh_token: Optional[s if not entry: return + entry_url = entry["url"] + # A client secret still in the file predates keyring support, so it moves + # with the tokens -- unless the keyring already holds one, which is the + # secret in use and must not be overwritten by the file's older copy. + file_client_secret = entry.get("connect_cloud_client_secret") + keyring_readable, keyring_client_secret = connect_cloud.client_secret_from_keyring( + entry_url, server.server_name + ) + migrating_secret = bool(file_client_secret) and keyring_readable and not keyring_client_secret + if migrating_secret: + in_keyring = connect_cloud.store_credentials_in_keyring( + entry_url, server.server_name, access_token, refresh_token, file_client_secret + ) + else: + in_keyring = connect_cloud.store_tokens_in_keyring( + entry_url, server.server_name, access_token, refresh_token + ) + # The file's copy only goes once the keyring is known to hold a secret; a + # read that failed says nothing about what is in there. + secret_in_keyring = in_keyring and (migrating_secret or bool(keyring_client_secret)) + # A refresh persists the new tokens and nothing else. Every other field # is taken from the saved entry alone — never from this run, which may # be publishing to a different account on the same login, or carrying @@ -3060,13 +3085,13 @@ def _persist_tokens(self, access_token: Optional[str], refresh_token: Optional[s # the token fields when they are None. store.set( server.server_name, - server.url, + entry_url, connect_cloud_account_name=entry.get("connect_cloud_account_name") or server.account_name, connect_cloud_account_id=entry.get("connect_cloud_account_id"), connect_cloud_client_id=entry.get("connect_cloud_client_id"), - connect_cloud_client_secret=entry.get("connect_cloud_client_secret"), - connect_cloud_access_token=access_token, - connect_cloud_refresh_token=refresh_token, + connect_cloud_client_secret=None if secret_in_keyring else file_client_secret, + connect_cloud_access_token=None if in_keyring else access_token, + connect_cloud_refresh_token=None if in_keyring else refresh_token, ) def get_current_user(self) -> JsonData: diff --git a/rsconnect/connect_cloud.py b/rsconnect/connect_cloud.py index 100e14a5..f32ed146 100644 --- a/rsconnect/connect_cloud.py +++ b/rsconnect/connect_cloud.py @@ -13,7 +13,18 @@ from urllib.parse import urlparse from .exception import RSConnectException -from .oauth import login_with_device_code, refresh_access_token, request_client_credentials_token +from .oauth import ( + ACCESS_TOKEN_FIELD, + CLIENT_SECRET_FIELD, + REFRESH_TOKEN_FIELD, + keyring_delete_values, + keyring_get_value, + keyring_read_value, + keyring_store_values, + login_with_device_code, + refresh_access_token, + request_client_credentials_token, +) # The OAuth scope Connect Cloud issues tokens for. "vivid" is the internal name # of the Connect Cloud API. @@ -230,3 +241,75 @@ def refresh(refresh_token: str, environment: Optional[str] = None) -> dict[str, refresh_token=refresh_token, scope=SCOPE, ) + + +_CREDENTIAL_FIELDS = (ACCESS_TOKEN_FIELD, REFRESH_TOKEN_FIELD, CLIENT_SECRET_FIELD) + + +def keyring_key(url: str, nickname: str) -> str: + """The system keyring key for a saved Connect Cloud credential. + + Every Connect Cloud entry records the same API URL, so the nickname is what + makes the key unique. Posit Connect keys its entries by URL alone and must + keep doing so, or existing logins stop finding their tokens. + """ + return "%s#%s" % (url, nickname) + + +def store_credentials_in_keyring( + url: str, + nickname: str, + access_token: Optional[str], + refresh_token: Optional[str], + client_secret: Optional[str], +) -> bool: + """Store a saved credential's secrets in the system keyring, replacing all of them. + + Returns False when no keyring is available, which means the caller has to keep + the secrets in servers.json instead. + """ + return keyring_store_values( + keyring_key(url, nickname), + { + ACCESS_TOKEN_FIELD: access_token, + REFRESH_TOKEN_FIELD: refresh_token, + CLIENT_SECRET_FIELD: client_secret, + }, + ) + + +def store_tokens_in_keyring(url: str, nickname: str, access_token: Optional[str], refresh_token: Optional[str]) -> bool: + """Store (or, for empty values, delete) a saved credential's tokens. + + Leaves any stored client secret alone: a token refresh rotates the tokens + only, and `rsconnect add` is what changes the credential itself. + """ + return keyring_store_values( + keyring_key(url, nickname), + {ACCESS_TOKEN_FIELD: access_token, REFRESH_TOKEN_FIELD: refresh_token}, + ) + + +def client_secret_from_keyring(url: str, nickname: str) -> tuple[bool, Optional[str]]: + """The stored service account client secret, and whether the keyring could be read. + + A read failure is not the same as no secret: overwriting one that may be there + with an older copy from servers.json would break the credential. + """ + return keyring_read_value(keyring_key(url, nickname), CLIENT_SECRET_FIELD) + + +def credentials_from_keyring(url: str, nickname: str) -> tuple[Optional[str], Optional[str], Optional[str]]: + """A saved credential's secrets from the system keyring, as + (access token, refresh token, client secret). Each is None when absent.""" + key = keyring_key(url, nickname) + return ( + keyring_get_value(key, ACCESS_TOKEN_FIELD), + keyring_get_value(key, REFRESH_TOKEN_FIELD), + keyring_get_value(key, CLIENT_SECRET_FIELD), + ) + + +def delete_credentials_from_keyring(url: str, nickname: str) -> None: + """Delete a saved credential's secrets from the system keyring.""" + keyring_delete_values(keyring_key(url, nickname), _CREDENTIAL_FIELDS) diff --git a/rsconnect/main.py b/rsconnect/main.py index e371ef8a..9a57d221 100644 --- a/rsconnect/main.py +++ b/rsconnect/main.py @@ -936,19 +936,34 @@ def add( account = cast(str, account) cloud_server = _connect_cloud_login(account, client_id, client_secret, url=server) + # The keyring holds the secrets when there is one, and the entry keeps only + # the account and client id. servers.json (written 0600) carries them + # otherwise, for machines with no usable keyring such as CI runners. + in_keyring = connect_cloud_auth.store_credentials_in_keyring( + cloud_server.url, + name, + cloud_server.access_token, + cloud_server.refresh_token, + cloud_server.client_secret, + ) server_store.set( name, cloud_server.url, connect_cloud_account_name=cloud_server.account_name, connect_cloud_account_id=cloud_server.account_id, connect_cloud_client_id=cloud_server.client_id, - connect_cloud_client_secret=cloud_server.client_secret, - connect_cloud_access_token=cloud_server.access_token, - connect_cloud_refresh_token=cloud_server.refresh_token, + connect_cloud_client_secret=None if in_keyring else cloud_server.client_secret, + connect_cloud_access_token=None if in_keyring else cloud_server.access_token, + connect_cloud_refresh_token=None if in_keyring else cloud_server.refresh_token, set_as_default=set_default, ) verb = "Updated" if old_server else "Added" click.echo('{} {} credential "{}" for account "{}".'.format(verb, cloud_server.remote_name, name, account)) + if not in_keyring: + click.secho( + "Note: keyring not available; credentials stored in local file (chmod 600).", + fg="yellow", + ) elif token: server = cast(str, server) account = cast(str, account) @@ -1040,7 +1055,12 @@ def list_servers(verbose: int): click.echo(" Posit Connect Cloud account: %s" % server["connect_cloud_account_name"]) if server.get("connect_cloud_client_id"): click.echo(" Service account client ID: %s" % server["connect_cloud_client_id"]) - if server.get("connect_cloud_access_token"): + access, _, client_secret = connect_cloud_auth.credentials_from_keyring( + server["url"], server["name"] + ) + if access or client_secret: + click.echo(" Credentials stored in system keyring") + elif server.get("connect_cloud_access_token") or server.get("connect_cloud_client_secret"): click.echo(" Credentials are saved") if server.get("api_key"): click.echo(" API key is saved") @@ -1163,6 +1183,11 @@ def remove( else: raise RSConnectException("You must specify one of -n/--name or -s/--server.") + # Removing the entry leaves any keyring secrets orphaned, since nothing + # else records the nickname they are keyed by. + if entry and entry.get("connect_cloud_account_name"): + connect_cloud_auth.delete_credentials_from_keyring(entry["url"], entry["name"]) + if message: click.echo(message) if removed_was_default: diff --git a/rsconnect/metadata.py b/rsconnect/metadata.py index 4bcc185a..168ce28b 100644 --- a/rsconnect/metadata.py +++ b/rsconnect/metadata.py @@ -434,6 +434,29 @@ def _get_connect_cloud_server(self, url: str): "use a service account credential directly." % saved ) + def _connect_cloud_secrets(self, entry: ServerDataDict) -> tuple[Optional[str], Optional[str], Optional[str]]: + """A Posit Connect Cloud entry's secrets, as (access, refresh, client secret). + + `rsconnect add` and token refresh store these in the system keyring when + there is one, leaving the fields in this file empty; those fields are the + fallback for machines without a usable keyring. Entries for other targets + have no Connect Cloud secrets to look for. + """ + access = entry.get("connect_cloud_access_token") + refresh = entry.get("connect_cloud_refresh_token") + client_secret = entry.get("connect_cloud_client_secret") + if not entry.get("connect_cloud_account_name"): + return access, refresh, client_secret + + keyring_access, keyring_refresh, keyring_secret = connect_cloud.credentials_from_keyring( + entry["url"], entry["name"] + ) + return ( + keyring_access or access, + keyring_refresh or refresh, + keyring_secret or client_secret, + ) + def get_all_servers(self): """ Returns a list of all known servers sorted by nickname. @@ -531,9 +554,10 @@ def set( target_data["connect_cloud_client_id"] = connect_cloud_client_id if connect_cloud_client_secret: target_data["connect_cloud_client_secret"] = connect_cloud_client_secret - # Tokens live in this file, which is written 0600. The system keyring is - # only used by `rsconnect login` for Connect, not here -- the same as - # shinyapps.io's token and secret, and as the R client's account DCF. + # Callers pass the tokens only when they could not be stored in the + # system keyring; this file, written 0600, is the fallback for machines + # without one -- the same as shinyapps.io's token and secret, and as the + # R client's account DCF. if connect_cloud_access_token: target_data["connect_cloud_access_token"] = connect_cloud_access_token if connect_cloud_refresh_token: @@ -558,6 +582,13 @@ def set( entry["default"] = True self._set(name, entry) # type: ignore + # Nothing records the URL a replaced Connect Cloud credential's keyring + # values are keyed by any more, so they would be both unreachable and able + # to shadow a later credential saved under the same nickname and URL. + if existing and existing.get("connect_cloud_account_name"): + if not connect_cloud_account_name or existing["url"] != url: + connect_cloud.delete_credentials_from_keyring(existing["url"], name) + def remove_by_name(self, name: str): """ Remove the server information for the given nickname. @@ -638,6 +669,7 @@ def resolve(self, name: Optional[str], url: Optional[str]) -> ServerData: entry = self._get_first_value() if entry: + cloud_access_token, cloud_refresh_token, cloud_client_secret = self._connect_cloud_secrets(entry) return ServerData( name or entry["name"], entry["url"], @@ -656,9 +688,9 @@ def resolve(self, name: Optional[str], url: Optional[str]) -> ServerData: connect_cloud_account_name=entry.get("connect_cloud_account_name"), connect_cloud_account_id=entry.get("connect_cloud_account_id"), connect_cloud_client_id=entry.get("connect_cloud_client_id"), - connect_cloud_client_secret=entry.get("connect_cloud_client_secret"), - connect_cloud_access_token=entry.get("connect_cloud_access_token"), - connect_cloud_refresh_token=entry.get("connect_cloud_refresh_token"), + connect_cloud_client_secret=cloud_client_secret, + connect_cloud_access_token=cloud_access_token, + connect_cloud_refresh_token=cloud_refresh_token, ) else: return ServerData( diff --git a/rsconnect/oauth.py b/rsconnect/oauth.py index 260a7945..0b1817ef 100644 --- a/rsconnect/oauth.py +++ b/rsconnect/oauth.py @@ -14,7 +14,7 @@ import time import webbrowser from http.server import BaseHTTPRequestHandler, HTTPServer as _HTTPServer -from typing import Any, Dict, Optional, Tuple, cast +from typing import Any, Dict, Iterable, Mapping, Optional, Tuple, cast from urllib.parse import parse_qs, urlencode, urlparse, urlunparse import click @@ -639,60 +639,136 @@ def _token_exchange_error(status: Optional[int], data: dict[str, Any]) -> RSConn # --------------------------------------------------------------------------- -def keyring_store_token(server_url: str, access_token: str, refresh_token: Optional[str]) -> bool: - """Store OAuth tokens in the system keyring. +ACCESS_TOKEN_FIELD = "access_token" +REFRESH_TOKEN_FIELD = "refresh_token" +CLIENT_SECRET_FIELD = "client_secret" - Returns True on success, False if keyring is not available. + +def keyring_store_values(key: str, values: Mapping[str, Optional[str]]) -> bool: + """Store credential values in the system keyring, deleting the empty ones. + + Entries are named ":". A Posit Connect caller passes the bare + server URL as the key: that format is frozen, because changing it would make + every existing `rsconnect login` miss its keychain entry. Callers that share + one URL across several saved credentials -- Posit Connect Cloud -- pass a key + that includes the nickname. + + Returns True on success, False if keyring is not available, which is the + caller's signal to fall back to servers.json. """ + # Only the import itself means "no keyring here": keyring imports its backend + # lazily, so an ImportError from set_password is a failure of one that exists. try: import keyring # type: ignore[import-untyped] - - keyring.set_password(_KEYRING_SERVICE, f"{server_url}:access_token", access_token) - if refresh_token: - keyring.set_password(_KEYRING_SERVICE, f"{server_url}:refresh_token", refresh_token) - else: - try: - keyring.delete_password(_KEYRING_SERVICE, f"{server_url}:refresh_token") - except keyring.errors.PasswordDeleteError: - pass - return True except ImportError: return False + + try: + for field, value in values.items(): + username = f"{key}:{field}" + if value: + keyring.set_password(_KEYRING_SERVICE, username, value) + else: + try: + keyring.delete_password(_KEYRING_SERVICE, username) + except keyring.errors.PasswordDeleteError: + pass + return True except Exception as e: logger.warning(f"keyring storage failed: {e}") + # The caller now writes all of these values to servers.json, and reads + # prefer the keyring, so a field written before the failure would shadow + # the file with a value that no longer belongs to the others. Returning + # False is only safe once they are gone. + if not keyring_delete_values(key, values): + raise RSConnectException( + "Could not store credentials in the system keyring, and the entries written before the " + "failure could not be removed either. Delete the rsconnect-python entries from your " + "keyring, then save the credential again." + ) from e return False -def keyring_get_tokens(server_url: str) -> Tuple[Optional[str], Optional[str]]: - """Retrieve OAuth tokens from the system keyring. +def keyring_read_value(key: str, field: str) -> Tuple[bool, Optional[str]]: + """Retrieve one credential value, saying whether the keyring could be read. - Returns (access_token, refresh_token), or (None, None) if unavailable. + A caller that decides what to write based on what is already stored needs to + tell "nothing stored" from "could not look": a machine with no keyring at all + knowably has nothing, but a keyring that raises could have anything. """ try: import keyring # type: ignore[import-untyped] - - access = keyring.get_password(_KEYRING_SERVICE, f"{server_url}:access_token") - refresh = keyring.get_password(_KEYRING_SERVICE, f"{server_url}:refresh_token") - return access, refresh except ImportError: - return None, None + return True, None + + try: + return True, keyring.get_password(_KEYRING_SERVICE, f"{key}:{field}") except Exception as e: logger.warning(f"keyring retrieval failed: {e}") - return None, None + return False, None -def keyring_delete_tokens(server_url: str) -> None: - """Delete OAuth tokens from the system keyring.""" +def keyring_get_value(key: str, field: str) -> Optional[str]: + """Retrieve one credential value from the system keyring, or None.""" + return keyring_read_value(key, field)[1] + + +def keyring_delete_values(key: str, fields: Iterable[str]) -> bool: + """Delete credential values from the system keyring. + + Returns whether the values are gone, which they also are when there is no + keyring holding them or they were never stored. + """ try: import keyring # type: ignore[import-untyped] + except ImportError: + return True + + try: + # Inside this block, not above: keyring itself imported, so there may be + # values in it, and without its errors module the "nothing was stored" case + # cannot be told from a deletion that failed. import keyring.errors # type: ignore[import-untyped] - for suffix in (":access_token", ":refresh_token"): + deleted = True + for field in fields: + username = f"{key}:{field}" try: - keyring.delete_password(_KEYRING_SERVICE, f"{server_url}{suffix}") + keyring.delete_password(_KEYRING_SERVICE, username) except keyring.errors.PasswordDeleteError: - pass - except ImportError: - pass + # The deletion did not happen. Usually because there was nothing + # stored, which is the state the caller is after, so check. + try: + if keyring.get_password(_KEYRING_SERVICE, username) is not None: + deleted = False + except Exception as e: + logger.warning(f"keyring deletion failed: {e}") + deleted = False + except Exception as e: + logger.warning(f"keyring deletion failed: {e}") + deleted = False + return deleted except Exception as e: logger.warning(f"keyring deletion failed: {e}") + return False + + +def keyring_store_token(key: str, access_token: Optional[str], refresh_token: Optional[str]) -> bool: + """Store OAuth tokens in the system keyring. + + Returns True on success, False if keyring is not available. + """ + return keyring_store_values(key, {ACCESS_TOKEN_FIELD: access_token, REFRESH_TOKEN_FIELD: refresh_token}) + + +def keyring_get_tokens(key: str) -> Tuple[Optional[str], Optional[str]]: + """Retrieve OAuth tokens from the system keyring. + + Returns (access_token, refresh_token), or (None, None) if unavailable. + """ + return keyring_get_value(key, ACCESS_TOKEN_FIELD), keyring_get_value(key, REFRESH_TOKEN_FIELD) + + +def keyring_delete_tokens(key: str) -> None: + """Delete OAuth tokens from the system keyring.""" + keyring_delete_values(key, (ACCESS_TOKEN_FIELD, REFRESH_TOKEN_FIELD)) diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 00000000..9b9f11e8 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,15 @@ +import sys + +import pytest + + +@pytest.fixture(autouse=True) +def no_system_keyring(monkeypatch: pytest.MonkeyPatch): + """Make the system keyring unavailable to every test. + + `keyring` is installed in the test environment (twine depends on it), so + without this the credential paths would read and write the machine's real + keychain. Tests that exercise keyring storage replace `sys.modules["keyring"]` + with a mock of their own, or patch the helpers in `rsconnect.oauth`. + """ + monkeypatch.setitem(sys.modules, "keyring", None) diff --git a/tests/test_connect_cloud.py b/tests/test_connect_cloud.py index 2ca0d99d..1422fd65 100644 --- a/tests/test_connect_cloud.py +++ b/tests/test_connect_cloud.py @@ -2,6 +2,7 @@ import io import json import os +import sys import tarfile import tempfile import unittest @@ -448,6 +449,37 @@ def _skip_account_check(test): test.addCleanup(patch.stop) +class FakeKeyring: + """A stand-in for the keyring module, backed by a dict.""" + + class errors: + class PasswordDeleteError(Exception): + pass + + def __init__(self): + self.passwords: Dict[Any, str] = {} + + def set_password(self, service: str, username: str, password: str) -> None: + self.passwords[(service, username)] = password + + def get_password(self, service: str, username: str) -> Optional[str]: + return self.passwords.get((service, username)) + + def delete_password(self, service: str, username: str) -> None: + if (service, username) not in self.passwords: + raise self.errors.PasswordDeleteError() + del self.passwords[(service, username)] + + +def _use_fake_keyring(test: unittest.TestCase) -> FakeKeyring: + """Give one test a working keyring; conftest makes it unavailable by default.""" + fake = FakeKeyring() + patch = mock.patch.dict(sys.modules, {"keyring": fake, "keyring.errors": fake.errors}) + patch.start() + test.addCleanup(patch.stop) + return fake + + class CliTestCase(unittest.TestCase): """A CliRunner against a temporary server store and a clean environment.""" @@ -1019,6 +1051,13 @@ def test_add_interactive_stores_tokens(self): ) self.assertIn("Posit Connect Cloud", result.output) + def test_add_says_where_the_credentials_went_without_a_keyring(self): + self._mock_device_login() + result = self.runner.invoke(cli, ["add", "-n", "cloud", "--connect-cloud", "-A", "acme"]) + + self.assertEqual(result.exit_code, 0, result.output) + self.assertIn("keyring not available", result.output) + def test_a_stale_ca_certificate_env_var_does_not_block_add(self): # CONNECT_CA_CERTIFICATE pointing at a missing file used to fail at CLI # parsing (click Path(exists=True)), before the Cloud target was known. @@ -2980,6 +3019,246 @@ def test_a_connect_server_url_is_unaffected(self): self.assertEqual(entry["name"], "prod") +class TestConnectCloudKeyringStorage(CliTestCase): + """Connect Cloud secrets go to the system keyring when there is one, keyed by URL + and nickname because one URL covers every Connect Cloud credential. The + servers.json fields stay as the fallback for machines without a usable keyring.""" + + def setUp(self): + super().setUp() + self.keyring = _use_fake_keyring(self) + self.base_dir = os.path.dirname(self.store.get_path()) + # Token write-back opens its own store, inside the function, from this module. + store_patch = mock.patch("rsconnect.metadata.ServerStore", lambda: ServerStore(base_dir=self.base_dir)) + store_patch.start() + self.addCleanup(store_patch.stop) + + def _stored(self, field: str, nickname: str = "cloud") -> Optional[str]: + return self.keyring.get_password("rsconnect-python", "%s#%s:%s" % (API, nickname, field)) + + def _store_in_keyring(self, field: str, value: str, nickname: str = "cloud") -> None: + self.keyring.set_password("rsconnect-python", "%s#%s:%s" % (API, nickname, field), value) + + def _saved_entry(self, nickname: str = "cloud") -> Any: + entry = ServerStore(base_dir=self.base_dir).get_by_name(nickname) + assert entry is not None + return entry + + def _refresh(self, **kwargs: Any) -> bool: + server = ConnectCloudServer("acme", access_token="stale", refresh_token="rt", server_name="cloud") + client = ConnectCloudClient(server) + with mock.patch("rsconnect.connect_cloud.refresh", **kwargs): + return client._attempt_token_refresh() + + def test_add_stores_the_tokens_in_the_keyring_and_not_in_the_file(self): + self._mock_device_login() + result = self.runner.invoke(cli, ["add", "-n", "cloud", "--connect-cloud", "-A", "acme"]) + + self.assertEqual(result.exit_code, 0, result.output) + self.assertEqual(self._stored("access_token"), "at") + self.assertEqual(self._stored("refresh_token"), "rt") + entry = self._saved_entry() + self.assertNotIn("connect_cloud_access_token", entry) + self.assertNotIn("connect_cloud_refresh_token", entry) + self.assertEqual(entry["connect_cloud_account_name"], "acme") + + def test_add_stores_a_service_account_secret_in_the_keyring(self): + with mock.patch( + "rsconnect.connect_cloud.request_client_credentials_token", return_value={"access_token": "at"} + ): + result = self.runner.invoke( + cli, + ["add", "-n", "cloud", "--connect-cloud", "-A", "acme", "--client-id", "cid", "--client-secret", "sec"], + ) + + self.assertEqual(result.exit_code, 0, result.output) + self.assertEqual(self._stored("client_secret"), "sec") + entry = self._saved_entry() + self.assertNotIn("connect_cloud_client_secret", entry) + # The client id is not a secret, and is what `list` shows to identify the credential. + self.assertEqual(entry["connect_cloud_client_id"], "cid") + + def test_each_nickname_keys_its_own_entries(self): + self._mock_device_login() + for nickname in ("cloud", "other"): + result = self.runner.invoke(cli, ["add", "-n", nickname, "--connect-cloud", "-A", "acme"]) + self.assertEqual(result.exit_code, 0, result.output) + + self.assertEqual(self._stored("access_token"), "at") + self.assertEqual(self._stored("access_token", nickname="other"), "at") + self.assertEqual(len(self.keyring.passwords), 4) + + def test_the_keyring_wins_over_the_fields_left_in_the_file(self): + # An entry saved before keyring support keeps its plaintext fields until the + # next add or refresh; whatever the keyring holds is used meanwhile. + self.store.set( + "cloud", + API, + connect_cloud_account_name="acme", + connect_cloud_access_token="file-at", + connect_cloud_refresh_token="file-rt", + connect_cloud_client_secret="file-secret", + ) + self._store_in_keyring("access_token", "keyring-at") + + data = self.store.resolve("cloud", None) + + self.assertEqual(data.connect_cloud_access_token, "keyring-at") + self.assertEqual(data.connect_cloud_refresh_token, "file-rt") + self.assertEqual(data.connect_cloud_client_secret, "file-secret") + + def test_a_refresh_moves_the_tokens_out_of_the_file(self): + self.store.set( + "cloud", + API, + connect_cloud_account_name="acme", + connect_cloud_access_token="stale", + connect_cloud_refresh_token="rt", + ) + + self.assertTrue(self._refresh(return_value={"access_token": "new-at", "refresh_token": "new-rt"})) + + self.assertEqual(self._stored("access_token"), "new-at") + self.assertEqual(self._stored("refresh_token"), "new-rt") + entry = self._saved_entry() + self.assertNotIn("connect_cloud_access_token", entry) + self.assertNotIn("connect_cloud_refresh_token", entry) + + def test_a_refresh_moves_a_client_secret_out_of_the_file_too(self): + self.store.set( + "cloud", + API, + connect_cloud_account_name="acme", + connect_cloud_client_id="cid", + connect_cloud_client_secret="file-secret", + connect_cloud_refresh_token="rt", + ) + + self.assertTrue(self._refresh(return_value={"access_token": "new-at"})) + + self.assertEqual(self._stored("client_secret"), "file-secret") + entry = self._saved_entry() + self.assertNotIn("connect_cloud_client_secret", entry) + self.assertEqual(entry["connect_cloud_client_id"], "cid") + + def test_a_refresh_keeps_a_client_secret_that_is_already_in_the_keyring(self): + self.store.set("cloud", API, connect_cloud_account_name="acme", connect_cloud_client_id="cid") + self._store_in_keyring("client_secret", "keyring-secret") + + self.assertTrue(self._refresh(return_value={"access_token": "new-at"})) + + self.assertEqual(self._stored("client_secret"), "keyring-secret") + + def test_a_refresh_does_not_overwrite_the_keyring_secret_with_the_files(self): + # A secret in both places means the file's copy predates the one in use; the + # keyring is what reads prefer, so it stays and the stale copy goes. + self.store.set( + "cloud", + API, + connect_cloud_account_name="acme", + connect_cloud_client_id="cid", + connect_cloud_client_secret="file-secret", + ) + self._store_in_keyring("client_secret", "keyring-secret") + + self.assertTrue(self._refresh(return_value={"access_token": "new-at"})) + + self.assertEqual(self._stored("client_secret"), "keyring-secret") + self.assertNotIn("connect_cloud_client_secret", self._saved_entry()) + + def test_a_keyring_read_failure_leaves_the_client_secret_where_it_is(self): + # A read that failed says nothing about what the keyring holds, so the + # file's copy is neither written over it nor dropped from the file. + self.store.set( + "cloud", + API, + connect_cloud_account_name="acme", + connect_cloud_client_id="cid", + connect_cloud_client_secret="file-secret", + connect_cloud_refresh_token="rt", + ) + secret_username = "%s#cloud:%s" % (API, "client_secret") + stored = self.keyring.get_password + + def failing_read(service: str, username: str) -> Optional[str]: + if username == secret_username: + raise Exception("keychain locked") + return stored(service, username) + + with mock.patch.object(self.keyring, "get_password", side_effect=failing_read): + self.assertTrue(self._refresh(return_value={"access_token": "new-at"})) + + self.assertIsNone(self._stored("client_secret")) + self.assertEqual(self._saved_entry()["connect_cloud_client_secret"], "file-secret") + + def test_replacing_a_credential_discards_its_keyring_values(self): + self.store.set("cloud", API, connect_cloud_account_name="acme") + self._store_in_keyring("access_token", "at") + + self.store.set("cloud", "https://connect.example.com", api_key="key") + + self.assertEqual(self.keyring.passwords, {}) + + def test_moving_a_credential_to_another_environment_discards_the_old_values(self): + self.store.set("cloud", API, connect_cloud_account_name="acme") + self._store_in_keyring("access_token", "at") + + self.store.set("cloud", "https://api.staging.connect.posit.cloud/v1", connect_cloud_account_name="acme") + + self.assertEqual(self.keyring.passwords, {}) + + def test_resaving_the_same_credential_keeps_its_keyring_values(self): + self.store.set("cloud", API, connect_cloud_account_name="acme") + self._store_in_keyring("access_token", "at") + + self.store.set("cloud", API, connect_cloud_account_name="other-account") + + self.assertEqual(self._stored("access_token"), "at") + + def test_an_expired_refresh_token_clears_the_keyring_tokens(self): + self.store.set("cloud", API, connect_cloud_account_name="acme") + self._store_in_keyring("access_token", "stale") + self._store_in_keyring("refresh_token", "rt") + self._store_in_keyring("client_secret", "sec") + + with self.assertRaises(RSConnectException): + self._refresh(side_effect=InvalidGrantError()) + + self.assertIsNone(self._stored("access_token")) + self.assertIsNone(self._stored("refresh_token")) + # Only the dead tokens go; the credential itself is what gets re-authenticated. + self.assertEqual(self._stored("client_secret"), "sec") + + def test_remove_deletes_the_keyring_entries(self): + self.store.set("cloud", API, connect_cloud_account_name="acme") + for field in ("access_token", "refresh_token", "client_secret"): + self._store_in_keyring(field, field + "-value") + + result = self.runner.invoke(cli, ["remove", "-n", "cloud"]) + + self.assertEqual(result.exit_code, 0, result.output) + self.assertEqual(self.keyring.passwords, {}) + + def test_remove_by_url_deletes_the_keyring_entries(self): + self.store.set("cloud", API, connect_cloud_account_name="acme") + self._store_in_keyring("access_token", "at") + + result = self.runner.invoke(cli, ["remove", "-s", connect_cloud.SERVER_NAME]) + + self.assertEqual(result.exit_code, 0, result.output) + self.assertEqual(self.keyring.passwords, {}) + + def test_list_reports_credentials_in_the_keyring(self): + self.store.set("cloud", API, connect_cloud_account_name="acme") + self._store_in_keyring("access_token", "at") + + result = self.runner.invoke(cli, ["list"]) + + self.assertEqual(result.exit_code, 0, result.output) + self.assertIn("Credentials stored in system keyring", result.output) + self.assertNotIn("Credentials are saved", result.output) + + class TestConnectCloudVisibility(unittest.TestCase): def _executor(self, visibility=None, server=None): executor = RSConnectExecutor.__new__(RSConnectExecutor) diff --git a/tests/test_oauth.py b/tests/test_oauth.py index cda9a275..7599ba11 100644 --- a/tests/test_oauth.py +++ b/tests/test_oauth.py @@ -19,7 +19,9 @@ exchange_token_for_api_key, generate_pkce_pair, keyring_delete_tokens, + keyring_delete_values, keyring_get_tokens, + keyring_read_value, keyring_store_token, login_with_browser, login_with_device_code, @@ -306,6 +308,10 @@ def test_invalid_grant_without_a_description(self, mock_http_server: MagicMock): assert raised.value.description is None +class _PasswordDeleteError(Exception): + """Stands in for keyring.errors.PasswordDeleteError, which means "nothing stored".""" + + class TestKeyringIntegration: def test_store_success(self): mock_keyring = MagicMock() @@ -333,6 +339,95 @@ def test_get_no_keyring(self): assert access is None assert refresh is None + def test_a_failure_partway_through_leaves_nothing_behind(self): + # The caller falls back to storing every value in servers.json, and reads + # prefer the keyring, so a field written before the failure would shadow it. + mock_keyring = MagicMock() + mock_keyring.set_password.side_effect = [None, Exception("keychain locked")] + mock_keyring_errors = MagicMock() + mock_keyring_errors.PasswordDeleteError = _PasswordDeleteError + with patch.dict("sys.modules", {"keyring": mock_keyring, "keyring.errors": mock_keyring_errors}): + result = keyring_store_token("https://example.com", "at-1", "rt-1") + + assert result is False + assert [call.args[1] for call in mock_keyring.delete_password.call_args_list] == [ + "https://example.com:access_token", + "https://example.com:refresh_token", + ] + + def test_a_cleanup_that_leaves_the_value_behind_is_reported(self): + # PasswordDeleteError says the deletion did not happen, which is only the + # state the caller needs when there was nothing there to delete. + mock_keyring = MagicMock() + mock_keyring.set_password.side_effect = [None, Exception("keychain locked")] + mock_keyring.delete_password.side_effect = _PasswordDeleteError() + mock_keyring.get_password.return_value = "at-1" + mock_keyring_errors = MagicMock() + mock_keyring_errors.PasswordDeleteError = _PasswordDeleteError + with patch.dict("sys.modules", {"keyring": mock_keyring, "keyring.errors": mock_keyring_errors}): + with pytest.raises(RSConnectException, match="could not be removed"): + keyring_store_token("https://example.com", "at-1", "rt-1") + + def test_a_read_failure_is_not_reported_as_an_absent_value(self): + mock_keyring = MagicMock() + mock_keyring.get_password.side_effect = Exception("keychain locked") + with patch.dict("sys.modules", {"keyring": mock_keyring}): + readable, value = keyring_read_value("https://example.com", "access_token") + + assert (readable, value) == (False, None) + + def test_no_keyring_at_all_reads_as_an_absent_value(self): + with patch.dict("sys.modules", {"keyring": None}): + assert keyring_read_value("https://example.com", "access_token") == (True, None) + + def test_deletion_without_the_keyring_errors_module_is_a_failure(self): + # keyring itself imported, so there may be values in it, and its errors + # module is what tells "nothing was stored" from a deletion that failed. + mock_keyring = MagicMock() + with patch.dict("sys.modules", {"keyring": mock_keyring, "keyring.errors": None}): + assert keyring_delete_values("https://example.com", ("access_token",)) is False + + def test_deletion_without_a_keyring_at_all_succeeds(self): + with patch.dict("sys.modules", {"keyring": None}): + assert keyring_delete_values("https://example.com", ("access_token",)) is True + + def test_a_backend_import_error_is_a_read_failure(self): + # keyring imports its backend lazily, so an ImportError from get_password + # comes from a keyring that exists and could hold anything. + mock_keyring = MagicMock() + mock_keyring.get_password.side_effect = ImportError("no backend module") + with patch.dict("sys.modules", {"keyring": mock_keyring}): + assert keyring_read_value("https://example.com", "access_token") == (False, None) + + def test_a_failure_that_cannot_be_cleaned_up_is_reported(self): + # Falling back to servers.json would leave the written field shadowing it, + # so this is not something the caller can carry on from. + mock_keyring = MagicMock() + mock_keyring.set_password.side_effect = [None, Exception("keychain locked")] + mock_keyring.delete_password.side_effect = Exception("keychain locked") + mock_keyring_errors = MagicMock() + mock_keyring_errors.PasswordDeleteError = _PasswordDeleteError + with patch.dict("sys.modules", {"keyring": mock_keyring, "keyring.errors": mock_keyring_errors}): + with pytest.raises(RSConnectException, match="could not be removed"): + keyring_store_token("https://example.com", "at-1", "rt-1") + + def test_connect_entries_are_named_by_url_alone(self): + # Frozen format: any change here makes every existing `rsconnect login` miss + # its keychain entry and orphans the old one. + mock_keyring = MagicMock() + with patch.dict("sys.modules", {"keyring": mock_keyring}): + keyring_store_token("https://connect.example.com", "at-1", "rt-1") + keyring_get_tokens("https://connect.example.com") + + assert [call.args for call in mock_keyring.set_password.call_args_list] == [ + ("rsconnect-python", "https://connect.example.com:access_token", "at-1"), + ("rsconnect-python", "https://connect.example.com:refresh_token", "rt-1"), + ] + assert [call.args for call in mock_keyring.get_password.call_args_list] == [ + ("rsconnect-python", "https://connect.example.com:access_token"), + ("rsconnect-python", "https://connect.example.com:refresh_token"), + ] + def test_delete_success(self): mock_keyring = MagicMock() mock_keyring_errors = MagicMock() From 25a7e975ce89c436c7aedbb28e46947062f20c5d Mon Sep 17 00:00:00 2001 From: Sam Perman Date: Fri, 14 Aug 2026 16:53:40 -0400 Subject: [PATCH 4/9] refactor: share the 401 refresh-and-retry skeleton between both clients 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. --- rsconnect/api.py | 74 ++++++++----------------------- rsconnect/http_support.py | 51 ++++++++++++++++++++++ rsconnect/metadata.py | 11 +++++ rsconnect/oauth.py | 43 +++++++++++++++++- tests/test_connect_cloud.py | 87 +++++++++++++++++++++++++++++++++++++ tests/test_oauth.py | 87 ++++++++++++++++++++++++++++++++++--- tests/utils.py | 29 +++++++++++++ 7 files changed, 317 insertions(+), 65 deletions(-) diff --git a/rsconnect/api.py b/rsconnect/api.py index 0201064b..07c3f98f 100644 --- a/rsconnect/api.py +++ b/rsconnect/api.py @@ -60,6 +60,7 @@ from .environment import fake_module_file_from_directory from .exception import DeploymentFailedException, RSConnectException from .http_support import ( + BearerTokenHTTPServer, CookieJar, HTTPResponse, HTTPServer, @@ -501,7 +502,7 @@ def server_supports_draft_deploy(server_version: Optional[str]) -> bool: return False -class RSConnectClient(HTTPServer): +class RSConnectClient(BearerTokenHTTPServer): def __init__(self, server: Union[RSConnectServer, SPCSConnectServer], cookies: Optional[CookieJar] = None): if cookies is None: cookies = server.cookie_jar @@ -533,30 +534,10 @@ def __init__(self, server: Union[RSConnectServer, SPCSConnectServer], cookies: O ): self.authorization(f"Bearer {server.oauth_access_token}") - def request( - self, - method: str, - path: str, - query_params: Optional[Mapping[str, "JsonData"]] = None, - body: "str | bytes | IO[bytes] | Mapping[str, Any] | list[Any] | None" = None, - maximum_redirects: int = 5, - decode_response: bool = True, - headers: Optional[Mapping[str, str]] = None, - ) -> "JsonData | HTTPResponse": - can_retry = isinstance(self._server, RSConnectServer) and bool(self._server.oauth_client_id) - start_pos: "int | None" = None - if can_retry and hasattr(body, "read"): - if getattr(body, "seekable", lambda: False)(): - start_pos = body.tell() # type: ignore[union-attr] - else: - body = body.read() # type: ignore[union-attr] - response = super().request(method, path, query_params, body, maximum_redirects, decode_response, headers) # pyright: ignore[reportUnknownArgumentType] - if can_retry and isinstance(response, HTTPResponse) and response.status == 401: - if self._attempt_token_refresh(): - if start_pos is not None: - body.seek(start_pos) # type: ignore[union-attr] - return super().request(method, path, query_params, body, maximum_redirects, decode_response, headers) # pyright: ignore[reportUnknownArgumentType] - return response + def _can_refresh_token(self) -> bool: + # An API key, a bootstrap JWT, or a Snowflake token exchange has nothing to + # mint a new credential from; only an OAuth login does. + return isinstance(self._server, RSConnectServer) and bool(self._server.oauth_client_id) def _attempt_token_refresh(self) -> bool: from .oauth import ( @@ -572,14 +553,11 @@ def _attempt_token_refresh(self) -> bool: server = cast(RSConnectServer, self._server) + # The keyring is where a login stores its tokens; the entry's own fields are + # the fallback for a machine without one. _, refresh_token = keyring_get_tokens(server.url) if not refresh_token: - store = ServerStore() - entry = None - if server.server_name: - entry = store.get_by_name(server.server_name) - if not entry: - entry = store.get_by_url(server.url) + entry = ServerStore().saved_entry(server.server_name, server.url) if entry: refresh_token = entry.get("oauth_refresh_token") # type: ignore[assignment] if not refresh_token: @@ -594,11 +572,7 @@ def _attempt_token_refresh(self) -> bool: # Client was deleted server-side; clear stale tokens and re-register keyring_delete_tokens(server.url) store = ServerStore() - entry = None - if server.server_name: - entry = store.get_by_name(server.server_name) - if not entry: - entry = store.get_by_url(server.url) + entry = store.saved_entry(server.server_name, server.url) if entry: entry_name = str(entry.get("name", server.server_name or server.url)) store.update_oauth_tokens(entry_name, None, None, None) @@ -630,11 +604,7 @@ def _attempt_token_refresh(self) -> bool: stored = keyring_store_token(server.url, new_access, new_refresh) if not stored: store = ServerStore() - entry = None - if server.server_name: - entry = store.get_by_name(server.server_name) - if not entry: - entry = store.get_by_url(server.url) + entry = store.saved_entry(server.server_name, server.url) if entry: entry_name = str(entry.get("name", server.server_name or server.url)) store.update_oauth_tokens(entry_name, new_access, new_refresh, new_expiry) @@ -2908,7 +2878,7 @@ class ConnectCloudAuthorization(TypedDict): _CONNECT_CLOUD_PUBLISH_PERMISSION = "content:create" -class ConnectCloudClient(HTTPServer): +class ConnectCloudClient(BearerTokenHTTPServer): """ An HTTP client to call the Posit Connect Cloud API. @@ -2934,21 +2904,11 @@ def _tweak_response(self, response: HTTPResponse) -> JsonData | HTTPResponse: else response ) - def request( - self, - method: str, - path: str, - query_params: Optional[Mapping[str, JsonData]] = None, - body: str | bytes | IO[bytes] | Mapping[str, Any] | list[Any] | None = None, - maximum_redirects: int = 5, - decode_response: bool = True, - headers: Optional[Mapping[str, str]] = None, - ) -> JsonData | HTTPResponse: - response = super().request(method, path, query_params, body, maximum_redirects, decode_response, headers) # pyright: ignore[reportUnknownArgumentType] - if isinstance(response, HTTPResponse) and response.status == 401: - if self._attempt_token_refresh(): - return super().request(method, path, query_params, body, maximum_redirects, decode_response, headers) # pyright: ignore[reportUnknownArgumentType] - return response + def _can_refresh_token(self) -> bool: + # The same two credentials _attempt_token_refresh mints from. Without either + # there is no retry to prepare a request body for. + server = self._server + return bool(server.refresh_token or (server.client_id and server.client_secret)) def _attempt_token_refresh(self) -> bool: """Mint a new access token and apply it to this client. diff --git a/rsconnect/http_support.py b/rsconnect/http_support.py index aced9661..34be9bc7 100644 --- a/rsconnect/http_support.py +++ b/rsconnect/http_support.py @@ -652,6 +652,57 @@ def _inject_cookies(self): del self._headers["Cookie"] +class BearerTokenHTTPServer(HTTPServer): + """An HTTPServer whose requests carry an OAuth access token. + + When a token expires the server answers 401, so the response is handled by + minting a new token and sending the request once more. Subclasses provide the + minting in `_attempt_token_refresh`, which also applies the new token to this + client, and say in `_can_refresh_token` whether there is anything to mint from. + """ + + def _can_refresh_token(self) -> bool: + return True + + def _attempt_token_refresh(self) -> bool: + """Mint a new access token and apply it to this client. + + Returns whether a new token was obtained. Raises for a credential that no + retry could fix, rather than leaving the caller with the opaque 401. + """ + raise NotImplementedError + + def request( + self, + method: str, + path: str, + query_params: Optional[Mapping[str, JsonData]] = None, + body: str | bytes | IO[bytes] | Mapping[str, Any] | list[Any] | None = None, + maximum_redirects: int = 5, + decode_response: bool = True, + headers: Optional[Mapping[str, str]] = None, + ) -> JsonData | HTTPResponse: + if not self._can_refresh_token(): + return super().request(method, path, query_params, body, maximum_redirects, decode_response, headers) # pyright: ignore[reportUnknownArgumentType] + + start_pos: int | None = None + if hasattr(body, "read"): + # The first attempt consumes a streamed body (a bundle upload), so a + # retry has to rewind it -- or hold it in memory when it cannot seek. + if getattr(body, "seekable", lambda: False)(): + start_pos = body.tell() # type: ignore[union-attr] + else: + body = body.read() # type: ignore[union-attr] + + response = super().request(method, path, query_params, body, maximum_redirects, decode_response, headers) # pyright: ignore[reportUnknownArgumentType] + if isinstance(response, HTTPResponse) and response.status == 401: + if self._attempt_token_refresh(): + if start_pos is not None: + body.seek(start_pos) # type: ignore[union-attr] + return super().request(method, path, query_params, body, maximum_redirects, decode_response, headers) # pyright: ignore[reportUnknownArgumentType] + return response + + class CookieJar(object): @staticmethod def from_dict(source: dict[str, Any]): diff --git a/rsconnect/metadata.py b/rsconnect/metadata.py index 168ce28b..f08783dc 100644 --- a/rsconnect/metadata.py +++ b/rsconnect/metadata.py @@ -385,6 +385,17 @@ def get_by_url(self, url: str): return self._get_connect_cloud_server(target) return self._get_by_value_attr("url", target) + def saved_entry(self, name: Optional[str], url: str) -> Optional[ServerDataDict]: + """The entry a server in hand came from: by its nickname, else by its URL. + + A one-shot target has no nickname, and neither does one resolved before the + nickname was carried on the server, so the URL is the fallback. + """ + entry = self.get_by_name(name) if name else None + if entry is None: + entry = self.get_by_url(url) + return entry + def has_connect_cloud_account(self, url: Optional[str]) -> bool: """Whether any Posit Connect Cloud credential is saved for this URL. diff --git a/rsconnect/oauth.py b/rsconnect/oauth.py index 0b1817ef..3ce8bc60 100644 --- a/rsconnect/oauth.py +++ b/rsconnect/oauth.py @@ -644,6 +644,27 @@ def _token_exchange_error(status: Optional[int], data: dict[str, Any]) -> RSConn CLIENT_SECRET_FIELD = "client_secret" +def _no_keyring_errors() -> tuple[type[BaseException], ...]: + """The exceptions that mean this machine has no keyring, for an `except` clause. + + keyring raises NoKeyringError from every operation when no backend is usable, + which is the normal state of a CI runner with the package installed. It is a + sibling of PasswordSetError and PasswordDeleteError under KeyringError, not a + subclass of either, so nothing catches it unless it is named here -- and being + treated as a keyring that failed rather than one that is absent is what broke the + servers.json fallback. Other KeyringError subclasses stay in the failure bucket: + a locked or half-initialized backend may well hold credentials. + + Empty, matching nothing, when the errors module will not import: the failure + bucket is the safe answer when the two cannot be told apart. + """ + try: + from keyring.errors import NoKeyringError # type: ignore[import-untyped] + except ImportError: + return () + return (NoKeyringError,) + + def keyring_store_values(key: str, values: Mapping[str, Optional[str]]) -> bool: """Store credential values in the system keyring, deleting the empty ones. @@ -674,6 +695,11 @@ def keyring_store_values(key: str, values: Mapping[str, Optional[str]]) -> bool: except keyring.errors.PasswordDeleteError: pass return True + except _no_keyring_errors() as e: + # Nothing was stored, so there is no partial write to clean up below -- and + # the cleanup would fail the same way, which used to make this raise. + logger.debug(f"no system keyring available: {e}") + return False except Exception as e: logger.warning(f"keyring storage failed: {e}") # The caller now writes all of these values to servers.json, and reads @@ -693,8 +719,9 @@ def keyring_read_value(key: str, field: str) -> Tuple[bool, Optional[str]]: """Retrieve one credential value, saying whether the keyring could be read. A caller that decides what to write based on what is already stored needs to - tell "nothing stored" from "could not look": a machine with no keyring at all - knowably has nothing, but a keyring that raises could have anything. + tell "nothing stored" from "could not look": a machine with no keyring, or none + with a usable backend, knowably has nothing, but a backend that failed could + have anything. """ try: import keyring # type: ignore[import-untyped] @@ -703,6 +730,9 @@ def keyring_read_value(key: str, field: str) -> Tuple[bool, Optional[str]]: try: return True, keyring.get_password(_KEYRING_SERVICE, f"{key}:{field}") + except _no_keyring_errors() as e: + logger.debug(f"no system keyring available: {e}") + return True, None except Exception as e: logger.warning(f"keyring retrieval failed: {e}") return False, None @@ -730,6 +760,7 @@ def keyring_delete_values(key: str, fields: Iterable[str]) -> bool: # cannot be told from a deletion that failed. import keyring.errors # type: ignore[import-untyped] + no_keyring = _no_keyring_errors() deleted = True for field in fields: username = f"{key}:{field}" @@ -741,13 +772,21 @@ def keyring_delete_values(key: str, fields: Iterable[str]) -> bool: try: if keyring.get_password(_KEYRING_SERVICE, username) is not None: deleted = False + except no_keyring: + raise except Exception as e: logger.warning(f"keyring deletion failed: {e}") deleted = False + except no_keyring: + # Answered for every field at once, below. + raise except Exception as e: logger.warning(f"keyring deletion failed: {e}") deleted = False return deleted + except _no_keyring_errors() as e: + logger.debug(f"no system keyring available: {e}") + return True except Exception as e: logger.warning(f"keyring deletion failed: {e}") return False diff --git a/tests/test_connect_cloud.py b/tests/test_connect_cloud.py index 1422fd65..cb9e3153 100644 --- a/tests/test_connect_cloud.py +++ b/tests/test_connect_cloud.py @@ -22,6 +22,7 @@ RSConnectExecutor, ) from rsconnect.exception import DeploymentFailedException, RSConnectException +from rsconnect.http_support import HTTPResponse, HTTPServer from rsconnect.log import VERBOSE from rsconnect.main import cli from rsconnect.metadata import AppStore, ServerData, ServerStore @@ -29,6 +30,8 @@ from rsconnect.oauth import InvalidClientError, InvalidGrantError from rsconnect.validation import validate_connection_options +from .utils import failing_keyring + ENV = ParameterSource.ENVIRONMENT TYPED = ParameterSource.COMMANDLINE @@ -1028,6 +1031,67 @@ def test_invalid_grant_from_a_client_credentials_grant_leaves_the_original_401(s self.assertIn("401", exception.message) +class TestConnectCloudStreamBodyRetry(unittest.TestCase): + """The retry-once skeleton is shared with the Posit Connect client, so a streamed + body is rewound before the retry here too rather than arriving empty.""" + + def _attempt_bodies(self, body: Any, server: Optional[ConnectCloudServer] = None, read: bool = True) -> list[Any]: + """The body each attempt was given, read out when `read`, with refresh stubbed.""" + client = ConnectCloudClient(server or ConnectCloudServer("acme", access_token="stale", refresh_token="rt")) + seen: list[Any] = [] + + def fake_request( + _self: Any, + method: str, + path: str, + query_params: Any = None, + body: Any = None, + maximum_redirects: int = 5, + decode_response: bool = True, + headers: Any = None, + ) -> Any: + seen.append(body.read() if read and hasattr(body, "read") else body) + response = mock.Mock(spec=HTTPResponse) + response.status = 401 if len(seen) == 1 else 200 + return response + + with mock.patch.object(HTTPServer, "request", fake_request): + with mock.patch.object(client, "_attempt_token_refresh", return_value=True): + client.request("POST", "/contents", body=body) + return seen + + def _retry_bodies(self, body: Any) -> list[Any]: + return self._attempt_bodies(body) + + def test_a_seekable_stream_is_rewound(self): + self.assertEqual(self._retry_bodies(io.BytesIO(b"payload")), [b"payload", b"payload"]) + + def test_a_stream_is_left_alone_when_there_is_nothing_to_refresh_with(self): + # No refresh token and no service account credential: nothing can be minted, + # so the request is sent once and its body is neither buffered nor rewound. + stream = io.BytesIO(b"payload") + seen = self._attempt_bodies(stream, server=ConnectCloudServer("acme", access_token="at"), read=False) + + self.assertEqual(seen, [stream]) + + def test_a_stream_that_cannot_seek_is_read_into_memory(self): + class NonSeekableStream(io.RawIOBase): + def __init__(self, data: bytes): + self._data = data + + def read(self, size: int = -1) -> bytes: + data, self._data = self._data, b"" + return data + + def readable(self) -> bool: + return True + + def seekable(self) -> bool: + return False + + self.assertEqual(self._retry_bodies(NonSeekableStream(b"payload")), [b"payload", b"payload"]) + + class TestConnectCloudAdd(CliTestCase): """CLI-level tests for `rsconnect add -s connect.posit.cloud`.""" @@ -1058,6 +1122,29 @@ def test_add_says_where_the_credentials_went_without_a_keyring(self): self.assertEqual(result.exit_code, 0, result.output) self.assertIn("keyring not available", result.output) + def test_add_falls_back_to_the_file_when_no_keyring_backend_is_usable(self): + # A CI runner has keyring installed with nothing behind it, which is the case + # the servers.json fallback exists for; it used to abort the command instead. + self._mock_device_login() + with failing_keyring(): + result = self.runner.invoke(cli, ["add", "-n", "cloud", "--connect-cloud", "-A", "acme"]) + + self.assertEqual(result.exit_code, 0, result.output) + self.assertIn("keyring not available", result.output) + entry = self.store.get_by_name("cloud") + assert entry is not None + self.assertEqual(entry["connect_cloud_access_token"], "at") + self.assertEqual(entry["connect_cloud_refresh_token"], "rt") + + def test_a_deploy_reads_the_credentials_back_without_a_keyring_backend(self): + self._mock_device_login() + with failing_keyring(): + self.runner.invoke(cli, ["add", "-n", "cloud", "--connect-cloud", "-A", "acme"]) + data = self.store.resolve("cloud", None) + + self.assertEqual(data.connect_cloud_access_token, "at") + self.assertEqual(data.connect_cloud_refresh_token, "rt") + def test_a_stale_ca_certificate_env_var_does_not_block_add(self): # CONNECT_CA_CERTIFICATE pointing at a missing file used to fail at CLI # parsing (click Path(exists=True)), before the Cloud target was known. diff --git a/tests/test_oauth.py b/tests/test_oauth.py index 7599ba11..2f2c29cb 100644 --- a/tests/test_oauth.py +++ b/tests/test_oauth.py @@ -10,6 +10,8 @@ from rsconnect.exception import RSConnectException from rsconnect.http_support import HTTPResponse from rsconnect.metadata import ServerData + +from .utils import failing_keyring from rsconnect.oauth import ( InvalidClientError, InvalidGrantError, @@ -312,6 +314,22 @@ class _PasswordDeleteError(Exception): """Stands in for keyring.errors.PasswordDeleteError, which means "nothing stored".""" +class _NoKeyringError(Exception): + """Stands in for keyring.errors.NoKeyringError, which means "no usable backend".""" + + +def _mock_keyring_errors() -> MagicMock: + """A stand-in for the keyring.errors module. + + The classes it names have to be real exceptions, since the code under test catches + them. + """ + errors = MagicMock() + errors.PasswordDeleteError = _PasswordDeleteError + errors.NoKeyringError = _NoKeyringError + return errors + + class TestKeyringIntegration: def test_store_success(self): mock_keyring = MagicMock() @@ -344,8 +362,7 @@ def test_a_failure_partway_through_leaves_nothing_behind(self): # prefer the keyring, so a field written before the failure would shadow it. mock_keyring = MagicMock() mock_keyring.set_password.side_effect = [None, Exception("keychain locked")] - mock_keyring_errors = MagicMock() - mock_keyring_errors.PasswordDeleteError = _PasswordDeleteError + mock_keyring_errors = _mock_keyring_errors() with patch.dict("sys.modules", {"keyring": mock_keyring, "keyring.errors": mock_keyring_errors}): result = keyring_store_token("https://example.com", "at-1", "rt-1") @@ -362,8 +379,7 @@ def test_a_cleanup_that_leaves_the_value_behind_is_reported(self): mock_keyring.set_password.side_effect = [None, Exception("keychain locked")] mock_keyring.delete_password.side_effect = _PasswordDeleteError() mock_keyring.get_password.return_value = "at-1" - mock_keyring_errors = MagicMock() - mock_keyring_errors.PasswordDeleteError = _PasswordDeleteError + mock_keyring_errors = _mock_keyring_errors() with patch.dict("sys.modules", {"keyring": mock_keyring, "keyring.errors": mock_keyring_errors}): with pytest.raises(RSConnectException, match="could not be removed"): keyring_store_token("https://example.com", "at-1", "rt-1") @@ -405,8 +421,7 @@ def test_a_failure_that_cannot_be_cleaned_up_is_reported(self): mock_keyring = MagicMock() mock_keyring.set_password.side_effect = [None, Exception("keychain locked")] mock_keyring.delete_password.side_effect = Exception("keychain locked") - mock_keyring_errors = MagicMock() - mock_keyring_errors.PasswordDeleteError = _PasswordDeleteError + mock_keyring_errors = _mock_keyring_errors() with patch.dict("sys.modules", {"keyring": mock_keyring, "keyring.errors": mock_keyring_errors}): with pytest.raises(RSConnectException, match="could not be removed"): keyring_store_token("https://example.com", "at-1", "rt-1") @@ -437,6 +452,66 @@ def test_delete_success(self): assert mock_keyring.delete_password.call_count == 2 +class TestNoKeyringBackend: + """keyring installed with no usable backend is the CI-runner case the servers.json + fallback exists for. Its fail backend raises NoKeyringError, which sits beside + PasswordDeleteError under KeyringError rather than under it, so it only counts as + "no keyring" by being named.""" + + def test_storing_falls_back_to_the_file(self): + with failing_keyring(): + assert keyring_store_token("https://example.com", "at-1", "rt-1") is False + + def test_storing_does_not_try_to_clean_up_after_itself(self): + # A backend that reports itself absent stored nothing, so there is no partial + # write to remove -- and the removal would fail the same way, which used to + # turn the fallback into a hard error. + with failing_keyring(): + with patch("rsconnect.oauth.keyring_delete_values") as cleanup: + assert keyring_store_token("https://example.com", "at-1", "rt-1") is False + cleanup.assert_not_called() + + def test_reading_reports_nothing_stored_rather_than_unreadable(self): + with failing_keyring(): + assert keyring_read_value("https://example.com", "access_token") == (True, None) + + def test_deleting_has_nothing_to_delete(self): + with failing_keyring(): + assert keyring_delete_values("https://example.com", ("access_token", "refresh_token")) is True + + @patch("rsconnect.oauth.login_with_browser") + @patch("rsconnect.oauth.register_client", return_value="new-client-id") + @patch("rsconnect.oauth.discover_oauth_metadata") + def test_login_stores_its_tokens_in_the_file( + self, + mock_discover: MagicMock, + mock_register: MagicMock, + mock_login: MagicMock, + ): + import tempfile + + from click.testing import CliRunner + + from rsconnect.main import cli + from rsconnect.metadata import ServerStore + + mock_discover.return_value = FAKE_METADATA + mock_login.return_value = {"access_token": "at-1", "refresh_token": "rt-1", "expires_in": 3600} + + store = ServerStore(base_dir=tempfile.mkdtemp()) + runner = CliRunner() + with patch("rsconnect.main.server_store", store): + with failing_keyring(): + result = runner.invoke(cli, ["login", "--server", FAKE_URL, "--name", "test-server"]) + + assert result.exit_code == 0, result.output + assert "keyring not available" in result.output + entry = store.get_by_name("test-server") + assert entry is not None + assert entry["oauth_access_token"] == "at-1" + assert entry["oauth_refresh_token"] == "rt-1" + + class TestLoginWithBrowser: @patch("rsconnect.oauth.webbrowser.open", return_value=True) @patch("rsconnect.oauth._exchange_code_for_token") diff --git a/tests/utils.py b/tests/utils.py index 274c62e3..e89c4c0d 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -2,12 +2,41 @@ import os import jwt import re +from contextlib import contextmanager from os.path import join, dirname, exists from packaging import version +from unittest import mock import pytest from rsconnect.api import RSConnectServer, RSConnectClient +# Captured while this module is imported, which is before the conftest fixture hides +# keyring from tests: failing_keyring() needs the real modules back. +try: + import keyring as _keyring + import keyring.backends.fail as _keyring_fail + import keyring.errors as _keyring_errors +except ImportError: # pragma: no cover + _keyring = None + + +@contextmanager +def failing_keyring(): + """Run with keyring installed and its fail backend active. + + That is the shape of a CI runner: the package is there, no backend is usable, and + every operation raises NoKeyringError. Credentials have to land in servers.json. + """ + if _keyring is None: # pragma: no cover + pytest.skip("keyring is not installed") + previous = _keyring.get_keyring() + _keyring.set_keyring(_keyring_fail.Keyring()) + try: + with mock.patch.dict(sys.modules, {"keyring": _keyring, "keyring.errors": _keyring_errors}): + yield + finally: + _keyring.set_keyring(previous) + def apply_common_args(args: list, server=None, key=None, cacert=None, insecure=False): if server: From 9a5f49ecc21a260956562e039738c871385fe23c Mon Sep 17 00:00:00 2001 From: Sam Perman Date: Mon, 17 Aug 2026 15:14:48 -0400 Subject: [PATCH 5/9] fix: restore Python 3.8 test collection for test_connect_cloud 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. --- tests/test_connect_cloud.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_connect_cloud.py b/tests/test_connect_cloud.py index cb9e3153..7f75a11f 100644 --- a/tests/test_connect_cloud.py +++ b/tests/test_connect_cloud.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import contextlib import io import json From b9eaa4be2141526cf791394769f3b9b48552189a Mon Sep 17 00:00:00 2001 From: Sam Perman Date: Mon, 17 Aug 2026 15:31:58 -0400 Subject: [PATCH 6/9] fix: restore fixture teardown order broken by the keyring conftest 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. --- tests/conftest.py | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 9b9f11e8..a2a25c2b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,12 +4,28 @@ @pytest.fixture(autouse=True) -def no_system_keyring(monkeypatch: pytest.MonkeyPatch): +def no_system_keyring(): """Make the system keyring unavailable to every test. `keyring` is installed in the test environment (twine depends on it), so without this the credential paths would read and write the machine's real keychain. Tests that exercise keyring storage replace `sys.modules["keyring"]` with a mock of their own, or patch the helpers in `rsconnect.oauth`. + + sys.modules is restored by hand rather than through the `monkeypatch` + fixture: an autouse fixture requesting `monkeypatch` hoists the shared + instance ahead of every test-level fixture, so its undo (including any + `monkeypatch.chdir`) would run after those fixtures' cleanup. That order + deletes a still-current working directory on Windows, which fails. """ - monkeypatch.setitem(sys.modules, "keyring", None) + absent = object() + previous = sys.modules.get("keyring", absent) + sys.modules["keyring"] = None # type: ignore[assignment] + try: + yield + finally: + if sys.modules.get("keyring") is None: + if previous is absent: + del sys.modules["keyring"] + else: + sys.modules["keyring"] = previous # type: ignore[assignment] From b0d4eabe733c5eddad0ad2f62e7bac753ae2d3a2 Mon Sep 17 00:00:00 2001 From: Sam Perman Date: Mon, 17 Aug 2026 15:40:56 -0400 Subject: [PATCH 7/9] fix: tell the keyring fixture's None marker from a deleted module entry 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. --- tests/conftest.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index a2a25c2b..f2e63d27 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -24,7 +24,9 @@ def no_system_keyring(): try: yield finally: - if sys.modules.get("keyring") is None: + # The sentinel default distinguishes the fixture's own None marker from + # a key some test deleted outright, which is left as that test's doing. + if sys.modules.get("keyring", absent) is None: if previous is absent: del sys.modules["keyring"] else: From 8ee4dce70a4415e8c82674e2019045109ec4405d Mon Sep 17 00:00:00 2001 From: Sam Perman Date: Mon, 17 Aug 2026 16:10:27 -0400 Subject: [PATCH 8/9] test: cover the keyring fixture teardown branches 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. --- tests/conftest.py | 34 ++++++++++++++---------- tests/test_conftest.py | 60 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 14 deletions(-) create mode 100644 tests/test_conftest.py diff --git a/tests/conftest.py b/tests/conftest.py index f2e63d27..6822ad3e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,10 +1,28 @@ import sys +from typing import Iterator import pytest +def _no_system_keyring() -> Iterator[None]: + """Generator behind the fixture, importable so tests can drive its teardown.""" + absent = object() + previous = sys.modules.get("keyring", absent) + sys.modules["keyring"] = None # type: ignore[assignment] + try: + yield + finally: + # The sentinel default distinguishes the fixture's own None marker from + # a key some test deleted outright, which is left as that test's doing. + if sys.modules.get("keyring", absent) is None: + if previous is absent: + del sys.modules["keyring"] + else: + sys.modules["keyring"] = previous # type: ignore[assignment] + + @pytest.fixture(autouse=True) -def no_system_keyring(): +def no_system_keyring() -> Iterator[None]: """Make the system keyring unavailable to every test. `keyring` is installed in the test environment (twine depends on it), so @@ -18,16 +36,4 @@ def no_system_keyring(): `monkeypatch.chdir`) would run after those fixtures' cleanup. That order deletes a still-current working directory on Windows, which fails. """ - absent = object() - previous = sys.modules.get("keyring", absent) - sys.modules["keyring"] = None # type: ignore[assignment] - try: - yield - finally: - # The sentinel default distinguishes the fixture's own None marker from - # a key some test deleted outright, which is left as that test's doing. - if sys.modules.get("keyring", absent) is None: - if previous is absent: - del sys.modules["keyring"] - else: - sys.modules["keyring"] = previous # type: ignore[assignment] + yield from _no_system_keyring() diff --git a/tests/test_conftest.py b/tests/test_conftest.py new file mode 100644 index 00000000..1dfde874 --- /dev/null +++ b/tests/test_conftest.py @@ -0,0 +1,60 @@ +import contextlib +import sys +import types +from typing import Generator, Iterator + +import pytest + +from tests.conftest import _no_system_keyring + + +def _drain(gen: Iterator[None]) -> None: + try: + next(gen) + except StopIteration: + pass + + +@pytest.fixture +def reinstate_marker() -> Iterator[None]: + # Reinstates the autouse fixture's marker even when the test fails, so a + # failure here cannot leak a stand-in module into later tests' teardowns. + # As a test-level fixture it unwinds before the autouse fixture does. + try: + yield + finally: + sys.modules["keyring"] = None # type: ignore[assignment] + + +@pytest.fixture +def fixture_gen(reinstate_marker: None) -> Iterator["Generator[None, None, None]"]: + # closing() finalizes a generator a failed assertion left suspended, so + # its teardown runs before reinstate_marker restores the marker rather + # than at garbage collection, after. + gen = _no_system_keyring() + with contextlib.closing(gen): + yield gen + + +def test_teardown_restores_the_previous_module(fixture_gen: "Generator[None, None, None]") -> None: + stand_in = types.ModuleType("keyring") + sys.modules["keyring"] = stand_in + next(fixture_gen) + assert sys.modules["keyring"] is None + _drain(fixture_gen) + assert sys.modules["keyring"] is stand_in + + +def test_teardown_removes_the_marker_when_nothing_was_stored(fixture_gen: "Generator[None, None, None]") -> None: + del sys.modules["keyring"] + next(fixture_gen) + assert sys.modules["keyring"] is None + _drain(fixture_gen) + assert "keyring" not in sys.modules + + +def test_teardown_leaves_a_deleted_key_deleted(fixture_gen: "Generator[None, None, None]") -> None: + next(fixture_gen) + del sys.modules["keyring"] + _drain(fixture_gen) + assert "keyring" not in sys.modules From 9018786a27554607ec8c79997cce5735d051a319 Mon Sep 17 00:00:00 2001 From: Sam Perman Date: Tue, 18 Aug 2026 07:35:42 -0400 Subject: [PATCH 9/9] test: cover the unknown-nickname error from ServerStore.resolve Nothing asserted this raise; the Connect integration suite exercised it only by accident, and the -n/-A test rework there removed even that. --- tests/test_metadata.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/test_metadata.py b/tests/test_metadata.py index 3464c7e5..849c1126 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -155,6 +155,13 @@ def test_resolve_from_args(self): self.assertEqual(server_data.ca_data, None) self.assertFalse(server_data.from_store) + def test_resolve_unknown_name_raises(self): + # A -n naming nothing must fail here rather than fall through to + # argument-style resolution, which would treat the nickname as a URL. + with self.assertRaises(RSConnectException) as context: + self.server_store.resolve("no-such-server", None) + self.assertEqual(context.exception.message, 'The nickname, "no-such-server", does not exist.') + def test_save_and_load(self): temp = tempfile.mkdtemp() server_store = ServerStore(base_dir=temp)