Connect Cloud auth: review follow-ups (refresh errors, credential selection, keyring, shared retry)

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 <account>` (or the
   `CONNECT_CLOUD_ACCOUNT` environment variable; a `SHINYAPPS_ACCOUNT` variable
   exported for shinyapps.io is ignored here), or `-n <nickname>` 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

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

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)

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]):

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: