diff --git a/AGENTS.md b/AGENTS.md index 52eee61b..2b31fa7c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -50,9 +50,9 @@ Run each package's suite in a **separate** `pytest` invocation, as nox does — ### Type Checking ```bash -nox -s basedpyright(bfabric) -nox -s basedpyright(bfabric_scripts) -nox -s basedpyright(bfabric_app_runner) +nox -s "basedpyright(package='bfabric')" +nox -s "basedpyright(package='bfabric_scripts')" +nox -s "basedpyright(package='bfabric_app_runner')" ``` ### Linting and formatting diff --git a/bfabric/docs/changelog.md b/bfabric/docs/changelog.md index 98a87faa..92418749 100644 --- a/bfabric/docs/changelog.md +++ b/bfabric/docs/changelog.md @@ -9,6 +9,10 @@ Minor breaking changes are still possible in `1.X.Y` but we try to announce them ## \[Unreleased\] +### Fixed + +- OAuth requests to an instance recorded with an `http://` base URL are sent over `https://` instead of failing on the server's redirect (loopback hosts excepted). `bfabric.oauth.https_base_url` exposes the upgrade. + ### Added - `upload_files` accepts `on_hash_progress`, called while each file's MD5 is computed; `bfabric-cli workunit upload` shows it as a progress bar. diff --git a/bfabric/src/bfabric/oauth/__init__.py b/bfabric/src/bfabric/oauth/__init__.py index c79b1ae3..751aece3 100644 --- a/bfabric/src/bfabric/oauth/__init__.py +++ b/bfabric/src/bfabric/oauth/__init__.py @@ -3,7 +3,7 @@ # Importing this module pulls authlib, joserfc and httpx; ``import bfabric`` alone does not. from bfabric.oauth._credential_provider import OAuthCredentialProvider from bfabric.oauth._device_code import device_code_login -from bfabric.oauth._endpoints import token_url +from bfabric.oauth._endpoints import https_base_url, token_url from bfabric.oauth._pkce import AuthorizationRequest, exchange_code, graphical_browser_available, pkce_login from bfabric.oauth._registration import ( delete_client, @@ -27,6 +27,7 @@ "device_code_login", "exchange_code", "graphical_browser_available", + "https_base_url", "pkce_login", "read_client", "register_client", diff --git a/bfabric/src/bfabric/oauth/_device_code.py b/bfabric/src/bfabric/oauth/_device_code.py index 398393fd..59d5ce89 100644 --- a/bfabric/src/bfabric/oauth/_device_code.py +++ b/bfabric/src/bfabric/oauth/_device_code.py @@ -10,7 +10,7 @@ from loguru import logger from bfabric.errors import BfabricOAuthError, raise_if_unavailable -from bfabric.oauth._endpoints import token_url +from bfabric.oauth._endpoints import https_base_url, token_url if TYPE_CHECKING: from bfabric.config.base_url import BaseUrl @@ -31,7 +31,7 @@ def _request_device_code( logger.debug("Requesting device code from {}", base_url) with raise_if_unavailable(base_url): response = httpx.post( - f"{base_url}/rest/oauth/device_authorization", + f"{https_base_url(base_url)}/rest/oauth/device_authorization", data={ "client_id": client_id, "scope": scope, diff --git a/bfabric/src/bfabric/oauth/_endpoints.py b/bfabric/src/bfabric/oauth/_endpoints.py index 55a3e2b9..351d7ad5 100644 --- a/bfabric/src/bfabric/oauth/_endpoints.py +++ b/bfabric/src/bfabric/oauth/_endpoints.py @@ -6,11 +6,27 @@ from __future__ import annotations -from urllib.parse import urlencode +from urllib.parse import urlencode, urlsplit, urlunsplit from bfabric.config.base_url import BaseUrl +_LOOPBACK_HOSTS = frozenset({"localhost", "127.0.0.1", "::1"}) + + +def https_base_url(base_url: str) -> str: + """*base_url* with ``http`` upgraded to ``https``, except for a loopback host (local development). + + OAuth requests carry codes, secrets and refresh tokens, so they must not go out in the clear. A + config that still records ``http://`` would otherwise send the first request unencrypted and then + fail, because the server answers with a redirect that ``httpx`` does not follow. + """ + parts = urlsplit(base_url) + if parts.scheme != "http" or parts.hostname in _LOOPBACK_HOSTS: + return base_url + return urlunsplit(parts._replace(scheme="https")) + + def authorize_url( base_url: str, *, @@ -36,9 +52,9 @@ def authorize_url( "scope": scope, } ) - return f"{BaseUrl(base_url)}/rest/oauth/authorize?{query}" + return f"{https_base_url(BaseUrl(base_url))}/rest/oauth/authorize?{query}" def token_url(base_url: str) -> str: """The token endpoint that issues, exchanges and refreshes tokens.""" - return f"{BaseUrl(base_url)}/rest/oauth/token" + return f"{https_base_url(BaseUrl(base_url))}/rest/oauth/token" diff --git a/bfabric/src/bfabric/oauth/_registration.py b/bfabric/src/bfabric/oauth/_registration.py index 2820100c..dcd88793 100644 --- a/bfabric/src/bfabric/oauth/_registration.py +++ b/bfabric/src/bfabric/oauth/_registration.py @@ -8,6 +8,7 @@ from loguru import logger from bfabric.errors import raise_if_unavailable +from bfabric.oauth._endpoints import https_base_url if TYPE_CHECKING: from bfabric.bfabric import Bfabric @@ -60,7 +61,7 @@ def register_client( :param grant_types: Explicit list of grant types to request (overrides the default) :returns: Registration response containing ``client_id``, ``client_secret``, etc. """ - url = f"{base_url}/rest/oauth/register" + url = f"{https_base_url(base_url)}/rest/oauth/register" resolved_grant_types = grant_types if grant_types is not None else _default_grant_types(service_user) body: dict[str, object] = { "client_name": client_name, diff --git a/bfabric/src/bfabric/oauth/_token_exchange.py b/bfabric/src/bfabric/oauth/_token_exchange.py index 710899c5..e4688580 100644 --- a/bfabric/src/bfabric/oauth/_token_exchange.py +++ b/bfabric/src/bfabric/oauth/_token_exchange.py @@ -15,7 +15,7 @@ from bfabric.errors import raise_if_unavailable -from bfabric.oauth._endpoints import token_url +from bfabric.oauth._endpoints import https_base_url, token_url from bfabric.oauth._url_token import UrlTokenContext if TYPE_CHECKING: @@ -66,7 +66,7 @@ def introspect_token( :raises httpx.HTTPStatusError: On non-2xx responses """ - url = f"{base_url}/rest/oauth/introspect" + url = f"{https_base_url(base_url)}/rest/oauth/introspect" logger.debug("Introspecting token at {}", url) with raise_if_unavailable(base_url): response = httpx.post( diff --git a/bfabric/src/bfabric/oauth/_url_token.py b/bfabric/src/bfabric/oauth/_url_token.py index a4d49d80..56c953f5 100644 --- a/bfabric/src/bfabric/oauth/_url_token.py +++ b/bfabric/src/bfabric/oauth/_url_token.py @@ -13,6 +13,7 @@ from bfabric.config.base_url import BaseUrl from bfabric.errors import raise_if_unavailable +from bfabric.oauth._endpoints import https_base_url from pydantic import BaseModel, ConfigDict, Field @@ -73,7 +74,7 @@ def _fetch_jwks(base_url: BaseUrl) -> dict[str, object]: return jwks logger.debug("Fetching JWKS from {}", base_url) - url = f"{base_url}/rest/oauth/jwks" + url = f"{https_base_url(base_url)}/rest/oauth/jwks" with raise_if_unavailable(base_url): response = httpx.get(url, timeout=30) _ = response.raise_for_status() diff --git a/bfabric_scripts/docs/changelog.md b/bfabric_scripts/docs/changelog.md index 7ca769d6..65d49c09 100644 --- a/bfabric_scripts/docs/changelog.md +++ b/bfabric_scripts/docs/changelog.md @@ -1,5 +1,9 @@ ## \[Unreleased\] +### Fixed + +- `bfabric-cli login` and `auth device-code` no longer fail on an environment recorded with an `http://` base URL; they use `https://` and save the corrected URL. + ### Changed - `bfabric-cli login` switches to the device-code flow over SSH, without a display, or with only a text browser, unless `--no-browser` is given. `bfabric-cli workunit upload` shows a progress bar while hashing, including an overall total and ETA, hashes 4 files at once (`--hash-workers`), and reuses checksums of unchanged files from earlier runs (`--no-hash-cache` to disable). `--chunk-size` (MiB) sets the upload chunk size; it defaults to 32 instead of 4, which is faster on fast links. diff --git a/bfabric_scripts/src/bfabric_scripts/cli/login/_common.py b/bfabric_scripts/src/bfabric_scripts/cli/login/_common.py index 8c84692a..00d8ee79 100644 --- a/bfabric_scripts/src/bfabric_scripts/cli/login/_common.py +++ b/bfabric_scripts/src/bfabric_scripts/cli/login/_common.py @@ -18,6 +18,7 @@ from bfabric_scripts.cli.interactive import confirm, is_interactive, select_choice, select_or_input, text_input from bfabric_scripts.cli.login._constants import SCOPE_PRESETS, SCOPE_PRESETS_BY_NAME from bfabric.config import BaseUrl +from bfabric.oauth import https_base_url from bfabric_scripts.cli.login._urls import KNOWN_INSTANCES, normalize_base_url if TYPE_CHECKING: @@ -69,7 +70,21 @@ def _pick_or_type(message: str, labels: dict[str, str], prompt: str) -> str | No def resolve_base_url(base_url: str | None, env: EnvironmentConfig | None) -> BaseUrl | None: - """Resolve the instance URL: explicit, else the environment's recorded one, else a picker.""" + """Resolve the instance URL: explicit, else the environment's recorded one, else a picker. + + A plain ``http`` URL is upgraded to ``https`` (loopback excepted), so a login never sends its OAuth + requests unencrypted and the environment is saved with the corrected URL. + """ + resolved = _resolve_raw_base_url(base_url, env) + if resolved is None: + return None + secure = BaseUrl(https_base_url(resolved)) + if secure != resolved: + print(f"Using {secure} instead of {resolved}: OAuth requires https.", file=sys.stderr) + return secure + + +def _resolve_raw_base_url(base_url: str | None, env: EnvironmentConfig | None) -> BaseUrl | None: if base_url is not None: return normalize_base_url(base_url) if env is not None: diff --git a/tests/bfabric/oauth/test_device_code.py b/tests/bfabric/oauth/test_device_code.py index cd21b207..a520566b 100644 --- a/tests/bfabric/oauth/test_device_code.py +++ b/tests/bfabric/oauth/test_device_code.py @@ -41,6 +41,14 @@ def test_posts_correct_params(self, mocker): ) assert result == mock_response.json.return_value + def test_upgrades_a_plain_http_base_url(self, mocker): + """A recorded ``http://`` URL would otherwise be sent in the clear, then fail on the server's redirect.""" + mock_post = mocker.patch("bfabric.oauth._device_code.httpx.post") + + _request_device_code("http://example.com/bfabric", client_id="c", scope="api:read") + + assert mock_post.call_args.args[0] == "https://example.com/bfabric/rest/oauth/device_authorization" + def test_raises_on_http_error(self, mocker): mock_response = mocker.MagicMock() mock_response.raise_for_status.side_effect = httpx.HTTPStatusError( diff --git a/tests/bfabric/oauth/test_endpoints.py b/tests/bfabric/oauth/test_endpoints.py index 2508d8f9..ee54f9b0 100644 --- a/tests/bfabric/oauth/test_endpoints.py +++ b/tests/bfabric/oauth/test_endpoints.py @@ -3,7 +3,8 @@ import pytest from bfabric.config.base_url import BaseUrl -from bfabric.oauth import token_url +from bfabric.oauth import https_base_url, token_url +from bfabric.oauth._endpoints import authorize_url class TestTokenUrl: @@ -17,3 +18,34 @@ def test_validates_the_base_url(self): # Only that the validation happens -- what BaseUrl accepts is tested where BaseUrl lives. with pytest.raises(ValueError): token_url("https://example.com/notbfabric") + + +class TestHttpsBaseUrl: + def test_upgrades_http(self): + assert https_base_url("http://fgcz-bfabric.uzh.ch/bfabric") == "https://fgcz-bfabric.uzh.ch/bfabric" + + def test_keeps_the_port(self): + assert https_base_url("http://example.com:8080/bfabric") == "https://example.com:8080/bfabric" + + def test_leaves_https_alone(self): + assert https_base_url("https://example.com/bfabric") == "https://example.com/bfabric" + + @pytest.mark.parametrize( + "url", ["http://localhost:8080/bfabric", "http://127.0.0.1:8000/bfabric", "http://[::1]:8000/bfabric"] + ) + def test_leaves_loopback_alone_for_local_development(self, url): + assert https_base_url(url) == url + + def test_token_url_uses_https(self): + assert token_url("http://example.com/bfabric") == "https://example.com/bfabric/rest/oauth/token" + + def test_authorize_url_uses_https(self): + url = authorize_url( + "http://example.com/bfabric", + client_id="c", + redirect_uri="http://127.0.0.1:1/callback", + code_challenge="x", + state="s", + scope="api:read", + ) + assert url.startswith("https://example.com/bfabric/rest/oauth/authorize?") diff --git a/tests/bfabric_scripts/cli/login/test_login_common.py b/tests/bfabric_scripts/cli/login/test_login_common.py index e6f4ba3f..3d53a4f8 100644 --- a/tests/bfabric_scripts/cli/login/test_login_common.py +++ b/tests/bfabric_scripts/cli/login/test_login_common.py @@ -186,6 +186,17 @@ def test_falls_back_to_the_recorded_base_url(self): env = EnvironmentConfig.model_validate({"base_url": "https://recorded.example.com/bfabric"}) assert resolve_base_url(None, env) == "https://recorded.example.com/bfabric" + def test_recorded_http_url_is_upgraded_to_https(self, capsys): + env = EnvironmentConfig.model_validate({"base_url": "http://recorded.example.com/bfabric"}) + assert resolve_base_url(None, env) == "https://recorded.example.com/bfabric" + assert "https" in capsys.readouterr().err + + def test_explicit_http_url_is_upgraded_to_https(self): + assert resolve_base_url("http://typed.example.com/bfabric", None) == "https://typed.example.com/bfabric" + + def test_explicit_loopback_http_url_is_kept(self): + assert resolve_base_url("http://localhost:8000/bfabric", None) == "http://localhost:8000/bfabric" + def test_explicit_value_wins_over_the_recorded_one(self): env = EnvironmentConfig.model_validate({"base_url": "https://recorded.example.com/bfabric"}) assert resolve_base_url("https://typed.example.com/bfabric", env) == "https://typed.example.com/bfabric"