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 and6243f29replaced 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 inc1f16d4. Removed rather than repaired. Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Claude
@@ -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:
|
||||
"""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):
|
||||
"""Auth me endpoint should be in OpenAPI spec."""
|
||||
response = client.get("/openapi.json")
|
||||
spec = response.json()
|
||||
assert "/auth/me" in spec["paths"]
|
||||
assert "get" in spec["paths"]["/auth/me"]
|
||||
assert self.PATH in spec["paths"]
|
||||
assert "get" in spec["paths"][self.PATH]
|
||||
|
||||
def test_auth_me_returns_401_without_forward_auth(self, client):
|
||||
"""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
|
||||
assert response.status_code == 401
|
||||
|
||||
@@ -840,7 +850,7 @@ class TestAuthMeEndpoint:
|
||||
spec = response.json()
|
||||
|
||||
# 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 "200" in me_endpoint["responses"]
|
||||
|
||||
@@ -920,7 +930,11 @@ class TestForwardAuthParsing:
|
||||
|
||||
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
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
||||
@@ -89,10 +89,6 @@ class TestGetSettings:
|
||||
settings2 = get_settings()
|
||||
assert settings1 is settings2
|
||||
|
||||
def test_model_aliases_property(self):
|
||||
"""Model aliases property should return dict."""
|
||||
settings = get_settings()
|
||||
aliases = settings.model_aliases
|
||||
assert isinstance(aliases, dict)
|
||||
assert "gpt-3.5-turbo" in aliases
|
||||
assert "gpt-4" in aliases
|
||||
# Removed: test_model_aliases_property. Settings.model_aliases mapped
|
||||
# gpt-3.5-turbo and gpt-4 onto local models, and was deleted along with the
|
||||
# Ollama integration in c1f16d4. The test outlived the feature it covered.
|
||||
|
||||
+114
-62
@@ -1,118 +1,173 @@
|
||||
"""Tests for OIDC authentication module."""
|
||||
import pytest
|
||||
from unittest.mock import patch, MagicMock, AsyncMock
|
||||
from unittest.mock import patch, MagicMock
|
||||
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:
|
||||
"""Test OIDCConfig class."""
|
||||
|
||||
def test_init_defaults(self):
|
||||
"""Config should initialize with disabled state."""
|
||||
"""Config should initialize disabled with no issuers or audiences."""
|
||||
config = OIDCConfig()
|
||||
|
||||
assert config.enabled is False
|
||||
assert config.issuer == ""
|
||||
assert config.audience == ""
|
||||
assert config.jwks_uri == ""
|
||||
assert config.issuers == []
|
||||
assert config.audiences == []
|
||||
|
||||
def test_configure_sets_values(self):
|
||||
"""configure should set all values."""
|
||||
config = OIDCConfig()
|
||||
config.configure(
|
||||
enabled=True,
|
||||
issuer="https://auth.example.com",
|
||||
audience="core-api"
|
||||
issuers=["https://auth.example.com"],
|
||||
audiences=["core-api"],
|
||||
)
|
||||
|
||||
assert config.enabled is True
|
||||
assert config.issuer == "https://auth.example.com"
|
||||
assert config.audience == "core-api"
|
||||
assert config.jwks_uri == "https://auth.example.com/jwks/"
|
||||
assert config.issuers == ["https://auth.example.com"]
|
||||
assert config.audiences == ["core-api"]
|
||||
|
||||
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.configure(
|
||||
enabled=True,
|
||||
issuer="https://auth.example.com/",
|
||||
audience="core-api"
|
||||
issuers=["https://auth.example.com/"],
|
||||
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:
|
||||
"""Test get_jwks function."""
|
||||
class TestGetJWKSForIssuer:
|
||||
"""Test get_jwks_for_issuer function."""
|
||||
|
||||
ISSUER = "https://auth.example.com"
|
||||
|
||||
def test_returns_empty_when_disabled(self):
|
||||
"""get_jwks should return empty dict when OIDC disabled."""
|
||||
# Save original state
|
||||
"""Should return an empty dict when OIDC is disabled."""
|
||||
original_enabled = oidc_config.enabled
|
||||
|
||||
try:
|
||||
oidc_config.enabled = False
|
||||
# Clear the cache
|
||||
get_jwks.cache_clear()
|
||||
|
||||
result = get_jwks()
|
||||
|
||||
assert result == {}
|
||||
assert get_jwks_for_issuer(self.ISSUER) == {}
|
||||
finally:
|
||||
# Restore original state
|
||||
oidc_config.enabled = original_enabled
|
||||
get_jwks.cache_clear()
|
||||
|
||||
@patch("src.auth.oidc.httpx.get")
|
||||
def test_fetches_jwks_when_enabled(self, mock_get):
|
||||
"""get_jwks should fetch JWKS when enabled."""
|
||||
# Save original state
|
||||
"""Should fetch from the issuer's derived JWKS URI."""
|
||||
original_enabled = oidc_config.enabled
|
||||
original_jwks_uri = oidc_config.jwks_uri
|
||||
|
||||
try:
|
||||
oidc_config.enabled = True
|
||||
oidc_config.jwks_uri = "https://auth.example.com/jwks/"
|
||||
get_jwks.cache_clear()
|
||||
|
||||
mock_response = MagicMock()
|
||||
mock_response.json.return_value = {"keys": [{"kid": "test"}]}
|
||||
mock_response.raise_for_status = MagicMock()
|
||||
mock_response.json.return_value = {"keys": [{"kid": "abc"}]}
|
||||
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()
|
||||
assert mock_get.call_args[0][0] == f"{self.ISSUER}/jwks/"
|
||||
finally:
|
||||
oidc_config.enabled = original_enabled
|
||||
oidc_config.jwks_uri = original_jwks_uri
|
||||
get_jwks.cache_clear()
|
||||
|
||||
@patch("src.auth.oidc.httpx.get")
|
||||
def test_raises_exception_on_error(self, mock_get):
|
||||
"""get_jwks should raise HTTPException on fetch error."""
|
||||
# Save original state
|
||||
def test_caches_per_issuer(self, mock_get):
|
||||
"""
|
||||
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_jwks_uri = oidc_config.jwks_uri
|
||||
|
||||
try:
|
||||
oidc_config.enabled = True
|
||||
oidc_config.jwks_uri = "https://auth.example.com/jwks/"
|
||||
get_jwks.cache_clear()
|
||||
mock_response = MagicMock()
|
||||
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:
|
||||
get_jwks()
|
||||
get_jwks_for_issuer(self.ISSUER)
|
||||
|
||||
assert exc_info.value.status_code == 503
|
||||
finally:
|
||||
oidc_config.enabled = original_enabled
|
||||
oidc_config.jwks_uri = original_jwks_uri
|
||||
get_jwks.cache_clear()
|
||||
|
||||
|
||||
class TestGetCurrentUser:
|
||||
@@ -121,9 +176,7 @@ class TestGetCurrentUser:
|
||||
@pytest.mark.asyncio
|
||||
async def test_returns_none_when_disabled(self):
|
||||
"""get_current_user should return None when OIDC disabled."""
|
||||
# Save original state
|
||||
original_enabled = oidc_config.enabled
|
||||
|
||||
try:
|
||||
oidc_config.enabled = False
|
||||
|
||||
@@ -136,9 +189,7 @@ class TestGetCurrentUser:
|
||||
@pytest.mark.asyncio
|
||||
async def test_raises_401_when_enabled_without_token(self):
|
||||
"""get_current_user should raise 401 when enabled but no token."""
|
||||
# Save original state
|
||||
original_enabled = oidc_config.enabled
|
||||
|
||||
try:
|
||||
oidc_config.enabled = True
|
||||
|
||||
@@ -157,11 +208,12 @@ class TestOIDCGlobalConfig:
|
||||
"""oidc_config should be an OIDCConfig instance."""
|
||||
assert isinstance(oidc_config, OIDCConfig)
|
||||
|
||||
def test_global_config_starts_disabled(self):
|
||||
"""oidc_config should start disabled by default."""
|
||||
# This tests the initial state before any configure() is called
|
||||
# The actual state depends on app configuration
|
||||
assert hasattr(oidc_config, 'enabled')
|
||||
assert hasattr(oidc_config, 'issuer')
|
||||
assert hasattr(oidc_config, 'audience')
|
||||
assert hasattr(oidc_config, 'jwks_uri')
|
||||
def test_global_config_exposes_the_multi_issuer_surface(self):
|
||||
"""
|
||||
Asserts the shape rather than the values, since the live state depends
|
||||
on app configuration. These four are what callers depend on.
|
||||
"""
|
||||
assert hasattr(oidc_config, "enabled")
|
||||
assert hasattr(oidc_config, "issuers")
|
||||
assert hasattr(oidc_config, "audiences")
|
||||
assert callable(oidc_config.get_jwks_uri)
|
||||
|
||||
Reference in New Issue
Block a user