From 2497224f9e494ed66877a8b251e31721c5fb75a2 Mon Sep 17 00:00:00 2001 From: Thomas Sibley Date: Fri, 26 Jan 2024 15:45:39 -0800 Subject: [PATCH 1/4] authn: Correct docstring of renew() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @victorlin pointed out¹ that a UserError is not raised by this function. While correcting that, I noted that other docstrings in this module use "saved credentials" as the term (matching the term we use in user-visible output) instead of "tokens", so I made this docstring follow suit. ¹ --- nextstrain/cli/authn/__init__.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/nextstrain/cli/authn/__init__.py b/nextstrain/cli/authn/__init__.py index aef7b76d..cee42425 100644 --- a/nextstrain/cli/authn/__init__.py +++ b/nextstrain/cli/authn/__init__.py @@ -100,12 +100,13 @@ def login(origin: Origin, credentials: Optional[Callable[[], Tuple[str, str]]] = def renew(origin: Origin) -> Optional[User]: """ - Renews existing tokens for *origin*, if possible. + Renews existing saved credentials for *origin*, if possible. Returns a :class:`User` object with renewed information about the logged in user when successful. - Raises a :class:`UserError` if authentication fails. + Returns ``None`` if there are no saved credentials or if they're unable to + be automatically renewed. """ assert origin From fc0780b9b77e56c86fb9449d65ca46ddea976b7f Mon Sep 17 00:00:00 2001 From: Thomas Sibley Date: Fri, 26 Jan 2024 16:11:57 -0800 Subject: [PATCH 2/4] authn.configuration: Improve error handling in openid_configuration() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @victorlin suggested¹ we improve error handling in this code path, and he's right. Catch and improve the output for the three most common errors we'll encounter: connection errors, HTTP errors, and JSON decoding errors. The original error is summarized in the error output, and by chaining the new exception from the original, the full chain in all its detail will be printed when NEXTSTRAIN_DEBUG=1 for troubleshooting. ¹ --- nextstrain/cli/authn/configuration.py | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/nextstrain/cli/authn/configuration.py b/nextstrain/cli/authn/configuration.py index cf270eb2..4fce9c4c 100644 --- a/nextstrain/cli/authn/configuration.py +++ b/nextstrain/cli/authn/configuration.py @@ -20,18 +20,31 @@ def openid_configuration(origin: Origin): assert origin with requests.Session() as http: - response = http.get(origin.rstrip("/") + "/.well-known/openid-configuration") + try: + response = http.get(origin.rstrip("/") + "/.well-known/openid-configuration") + response.raise_for_status() + return response.json() - if response.status_code == 404: + except requests.exceptions.ConnectionError as err: raise UserError(f""" - Failed to retrieve authentication metadata for {origin}. + Could not connect to {origin} to retrieve + authentication metadata: + + {type(err).__name__}: {err} + + That remote may be invalid or you may be experiencing network + connectivity issues. + """) from err + + except (requests.exceptions.HTTPError, requests.exceptions.JSONDecodeError) as err: + raise UserError(f""" + Failed to retrieve authentication metadata for {origin}: + + {type(err).__name__}: {err} That remote seems unlikely to be an alternate nextstrain.org instance or an internal Nextstrain Groups Server instance. - """) - - response.raise_for_status() - return response.json() + """) from err def client_configuration(origin: Origin): From 0d762610b545010909ab080d14da70385b065478 Mon Sep 17 00:00:00 2001 From: Thomas Sibley Date: Fri, 26 Jan 2024 16:35:58 -0800 Subject: [PATCH 3/4] authn: Short-circuit early in current_user() when there are no tokens Avoids needlessly trying to fetch authn metadata from the remote origin, which leads to nicer UX. For example, previously `nextstrain whoami bogus` resulted in a connection error re: fetching authn metadata but now it results in a more sensible "not logged in" error. --- nextstrain/cli/authn/__init__.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/nextstrain/cli/authn/__init__.py b/nextstrain/cli/authn/__init__.py index cee42425..20367146 100644 --- a/nextstrain/cli/authn/__init__.py +++ b/nextstrain/cli/authn/__init__.py @@ -184,9 +184,15 @@ def current_user(origin: Origin) -> Optional[User]: """ assert origin - session = Session(origin) tokens = _load_tokens(origin) + # Short-circuit if we don't have any tokens to speak of. Avoids trying to + # fetch authn metadata from the remote origin. + if all(token is None for token in tokens.values()): + return None + + session = Session(origin) + try: try: session.verify_tokens(**tokens) From a753a47c9c6cb9669a4f3bcbd2a0ecd9e8739fec Mon Sep 17 00:00:00 2001 From: Thomas Sibley Date: Fri, 26 Jan 2024 16:50:38 -0800 Subject: [PATCH 4/4] CHANGES: Document improved errors --- CHANGES.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 31ca336f..65060f77 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -13,6 +13,12 @@ development source code and as such may not be routinely kept up to date. # __NEXT__ +## Improvements + +* Several kinds of errors from `nextstrain login` and `nextstrain whoami` + related to their interactions with a remote server are now clearer. + ([#347](https://github.com/nextstrain/cli/pull/347)) + # 8.0.0 (18 January 2024)