From d0ea7f661d3befa9dc5d8f6c48b6b1572b00eed1 Mon Sep 17 00:00:00 2001 From: Simone Date: Sat, 25 Jul 2026 15:07:13 +0200 Subject: [PATCH] fix(web): fall back to the PAT when Gitea refuses the login token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The login token took precedence unconditionally, so a configured keyring PAT could never help while an operator was logged in — and on this instance the OIDC token (scopes `openid profile`) is exactly what Gitea refuses with a 403 for insufficient API scope. `ExecutionsView` now carries `refused` (401 or a final 403) and the web layer retries once with the PAT when the token it used was not already the PAT. A refused PAT is never sent twice, and a 401 still gets its single refresh attempt first. --- docs/WEB-UI.md | 9 +- src/ci_orchestrator/operations/executions.py | 35 +++++--- src/ci_orchestrator/web/app.py | 40 ++++++--- tests/python/test_web_executions.py | 88 ++++++++++++++++++++ 4 files changed, 142 insertions(+), 30 deletions(-) diff --git a/docs/WEB-UI.md b/docs/WEB-UI.md index fe98197..e37a09d 100644 --- a/docs/WEB-UI.md +++ b/docs/WEB-UI.md @@ -304,10 +304,11 @@ automatically; a refusal that refresh cannot fix shows a "log in again" link in the card. Optional fallback: store a Gitea PAT in the keyring and name it in -`[web].gitea_token_cred`. It is used only when no logged-in token is -available — after a service restart (which empties the in-process map while -session cookies stay valid), or for an operator whose own token lacks the -scope. Set it exactly like the other secrets: +`[web].gitea_token_cred`. It is used when no logged-in token is available — +after a service restart (which empties the in-process map while session cookies +stay valid) — **and** when Gitea refuses the login token (`401`, or `403` for +insufficient scope), which is what an OIDC token limited to `openid profile` +gets. Set it exactly like the other secrets: ```bash sudo -u ci-runner /opt/ci/venv/bin/python -m ci_orchestrator creds set \ diff --git a/src/ci_orchestrator/operations/executions.py b/src/ci_orchestrator/operations/executions.py index bc729c8..03806ed 100644 --- a/src/ci_orchestrator/operations/executions.py +++ b/src/ci_orchestrator/operations/executions.py @@ -112,6 +112,10 @@ class ExecutionsView: ``auth_failed`` distinguishes "the token was refused" (``401``) from any other failure, so the caller can try refreshing it once before giving up. + ``refused`` is the wider "this token will not work" signal (``401`` *or* a + final ``403``, i.e. insufficient scope): the web layer uses it to fall back + from the operator's login token to the configured PAT, which is the only + way a PAT can help while a scope-less login token is in play. """ runs: list[Execution] @@ -119,6 +123,7 @@ class ExecutionsView: source: str error: str | None auth_failed: bool = False + refused: bool = False def _monotonic() -> float: @@ -126,8 +131,8 @@ def _monotonic() -> float: return time.monotonic() -# key -> (fetched_at, runs, source, error, auth_failed) -_CacheEntry = tuple[float, list[dict[str, Any]], str, str | None, bool] +# key -> (fetched_at, runs, source, error, auth_failed, refused) +_CacheEntry = tuple[float, list[dict[str, Any]], str, str | None, bool, bool] _CACHE: dict[tuple[str, str, int], _CacheEntry] = {} @@ -237,10 +242,10 @@ def _fetch(url: str, token: str) -> list[dict[str, Any]]: def _fetch_runs( base_url: str, token: str, limit: int -) -> tuple[list[dict[str, Any]], str, str | None, bool]: +) -> tuple[list[dict[str, Any]], str, str | None, bool, bool]: """Admin endpoint first, user endpoint as the non-admin fallback. - Returns ``(runs, source, error, auth_failed)``; never raises. + Returns ``(runs, source, error, auth_failed, refused)``; never raises. """ query = urllib.parse.urlencode({"limit": limit}) admin_url = f"{base_url}/api/v1/admin/actions/runs?{query}" @@ -248,13 +253,15 @@ def _fetch_runs( last_error: str | None = None auth_failed = False + refused = False for source, url in (("admin", admin_url), ("user", user_url)): try: - return _fetch(url, token), source, None, False + return _fetch(url, token), source, None, False, False except urllib.error.HTTPError as exc: last_error = f"Gitea API returned HTTP {exc.code}." if exc.code == 401: auth_failed = True + refused = True break if exc.code in (403, 404) and source == "admin": # Token is not admin-scoped: try the per-user listing. @@ -264,6 +271,7 @@ def _fetch_runs( # authenticated it and then rejected its scope. Say so -- # "HTTP 403" alone sends the operator hunting the wrong thing. last_error = _SCOPE_HINT + refused = True break except (urllib.error.URLError, TimeoutError, OSError) as exc: last_error = f"Gitea unreachable: {exc}." @@ -273,23 +281,23 @@ def _fetch_runs( break logger.warning("executions fetch failed: %s", last_error) - return [], "none", last_error, auth_failed + return [], "none", last_error, auth_failed, refused def _cached_runs( base_url: str, token: str, limit: int -) -> tuple[list[dict[str, Any]], str, str | None, bool]: +) -> tuple[list[dict[str, Any]], str, str | None, bool, bool]: """:func:`_fetch_runs` behind the TTL cache (errors are cached too).""" key = (base_url, token, limit) now = _monotonic() entry = _CACHE.get(key) if entry is not None and now - entry[0] < _TTL_SEC: - _fetched_at, runs, source, error, auth_failed = entry - return runs, source, error, auth_failed + _fetched_at, runs, source, error, auth_failed, refused = entry + return runs, source, error, auth_failed, refused - runs, source, error, auth_failed = _fetch_runs(base_url, token, limit) - _CACHE[key] = (now, runs, source, error, auth_failed) - return runs, source, error, auth_failed + runs, source, error, auth_failed, refused = _fetch_runs(base_url, token, limit) + _CACHE[key] = (now, runs, source, error, auth_failed, refused) + return runs, source, error, auth_failed, refused def _attach_vms( @@ -365,7 +373,7 @@ def list_executions( error="Gitea token not configured.", ) - raw_runs, source, error, auth_failed = _cached_runs( + raw_runs, source, error, auth_failed, refused = _cached_runs( base_url.rstrip("/"), token, effective_limit ) if error is not None: @@ -375,6 +383,7 @@ def list_executions( source=source, error=error, auth_failed=auth_failed, + refused=refused, ) runs, unmatched = _attach_vms( diff --git a/src/ci_orchestrator/web/app.py b/src/ci_orchestrator/web/app.py index 8f1e482..e0a842d 100644 --- a/src/ci_orchestrator/web/app.py +++ b/src/ci_orchestrator/web/app.py @@ -168,32 +168,46 @@ async def _fetch_executions( Token precedence: the access token from this operator's own OIDC login (held server-side, never in the cookie -- see ``web/auth.py``), then the - optional keyring PAT. A ``401`` triggers exactly one refresh attempt - before the card falls back to its degraded state. + optional keyring PAT. A ``401`` triggers exactly one refresh attempt, and a + token Gitea *refuses* (``401``, or ``403`` for insufficient scope) falls + back to the PAT -- without that fallback a configured PAT could never help + while a logged-in operator holds a scope-less login token, which is exactly + what an OIDC token limited to ``openid profile`` is. """ username = request.session.get("user") stored = gitea_token_for(request.app, username) if stored is not None and stored.expired and username: stored = await refresh_gitea_token(request, str(username)) or stored - token = stored.access_token if stored is not None else _pat_fallback(config) + pat = _pat_fallback(config) + token = stored.access_token if stored is not None else pat - try: - view = list_executions(config, vms, token=token) - except Exception: # the operations layer already degrades; belt and braces - return ExecutionsView( - runs=[], unmatched_vms=vms, source="none", error="Executions unavailable." - ) + view = _list_executions(config, vms, token) if view.auth_failed and stored is not None and username: refreshed = await refresh_gitea_token(request, str(username)) if refreshed is not None: - try: - return list_executions(config, vms, token=refreshed.access_token) - except Exception: - return view + retried = _list_executions(config, vms, refreshed.access_token) + if not retried.refused: + return retried + view = retried + + if view.refused and pat is not None and token != pat: + return _list_executions(config, vms, pat) return view +def _list_executions( + config: Config, vms: list[LiveVm], token: str | None +) -> ExecutionsView: + """``list_executions`` with a belt-and-braces guard. Never raises.""" + try: + return list_executions(config, vms, token=token) + except Exception: # the operations layer already degrades; belt and braces + return ExecutionsView( + runs=[], unmatched_vms=vms, source="none", error="Executions unavailable." + ) + + def _fetch_disk_status(config: Config) -> DiskStatus | None: """Disk usage for ``config.paths.root``, or ``None`` if unreadable.""" try: diff --git a/tests/python/test_web_executions.py b/tests/python/test_web_executions.py index eb02154..42a19ae 100644 --- a/tests/python/test_web_executions.py +++ b/tests/python/test_web_executions.py @@ -565,6 +565,94 @@ def test_login_token_is_used_for_the_card( assert tokens == ["login-token"] +def test_scope_refused_login_token_falls_back_to_the_pat( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A logged-in operator whose token lacks API scope must still get runs.""" + _install_fake_keyring(monkeypatch) + app = create_app(_make_config(tmp_path, oidc=True, token_cred=_PAT_CRED_NAME)) + app.state.oauth.gitea = _StubOAuthClient({"access_token": "scope-less"}) + app.dependency_overrides[get_backend] = lambda: _FakeBackend([]) + client = TestClient(app) + _login(app, client) + + seen: list[str | None] = [] + + def fake_list_executions( + _config: Config, _vms: list[LiveVm], *, token: str | None, **_kw: Any + ) -> ExecutionsView: + seen.append(token) + if token == "scope-less": + return ExecutionsView( + runs=[], + unmatched_vms=[], + source="none", + error="no API read scope", + refused=True, + ) + return ExecutionsView( + runs=[_execution()], unmatched_vms=[], source="admin", error=None + ) + + monkeypatch.setattr(app_module, "list_executions", fake_list_executions) + + resp = client.get("/executions") + + assert seen == ["scope-less", _FAKE_PAT] + assert "#12" in resp.text + + +def test_no_pat_fallback_when_none_configured( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Without a PAT there is nothing to retry with -- one call, then degrade.""" + _install_fake_keyring(monkeypatch) + app = create_app(_make_config(tmp_path, oidc=True)) + app.state.oauth.gitea = _StubOAuthClient({"access_token": "scope-less"}) + app.dependency_overrides[get_backend] = lambda: _FakeBackend([]) + client = TestClient(app) + _login(app, client) + tokens = _stub_view( + monkeypatch, + ExecutionsView( + runs=[], + unmatched_vms=[], + source="none", + error="no API read scope", + refused=True, + ), + ) + + resp = client.get("/executions") + + assert tokens == ["scope-less"] + assert "no API read scope" in resp.text + + +def test_pat_is_not_retried_against_itself( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A refused PAT (no login token in play) must not be sent twice.""" + _install_fake_keyring(monkeypatch) + tokens = _stub_view( + monkeypatch, + ExecutionsView( + runs=[], + unmatched_vms=[], + source="none", + error="no API read scope", + refused=True, + ), + ) + app = create_app(_make_config(tmp_path, token_cred=_PAT_CRED_NAME)) + app.dependency_overrides[get_backend] = lambda: _FakeBackend([]) + client = TestClient(app) + + client.get("/executions") + + assert tokens == [_FAKE_PAT] + + def test_401_triggers_one_refresh_then_retries( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: