Compare commits

...
11 Commits
Author SHA1 Message Date
jpmschweitzer 3536c40608 fix(permissions): narrow rm -rf deny globs to their exact forms
The trailing wildcard on the three rm -rf deny entries spanned path
separators, so Bash(rm -rf /*) matched every absolute path on the
machine rather than the filesystem root, and the ~ and $HOME entries
had the same shape. Narrowed to the exact literal forms.

These rules match literal command text, so they still stop a typo on
rm -rf /, rm -rf ~ or rm -rf $HOME exactly, but they no longer stop a
recursive delete aimed at any other path. That reduced cover is
deliberate, not an oversight.
2026-08-25 20:31:15 +02:00
jpmschweitzerandClaude 0c7e1cb04e fix(tests): mock the live DNS service instance, not a dead legacy class (T-55)
test_dns_lookup_returns_result patched src.controllers.tools_controller.
DNSService, which is never imported by the request path under test.
client wraps src.main.app, which routes /tools/dns/lookup through
src.domains.tools.controller.tools_controller — a singleton constructed
at import time from src.domains.tools.dns.service.DNSService. The patched
class was dead; the mock was never consulted, so the test issued a real
DNS query for example.com and asserted on its outcome. With no network
the query times out and the assertion fails (D-26).

The three sibling tests in the same class patch the same dead class and
also run unmocked, but happen not to notice: DNS failures are caught
inside DNSService.lookup() and returned as a normal 200 response with
success=False, and their assertions only check status_code / DNSQueryError
branches that don't depend on resolution actually succeeding. Only this
test's `data["success"] is True` assertion is sensitive to the real
network outcome, which is why it's the only one D-26's namespace run
catches. Not touched here — out of T-55's scope, flagging for the record.

Fix patches tools_controller.dns_service, the actual instance attribute
the live route calls, via patch.object on the singleton rather than
patch() on the constructor class (the instance already exists by the
time a class-level patch would apply).

Verified:
- unshare -rn (lo up): 381 passed, exit 0 (was 1 failed, 380 passed, exit 2)
- with network: 381 passed, exit 0 (unchanged from before the fix)
- mutation check: retargeted the patch.object to a nonexistent attribute
  name, confirmed count==1 before editing; namespace run then reproduced
  the original failure (1 failed, 380 passed); reverted and reconfirmed
  381 passed, exit 0.

Co-Authored-By: Claude <noreply@anthropic.com>
2026-08-18 15:33:58 +02:00
jpmschweitzerandClaude 6695215aeb build(make): prove setup worked instead of assuming pip's exit code (T-47)
pip install exits 0 whether the result is usable or not — that is the D-24
shape exactly, a step whose job is to not fail. On 2026-08-09 the venv here
existed and pip had already succeeded, but sqlalchemy was declared in
requirements.txt and not installed. That surfaced as 11 pytest collection
errors that read as broken imports rather than as an environment problem.

setup now ends with `pytest --collect-only tests/`, which exercises every
import the suite touches without running anything. pip check was considered
as a cheaper alternative and rejected: it only checks the installed set's
internal consistency against itself, so it would not have caught this case —
sqlalchemy was still present as another package's transitive dependency even
after being dropped from requirements.txt. collect-only checks declared vs.
actually usable directly, which is the axis that broke.

Verified: two clean runs (33.5s cold, 5.7s idempotent re-run, second changes
nothing). Reproduced the original failure by uninstalling sqlalchemy from an
otherwise-correct venv — collect-only alone then exits 2 with 11 collection
errors; make setup against that same state reinstalls it and exits 0 with
381/381 collected.

Co-Authored-By: Claude <noreply@anthropic.com>
2026-08-17 12:05:28 +02:00
jpmschweitzerandClaude d2e0a49e7c build(ci): move the pre-push gate into the Makefile
The hook carried ~50 lines of gitleaks logic and a comment explaining it was
self-contained because "this repo has no Makefile". It has one now, so the
reason is gone and the arrangement is backwards: a hook is a trigger, and
logic belongs where it can be read, run by hand, and changed under review.

.githooks/pre-push is now a byte-identical shim onto `make pre-push` in every
repo in the workspace. The scan itself moves to ci/secrets.sh unchanged, and
`make secrets` runs it on its own.

The call surface is identical everywhere; what it runs is not, and should not
be — each repo gates what it actually has. That is the point of standardising
the name rather than the contents: nobody has to read a repo to find out how
to check it.

secrets runs first, deliberately. It is the only failure here that cannot be
undone by fixing it afterwards — a failed lint costs another commit, a pushed
credential is cached and indexed whether or not it is later deleted.

Some of these gates fail today, on lint debt that predates them, and they are
left wired anyway. The board was measured once and written down in T-56
instead of being worked around here. Narrowing each gate to whatever already
passes would produce a gate that reports success for doing nothing, which is
the failure this workspace keeps rediscovering.

Co-Authored-By: Claude <noreply@anthropic.com>
2026-08-09 18:57:22 +02:00
jpmschweitzerandClaude 755aa61107 ci(make): reserve exit 69 for "could not run" (D-26)
Environment guards now exit 69 rather than 1, so a caller can tell a suite
that could not start from one that ran and failed. The first toj test sweep
reported "3 repositories failed" and none of the three had executed a test —
two could not find go, one had no venv. That points the reader at the tests
when the fault is in the environment.

Only the environment guards change. A gitleaks finding, a failed test run and
a vulncheck hit still exit 1, because those did run and did fail.

Co-Authored-By: Claude <noreply@anthropic.com>
2026-08-09 15:56:19 +02:00
jpmschweitzerandClaude 01349a83f2 test: repair the suite against the current API
The suite could not even collect: the venv was missing declared dependencies,
and five tests asserted an API that had moved on. 11 collection errors to 381
passing.

test_oidc.py was written for the single-issuer API and 6243f29 replaced it.
issuer and audience became lists, jwks_uri stopped being an attribute in
favour of get_jwks_uri(issuer), get_jwks became get_jwks_for_issuer, and the
lru_cache became a per-issuer dict so cache_clear no longer exists. Rewritten
against the current surface, with coverage added for the two behaviours the
multi-issuer change introduced and never tested: is_valid_issuer rejecting an
unconfigured issuer, and the cache keying per issuer. Both are
security-relevant — a shared cache would serve one issuer keys for another.

Three /auth/me tests asserted a path that does not exist. The route is
declared as /me inside AuthController.create_router() and mounts at
/auth/users/me; the generated spec is authoritative and the local app and the
deployed service agree on it. Those tests had never passed.

test_handles_empty_groups expected groups == [""] for an empty header. oidc.py
has returned [] since the initial commit, and [] is correct — [""] would also
be unsafe, since any check doing "" in groups would match.

test_model_aliases_property covered Settings.model_aliases, deleted with the
Ollama integration in c1f16d4. Removed rather than repaired.

Co-Authored-By: Claude <noreply@anthropic.com>
2026-08-09 15:21:43 +02:00
jpmschweitzerandClaude 7815e1c231 build: add the Makefile command surface (D-27)
Every repo gets one at the root: help, plus test and lint where those exist.
The point is that a target name means the same thing in every repo, so an
agent or a person can act without reading the repo first.

Paths resolve here rather than in callers (D-10). python3 on this host is 3.8
and cannot parse these sources, and a bare pytest or ruff resolves only in a
login shell — so both are named explicitly through the venv, and a missing
venv fails with the command to fix it rather than a bare no-such-file.

Co-Authored-By: Claude <noreply@anthropic.com>
2026-08-09 15:10:31 +02:00
jpmschweitzerandClaude e9fc09af26 chore(claude): pin PQL_VAULT per project so cwd stops choosing the vault
pql is now a bare word on PATH, which removed the long incantation that had
been forcing --vault into every call by habit. Convenience lowered the cost
of the wrong thing without lowering the cost of the right one: a three-word
pql ticket new targets whichever vault the cwd happens to sit in, and there
are nine of them with colliding id sequences.

PQL_VAULT in each project settings file makes the vault a property of the
session rather than of the working directory — the same lesson Rule 3 records
for git -C, applied to pql. Verified the env var overrides cwd discovery,
that an explicit --vault still beats the env var, and that the harness
hot-reloads it without a restart.

This does not make provenance visible: no output says which vault answered,
so a forgotten --vault still returns a well-formed answer about the wrong
dataset. That remains T-37.

Co-Authored-By: Claude <noreply@anthropic.com>
2026-08-09 13:48:58 +02:00
jpmschweitzerandClaude 56b991c965 chore(claude): deny toj in the sub-repos
toj is now on the global PATH as /usr/local/bin/toj, so its scope boundary
had to stop being "the absolute path is inconvenient to type" and start
being a rule. Its repo and settings verbs operate on the workspace root; run
from inside this repo they answer about the wrong tree.

Both spellings are denied, bare and absolute, because a deny with one
spelling left open is decorative.

Co-Authored-By: Claude <noreply@anthropic.com>
2026-08-09 13:42:03 +02:00
jpmschweitzerandClaude 387a3dbb02 ci: gate pushes on a gitleaks scan of the outgoing commits
No repo here scanned for committed credentials. The hook is self-contained
rather than delegating to a Makefile, because this repo has none and a hook
reaching into a sibling repo breaks the moment this one is cloned elsewhere.

Scans the outgoing range rather than full history: history carries settled
findings — test fixtures, vendored third-party code — and a gate that fails
on something unfixable gets bypassed within a week.

Setting core.hooksPath means pql init must replant its replication shims into
.githooks, which is why they are gitignored here alongside the tracked
pre-push. Same layout pql itself uses.

Co-Authored-By: Claude <noreply@anthropic.com>
2026-08-09 12:48:53 +02:00
jpmschweitzerandClaude 7f90369621 docs: qualify workspace decision ids cited from this repo
Decision ids are per-vault sequences, so they collide by construction
once there is more than one vault -- and every repo now has one. A bare
D-15 here will mean this repo's D-15 the moment this repo records one.
Cross-vault references are therefore qualified: workspace D-15.

Not hypothetical: pql holds D-1 through D-31 while the workspace holds
D-1 through D-21, so every workspace id currently collides with an
unrelated pql one. A bare id is not wrong the day it is written -- it
decays into wrong as the other vault grows, and nothing flags it.

Co-Authored-By: Claude <noreply@anthropic.com>
2026-08-09 04:17:07 +02:00
10 changed files with 335 additions and 115 deletions
+37 -30
View File
@@ -1,4 +1,7 @@
{ {
"env": {
"PQL_VAULT": "/mnt/media/Projects/core-api"
},
"permissions": { "permissions": {
"allow": [ "allow": [
"Bash(pql)", "Bash(pql)",
@@ -16,47 +19,51 @@
"Bash(curl -s http://localhost:8083/*)" "Bash(curl -s http://localhost:8083/*)"
], ],
"deny": [ "deny": [
"Bash(sudo *)", "Bash(/mnt/media/Projects/cladmin/ops/bin/toj)",
"Bash(su *)", "Bash(/mnt/media/Projects/cladmin/ops/bin/toj:*)",
"Bash(rm -rf /*)", "Bash(alembic downgrade base*)",
"Bash(rm -rf ~*)",
"Bash(rm -rf $HOME*)",
"Bash(mkfs*)",
"Bash(dd if=*)",
"Bash(chmod 777 *)",
"Bash(chmod -R 777 *)", "Bash(chmod -R 777 *)",
"Bash(find * -exec*)", "Bash(chmod 777 *)",
"Bash(dd if=*)",
"Bash(find * -delete*)", "Bash(find * -delete*)",
"Bash(git add -A*)", "Bash(find * -exec*)",
"Bash(git * add -A*)",
"Bash(git add --all*)",
"Bash(git * add --all*)", "Bash(git * add --all*)",
"Bash(git add .)", "Bash(git * add -A*)",
"Bash(git * add .)", "Bash(git * add .)",
"Bash(git push --force*)",
"Bash(git * push --force*)",
"Bash(git push -f*)",
"Bash(git * push -f*)",
"Bash(git reset --hard*)",
"Bash(git * reset --hard*)",
"Bash(git clean -fd*)",
"Bash(git * clean -fd*)",
"Bash(git clean -fdx*)",
"Bash(git * clean -fdx*)",
"Bash(git branch -D *)",
"Bash(git * branch -D *)", "Bash(git * branch -D *)",
"Bash(git checkout -- *)",
"Bash(git * checkout -- *)", "Bash(git * checkout -- *)",
"Bash(git restore .*)", "Bash(git * clean -fd*)",
"Bash(git * restore .*)", "Bash(git * clean -fdx*)",
"Bash(git commit --no-verify*)",
"Bash(git * commit --no-verify*)", "Bash(git * commit --no-verify*)",
"Bash(git merge --no-ff*)",
"Bash(git * merge --no-ff*)", "Bash(git * merge --no-ff*)",
"Bash(git * push --force*)",
"Bash(git * push -f*)",
"Bash(git * reset --hard*)",
"Bash(git * restore .*)",
"Bash(git add --all*)",
"Bash(git add -A*)",
"Bash(git add .)",
"Bash(git branch -D *)",
"Bash(git checkout -- *)",
"Bash(git clean -fd*)",
"Bash(git clean -fdx*)",
"Bash(git commit --no-verify*)",
"Bash(git merge --no-ff*)",
"Bash(git push --force*)",
"Bash(git push -f*)",
"Bash(git reset --hard*)",
"Bash(git restore .*)",
"Bash(mkfs*)",
"Bash(psql * -c DROP*)", "Bash(psql * -c DROP*)",
"Bash(psql * DROP DATABASE*)", "Bash(psql * DROP DATABASE*)",
"Bash(psql * TRUNCATE*)", "Bash(psql * TRUNCATE*)",
"Bash(alembic downgrade base*)" "Bash(rm -rf $HOME)",
"Bash(rm -rf /)",
"Bash(rm -rf ~)",
"Bash(su *)",
"Bash(sudo *)",
"Bash(toj)",
"Bash(toj:*)"
] ]
} }
} }
+13
View File
@@ -0,0 +1,13 @@
#!/usr/bin/env bash
# Trigger only. The checks live in the Makefile, where they can be read, run by
# hand (`make pre-push`), and changed under review.
#
# This file is identical in every repo in this workspace, deliberately: the call
# surface is the same everywhere even though what each gate runs is not, so
# nobody has to read a repo to find out how to check it (D-27).
#
# Enable per clone with: git config core.hooksPath .githooks
# Never bypass with --no-verify. Suppress a specific finding deliberately
# instead, with a reason — see `make pre-push`.
set -euo pipefail
exec make -C "$(git rev-parse --show-toplevel)" pre-push
+8
View File
@@ -145,3 +145,11 @@ Thumbs.db
.pql/* .pql/*
!.pql/changelog/ !.pql/changelog/
# pql shims planted by `pql init` into the dir core.hooksPath points at.
# Per-clone: each embeds the absolute path of the pql binary that planted it.
# Only .githooks/pre-push is shared.
.githooks/pre-commit
.githooks/post-merge
.githooks/post-checkout
.githooks/post-rewrite
+1 -1
View File
@@ -119,7 +119,7 @@ before tagging a release.
Work lives in **pql**, not a markdown TODO. **This repo's vault is standalone** — its tickets Work lives in **pql**, not a markdown TODO. **This repo's vault is standalone** — its tickets
and its internal decisions live here in `.pql/` and `governance/`, and travel with a clone, and its internal decisions live here in `.pql/` and `governance/`, and travel with a clone,
because `.pql/changelog/` is committed and replayed by the git hooks (D-15). The databases are because `.pql/changelog/` is committed and replayed by the git hooks (workspace D-15). The databases are
gitignored and rebuildable with `pql plan rebuild`. gitignored and rebuildable with `pql plan rebuild`.
`pql` is **not** on the non-interactive `PATH` — invoke it as `pql` is **not** on the non-interactive `PATH` — invoke it as
+67
View File
@@ -0,0 +1,67 @@
# core-api — the repo's command surface (D-27).
#
# Paths resolve here rather than in callers (D-10). `python3` on this host is
# 3.8 and cannot parse these sources, and a bare `pytest` resolves only in a
# login shell — so both are named explicitly through the venv.
VENV := $(CURDIR)/.venv
PYTHON ?= python3.12
.DEFAULT_GOAL := help
.PHONY: help
help: ## Show this help
@grep -hE '^[a-z][a-z0-9_-]*:.*?## ' $(MAKEFILE_LIST) \
| awk 'BEGIN{FS=":.*?## "}{printf " \033[36m%-14s\033[0m %s\n", $$1, $$2}'
.PHONY: setup
setup: ## Create the venv, install dependencies, and prove the result is usable
$(PYTHON) -m venv .venv
$(VENV)/bin/pip install -r requirements.txt -r dev-requirements.txt
# Exit 0 from pip install is not evidence (D-24) — it is the step's job not
# to fail, so a broken result and a working one look identical from here.
# On 2026-08-09 the venv existed and pip had exited 0, but sqlalchemy was
# declared in requirements.txt and not installed; that surfaced as 11
# pytest collection errors that read as broken imports, not as a setup
# problem. `--collect-only` exercises every import the suite touches
# without running a single test, so it catches exactly that class of
# drift and stays cheap. `pip check` was considered too, but it only
# verifies the *installed* set's internal consistency against itself —
# it would not have caught this case, because sqlalchemy was still
# present as another package's transitive dependency even when dropped
# from requirements.txt. collect-only checks declared-vs-actually-usable
# directly, which is the axis that broke.
$(VENV)/bin/python -m pytest --collect-only tests/
.PHONY: test
test: ## Run the test suite
@test -x $(VENV)/bin/python || { echo "FAIL — no venv; run: make setup"; exit 69; }
$(VENV)/bin/python -m pytest tests/
# No `lint` target, deliberately. This repo configures no linter — no ruff or
# flake8 config, and neither in requirements. Per D-27 the name is reserved for
# repos that lint; an empty target here would report clean for something never
# run. Add the target when a linter is added, not before.
# git hands a hook a non-login shell, which never sees ~/.local/bin — where
# gitleaks lands. Without this the scan reports "not installed" on every push,
# which is a check that fails open (D-24).
export PATH := $(HOME)/.local/bin:/usr/local/bin:$(PATH)
.PHONY: secrets
secrets: ## Scan the commits about to be pushed for credentials
@ci/secrets.sh
# The call surface is identical in every repo; what it runs is not.
#
# `secrets` runs first, deliberately: it is the only failure here that cannot be
# undone by fixing it afterwards. A failed lint costs another commit; a pushed
# credential is cached and indexed whether or not it is later deleted.
#
# Some of these fail today, and are left wired anyway. The state was measured
# once and written down in T-56 rather than being worked around here — a gate
# quietly narrowed to what already passes is a gate that reports success for
# doing nothing, which is the failure this workspace keeps rediscovering.
.PHONY: pre-push
pre-push: secrets ## Everything the pre-push hook runs
@echo " -- not gated here yet: lint (no linter configured) and test (T-56)"
Executable
+50
View File
@@ -0,0 +1,50 @@
#!/usr/bin/env bash
# Secret scan over the commits about to be pushed.
#
# Lives here rather than inside .githooks/pre-push so it can be read, run by
# hand (`make secrets`), and changed under review. A hook is a trigger; it is
# not a home for logic. Identical in every repo in this workspace (D-27).
set -euo pipefail
cd "$(git rev-parse --show-toplevel)"
# A non-login shell — which is what git gives a hook — skips /etc/profile.d
# and never sees ~/.local/bin, where the gitleaks release tarball lands.
# Without this the scan reports "not installed" on every push.
[ -d "$HOME/.local/bin" ] && PATH="$HOME/.local/bin:$PATH"
if ! command -v gitleaks >/dev/null 2>&1; then
echo "FAIL secrets — gitleaks not installed, so this check would be a no-op pretending to pass." >&2
echo " https://github.com/gitleaks/gitleaks/releases → ~/.local/bin/gitleaks" >&2
exit 1
fi
# Scan the outgoing range, not full history. History here carries findings
# that are settled — test fixtures and vendored third-party code — and a gate
# that fails on something unfixable gets bypassed within a week. What matters
# is what is about to leave this machine.
if upstream=$(git rev-parse --abbrev-ref --symbolic-full-name '@{u}' 2>/dev/null); then
range="$upstream..HEAD"
elif git rev-parse --verify --quiet origin/main >/dev/null; then
range="origin/main..HEAD"
else
range=""
fi
if [ -z "$range" ]; then
gitleaks dir . --redact --no-banner --exit-code 1 || {
echo "FAIL secrets — gitleaks found a credential in the working tree." >&2; exit 1; }
exit 0
fi
[ -n "$(git log --oneline "$range" 2>/dev/null)" ] || exit 0
gitleaks git . --log-opts="$range" --redact --no-banner --exit-code 1 >/dev/null 2>&1 || {
echo "FAIL secrets — gitleaks found a credential in the commits being pushed." >&2
echo " inspect (values redacted): gitleaks git . --log-opts=\"$range\" --redact" >&2
echo " then remove and rotate it, or suppress deliberately:" >&2
echo " inline '# gitleaks:allow <reason>'" >&2
echo " or add the fingerprint to .gitleaksignore WITH a reason" >&2
exit 1
}
echo " ok secrets"
+21 -7
View File
@@ -815,22 +815,32 @@ class TestPhase4Schemas:
# ============================================================================= # =============================================================================
# GET /auth/me Endpoint Tests (NPM Forward Auth) # GET /auth/users/me Endpoint Tests (NPM Forward Auth)
# ============================================================================= # =============================================================================
class TestAuthMeEndpoint: class TestAuthMeEndpoint:
"""Test GET /auth/me endpoint with NPM forward auth.""" """
Test GET /auth/users/me with NPM forward auth.
The path is /auth/users/me, not /auth/me — the route is declared as "/me"
inside AuthController.create_router(), which mounts under a users prefix.
These tests asserted /auth/me and had never passed; the generated spec is
authoritative and both the local app and the deployed service agree on 62
paths including this one.
"""
PATH = "/auth/users/me"
def test_auth_me_in_openapi(self, client): def test_auth_me_in_openapi(self, client):
"""Auth me endpoint should be in OpenAPI spec.""" """Auth me endpoint should be in OpenAPI spec."""
response = client.get("/openapi.json") response = client.get("/openapi.json")
spec = response.json() spec = response.json()
assert "/auth/me" in spec["paths"] assert self.PATH in spec["paths"]
assert "get" in spec["paths"]["/auth/me"] assert "get" in spec["paths"][self.PATH]
def test_auth_me_returns_401_without_forward_auth(self, client): def test_auth_me_returns_401_without_forward_auth(self, client):
"""Should return 401 when accessed without forward auth headers.""" """Should return 401 when accessed without forward auth headers."""
response = client.get("/auth/me") response = client.get(self.PATH)
# Without NPM forward auth headers, should return 401 # Without NPM forward auth headers, should return 401
assert response.status_code == 401 assert response.status_code == 401
@@ -840,7 +850,7 @@ class TestAuthMeEndpoint:
spec = response.json() spec = response.json()
# Check response schema references AuthSyncResponse # Check response schema references AuthSyncResponse
me_endpoint = spec["paths"]["/auth/me"]["get"] me_endpoint = spec["paths"][self.PATH]["get"]
assert "responses" in me_endpoint assert "responses" in me_endpoint
assert "200" in me_endpoint["responses"] assert "200" in me_endpoint["responses"]
@@ -920,7 +930,11 @@ class TestForwardAuthParsing:
result = await get_forward_auth_user(mock_request) result = await get_forward_auth_user(mock_request)
assert result["groups"] == [""] # An empty groups header means no groups, not one group named "".
# oidc.py has guarded this since the initial commit — this assertion
# expected [""] and had never passed. [""] would also be unsafe: any
# authorization check doing `"" in groups` would match.
assert result["groups"] == []
assert result["name"] == "jdoe" # Falls back to username assert result["name"] == "jdoe" # Falls back to username
@pytest.mark.asyncio @pytest.mark.asyncio
+3 -7
View File
@@ -89,10 +89,6 @@ class TestGetSettings:
settings2 = get_settings() settings2 = get_settings()
assert settings1 is settings2 assert settings1 is settings2
def test_model_aliases_property(self): # Removed: test_model_aliases_property. Settings.model_aliases mapped
"""Model aliases property should return dict.""" # gpt-3.5-turbo and gpt-4 onto local models, and was deleted along with the
settings = get_settings() # Ollama integration in c1f16d4. The test outlived the feature it covered.
aliases = settings.model_aliases
assert isinstance(aliases, dict)
assert "gpt-3.5-turbo" in aliases
assert "gpt-4" in aliases
+114 -62
View File
@@ -1,118 +1,173 @@
"""Tests for OIDC authentication module.""" """Tests for OIDC authentication module."""
import pytest import pytest
from unittest.mock import patch, MagicMock, AsyncMock from unittest.mock import patch, MagicMock
from fastapi import HTTPException from fastapi import HTTPException
from src.auth.oidc import OIDCConfig, oidc_config, get_jwks, get_current_user from src.auth.oidc import (
OIDCConfig,
oidc_config,
get_jwks_for_issuer,
get_current_user,
_jwks_cache,
)
@pytest.fixture(autouse=True)
def clear_jwks_cache():
"""
The JWKS cache is module-level state, so a fetch in one test would satisfy
the next one and hide a regression. Clearing on both sides keeps the tests
order-independent.
"""
_jwks_cache.clear()
yield
_jwks_cache.clear()
class TestOIDCConfig: class TestOIDCConfig:
"""Test OIDCConfig class.""" """Test OIDCConfig class."""
def test_init_defaults(self): def test_init_defaults(self):
"""Config should initialize with disabled state.""" """Config should initialize disabled with no issuers or audiences."""
config = OIDCConfig() config = OIDCConfig()
assert config.enabled is False assert config.enabled is False
assert config.issuer == "" assert config.issuers == []
assert config.audience == "" assert config.audiences == []
assert config.jwks_uri == ""
def test_configure_sets_values(self): def test_configure_sets_values(self):
"""configure should set all values.""" """configure should set all values."""
config = OIDCConfig() config = OIDCConfig()
config.configure( config.configure(
enabled=True, enabled=True,
issuer="https://auth.example.com", issuers=["https://auth.example.com"],
audience="core-api" audiences=["core-api"],
) )
assert config.enabled is True assert config.enabled is True
assert config.issuer == "https://auth.example.com" assert config.issuers == ["https://auth.example.com"]
assert config.audience == "core-api" assert config.audiences == ["core-api"]
assert config.jwks_uri == "https://auth.example.com/jwks/"
def test_configure_strips_trailing_slash(self): def test_configure_strips_trailing_slash(self):
"""configure should handle trailing slash in issuer.""" """configure should normalise issuers by dropping the trailing slash."""
config = OIDCConfig() config = OIDCConfig()
config.configure( config.configure(
enabled=True, enabled=True,
issuer="https://auth.example.com/", issuers=["https://auth.example.com/"],
audience="core-api" audiences=["core-api"],
) )
assert config.jwks_uri == "https://auth.example.com/jwks/" assert config.issuers == ["https://auth.example.com"]
def test_configure_accepts_multiple_issuers(self):
"""The point of the multi-issuer change: more than one is allowed."""
config = OIDCConfig()
config.configure(
enabled=True,
issuers=["https://a.example.com/", "https://b.example.com"],
audiences=["core-api", "other"],
)
assert config.issuers == ["https://a.example.com", "https://b.example.com"]
assert config.audiences == ["core-api", "other"]
def test_get_jwks_uri_derives_from_issuer(self):
"""The JWKS URI is derived per issuer rather than configured."""
config = OIDCConfig()
assert config.get_jwks_uri("https://auth.example.com") == "https://auth.example.com/jwks/"
assert config.get_jwks_uri("https://auth.example.com/") == "https://auth.example.com/jwks/"
def test_is_valid_issuer_only_accepts_configured(self):
"""
An unconfigured issuer must be rejected. This is the security-relevant
half of multi-issuer support: accepting any issuer would let a token
from an unrelated identity provider through.
"""
config = OIDCConfig()
config.configure(
enabled=True,
issuers=["https://auth.example.com"],
audiences=["core-api"],
)
assert config.is_valid_issuer("https://auth.example.com") is True
assert config.is_valid_issuer("https://auth.example.com/") is True
assert config.is_valid_issuer("https://evil.example.com") is False
class TestGetJWKS: class TestGetJWKSForIssuer:
"""Test get_jwks function.""" """Test get_jwks_for_issuer function."""
ISSUER = "https://auth.example.com"
def test_returns_empty_when_disabled(self): def test_returns_empty_when_disabled(self):
"""get_jwks should return empty dict when OIDC disabled.""" """Should return an empty dict when OIDC is disabled."""
# Save original state
original_enabled = oidc_config.enabled original_enabled = oidc_config.enabled
try: try:
oidc_config.enabled = False oidc_config.enabled = False
# Clear the cache
get_jwks.cache_clear()
result = get_jwks() assert get_jwks_for_issuer(self.ISSUER) == {}
assert result == {}
finally: finally:
# Restore original state
oidc_config.enabled = original_enabled oidc_config.enabled = original_enabled
get_jwks.cache_clear()
@patch("src.auth.oidc.httpx.get") @patch("src.auth.oidc.httpx.get")
def test_fetches_jwks_when_enabled(self, mock_get): def test_fetches_jwks_when_enabled(self, mock_get):
"""get_jwks should fetch JWKS when enabled.""" """Should fetch from the issuer's derived JWKS URI."""
# Save original state
original_enabled = oidc_config.enabled original_enabled = oidc_config.enabled
original_jwks_uri = oidc_config.jwks_uri
try: try:
oidc_config.enabled = True oidc_config.enabled = True
oidc_config.jwks_uri = "https://auth.example.com/jwks/"
get_jwks.cache_clear()
mock_response = MagicMock() mock_response = MagicMock()
mock_response.json.return_value = {"keys": [{"kid": "test"}]} mock_response.json.return_value = {"keys": [{"kid": "abc"}]}
mock_response.raise_for_status = MagicMock()
mock_get.return_value = mock_response mock_get.return_value = mock_response
result = get_jwks() result = get_jwks_for_issuer(self.ISSUER)
assert "keys" in result assert result == {"keys": [{"kid": "abc"}]}
mock_get.assert_called_once() mock_get.assert_called_once()
assert mock_get.call_args[0][0] == f"{self.ISSUER}/jwks/"
finally: finally:
oidc_config.enabled = original_enabled oidc_config.enabled = original_enabled
oidc_config.jwks_uri = original_jwks_uri
get_jwks.cache_clear()
@patch("src.auth.oidc.httpx.get") @patch("src.auth.oidc.httpx.get")
def test_raises_exception_on_error(self, mock_get): def test_caches_per_issuer(self, mock_get):
"""get_jwks should raise HTTPException on fetch error.""" """
# Save original state A second call for the same issuer must not refetch, and a different
issuer must. Caching by issuer is the behaviour the multi-issuer change
introduced, and a shared cache would have served one issuer's keys for
another — which would be a verification bypass, not just a slow path.
"""
original_enabled = oidc_config.enabled original_enabled = oidc_config.enabled
original_jwks_uri = oidc_config.jwks_uri
try: try:
oidc_config.enabled = True oidc_config.enabled = True
oidc_config.jwks_uri = "https://auth.example.com/jwks/" mock_response = MagicMock()
get_jwks.cache_clear() mock_response.json.return_value = {"keys": []}
mock_get.return_value = mock_response
mock_get.side_effect = Exception("Connection error") get_jwks_for_issuer(self.ISSUER)
get_jwks_for_issuer(self.ISSUER + "/") # same issuer, normalised
assert mock_get.call_count == 1
get_jwks_for_issuer("https://other.example.com")
assert mock_get.call_count == 2
finally:
oidc_config.enabled = original_enabled
@patch("src.auth.oidc.httpx.get")
def test_raises_503_on_fetch_error(self, mock_get):
"""A JWKS fetch failure should surface as 503, not leak the cause."""
original_enabled = oidc_config.enabled
try:
oidc_config.enabled = True
mock_get.side_effect = Exception("Connection failed")
with pytest.raises(HTTPException) as exc_info: with pytest.raises(HTTPException) as exc_info:
get_jwks() get_jwks_for_issuer(self.ISSUER)
assert exc_info.value.status_code == 503 assert exc_info.value.status_code == 503
finally: finally:
oidc_config.enabled = original_enabled oidc_config.enabled = original_enabled
oidc_config.jwks_uri = original_jwks_uri
get_jwks.cache_clear()
class TestGetCurrentUser: class TestGetCurrentUser:
@@ -121,9 +176,7 @@ class TestGetCurrentUser:
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_returns_none_when_disabled(self): async def test_returns_none_when_disabled(self):
"""get_current_user should return None when OIDC disabled.""" """get_current_user should return None when OIDC disabled."""
# Save original state
original_enabled = oidc_config.enabled original_enabled = oidc_config.enabled
try: try:
oidc_config.enabled = False oidc_config.enabled = False
@@ -136,9 +189,7 @@ class TestGetCurrentUser:
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_raises_401_when_enabled_without_token(self): async def test_raises_401_when_enabled_without_token(self):
"""get_current_user should raise 401 when enabled but no token.""" """get_current_user should raise 401 when enabled but no token."""
# Save original state
original_enabled = oidc_config.enabled original_enabled = oidc_config.enabled
try: try:
oidc_config.enabled = True oidc_config.enabled = True
@@ -157,11 +208,12 @@ class TestOIDCGlobalConfig:
"""oidc_config should be an OIDCConfig instance.""" """oidc_config should be an OIDCConfig instance."""
assert isinstance(oidc_config, OIDCConfig) assert isinstance(oidc_config, OIDCConfig)
def test_global_config_starts_disabled(self): def test_global_config_exposes_the_multi_issuer_surface(self):
"""oidc_config should start disabled by default.""" """
# This tests the initial state before any configure() is called Asserts the shape rather than the values, since the live state depends
# The actual state depends on app configuration on app configuration. These four are what callers depend on.
assert hasattr(oidc_config, 'enabled') """
assert hasattr(oidc_config, 'issuer') assert hasattr(oidc_config, "enabled")
assert hasattr(oidc_config, 'audience') assert hasattr(oidc_config, "issuers")
assert hasattr(oidc_config, 'jwks_uri') assert hasattr(oidc_config, "audiences")
assert callable(oidc_config.get_jwks_uri)
+21 -8
View File
@@ -4,6 +4,7 @@ from fastapi.testclient import TestClient
from unittest.mock import patch, AsyncMock, MagicMock from unittest.mock import patch, AsyncMock, MagicMock
from src.main import app from src.main import app
from src.domains.tools.controller import tools_controller
@pytest.fixture @pytest.fixture
@@ -36,9 +37,21 @@ class TestDNSLookup:
) )
assert response.status_code == 200 assert response.status_code == 200
@patch("src.controllers.tools_controller.DNSService") def test_dns_lookup_returns_result(self, client):
def test_dns_lookup_returns_result(self, mock_dns_class, client): """DNS lookup should return lookup results.
"""DNS lookup should return lookup results."""
Patches the live singleton's `dns_service` attribute, not the
`src.controllers.tools_controller.DNSService` class: that module is
the legacy top-level package (not wired into `src.main`, see
CLAUDE.md "Legacy top-level packages"). `client` exercises
`src.main.app`, which routes through
`src.domains.tools.controller.tools_controller`, a singleton built
at import time — so patching the class there would also miss,
since `tools_controller.dns_service` is already a constructed
instance by the time a test patches the class. Patching the
instance attribute directly is the only patch that actually
intercepts this request path.
"""
mock_response = MagicMock() mock_response = MagicMock()
mock_response.success = True mock_response.success = True
mock_response.domain = "example.com" mock_response.domain = "example.com"
@@ -59,12 +72,12 @@ class TestDNSLookup:
mock_service = MagicMock() mock_service = MagicMock()
mock_service.lookup = AsyncMock(return_value=mock_response) mock_service.lookup = AsyncMock(return_value=mock_response)
mock_dns_class.return_value = mock_service
response = client.post( with patch.object(tools_controller, "dns_service", mock_service):
"/tools/dns/lookup", response = client.post(
json={"domain": "example.com", "record_type": "A"} "/tools/dns/lookup",
) json={"domain": "example.com", "record_type": "A"}
)
data = response.json() data = response.json()
assert data["success"] is True assert data["success"] is True