diff --git a/CHANGELOG.md b/CHANGELOG.md index dc464949..d4e6c3b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ ### Added +- Four failures a client had to tell apart by their sentence have a stable `code` (#1000, API contract 1.79.0, additive). The full async build queue answers 429 `build_queue_full` — it shared its status with `auth_throttled` and had no code. An authentication failure answers `unauthorized`, or `token_expired` when the bearer token's `exp` has passed, or 503 `auth_unavailable` when the JWKS could not be fetched. The overload 503 written to the socket before a request is read answers `server_overloaded`. Every body keeps its `error` text. A run a restart interrupted got its code, `credentials_required`, with #996. - `card.json` is declared in the contract as `DatasetCard` and states the licence mismatch and "no processing" as fields (#955, API contract 1.75.0, additive). The card sections every Gold output has carried since 1.71.0 (#694) had no schema, and two facts lived only in sentences a client had to match: `…; the provider declares: …` and `No transformation declared: values are as the source gave them.` Each `provenance` entry now also has `license_declared` (the BuildSpec's licence as written, with its link; null when none), `license_provider` (the kpubdata catalog's terms; null when none) and `license_mismatch` (both known and the provider's terms are not in the declared licence — the condition the sentence already used), and the card has `processing_declared` (false when only the no-transformation sentence is listed; a composed output's join counts as processing). The sentences are unchanged for the README. `GET /artifacts/{run_id}/{file_path}` says `gold//card.json` is a `DatasetCard` and gains an `application/json` example of one, so `contract/fixtures/responses.json` carries card fixtures. Tests validate real `card_sections` output, from the functions and from single and composed builds, against the schema, and fail when the writer sends a field the schema does not name. Cards written before 1.75.0 lack the four fields. - `GET /admin/runs` gives `total`, how many runs there are before `limit` (#948, API contract 1.73.0, additive). `count` was always the runs in the response — `len(runs)` after the `limit` cut — and is now documented as that, not a total; `total` is the build index's runs plus the queued and running jobs it does not hold yet, each counted once, with one `COUNT(*)` and a key lookup rather than reading every row (`BuildIndex.count_builds`, SQLite and CUBRID). `count < total` means older runs were left out, so an admin screen can say "N of M". A client talking to an older Builder finds `total` absent. - Saved analyses record the SQL they are written in (#875, step 14 of ADR 0021, API contract 1.76.0): `sql_dialect`, `engine`, `engine_version`, `query_contract_version` and `migration_required`. An existing analysis store gains the columns when Builder starts, and its analyses — saved for the Polars engine — are `legacy-polars`. Those are never re-run on DuckDB as if nothing had changed: `POST /analyses/{analysis_id}/run` answers 409 `analysis_migration_required` (Studio: kpubdata-lab/kpubdata-studio#565) until the SQL is reviewed and saved as a new analysis; reading and deleting them (which releases the snapshot hold) work as before. diff --git a/contract/builder-api.yaml b/contract/builder-api.yaml index e86181b0..79d6bda4 100644 --- a/contract/builder-api.yaml +++ b/contract/builder-api.yaml @@ -1,7 +1,7 @@ openapi: 3.1.0 info: title: KPubData Builder Service API - version: "1.78.0" + version: "1.79.0" description: >- KPubData Builder(패키지 `kpubdata-builder`) 서비스 계약. BuildSpec 검증, preview, build 실행, artifact 조회, 계약 버전 조회를 제공한다. CLI와 HTTP service mode(#36)가 동일한 도메인 계약을 @@ -20,6 +20,7 @@ info: 추가한다(#504, additive). v1.8.0은 stable principal별 encrypted Provider credential CRUD와 Provider status/test API를 추가한다(#492, additive). v1.9.0은 `/query` 응답에 child startup과 Polars engine 실행 시간을 추가한다(#523, additive). + v1.79.0 gives four failures a stable `code` (#1000, additive): the full async build queue's 429 is `build_queue_full` (it was told from 429 `auth_throttled` only by its sentence), an authentication failure is `unauthorized`, `token_expired` (the bearer token's `exp` has passed — get a new token) or, with 503, `auth_unavailable` (the JWKS could not be fetched), and the overload 503 written before a request is read is `server_overloaded`. Each body keeps its `error` text. v1.78.0 lets the owner read a run a restart interrupted (#996, additive): `GET /builds/{run_id}` answers such a run as `failed` with the new optional `BuildJob.code` `credentials_required` and the reason in `error`, and `GET /builds/{run_id}/events` shows its `run_failed` event, where both answered 404 before; another user still gets what a missing run gets. The submitter of an async run is now recorded with its events, so this holds for runs submitted from this version on. v1.77.0 stops a snapshot profile's range from disclosing a single record's value (#903, additive): `ColumnRange.min`/`max` are read after removing the `range_trim` lowest and the `range_trim` highest values, the range's `status` is `trimmed` instead of `exact`, and it carries `trimmed_count`. `SnapshotProfile` gains `range_trim`, and `min_range_values` is at least `2 * range_trim + 1`. A client that showed only `exact` ranges shows none until it reads `trimmed`. v1.76.0 records which SQL a saved analysis is written in (#875, additive): `SavedAnalysis` gains `sql_dialect` (`duckdb`, or `legacy-polars` for one saved before the DuckDB cutover), `engine`, `engine_version`, `query_contract_version` and `migration_required`. A legacy analysis is never re-run on DuckDB: `POST /analyses/{analysis_id}/run` answers 409 `analysis_migration_required` until its SQL is reviewed and saved as a new analysis. Existing stores gain the columns on start; their rows are the legacy ones. @@ -1454,11 +1455,17 @@ paths: schema: $ref: "#/components/schemas/Error" "429": - description: async build queue가 가득 참 + description: async build queue가 가득 참 (`build_queue_full`, #1000) content: application/json: schema: $ref: "#/components/schemas/Error" + examples: + BuildQueueFull: + summary: The async build queue is full + value: + error: async build queue is full + code: build_queue_full "500": description: job 접수 실패(event 기록/큐잉 불가 — job은 생성되지 않음) content: @@ -4184,7 +4191,10 @@ components: code: signup_rejected Unauthorized: - description: X-API-Key가 누락되었거나 서버에 설정된 키와 일치하지 않음 + description: >- + X-API-Key가 누락되었거나 서버에 설정된 키와 일치하지 않음, 또는 Bearer 토큰이 + 검증되지 않음. `code` 는 `unauthorized`, 토큰의 `exp` 가 지났으면 + `token_expired` 다(#1000). content: application/json: schema: @@ -4193,7 +4203,8 @@ components: MissingOrInvalidApiKey: summary: 인증 정보가 없거나 일치하지 않음 value: - error: unauthorized + error: invalid api key + code: unauthorized PiiDeclarationUnavailable: description: >- @@ -4408,6 +4419,13 @@ components: (`SignupNotApprovedError`, `components.responses.SignupNotApproved`). `revision_conflict`(#820, #947, 409) — a revision save or revert based on a stale revision; the body carries `current_revision` (`RevisionConflictError`). + `build_queue_full`(#1000, 429) — `POST /builds` when the async build queue + is full; try again later. `unauthorized` / `token_expired`(#1000, 401) — any + operation: the credentials were refused, or the bearer token's `exp` has + passed and a new token is needed. `auth_unavailable`(#1000, 503) — the + JWKS could not be fetched, so the credentials were not judged. + `server_overloaded`(#1000, 503) — too many requests in flight; the response + carries `Retry-After`. redistribution: description: >- `code: redistribution_forbidden`일 때만 온다(#688): 판정과 source별 이유. diff --git a/contract/fixtures/responses.json b/contract/fixtures/responses.json index a15cec92..2ff29d91 100644 --- a/contract/fixtures/responses.json +++ b/contract/fixtures/responses.json @@ -1,6 +1,6 @@ { "fixture_format": 1, - "contract_version": "1.78.0", + "contract_version": "1.79.0", "probe_field": "future_optional_field", "rules": "A response may gain optional fields in any minor version; a client ignores fields it does not know. A required field keeps its name and type until the next major version; a client rejects a body whose required field is missing or mistyped.", "fixtures": [ @@ -5097,6 +5097,30 @@ }, "broken_path": "$.error" }, + { + "operation_id": "submitBuild", + "method": "POST", + "path": "/builds", + "status": 429, + "example": "BuildQueueFull", + "current": { + "error": "async build queue is full", + "code": "build_queue_full" + }, + "with_additive_fields": { + "error": "async build queue is full", + "code": "build_queue_full", + "future_optional_field": "added by a later minor contract version" + }, + "additive_paths": [ + "$" + ], + "required_type_broken": { + "error": 12345, + "code": "build_queue_full" + }, + "broken_path": "$.error" + }, { "operation_id": "listBuilds", "method": "GET", @@ -5694,17 +5718,20 @@ "status": 401, "example": "MissingOrInvalidApiKey", "current": { - "error": "unauthorized" + "error": "invalid api key", + "code": "unauthorized" }, "with_additive_fields": { - "error": "unauthorized", + "error": "invalid api key", + "code": "unauthorized", "future_optional_field": "added by a later minor contract version" }, "additive_paths": [ "$" ], "required_type_broken": { - "error": 12345 + "error": 12345, + "code": "unauthorized" }, "broken_path": "$.error" }, diff --git a/src/kpubdata_builder/service/app.py b/src/kpubdata_builder/service/app.py index 62a821ac..c0b21356 100644 --- a/src/kpubdata_builder/service/app.py +++ b/src/kpubdata_builder/service/app.py @@ -399,7 +399,10 @@ def _enforce_ownership() -> bool: # is not disclosed (#903, additive). # 1.77.0 -> 1.78.0: a run a restart interrupted is readable by its owner — BuildJob # gains the optional `code` (`credentials_required`) (#996, additive). -API_CONTRACT_VERSION = "1.78.0" +# 1.78.0 -> 1.79.0: stable codes for the full build queue (429 build_queue_full), +# authentication failures (unauthorized, token_expired, auth_unavailable) and the +# overload 503 (server_overloaded) (#1000, additive). +API_CONTRACT_VERSION = "1.79.0" #: manifest status vocabulary (ok/failed/cancelled) → publish status vocabulary @@ -1455,7 +1458,9 @@ def _dispatch_impl( # fault). if principal.status_code == 401: service._auth_throttle.record_failure(client_id) - return ServiceResponse(principal.status_code, {"error": principal.reason}) + return ServiceResponse( + principal.status_code, {"error": principal.reason, "code": principal.code} + ) # Successful authentication clears failure record — normal client that received # a few 401s due to token expiry doesn't get throttled during subsequent normal diff --git a/src/kpubdata_builder/service/auth.py b/src/kpubdata_builder/service/auth.py index 0a205b62..83e6014d 100644 --- a/src/kpubdata_builder/service/auth.py +++ b/src/kpubdata_builder/service/auth.py @@ -172,6 +172,24 @@ class AuthError: reason: str status_code: int = 401 + # Set where the failure is made, so ``code`` does not depend on how ``reason`` is + # worded. + expired: bool = False + + @property + def code(self) -> str: + """A stable code for the failure, so a client does not branch on ``reason`` (#1000). + + ``token_expired`` — the bearer token's ``exp`` has passed: get a new token and + send the request again. ``auth_unavailable`` — the JWKS could not be fetched + (503): the credentials were not judged, try again. ``unauthorized`` — every + other refusal: a missing or wrong API key, a token that does not verify. + """ + if self.status_code == 503: + return "auth_unavailable" + if self.expired: + return "token_expired" + return "unauthorized" def _is_dev_mode() -> bool: @@ -466,7 +484,10 @@ def _verify_bearer_token(token: str) -> Principal | AuthError: options={"require": ["exp", "iat", "iss", "sub"]}, ) except jwt.PyJWTError as exc: - return AuthError(reason=f"invalid token: {type(exc).__name__}") + return AuthError( + reason=f"invalid token: {type(exc).__name__}", + expired=isinstance(exc, jwt.ExpiredSignatureError), + ) if not payload.get("email_verified", False): return AuthError(reason="email not verified") diff --git a/src/kpubdata_builder/service/build_runs_api.py b/src/kpubdata_builder/service/build_runs_api.py index b89a8205..47189b0a 100644 --- a/src/kpubdata_builder/service/build_runs_api.py +++ b/src/kpubdata_builder/service/build_runs_api.py @@ -478,7 +478,11 @@ def _record_enqueue_failure() -> None: raise RuntimeError("existing async build is missing snapshot") return ServiceResponse(200, result.snapshot.to_body()) case "queue_full": - return ServiceResponse(429, {"error": "async build queue is full"}) + # A code of its own (#1000): the other 429 on this route's way in is + # `auth_throttled`, and the sentence was the only way to tell them apart. + return ServiceResponse( + 429, {"error": "async build queue is full", "code": "build_queue_full"} + ) case unreachable: assert_never(unreachable) diff --git a/src/kpubdata_builder/service/http.py b/src/kpubdata_builder/service/http.py index ec08273a..fe28168a 100644 --- a/src/kpubdata_builder/service/http.py +++ b/src/kpubdata_builder/service/http.py @@ -79,7 +79,7 @@ def _overloaded_response(allowed_origins: frozenset[str] = frozenset()) -> bytes (a bearer header is not one). The body is a constant and says nothing a page from another origin could not learn by being refused. """ - body = b'{"error": "server overloaded"}' + body = b'{"error": "server overloaded", "code": "server_overloaded"}' cors = b"" # This is written to the socket as bytes, so a configured origin with a line break # or a non-ASCII character would corrupt the response: send no CORS header then. diff --git a/tests/unit/test_oidc_auth.py b/tests/unit/test_oidc_auth.py index d1a7b787..b0def735 100644 --- a/tests/unit/test_oidc_auth.py +++ b/tests/unit/test_oidc_auth.py @@ -8,6 +8,7 @@ import logging import time +from pathlib import Path import jwt import pytest @@ -271,6 +272,8 @@ def test_expired_token(self, oidc_env: bytes) -> None: token = _make_token(oidc_env, exp=now - 120) result = authenticate(bearer_token=f"Bearer {token}") assert isinstance(result, AuthError) + # A code of its own (#1000): the client gets a new token instead of reading the reason. + assert result.code == "token_expired" def test_wrong_audience(self, oidc_env: bytes) -> None: token = _make_token(oidc_env, aud="wrong-client") @@ -770,6 +773,57 @@ def test_non_owner_denied_via_legacy_path(self) -> None: assert not principal_owns(created_by="oidc:a", owner_id=None, principal=principal) +class TestStableAuthCodes: + """An authentication failure says what kind it is without its sentence (#1000).""" + + def test_a_refused_credential_is_unauthorized(self, oidc_env: bytes) -> None: + result = authenticate(bearer_token=f"Bearer {_make_token(oidc_env, aud='wrong-client')}") + + assert isinstance(result, AuthError) + assert (result.status_code, result.code) == (401, "unauthorized") + + def test_the_code_does_not_read_the_sentence(self) -> None: + # ``token_expired`` is set where the failure is made; rewording ``reason`` — or a + # reason that happens to end the same way — does not change the code. + assert AuthError(reason="the token is past its exp", expired=True).code == "token_expired" + assert AuthError(reason="invalid token: ExpiredSignatureError").code == "unauthorized" + + @pytest.mark.parametrize("sent", [None, "wrong-key"]) + def test_a_missing_key_and_a_wrong_key_get_the_contract_example( + self, sent: str | None, monkeypatch: pytest.MonkeyPatch + ) -> None: + # The contract's one ``Unauthorized`` example stands for both, so both must + # answer with its sentence. + monkeypatch.setenv("KPUBDATA_BUILDER_API_KEY", "secret-key") + + result = authenticate(api_key=sent) + + assert isinstance(result, AuthError) + assert (result.reason, result.code) == ("invalid api key", "unauthorized") + + def test_an_unreachable_jwks_is_auth_unavailable(self) -> None: + assert AuthError(reason="auth service unavailable (jwks)", status_code=503).code == ( + "auth_unavailable" + ) + + def test_the_response_body_carries_the_code( + self, oidc_env: bytes, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from kpubdata_builder.service import BuilderService, ServiceResponse, dispatch + + service = BuilderService(output_root=tmp_path, client_factory=lambda **_kw: None) + expired = _make_token(oidc_env, exp=int(time.time()) - 120) + + response = dispatch(service, "GET", "/datasets", None, bearer_token=f"Bearer {expired}") + + assert isinstance(response, ServiceResponse) + assert response.status_code == 401 + assert response.body == { + "error": "invalid token: ExpiredSignatureError", + "code": "token_expired", + } + + class TestKeycloakAccessToken: """What Studio sends is a Keycloak access token (kpubdata-studio#722). diff --git a/tests/unit/test_service.py b/tests/unit/test_service.py index 7af29120..ce8cd2f1 100644 --- a/tests/unit/test_service.py +++ b/tests/unit/test_service.py @@ -1716,7 +1716,7 @@ def test_overloaded_response_is_a_well_formed_http_message(self) -> None: head, _, body = _OVERLOADED_RESPONSE.partition(b"\r\n\r\n") headers = dict(line.split(b": ", 1) for line in head.split(b"\r\n")[1:]) assert int(headers[b"Content-Length"]) == len(body) - assert json.loads(body) == {"error": "server overloaded"} + assert json.loads(body) == {"error": "server overloaded", "code": "server_overloaded"} @staticmethod def _overloaded_headers(allowed: frozenset[str]) -> dict[bytes, bytes]: @@ -1725,7 +1725,7 @@ def _overloaded_headers(allowed: frozenset[str]) -> dict[bytes, bytes]: head, _, body = _overloaded_response(allowed).partition(b"\r\n\r\n") headers = dict(line.split(b": ", 1) for line in head.split(b"\r\n")[1:]) assert int(headers[b"Content-Length"]) == len(body) - assert json.loads(body) == {"error": "server overloaded"} + assert json.loads(body) == {"error": "server overloaded", "code": "server_overloaded"} return headers def test_overloaded_response_has_no_cors_header_without_an_allowed_origin(self) -> None: diff --git a/tests/unit/test_stable_error_codes.py b/tests/unit/test_stable_error_codes.py new file mode 100644 index 00000000..5da20a11 --- /dev/null +++ b/tests/unit/test_stable_error_codes.py @@ -0,0 +1,36 @@ +"""Failures a client had to tell apart by their sentence carry a stable code (#1000).""" + +from __future__ import annotations + +import json +import threading +from pathlib import Path + +from kpubdata_builder.service.http import _overloaded_response +from tests.unit.test_service_jobs import VALID_SPEC_YAML, _BlockingBuildService + + +def test_a_full_build_queue_says_so_with_a_code(tmp_path: Path) -> None: + """429 was shared with ``auth_throttled`` and had no code of its own.""" + entered = threading.Event() + release = threading.Event() + service = _BlockingBuildService( + output_root=tmp_path, entered=entered, release=release, async_max_queue_size=1 + ) + try: + service.submit_build(VALID_SPEC_YAML, run_id="run1", created_by="tester") + assert entered.wait(timeout=5) + service.submit_build(VALID_SPEC_YAML, run_id="run2", created_by="tester") + + saturated = service.submit_build(VALID_SPEC_YAML, run_id="run3", created_by="tester") + finally: + release.set() + + assert saturated.status_code == 429 + assert saturated.body == {"error": "async build queue is full", "code": "build_queue_full"} + + +def test_the_overload_response_carries_its_code() -> None: + _head, _, body = _overloaded_response().partition(b"\r\n\r\n") + + assert json.loads(body)["code"] == "server_overloaded"