From 419b4a52ac88888274c94bbd83c37ec7e4ed3c8f Mon Sep 17 00:00:00 2001 From: Leonardo Schwarz Date: Thu, 13 Aug 2026 09:42:19 +0200 Subject: [PATCH 1/3] fix(bfabric): canonicalise base_url without a trailing slash `_validate_base_url` appended a trailing slash that essentially nothing wanted: 16 call sites stripped it right back off to build a URL, and the two SOAP engines forgot to, emitting `.../bfabric//workunit?wsdl` on every `Bfabric.connect()`. The CLI's `normalize_base_url` had already settled on the slash-free form, which is also what gets persisted to `~/.bfabricpy.yml`. Flip the canonical form and canonicalise once at each public boundary -- the `connect_*` classmethods and `WebappClient.create` now build the `BfabricClientConfig` first and read `base_url` back from it -- so the downstream strips become dead rather than merely relocated. Going through the validator is also stronger than `rstrip`: it lowercases the host, drops a default port, and rejects a non-HTTP URL up front. `entities/core/uri.py` had to flip alongside. It hardcoded the instance component as `.../bfabric/`, and `EntityReader` compares that for string equality against `config.base_url` at three sites, so changing only the config would have made every `read_uris`/`query` raise "Unsupported B-Fabric instance". `EntityUri` strings are unaffected -- `as_uri()` appends its own slash before `urljoin`. Two strips survive deliberately. `api_to_rest_url` is exported from `bfabric.transfer` and its strip is load-bearing for the `.../api/` case, where `endswith("/api")` is False without it. The one in `compute_token_cache_path` is gone instead: every caller passes a canonical value and the resulting key is byte-identical to today's, so no cached token is orphaned -- a test now pins exactly that. The trailing slash was originally introduced in #341 so that `urljoin(base_url, ...)` in `Entity.web_url` would not drop the `/bfabric` path segment. That call site has since been replaced by the `EntityUri` machinery, and both remaining `urljoin` callers append their own slash, so the reason no longer applies. Closes #576 --- .../docs/api_reference/entity_uri/index.md | 6 +-- .../docs/api_reference/token_data/index.md | 4 +- bfabric/docs/changelog.md | 4 ++ bfabric/docs/getting_started/configuration.md | 11 +++-- .../docs/getting_started/troubleshooting.md | 6 +-- .../creating_a_client/server_webapp_usage.md | 28 +++++------ .../user_guides/reading_data/entity_api.md | 2 +- bfabric/src/bfabric/_oauth/device_code.py | 1 - bfabric/src/bfabric/_oauth/pkce.py | 1 - bfabric/src/bfabric/_oauth/registration.py | 6 +-- bfabric/src/bfabric/_oauth/token_cache.py | 5 +- bfabric/src/bfabric/_oauth/token_exchange.py | 4 +- bfabric/src/bfabric/_oauth/url_token.py | 2 +- bfabric/src/bfabric/_oauth/webapp_client.py | 4 +- bfabric/src/bfabric/bfabric.py | 17 +++---- .../bfabric/config/bfabric_client_config.py | 8 +-- bfabric/src/bfabric/entities/core/uri.py | 4 +- bfabric_asgi_auth/README.md | 2 +- bfabric_asgi_auth/docs/changelog.md | 1 + bfabric_asgi_auth/tests/unit/test_user.py | 2 +- bfabric_scripts/docs/changelog.md | 1 + bfabric_scripts/example/test_webapp_flow.py | 2 +- .../src/bfabric_scripts/cli/login/register.py | 5 +- .../config/test_bfabric_client_config.py | 21 +++++--- tests/bfabric/config/test_config_data.py | 6 +-- tests/bfabric/config/test_config_file.py | 20 ++++---- tests/bfabric/config/test_config_writer.py | 4 +- tests/bfabric/conftest.py | 2 +- tests/bfabric/entities/core/test_entity.py | 4 +- .../entities/core/test_entity_reader.py | 26 +++++----- tests/bfabric/entities/core/test_has_many.py | 2 +- .../bfabric/entities/core/test_references.py | 4 +- tests/bfabric/entities/core/test_uri.py | 24 ++++----- tests/bfabric/entities/test_dataset.py | 2 +- tests/bfabric/oauth/test_device_code.py | 25 ---------- tests/bfabric/oauth/test_registration.py | 4 +- tests/bfabric/oauth/test_token_cache.py | 12 +++-- tests/bfabric/oauth/test_token_exchange.py | 27 ---------- tests/bfabric/oauth/test_url_token.py | 2 +- .../operations/dataset/test_operations.py | 2 +- .../operations/workunit/test_create.py | 2 +- tests/bfabric/test_bfabric.py | 49 ++++++++++++------- .../test_resolve_bfabric_annotation_specs.py | 2 +- .../cli/login/test_cmd_auth_pat.py | 2 +- .../cli/workunit/test_cmd_workunit_diff.py | 2 +- .../feeder/test_path_convention_compms.py | 2 +- 46 files changed, 177 insertions(+), 195 deletions(-) diff --git a/bfabric/docs/api_reference/entity_uri/index.md b/bfabric/docs/api_reference/entity_uri/index.md index 9b8b8e465..1da5f9a49 100644 --- a/bfabric/docs/api_reference/entity_uri/index.md +++ b/bfabric/docs/api_reference/entity_uri/index.md @@ -59,7 +59,7 @@ from bfabric.entities.core.uri import EntityUri uri = EntityUri("https://fgcz-bfabric.uzh.ch/bfabric/sample/show.html?id=123") # Access components -print(uri.components.bfabric_instance) # "https://fgcz-bfabric.uzh.ch/bfabric/" +print(uri.components.bfabric_instance) # "https://fgcz-bfabric.uzh.ch/bfabric" print(uri.components.entity_type) # "sample" print(uri.components.entity_id) # 123 ``` @@ -94,7 +94,7 @@ from bfabric.entities.core.uri import EntityUri # Build URI from parts uri = EntityUri.from_components( - bfabric_instance="https://fgcz-bfabric.uzh.ch/bfabric/", + bfabric_instance="https://fgcz-bfabric.uzh.ch/bfabric", entity_type="sample", entity_id=123, ) @@ -133,7 +133,7 @@ entities = reader.read_uris(uris) | Component | Description | Example | |-----------|-------------|---------| -| `bfabric_instance` | Base URL of B-Fabric instance | `https://fgcz-bfabric.uzh.ch/bfabric/` | +| `bfabric_instance` | Base URL of B-Fabric instance | `https://fgcz-bfabric.uzh.ch/bfabric` | | `entity_type` | Entity name (lowercase) | `sample`, `project`, `workunit` | | `entity_id` | Numeric entity ID | `123` | diff --git a/bfabric/docs/api_reference/token_data/index.md b/bfabric/docs/api_reference/token_data/index.md index 4ce60b7c7..11e1bb26d 100644 --- a/bfabric/docs/api_reference/token_data/index.md +++ b/bfabric/docs/api_reference/token_data/index.md @@ -36,8 +36,8 @@ from bfabric import Bfabric from bfabric.experimental.webapp_integration_settings import TokenValidationSettings settings = TokenValidationSettings( - validation_bfabric_instance="https://fgcz-bfabric.uzh.ch/bfabric/", - supported_bfabric_instances=["https://fgcz-bfabric.uzh.ch/bfabric/"], + validation_bfabric_instance="https://fgcz-bfabric.uzh.ch/bfabric", + supported_bfabric_instances=["https://fgcz-bfabric.uzh.ch/bfabric"], ) client, token_data = Bfabric.connect_token(token=token, settings=settings) diff --git a/bfabric/docs/changelog.md b/bfabric/docs/changelog.md index 6ffefd4c5..adbaba19c 100644 --- a/bfabric/docs/changelog.md +++ b/bfabric/docs/changelog.md @@ -27,9 +27,13 @@ Minor breaking changes are still possible in `1.X.Y` but we try to announce them - PKCE's printed-URL fallback and timeout error now name the loopback redirect target and point at the device-code flow. - `use_client` logs the reported error's traceback at DEBUG; the `Error: ` line and exit code 1 are unchanged. - `create_workunit` accepts a plain mapping for `params`, validated internally so an invalid mapping raises `ValidationError` before any write. +- **Breaking: `BfabricClientConfig.base_url` is canonicalised *without* a trailing slash**, reversing the 1.15.0 "always ends with exactly one `/`". Config files and `connect_*` arguments still accept one. +- `Entity.bfabric_instance` and `EntityUri.components.bfabric_instance` follow the same form; `EntityUri` strings are unchanged. +- `connect_oauth` / `connect_pkce` / `connect_device_code` / `connect_pat` and `WebappClient.create` canonicalise `base_url` through `BfabricClientConfig`, so a host with mixed case or a default port is normalised too, and a non-HTTP URL is rejected up front. ### Fixed +- The SUDS and Zeep WSDL URLs no longer contain a doubled slash (`…/bfabric//workunit?wsdl`), and neither do the `show.html` links printed by `bfabric_read` and `bfabric-cli api read`. - `ResultContainer.assert_success` raises `BfabricRequestError` instead of a bare `RuntimeError`; it remains a `RuntimeError` subclass, so existing `except RuntimeError` handlers keep working. - The "could not find the config file" and "empty list provided for deletion" diagnostics go through loguru at WARNING instead of `print()`, so they honour the configured log level and sink. - `setup_script_logging` no longer skips setup in subprocesses, which fell back to loguru's verbose DEBUG default; its repeat guard is now process-local. diff --git a/bfabric/docs/getting_started/configuration.md b/bfabric/docs/getting_started/configuration.md index 7d12836dc..efa49874a 100644 --- a/bfabric/docs/getting_started/configuration.md +++ b/bfabric/docs/getting_started/configuration.md @@ -15,14 +15,17 @@ GENERAL: PRODUCTION: login: yourBfabricLogin password: yourBfabricWebServicePassword # Get from B-Fabric profile - base_url: https://fgcz-bfabric.uzh.ch/bfabric/ + base_url: https://fgcz-bfabric.uzh.ch/bfabric TEST: login: yourBfabricLogin password: yourBfabricWebServicePassword - base_url: https://fgcz-bfabric-test.uzh.ch/bfabric/ + base_url: https://fgcz-bfabric-test.uzh.ch/bfabric ``` +A trailing slash on `base_url` is accepted and dropped, so `client.config.base_url` always reads back +without one. + ### Web Service Password The password in your config file is **NOT** your login password. Find your web service password: @@ -80,7 +83,7 @@ python script.py # Will use TEST environment Complete configuration override (highest priority). Used primarily for integration tests, where it needs to be prevented that the regular config file leads to the wrong B-Fabric instance being modified. ```bash -export BFABRICPY_CONFIG_OVERRIDE='{"client": {"base_url": "https://fgcz-bfabric.uzh.ch/bfabric/"}, "auth": {"login": "myuser", "password": "mypass"}}' +export BFABRICPY_CONFIG_OVERRIDE='{"client": {"base_url": "https://fgcz-bfabric.uzh.ch/bfabric"}, "auth": {"login": "myuser", "password": "mypass"}}' python script.py # Uses this config, ignoring ~/.bfabricpy.yml ``` @@ -103,7 +106,7 @@ from bfabric.config import BfabricAuth, BfabricClientConfig # Create config programmatically client_config = BfabricClientConfig( - base_url="https://fgcz-bfabric.uzh.ch/bfabric/", + base_url="https://fgcz-bfabric.uzh.ch/bfabric", engine="SUDS", # default; "ZEEP" is also available but requires `pip install bfabric[zeep]` ) diff --git a/bfabric/docs/getting_started/troubleshooting.md b/bfabric/docs/getting_started/troubleshooting.md index 78be61289..d61f7662b 100644 --- a/bfabric/docs/getting_started/troubleshooting.md +++ b/bfabric/docs/getting_started/troubleshooting.md @@ -54,7 +54,7 @@ GENERAL: PRODUCTION: login: yourBfabricLogin password: yourBfabricWebServicePassword - base_url: https://fgcz-bfabric.uzh.ch/bfabric/ + base_url: https://fgcz-bfabric.uzh.ch/bfabric ``` ## Query Issues @@ -148,8 +148,8 @@ print(f"User: {client.auth.login}") Common URLs: -- **Production**: `https://fgcz-bfabric.uzh.ch/bfabric/` -- **Test**: `https://fgcz-bfabric-test.uzh.ch/bfabric/` +- **Production**: `https://fgcz-bfabric.uzh.ch/bfabric` +- **Test**: `https://fgcz-bfabric-test.uzh.ch/bfabric` If you're connecting to the wrong instance, update your config file or use a different environment: diff --git a/bfabric/docs/user_guides/creating_a_client/server_webapp_usage.md b/bfabric/docs/user_guides/creating_a_client/server_webapp_usage.md index aca2ed9c0..b7dcaa240 100644 --- a/bfabric/docs/user_guides/creating_a_client/server_webapp_usage.md +++ b/bfabric/docs/user_guides/creating_a_client/server_webapp_usage.md @@ -29,10 +29,10 @@ from bfabric.experimental.webapp_integration_settings import TokenValidationSett # Configure which B-Fabric instances are allowed settings = TokenValidationSettings( - validation_bfabric_instance="https://fgcz-bfabric.uzh.ch/bfabric/", + validation_bfabric_instance="https://fgcz-bfabric.uzh.ch/bfabric", supported_bfabric_instances=[ - "https://fgcz-bfabric.uzh.ch/bfabric/", - "https://fgcz-bfabric-test.uzh.ch/bfabric/", + "https://fgcz-bfabric.uzh.ch/bfabric", + "https://fgcz-bfabric-test.uzh.ch/bfabric", ], ) @@ -61,12 +61,12 @@ from bfabric import Bfabric from bfabric.rest.token_data import get_token_data # Validate token first -base_url = "https://fgcz-bfabric.uzh.ch/bfabric/" +base_url = "https://fgcz-bfabric.uzh.ch/bfabric" token = "your_token_here" token_data = get_token_data(base_url=base_url, token=token) # Check if the token is from an allowed instance -allowed_instances = ["https://fgcz-bfabric.uzh.ch/bfabric/"] +allowed_instances = ["https://fgcz-bfabric.uzh.ch/bfabric"] if token_data.caller not in allowed_instances: raise ValueError(f"Token from {token_data.caller} is not allowed") @@ -84,10 +84,10 @@ The `TokenValidationSettings` class configures which B-Fabric instances are allo from bfabric.experimental.webapp_integration_settings import TokenValidationSettings settings = TokenValidationSettings( - validation_bfabric_instance="https://fgcz-bfabric.uzh.ch/bfabric/", + validation_bfabric_instance="https://fgcz-bfabric.uzh.ch/bfabric", supported_bfabric_instances=[ - "https://fgcz-bfabric.uzh.ch/bfabric/", - "https://fgcz-bfabric-test.uzh.ch/bfabric/", + "https://fgcz-bfabric.uzh.ch/bfabric", + "https://fgcz-bfabric-test.uzh.ch/bfabric", ], ) ``` @@ -110,10 +110,10 @@ from bfabric.experimental.webapp_integration_settings import WebappIntegrationSe from bfabric.config import BfabricAuth settings = WebappIntegrationSettings( - validation_bfabric_instance="https://fgcz-bfabric.uzh.ch/bfabric/", - supported_bfabric_instances=["https://fgcz-bfabric.uzh.ch/bfabric/"], + validation_bfabric_instance="https://fgcz-bfabric.uzh.ch/bfabric", + supported_bfabric_instances=["https://fgcz-bfabric.uzh.ch/bfabric"], feeder_user_credentials={ - "https://fgcz-bfabric.uzh.ch/bfabric/": BfabricAuth( + "https://fgcz-bfabric.uzh.ch/bfabric": BfabricAuth( login="feeder_user", password="feeder_user_password" ), }, @@ -137,10 +137,10 @@ Always restrict `supported_bfabric_instances` to only the instances you trust: ```python settings = TokenValidationSettings( - validation_bfabric_instance="https://fgcz-bfabric.uzh.ch/bfabric/", + validation_bfabric_instance="https://fgcz-bfabric.uzh.ch/bfabric", supported_bfabric_instances=[ - "https://fgcz-bfabric.uzh.ch/bfabric/", # Only allow production - # "https://fgcz-bfabric-test.uzh.ch/bfabric/", # Commented out to prevent test tokens + "https://fgcz-bfabric.uzh.ch/bfabric", # Only allow production + # "https://fgcz-bfabric-test.uzh.ch/bfabric", # Commented out to prevent test tokens ], ) ``` diff --git a/bfabric/docs/user_guides/reading_data/entity_api.md b/bfabric/docs/user_guides/reading_data/entity_api.md index ebd0e598b..ff122a318 100644 --- a/bfabric/docs/user_guides/reading_data/entity_api.md +++ b/bfabric/docs/user_guides/reading_data/entity_api.md @@ -78,7 +78,7 @@ sample = reader.read_id(entity_type="sample", entity_id=123) # Entity identifier print(sample.id) # 123 print(sample.classname) # "sample" -print(sample.bfabric_instance) # "https://fgcz-bfabric.uzh.ch/bfabric/" +print(sample.bfabric_instance) # "https://fgcz-bfabric.uzh.ch/bfabric" # Entity URI uri = sample.uri diff --git a/bfabric/src/bfabric/_oauth/device_code.py b/bfabric/src/bfabric/_oauth/device_code.py index 711323742..528e47548 100644 --- a/bfabric/src/bfabric/_oauth/device_code.py +++ b/bfabric/src/bfabric/_oauth/device_code.py @@ -154,7 +154,6 @@ def device_code_login( :returns: Token dict with ``access_token``, ``refresh_token``, etc. :raises RuntimeError: On timeout, expired token, or access denied """ - base_url = base_url.rstrip("/") logger.debug("Starting device code flow for {}", base_url) device_response = _request_device_code(base_url, client_id=client_id, scope=scope) diff --git a/bfabric/src/bfabric/_oauth/pkce.py b/bfabric/src/bfabric/_oauth/pkce.py index 6b6f5a4a2..08cded710 100644 --- a/bfabric/src/bfabric/_oauth/pkce.py +++ b/bfabric/src/bfabric/_oauth/pkce.py @@ -183,7 +183,6 @@ def pkce_login( :returns: Token dict with ``access_token``, ``refresh_token``, etc. :raises RuntimeError: On timeout, CSRF state mismatch, or authorization error """ - base_url = base_url.rstrip("/") logger.debug("Starting PKCE login flow for {}", base_url) verifier = _generate_verifier() challenge = _generate_challenge(verifier) diff --git a/bfabric/src/bfabric/_oauth/registration.py b/bfabric/src/bfabric/_oauth/registration.py index c44412ce1..b6b161e01 100644 --- a/bfabric/src/bfabric/_oauth/registration.py +++ b/bfabric/src/bfabric/_oauth/registration.py @@ -58,7 +58,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.rstrip('/')}/rest/oauth/register" + url = f"{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, @@ -115,10 +115,8 @@ def register_webapp( :returns: Dict with ``"oauth"`` (registration response) and ``"application"`` (save response) keys """ - base_url = client.config.base_url.rstrip("/") - oauth_result = register_client( - base_url=base_url, + base_url=client.config.base_url, token=token, client_name=app_name, redirect_uri=web_url, diff --git a/bfabric/src/bfabric/_oauth/token_cache.py b/bfabric/src/bfabric/_oauth/token_cache.py index 4fc8e0686..761632139 100644 --- a/bfabric/src/bfabric/_oauth/token_cache.py +++ b/bfabric/src/bfabric/_oauth/token_cache.py @@ -16,8 +16,11 @@ def compute_token_cache_path(base_url: str, client_id: str, env_name: str) -> Pa The path is ``~/.bfabric/tokens/{hash}.json`` where *hash* is the first 16 hex characters of the SHA-256 digest of ``base_url + '\\0' + client_id + '\\0' + env_name``. This ensures different identities on the same server get separate caches. + + *base_url* must be canonical (as produced by ``BfabricClientConfig``), or the key will not match + the cache the CLI wrote. """ - key = base_url.rstrip("/") + "\0" + client_id + "\0" + env_name + key = base_url + "\0" + client_id + "\0" + env_name url_hash = hashlib.sha256(key.encode()).hexdigest()[:16] return Path("~/.bfabric/tokens") / f"{url_hash}.json" diff --git a/bfabric/src/bfabric/_oauth/token_exchange.py b/bfabric/src/bfabric/_oauth/token_exchange.py index a7a5b94fd..9a21d558a 100644 --- a/bfabric/src/bfabric/_oauth/token_exchange.py +++ b/bfabric/src/bfabric/_oauth/token_exchange.py @@ -32,7 +32,7 @@ def exchange_token( :returns: Token response dict with ``access_token``, ``refresh_token``, etc. :raises httpx.HTTPStatusError: On non-2xx responses """ - url = f"{base_url.rstrip('/')}/rest/oauth/token" + url = f"{base_url}/rest/oauth/token" logger.debug("Exchanging launch token at {}", url) response = httpx.post( url, @@ -70,7 +70,7 @@ def introspect_token( :returns: :class:`UrlTokenContext` with entity claims :raises httpx.HTTPStatusError: On non-2xx responses """ - url = f"{base_url.rstrip('/')}/rest/oauth/introspect" + url = f"{base_url}/rest/oauth/introspect" logger.debug("Introspecting token at {}", url) response = httpx.post( url, diff --git a/bfabric/src/bfabric/_oauth/url_token.py b/bfabric/src/bfabric/_oauth/url_token.py index 0a12c9576..591d35403 100644 --- a/bfabric/src/bfabric/_oauth/url_token.py +++ b/bfabric/src/bfabric/_oauth/url_token.py @@ -66,7 +66,7 @@ def _fetch_jwks(base_url: str) -> dict[str, object]: return jwks logger.debug("Fetching JWKS from {}", base_url) - url = f"{base_url.rstrip('/')}/rest/oauth/jwks" + url = f"{base_url}/rest/oauth/jwks" response = httpx.get(url, timeout=30) _ = response.raise_for_status() jwks: dict[str, object] = response.json() # pyright: ignore[reportAny] diff --git a/bfabric/src/bfabric/_oauth/webapp_client.py b/bfabric/src/bfabric/_oauth/webapp_client.py index 7cf35a553..d5cc32781 100644 --- a/bfabric/src/bfabric/_oauth/webapp_client.py +++ b/bfabric/src/bfabric/_oauth/webapp_client.py @@ -58,7 +58,8 @@ def create( from bfabric.config import BfabricClientConfig from bfabric.config.config_data import ConfigData - base_url = base_url.rstrip("/") + config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] + base_url = config.base_url token_url = f"{base_url}/rest/oauth/token" # 1. Exchange the short-lived launch token for access + refresh tokens @@ -83,7 +84,6 @@ def create( grant_type="refresh_token", token_cache_path=user_token_cache_path, ) - config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] user_client = Bfabric( config_data=ConfigData(client=config, auth=None), _credential_provider=user_provider, diff --git a/bfabric/src/bfabric/bfabric.py b/bfabric/src/bfabric/bfabric.py index 2e1cc6c08..c341de7d2 100644 --- a/bfabric/src/bfabric/bfabric.py +++ b/bfabric/src/bfabric/bfabric.py @@ -126,7 +126,7 @@ def _connect_oauth_from_config(cls, config_data: ConfigData) -> Bfabric: from bfabric._oauth.credential_provider import OAuthCredentialProvider from bfabric._oauth.token_cache import TokenCache, compute_token_cache_path - base_url = config_data.client.base_url.rstrip("/") + base_url = config_data.client.base_url if not config_data.client_id: raise ValueError( "OAuth config is missing 'client_id'. Set it in the config environment " @@ -195,7 +195,7 @@ def connect_webapp( cls, token: str, *, - validation_instance_url: str = "https://fgcz-bfabric.uzh.ch/bfabric/", + validation_instance_url: str = "https://fgcz-bfabric.uzh.ch/bfabric", config_file_path: None = None, config_file_env: None = None, ) -> tuple[Bfabric, TokenData]: @@ -259,7 +259,8 @@ def connect_oauth( """ from bfabric._oauth.credential_provider import OAuthCredentialProvider - base_url = base_url.rstrip("/") + config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] + base_url = config.base_url token_url = f"{base_url}/rest/oauth/token" provider = OAuthCredentialProvider( client_id=client_id, @@ -269,7 +270,6 @@ def connect_oauth( grant_type="client_credentials", token_cache_path=token_cache_path, ) - config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] config_data = ConfigData(client=config, auth=None) return cls(config_data=config_data, _credential_provider=provider) @@ -302,7 +302,8 @@ def connect_pkce( from bfabric._oauth.credential_provider import OAuthCredentialProvider from bfabric._oauth.pkce import pkce_login - base_url = base_url.rstrip("/") + config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] + base_url = config.base_url token = pkce_login( base_url, client_id=client_id, @@ -321,7 +322,6 @@ def connect_pkce( scope=scope, token_cache_path=token_cache_path, ) - config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] config_data = ConfigData(client=config, auth=None) return cls(config_data=config_data, _credential_provider=provider) @@ -354,7 +354,8 @@ def connect_device_code( from bfabric._oauth.credential_provider import OAuthCredentialProvider from bfabric._oauth.device_code import device_code_login - base_url = base_url.rstrip("/") + config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] + base_url = config.base_url token = device_code_login( base_url, client_id=client_id, @@ -371,7 +372,6 @@ def connect_device_code( scope=scope, token_cache_path=token_cache_path, ) - config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] config_data = ConfigData(client=config, auth=None) return cls(config_data=config_data, _credential_provider=provider) @@ -395,7 +395,6 @@ def connect_pat( from bfabric.config.bfabric_auth import OAUTH_LOGIN - base_url = base_url.rstrip("/") pat_value: str = pat.get_secret_value() if isinstance(pat, SecretStr) else pat auth = BfabricAuth(login=OAUTH_LOGIN, password=SecretStr(pat_value)) config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] diff --git a/bfabric/src/bfabric/config/bfabric_client_config.py b/bfabric/src/bfabric/config/bfabric_client_config.py index 2491259f7..999382cfb 100644 --- a/bfabric/src/bfabric/config/bfabric_client_config.py +++ b/bfabric/src/bfabric/config/bfabric_client_config.py @@ -7,10 +7,12 @@ def _validate_base_url(value: str) -> str: - """Validates that the base URL is indeed a valid HTTP URL and ensures it ends with a slash.""" - value = value.rstrip("/") + "/" + """Validates that the base URL is indeed a valid HTTP URL, canonicalised without a trailing slash. + + The strip has to come after validation: ``AnyHttpUrl`` re-adds the slash for a URL with an empty path. + """ http_url = TypeAdapter(AnyHttpUrl).validate_python(value) - return str(http_url) + return str(http_url).rstrip("/") _ValidatedBaseUrl = Annotated[str, AfterValidator(_validate_base_url)] diff --git a/bfabric/src/bfabric/entities/core/uri.py b/bfabric/src/bfabric/entities/core/uri.py index d6f1cfded..1315cd426 100644 --- a/bfabric/src/bfabric/entities/core/uri.py +++ b/bfabric/src/bfabric/entities/core/uri.py @@ -60,7 +60,9 @@ def invalid(reason: str) -> ValueError: raise invalid(f"expected query exactly 'id=' and no fragment; {_NORMALIZE_HINT}") return EntityUriComponents( - bfabric_instance=HttpUrl(f"{parsed.scheme}://{parsed.netloc.lower()}/bfabric/"), + # Slash-free, matching the canonical form of BfabricClientConfig.base_url that EntityReader + # compares this against. + bfabric_instance=HttpUrl(f"{parsed.scheme}://{parsed.netloc.lower()}/bfabric"), entity_type=segments[1], entity_id=int(entity_id), ) diff --git a/bfabric_asgi_auth/README.md b/bfabric_asgi_auth/README.md index eededca05..5eb7815b6 100644 --- a/bfabric_asgi_auth/README.md +++ b/bfabric_asgi_auth/README.md @@ -25,7 +25,7 @@ token_validator = create_mock_validator() # Accepts tokens starting with 'valid from bfabric_asgi_auth import create_bfabric_validator token_validator = create_bfabric_validator( - validation_instance_url="https://fgcz-bfabric-test.uzh.ch/bfabric/" + validation_instance_url="https://fgcz-bfabric-test.uzh.ch/bfabric" ) ``` diff --git a/bfabric_asgi_auth/docs/changelog.md b/bfabric_asgi_auth/docs/changelog.md index cb172f858..ae74d08de 100644 --- a/bfabric_asgi_auth/docs/changelog.md +++ b/bfabric_asgi_auth/docs/changelog.md @@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - Require `starlette>=1.0.1,<2` (was `>=0.50.0,<1`) to keep starlette above the advisory floor ([#505](https://github.com/fgcz/bfabricPy/issues/505)) - Improved secret key example in README to use `secrets.token_urlsafe(64)` with a warning when generating random keys ([#434](https://github.com/fgcz/bfabricPy/issues/434)) - Fixed redirect URL scheme handling: protocol-relative URLs (//example.com) and absolute URLs with wrong scheme are now corrected based on X-Forwarded-Proto headers +- `BfabricUser.get_bfabric_client().config.base_url` no longer ends with a trailing slash, following the core canonicalisation. The `bfabric_instance` in the session is unchanged. - Honor `scope["root_path"]` when the app is mounted behind a reverse-proxy sub-path: landing/logout matching strips a leading `root_path` from `scope["path"]`, and root-relative redirect targets (e.g. `authenticated_path="/"`) are prefixed with `root_path` so the browser lands at the correct public URL ## \[0.0.1\] - 2026-01-06 diff --git a/bfabric_asgi_auth/tests/unit/test_user.py b/bfabric_asgi_auth/tests/unit/test_user.py index 2b5111721..76fc1d4f9 100644 --- a/bfabric_asgi_auth/tests/unit/test_user.py +++ b/bfabric_asgi_auth/tests/unit/test_user.py @@ -60,4 +60,4 @@ def test_get_bfabric_client(self, user: BfabricUser) -> None: assert isinstance(client, Bfabric) assert client.auth.login == "testuser" assert client.auth.password.get_secret_value() == "a" * 32 - assert str(client.config.base_url) == "https://fgcz-bfabric.uzh.ch/bfabric/" + assert str(client.config.base_url) == "https://fgcz-bfabric.uzh.ch/bfabric" diff --git a/bfabric_scripts/docs/changelog.md b/bfabric_scripts/docs/changelog.md index 346fb5b3f..a1e43bb49 100644 --- a/bfabric_scripts/docs/changelog.md +++ b/bfabric_scripts/docs/changelog.md @@ -33,6 +33,7 @@ Versioning currently follows `X.Y.Z` semantic versioning, independent of the `bf - `api inspect` reports that it requires the SUDS engine instead of failing with an `AttributeError` when the configured engine is Zeep. - `auth register-webapp` prints `Error: ...` and exits 1 when the OAuth session cannot be refreshed, instead of a raw traceback. - `auth register` no longer prompts for an *Employee Bearer token* when a login exists; it authenticates as the environment in effect. +- `auth register` canonicalises its `base_url` argument like the other `auth` commands, so a bare host or a trailing slash works and a non-HTTP URL is rejected with a plain message. ## \[1.16.0\] - 2026-08-03 diff --git a/bfabric_scripts/example/test_webapp_flow.py b/bfabric_scripts/example/test_webapp_flow.py index 0465acc6d..a0fabee07 100644 --- a/bfabric_scripts/example/test_webapp_flow.py +++ b/bfabric_scripts/example/test_webapp_flow.py @@ -42,7 +42,7 @@ def main() -> None: # Connect to B-Fabric print("Connecting to B-Fabric...") client = Bfabric.connect(config_file_env=config_env) - base_url = client.config.base_url.rstrip("/") + base_url = client.config.base_url bearer_token = client.auth.password.get_secret_value() print(f" base_url: {base_url}") diff --git a/bfabric_scripts/src/bfabric_scripts/cli/login/register.py b/bfabric_scripts/src/bfabric_scripts/cli/login/register.py index 6debaedfc..303d127b9 100644 --- a/bfabric_scripts/src/bfabric_scripts/cli/login/register.py +++ b/bfabric_scripts/src/bfabric_scripts/cli/login/register.py @@ -12,6 +12,7 @@ from bfabric._oauth.registration import register_client from bfabric.config import DEFAULT_CONFIG_FILE from bfabric_scripts.cli.login._constants import DEFAULT_REGISTRATION_SCOPE +from bfabric_scripts.cli.login._urls import normalize_base_url def _resolve_token_from_config(config_env: str | None, config_file: Path) -> tuple[str, str]: @@ -27,7 +28,7 @@ def _resolve_token_from_config(config_env: str | None, config_file: Path) -> tup try: client = Bfabric.connect(config_file_path=config_file, config_file_env=config_env or "default") - return client.auth.password.get_secret_value(), str(client.config.base_url).rstrip("/") + return client.auth.password.get_secret_value(), str(client.config.base_url) except Exception as e: print(f"Error: {e}", file=sys.stderr) raise SystemExit(1) from None @@ -94,7 +95,7 @@ def cmd_login_register( try: result = register_client( - base_url=resolved_base_url, + base_url=normalize_base_url(resolved_base_url), token=resolved_token, client_name=client_name, redirect_uri=redirect_uri, diff --git a/tests/bfabric/config/test_bfabric_client_config.py b/tests/bfabric/config/test_bfabric_client_config.py index cd8e43d7e..b9fc404f1 100644 --- a/tests/bfabric/config/test_bfabric_client_config.py +++ b/tests/bfabric/config/test_bfabric_client_config.py @@ -48,7 +48,12 @@ class TestBaseUrl: ) def test_normalizes_slash(self, base_url): config = BfabricClientConfig(base_url=base_url) - assert config.base_url == "https://example.com/bfabric/" + assert config.base_url == "https://example.com/bfabric" + + def test_normalizes_host_only_url(self): + # AnyHttpUrl re-adds the slash for an empty path, so the strip has to happen after validation. + config = BfabricClientConfig(base_url="https://example.com") + assert config.base_url == "https://example.com" def test_bfabric_config_copy_with_overrides(mock_config: BfabricClientConfig) -> None: @@ -56,9 +61,9 @@ def test_bfabric_config_copy_with_overrides(mock_config: BfabricClientConfig) -> base_url="https://example.com/new-url", application_ids={"new": 2}, ) - assert new_config.base_url == "https://example.com/new-url/" + assert new_config.base_url == "https://example.com/new-url" assert new_config.application_ids == {"new": 2} - assert mock_config.base_url == "https://example.com/" + assert mock_config.base_url == "https://example.com" assert mock_config.application_ids == {"app": 1} @@ -66,9 +71,9 @@ def test_bfabric_config_copy_with_replaced_when_none( mock_config: BfabricClientConfig, ) -> None: new_config = mock_config.copy_with(base_url=None, application_ids=None) - assert new_config.base_url == "https://example.com/" + assert new_config.base_url == "https://example.com" assert new_config.application_ids == {"app": 1} - assert mock_config.base_url == "https://example.com/" + assert mock_config.base_url == "https://example.com" assert mock_config.application_ids == {"app": 1} @@ -86,7 +91,7 @@ def test_bfabric_config_read_yml_bypath_default(mocker: MockerFixture, example_c config, auth = read_config_file(example_config_path) assert auth.login == "testuser" assert auth.password.get_secret_value() == "01234567890123456789012345678901" - assert config.base_url == "https://prod-server.example.com/api/" + assert config.base_url == "https://prod-server.example.com/api" logot.assert_logged( logged.debug(f"Reading configuration from: {str(example_config_path.absolute())} config_env=None") @@ -103,7 +108,7 @@ def test_bfabric_config_read_yml_bypath_environment_variable( config, auth = read_config_file(example_config_path) assert auth.login == "testuser" assert auth.password.get_secret_value() == "012345678901234567890123456789ff" - assert config.base_url == "https://test-server.example.com/api/" + assert config.base_url == "https://test-server.example.com/api" logot.assert_logged( logged.debug(f"Reading configuration from: {str(example_config_path.absolute())} config_env=None") @@ -115,7 +120,7 @@ def test_bfabric_config_read_yml_bypath_environment_variable( def test_repr(mock_config: BfabricClientConfig, variant) -> None: rep = variant(mock_config) assert rep == ( - "BfabricClientConfig(base_url='https://example.com/', application_ids={'app': 1}, job_notification_emails=''," + "BfabricClientConfig(base_url='https://example.com', application_ids={'app': 1}, job_notification_emails=''," " engine=BfabricAPIEngineType.SUDS)" ) diff --git a/tests/bfabric/config/test_config_data.py b/tests/bfabric/config/test_config_data.py index e623ba722..488519ee7 100644 --- a/tests/bfabric/config/test_config_data.py +++ b/tests/bfabric/config/test_config_data.py @@ -54,7 +54,7 @@ def test_from_file_when_default(self, mocker, example_config_path, include_auth) loaded_config = load_config_data( config_file_path=example_config_path, config_file_env="default", include_auth=include_auth ) - assert loaded_config.client.base_url == "https://prod-server.example.com/api/" + assert loaded_config.client.base_url == "https://prod-server.example.com/api" if include_auth: assert loaded_config.auth.login == "testuser" else: @@ -68,7 +68,7 @@ def test_from_file_when_default_and_env(self, mocker, example_config_path, inclu loaded_config = load_config_data( config_file_path=example_config_path, config_file_env="default", include_auth=include_auth ) - assert loaded_config.client.base_url == "https://test-server.example.com/api/" + assert loaded_config.client.base_url == "https://test-server.example.com/api" if include_auth: assert loaded_config.auth.login == "testuser" else: @@ -82,7 +82,7 @@ def test_from_file_when_default_and_override(self, mocker, example_config_path, loaded_config = load_config_data( config_file_path=example_config_path, config_file_env="TEST", include_auth=include_auth ) - assert loaded_config.client.base_url == "https://test-server.example.com/api/" + assert loaded_config.client.base_url == "https://test-server.example.com/api" if include_auth: assert loaded_config.auth.login == "testuser" else: diff --git a/tests/bfabric/config/test_config_file.py b/tests/bfabric/config/test_config_file.py index e599918c5..f4d3f38de 100644 --- a/tests/bfabric/config/test_config_file.py +++ b/tests/bfabric/config/test_config_file.py @@ -62,14 +62,14 @@ def test_general_config(data_with_auth): def test_environment_config_when_auth(data_with_auth): config = EnvironmentConfig.model_validate(data_with_auth["PRODUCTION"]) - assert config.config.base_url == "https://example.com/" + assert config.config.base_url == "https://example.com" assert config.auth.login == "test-dummy" assert config.auth.password.get_secret_value() == "00000000001111111111222222222233" def test_environment_config_when_no_auth(data_no_auth): config = EnvironmentConfig.model_validate(data_no_auth["PRODUCTION"]) - assert config.config.base_url == "https://example.com/" + assert config.config.base_url == "https://example.com" assert config.auth is None @@ -82,7 +82,7 @@ def test_environment_config_when_pat(): "pat": "short-pat-token", } ) - assert config.config.base_url == "https://example.com/" + assert config.config.base_url == "https://example.com" assert config.auth_method == "pat" assert config.auth.login == OAUTH_LOGIN assert config.auth.password.get_secret_value() == "short-pat-token" @@ -105,7 +105,7 @@ def test_config_file_when_auth(data_with_auth): config = ConfigFile.model_validate(data_with_auth) assert config.general.default_config == "PRODUCTION" assert len(config.environments) == 1 - assert config.environments["PRODUCTION"].config.base_url == "https://example.com/" + assert config.environments["PRODUCTION"].config.base_url == "https://example.com" assert config.environments["PRODUCTION"].auth.login == "test-dummy" assert config.environments["PRODUCTION"].auth.password.get_secret_value() == "00000000001111111111222222222233" @@ -114,7 +114,7 @@ def test_config_file_when_no_auth(data_no_auth): config = ConfigFile.model_validate(data_no_auth) assert config.general.default_config == "PRODUCTION" assert len(config.environments) == 1 - assert config.environments["PRODUCTION"].config.base_url == "https://example.com/" + assert config.environments["PRODUCTION"].config.base_url == "https://example.com" assert config.environments["PRODUCTION"].auth is None @@ -122,9 +122,9 @@ def test_config_file_when_multiple(data_multiple): config = ConfigFile.model_validate(data_multiple) assert config.general.default_config == "PRODUCTION" assert len(config.environments) == 2 - assert config.environments["PRODUCTION"].config.base_url == "https://example.com/" + assert config.environments["PRODUCTION"].config.base_url == "https://example.com" assert config.environments["PRODUCTION"].auth is None - assert config.environments["TEST"].config.base_url == "https://test.example.com/" + assert config.environments["TEST"].config.base_url == "https://test.example.com" assert config.environments["TEST"].auth.login == "test-dummy" assert config.environments["TEST"].auth.password.get_secret_value() == "00000000001111111111222222222233" @@ -181,7 +181,7 @@ def config(config_data): @staticmethod def test_validate(config): assert config.general.default_config is None - assert config.environments["PRODUCTION"].config.base_url == "https://example.com/" + assert config.environments["PRODUCTION"].config.base_url == "https://example.com" @staticmethod def test_get_selected_config_env(config): @@ -216,14 +216,14 @@ def test_bypath_all_fields(self, example_config_path: Path) -> None: assert auth.login == "testuser" assert auth.password.get_secret_value() == "012345678901234567890123456789ff" - assert config.base_url == "https://test-server.example.com/api/" + assert config.base_url == "https://test-server.example.com/api" assert config.application_ids == applications_dict_ground_truth assert config.job_notification_emails == job_notification_emails_ground_truth def test_when_empty_optional(self, example_config_path: Path, logot: Logot) -> None: config, auth = read_config_file(example_config_path, config_env="STANDBY") assert auth is None - assert config.base_url == "https://standby-server.example.com/api/" + assert config.base_url == "https://standby-server.example.com/api" assert config.application_ids == {} assert config.job_notification_emails == "" logot.assert_logged( diff --git a/tests/bfabric/config/test_config_writer.py b/tests/bfabric/config/test_config_writer.py index e28775b23..9d8cb776b 100644 --- a/tests/bfabric/config/test_config_writer.py +++ b/tests/bfabric/config/test_config_writer.py @@ -191,7 +191,7 @@ def test_pat_env_round_trips(self, tmp_path): assert env.auth is not None assert env.auth.login == OAUTH_LOGIN assert env.auth.password.get_secret_value() == "secret-pat" - assert env.config.base_url == "https://example.com/" + assert env.config.base_url == "https://example.com" def test_oauth_env_round_trips(self, tmp_path): config_path = tmp_path / "config.yml" @@ -206,7 +206,7 @@ def test_oauth_env_round_trips(self, tmp_path): assert env.auth is None assert env.auth_method == "oauth" assert env.client_id == "cid" - assert env.config.base_url == "https://example.com/" + assert env.config.base_url == "https://example.com" def test_rejects_unparseable_env(self, tmp_path): # base_url is required by BfabricClientConfig; without it the written file would fail to diff --git a/tests/bfabric/conftest.py b/tests/bfabric/conftest.py index f456dc414..3d8171104 100644 --- a/tests/bfabric/conftest.py +++ b/tests/bfabric/conftest.py @@ -9,4 +9,4 @@ def pytest_runtest_setup() -> None: @pytest.fixture def bfabric_instance() -> str: - return "https://bfabric.example.org/bfabric/" + return "https://bfabric.example.org/bfabric" diff --git a/tests/bfabric/entities/core/test_entity.py b/tests/bfabric/entities/core/test_entity.py index 0bda242da..41198e0f2 100644 --- a/tests/bfabric/entities/core/test_entity.py +++ b/tests/bfabric/entities/core/test_entity.py @@ -36,7 +36,7 @@ def test_classname(mock_entity) -> None: @pytest.mark.parametrize("mock_entity_has_client", [True, False], indirect=True) def test_uri(mock_entity, bfabric_instance) -> None: - assert mock_entity.uri == f"{bfabric_instance}testendpoint/show.html?id=1" + assert mock_entity.uri == f"{bfabric_instance}/testendpoint/show.html?id=1" def test_data_dict(mock_entity, mock_data_dict) -> None: @@ -225,7 +225,7 @@ def test_repr(mock_entity) -> None: assert repr(mock_entity) == ( "Entity(" "data_dict={'id': 1, 'name': 'Test Entity', 'classname': 'testendpoint'}, " - "bfabric_instance='https://bfabric.example.org/bfabric/'" + "bfabric_instance='https://bfabric.example.org/bfabric'" ")" ) diff --git a/tests/bfabric/entities/core/test_entity_reader.py b/tests/bfabric/entities/core/test_entity_reader.py index 6b5a53ca5..dd3fa09c1 100644 --- a/tests/bfabric/entities/core/test_entity_reader.py +++ b/tests/bfabric/entities/core/test_entity_reader.py @@ -35,22 +35,22 @@ def mock_instantiate_entity(mocker): @pytest.fixture def uri_project_1(bfabric_instance): - return EntityUri(f"{bfabric_instance}project/show.html?id=100") + return EntityUri(f"{bfabric_instance}/project/show.html?id=100") @pytest.fixture def uri_project_2(bfabric_instance): - return EntityUri(f"{bfabric_instance}project/show.html?id=200") + return EntityUri(f"{bfabric_instance}/project/show.html?id=200") @pytest.fixture def uri_user_1(bfabric_instance): - return EntityUri(f"{bfabric_instance}user/show.html?id=1") + return EntityUri(f"{bfabric_instance}/user/show.html?id=1") @pytest.fixture def uri_user_2(bfabric_instance): - return EntityUri(f"{bfabric_instance}user/show.html?id=2") + return EntityUri(f"{bfabric_instance}/user/show.html?id=2") @pytest.fixture @@ -262,7 +262,7 @@ def test_unsupported_instance_raises_error( entity_reader.read_uris([uri_project_1, uri_wrong_instance]) assert "Unsupported B-Fabric instances" in str(exc_info.value) - assert "https://other-instance.example.org/bfabric/" in str(exc_info.value) + assert "https://other-instance.example.org/bfabric" in str(exc_info.value) class TestReadId: @@ -307,7 +307,7 @@ def test_with_custom_bfabric_instance( mock_entity_project_1, ): """Test reading with a custom bfabric_instance.""" - custom_instance = "https://bfabric.example.org/bfabric/" + custom_instance = "https://bfabric.example.org/bfabric" mock_cache_stack.item_get_all.return_value = {} mock_multi_query.read_multi.return_value = [{"id": 100, "classname": "project", "name": "Project 1"}] mock_instantiate_entity.return_value = mock_entity_project_1 @@ -391,7 +391,7 @@ def test_some_missing_entities( result = entity_reader.read_ids(entity_type="project", entity_ids=[100, 999]) - expected_uri_999 = EntityUri(f"{bfabric_instance}project/show.html?id=999") + expected_uri_999 = EntityUri(f"{bfabric_instance}/project/show.html?id=999") assert result == {uri_project_1: mock_entity_project_1, expected_uri_999: None} def test_all_missing_entities(self, entity_reader, mock_cache_stack, mock_multi_query, bfabric_instance): @@ -401,8 +401,8 @@ def test_all_missing_entities(self, entity_reader, mock_cache_stack, mock_multi_ result = entity_reader.read_ids(entity_type="project", entity_ids=[999, 888]) - expected_uri_999 = EntityUri(f"{bfabric_instance}project/show.html?id=999") - expected_uri_888 = EntityUri(f"{bfabric_instance}project/show.html?id=888") + expected_uri_999 = EntityUri(f"{bfabric_instance}/project/show.html?id=999") + expected_uri_888 = EntityUri(f"{bfabric_instance}/project/show.html?id=888") assert result == {expected_uri_999: None, expected_uri_888: None} def test_empty_list(self, entity_reader, mock_cache_stack): @@ -423,7 +423,7 @@ def test_with_custom_bfabric_instance( bfabric_instance, ): """Test reading with a custom bfabric_instance.""" - custom_instance = "https://bfabric.example.org/bfabric/" + custom_instance = "https://bfabric.example.org/bfabric" mock_cache_stack.item_get_all.return_value = {} mock_multi_query.read_multi.return_value = [{"id": 100, "classname": "project", "name": "Project 1"}] mock_instantiate_entity.return_value = mock_entity_project_1 @@ -431,7 +431,7 @@ def test_with_custom_bfabric_instance( result = entity_reader.read_ids(entity_type="project", entity_ids=[100], bfabric_instance=custom_instance) # Verify URI was constructed with custom instance - expected_uri = EntityUri(f"{custom_instance}project/show.html?id=100") + expected_uri = EntityUri(f"{custom_instance}/project/show.html?id=100") assert expected_uri in result @@ -481,7 +481,7 @@ def test_empty_results(self, entity_reader, mock_cache_stack, mock_client, bfabr def test_instance_mismatch_raises_error(self, entity_reader, mock_client, bfabric_instance): """Test that querying a different instance raises ValueError.""" - different_instance = "https://other-instance.example.org/bfabric/" + different_instance = "https://other-instance.example.org/bfabric" with pytest.raises(ValueError) as exc_info: entity_reader.query(entity_type="project", obj={}, bfabric_instance=different_instance, max_results=100) @@ -621,7 +621,7 @@ class TestEntityResult: @pytest.fixture def uri(self, bfabric_instance): def _make(entity_id: int, entity_type: str = "resource") -> EntityUri: - return EntityUri(f"{bfabric_instance}{entity_type}/show.html?id={entity_id}") + return EntityUri(f"{bfabric_instance}/{entity_type}/show.html?id={entity_id}") return _make diff --git a/tests/bfabric/entities/core/test_has_many.py b/tests/bfabric/entities/core/test_has_many.py index 8dc7d7d4d..2d0936731 100644 --- a/tests/bfabric/entities/core/test_has_many.py +++ b/tests/bfabric/entities/core/test_has_many.py @@ -75,7 +75,7 @@ def test_iter(entity): def test_repr(entity): assert ( repr(entity) - == "MockEntity(data_dict={'classname': 'mock', 'id': 1000, 'many': [{'id': 10, 'classname': 'testreferenced', 'name': 'Referenced Entity 10'}, {'id': 20, 'classname': 'testreferenced', 'name': 'Referenced Entity 20'}]}, bfabric_instance='https://bfabric.example.org/bfabric/')" + == "MockEntity(data_dict={'classname': 'mock', 'id': 1000, 'many': [{'id': 10, 'classname': 'testreferenced', 'name': 'Referenced Entity 10'}, {'id': 20, 'classname': 'testreferenced', 'name': 'Referenced Entity 20'}]}, bfabric_instance='https://bfabric.example.org/bfabric')" ) @staticmethod diff --git a/tests/bfabric/entities/core/test_references.py b/tests/bfabric/entities/core/test_references.py index 474750915..0c60da2a3 100644 --- a/tests/bfabric/entities/core/test_references.py +++ b/tests/bfabric/entities/core/test_references.py @@ -52,7 +52,7 @@ def entity_reader(mocker, entity_reader_constructor): @pytest.fixture def mock_project(mocker, bfabric_instance): project = mocker.MagicMock(name="mock_project") - project.uri = f"{bfabric_instance}project/show.html?id=3000" + project.uri = f"{bfabric_instance}/project/show.html?id=3000" project.data_dict = {"id": 3000, "classname": "project", "name": "Mock Project"} return project @@ -60,7 +60,7 @@ def mock_project(mocker, bfabric_instance): @pytest.fixture def mock_user(mocker, bfabric_instance): user = mocker.MagicMock(name="mock_user") - user.uri = f"{bfabric_instance}user/show.html?id=1" + user.uri = f"{bfabric_instance}/user/show.html?id=1" user.data_dict = {"id": 1, "classname": "user", "name": "Test User", "email": "test@bfabric.example.org"} return user diff --git a/tests/bfabric/entities/core/test_uri.py b/tests/bfabric/entities/core/test_uri.py index fb927d957..8a05ffc67 100644 --- a/tests/bfabric/entities/core/test_uri.py +++ b/tests/bfabric/entities/core/test_uri.py @@ -43,12 +43,12 @@ def test_components_property(self): uri = "https://fgcz-bfabric.uzh.ch/bfabric/project/show.html?id=3000" entity_uri = EntityUri(uri) components = entity_uri.components - assert components.bfabric_instance == HttpUrl("https://fgcz-bfabric.uzh.ch/bfabric/") + assert components.bfabric_instance == HttpUrl("https://fgcz-bfabric.uzh.ch/bfabric") assert components.entity_type == "project" assert components.entity_id == 3000 @pytest.mark.parametrize( - "bfabric_instance", ["https://bfabric.example.com/bfabric/", "https://bfabric.example.com/bfabric"] + "bfabric_instance", ["https://bfabric.example.com/bfabric", "https://bfabric.example.com/bfabric"] ) def test_from_components(self, bfabric_instance: str): entity_uri = EntityUri.from_components(bfabric_instance, "dataset", 1234) @@ -84,7 +84,7 @@ def test_idempotent_on_entity_uri(self): def test_components(self): components = EntityUri.normalize(f"{CANONICAL_URI}&tab=details").components - assert components.bfabric_instance == HttpUrl("https://fgcz-bfabric.uzh.ch/bfabric/") + assert components.bfabric_instance == HttpUrl("https://fgcz-bfabric.uzh.ch/bfabric") assert components.entity_type == "workunit" assert components.entity_id == 346001 @@ -122,13 +122,13 @@ class TestEntityUriComponents: @pytest.mark.parametrize( "bfabric_instance", [ - "https://fgcz-bfabric.uzh.ch/bfabric/", - "https://bfabric.example.com/bfabric/", - "http://localhost:8080/bfabric/", + "https://fgcz-bfabric.uzh.ch/bfabric", + "https://bfabric.example.com/bfabric", + "http://localhost:8080/bfabric", ], ) def test_valid(self, bfabric_instance): - uri = f"{bfabric_instance}project/show.html?id=3000" + uri = f"{bfabric_instance}/project/show.html?id=3000" parsed = _parse_uri_components(uri) assert parsed.bfabric_instance == HttpUrl(bfabric_instance) assert parsed.entity_type == "project" @@ -148,7 +148,7 @@ def test_invalid(self, uri): _parse_uri_components(uri) @pytest.mark.parametrize( - "bfabric_instance", ["https://bfabric.example.com/bfabric/", "https://bfabric.example.com/bfabric"] + "bfabric_instance", ["https://bfabric.example.com/bfabric", "https://bfabric.example.com/bfabric"] ) def test_as_uri(self, bfabric_instance): components = EntityUriComponents(bfabric_instance=bfabric_instance, entity_type="project", entity_id=3000) @@ -192,7 +192,7 @@ def test_from_uris_groups_by_type_and_instance(self): # Verify groups contain correct URIs groups_dict = {(key.bfabric_instance, key.entity_type): uris for key, uris in grouped.items()} - assert groups_dict[("https://instance1.example.org/bfabric/", "project")] == [uri1, uri2] - assert groups_dict[("https://instance1.example.org/bfabric/", "user")] == [uri3] - assert groups_dict[("https://instance2.example.org/bfabric/", "project")] == [uri4] - assert groups_dict[("https://instance2.example.org/bfabric/", "user")] == [uri5] + assert groups_dict[("https://instance1.example.org/bfabric", "project")] == [uri1, uri2] + assert groups_dict[("https://instance1.example.org/bfabric", "user")] == [uri3] + assert groups_dict[("https://instance2.example.org/bfabric", "project")] == [uri4] + assert groups_dict[("https://instance2.example.org/bfabric", "user")] == [uri5] diff --git a/tests/bfabric/entities/test_dataset.py b/tests/bfabric/entities/test_dataset.py index af30109df..f976d64b7 100644 --- a/tests/bfabric/entities/test_dataset.py +++ b/tests/bfabric/entities/test_dataset.py @@ -113,7 +113,7 @@ def test_get_parquet(mock_dataset: Dataset) -> None: def test_repr(mock_empty_dataset: Dataset) -> None: assert ( repr(mock_empty_dataset) == "Dataset(data_dict={'id': 1234, 'attribute': [], 'item': []}, " - "bfabric_instance='https://bfabric.example.org/bfabric/')" + "bfabric_instance='https://bfabric.example.org/bfabric')" ) diff --git a/tests/bfabric/oauth/test_device_code.py b/tests/bfabric/oauth/test_device_code.py index 0454d38d2..8dcaa0e88 100644 --- a/tests/bfabric/oauth/test_device_code.py +++ b/tests/bfabric/oauth/test_device_code.py @@ -294,31 +294,6 @@ def test_prints_verification_uri_complete(self, mocker, capsys): captured = capsys.readouterr() assert "https://example.com/device?user_code=WXYZ-9876" in captured.err - def test_strips_trailing_slash(self, mocker): - device_response = { - "device_code": "dc_456", - "user_code": "CODE", - "verification_uri": "https://example.com/device", - "interval": 5, - } - - mock_request = mocker.patch( - "bfabric._oauth.device_code._request_device_code", - return_value=device_response, - ) - mock_poll = mocker.patch( - "bfabric._oauth.device_code._poll_for_token", - return_value={"access_token": "at"}, - ) - device_code_login("https://example.com/bfabric///", client_id="test-cli", scope="api:read") - - mock_request.assert_called_once_with( - "https://example.com/bfabric", - client_id="test-cli", - scope="api:read", - ) - assert mock_poll.call_args[0][0] == "https://example.com/bfabric" - def test_default_interval_when_missing(self, mocker): device_response = { "device_code": "dc_456", diff --git a/tests/bfabric/oauth/test_registration.py b/tests/bfabric/oauth/test_registration.py index b540a9d18..f57f49694 100644 --- a/tests/bfabric/oauth/test_registration.py +++ b/tests/bfabric/oauth/test_registration.py @@ -120,7 +120,7 @@ def test_without_optional_params(self, mock_httpx_post): def test_normalizes_trailing_slash(self, mock_httpx_post): register_client( - base_url="https://example.com/bfabric/", + base_url="https://example.com/bfabric", token="tok", client_name="app", redirect_uri="http://localhost/cb", @@ -157,7 +157,7 @@ class TestRegisterWebapp: @pytest.fixture def mock_client(self, mocker): client = mocker.MagicMock(name="bfabric_client") - client.config.base_url = "https://example.com/bfabric/" + client.config.base_url = "https://example.com/bfabric" client.save.return_value = mocker.MagicMock(name="save_result") return client diff --git a/tests/bfabric/oauth/test_token_cache.py b/tests/bfabric/oauth/test_token_cache.py index 03704f403..815f0c86f 100644 --- a/tests/bfabric/oauth/test_token_cache.py +++ b/tests/bfabric/oauth/test_token_cache.py @@ -7,6 +7,7 @@ import pytest from bfabric._oauth.token_cache import TokenCache, compute_token_cache_path +from bfabric.config import BfabricClientConfig @pytest.fixture @@ -67,10 +68,13 @@ def test_deterministic(self): p2 = compute_token_cache_path("https://example.com/bfabric", "my-client", "PROD") assert p1 == p2 - def test_trailing_slash_ignored(self): - p1 = compute_token_cache_path("https://example.com/bfabric", "c", "PROD") - p2 = compute_token_cache_path("https://example.com/bfabric/", "c", "PROD") - assert p1 == p2 + def test_canonical_base_url_keeps_the_key_stable(self): + # The key is not slash-insensitive by itself; canonicalising upstream is what makes a cache + # written from a slashed config still resolve, so an existing login survives. + config = BfabricClientConfig(base_url="https://example.com/bfabric/") + assert compute_token_cache_path(config.base_url, "c", "PROD") == compute_token_cache_path( + "https://example.com/bfabric", "c", "PROD" + ) def test_different_client_id_gives_different_path(self): p1 = compute_token_cache_path("https://example.com/bfabric", "client-a", "PROD") diff --git a/tests/bfabric/oauth/test_token_exchange.py b/tests/bfabric/oauth/test_token_exchange.py index 21d722495..67864ba39 100644 --- a/tests/bfabric/oauth/test_token_exchange.py +++ b/tests/bfabric/oauth/test_token_exchange.py @@ -43,22 +43,6 @@ def test_posts_correct_payload(self, mocker): "token_type": "Bearer", } - def test_strips_trailing_slash(self, mocker): - mock_post = mocker.patch("bfabric._oauth.token_exchange.httpx.post") - mock_response = mocker.MagicMock() - mock_response.json.return_value = {"access_token": "at"} - mock_post.return_value = mock_response - - exchange_token( - f"{BASE_URL}///", - "jwt", - client_id="cid", - client_secret="cs", - ) - - url = mock_post.call_args[0][0] - assert url == f"{BASE_URL}/rest/oauth/token" - def test_raises_on_http_error(self, mocker): import httpx @@ -122,17 +106,6 @@ def test_handles_minimal_claims(self, mocker): assert ctx.application_id is None assert ctx.subject == "user" - def test_strips_trailing_slash(self, mocker): - mock_post = mocker.patch("bfabric._oauth.token_exchange.httpx.post") - mock_response = mocker.MagicMock() - mock_response.json.return_value = {"sub": "u"} - mock_post.return_value = mock_response - - introspect_token(f"{BASE_URL}///", "at", client_id="cid", client_secret="cs") - - url = mock_post.call_args[0][0] - assert url == f"{BASE_URL}/rest/oauth/introspect" - def test_raises_on_http_error(self, mocker): import httpx diff --git a/tests/bfabric/oauth/test_url_token.py b/tests/bfabric/oauth/test_url_token.py index c4706c2f5..6185d5203 100644 --- a/tests/bfabric/oauth/test_url_token.py +++ b/tests/bfabric/oauth/test_url_token.py @@ -73,5 +73,5 @@ def test_refetches_expired_jwks(self, mock_httpx_get, mock_joserfc): assert mock_httpx_get.call_count == 2 def test_normalizes_trailing_slash(self, mock_httpx_get, mock_joserfc): - verify_jwt("https://example.com/bfabric/", "token") + verify_jwt("https://example.com/bfabric", "token") mock_httpx_get.assert_called_once_with("https://example.com/bfabric/rest/oauth/jwks", timeout=30) diff --git a/tests/bfabric/operations/dataset/test_operations.py b/tests/bfabric/operations/dataset/test_operations.py index c903e5bf8..1d1f8e164 100644 --- a/tests/bfabric/operations/dataset/test_operations.py +++ b/tests/bfabric/operations/dataset/test_operations.py @@ -15,7 +15,7 @@ @pytest.fixture def mock_client(mocker): client = mocker.MagicMock(name="Bfabric") - client.config.base_url = "https://test.example.com/bfabric/" + client.config.base_url = "https://test.example.com/bfabric" return client diff --git a/tests/bfabric/operations/workunit/test_create.py b/tests/bfabric/operations/workunit/test_create.py index 4221131cc..821b3596e 100644 --- a/tests/bfabric/operations/workunit/test_create.py +++ b/tests/bfabric/operations/workunit/test_create.py @@ -152,7 +152,7 @@ def test_create_workunit_returned_entity_has_usable_uri(mock_client, bfabric_ins workunit = create_workunit(mock_client, _params()) - assert str(workunit.uri) == f"{bfabric_instance}workunit/show.html?id=42" + assert str(workunit.uri) == f"{bfabric_instance}/workunit/show.html?id=42" def test_create_workunit_returns_metadata_only_entity(mock_client): diff --git a/tests/bfabric/test_bfabric.py b/tests/bfabric/test_bfabric.py index 8e580a2dc..64838ff1e 100644 --- a/tests/bfabric/test_bfabric.py +++ b/tests/bfabric/test_bfabric.py @@ -18,7 +18,7 @@ @pytest.fixture def mock_config(): - return BfabricClientConfig(engine=BfabricAPIEngineType.SUDS, base_url="https://example.com/api/") + return BfabricClientConfig(engine=BfabricAPIEngineType.SUDS, base_url="https://example.com/api") @pytest.fixture @@ -94,7 +94,7 @@ def test_connect_pat_env_from_config_file(mocker, tmp_path): def test_connect_webapp(mocker, mock_config): mock_get_token_data = mocker.patch( "bfabric.bfabric.get_token_data", - return_value=mocker.MagicMock(user="test_user", user_ws_password="x" * 32, caller="https://example.com/api/"), + return_value=mocker.MagicMock(user="test_user", user_ws_password="x" * 32, caller="https://example.com/api"), ) mocker.patch.object(Bfabric, "_log_version_message") @@ -102,7 +102,7 @@ def test_connect_webapp(mocker, mock_config): assert client.auth.login == "test_user" assert client.auth.password == SecretStr("x" * 32) - assert client.config.base_url == "https://example.com/api/" + assert client.config.base_url == "https://example.com/api" assert data == mock_get_token_data.return_value mock_get_token_data.assert_called_once_with(base_url="https://example.com/validation/", token="test_token") @@ -111,8 +111,8 @@ def test_connect_webapp(mocker, mock_config): @pytest.fixture def token_validation_settings(): class MockSettings: - validation_bfabric_instance = "https://example.com/bfabric/" - supported_bfabric_instances = ["https://example.com/bfabric/"] + validation_bfabric_instance = "https://example.com/bfabric" + supported_bfabric_instances = ["https://example.com/bfabric"] return MockSettings() @@ -122,13 +122,13 @@ def mock_validate_token(mocker): func = mocker.patch("bfabric.bfabric.validate_token") func.return_value.user = "test_user" func.return_value.user_ws_password = SecretStr("x" * 32) - func.return_value.caller = "https://example.com/bfabric/" + func.return_value.caller = "https://example.com/bfabric" return func def test_connect_token(mock_config, token_validation_settings, mock_validate_token): client, data = Bfabric.connect_token(token="test_token", settings=token_validation_settings) - assert client.config.base_url == "https://example.com/bfabric/" + assert client.config.base_url == "https://example.com/bfabric" assert client.auth.login == "test_user" assert client.auth.password == SecretStr("x" * 32) assert data == mock_validate_token.return_value @@ -136,7 +136,7 @@ def test_connect_token(mock_config, token_validation_settings, mock_validate_tok async def test_connect_token_async(mock_config, token_validation_settings, mock_validate_token): client, data = await Bfabric.connect_token_async(token="test_token", settings=token_validation_settings) - assert client.config.base_url == "https://example.com/bfabric/" + assert client.config.base_url == "https://example.com/bfabric" assert client.auth.login == "test_user" assert client.auth.password == SecretStr("x" * 32) assert data == mock_validate_token.return_value @@ -423,6 +423,19 @@ def test_upload_resource(bfabric_instance, mocker): ) +class TestEngineUrl: + """The URL the default engine builds from a canonicalised ``base_url``.""" + + def test_suds_wsdl_url_has_a_single_slash(self, mocker): + construct_client = mocker.patch("bfabric.engine.engine_suds.Client") + mocker.patch.object(Bfabric, "_log_version_message") + client = Bfabric( + config_data=ConfigData(client=BfabricClientConfig(base_url="https://example.com/bfabric"), auth=None) + ) + client._engine._get_suds_service("sample") + construct_client.assert_called_once_with("https://example.com/bfabric/sample?wsdl", cache=None) + + def test_get_version_message(mock_config, bfabric_instance): mock_config.base_url = "dummy_url" line1, line2 = bfabric_instance._get_version_message() @@ -445,7 +458,7 @@ def test_log_version_message(mocker, bfabric_instance): def test_repr(bfabric_instance, variant): assert ( variant(bfabric_instance) == "Bfabric(config_data=ConfigData(" - "client=BfabricClientConfig(base_url='https://example.com/api/', application_ids={}, " + "client=BfabricClientConfig(base_url='https://example.com/api', application_ids={}, " "job_notification_emails='', engine=BfabricAPIEngineType.SUDS), auth=None, " "auth_method=None, client_id=None, env_name=None))" ) @@ -500,7 +513,7 @@ def test_creates_instance_with_provider(self, mocker): ) assert client._credential_provider == mock_provider_cls.return_value assert client._auth is None - assert client.config.base_url == "https://example.com/bfabric/" + assert client.config.base_url == "https://example.com/bfabric" def test_auth_property_uses_provider(self, mocker): mocker.patch.object(Bfabric, "_log_version_message") @@ -525,11 +538,11 @@ def test_strips_trailing_slash(self, mocker): client = Bfabric.connect_oauth( client_id="id", client_secret="secret", - base_url="https://example.com/bfabric/", + base_url="https://example.com/bfabric", scope=_TEST_SCOPE, ) - assert client.config.base_url == "https://example.com/bfabric/" + assert client.config.base_url == "https://example.com/bfabric" call_kwargs = mock_provider_cls.call_args[1] assert call_kwargs["token_url"] == "https://example.com/bfabric/rest/oauth/token" @@ -613,7 +626,7 @@ def test_creates_instance_with_provider(self, mocker): ) assert client._credential_provider == mock_provider_cls.return_value assert client._auth is None - assert client.config.base_url == "https://example.com/bfabric/" + assert client.config.base_url == "https://example.com/bfabric" def test_parameter_forwarding(self, mocker): mocker.patch.object(Bfabric, "_log_version_message") @@ -657,7 +670,7 @@ def test_strips_trailing_slash(self, mocker): scope=_TEST_SCOPE, ) - assert client.config.base_url == "https://example.com/bfabric/" + assert client.config.base_url == "https://example.com/bfabric" call_kwargs = mock_provider_cls.call_args[1] assert call_kwargs["token_url"] == "https://example.com/bfabric/rest/oauth/token" @@ -696,7 +709,7 @@ def test_creates_instance_with_provider(self, mocker): ) assert client._credential_provider == mock_provider_cls.return_value assert client._auth is None - assert client.config.base_url == "https://example.com/bfabric/" + assert client.config.base_url == "https://example.com/bfabric" def test_parameter_forwarding(self, mocker): mocker.patch.object(Bfabric, "_log_version_message") @@ -736,7 +749,7 @@ def test_strips_trailing_slash(self, mocker): scope=_TEST_SCOPE, ) - assert client.config.base_url == "https://example.com/bfabric/" + assert client.config.base_url == "https://example.com/bfabric" call_kwargs = mock_provider_cls.call_args[1] assert call_kwargs["token_url"] == "https://example.com/bfabric/rest/oauth/token" @@ -752,7 +765,7 @@ def test_creates_instance_with_auth(self, mocker): assert client.auth.login == OAUTH_LOGIN assert client.auth.password.get_secret_value() == "my_personal_access_token" - assert client.config.base_url == "https://example.com/bfabric/" + assert client.config.base_url == "https://example.com/bfabric" def test_no_credential_provider(self, mocker): mocker.patch.object(Bfabric, "_log_version_message") @@ -772,7 +785,7 @@ def test_strips_trailing_slash(self, mocker): pat="my_pat", ) - assert client.config.base_url == "https://example.com/bfabric/" + assert client.config.base_url == "https://example.com/bfabric" def test_accepts_secret_str(self, mocker): mocker.patch.object(Bfabric, "_log_version_message") diff --git a/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_annotation_specs.py b/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_annotation_specs.py index 0eab22b71..6450e3e67 100644 --- a/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_annotation_specs.py +++ b/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_annotation_specs.py @@ -16,7 +16,7 @@ @pytest.fixture def bfabric_instance(): - return "https://bfabric.example.org/bfabric/" + return "https://bfabric.example.org/bfabric" @pytest.fixture diff --git a/tests/bfabric_scripts/cli/login/test_cmd_auth_pat.py b/tests/bfabric_scripts/cli/login/test_cmd_auth_pat.py index 7623d7b17..024edcf78 100644 --- a/tests/bfabric_scripts/cli/login/test_cmd_auth_pat.py +++ b/tests/bfabric_scripts/cli/login/test_cmd_auth_pat.py @@ -31,7 +31,7 @@ def test_writes_config_with_flag(self, tmp_path, capsys): def test_strips_trailing_slash(self, tmp_path): config_file = tmp_path / "config.yml" cmd_auth_pat( - base_url="https://example.com/bfabric/", + base_url="https://example.com/bfabric", pat="tok", config_env="PROD", config_file=config_file, diff --git a/tests/bfabric_scripts/cli/workunit/test_cmd_workunit_diff.py b/tests/bfabric_scripts/cli/workunit/test_cmd_workunit_diff.py index 78913a9dc..cc85328e1 100644 --- a/tests/bfabric_scripts/cli/workunit/test_cmd_workunit_diff.py +++ b/tests/bfabric_scripts/cli/workunit/test_cmd_workunit_diff.py @@ -17,7 +17,7 @@ @pytest.fixture def mock_client(mocker): client = mocker.Mock(spec=Bfabric) - client.config.base_url = "https://fgcz-bfabric.uzh.ch/bfabric/" + client.config.base_url = "https://fgcz-bfabric.uzh.ch/bfabric" return client diff --git a/tests/bfabric_scripts/feeder/test_path_convention_compms.py b/tests/bfabric_scripts/feeder/test_path_convention_compms.py index eede7b10f..eba53e32d 100644 --- a/tests/bfabric_scripts/feeder/test_path_convention_compms.py +++ b/tests/bfabric_scripts/feeder/test_path_convention_compms.py @@ -9,7 +9,7 @@ @pytest.fixture def mock_storage(mocker): data_dict = {"projectfolderprefix": "x", "basepath": "/base/path"} - return Storage(data_dict, client=None, bfabric_instance="https://example.com/bfabric/") + return Storage(data_dict, client=None, bfabric_instance="https://example.com/bfabric") @pytest.fixture From f3fff876844ef8758d861ded694b2a1189613ead Mon Sep 17 00:00:00 2001 From: Leonardo Schwarz Date: Thu, 13 Aug 2026 10:19:46 +0200 Subject: [PATCH 2/3] refactor(bfabric): enforce the canonical base_url form in the type system The slash-free canonical form was a convention nothing checked: a caller could hand a raw string to `pkce_login` or `compute_token_cache_path` and nothing would object. That is not hypothetical -- `bfabric-cli auth register` was passing a raw `--base-url` straight into `register_client`, previously masked by a defensive `rstrip`. Add `bfabric.config.CanonicalBaseUrl`, a `str` subclass that validates and canonicalises in `__new__`, mirroring the existing `EntityUri` pattern. It stays a `str`, so interpolation, comparison and dict keys are unaffected, but annotating a parameter with it lets basedpyright reject a value that never passed through canonicalisation -- which `Annotated[str, AfterValidator(...)]` cannot, being indistinguishable from `str`. Public entry points (`connect_*`, `WebappClient.create`) keep taking `str` and canonicalise once; private helpers and the entity layer require the type. That collapses the previous construct-then-read-back dance into `base_url = CanonicalBaseUrl(base_url)`, with the config built where it belongs. `EntityUriComponents.bfabric_instance` moves from pydantic's `HttpUrl` to the same type, removing the third spelling of one concept. The `str()` coercion when building a `GroupKey` existed only because `HttpUrl` is not a `str`, and sat on exactly the seam that diverged in #576; the retype also clears a grandfathered `reportArgumentType` from the baseline. Two fixes fall out of typing the boundary: `validate_token` compared the server's `caller` against the configured `supported_bfabric_instances` by exact string equality, so a trailing slash on either side rejected a valid token. Both sides are now canonicalised. This is legacy API, so the fix is local and the settings stay `str`. Typing `normalize_base_url`'s return exposed that the CLI writes its config through unsafe `yaml.dump`, which serialised the subclass as `!!python/object/new:` -- a file the `safe_load` on read cannot parse. The writer now uses `safe_dump` (matching the reader) and the CLI writes plain strings, so a non-YAML type fails loudly instead of corrupting `~/.bfabricpy.yml`. The baseline loses two entries and gains none. --- .basedpyright/baseline.bfabric.json | 16 ---- bfabric/docs/changelog.md | 5 +- .../src/bfabric/_oauth/credential_provider.py | 5 +- bfabric/src/bfabric/_oauth/device_code.py | 10 ++- bfabric/src/bfabric/_oauth/pkce.py | 6 +- bfabric/src/bfabric/_oauth/registration.py | 3 +- bfabric/src/bfabric/_oauth/token_cache.py | 6 +- bfabric/src/bfabric/_oauth/token_exchange.py | 9 +- bfabric/src/bfabric/_oauth/url_token.py | 8 +- bfabric/src/bfabric/_oauth/webapp_client.py | 6 +- bfabric/src/bfabric/bfabric.py | 20 +++-- bfabric/src/bfabric/config/__init__.py | 3 +- bfabric/src/bfabric/config/base_url.py | 34 ++++++++ .../bfabric/config/bfabric_client_config.py | 18 +--- bfabric/src/bfabric/config/config_writer.py | 4 +- bfabric/src/bfabric/engine/engine_suds.py | 3 +- bfabric/src/bfabric/engine/engine_zeep.py | 3 +- bfabric/src/bfabric/entities/core/entity.py | 9 +- .../bfabric/entities/core/entity_reader.py | 38 +++++---- .../bfabric/entities/core/import_entity.py | 5 +- .../src/bfabric/entities/core/references.py | 11 +-- bfabric/src/bfabric/entities/core/uri.py | 17 ++-- bfabric/src/bfabric/entities/core/users.py | 5 +- bfabric/src/bfabric/rest/token_data.py | 7 +- .../src/bfabric_scripts/cli/login/_common.py | 6 +- .../src/bfabric_scripts/cli/login/_urls.py | 8 +- .../bfabric_scripts/cli/login/oauth_login.py | 10 ++- .../src/bfabric_scripts/cli/login/pat.py | 2 +- tests/bfabric/config/test_base_url.py | 85 +++++++++++++++++++ tests/bfabric/entities/core/test_uri.py | 8 +- tests/bfabric/rest/test_token_data.py | 40 ++++++++- tests/bfabric/test_bfabric.py | 8 ++ 32 files changed, 307 insertions(+), 111 deletions(-) create mode 100644 bfabric/src/bfabric/config/base_url.py create mode 100644 tests/bfabric/config/test_base_url.py diff --git a/.basedpyright/baseline.bfabric.json b/.basedpyright/baseline.bfabric.json index 2c7242171..bba7f3436 100644 --- a/.basedpyright/baseline.bfabric.json +++ b/.basedpyright/baseline.bfabric.json @@ -139,14 +139,6 @@ "lineCount": 1 } }, - { - "code": "reportCallIssue", - "range": { - "startColumn": 17, - "endColumn": 55, - "lineCount": 1 - } - }, { "code": "reportArgumentType", "range": { @@ -1703,14 +1695,6 @@ } ], "./bfabric/src/bfabric/entities/core/uri.py": [ - { - "code": "reportArgumentType", - "range": { - "startColumn": 29, - "endColumn": 45, - "lineCount": 1 - } - }, { "code": "reportUnknownMemberType", "range": { diff --git a/bfabric/docs/changelog.md b/bfabric/docs/changelog.md index adbaba19c..1a6274850 100644 --- a/bfabric/docs/changelog.md +++ b/bfabric/docs/changelog.md @@ -29,11 +29,14 @@ Minor breaking changes are still possible in `1.X.Y` but we try to announce them - `create_workunit` accepts a plain mapping for `params`, validated internally so an invalid mapping raises `ValidationError` before any write. - **Breaking: `BfabricClientConfig.base_url` is canonicalised *without* a trailing slash**, reversing the 1.15.0 "always ends with exactly one `/`". Config files and `connect_*` arguments still accept one. - `Entity.bfabric_instance` and `EntityUri.components.bfabric_instance` follow the same form; `EntityUri` strings are unchanged. -- `connect_oauth` / `connect_pkce` / `connect_device_code` / `connect_pat` and `WebappClient.create` canonicalise `base_url` through `BfabricClientConfig`, so a host with mixed case or a default port is normalised too, and a non-HTTP URL is rejected up front. +- `connect_oauth` / `connect_pkce` / `connect_device_code` / `connect_pat` and `WebappClient.create` canonicalise `base_url` through the new `CanonicalBaseUrl`, so a host with mixed case or a default port is normalised too, and a non-HTTP URL is rejected up front. +- **Breaking: new `bfabric.config.CanonicalBaseUrl`**, a `str` subclass holding a validated slash-free instance URL. `BfabricClientConfig.base_url`, `Entity.bfabric_instance` and `EntityUriComponents.bfabric_instance` (was a pydantic `HttpUrl`) are now this type, and `EntityReader`'s `bfabric_instance` arguments require it. It behaves as a `str` everywhere, so interpolation, comparison and dict keys are unaffected; constructing `BfabricClientConfig` directly now needs `CanonicalBaseUrl(...)`, while `model_validate` still accepts a plain string. +- `bfabric-cli auth login` / `auth pat` write the config with `yaml.safe_dump`, matching the `safe_load` used to read it back. ### Fixed - The SUDS and Zeep WSDL URLs no longer contain a doubled slash (`…/bfabric//workunit?wsdl`), and neither do the `show.html` links printed by `bfabric_read` and `bfabric-cli api read`. +- `validate_token` canonicalises both the token's `caller` and the configured `supported_bfabric_instances` before comparing, so a trailing slash on either side no longer rejects a valid token. - `ResultContainer.assert_success` raises `BfabricRequestError` instead of a bare `RuntimeError`; it remains a `RuntimeError` subclass, so existing `except RuntimeError` handlers keep working. - The "could not find the config file" and "empty list provided for deletion" diagnostics go through loguru at WARNING instead of `print()`, so they honour the configured log level and sink. - `setup_script_logging` no longer skips setup in subprocesses, which fell back to loguru's verbose DEBUG default; its repeat guard is now process-local. diff --git a/bfabric/src/bfabric/_oauth/credential_provider.py b/bfabric/src/bfabric/_oauth/credential_provider.py index e78eb5d37..078d21d68 100644 --- a/bfabric/src/bfabric/_oauth/credential_provider.py +++ b/bfabric/src/bfabric/_oauth/credential_provider.py @@ -31,6 +31,7 @@ from bfabric._oauth.token_cache import TokenCache, compute_token_cache_path if TYPE_CHECKING: + from bfabric.config.base_url import CanonicalBaseUrl from pathlib import Path @@ -120,7 +121,9 @@ def __init__( self._persist() @classmethod - def cache_login_token(cls, base_url: str, *, client_id: str, token: dict[str, object], env_name: str) -> Path: + def cache_login_token( + cls, base_url: CanonicalBaseUrl, *, client_id: str, token: dict[str, object], env_name: str + ) -> Path: """Normalize and cache a freshly obtained login *token*, returning its cache path. Ingesting the token derives its absolute ``expires_at`` (from ``expires_in``) and writes the diff --git a/bfabric/src/bfabric/_oauth/device_code.py b/bfabric/src/bfabric/_oauth/device_code.py index 528e47548..8b634b2de 100644 --- a/bfabric/src/bfabric/_oauth/device_code.py +++ b/bfabric/src/bfabric/_oauth/device_code.py @@ -14,15 +14,19 @@ import sys import time +from typing import TYPE_CHECKING import httpx from loguru import logger from bfabric.errors import BfabricOAuthError +if TYPE_CHECKING: + from bfabric.config.base_url import CanonicalBaseUrl + def _request_device_code( - base_url: str, + base_url: CanonicalBaseUrl, *, client_id: str, scope: str, @@ -51,7 +55,7 @@ def _request_device_code( def _poll_for_token( - base_url: str, + base_url: CanonicalBaseUrl, *, device_code: str, client_id: str, @@ -136,7 +140,7 @@ def _poll_for_token( def device_code_login( - base_url: str, + base_url: CanonicalBaseUrl, *, client_id: str, scope: str, diff --git a/bfabric/src/bfabric/_oauth/pkce.py b/bfabric/src/bfabric/_oauth/pkce.py index 08cded710..1354e4a18 100644 --- a/bfabric/src/bfabric/_oauth/pkce.py +++ b/bfabric/src/bfabric/_oauth/pkce.py @@ -15,12 +15,16 @@ from dataclasses import dataclass from http.server import BaseHTTPRequestHandler, HTTPServer from urllib.parse import parse_qs, urlencode, urlparse +from typing import TYPE_CHECKING import httpx from loguru import logger from bfabric.errors import BfabricOAuthError +if TYPE_CHECKING: + from bfabric.config.base_url import CanonicalBaseUrl + _REMOTE_HOST_CAVEAT = "On a remote host, use 'bfabric-cli auth device-code' instead." @@ -166,7 +170,7 @@ def _exchange_code( def pkce_login( - base_url: str, + base_url: CanonicalBaseUrl, *, client_id: str, scope: str, diff --git a/bfabric/src/bfabric/_oauth/registration.py b/bfabric/src/bfabric/_oauth/registration.py index b6b161e01..3bb6c27c1 100644 --- a/bfabric/src/bfabric/_oauth/registration.py +++ b/bfabric/src/bfabric/_oauth/registration.py @@ -9,6 +9,7 @@ if TYPE_CHECKING: from bfabric.bfabric import Bfabric + from bfabric.config.base_url import CanonicalBaseUrl from bfabric.results.result_container import ResultContainer from bfabric.typing import ApiRequestDataType @@ -32,7 +33,7 @@ def _default_grant_types(service_user: str | None) -> list[str]: def register_client( - base_url: str, + base_url: CanonicalBaseUrl, token: str, client_name: str, redirect_uri: str, diff --git a/bfabric/src/bfabric/_oauth/token_cache.py b/bfabric/src/bfabric/_oauth/token_cache.py index 761632139..d52e344c5 100644 --- a/bfabric/src/bfabric/_oauth/token_cache.py +++ b/bfabric/src/bfabric/_oauth/token_cache.py @@ -6,11 +6,15 @@ import json import os from pathlib import Path +from typing import TYPE_CHECKING from loguru import logger +if TYPE_CHECKING: + from bfabric.config.base_url import CanonicalBaseUrl -def compute_token_cache_path(base_url: str, client_id: str, env_name: str) -> Path: + +def compute_token_cache_path(base_url: CanonicalBaseUrl, client_id: str, env_name: str) -> Path: """Return the default token cache path for a given base URL, client ID, and environment name. The path is ``~/.bfabric/tokens/{hash}.json`` where *hash* is the first 16 diff --git a/bfabric/src/bfabric/_oauth/token_exchange.py b/bfabric/src/bfabric/_oauth/token_exchange.py index 9a21d558a..3e0b6e5d6 100644 --- a/bfabric/src/bfabric/_oauth/token_exchange.py +++ b/bfabric/src/bfabric/_oauth/token_exchange.py @@ -7,14 +7,19 @@ from __future__ import annotations +from typing import TYPE_CHECKING + import httpx from loguru import logger from bfabric._oauth.url_token import UrlTokenContext +if TYPE_CHECKING: + from bfabric.config.base_url import CanonicalBaseUrl + def exchange_token( - base_url: str, + base_url: CanonicalBaseUrl, launch_token: str, *, client_id: str, @@ -52,7 +57,7 @@ def exchange_token( def introspect_token( - base_url: str, + base_url: CanonicalBaseUrl, access_token: str, *, client_id: str, diff --git a/bfabric/src/bfabric/_oauth/url_token.py b/bfabric/src/bfabric/_oauth/url_token.py index 591d35403..a70152d2d 100644 --- a/bfabric/src/bfabric/_oauth/url_token.py +++ b/bfabric/src/bfabric/_oauth/url_token.py @@ -5,6 +5,7 @@ import threading import time from datetime import datetime +from typing import TYPE_CHECKING import httpx from joserfc import jwt as joserfc_jwt @@ -12,6 +13,9 @@ from loguru import logger from pydantic import BaseModel, ConfigDict, Field +if TYPE_CHECKING: + from bfabric.config.base_url import CanonicalBaseUrl + class UrlTokenContext(BaseModel): """Claims extracted from a B-Fabric URL token JWT. @@ -54,7 +58,7 @@ def is_employee(self) -> bool: _JWKS_CACHE_TTL = 3600 # 1 hour -def _fetch_jwks(base_url: str) -> dict[str, object]: +def _fetch_jwks(base_url: CanonicalBaseUrl) -> dict[str, object]: """Fetch (and cache for 1 hour) the JWKS from the B-Fabric server.""" now = time.time() with _jwks_lock: @@ -75,7 +79,7 @@ def _fetch_jwks(base_url: str) -> dict[str, object]: return jwks -def verify_jwt(base_url: str, token: str) -> dict[str, object]: +def verify_jwt(base_url: CanonicalBaseUrl, token: str) -> dict[str, object]: """Verify the JWT signature + expiry against the B-Fabric JWKS endpoint. :param base_url: B-Fabric instance URL (e.g. ``https://bfabric.example.com/bfabric``) diff --git a/bfabric/src/bfabric/_oauth/webapp_client.py b/bfabric/src/bfabric/_oauth/webapp_client.py index d5cc32781..ac92fe773 100644 --- a/bfabric/src/bfabric/_oauth/webapp_client.py +++ b/bfabric/src/bfabric/_oauth/webapp_client.py @@ -55,11 +55,10 @@ def create( from bfabric._oauth.credential_provider import OAuthCredentialProvider from bfabric._oauth.token_exchange import exchange_token from bfabric._oauth.url_token import UrlTokenContext, verify_jwt - from bfabric.config import BfabricClientConfig + from bfabric.config import BfabricClientConfig, CanonicalBaseUrl from bfabric.config.config_data import ConfigData - config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] - base_url = config.base_url + base_url = CanonicalBaseUrl(base_url) token_url = f"{base_url}/rest/oauth/token" # 1. Exchange the short-lived launch token for access + refresh tokens @@ -84,6 +83,7 @@ def create( grant_type="refresh_token", token_cache_path=user_token_cache_path, ) + config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] user_client = Bfabric( config_data=ConfigData(client=config, auth=None), _credential_provider=user_provider, diff --git a/bfabric/src/bfabric/bfabric.py b/bfabric/src/bfabric/bfabric.py index c341de7d2..1304d75c5 100644 --- a/bfabric/src/bfabric/bfabric.py +++ b/bfabric/src/bfabric/bfabric.py @@ -28,7 +28,7 @@ from loguru import logger from rich.console import Console -from bfabric.config import DEFAULT_CONFIG_FILE, BfabricAuth, BfabricClientConfig +from bfabric.config import DEFAULT_CONFIG_FILE, BfabricAuth, BfabricClientConfig, CanonicalBaseUrl from bfabric.config.bfabric_client_config import BfabricAPIEngineType from bfabric.config.config_data import ConfigData, load_config_data from bfabric.config.config_file import read_config_file @@ -259,8 +259,7 @@ def connect_oauth( """ from bfabric._oauth.credential_provider import OAuthCredentialProvider - config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] - base_url = config.base_url + base_url = CanonicalBaseUrl(base_url) token_url = f"{base_url}/rest/oauth/token" provider = OAuthCredentialProvider( client_id=client_id, @@ -270,6 +269,7 @@ def connect_oauth( grant_type="client_credentials", token_cache_path=token_cache_path, ) + config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] config_data = ConfigData(client=config, auth=None) return cls(config_data=config_data, _credential_provider=provider) @@ -302,8 +302,7 @@ def connect_pkce( from bfabric._oauth.credential_provider import OAuthCredentialProvider from bfabric._oauth.pkce import pkce_login - config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] - base_url = config.base_url + base_url = CanonicalBaseUrl(base_url) token = pkce_login( base_url, client_id=client_id, @@ -322,6 +321,7 @@ def connect_pkce( scope=scope, token_cache_path=token_cache_path, ) + config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] config_data = ConfigData(client=config, auth=None) return cls(config_data=config_data, _credential_provider=provider) @@ -354,8 +354,7 @@ def connect_device_code( from bfabric._oauth.credential_provider import OAuthCredentialProvider from bfabric._oauth.device_code import device_code_login - config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] - base_url = config.base_url + base_url = CanonicalBaseUrl(base_url) token = device_code_login( base_url, client_id=client_id, @@ -372,6 +371,7 @@ def connect_device_code( scope=scope, token_cache_path=token_cache_path, ) + config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] config_data = ConfigData(client=config, auth=None) return cls(config_data=config_data, _credential_provider=provider) @@ -395,6 +395,7 @@ def connect_pat( from bfabric.config.bfabric_auth import OAUTH_LOGIN + base_url = CanonicalBaseUrl(base_url) pat_value: str = pat.get_secret_value() if isinstance(pat, SecretStr) else pat auth = BfabricAuth(login=OAUTH_LOGIN, password=SecretStr(pat_value)) config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] @@ -673,6 +674,7 @@ def get_system_auth( ) resolved_path = Path(config_path or "~/.bfabricpy.yml").expanduser() + canonical_base_url = CanonicalBaseUrl(base_url) if base_url is not None else None # Use the provided config data from arguments instead of the file if not resolved_path.is_file(): @@ -680,13 +682,13 @@ def get_system_auth( # NOTE: If user explicitly specifies a path to a wrong config file, this has to be an exception raise OSError(f"Explicitly specified config file does not exist: {resolved_path}") logger.warning(f"could not find the config file in the default location: {resolved_path}") - config = BfabricClientConfig(base_url=base_url) + config = BfabricClientConfig(base_url=canonical_base_url) # pyright: ignore[reportCallIssue] auth = None if login is None or password is None else BfabricAuth(login=login, password=password) # Load config from file, override some of the fields with the provided ones else: config, auth = read_config_file(resolved_path, config_env=config_env) - config = config.copy_with(base_url=base_url) + config = config.copy_with(base_url=canonical_base_url) if (login is not None) and (password is not None): auth = BfabricAuth(login=login, password=password) elif (login is None) and (password is None): diff --git a/bfabric/src/bfabric/config/__init__.py b/bfabric/src/bfabric/config/__init__.py index e1b34955d..f19706469 100644 --- a/bfabric/src/bfabric/config/__init__.py +++ b/bfabric/src/bfabric/config/__init__.py @@ -1,5 +1,6 @@ +from .base_url import CanonicalBaseUrl from .bfabric_auth import BfabricAuth from .bfabric_client_config import BfabricClientConfig from .config_file import ConfigFile, DEFAULT_CONFIG_FILE -__all__ = ["BfabricAuth", "BfabricClientConfig", "ConfigFile", "DEFAULT_CONFIG_FILE"] +__all__ = ["BfabricAuth", "BfabricClientConfig", "CanonicalBaseUrl", "ConfigFile", "DEFAULT_CONFIG_FILE"] diff --git a/bfabric/src/bfabric/config/base_url.py b/bfabric/src/bfabric/config/base_url.py new file mode 100644 index 000000000..4bf2f836a --- /dev/null +++ b/bfabric/src/bfabric/config/base_url.py @@ -0,0 +1,34 @@ +"""The canonical form of a B-Fabric instance base URL. + +Kept in a leaf module importing only pydantic, so the entity layer can depend on it without +introducing an import cycle. +""" + +from __future__ import annotations + +from pydantic import AnyHttpUrl, GetCoreSchemaHandler, TypeAdapter +from pydantic_core import core_schema + + +class CanonicalBaseUrl(str): + """A validated B-Fabric instance base URL, canonicalised without a trailing slash. + + A ``str`` subclass rather than a wrapper model: the value is interpolated into request URLs and + used as a cache key, so it has to behave like a string everywhere. Annotating a parameter with it + is what lets the type checker reject a base URL that never passed through canonicalisation -- + ``Annotated[str, AfterValidator(...)]`` validates at runtime but is indistinguishable from ``str``. + + Construction is idempotent and total, which is what a cache key needs. + """ + + def __new__(cls, value: str) -> CanonicalBaseUrl: + if isinstance(value, cls): + return value + http_url = TypeAdapter(AnyHttpUrl).validate_python(value) + # The strip has to come after validation: AnyHttpUrl re-adds the slash for an empty path. + return super().__new__(cls, str(http_url).rstrip("/")) + + @classmethod + def __get_pydantic_core_schema__(cls, source_type: object, handler: GetCoreSchemaHandler) -> core_schema.CoreSchema: + _ = source_type, handler + return core_schema.no_info_after_validator_function(cls, core_schema.str_schema()) diff --git a/bfabric/src/bfabric/config/bfabric_client_config.py b/bfabric/src/bfabric/config/bfabric_client_config.py index 999382cfb..468ba3df0 100644 --- a/bfabric/src/bfabric/config/bfabric_client_config.py +++ b/bfabric/src/bfabric/config/bfabric_client_config.py @@ -3,19 +3,9 @@ from enum import StrEnum from typing import Annotated -from pydantic import AfterValidator, AnyHttpUrl, BaseModel, Field, TypeAdapter +from pydantic import BaseModel, Field - -def _validate_base_url(value: str) -> str: - """Validates that the base URL is indeed a valid HTTP URL, canonicalised without a trailing slash. - - The strip has to come after validation: ``AnyHttpUrl`` re-adds the slash for a URL with an empty path. - """ - http_url = TypeAdapter(AnyHttpUrl).validate_python(value) - return str(http_url).rstrip("/") - - -_ValidatedBaseUrl = Annotated[str, AfterValidator(_validate_base_url)] +from bfabric.config.base_url import CanonicalBaseUrl class BfabricAPIEngineType(StrEnum): @@ -37,14 +27,14 @@ class BfabricClientConfig(BaseModel): :param engine: The API engine to use (optional). """ - base_url: _ValidatedBaseUrl + base_url: CanonicalBaseUrl application_ids: Annotated[dict[str, int], Field(default_factory=dict)] job_notification_emails: Annotated[str, Field(default="")] engine: BfabricAPIEngineType = BfabricAPIEngineType.SUDS def copy_with( self, - base_url: str | None = None, + base_url: CanonicalBaseUrl | None = None, application_ids: dict[str, int] | None = None, engine: BfabricAPIEngineType | None = None, ) -> BfabricClientConfig: diff --git a/bfabric/src/bfabric/config/config_writer.py b/bfabric/src/bfabric/config/config_writer.py index 18084a66e..c739dad0d 100644 --- a/bfabric/src/bfabric/config/config_writer.py +++ b/bfabric/src/bfabric/config/config_writer.py @@ -1,6 +1,6 @@ """Write environment entries to the bfabricpy YAML config file. -Note: rewriting the file drops any YAML comments in it (``yaml.dump`` doesn't preserve them). +Note: rewriting the file drops any YAML comments in it (``yaml.safe_dump`` doesn't preserve them). """ from __future__ import annotations @@ -40,7 +40,7 @@ def _write_config_file(config_path: Path, data: Mapping[str, object]) -> None: """Serialize *data* to *config_path* as YAML, mode ``0o600`` (fchmod forces it on existing files).""" config_path = Path(config_path).expanduser() config_path.parent.mkdir(parents=True, exist_ok=True) - serialized = yaml.dump(data, default_flow_style=False, sort_keys=False).encode() + serialized = yaml.safe_dump(data, default_flow_style=False, sort_keys=False).encode() fd = os.open(str(config_path), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) try: os.fchmod(fd, 0o600) diff --git a/bfabric/src/bfabric/engine/engine_suds.py b/bfabric/src/bfabric/engine/engine_suds.py index 9cc3de460..ece812736 100644 --- a/bfabric/src/bfabric/engine/engine_suds.py +++ b/bfabric/src/bfabric/engine/engine_suds.py @@ -17,13 +17,14 @@ from suds.serviceproxy import ServiceProxy from bfabric.config import BfabricAuth + from bfabric.config.base_url import CanonicalBaseUrl from bfabric.typing import ApiRequestObjectType class EngineSUDS: """B-Fabric API SUDS Engine.""" - def __init__(self, base_url: str, drop_underscores: bool = True) -> None: + def __init__(self, base_url: CanonicalBaseUrl, drop_underscores: bool = True) -> None: self._cl = {} self._base_url = base_url self._drop_underscores = drop_underscores diff --git a/bfabric/src/bfabric/engine/engine_zeep.py b/bfabric/src/bfabric/engine/engine_zeep.py index 4bee0ff5e..fdae43526 100644 --- a/bfabric/src/bfabric/engine/engine_zeep.py +++ b/bfabric/src/bfabric/engine/engine_zeep.py @@ -14,13 +14,14 @@ if TYPE_CHECKING: from bfabric.config import BfabricAuth + from bfabric.config.base_url import CanonicalBaseUrl from bfabric.typing import ApiRequestObjectType class EngineZeep: """B-Fabric API Zeep Engine""" - def __init__(self, base_url: str) -> None: + def __init__(self, base_url: CanonicalBaseUrl) -> None: self._cl = {} self._base_url = base_url diff --git a/bfabric/src/bfabric/entities/core/entity.py b/bfabric/src/bfabric/entities/core/entity.py index 6994581e1..14d4744c1 100644 --- a/bfabric/src/bfabric/entities/core/entity.py +++ b/bfabric/src/bfabric/entities/core/entity.py @@ -14,6 +14,7 @@ from typing import Any from bfabric import Bfabric + from bfabric.config.base_url import CanonicalBaseUrl from bfabric.entities.core.references import References from bfabric.typing import ApiResponseDataType, ApiResponseObjectType @@ -25,7 +26,7 @@ def __init__( self, data_dict: ApiResponseObjectType, client: Bfabric | None = None, - bfabric_instance: str | None = None, + bfabric_instance: CanonicalBaseUrl | None = None, ) -> None: # note: client may be removed completely in the future, # as I think it is a design mistake to have put them into these classes @@ -51,7 +52,7 @@ def id(self) -> int: return value @property - def bfabric_instance(self) -> str: + def bfabric_instance(self) -> CanonicalBaseUrl: """The bfabric instance URL associated with the entity.""" return self.__bfabric_instance @@ -148,7 +149,9 @@ def dump_yaml(self, path: Path) -> None: yaml.safe_dump(self.__data_dict, file) @classmethod - def load_yaml(cls, path: Path, client: Bfabric | None = None, bfabric_instance: str | None = None) -> Self: + def load_yaml( + cls, path: Path, client: Bfabric | None = None, bfabric_instance: CanonicalBaseUrl | None = None + ) -> Self: """Loads an entity from a YAML file.""" # TODO (#351): to be extended import yaml diff --git a/bfabric/src/bfabric/entities/core/entity_reader.py b/bfabric/src/bfabric/entities/core/entity_reader.py index f040f0c4c..bd1e26629 100644 --- a/bfabric/src/bfabric/entities/core/entity_reader.py +++ b/bfabric/src/bfabric/entities/core/entity_reader.py @@ -14,6 +14,7 @@ from collections.abc import Iterable, Sequence from bfabric import Bfabric + from bfabric.config.base_url import CanonicalBaseUrl from bfabric.typing import ApiRequestObjectType, ApiResponseDataType, ApiResponseObjectType @@ -144,24 +145,26 @@ def read_uris( @overload def read_id( - self, entity_type: type[EntityT], entity_id: int | str, bfabric_instance: str | None = None + self, entity_type: type[EntityT], entity_id: int | str, bfabric_instance: CanonicalBaseUrl | None = None ) -> EntityT | None: ... @overload def read_id( self, entity_type: str, entity_id: int | str, - bfabric_instance: str | None = None, + bfabric_instance: CanonicalBaseUrl | None = None, *, expected_type: type[EntityT], ) -> EntityT | None: ... @overload - def read_id(self, entity_type: str, entity_id: int | str, bfabric_instance: str | None = None) -> Entity | None: ... + def read_id( + self, entity_type: str, entity_id: int | str, bfabric_instance: CanonicalBaseUrl | None = None + ) -> Entity | None: ... def read_id( self, entity_type: str | type[EntityT], entity_id: int | str, - bfabric_instance: str | None = None, + bfabric_instance: CanonicalBaseUrl | None = None, *, expected_type: type[EntityT] = Entity, ) -> EntityT | None: @@ -191,26 +194,29 @@ def read_id( @overload def read_ids( - self, entity_type: type[EntityT], entity_ids: Sequence[int | str], bfabric_instance: str | None = None + self, + entity_type: type[EntityT], + entity_ids: Sequence[int | str], + bfabric_instance: CanonicalBaseUrl | None = None, ) -> EntityResult[EntityT]: ... @overload def read_ids( self, entity_type: str, entity_ids: Sequence[int | str], - bfabric_instance: str | None = None, + bfabric_instance: CanonicalBaseUrl | None = None, *, expected_type: type[EntityT], ) -> EntityResult[EntityT]: ... @overload def read_ids( - self, entity_type: str, entity_ids: Sequence[int | str], bfabric_instance: str | None = None + self, entity_type: str, entity_ids: Sequence[int | str], bfabric_instance: CanonicalBaseUrl | None = None ) -> EntityResult[Entity]: ... def read_ids( self, entity_type: str | type[EntityT], entity_ids: Sequence[int | str], - bfabric_instance: str | None = None, + bfabric_instance: CanonicalBaseUrl | None = None, *, expected_type: type[EntityT] = Entity, ) -> EntityResult[EntityT]: @@ -239,7 +245,7 @@ def query( self, entity_type: type[EntityT], obj: ApiRequestObjectType, - bfabric_instance: str | None = None, + bfabric_instance: CanonicalBaseUrl | None = None, max_results: int | None = 100, ) -> dict[EntityUri, EntityT]: ... @overload @@ -247,7 +253,7 @@ def query( self, entity_type: str, obj: ApiRequestObjectType, - bfabric_instance: str | None = None, + bfabric_instance: CanonicalBaseUrl | None = None, max_results: int | None = 100, *, expected_type: type[EntityT], @@ -257,14 +263,14 @@ def query( self, entity_type: str, obj: ApiRequestObjectType, - bfabric_instance: str | None = None, + bfabric_instance: CanonicalBaseUrl | None = None, max_results: int | None = 100, ) -> dict[EntityUri, Entity]: ... def query( self, entity_type: str | type[EntityT], obj: ApiRequestObjectType, - bfabric_instance: str | None = None, + bfabric_instance: CanonicalBaseUrl | None = None, max_results: int | None = 100, *, expected_type: type[EntityT] = Entity, @@ -310,26 +316,26 @@ def query( @overload def query_one( - self, entity_type: type[EntityT], obj: ApiRequestObjectType, bfabric_instance: str | None = None + self, entity_type: type[EntityT], obj: ApiRequestObjectType, bfabric_instance: CanonicalBaseUrl | None = None ) -> EntityT | None: ... @overload def query_one( self, entity_type: str, obj: ApiRequestObjectType, - bfabric_instance: str | None = None, + bfabric_instance: CanonicalBaseUrl | None = None, *, expected_type: type[EntityT], ) -> EntityT | None: ... @overload def query_one( - self, entity_type: str, obj: ApiRequestObjectType, bfabric_instance: str | None = None + self, entity_type: str, obj: ApiRequestObjectType, bfabric_instance: CanonicalBaseUrl | None = None ) -> Entity | None: ... def query_one( self, entity_type: str | type[EntityT], obj: ApiRequestObjectType, - bfabric_instance: str | None = None, + bfabric_instance: CanonicalBaseUrl | None = None, *, expected_type: type[EntityT] = Entity, ) -> EntityT | None: diff --git a/bfabric/src/bfabric/entities/core/import_entity.py b/bfabric/src/bfabric/entities/core/import_entity.py index 13cd18812..0cffdac3d 100644 --- a/bfabric/src/bfabric/entities/core/import_entity.py +++ b/bfabric/src/bfabric/entities/core/import_entity.py @@ -5,6 +5,7 @@ if TYPE_CHECKING: from bfabric import Bfabric + from bfabric.config.base_url import CanonicalBaseUrl from bfabric.entities.core.entity import Entity from bfabric.typing import ApiResponseObjectType @@ -36,7 +37,9 @@ def entity_type_of(entity_class: type[Entity]) -> str: return entity_class.__name__.lower() -def instantiate_entity(data_dict: ApiResponseObjectType, client: Bfabric | None, bfabric_instance: str) -> Entity: +def instantiate_entity( + data_dict: ApiResponseObjectType, client: Bfabric | None, bfabric_instance: CanonicalBaseUrl +) -> Entity: """Instantiates an entity given its data dictionary with the most specific class possible.""" entity_class_name = data_dict["classname"] if not isinstance(entity_class_name, str): diff --git a/bfabric/src/bfabric/entities/core/references.py b/bfabric/src/bfabric/entities/core/references.py index 3d92cb5f1..1f31c505a 100644 --- a/bfabric/src/bfabric/entities/core/references.py +++ b/bfabric/src/bfabric/entities/core/references.py @@ -12,6 +12,7 @@ if TYPE_CHECKING: from bfabric import Bfabric + from bfabric.config.base_url import CanonicalBaseUrl from bfabric.entities.core.entity import Entity from bfabric.typing import ApiResponseDataType, ApiResponseObjectType @@ -34,9 +35,9 @@ class References: This class receives a reference to the entity's data dictionary, updating it in-place when references are loaded. """ - def __init__(self, client: Bfabric, bfabric_instance: str, data_ref: ApiResponseObjectType) -> None: + def __init__(self, client: Bfabric, bfabric_instance: CanonicalBaseUrl, data_ref: ApiResponseObjectType) -> None: self._client: Bfabric = client - self._bfabric_instance: str = bfabric_instance + self._bfabric_instance: CanonicalBaseUrl = bfabric_instance self._data_ref: ApiResponseObjectType = data_ref # Retrieve information about all reference fields @@ -122,7 +123,7 @@ def __load(self, ref_info: _ReferenceInformation) -> None: @classmethod def __extract_reference_info( - cls, data_ref: ApiResponseObjectType, bfabric_instance: str + cls, data_ref: ApiResponseObjectType, bfabric_instance: CanonicalBaseUrl ) -> dict[str, _ReferenceInformation]: references: dict[str, _ReferenceInformation] = {} for name, value in data_ref.items(): @@ -133,7 +134,7 @@ def __extract_reference_info( @classmethod def __extract_reference_info_item( - cls, name: str, value: ApiResponseDataType, bfabric_instance: str + cls, name: str, value: ApiResponseDataType, bfabric_instance: CanonicalBaseUrl ) -> _ReferenceInformation | None: if isinstance(value, dict) and "classname" in value and "id" in value: info = cls.__extract_reference_info_item_dict(value, bfabric_instance) @@ -156,7 +157,7 @@ def __extract_reference_info_item( @classmethod def __extract_reference_info_item_dict( - cls, value: ApiResponseDataType, bfabric_instance: str + cls, value: ApiResponseDataType, bfabric_instance: CanonicalBaseUrl ) -> dict[str, EntityUri | bool]: # value is guaranteed to be a dict by the caller's isinstance check value_dict = cast("dict[str, ApiResponseDataType]", value) diff --git a/bfabric/src/bfabric/entities/core/uri.py b/bfabric/src/bfabric/entities/core/uri.py index 1315cd426..98350da15 100644 --- a/bfabric/src/bfabric/entities/core/uri.py +++ b/bfabric/src/bfabric/entities/core/uri.py @@ -9,12 +9,13 @@ AfterValidator, BaseModel, ConfigDict, - HttpUrl, StringConstraints, TypeAdapter, ) from pydantic_core import core_schema +from bfabric.config.base_url import CanonicalBaseUrl + if TYPE_CHECKING: from collections.abc import Iterator @@ -60,9 +61,7 @@ def invalid(reason: str) -> ValueError: raise invalid(f"expected query exactly 'id=' and no fragment; {_NORMALIZE_HINT}") return EntityUriComponents( - # Slash-free, matching the canonical form of BfabricClientConfig.base_url that EntityReader - # compares this against. - bfabric_instance=HttpUrl(f"{parsed.scheme}://{parsed.netloc.lower()}/bfabric"), + bfabric_instance=CanonicalBaseUrl(f"{parsed.scheme}://{parsed.netloc.lower()}/bfabric"), entity_type=segments[1], entity_id=int(entity_id), ) @@ -100,7 +99,7 @@ def __new__(cls, uri: str | EntityUri) -> EntityUri: return instance @classmethod - def from_components(cls, bfabric_instance: str, entity_type: str, entity_id: int) -> EntityUri: + def from_components(cls, bfabric_instance: CanonicalBaseUrl, entity_type: str, entity_id: int) -> EntityUri: """Create EntityUri from individual components. Args: @@ -152,7 +151,7 @@ class EntityUriComponents(BaseModel): entity_id: Numeric entity ID (must be positive) """ - bfabric_instance: HttpUrl + bfabric_instance: CanonicalBaseUrl entity_type: Annotated[str, StringConstraints(pattern=r"^[a-z]+$")] entity_id: Annotated[int, annotated_types.Gt(0)] @@ -172,7 +171,7 @@ class GroupKey(BaseModel): """Grouping key for EntityUris.""" model_config = ConfigDict(frozen=True) - bfabric_instance: str + bfabric_instance: CanonicalBaseUrl entity_type: str groups: dict[GroupKey, list[EntityUri]] = {} @@ -193,8 +192,6 @@ def from_uris(cls, uris: list[EntityUri]) -> GroupedUris: """ groups = defaultdict(list) for uri in uris: - key = cls.GroupKey( - bfabric_instance=str(uri.components.bfabric_instance), entity_type=uri.components.entity_type - ) + key = cls.GroupKey(bfabric_instance=uri.components.bfabric_instance, entity_type=uri.components.entity_type) groups[key].append(uri) return cls(groups=dict(groups)) diff --git a/bfabric/src/bfabric/entities/core/users.py b/bfabric/src/bfabric/entities/core/users.py index c01893c92..85cc06d96 100644 --- a/bfabric/src/bfabric/entities/core/users.py +++ b/bfabric/src/bfabric/entities/core/users.py @@ -4,6 +4,7 @@ if TYPE_CHECKING: + from bfabric.config.base_url import CanonicalBaseUrl from bfabric.entities.core.entity_reader import EntityReader from bfabric.entities.user import User @@ -15,7 +16,7 @@ def __init__(self, entity_reader: EntityReader) -> None: self._users = [] self._entity_reader = entity_reader - def get_by_id(self, bfabric_instance: str, id: int) -> User | None: + def get_by_id(self, bfabric_instance: CanonicalBaseUrl, id: int) -> User | None: """Gets a user by their ID.""" # check if exists for user in self._users: @@ -31,7 +32,7 @@ def get_by_id(self, bfabric_instance: str, id: int) -> User | None: self._users.append(user) return user - def get_by_login(self, bfabric_instance: str, login: str) -> User | None: + def get_by_login(self, bfabric_instance: CanonicalBaseUrl, login: str) -> User | None: """Gets a user by their login name.""" from bfabric.entities.user import User as UserEntity diff --git a/bfabric/src/bfabric/rest/token_data.py b/bfabric/src/bfabric/rest/token_data.py index 78a8b5615..f73dd2f2a 100644 --- a/bfabric/src/bfabric/rest/token_data.py +++ b/bfabric/src/bfabric/rest/token_data.py @@ -19,6 +19,7 @@ from pydantic import ValidationError +from bfabric.config.base_url import CanonicalBaseUrl from bfabric.entities.core.import_entity import import_entity from bfabric.errors import ( BfabricInstanceNotConfiguredError, @@ -132,6 +133,10 @@ async def validate_token( token_data = await get_token_data_async( base_url=settings.validation_bfabric_instance, token=token, http_client=http_client ) - if token_data.caller not in settings.supported_bfabric_instances: + # Both sides are canonicalised before comparing: the server picks the form of ``caller``, the + # operator picks the form of the configured instances, and a trailing slash must not decide + # whether a token is accepted. The error still reports the raw value the server sent. + supported = {CanonicalBaseUrl(instance) for instance in settings.supported_bfabric_instances} + if CanonicalBaseUrl(token_data.caller) not in supported: raise BfabricInstanceNotConfiguredError(token_data.caller) return token_data diff --git a/bfabric_scripts/src/bfabric_scripts/cli/login/_common.py b/bfabric_scripts/src/bfabric_scripts/cli/login/_common.py index a6b29ae79..1aca42413 100644 --- a/bfabric_scripts/src/bfabric_scripts/cli/login/_common.py +++ b/bfabric_scripts/src/bfabric_scripts/cli/login/_common.py @@ -14,6 +14,7 @@ from bfabric.config.config_file import ConfigFile, EnvironmentConfig 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 CanonicalBaseUrl from bfabric_scripts.cli.login._urls import KNOWN_INSTANCES, normalize_base_url # Interactive-only sentinel: choosing it opens a free-text prompt. @@ -61,7 +62,7 @@ def _pick_or_type(message: str, labels: dict[str, str], prompt: str) -> str | No return text_input(prompt) if picked == _CUSTOM else picked -def resolve_base_url(base_url: str | None, env: EnvironmentConfig | None) -> str | None: +def resolve_base_url(base_url: str | None, env: EnvironmentConfig | None) -> CanonicalBaseUrl | None: """Resolve the instance URL: explicit, else the environment's recorded one, else a picker.""" if base_url is not None: return normalize_base_url(base_url) @@ -75,7 +76,8 @@ def resolve_base_url(base_url: str | None, env: EnvironmentConfig | None) -> str picked = _pick_or_type("Select the B-Fabric instance", labels, "B-Fabric instance URL") if not picked: return None - return KNOWN_INSTANCES.get(picked) or normalize_base_url(picked) + known = KNOWN_INSTANCES.get(picked) + return CanonicalBaseUrl(known) if known else normalize_base_url(picked) def resolve_scope(scope: str | None, env: EnvironmentConfig | None = None) -> str | None: diff --git a/bfabric_scripts/src/bfabric_scripts/cli/login/_urls.py b/bfabric_scripts/src/bfabric_scripts/cli/login/_urls.py index 62d4637ba..fdc863d28 100644 --- a/bfabric_scripts/src/bfabric_scripts/cli/login/_urls.py +++ b/bfabric_scripts/src/bfabric_scripts/cli/login/_urls.py @@ -4,6 +4,8 @@ from urllib.parse import urlsplit, urlunsplit +from bfabric.config import CanonicalBaseUrl + # Suggested environment name -> instance base URL. KNOWN_INSTANCES: dict[str, str] = { "fgcz-prod": "https://fgcz-bfabric.uzh.ch/bfabric", @@ -22,7 +24,7 @@ def instance_host(base_url: str) -> str: _BY_HOST: dict[str, tuple[str, str]] = {instance_host(url): (name, url) for name, url in KNOWN_INSTANCES.items()} -def normalize_base_url(raw: str) -> str: +def normalize_base_url(raw: str) -> CanonicalBaseUrl: """Canonicalise a base URL: default the scheme to https, lowercase the host, drop a trailing slash, and expand a bare known host to that instance's full base URL. @@ -42,8 +44,8 @@ def normalize_base_url(raw: str) -> str: host = parts.netloc.lower() # Only expand a bare host: rewriting an explicit path would break an unusual deployment. if not parts.path.strip("/") and host in _BY_HOST: - return _BY_HOST[host][1] - return urlunsplit((parts.scheme, host, parts.path.rstrip("/"), "", "")) + return CanonicalBaseUrl(_BY_HOST[host][1]) + return CanonicalBaseUrl(urlunsplit((parts.scheme, host, parts.path.rstrip("/"), "", ""))) def suggest_env_name(base_url: str) -> str: diff --git a/bfabric_scripts/src/bfabric_scripts/cli/login/oauth_login.py b/bfabric_scripts/src/bfabric_scripts/cli/login/oauth_login.py index 762c4894a..4c1788b4e 100644 --- a/bfabric_scripts/src/bfabric_scripts/cli/login/oauth_login.py +++ b/bfabric_scripts/src/bfabric_scripts/cli/login/oauth_login.py @@ -27,6 +27,7 @@ resolve_set_default, ) from bfabric_scripts.cli.login._constants import DEFAULT_CLIENT_ID +from bfabric.config import CanonicalBaseUrl from bfabric_scripts.cli.login._urls import normalize_base_url, suggest_env_name _SCOPE_HELP = ( @@ -45,7 +46,7 @@ class _LoginParams: """Everything a login needs, resolved from the command line, config, or a prompt.""" config_env: str - base_url: str + base_url: CanonicalBaseUrl client_id: str scope: str set_default: bool @@ -134,7 +135,12 @@ def _persist(token: dict[str, object], params: _LoginParams, config_file: Path) _ = OAuthCredentialProvider.cache_login_token( params.base_url, client_id=params.client_id, token=token, env_name=params.config_env ) - data = {"base_url": params.base_url, "auth_method": "oauth", "client_id": params.client_id, "scope": params.scope} + data = { + "base_url": str(params.base_url), + "auth_method": "oauth", + "client_id": params.client_id, + "scope": params.scope, + } write_environment_to_config(config_file, params.config_env, data, set_default=params.set_default) print("Authenticated successfully.") print(f"Config saved to environment '{params.config_env}' in {config_file}") diff --git a/bfabric_scripts/src/bfabric_scripts/cli/login/pat.py b/bfabric_scripts/src/bfabric_scripts/cli/login/pat.py index efbf3d067..93a51690d 100644 --- a/bfabric_scripts/src/bfabric_scripts/cli/login/pat.py +++ b/bfabric_scripts/src/bfabric_scripts/cli/login/pat.py @@ -52,7 +52,7 @@ def cmd_auth_pat( print("Warning: passing secrets via CLI flags is insecure (visible in ps, shell history).", file=sys.stderr) # Store under ``pat``, not ``login``/``password``: a PAT isn't 32 chars, so an old (<=1.19.0) # client validating every environment would reject it and poison the shared config. - env_data = {"base_url": normalize_base_url(base_url), "auth_method": "pat", "pat": pat} + env_data = {"base_url": str(normalize_base_url(base_url)), "auth_method": "pat", "pat": pat} write_environment_to_config(config_file, config_env, env_data, set_default=set_default) print("Authenticated successfully.") print(f"Config saved to environment '{config_env}' in {config_file}") diff --git a/tests/bfabric/config/test_base_url.py b/tests/bfabric/config/test_base_url.py new file mode 100644 index 000000000..8fd97c50f --- /dev/null +++ b/tests/bfabric/config/test_base_url.py @@ -0,0 +1,85 @@ +import pickle + +import pytest +import yaml +from pydantic import ValidationError + +from bfabric.config import BfabricClientConfig, CanonicalBaseUrl + + +class TestCanonicalisation: + @pytest.mark.parametrize( + "raw", + [ + "https://example.com/bfabric", + "https://example.com/bfabric/", + "https://example.com/bfabric////", + ], + ) + def test_drops_trailing_slashes(self, raw): + assert CanonicalBaseUrl(raw) == "https://example.com/bfabric" + + def test_host_only_url_keeps_no_slash(self): + # AnyHttpUrl re-adds the slash for an empty path, so the strip has to happen after validation. + assert CanonicalBaseUrl("https://example.com") == "https://example.com" + + def test_normalizes_host_case_and_default_port(self): + assert CanonicalBaseUrl("https://EXAMPLE.com:443/bfabric") == "https://example.com/bfabric" + + def test_is_idempotent(self): + once = CanonicalBaseUrl("https://example.com/bfabric/") + assert CanonicalBaseUrl(once) == once + + @pytest.mark.parametrize("raw", ["not a url", "", "ftp://example.com/bfabric"]) + def test_rejects_non_http_url(self, raw): + with pytest.raises(ValidationError): + CanonicalBaseUrl(raw) + + +class TestBehavesLikeStr: + """The reason this is a ``str`` subclass rather than a wrapper model.""" + + def test_interpolates_without_ceremony(self): + url = CanonicalBaseUrl("https://example.com/bfabric") + assert f"{url}/rest/oauth/token" == "https://example.com/bfabric/rest/oauth/token" + + def test_compares_and_hashes_as_str(self): + url = CanonicalBaseUrl("https://example.com/bfabric/") + assert url == "https://example.com/bfabric" + assert {url: 1}["https://example.com/bfabric"] == 1 + + def test_survives_pickling(self): + url = CanonicalBaseUrl("https://example.com/bfabric") + assert pickle.loads(pickle.dumps(url)) == url + + +class TestOnTheConfigModel: + def test_field_is_canonicalised(self): + config = BfabricClientConfig(base_url=CanonicalBaseUrl("https://example.com/bfabric/")) + assert config.base_url == "https://example.com/bfabric" + assert isinstance(config.base_url, CanonicalBaseUrl) + + def test_model_validate_accepts_a_plain_string(self): + # The config is the boundary where un-canonicalised input legitimately arrives. + config = BfabricClientConfig.model_validate({"base_url": "https://example.com/bfabric/"}) + assert config.base_url == "https://example.com/bfabric" + + def test_json_dump_yields_a_plain_str(self): + config = BfabricClientConfig.model_validate({"base_url": "https://example.com/bfabric"}) + dumped = config.model_dump(mode="json") + assert type(dumped["base_url"]) is str + + def test_json_dump_is_yaml_safe_dumpable(self): + """Guards the trap: a str subclass reaching a yaml dumper writes an unloadable file. + + ``yaml.safe_dump`` raises on a subclass and plain ``yaml.dump`` silently emits a + ``!!python/object/new:`` tag, so anything bound for YAML must go through json-mode dumping. + """ + config = BfabricClientConfig.model_validate({"base_url": "https://example.com/bfabric"}) + serialized = yaml.safe_dump(config.model_dump(mode="json")) + assert yaml.safe_load(serialized)["base_url"] == "https://example.com/bfabric" + + def test_round_trip_dump_reloads(self): + config = BfabricClientConfig.model_validate({"base_url": "https://example.com/bfabric"}) + dumped = config.model_dump(mode="json", round_trip=True) + assert BfabricClientConfig.model_validate(dumped) == config diff --git a/tests/bfabric/entities/core/test_uri.py b/tests/bfabric/entities/core/test_uri.py index 8a05ffc67..be1ce84f5 100644 --- a/tests/bfabric/entities/core/test_uri.py +++ b/tests/bfabric/entities/core/test_uri.py @@ -1,5 +1,5 @@ import pytest -from pydantic import HttpUrl, BaseModel +from pydantic import BaseModel from bfabric.entities.core.uri import EntityUri, EntityUriComponents, GroupedUris from bfabric.entities.core.uri import _parse_uri_components @@ -43,7 +43,7 @@ def test_components_property(self): uri = "https://fgcz-bfabric.uzh.ch/bfabric/project/show.html?id=3000" entity_uri = EntityUri(uri) components = entity_uri.components - assert components.bfabric_instance == HttpUrl("https://fgcz-bfabric.uzh.ch/bfabric") + assert components.bfabric_instance == "https://fgcz-bfabric.uzh.ch/bfabric" assert components.entity_type == "project" assert components.entity_id == 3000 @@ -84,7 +84,7 @@ def test_idempotent_on_entity_uri(self): def test_components(self): components = EntityUri.normalize(f"{CANONICAL_URI}&tab=details").components - assert components.bfabric_instance == HttpUrl("https://fgcz-bfabric.uzh.ch/bfabric") + assert components.bfabric_instance == "https://fgcz-bfabric.uzh.ch/bfabric" assert components.entity_type == "workunit" assert components.entity_id == 346001 @@ -130,7 +130,7 @@ class TestEntityUriComponents: def test_valid(self, bfabric_instance): uri = f"{bfabric_instance}/project/show.html?id=3000" parsed = _parse_uri_components(uri) - assert parsed.bfabric_instance == HttpUrl(bfabric_instance) + assert parsed.bfabric_instance == bfabric_instance assert parsed.entity_type == "project" assert parsed.entity_id == 3000 diff --git a/tests/bfabric/rest/test_token_data.py b/tests/bfabric/rest/test_token_data.py index be32a3738..19e35e8d4 100644 --- a/tests/bfabric/rest/test_token_data.py +++ b/tests/bfabric/rest/test_token_data.py @@ -5,8 +5,8 @@ import httpx from pydantic import SecretStr, ValidationError -from bfabric.errors import BfabricTokenExpiredError, BfabricTokenInvalidError -from bfabric.rest.token_data import TokenData, get_token_data, get_token_data_async +from bfabric.errors import BfabricInstanceNotConfiguredError, BfabricTokenExpiredError, BfabricTokenInvalidError +from bfabric.rest.token_data import TokenData, get_token_data, get_token_data_async, validate_token @pytest.fixture @@ -174,3 +174,39 @@ def test_get_token_data(mocker, token_data, base_url): call_args = mock_get_token_data_async.call_args assert call_args.kwargs["base_url"] == base_url assert call_args.kwargs["token"] == "mock-token" + + +class TestValidateToken: + """The instance allow-list check. + + The server picks the form of ``caller`` and the operator picks the form of the configured + instances, so a trailing slash must not decide whether a token is accepted. + """ + + @pytest.fixture + def settings(self): + class MockSettings: + validation_bfabric_instance = "https://example.com/bfabric" + supported_bfabric_instances = ["https://example.com/bfabric"] + + return MockSettings() + + @pytest.fixture + def patched_get(self, mocker, token_data): + return mocker.patch("bfabric.rest.token_data.get_token_data_async", return_value=token_data) + + @pytest.mark.parametrize("caller", ["https://example.com/bfabric", "https://example.com/bfabric/"]) + async def test_accepts_either_caller_form(self, settings, patched_get, token_data, caller): + token_data.caller = caller + assert await validate_token(token="mock-token", settings=settings) is token_data + + @pytest.mark.parametrize("configured", ["https://example.com/bfabric", "https://example.com/bfabric/"]) + async def test_accepts_either_configured_form(self, settings, patched_get, token_data, configured): + settings.supported_bfabric_instances = [configured] + token_data.caller = "https://example.com/bfabric/" + assert await validate_token(token="mock-token", settings=settings) is token_data + + async def test_rejects_a_genuinely_different_instance(self, settings, patched_get, token_data): + token_data.caller = "https://other.example.com/bfabric" + with pytest.raises(BfabricInstanceNotConfiguredError): + await validate_token(token="mock-token", settings=settings) diff --git a/tests/bfabric/test_bfabric.py b/tests/bfabric/test_bfabric.py index 64838ff1e..cedf7047e 100644 --- a/tests/bfabric/test_bfabric.py +++ b/tests/bfabric/test_bfabric.py @@ -6,6 +6,7 @@ from pydantic import SecretStr from bfabric import Bfabric, BfabricAPIEngineType, BfabricClientConfig, BfabricAuth +from bfabric.config import CanonicalBaseUrl from bfabric.config import DEFAULT_CONFIG_FILE from bfabric.config.bfabric_auth import OAUTH_LOGIN from bfabric.config.config_data import ConfigData @@ -840,3 +841,10 @@ def test_password_client_round_trip(self, mocker): restored = pickle.loads(pickle.dumps(client)) # noqa: S301 assert restored._credential_provider is None assert restored.auth.login == "user" + + def test_canonical_base_url_survives_round_trip(self): + """``config.base_url`` is a ``str`` subclass, which pickles by re-calling the constructor.""" + config = BfabricClientConfig.model_validate({"base_url": "https://example.com/bfabric/"}) + restored = pickle.loads(pickle.dumps(config)) # noqa: S301 + assert restored.base_url == "https://example.com/bfabric" + assert isinstance(restored.base_url, CanonicalBaseUrl) From 975b338ec143aff8794c1bbaa5e78a9a48058a40 Mon Sep 17 00:00:00 2001 From: Leonardo Schwarz Date: Thu, 13 Aug 2026 10:39:40 +0200 Subject: [PATCH 3/3] refactor(bfabric): make the canonical base URL a type, not a convention #596 fixed the canonical form of base_url and canonicalises it once per public boundary, but the resulting invariant was convention only: nothing stopped a caller handing a raw string to pkce_login or compute_token_cache_path. That was not hypothetical -- `auth register` was doing exactly that, masked by a defensive rstrip until #596 removed it. The failure modes are asymmetric. A stray slash in a URL yields `//`, which servers tolerate, which is why #576 went unnoticed for months. A stray slash in compute_token_cache_path changes the SHA-256, so the cache misses and the user is told to log in again while their token sits on disk. bfabric.BaseUrl is a str subclass that validates and canonicalises in __new__, so basedpyright rejects an unvalidated string at the boundary while every f-string, dict key and httpx call keeps working. It replaces three spellings of one concept: base_url: str, bfabric_instance: str, and EntityUriComponents.bfabric_instance: HttpUrl -- the last of which forced the str() coercion in uri.py that sat on the exact seam #576 broke. Verified the type checker actually catches it before migrating: annotating one function surfaced two real call sites, so neither the position-independent baselines nor the neighbouring inline ignores absorb the error. Two consequences worth noting: - yaml.dump would serialise a str subclass as `!!python/object/new:`, silently writing a config that no longer loads. config_writer now uses safe_dump, which rejects it loudly instead; the two CLI write paths coerce to str. - BaseUrl raises a plain ValueError rather than a pydantic ValidationError, so the CLI can print it directly. Pydantic still wraps it on a model field. Also fixes a latent bug in validate_token, which compared the server's caller against the configured instances with no canonicalisation on either side. --- bfabric/docs/changelog.md | 5 +-- bfabric/src/bfabric/__init__.py | 2 ++ .../src/bfabric/_oauth/credential_provider.py | 6 ++-- bfabric/src/bfabric/_oauth/device_code.py | 9 +++-- bfabric/src/bfabric/_oauth/pkce.py | 8 ++--- bfabric/src/bfabric/_oauth/registration.py | 5 ++- bfabric/src/bfabric/_oauth/token_cache.py | 7 ++-- bfabric/src/bfabric/_oauth/token_exchange.py | 9 +++-- bfabric/src/bfabric/_oauth/url_token.py | 7 ++-- bfabric/src/bfabric/_oauth/webapp_client.py | 7 ++-- bfabric/src/bfabric/bfabric.py | 29 ++++++--------- bfabric/src/bfabric/config/__init__.py | 4 +-- bfabric/src/bfabric/config/base_url.py | 30 +++++++--------- .../bfabric/config/bfabric_client_config.py | 12 +++---- bfabric/src/bfabric/config/config_writer.py | 2 ++ bfabric/src/bfabric/engine/engine_suds.py | 5 ++- bfabric/src/bfabric/engine/engine_zeep.py | 5 ++- bfabric/src/bfabric/entities/core/entity.py | 11 +++--- .../bfabric/entities/core/entity_reader.py | 35 +++++++++---------- .../bfabric/entities/core/import_entity.py | 7 ++-- .../src/bfabric/entities/core/references.py | 13 ++++--- bfabric/src/bfabric/entities/core/uri.py | 10 +++--- bfabric/src/bfabric/entities/core/users.py | 7 ++-- bfabric/src/bfabric/rest/token_data.py | 11 +++--- .../src/bfabric_scripts/cli/login/_common.py | 9 ++--- .../src/bfabric_scripts/cli/login/_urls.py | 27 +++++--------- .../bfabric_scripts/cli/login/oauth_login.py | 4 +-- tests/bfabric/config/test_backward_compat.py | 1 - tests/bfabric/config/test_base_url.py | 30 +++++++++------- tests/bfabric/config/test_config_writer.py | 10 ++++++ tests/bfabric/test_bfabric.py | 4 +-- .../cli/login/test_cmd_auth_login.py | 2 +- 32 files changed, 153 insertions(+), 180 deletions(-) diff --git a/bfabric/docs/changelog.md b/bfabric/docs/changelog.md index 1a6274850..f3c838d88 100644 --- a/bfabric/docs/changelog.md +++ b/bfabric/docs/changelog.md @@ -29,8 +29,9 @@ Minor breaking changes are still possible in `1.X.Y` but we try to announce them - `create_workunit` accepts a plain mapping for `params`, validated internally so an invalid mapping raises `ValidationError` before any write. - **Breaking: `BfabricClientConfig.base_url` is canonicalised *without* a trailing slash**, reversing the 1.15.0 "always ends with exactly one `/`". Config files and `connect_*` arguments still accept one. - `Entity.bfabric_instance` and `EntityUri.components.bfabric_instance` follow the same form; `EntityUri` strings are unchanged. -- `connect_oauth` / `connect_pkce` / `connect_device_code` / `connect_pat` and `WebappClient.create` canonicalise `base_url` through the new `CanonicalBaseUrl`, so a host with mixed case or a default port is normalised too, and a non-HTTP URL is rejected up front. -- **Breaking: new `bfabric.config.CanonicalBaseUrl`**, a `str` subclass holding a validated slash-free instance URL. `BfabricClientConfig.base_url`, `Entity.bfabric_instance` and `EntityUriComponents.bfabric_instance` (was a pydantic `HttpUrl`) are now this type, and `EntityReader`'s `bfabric_instance` arguments require it. It behaves as a `str` everywhere, so interpolation, comparison and dict keys are unaffected; constructing `BfabricClientConfig` directly now needs `CanonicalBaseUrl(...)`, while `model_validate` still accepts a plain string. +- `connect_oauth` / `connect_pkce` / `connect_device_code` / `connect_pat` and `WebappClient.create` canonicalise `base_url` through the new `BaseUrl`, so a host with mixed case or a default port is normalised too, and a non-HTTP URL is rejected up front with a plain `ValueError`. +- **Breaking: new `bfabric.BaseUrl`**, a `str` subclass holding a validated slash-free instance URL. `BfabricClientConfig.base_url`, `Entity.bfabric_instance` and `EntityUriComponents.bfabric_instance` (was a pydantic `HttpUrl`) are now this type, and `EntityReader`'s `bfabric_instance` arguments require it. +- It behaves as a `str` everywhere, so interpolation, comparison and dict keys are unaffected. Constructing `BfabricClientConfig` directly now needs `BaseUrl(...)`; `model_validate` still accepts a plain string. - `bfabric-cli auth login` / `auth pat` write the config with `yaml.safe_dump`, matching the `safe_load` used to read it back. ### Fixed diff --git a/bfabric/src/bfabric/__init__.py b/bfabric/src/bfabric/__init__.py index 91671b709..6016a589a 100644 --- a/bfabric/src/bfabric/__init__.py +++ b/bfabric/src/bfabric/__init__.py @@ -1,10 +1,12 @@ import importlib.metadata from bfabric.bfabric import Bfabric +from bfabric.config.base_url import BaseUrl from bfabric.config.bfabric_auth import BfabricAuth from bfabric.config.bfabric_client_config import BfabricAPIEngineType, BfabricClientConfig __all__ = [ + "BaseUrl", "Bfabric", "BfabricAPIEngineType", "BfabricAuth", diff --git a/bfabric/src/bfabric/_oauth/credential_provider.py b/bfabric/src/bfabric/_oauth/credential_provider.py index 078d21d68..8f8cce2b4 100644 --- a/bfabric/src/bfabric/_oauth/credential_provider.py +++ b/bfabric/src/bfabric/_oauth/credential_provider.py @@ -31,7 +31,7 @@ from bfabric._oauth.token_cache import TokenCache, compute_token_cache_path if TYPE_CHECKING: - from bfabric.config.base_url import CanonicalBaseUrl + from bfabric.config.base_url import BaseUrl from pathlib import Path @@ -121,9 +121,7 @@ def __init__( self._persist() @classmethod - def cache_login_token( - cls, base_url: CanonicalBaseUrl, *, client_id: str, token: dict[str, object], env_name: str - ) -> Path: + def cache_login_token(cls, base_url: BaseUrl, *, client_id: str, token: dict[str, object], env_name: str) -> Path: """Normalize and cache a freshly obtained login *token*, returning its cache path. Ingesting the token derives its absolute ``expires_at`` (from ``expires_in``) and writes the diff --git a/bfabric/src/bfabric/_oauth/device_code.py b/bfabric/src/bfabric/_oauth/device_code.py index 8b634b2de..888311c80 100644 --- a/bfabric/src/bfabric/_oauth/device_code.py +++ b/bfabric/src/bfabric/_oauth/device_code.py @@ -22,11 +22,11 @@ from bfabric.errors import BfabricOAuthError if TYPE_CHECKING: - from bfabric.config.base_url import CanonicalBaseUrl + from bfabric.config.base_url import BaseUrl def _request_device_code( - base_url: CanonicalBaseUrl, + base_url: BaseUrl, *, client_id: str, scope: str, @@ -55,7 +55,7 @@ def _request_device_code( def _poll_for_token( - base_url: CanonicalBaseUrl, + base_url: BaseUrl, *, device_code: str, client_id: str, @@ -140,7 +140,7 @@ def _poll_for_token( def device_code_login( - base_url: CanonicalBaseUrl, + base_url: BaseUrl, *, client_id: str, scope: str, @@ -151,7 +151,6 @@ def device_code_login( Requests a device code, displays the user code and verification URI, then polls until the user authorizes or the request times out. - :param base_url: B-Fabric instance URL (e.g. ``https://bfabric.example.com/bfabric``) :param client_id: OAuth client ID :param scope: OAuth scope :param timeout: Seconds to wait for the user to authorize diff --git a/bfabric/src/bfabric/_oauth/pkce.py b/bfabric/src/bfabric/_oauth/pkce.py index 1354e4a18..74d85e175 100644 --- a/bfabric/src/bfabric/_oauth/pkce.py +++ b/bfabric/src/bfabric/_oauth/pkce.py @@ -14,8 +14,8 @@ import webbrowser from dataclasses import dataclass from http.server import BaseHTTPRequestHandler, HTTPServer -from urllib.parse import parse_qs, urlencode, urlparse from typing import TYPE_CHECKING +from urllib.parse import parse_qs, urlencode, urlparse import httpx from loguru import logger @@ -23,8 +23,7 @@ from bfabric.errors import BfabricOAuthError if TYPE_CHECKING: - from bfabric.config.base_url import CanonicalBaseUrl - + from bfabric.config.base_url import BaseUrl _REMOTE_HOST_CAVEAT = "On a remote host, use 'bfabric-cli auth device-code' instead." @@ -170,7 +169,7 @@ def _exchange_code( def pkce_login( - base_url: CanonicalBaseUrl, + base_url: BaseUrl, *, client_id: str, scope: str, @@ -180,7 +179,6 @@ def pkce_login( ) -> dict[str, object]: """Perform an OAuth 2.0 Authorization Code flow with PKCE. - :param base_url: B-Fabric instance URL (e.g. ``https://bfabric.example.com/bfabric``) :param port: Local port for the callback server (``0`` = auto-assign) :param open_browser: If ``False``, or if the browser fails to open, the URL is printed to stderr :param timeout: Seconds to wait for the user to complete login diff --git a/bfabric/src/bfabric/_oauth/registration.py b/bfabric/src/bfabric/_oauth/registration.py index 3bb6c27c1..7d8f88776 100644 --- a/bfabric/src/bfabric/_oauth/registration.py +++ b/bfabric/src/bfabric/_oauth/registration.py @@ -9,7 +9,7 @@ if TYPE_CHECKING: from bfabric.bfabric import Bfabric - from bfabric.config.base_url import CanonicalBaseUrl + from bfabric.config.base_url import BaseUrl from bfabric.results.result_container import ResultContainer from bfabric.typing import ApiRequestDataType @@ -33,7 +33,7 @@ def _default_grant_types(service_user: str | None) -> list[str]: def register_client( - base_url: CanonicalBaseUrl, + base_url: BaseUrl, token: str, client_name: str, redirect_uri: str, @@ -50,7 +50,6 @@ def register_client( ``token-exchange``, ``refresh_token`` and ``authorization_code`` always, plus ``client_credentials`` when *service_user* is provided. Pass *grant_types* to override. - :param base_url: B-Fabric instance URL (e.g. ``https://bfabric.example.com/bfabric``) :param token: Employee Bearer token for authorization :param client_name: Human-readable name for the client :param redirect_uri: OAuth redirect URI for the client diff --git a/bfabric/src/bfabric/_oauth/token_cache.py b/bfabric/src/bfabric/_oauth/token_cache.py index d52e344c5..9da3ac880 100644 --- a/bfabric/src/bfabric/_oauth/token_cache.py +++ b/bfabric/src/bfabric/_oauth/token_cache.py @@ -11,18 +11,15 @@ from loguru import logger if TYPE_CHECKING: - from bfabric.config.base_url import CanonicalBaseUrl + from bfabric.config.base_url import BaseUrl -def compute_token_cache_path(base_url: CanonicalBaseUrl, client_id: str, env_name: str) -> Path: +def compute_token_cache_path(base_url: BaseUrl, client_id: str, env_name: str) -> Path: """Return the default token cache path for a given base URL, client ID, and environment name. The path is ``~/.bfabric/tokens/{hash}.json`` where *hash* is the first 16 hex characters of the SHA-256 digest of ``base_url + '\\0' + client_id + '\\0' + env_name``. This ensures different identities on the same server get separate caches. - - *base_url* must be canonical (as produced by ``BfabricClientConfig``), or the key will not match - the cache the CLI wrote. """ key = base_url + "\0" + client_id + "\0" + env_name url_hash = hashlib.sha256(key.encode()).hexdigest()[:16] diff --git a/bfabric/src/bfabric/_oauth/token_exchange.py b/bfabric/src/bfabric/_oauth/token_exchange.py index 3e0b6e5d6..4179e277f 100644 --- a/bfabric/src/bfabric/_oauth/token_exchange.py +++ b/bfabric/src/bfabric/_oauth/token_exchange.py @@ -7,6 +7,7 @@ from __future__ import annotations + from typing import TYPE_CHECKING import httpx @@ -15,11 +16,11 @@ from bfabric._oauth.url_token import UrlTokenContext if TYPE_CHECKING: - from bfabric.config.base_url import CanonicalBaseUrl + from bfabric.config.base_url import BaseUrl def exchange_token( - base_url: CanonicalBaseUrl, + base_url: BaseUrl, launch_token: str, *, client_id: str, @@ -30,7 +31,6 @@ def exchange_token( POSTs to ``{base_url}/rest/oauth/token`` with ``grant_type=urn:ietf:params:oauth:grant-type:token-exchange``. - :param base_url: B-Fabric instance URL (e.g. ``https://bfabric.example.com/bfabric``) :param launch_token: The short-lived JWT from the launch URL :param client_id: OAuth client ID for the webapp :param client_secret: OAuth client secret for the webapp @@ -57,7 +57,7 @@ def exchange_token( def introspect_token( - base_url: CanonicalBaseUrl, + base_url: BaseUrl, access_token: str, *, client_id: str, @@ -68,7 +68,6 @@ def introspect_token( POSTs to ``{base_url}/rest/oauth/introspect`` using ``client_secret_basic`` auth and returns a :class:`UrlTokenContext` with the extracted claims. - :param base_url: B-Fabric instance URL :param access_token: The access token to introspect :param client_id: OAuth client ID for the webapp :param client_secret: OAuth client secret for the webapp diff --git a/bfabric/src/bfabric/_oauth/url_token.py b/bfabric/src/bfabric/_oauth/url_token.py index a70152d2d..a4d21a0a5 100644 --- a/bfabric/src/bfabric/_oauth/url_token.py +++ b/bfabric/src/bfabric/_oauth/url_token.py @@ -14,7 +14,7 @@ from pydantic import BaseModel, ConfigDict, Field if TYPE_CHECKING: - from bfabric.config.base_url import CanonicalBaseUrl + from bfabric.config.base_url import BaseUrl class UrlTokenContext(BaseModel): @@ -58,7 +58,7 @@ def is_employee(self) -> bool: _JWKS_CACHE_TTL = 3600 # 1 hour -def _fetch_jwks(base_url: CanonicalBaseUrl) -> dict[str, object]: +def _fetch_jwks(base_url: BaseUrl) -> dict[str, object]: """Fetch (and cache for 1 hour) the JWKS from the B-Fabric server.""" now = time.time() with _jwks_lock: @@ -79,10 +79,9 @@ def _fetch_jwks(base_url: CanonicalBaseUrl) -> dict[str, object]: return jwks -def verify_jwt(base_url: CanonicalBaseUrl, token: str) -> dict[str, object]: +def verify_jwt(base_url: BaseUrl, token: str) -> dict[str, object]: """Verify the JWT signature + expiry against the B-Fabric JWKS endpoint. - :param base_url: B-Fabric instance URL (e.g. ``https://bfabric.example.com/bfabric``) :param token: The raw JWT string :returns: The verified claims dictionary :raises: ``joserfc.errors.JoseError`` subclasses on invalid/expired tokens diff --git a/bfabric/src/bfabric/_oauth/webapp_client.py b/bfabric/src/bfabric/_oauth/webapp_client.py index ac92fe773..f9a8ee800 100644 --- a/bfabric/src/bfabric/_oauth/webapp_client.py +++ b/bfabric/src/bfabric/_oauth/webapp_client.py @@ -43,7 +43,6 @@ def create( (from the URL) into long-lived access + refresh tokens, then decodes the access token JWT locally to extract entity context. - :param base_url: B-Fabric instance URL (e.g. ``https://bfabric.example.com/bfabric``) :param launch_token: The short-lived JWT from the URL ``jwt`` parameter :param client_id: OAuth client ID for the webapp :param client_secret: OAuth client secret for the webapp @@ -55,10 +54,10 @@ def create( from bfabric._oauth.credential_provider import OAuthCredentialProvider from bfabric._oauth.token_exchange import exchange_token from bfabric._oauth.url_token import UrlTokenContext, verify_jwt - from bfabric.config import BfabricClientConfig, CanonicalBaseUrl + from bfabric.config import BfabricClientConfig, BaseUrl from bfabric.config.config_data import ConfigData - base_url = CanonicalBaseUrl(base_url) + base_url = BaseUrl(base_url) token_url = f"{base_url}/rest/oauth/token" # 1. Exchange the short-lived launch token for access + refresh tokens @@ -83,7 +82,7 @@ def create( grant_type="refresh_token", token_cache_path=user_token_cache_path, ) - config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] + config = BfabricClientConfig(base_url=base_url) user_client = Bfabric( config_data=ConfigData(client=config, auth=None), _credential_provider=user_provider, diff --git a/bfabric/src/bfabric/bfabric.py b/bfabric/src/bfabric/bfabric.py index 1304d75c5..7527f2e81 100644 --- a/bfabric/src/bfabric/bfabric.py +++ b/bfabric/src/bfabric/bfabric.py @@ -28,7 +28,7 @@ from loguru import logger from rich.console import Console -from bfabric.config import DEFAULT_CONFIG_FILE, BfabricAuth, BfabricClientConfig, CanonicalBaseUrl +from bfabric.config import DEFAULT_CONFIG_FILE, BfabricAuth, BfabricClientConfig, BaseUrl from bfabric.config.bfabric_client_config import BfabricAPIEngineType from bfabric.config.config_data import ConfigData, load_config_data from bfabric.config.config_file import read_config_file @@ -253,13 +253,12 @@ def connect_oauth( :param client_id: OAuth client ID (from ``register_client`` or admin setup) :param client_secret: OAuth client secret - :param base_url: B-Fabric instance URL (e.g. ``https://bfabric.example.com/bfabric``) :param scope: OAuth scope :param token_cache_path: Optional path to cache tokens on disk (survives restarts) """ from bfabric._oauth.credential_provider import OAuthCredentialProvider - base_url = CanonicalBaseUrl(base_url) + base_url = BaseUrl(base_url) token_url = f"{base_url}/rest/oauth/token" provider = OAuthCredentialProvider( client_id=client_id, @@ -269,8 +268,7 @@ def connect_oauth( grant_type="client_credentials", token_cache_path=token_cache_path, ) - config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] - config_data = ConfigData(client=config, auth=None) + config_data = ConfigData(client=BfabricClientConfig(base_url=base_url), auth=None) return cls(config_data=config_data, _credential_provider=provider) @classmethod @@ -291,7 +289,6 @@ def connect_pkce( the user logs in, tokens are exchanged automatically and the returned client uses :class:`OAuthCredentialProvider` for transparent refresh. - :param base_url: B-Fabric instance URL (e.g. ``https://bfabric.example.com/bfabric``) :param client_id: OAuth client ID :param scope: OAuth scope :param port: Local port for the callback server (``0`` = auto-assign) @@ -302,7 +299,7 @@ def connect_pkce( from bfabric._oauth.credential_provider import OAuthCredentialProvider from bfabric._oauth.pkce import pkce_login - base_url = CanonicalBaseUrl(base_url) + base_url = BaseUrl(base_url) token = pkce_login( base_url, client_id=client_id, @@ -321,8 +318,7 @@ def connect_pkce( scope=scope, token_cache_path=token_cache_path, ) - config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] - config_data = ConfigData(client=config, auth=None) + config_data = ConfigData(client=BfabricClientConfig(base_url=base_url), auth=None) return cls(config_data=config_data, _credential_provider=provider) @classmethod @@ -345,7 +341,6 @@ def connect_device_code( This flow is suitable for headless environments (SSH, containers) where a localhost redirect is not feasible. - :param base_url: B-Fabric instance URL (e.g. ``https://bfabric.example.com/bfabric``) :param client_id: OAuth client ID :param scope: OAuth scope :param timeout: Seconds to wait for the user to authorize @@ -354,7 +349,7 @@ def connect_device_code( from bfabric._oauth.credential_provider import OAuthCredentialProvider from bfabric._oauth.device_code import device_code_login - base_url = CanonicalBaseUrl(base_url) + base_url = BaseUrl(base_url) token = device_code_login( base_url, client_id=client_id, @@ -371,8 +366,7 @@ def connect_device_code( scope=scope, token_cache_path=token_cache_path, ) - config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] - config_data = ConfigData(client=config, auth=None) + config_data = ConfigData(client=BfabricClientConfig(base_url=base_url), auth=None) return cls(config_data=config_data, _credential_provider=provider) @classmethod @@ -388,17 +382,16 @@ def connect_pat( API accepts them directly. There is no automatic refresh; if the token expires a new one must be obtained. - :param base_url: B-Fabric instance URL (e.g. ``https://bfabric.example.com/bfabric``) :param pat: Personal Access Token (string or ``SecretStr``) """ from pydantic import SecretStr from bfabric.config.bfabric_auth import OAUTH_LOGIN - base_url = CanonicalBaseUrl(base_url) + base_url = BaseUrl(base_url) pat_value: str = pat.get_secret_value() if isinstance(pat, SecretStr) else pat auth = BfabricAuth(login=OAUTH_LOGIN, password=SecretStr(pat_value)) - config = BfabricClientConfig(base_url=base_url) # pyright: ignore[reportCallIssue] + config = BfabricClientConfig(base_url=base_url) config_data = ConfigData(client=config, auth=auth) return cls(config_data=config_data) @@ -674,7 +667,7 @@ def get_system_auth( ) resolved_path = Path(config_path or "~/.bfabricpy.yml").expanduser() - canonical_base_url = CanonicalBaseUrl(base_url) if base_url is not None else None + canonical_base_url = BaseUrl(base_url) if base_url is not None else None # Use the provided config data from arguments instead of the file if not resolved_path.is_file(): @@ -682,7 +675,7 @@ def get_system_auth( # NOTE: If user explicitly specifies a path to a wrong config file, this has to be an exception raise OSError(f"Explicitly specified config file does not exist: {resolved_path}") logger.warning(f"could not find the config file in the default location: {resolved_path}") - config = BfabricClientConfig(base_url=canonical_base_url) # pyright: ignore[reportCallIssue] + config = BfabricClientConfig(base_url=canonical_base_url) # pyright: ignore[reportArgumentType] auth = None if login is None or password is None else BfabricAuth(login=login, password=password) # Load config from file, override some of the fields with the provided ones diff --git a/bfabric/src/bfabric/config/__init__.py b/bfabric/src/bfabric/config/__init__.py index f19706469..18451baf1 100644 --- a/bfabric/src/bfabric/config/__init__.py +++ b/bfabric/src/bfabric/config/__init__.py @@ -1,6 +1,6 @@ -from .base_url import CanonicalBaseUrl +from .base_url import BaseUrl from .bfabric_auth import BfabricAuth from .bfabric_client_config import BfabricClientConfig from .config_file import ConfigFile, DEFAULT_CONFIG_FILE -__all__ = ["BfabricAuth", "BfabricClientConfig", "CanonicalBaseUrl", "ConfigFile", "DEFAULT_CONFIG_FILE"] +__all__ = ["BfabricAuth", "BfabricClientConfig", "BaseUrl", "ConfigFile", "DEFAULT_CONFIG_FILE"] diff --git a/bfabric/src/bfabric/config/base_url.py b/bfabric/src/bfabric/config/base_url.py index 4bf2f836a..f363a25d4 100644 --- a/bfabric/src/bfabric/config/base_url.py +++ b/bfabric/src/bfabric/config/base_url.py @@ -1,32 +1,28 @@ -"""The canonical form of a B-Fabric instance base URL. - -Kept in a leaf module importing only pydantic, so the entity layer can depend on it without -introducing an import cycle. -""" +"""The type of a B-Fabric instance base URL.""" from __future__ import annotations -from pydantic import AnyHttpUrl, GetCoreSchemaHandler, TypeAdapter +from pydantic import AnyHttpUrl, GetCoreSchemaHandler, TypeAdapter, ValidationError from pydantic_core import core_schema -class CanonicalBaseUrl(str): - """A validated B-Fabric instance base URL, canonicalised without a trailing slash. - - A ``str`` subclass rather than a wrapper model: the value is interpolated into request URLs and - used as a cache key, so it has to behave like a string everywhere. Annotating a parameter with it - is what lets the type checker reject a base URL that never passed through canonicalisation -- - ``Annotated[str, AfterValidator(...)]`` validates at runtime but is indistinguishable from ``str``. +class BaseUrl(str): + """A validated B-Fabric instance base URL, without a trailing slash, e.g. ``https://x.uzh.ch/bfabric``. - Construction is idempotent and total, which is what a cache key needs. + A ``str`` subclass, because the value is interpolated into request URLs and used as a cache key -- + while still being a type the checker can tell apart from a string that never passed through here. + Construction is idempotent, which is what a cache key needs. """ - def __new__(cls, value: str) -> CanonicalBaseUrl: + def __new__(cls, value: str) -> BaseUrl: if isinstance(value, cls): return value - http_url = TypeAdapter(AnyHttpUrl).validate_python(value) + try: + url = TypeAdapter(AnyHttpUrl).validate_python(value) + except ValidationError as error: + raise ValueError(f"Not a valid http(s) URL: {value!r}") from error # The strip has to come after validation: AnyHttpUrl re-adds the slash for an empty path. - return super().__new__(cls, str(http_url).rstrip("/")) + return super().__new__(cls, str(url).rstrip("/")) @classmethod def __get_pydantic_core_schema__(cls, source_type: object, handler: GetCoreSchemaHandler) -> core_schema.CoreSchema: diff --git a/bfabric/src/bfabric/config/bfabric_client_config.py b/bfabric/src/bfabric/config/bfabric_client_config.py index 468ba3df0..2d6427cc7 100644 --- a/bfabric/src/bfabric/config/bfabric_client_config.py +++ b/bfabric/src/bfabric/config/bfabric_client_config.py @@ -1,11 +1,10 @@ from __future__ import annotations from enum import StrEnum -from typing import Annotated from pydantic import BaseModel, Field -from bfabric.config.base_url import CanonicalBaseUrl +from bfabric.config.base_url import BaseUrl class BfabricAPIEngineType(StrEnum): @@ -21,20 +20,19 @@ def __repr__(self) -> str: class BfabricClientConfig(BaseModel): """Holds the configuration for the B-Fabric client for connecting to particular instance of B-Fabric. - :param base_url: The API base url :param application_ids (optional): Map of application names to ids. :param job_notification_emails (optional): Space-separated list of email addresses to notify when a job finishes. :param engine: The API engine to use (optional). """ - base_url: CanonicalBaseUrl - application_ids: Annotated[dict[str, int], Field(default_factory=dict)] - job_notification_emails: Annotated[str, Field(default="")] + base_url: BaseUrl + application_ids: dict[str, int] = Field(default_factory=dict) + job_notification_emails: str = "" engine: BfabricAPIEngineType = BfabricAPIEngineType.SUDS def copy_with( self, - base_url: CanonicalBaseUrl | None = None, + base_url: BaseUrl | None = None, application_ids: dict[str, int] | None = None, engine: BfabricAPIEngineType | None = None, ) -> BfabricClientConfig: diff --git a/bfabric/src/bfabric/config/config_writer.py b/bfabric/src/bfabric/config/config_writer.py index c739dad0d..8f4d4b6a1 100644 --- a/bfabric/src/bfabric/config/config_writer.py +++ b/bfabric/src/bfabric/config/config_writer.py @@ -1,6 +1,8 @@ """Write environment entries to the bfabricpy YAML config file. Note: rewriting the file drops any YAML comments in it (``yaml.safe_dump`` doesn't preserve them). +Values must be plain scalars: ``safe_dump`` rejects a ``str`` subclass such as ``BaseUrl`` outright, +whereas ``yaml.dump`` would quietly write a ``!!python/object/new:`` tag that no longer loads. """ from __future__ import annotations diff --git a/bfabric/src/bfabric/engine/engine_suds.py b/bfabric/src/bfabric/engine/engine_suds.py index ece812736..2655960b2 100644 --- a/bfabric/src/bfabric/engine/engine_suds.py +++ b/bfabric/src/bfabric/engine/engine_suds.py @@ -16,15 +16,14 @@ if TYPE_CHECKING: from suds.serviceproxy import ServiceProxy - from bfabric.config import BfabricAuth - from bfabric.config.base_url import CanonicalBaseUrl + from bfabric.config import BaseUrl, BfabricAuth from bfabric.typing import ApiRequestObjectType class EngineSUDS: """B-Fabric API SUDS Engine.""" - def __init__(self, base_url: CanonicalBaseUrl, drop_underscores: bool = True) -> None: + def __init__(self, base_url: BaseUrl, drop_underscores: bool = True) -> None: self._cl = {} self._base_url = base_url self._drop_underscores = drop_underscores diff --git a/bfabric/src/bfabric/engine/engine_zeep.py b/bfabric/src/bfabric/engine/engine_zeep.py index fdae43526..69142a2e9 100644 --- a/bfabric/src/bfabric/engine/engine_zeep.py +++ b/bfabric/src/bfabric/engine/engine_zeep.py @@ -13,15 +13,14 @@ from bfabric.results.result_container import ResultContainer if TYPE_CHECKING: - from bfabric.config import BfabricAuth - from bfabric.config.base_url import CanonicalBaseUrl + from bfabric.config import BaseUrl, BfabricAuth from bfabric.typing import ApiRequestObjectType class EngineZeep: """B-Fabric API Zeep Engine""" - def __init__(self, base_url: CanonicalBaseUrl) -> None: + def __init__(self, base_url: BaseUrl) -> None: self._cl = {} self._base_url = base_url diff --git a/bfabric/src/bfabric/entities/core/entity.py b/bfabric/src/bfabric/entities/core/entity.py index 14d4744c1..f67edc8dd 100644 --- a/bfabric/src/bfabric/entities/core/entity.py +++ b/bfabric/src/bfabric/entities/core/entity.py @@ -13,8 +13,7 @@ from pathlib import Path from typing import Any - from bfabric import Bfabric - from bfabric.config.base_url import CanonicalBaseUrl + from bfabric import BaseUrl, Bfabric from bfabric.entities.core.references import References from bfabric.typing import ApiResponseDataType, ApiResponseObjectType @@ -26,7 +25,7 @@ def __init__( self, data_dict: ApiResponseObjectType, client: Bfabric | None = None, - bfabric_instance: CanonicalBaseUrl | None = None, + bfabric_instance: BaseUrl | None = None, ) -> None: # note: client may be removed completely in the future, # as I think it is a design mistake to have put them into these classes @@ -52,7 +51,7 @@ def id(self) -> int: return value @property - def bfabric_instance(self) -> CanonicalBaseUrl: + def bfabric_instance(self) -> BaseUrl: """The bfabric instance URL associated with the entity.""" return self.__bfabric_instance @@ -149,9 +148,7 @@ def dump_yaml(self, path: Path) -> None: yaml.safe_dump(self.__data_dict, file) @classmethod - def load_yaml( - cls, path: Path, client: Bfabric | None = None, bfabric_instance: CanonicalBaseUrl | None = None - ) -> Self: + def load_yaml(cls, path: Path, client: Bfabric | None = None, bfabric_instance: BaseUrl | None = None) -> Self: """Loads an entity from a YAML file.""" # TODO (#351): to be extended import yaml diff --git a/bfabric/src/bfabric/entities/core/entity_reader.py b/bfabric/src/bfabric/entities/core/entity_reader.py index bd1e26629..d9a39e52d 100644 --- a/bfabric/src/bfabric/entities/core/entity_reader.py +++ b/bfabric/src/bfabric/entities/core/entity_reader.py @@ -13,8 +13,7 @@ if TYPE_CHECKING: from collections.abc import Iterable, Sequence - from bfabric import Bfabric - from bfabric.config.base_url import CanonicalBaseUrl + from bfabric import BaseUrl, Bfabric from bfabric.typing import ApiRequestObjectType, ApiResponseDataType, ApiResponseObjectType @@ -145,26 +144,26 @@ def read_uris( @overload def read_id( - self, entity_type: type[EntityT], entity_id: int | str, bfabric_instance: CanonicalBaseUrl | None = None + self, entity_type: type[EntityT], entity_id: int | str, bfabric_instance: BaseUrl | None = None ) -> EntityT | None: ... @overload def read_id( self, entity_type: str, entity_id: int | str, - bfabric_instance: CanonicalBaseUrl | None = None, + bfabric_instance: BaseUrl | None = None, *, expected_type: type[EntityT], ) -> EntityT | None: ... @overload def read_id( - self, entity_type: str, entity_id: int | str, bfabric_instance: CanonicalBaseUrl | None = None + self, entity_type: str, entity_id: int | str, bfabric_instance: BaseUrl | None = None ) -> Entity | None: ... def read_id( self, entity_type: str | type[EntityT], entity_id: int | str, - bfabric_instance: CanonicalBaseUrl | None = None, + bfabric_instance: BaseUrl | None = None, *, expected_type: type[EntityT] = Entity, ) -> EntityT | None: @@ -197,26 +196,26 @@ def read_ids( self, entity_type: type[EntityT], entity_ids: Sequence[int | str], - bfabric_instance: CanonicalBaseUrl | None = None, + bfabric_instance: BaseUrl | None = None, ) -> EntityResult[EntityT]: ... @overload def read_ids( self, entity_type: str, entity_ids: Sequence[int | str], - bfabric_instance: CanonicalBaseUrl | None = None, + bfabric_instance: BaseUrl | None = None, *, expected_type: type[EntityT], ) -> EntityResult[EntityT]: ... @overload def read_ids( - self, entity_type: str, entity_ids: Sequence[int | str], bfabric_instance: CanonicalBaseUrl | None = None + self, entity_type: str, entity_ids: Sequence[int | str], bfabric_instance: BaseUrl | None = None ) -> EntityResult[Entity]: ... def read_ids( self, entity_type: str | type[EntityT], entity_ids: Sequence[int | str], - bfabric_instance: CanonicalBaseUrl | None = None, + bfabric_instance: BaseUrl | None = None, *, expected_type: type[EntityT] = Entity, ) -> EntityResult[EntityT]: @@ -245,7 +244,7 @@ def query( self, entity_type: type[EntityT], obj: ApiRequestObjectType, - bfabric_instance: CanonicalBaseUrl | None = None, + bfabric_instance: BaseUrl | None = None, max_results: int | None = 100, ) -> dict[EntityUri, EntityT]: ... @overload @@ -253,7 +252,7 @@ def query( self, entity_type: str, obj: ApiRequestObjectType, - bfabric_instance: CanonicalBaseUrl | None = None, + bfabric_instance: BaseUrl | None = None, max_results: int | None = 100, *, expected_type: type[EntityT], @@ -263,14 +262,14 @@ def query( self, entity_type: str, obj: ApiRequestObjectType, - bfabric_instance: CanonicalBaseUrl | None = None, + bfabric_instance: BaseUrl | None = None, max_results: int | None = 100, ) -> dict[EntityUri, Entity]: ... def query( self, entity_type: str | type[EntityT], obj: ApiRequestObjectType, - bfabric_instance: CanonicalBaseUrl | None = None, + bfabric_instance: BaseUrl | None = None, max_results: int | None = 100, *, expected_type: type[EntityT] = Entity, @@ -316,26 +315,26 @@ def query( @overload def query_one( - self, entity_type: type[EntityT], obj: ApiRequestObjectType, bfabric_instance: CanonicalBaseUrl | None = None + self, entity_type: type[EntityT], obj: ApiRequestObjectType, bfabric_instance: BaseUrl | None = None ) -> EntityT | None: ... @overload def query_one( self, entity_type: str, obj: ApiRequestObjectType, - bfabric_instance: CanonicalBaseUrl | None = None, + bfabric_instance: BaseUrl | None = None, *, expected_type: type[EntityT], ) -> EntityT | None: ... @overload def query_one( - self, entity_type: str, obj: ApiRequestObjectType, bfabric_instance: CanonicalBaseUrl | None = None + self, entity_type: str, obj: ApiRequestObjectType, bfabric_instance: BaseUrl | None = None ) -> Entity | None: ... def query_one( self, entity_type: str | type[EntityT], obj: ApiRequestObjectType, - bfabric_instance: CanonicalBaseUrl | None = None, + bfabric_instance: BaseUrl | None = None, *, expected_type: type[EntityT] = Entity, ) -> EntityT | None: diff --git a/bfabric/src/bfabric/entities/core/import_entity.py b/bfabric/src/bfabric/entities/core/import_entity.py index 0cffdac3d..6afd90fd2 100644 --- a/bfabric/src/bfabric/entities/core/import_entity.py +++ b/bfabric/src/bfabric/entities/core/import_entity.py @@ -4,8 +4,7 @@ from typing import TYPE_CHECKING if TYPE_CHECKING: - from bfabric import Bfabric - from bfabric.config.base_url import CanonicalBaseUrl + from bfabric import BaseUrl, Bfabric from bfabric.entities.core.entity import Entity from bfabric.typing import ApiResponseObjectType @@ -37,9 +36,7 @@ def entity_type_of(entity_class: type[Entity]) -> str: return entity_class.__name__.lower() -def instantiate_entity( - data_dict: ApiResponseObjectType, client: Bfabric | None, bfabric_instance: CanonicalBaseUrl -) -> Entity: +def instantiate_entity(data_dict: ApiResponseObjectType, client: Bfabric | None, bfabric_instance: BaseUrl) -> Entity: """Instantiates an entity given its data dictionary with the most specific class possible.""" entity_class_name = data_dict["classname"] if not isinstance(entity_class_name, str): diff --git a/bfabric/src/bfabric/entities/core/references.py b/bfabric/src/bfabric/entities/core/references.py index 1f31c505a..4ece27a99 100644 --- a/bfabric/src/bfabric/entities/core/references.py +++ b/bfabric/src/bfabric/entities/core/references.py @@ -11,8 +11,7 @@ from bfabric.entities.core.uri import EntityUri if TYPE_CHECKING: - from bfabric import Bfabric - from bfabric.config.base_url import CanonicalBaseUrl + from bfabric import BaseUrl, Bfabric from bfabric.entities.core.entity import Entity from bfabric.typing import ApiResponseDataType, ApiResponseObjectType @@ -35,9 +34,9 @@ class References: This class receives a reference to the entity's data dictionary, updating it in-place when references are loaded. """ - def __init__(self, client: Bfabric, bfabric_instance: CanonicalBaseUrl, data_ref: ApiResponseObjectType) -> None: + def __init__(self, client: Bfabric, bfabric_instance: BaseUrl, data_ref: ApiResponseObjectType) -> None: self._client: Bfabric = client - self._bfabric_instance: CanonicalBaseUrl = bfabric_instance + self._bfabric_instance: BaseUrl = bfabric_instance self._data_ref: ApiResponseObjectType = data_ref # Retrieve information about all reference fields @@ -123,7 +122,7 @@ def __load(self, ref_info: _ReferenceInformation) -> None: @classmethod def __extract_reference_info( - cls, data_ref: ApiResponseObjectType, bfabric_instance: CanonicalBaseUrl + cls, data_ref: ApiResponseObjectType, bfabric_instance: BaseUrl ) -> dict[str, _ReferenceInformation]: references: dict[str, _ReferenceInformation] = {} for name, value in data_ref.items(): @@ -134,7 +133,7 @@ def __extract_reference_info( @classmethod def __extract_reference_info_item( - cls, name: str, value: ApiResponseDataType, bfabric_instance: CanonicalBaseUrl + cls, name: str, value: ApiResponseDataType, bfabric_instance: BaseUrl ) -> _ReferenceInformation | None: if isinstance(value, dict) and "classname" in value and "id" in value: info = cls.__extract_reference_info_item_dict(value, bfabric_instance) @@ -157,7 +156,7 @@ def __extract_reference_info_item( @classmethod def __extract_reference_info_item_dict( - cls, value: ApiResponseDataType, bfabric_instance: CanonicalBaseUrl + cls, value: ApiResponseDataType, bfabric_instance: BaseUrl ) -> dict[str, EntityUri | bool]: # value is guaranteed to be a dict by the caller's isinstance check value_dict = cast("dict[str, ApiResponseDataType]", value) diff --git a/bfabric/src/bfabric/entities/core/uri.py b/bfabric/src/bfabric/entities/core/uri.py index 98350da15..b06c4a36b 100644 --- a/bfabric/src/bfabric/entities/core/uri.py +++ b/bfabric/src/bfabric/entities/core/uri.py @@ -14,7 +14,7 @@ ) from pydantic_core import core_schema -from bfabric.config.base_url import CanonicalBaseUrl +from bfabric.config.base_url import BaseUrl if TYPE_CHECKING: from collections.abc import Iterator @@ -61,7 +61,7 @@ def invalid(reason: str) -> ValueError: raise invalid(f"expected query exactly 'id=' and no fragment; {_NORMALIZE_HINT}") return EntityUriComponents( - bfabric_instance=CanonicalBaseUrl(f"{parsed.scheme}://{parsed.netloc.lower()}/bfabric"), + bfabric_instance=BaseUrl(f"{parsed.scheme}://{parsed.netloc.lower()}/bfabric"), entity_type=segments[1], entity_id=int(entity_id), ) @@ -99,7 +99,7 @@ def __new__(cls, uri: str | EntityUri) -> EntityUri: return instance @classmethod - def from_components(cls, bfabric_instance: CanonicalBaseUrl, entity_type: str, entity_id: int) -> EntityUri: + def from_components(cls, bfabric_instance: BaseUrl, entity_type: str, entity_id: int) -> EntityUri: """Create EntityUri from individual components. Args: @@ -151,7 +151,7 @@ class EntityUriComponents(BaseModel): entity_id: Numeric entity ID (must be positive) """ - bfabric_instance: CanonicalBaseUrl + bfabric_instance: BaseUrl entity_type: Annotated[str, StringConstraints(pattern=r"^[a-z]+$")] entity_id: Annotated[int, annotated_types.Gt(0)] @@ -171,7 +171,7 @@ class GroupKey(BaseModel): """Grouping key for EntityUris.""" model_config = ConfigDict(frozen=True) - bfabric_instance: CanonicalBaseUrl + bfabric_instance: BaseUrl entity_type: str groups: dict[GroupKey, list[EntityUri]] = {} diff --git a/bfabric/src/bfabric/entities/core/users.py b/bfabric/src/bfabric/entities/core/users.py index 85cc06d96..128f2cd91 100644 --- a/bfabric/src/bfabric/entities/core/users.py +++ b/bfabric/src/bfabric/entities/core/users.py @@ -2,9 +2,8 @@ from typing import TYPE_CHECKING - if TYPE_CHECKING: - from bfabric.config.base_url import CanonicalBaseUrl + from bfabric.config.base_url import BaseUrl from bfabric.entities.core.entity_reader import EntityReader from bfabric.entities.user import User @@ -16,7 +15,7 @@ def __init__(self, entity_reader: EntityReader) -> None: self._users = [] self._entity_reader = entity_reader - def get_by_id(self, bfabric_instance: CanonicalBaseUrl, id: int) -> User | None: + def get_by_id(self, bfabric_instance: BaseUrl, id: int) -> User | None: """Gets a user by their ID.""" # check if exists for user in self._users: @@ -32,7 +31,7 @@ def get_by_id(self, bfabric_instance: CanonicalBaseUrl, id: int) -> User | None: self._users.append(user) return user - def get_by_login(self, bfabric_instance: CanonicalBaseUrl, login: str) -> User | None: + def get_by_login(self, bfabric_instance: BaseUrl, login: str) -> User | None: """Gets a user by their login name.""" from bfabric.entities.user import User as UserEntity diff --git a/bfabric/src/bfabric/rest/token_data.py b/bfabric/src/bfabric/rest/token_data.py index f73dd2f2a..94d58b675 100644 --- a/bfabric/src/bfabric/rest/token_data.py +++ b/bfabric/src/bfabric/rest/token_data.py @@ -19,7 +19,7 @@ from pydantic import ValidationError -from bfabric.config.base_url import CanonicalBaseUrl +from bfabric.config.base_url import BaseUrl from bfabric.entities.core.import_entity import import_entity from bfabric.errors import ( BfabricInstanceNotConfiguredError, @@ -133,10 +133,9 @@ async def validate_token( token_data = await get_token_data_async( base_url=settings.validation_bfabric_instance, token=token, http_client=http_client ) - # Both sides are canonicalised before comparing: the server picks the form of ``caller``, the - # operator picks the form of the configured instances, and a trailing slash must not decide - # whether a token is accepted. The error still reports the raw value the server sent. - supported = {CanonicalBaseUrl(instance) for instance in settings.supported_bfabric_instances} - if CanonicalBaseUrl(token_data.caller) not in supported: + # Canonicalise both sides: the server picks the form of ``caller``, the operator picks the form of + # the configured instances, and a trailing slash must not decide whether a token is accepted. + supported = {BaseUrl(instance) for instance in settings.supported_bfabric_instances} + if BaseUrl(token_data.caller) not in supported: raise BfabricInstanceNotConfiguredError(token_data.caller) return token_data diff --git a/bfabric_scripts/src/bfabric_scripts/cli/login/_common.py b/bfabric_scripts/src/bfabric_scripts/cli/login/_common.py index 1aca42413..f419946a0 100644 --- a/bfabric_scripts/src/bfabric_scripts/cli/login/_common.py +++ b/bfabric_scripts/src/bfabric_scripts/cli/login/_common.py @@ -14,7 +14,7 @@ from bfabric.config.config_file import ConfigFile, EnvironmentConfig 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 CanonicalBaseUrl +from bfabric.config import BaseUrl from bfabric_scripts.cli.login._urls import KNOWN_INSTANCES, normalize_base_url # Interactive-only sentinel: choosing it opens a free-text prompt. @@ -62,12 +62,13 @@ def _pick_or_type(message: str, labels: dict[str, str], prompt: str) -> str | No return text_input(prompt) if picked == _CUSTOM else picked -def resolve_base_url(base_url: str | None, env: EnvironmentConfig | None) -> CanonicalBaseUrl | None: +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.""" if base_url is not None: return normalize_base_url(base_url) if env is not None: - return normalize_base_url(str(env.config.base_url)) + # Already canonical, having been through the config model on the way in. + return env.config.base_url if not is_interactive(): return None # First-login picker over the known instances, plus free-text entry. @@ -77,7 +78,7 @@ def resolve_base_url(base_url: str | None, env: EnvironmentConfig | None) -> Can if not picked: return None known = KNOWN_INSTANCES.get(picked) - return CanonicalBaseUrl(known) if known else normalize_base_url(picked) + return BaseUrl(known) if known else normalize_base_url(picked) def resolve_scope(scope: str | None, env: EnvironmentConfig | None = None) -> str | None: diff --git a/bfabric_scripts/src/bfabric_scripts/cli/login/_urls.py b/bfabric_scripts/src/bfabric_scripts/cli/login/_urls.py index fdc863d28..2712af37a 100644 --- a/bfabric_scripts/src/bfabric_scripts/cli/login/_urls.py +++ b/bfabric_scripts/src/bfabric_scripts/cli/login/_urls.py @@ -2,9 +2,9 @@ from __future__ import annotations -from urllib.parse import urlsplit, urlunsplit +from urllib.parse import urlsplit -from bfabric.config import CanonicalBaseUrl +from bfabric.config import BaseUrl # Suggested environment name -> instance base URL. KNOWN_INSTANCES: dict[str, str] = { @@ -24,28 +24,17 @@ def instance_host(base_url: str) -> str: _BY_HOST: dict[str, tuple[str, str]] = {instance_host(url): (name, url) for name, url in KNOWN_INSTANCES.items()} -def normalize_base_url(raw: str) -> CanonicalBaseUrl: - """Canonicalise a base URL: default the scheme to https, lowercase the host, drop a trailing - slash, and expand a bare known host to that instance's full base URL. +def normalize_base_url(raw: str) -> BaseUrl: + """Canonicalise a base URL, defaulting the scheme to https and expanding a bare known host. - :raises ValueError: If *raw* is empty or not http(s) — rejected here, not minutes later inside the + :raises ValueError: If *raw* is not an http(s) URL — rejected here, not minutes later inside the browser flow as an opaque ``httpx.InvalidURL``. """ candidate = raw.strip() - if not candidate: - raise ValueError("Base URL must not be empty.") - if "//" not in candidate: - candidate = f"https://{candidate}" - parts = urlsplit(candidate) - if parts.scheme not in ("http", "https"): - raise ValueError(f"Base URL must use http or https, got {parts.scheme!r}.") - if not parts.netloc: - raise ValueError(f"Base URL {raw!r} has no host.") - host = parts.netloc.lower() + url = BaseUrl(candidate if "//" in candidate else f"https://{candidate}") + host = instance_host(url) # Only expand a bare host: rewriting an explicit path would break an unusual deployment. - if not parts.path.strip("/") and host in _BY_HOST: - return CanonicalBaseUrl(_BY_HOST[host][1]) - return CanonicalBaseUrl(urlunsplit((parts.scheme, host, parts.path.rstrip("/"), "", ""))) + return BaseUrl(_BY_HOST[host][1]) if not urlsplit(url).path and host in _BY_HOST else url def suggest_env_name(base_url: str) -> str: diff --git a/bfabric_scripts/src/bfabric_scripts/cli/login/oauth_login.py b/bfabric_scripts/src/bfabric_scripts/cli/login/oauth_login.py index 4c1788b4e..903fd8b8a 100644 --- a/bfabric_scripts/src/bfabric_scripts/cli/login/oauth_login.py +++ b/bfabric_scripts/src/bfabric_scripts/cli/login/oauth_login.py @@ -27,7 +27,7 @@ resolve_set_default, ) from bfabric_scripts.cli.login._constants import DEFAULT_CLIENT_ID -from bfabric.config import CanonicalBaseUrl +from bfabric.config import BaseUrl from bfabric_scripts.cli.login._urls import normalize_base_url, suggest_env_name _SCOPE_HELP = ( @@ -46,7 +46,7 @@ class _LoginParams: """Everything a login needs, resolved from the command line, config, or a prompt.""" config_env: str - base_url: CanonicalBaseUrl + base_url: BaseUrl client_id: str scope: str set_default: bool diff --git a/tests/bfabric/config/test_backward_compat.py b/tests/bfabric/config/test_backward_compat.py index 06a77be75..2063b8544 100644 --- a/tests/bfabric/config/test_backward_compat.py +++ b/tests/bfabric/config/test_backward_compat.py @@ -17,7 +17,6 @@ from bfabric.config.config_file import EnvironmentConfig - # --- Vendored replica of the bfabric 1.19.0 config schema (do not "fix" to match new code) --- diff --git a/tests/bfabric/config/test_base_url.py b/tests/bfabric/config/test_base_url.py index 8fd97c50f..2a04e936c 100644 --- a/tests/bfabric/config/test_base_url.py +++ b/tests/bfabric/config/test_base_url.py @@ -4,7 +4,7 @@ import yaml from pydantic import ValidationError -from bfabric.config import BfabricClientConfig, CanonicalBaseUrl +from bfabric.config import BfabricClientConfig, BaseUrl class TestCanonicalisation: @@ -17,47 +17,53 @@ class TestCanonicalisation: ], ) def test_drops_trailing_slashes(self, raw): - assert CanonicalBaseUrl(raw) == "https://example.com/bfabric" + assert BaseUrl(raw) == "https://example.com/bfabric" def test_host_only_url_keeps_no_slash(self): # AnyHttpUrl re-adds the slash for an empty path, so the strip has to happen after validation. - assert CanonicalBaseUrl("https://example.com") == "https://example.com" + assert BaseUrl("https://example.com") == "https://example.com" def test_normalizes_host_case_and_default_port(self): - assert CanonicalBaseUrl("https://EXAMPLE.com:443/bfabric") == "https://example.com/bfabric" + assert BaseUrl("https://EXAMPLE.com:443/bfabric") == "https://example.com/bfabric" def test_is_idempotent(self): - once = CanonicalBaseUrl("https://example.com/bfabric/") - assert CanonicalBaseUrl(once) == once + once = BaseUrl("https://example.com/bfabric/") + assert BaseUrl(once) == once @pytest.mark.parametrize("raw", ["not a url", "", "ftp://example.com/bfabric"]) def test_rejects_non_http_url(self, raw): + # A plain ValueError, not a pydantic ValidationError: the CLI prints this straight to the user. + with pytest.raises(ValueError, match="Not a valid http"): + BaseUrl(raw) + + def test_rejection_surfaces_as_a_validation_error_on_a_model(self): + # Pydantic wraps a validator's ValueError, so model errors keep their usual shape. with pytest.raises(ValidationError): - CanonicalBaseUrl(raw) + BfabricClientConfig.model_validate({"base_url": "not a url"}) class TestBehavesLikeStr: """The reason this is a ``str`` subclass rather than a wrapper model.""" def test_interpolates_without_ceremony(self): - url = CanonicalBaseUrl("https://example.com/bfabric") + url = BaseUrl("https://example.com/bfabric") assert f"{url}/rest/oauth/token" == "https://example.com/bfabric/rest/oauth/token" def test_compares_and_hashes_as_str(self): - url = CanonicalBaseUrl("https://example.com/bfabric/") + url = BaseUrl("https://example.com/bfabric/") assert url == "https://example.com/bfabric" assert {url: 1}["https://example.com/bfabric"] == 1 def test_survives_pickling(self): - url = CanonicalBaseUrl("https://example.com/bfabric") + url = BaseUrl("https://example.com/bfabric") assert pickle.loads(pickle.dumps(url)) == url class TestOnTheConfigModel: def test_field_is_canonicalised(self): - config = BfabricClientConfig(base_url=CanonicalBaseUrl("https://example.com/bfabric/")) + config = BfabricClientConfig(base_url=BaseUrl("https://example.com/bfabric/")) assert config.base_url == "https://example.com/bfabric" - assert isinstance(config.base_url, CanonicalBaseUrl) + assert isinstance(config.base_url, BaseUrl) def test_model_validate_accepts_a_plain_string(self): # The config is the boundary where un-canonicalised input legitimately arrives. diff --git a/tests/bfabric/config/test_config_writer.py b/tests/bfabric/config/test_config_writer.py index 9d8cb776b..08ec59ec6 100644 --- a/tests/bfabric/config/test_config_writer.py +++ b/tests/bfabric/config/test_config_writer.py @@ -7,6 +7,7 @@ import yaml from pydantic import ValidationError +from bfabric.config import BaseUrl from bfabric.config.bfabric_auth import OAUTH_LOGIN from bfabric.config.config_file import ConfigFile from bfabric.config.config_writer import ( @@ -18,6 +19,15 @@ class TestWriteEnvironmentToConfig: + def test_refuses_a_str_subclass_rather_than_tagging_it(self, tmp_path): + """A ``BaseUrl`` must be coerced by the caller; ``yaml.dump`` would write an unloadable + ``!!python/object/new:`` tag into the user's config instead.""" + config_path = tmp_path / "config.yml" + with pytest.raises(yaml.YAMLError): + write_environment_to_config( + config_path, "PROD", {"base_url": BaseUrl("https://example.com/bfabric")}, set_default=True + ) + def test_creates_new_file(self, tmp_path): config_path = tmp_path / "config.yml" write_environment_to_config(config_path, "PROD", {"base_url": "https://example.com"}, set_default=True) diff --git a/tests/bfabric/test_bfabric.py b/tests/bfabric/test_bfabric.py index cedf7047e..1c54abd9a 100644 --- a/tests/bfabric/test_bfabric.py +++ b/tests/bfabric/test_bfabric.py @@ -6,7 +6,7 @@ from pydantic import SecretStr from bfabric import Bfabric, BfabricAPIEngineType, BfabricClientConfig, BfabricAuth -from bfabric.config import CanonicalBaseUrl +from bfabric.config import BaseUrl from bfabric.config import DEFAULT_CONFIG_FILE from bfabric.config.bfabric_auth import OAUTH_LOGIN from bfabric.config.config_data import ConfigData @@ -847,4 +847,4 @@ def test_canonical_base_url_survives_round_trip(self): config = BfabricClientConfig.model_validate({"base_url": "https://example.com/bfabric/"}) restored = pickle.loads(pickle.dumps(config)) # noqa: S301 assert restored.base_url == "https://example.com/bfabric" - assert isinstance(restored.base_url, CanonicalBaseUrl) + assert isinstance(restored.base_url, BaseUrl) diff --git a/tests/bfabric_scripts/cli/login/test_cmd_auth_login.py b/tests/bfabric_scripts/cli/login/test_cmd_auth_login.py index 358273d06..5c53f492a 100644 --- a/tests/bfabric_scripts/cli/login/test_cmd_auth_login.py +++ b/tests/bfabric_scripts/cli/login/test_cmd_auth_login.py @@ -371,5 +371,5 @@ def test_rejects_a_non_http_url(self, tmp_path, mocker, capsys): mock_pkce.assert_not_called() assert not config_file.exists() err = capsys.readouterr().err - assert "http or https" in err + assert "Not a valid http(s) URL" in err assert "Login aborted." in err