From b63c96b82b3aaab53caf3792e33bf5c1810adc83 Mon Sep 17 00:00:00 2001 From: Thomas Sibley Date: Mon, 23 Oct 2023 13:55:01 -0700 Subject: [PATCH 1/8] aws.cognito: Update the refresh token when renewing tokens, if applicable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reading the OAuth 2.0 spec again¹, I noted that: The authorization server MAY issue a new refresh token, in which case the client MUST discard the old refresh token and replace it with the new refresh token. The authorization server MAY revoke the old refresh token after issuing a new refresh token to the client. I had assumed the refresh token itself was never renewed as I never observed AWS Cognito doing so in practice. I addressed this assumption for nextstrain.org², which is using OAuth 2.0, but it also got me thinking about Nextstrain CLI here, which isn't using OAuth 2.0 per se but is using a Cognito API that's very similar. Like the OAuth 2.0 spec, the documented Cognito response type definitions include the possibility of an updated refresh token.³ Perhaps Cognito even does renew them, but only as necessary and we've never happened to notice this issue! Regardless, we're planning to move to generic OAuth 2.0 authentication instead of using Cognito's API, or at least support the former in addition to the latter. This change of logic will help smooth that switch. ¹ ² ³ --- nextstrain/cli/aws/cognito/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nextstrain/cli/aws/cognito/__init__.py b/nextstrain/cli/aws/cognito/__init__.py index 94c20ef9..9cd22018 100644 --- a/nextstrain/cli/aws/cognito/__init__.py +++ b/nextstrain/cli/aws/cognito/__init__.py @@ -194,7 +194,7 @@ def renew_tokens(self, *, refresh_token): self.verify_tokens( id_token = result.get("IdToken"), access_token = result.get("AccessToken"), - refresh_token = refresh_token) + refresh_token = result.get("RefreshToken", refresh_token)) def verify_tokens(self, *, id_token, access_token, refresh_token): From de42478187fd6949433fe766c463a0d78c7ef51c Mon Sep 17 00:00:00 2001 From: Thomas Sibley Date: Mon, 13 Nov 2023 23:17:22 -0800 Subject: [PATCH 2/8] =?UTF-8?q?remote.nextstrain=5Fdot=5Forg:=20Emit=20the?= =?UTF-8?q?=20actual=20endpoint=20URL=20for=20deleted=20resources=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …like the other remote actions/commands do. Being precise about what's being acted upon is important and useful. Note that at the moment this is only not identical to the previous output when NEXTSTRAIN_DOT_ORG is set to something other than . --- nextstrain/cli/remote/nextstrain_dot_org.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/nextstrain/cli/remote/nextstrain_dot_org.py b/nextstrain/cli/remote/nextstrain_dot_org.py index f148380d..235f95d2 100644 --- a/nextstrain/cli/remote/nextstrain_dot_org.py +++ b/nextstrain/cli/remote/nextstrain_dot_org.py @@ -464,12 +464,14 @@ def delete(url: urllib.parse.ParseResult, recursively: bool = False, dry_run: bo raise UserError(f"Path {path} does not seem to exist") for resource in resources: - yield "nextstrain.org" + str(resource.path) + endpoint = api_endpoint(resource.path) + + yield endpoint if dry_run: continue - response = http.delete(api_endpoint(resource.path)) + response = http.delete(endpoint) raise_for_status(response) From f72aeb8b2666ebcf2d51310099df7bc3663e0cfd Mon Sep 17 00:00:00 2001 From: Thomas Sibley Date: Tue, 14 Nov 2023 16:21:05 -0800 Subject: [PATCH 3/8] remote.nextstrain_dot_org: Head off potential bugs in authn error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There was a narrow space for bugs here since it was reconstructing what the user associated with the request was *expected* to be, but not what it necessarily *actually* was, e.g. what was used to send the actual Authorization: Bearer … header in the request. From a given requests.Response object we can't access the auth provider directly (by design), so we have to arrange a backchannel to pass along to the error handler the actual user authn information used in the request. A simple custom attribute on the request object does the job nicely. One alternative would be a custom request header, e.g. Nextstrain-Username: trs or some such, but I like the attribute because it's more targeted in scope and doesn't send unnecessary information. Another alternative would be for the error handler to parse the token in the Authorization header—it doesn't really even need to verify the JWT, just parse it—and extract the username that way. But that's a bit more involved and the simplicity of a custom attribute is more compelling. --- nextstrain/cli/remote/nextstrain_dot_org.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/nextstrain/cli/remote/nextstrain_dot_org.py b/nextstrain/cli/remote/nextstrain_dot_org.py index 235f95d2..6cb38d85 100644 --- a/nextstrain/cli/remote/nextstrain_dot_org.py +++ b/nextstrain/cli/remote/nextstrain_dot_org.py @@ -650,6 +650,10 @@ def __init__(self): def __call__(self, request: requests.PreparedRequest) -> requests.PreparedRequest: if self.user and origin(request.url) == origin(NEXTSTRAIN_DOT_ORG): request.headers["Authorization"] = self.user.http_authorization + + # Used in error handling for more informative error messages + request._user = self.user # type: ignore + return request @@ -710,7 +714,10 @@ def raise_for_status(response: requests.Response) -> None: """, msg = indent("\n".join(wrap(msg)), " ")) from err elif status in {401, 403}: - user = current_user() + try: + user = response.request._user # type: ignore + except AttributeError: + user = None if user: challenge = authn_challenge(response) if status == 401 else None From 62d2a846ec678f40e028e96f4d377c388de49cf9 Mon Sep 17 00:00:00 2001 From: Thomas Sibley Date: Wed, 15 Nov 2023 21:48:21 -0800 Subject: [PATCH 4/8] remote.s3: Update documentation to reflect that the Groups migration is complete As of last spring! Related-to: --- nextstrain/cli/remote/s3.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/nextstrain/cli/remote/s3.py b/nextstrain/cli/remote/s3.py index 5769a644..180a46fd 100644 --- a/nextstrain/cli/remote/s3.py +++ b/nextstrain/cli/remote/s3.py @@ -7,10 +7,7 @@ Nextstrain :term:`datasets ` and :term:`narratives ` hosted on `Amazon S3 `_. This functionality is primarily intended for use by the Nextstrain team and -operators of self-hosted :term:`docs:Auspice` instances. It is also used to -manage the contents of :doc:`Nextstrain Groups -` that have not migrated to using the -:doc:`/remotes/nextstrain.org`. +operators of self-hosted :term:`docs:Auspice` instances. Remote paths From fbc58a2899fed0826f6edba66695a85873cf01eb Mon Sep 17 00:00:00 2001 From: Thomas Sibley Date: Wed, 15 Nov 2023 22:31:37 -0800 Subject: [PATCH 5/8] Avoid importing command modules by their short names into nextstrain.cli This avoids issues where nextstrain.cli.command.remote masks the nextstrain.cli.remote module when attempting to use the former, e.g. import nextstrain.cli.remote will get you nextstrain.cli.command.remote because it was imported as "remote" into nextstrain.cli. I thought maybe you could work around this, e.g. with import nextstrain.cli.remote as remote but no dice. This happens because importing x.y.z first imports x, then imports x.y, and finally x.y.z and during this each nested module is made available in its parent by design. In any case, switching to nextstrain.cli.command.all_commands nicely parallels the existing nextstrain.cli.runner.all_runners variable. --- nextstrain/cli/__init__.py | 27 ++----------------- nextstrain/cli/command/__init__.py | 43 ++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 25 deletions(-) diff --git a/nextstrain/cli/__init__.py b/nextstrain/cli/__init__.py index 5c1b110d..57ec8694 100644 --- a/nextstrain/cli/__init__.py +++ b/nextstrain/cli/__init__.py @@ -17,7 +17,7 @@ from types import SimpleNamespace from .argparse import HelpFormatter, register_commands, register_default_command -from .command import build, view, deploy, remote, shell, update, setup, check_setup, login, logout, whoami, version, init_shell, authorization, debugger +from .command import all_commands, version from .debug import DEBUGGING from .errors import NextstrainCliError, UsageError from .util import warn @@ -69,31 +69,8 @@ def make_parser(): formatter_class = HelpFormatter, ) - # Maintain these manually for now while the list is very small. If we need - # to support pluggable commands or command discovery, we can switch to - # using the "entry points" system: - # https://setuptools.readthedocs.io/en/latest/setuptools.html#dynamic-discovery-of-services-and-plugins - # - commands = [ - build, - view, - deploy, - remote, - shell, - update, - setup, - check_setup, - login, - logout, - whoami, - version, - init_shell, - authorization, - debugger, - ] - register_default_command(parser) - register_commands(parser, commands) + register_commands(parser, all_commands) register_version_alias(parser) return parser diff --git a/nextstrain/cli/command/__init__.py b/nextstrain/cli/command/__init__.py index e69de29b..6f6b0931 100644 --- a/nextstrain/cli/command/__init__.py +++ b/nextstrain/cli/command/__init__.py @@ -0,0 +1,43 @@ +from . import ( + build, + view, + deploy, + remote, + shell, + update, + setup, + check_setup, + login, + logout, + whoami, + version, + init_shell, + authorization, + debugger, +) + +# Maintain this list manually for now while its relatively static. If we need +# to support pluggable commands or command discovery, we can switch to using +# the "entry points" system: +# https://setuptools.readthedocs.io/en/latest/setuptools.html#dynamic-discovery-of-services-and-plugins +# +# The order of this list is important and intentional: it determines the order +# in various user interfaces, e.g. `nextstrain --help`. +# +all_commands = [ + build, + view, + deploy, + remote, + shell, + update, + setup, + check_setup, + login, + logout, + whoami, + version, + init_shell, + authorization, + debugger, +] From 1c6a658b4762cc98fd39c79948f66ad21d75a267 Mon Sep 17 00:00:00 2001 From: Thomas Sibley Date: Wed, 8 Nov 2023 21:17:30 -0800 Subject: [PATCH 6/8] dev: Ensure HOST and PORT are stable when generating command doc These are used in `nextstrain view --help` output, and setting them during development (e.g. for `make -C doc livehtml`) would otherwise influence the generated doc. --- devel/generate-command-doc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/devel/generate-command-doc b/devel/generate-command-doc index 34ba1083..a2fd4d80 100755 --- a/devel/generate-command-doc +++ b/devel/generate-command-doc @@ -46,6 +46,10 @@ os.environ.update({ # Ensure we detect a browser for stable `nextstrain view` output. "BROWSER": "/bin/true", + + # Ensure HOST and PORT are stable for `nextstrain view` output. + "HOST": "127.0.0.1", + "PORT": "4000", }) from nextstrain.cli import make_parser From 3261aacdd8a1c564c4db392d7388eab871d2349e Mon Sep 17 00:00:00 2001 From: Thomas Sibley Date: Sat, 18 Nov 2023 01:42:00 -0800 Subject: [PATCH 7/8] browser: New module for reasonably ergonomic web browser interaction Based on `nextstrain view`'s interaction with the standard library's webbrowser module. Arranges to launch the browser in a separate thread, as this will be good enough for many contexts. `nextstrain view` itself still arranges to launch the browser in a separate _process_, since view's main thread/process exec's into a new program shortly after launching the browser. --- nextstrain/cli/browser.py | 58 ++++++++++++++++++++++++++++++++++ nextstrain/cli/command/view.py | 18 ++--------- 2 files changed, 60 insertions(+), 16 deletions(-) create mode 100644 nextstrain/cli/browser.py diff --git a/nextstrain/cli/browser.py b/nextstrain/cli/browser.py new file mode 100644 index 00000000..dd8a8239 --- /dev/null +++ b/nextstrain/cli/browser.py @@ -0,0 +1,58 @@ +""" +Web browser interaction. + +.. envvar:: BROWSER + + A ``PATH``-like list of web browsers to try in preference order, before + falling back to a set of default browsers. May be program names, e.g. + ``firefox``, or absolute paths to specific executables, e.g. + ``/usr/bin/firefox``. +""" +import webbrowser +from threading import Thread, ThreadError +from os import environ +from .util import warn + + +# Avoid text-mode browsers +TERM = environ.pop("TERM", None) +try: + BROWSER = webbrowser.get() +except: + BROWSER = None +finally: + if TERM is not None: + environ["TERM"] = TERM + + +def open_browser(url: str, new_thread: bool = True): + """ + Opens *url* in a web browser. + + Opens in a new tab, if possible, and raises the window to the top, if + possible. + + Launches the browser from a separate thread by default so waiting on the + browser child process doesn't block the main (or calling) thread. Set + *new_thread* to False to launch from the same thread as the caller (e.g. if + you've already spawned a dedicated thread or process for the browser). + Note that some registered browsers launch in the background themselves, but + not all do, so this feature makes launch behaviour consistent across + browsers. + + Prints a warning to stderr if a browser can't be found or can't be + launched, as automatically opening a browser is considered a + nice-but-not-necessary feature. + """ + if not BROWSER: + warn(f"Couldn't open <{url}> in browser: no browser found") + return + + try: + if new_thread: + Thread(target = open_browser, args = (url, False), daemon = True).start() + else: + # new = 2 means new tab, if possible + BROWSER.open(url, new = 2, autoraise = True) + except (ThreadError, webbrowser.Error) as err: + warn(f"Couldn't open <{url}> in browser: {err!r}") diff --git a/nextstrain/cli/command/view.py b/nextstrain/cli/command/view.py index 2a5eb008..f8eb5309 100644 --- a/nextstrain/cli/command/view.py +++ b/nextstrain/cli/command/view.py @@ -48,7 +48,6 @@ from multiprocessing import Process, ProcessError import re import requests -import webbrowser from inspect import cleandoc from os import environ from pathlib import Path @@ -57,6 +56,7 @@ from typing import Iterable, NamedTuple, Tuple, Union from .. import runner from ..argparse import add_extended_help_flags, SUPPRESS, SKIP_AUTO_DEFAULT_IN_HELP +from ..browser import BROWSER, open_browser as __open_browser from ..runner import docker, ambient, conda, singularity from ..util import colored, remove_suffix, warn from ..volume import NamedVolume @@ -67,16 +67,6 @@ PORT = environ.get("PORT") or "4000" -# Avoid text-mode browsers -TERM = environ.pop("TERM", None) -try: - BROWSER = webbrowser.get() -except: - BROWSER = None -finally: - if TERM is not None: - environ["TERM"] = TERM - OPEN_DEFAULT = bool(BROWSER) @@ -454,8 +444,4 @@ def _open_browser(url: str): warn(f"Couldn't open <{url}> in browser: Auspice never started listening") return - try: - # new = 2 means new tab, if possible - BROWSER.open(url, new = 2, autoraise = True) - except webbrowser.Error as err: - warn(f"Couldn't open <{url}> in browser: {err!r}") + __open_browser(url, new_thread = False) From 32f06feca1608c3f90af17e1c28883e63243d845 Mon Sep 17 00:00:00 2001 From: Thomas Sibley Date: Tue, 21 Nov 2023 14:12:45 -0800 Subject: [PATCH 8/8] browser: Support NOBROWSER environment variable to prevent launching any browser --- nextstrain/cli/browser.py | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/nextstrain/cli/browser.py b/nextstrain/cli/browser.py index dd8a8239..3e77ffaf 100644 --- a/nextstrain/cli/browser.py +++ b/nextstrain/cli/browser.py @@ -7,6 +7,12 @@ falling back to a set of default browsers. May be program names, e.g. ``firefox``, or absolute paths to specific executables, e.g. ``/usr/bin/firefox``. + +.. envvar:: NOBROWSER + + If set to a truthy value (e.g. 1) then no web browser will be considered + available. This can be useful to prevent opening of a browser when there + are not other means of doing so. """ import webbrowser from threading import Thread, ThreadError @@ -14,15 +20,18 @@ from .util import warn -# Avoid text-mode browsers -TERM = environ.pop("TERM", None) -try: - BROWSER = webbrowser.get() -except: +if environ.get("NOBROWSER"): BROWSER = None -finally: - if TERM is not None: - environ["TERM"] = TERM +else: + # Avoid text-mode browsers + TERM = environ.pop("TERM", None) + try: + BROWSER = webbrowser.get() + except: + BROWSER = None + finally: + if TERM is not None: + environ["TERM"] = TERM def open_browser(url: str, new_thread: bool = True):