From 01349a83f22c73524c6a4945c8c9f3fd3191cb43 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Sun, 9 Aug 2026 15:21:43 +0200 Subject: [PATCH] test: repair the suite against the current API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- tests/test_auth_controller.py | 28 ++++-- tests/test_config.py | 10 +- tests/test_oidc.py | 176 ++++++++++++++++++++++------------ 3 files changed, 138 insertions(+), 76 deletions(-) diff --git a/tests/test_auth_controller.py b/tests/test_auth_controller.py index a046318..1d4475c 100644 --- a/tests/test_auth_controller.py +++ b/tests/test_auth_controller.py @@ -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 diff --git a/tests/test_config.py b/tests/test_config.py index d87837a..d49ea91 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -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. diff --git a/tests/test_oidc.py b/tests/test_oidc.py index b93e52a..879a618 100644 --- a/tests/test_oidc.py +++ b/tests/test_oidc.py @@ -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)