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/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 6d4d7ba0..07c3f98f 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 @@ -59,6 +60,7 @@ from .environment import fake_module_file_from_directory from .exception import DeploymentFailedException, RSConnectException from .http_support import ( + BearerTokenHTTPServer, CookieJar, HTTPResponse, HTTPServer, @@ -500,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 @@ -532,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 ( @@ -571,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: @@ -593,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) @@ -629,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) @@ -1576,12 +1547,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) @@ -1652,11 +1623,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 \ @@ -2899,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. @@ -2925,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. @@ -2947,10 +2916,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 +2934,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 +2967,92 @@ 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 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. + """ + 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 + + 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 + # 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, + 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=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: response = self.get("/users/me") 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/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/main.py b/rsconnect/main.py index c9faa975..9a57d221 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): @@ -933,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) @@ -1037,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") @@ -1160,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 c54e1869..f08783dc 100644 --- a/rsconnect/metadata.py +++ b/rsconnect/metadata.py @@ -370,28 +370,37 @@ 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 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. 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 +417,55 @@ 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 _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): @@ -546,9 +565,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: @@ -573,6 +593,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. @@ -590,8 +617,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 +647,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 +665,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,13 +673,14 @@ 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: 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"], @@ -673,9 +699,9 @@ def resolve(self, name: Optional[str], url: Optional[str], account_name: Optiona 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 bf03b8c7..3ce8bc60 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 @@ -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", @@ -622,60 +639,175 @@ 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 _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. + + 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] + except ImportError: + return False - 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 + 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 ImportError: + 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 + # 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, or none + with a usable backend, knowably has nothing, but a backend that failed 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 _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 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"): + no_keyring = _no_keyring_errors() + 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 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 + + +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/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/conftest.py b/tests/conftest.py new file mode 100644 index 00000000..6822ad3e --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,39 @@ +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() -> Iterator[None]: + """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. + """ + 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 diff --git a/tests/test_connect_cloud.py b/tests/test_connect_cloud.py index b2f459ea..7f75a11f 100644 --- a/tests/test_connect_cloud.py +++ b/tests/test_connect_cloud.py @@ -1,7 +1,10 @@ +from __future__ import annotations + import contextlib import io import json import os +import sys import tarfile import tempfile import unittest @@ -21,12 +24,16 @@ 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 from rsconnect.models import AppModes +from rsconnect.oauth import InvalidClientError, InvalidGrantError from rsconnect.validation import validate_connection_options +from .utils import failing_keyring + ENV = ParameterSource.ENVIRONMENT TYPED = ParameterSource.COMMANDLINE @@ -447,6 +454,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.""" @@ -720,12 +758,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 +907,192 @@ 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 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`.""" @@ -875,6 +1117,36 @@ 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_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. @@ -1260,12 +1532,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 @@ -2457,8 +2786,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()) @@ -2466,10 +2795,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) @@ -2520,7 +2849,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) @@ -2665,9 +2994,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) @@ -2689,6 +3018,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") @@ -2708,35 +3045,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 @@ -2752,18 +3096,258 @@ 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") +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_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) diff --git a/tests/test_oauth.py b/tests/test_oauth.py index 7bfc2e22..2f2c29cb 100644 --- a/tests/test_oauth.py +++ b/tests/test_oauth.py @@ -10,15 +10,20 @@ 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, _exchange_code_for_token, _poll_for_device_token, discover_oauth_metadata, 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, @@ -290,6 +295,40 @@ 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 _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): @@ -318,6 +357,92 @@ 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 = _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") + + 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 = _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") + + 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 = _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") + + 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() @@ -327,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: