fix(web): fall back to the PAT when Gitea refuses the login token
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.
This commit is contained in:
+5
-4
@@ -304,10 +304,11 @@ automatically; a refusal that refresh cannot fix shows a "log in again" link
|
|||||||
in the card.
|
in the card.
|
||||||
|
|
||||||
Optional fallback: store a Gitea PAT in the keyring and name it in
|
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
|
`[web].gitea_token_cred`. It is used when no logged-in token is available —
|
||||||
available — after a service restart (which empties the in-process map while
|
after a service restart (which empties the in-process map while session cookies
|
||||||
session cookies stay valid), or for an operator whose own token lacks the
|
stay valid) — **and** when Gitea refuses the login token (`401`, or `403` for
|
||||||
scope. Set it exactly like the other secrets:
|
insufficient scope), which is what an OIDC token limited to `openid profile`
|
||||||
|
gets. Set it exactly like the other secrets:
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
sudo -u ci-runner /opt/ci/venv/bin/python -m ci_orchestrator creds set \
|
sudo -u ci-runner /opt/ci/venv/bin/python -m ci_orchestrator creds set \
|
||||||
|
|||||||
@@ -112,6 +112,10 @@ class ExecutionsView:
|
|||||||
|
|
||||||
``auth_failed`` distinguishes "the token was refused" (``401``) from any
|
``auth_failed`` distinguishes "the token was refused" (``401``) from any
|
||||||
other failure, so the caller can try refreshing it once before giving up.
|
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]
|
runs: list[Execution]
|
||||||
@@ -119,6 +123,7 @@ class ExecutionsView:
|
|||||||
source: str
|
source: str
|
||||||
error: str | None
|
error: str | None
|
||||||
auth_failed: bool = False
|
auth_failed: bool = False
|
||||||
|
refused: bool = False
|
||||||
|
|
||||||
|
|
||||||
def _monotonic() -> float:
|
def _monotonic() -> float:
|
||||||
@@ -126,8 +131,8 @@ def _monotonic() -> float:
|
|||||||
return time.monotonic()
|
return time.monotonic()
|
||||||
|
|
||||||
|
|
||||||
# key -> (fetched_at, runs, source, error, auth_failed)
|
# key -> (fetched_at, runs, source, error, auth_failed, refused)
|
||||||
_CacheEntry = tuple[float, list[dict[str, Any]], str, str | None, bool]
|
_CacheEntry = tuple[float, list[dict[str, Any]], str, str | None, bool, bool]
|
||||||
_CACHE: dict[tuple[str, str, int], _CacheEntry] = {}
|
_CACHE: dict[tuple[str, str, int], _CacheEntry] = {}
|
||||||
|
|
||||||
|
|
||||||
@@ -237,10 +242,10 @@ def _fetch(url: str, token: str) -> list[dict[str, Any]]:
|
|||||||
|
|
||||||
def _fetch_runs(
|
def _fetch_runs(
|
||||||
base_url: str, token: str, limit: int
|
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.
|
"""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})
|
query = urllib.parse.urlencode({"limit": limit})
|
||||||
admin_url = f"{base_url}/api/v1/admin/actions/runs?{query}"
|
admin_url = f"{base_url}/api/v1/admin/actions/runs?{query}"
|
||||||
@@ -248,13 +253,15 @@ def _fetch_runs(
|
|||||||
|
|
||||||
last_error: str | None = None
|
last_error: str | None = None
|
||||||
auth_failed = False
|
auth_failed = False
|
||||||
|
refused = False
|
||||||
for source, url in (("admin", admin_url), ("user", user_url)):
|
for source, url in (("admin", admin_url), ("user", user_url)):
|
||||||
try:
|
try:
|
||||||
return _fetch(url, token), source, None, False
|
return _fetch(url, token), source, None, False, False
|
||||||
except urllib.error.HTTPError as exc:
|
except urllib.error.HTTPError as exc:
|
||||||
last_error = f"Gitea API returned HTTP {exc.code}."
|
last_error = f"Gitea API returned HTTP {exc.code}."
|
||||||
if exc.code == 401:
|
if exc.code == 401:
|
||||||
auth_failed = True
|
auth_failed = True
|
||||||
|
refused = True
|
||||||
break
|
break
|
||||||
if exc.code in (403, 404) and source == "admin":
|
if exc.code in (403, 404) and source == "admin":
|
||||||
# Token is not admin-scoped: try the per-user listing.
|
# 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 --
|
# authenticated it and then rejected its scope. Say so --
|
||||||
# "HTTP 403" alone sends the operator hunting the wrong thing.
|
# "HTTP 403" alone sends the operator hunting the wrong thing.
|
||||||
last_error = _SCOPE_HINT
|
last_error = _SCOPE_HINT
|
||||||
|
refused = True
|
||||||
break
|
break
|
||||||
except (urllib.error.URLError, TimeoutError, OSError) as exc:
|
except (urllib.error.URLError, TimeoutError, OSError) as exc:
|
||||||
last_error = f"Gitea unreachable: {exc}."
|
last_error = f"Gitea unreachable: {exc}."
|
||||||
@@ -273,23 +281,23 @@ def _fetch_runs(
|
|||||||
break
|
break
|
||||||
|
|
||||||
logger.warning("executions fetch failed: %s", last_error)
|
logger.warning("executions fetch failed: %s", last_error)
|
||||||
return [], "none", last_error, auth_failed
|
return [], "none", last_error, auth_failed, refused
|
||||||
|
|
||||||
|
|
||||||
def _cached_runs(
|
def _cached_runs(
|
||||||
base_url: str, token: str, limit: int
|
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)."""
|
""":func:`_fetch_runs` behind the TTL cache (errors are cached too)."""
|
||||||
key = (base_url, token, limit)
|
key = (base_url, token, limit)
|
||||||
now = _monotonic()
|
now = _monotonic()
|
||||||
entry = _CACHE.get(key)
|
entry = _CACHE.get(key)
|
||||||
if entry is not None and now - entry[0] < _TTL_SEC:
|
if entry is not None and now - entry[0] < _TTL_SEC:
|
||||||
_fetched_at, runs, source, error, auth_failed = entry
|
_fetched_at, runs, source, error, auth_failed, refused = entry
|
||||||
return runs, source, error, auth_failed
|
return runs, source, error, auth_failed, refused
|
||||||
|
|
||||||
runs, source, error, auth_failed = _fetch_runs(base_url, token, limit)
|
runs, source, error, auth_failed, refused = _fetch_runs(base_url, token, limit)
|
||||||
_CACHE[key] = (now, runs, source, error, auth_failed)
|
_CACHE[key] = (now, runs, source, error, auth_failed, refused)
|
||||||
return runs, source, error, auth_failed
|
return runs, source, error, auth_failed, refused
|
||||||
|
|
||||||
|
|
||||||
def _attach_vms(
|
def _attach_vms(
|
||||||
@@ -365,7 +373,7 @@ def list_executions(
|
|||||||
error="Gitea token not configured.",
|
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
|
base_url.rstrip("/"), token, effective_limit
|
||||||
)
|
)
|
||||||
if error is not None:
|
if error is not None:
|
||||||
@@ -375,6 +383,7 @@ def list_executions(
|
|||||||
source=source,
|
source=source,
|
||||||
error=error,
|
error=error,
|
||||||
auth_failed=auth_failed,
|
auth_failed=auth_failed,
|
||||||
|
refused=refused,
|
||||||
)
|
)
|
||||||
|
|
||||||
runs, unmatched = _attach_vms(
|
runs, unmatched = _attach_vms(
|
||||||
|
|||||||
@@ -168,30 +168,44 @@ async def _fetch_executions(
|
|||||||
|
|
||||||
Token precedence: the access token from this operator's own OIDC login
|
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
|
(held server-side, never in the cookie -- see ``web/auth.py``), then the
|
||||||
optional keyring PAT. A ``401`` triggers exactly one refresh attempt
|
optional keyring PAT. A ``401`` triggers exactly one refresh attempt, and a
|
||||||
before the card falls back to its degraded state.
|
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")
|
username = request.session.get("user")
|
||||||
stored = gitea_token_for(request.app, username)
|
stored = gitea_token_for(request.app, username)
|
||||||
if stored is not None and stored.expired and username:
|
if stored is not None and stored.expired and username:
|
||||||
stored = await refresh_gitea_token(request, str(username)) or stored
|
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)
|
||||||
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."
|
|
||||||
)
|
|
||||||
|
|
||||||
if view.auth_failed and stored is not None and username:
|
if view.auth_failed and stored is not None and username:
|
||||||
refreshed = await refresh_gitea_token(request, str(username))
|
refreshed = await refresh_gitea_token(request, str(username))
|
||||||
if refreshed is not None:
|
if refreshed is not None:
|
||||||
|
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:
|
try:
|
||||||
return list_executions(config, vms, token=refreshed.access_token)
|
return list_executions(config, vms, token=token)
|
||||||
except Exception:
|
except Exception: # the operations layer already degrades; belt and braces
|
||||||
return view
|
return ExecutionsView(
|
||||||
return view
|
runs=[], unmatched_vms=vms, source="none", error="Executions unavailable."
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
def _fetch_disk_status(config: Config) -> DiskStatus | None:
|
def _fetch_disk_status(config: Config) -> DiskStatus | None:
|
||||||
|
|||||||
@@ -565,6 +565,94 @@ def test_login_token_is_used_for_the_card(
|
|||||||
assert tokens == ["login-token"]
|
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(
|
def test_401_triggers_one_refresh_then_retries(
|
||||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||||
) -> None:
|
) -> None:
|
||||||
|
|||||||
Reference in New Issue
Block a user