From b875eaf0690c41c930fa5cefd9e9f7721e2c7a52 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 3 May 2026 14:21:12 +0200 Subject: [PATCH] fix(auth): address PR #758 round-7 medium/minor review - Gate browser session creation on a successful refresh token. When the IdP returns no refresh token, SessionAuthBackend would silently reject every subsequent request and bounce the user back to /oauth/login in a loop. The callback now bails with a 400 + correlation ID + actionable hint about offline_access *before* writing browser_sessions or setting the cookie. Pinned by a new end-to-end unit test. - Evict orphaned browser_sessions rows in SessionAuthBackend when the associated refresh token is gone, instead of letting them accumulate until TTL cleanup. Best-effort; deletion errors stay non-fatal. - Demote identity-bearing logs in the Flow 2 OAuth callback (user_id, scopes, audience, expires_at) from INFO to DEBUG so they don't leak into multi-tenant log aggregation on every provision. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../auth/browser_oauth_routes.py | 59 ++++--- nextcloud_mcp_server/auth/oauth_routes.py | 25 +-- nextcloud_mcp_server/auth/session_backend.py | 11 ++ tests/unit/test_browser_oauth_routes.py | 151 +++++++++++++++++- tests/unit/test_oauth_logout.py | 10 +- 5 files changed, 221 insertions(+), 35 deletions(-) diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index c155c168..0152c5e1 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -568,30 +568,47 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo token_data.get("scope", "").split() if token_data.get("scope") else None ) - # Store refresh token (for background jobs ONLY) - if refresh_token: - logger.debug( - "Storing refresh token for user_id=%s state=%s... scopes=%s expires_at=%s", + # Store refresh token (for background jobs ONLY). The browser session + # itself is gated on this — without a refresh token, ``SessionAuthBackend`` + # would reject every subsequent request and silently bounce the user back + # to ``/oauth/login`` (PR #758 round-7 medium 1). + if not refresh_token: + correlation_id = secrets.token_urlsafe(8) + logger.error( + "No refresh token in token response — cannot establish browser " + "session (correlation_id=%s, user_id=%s)", + correlation_id, user_id, - state[:16], - granted_scopes, - refresh_expires_at, ) - await storage.store_refresh_token( - user_id=user_id, - refresh_token=refresh_token, - expires_at=refresh_expires_at, - flow_type="browser", # Browser-based login flow - provisioning_client_id=state, # Store state for unified session lookup - scopes=granted_scopes, + return HTMLResponse( + f"

Login Failed

" + f"

The identity provider did not return a refresh token, so a " + f"persistent session could not be established. Make sure " + f"offline_access is granted in the IdP configuration.

" + f"

Correlation ID: {html_escape(correlation_id)}

", + status_code=400, ) - logger.info( - "Refresh token stored for user %s (lookup key: %s...)", - user_id, - state[:16], - ) - else: - logger.warning("No refresh token in token response - cannot store session") + + logger.debug( + "Storing refresh token for user_id=%s state=%s... scopes=%s expires_at=%s", + user_id, + state[:16], + granted_scopes, + refresh_expires_at, + ) + await storage.store_refresh_token( + user_id=user_id, + refresh_token=refresh_token, + expires_at=refresh_expires_at, + flow_type="browser", # Browser-based login flow + provisioning_client_id=state, # Store state for unified session lookup + scopes=granted_scopes, + ) + logger.info( + "Refresh token stored for user %s (lookup key: %s...)", + user_id, + state[:16], + ) # Query and cache user profile (for browser UI display) access_token = token_data.get("access_token") diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index 919b04d8..92ac6268 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -702,16 +702,19 @@ async def oauth_callback_nextcloud(request: Request): # Some IdPs (e.g. AWS Cognito) return refresh_expires_in as a JSON # string rather than an int; coerce to be safe. refresh_expires_at = int(time.time()) + int(refresh_expires_in) - logger.info(" refresh_expires_in: %ss", refresh_expires_in) - logger.info(" refresh_expires_at: %s", refresh_expires_at) + logger.debug(" refresh_expires_in: %ss", refresh_expires_in) + logger.debug(" refresh_expires_at: %s", refresh_expires_at) - logger.info("Storing refresh token:") - logger.info(" user_id: %s", user_id) - logger.info(" flow_type: flow2") - logger.info(" token_audience: nextcloud") - logger.info(" provisioning_client_id: %s...", state[:16]) - logger.info(" scopes: %s", granted_scopes) - logger.info(" expires_at: %s", refresh_expires_at) + # Identity-bearing fields stay at DEBUG so they don't reach + # multi-tenant log aggregation on every Flow 2 provision (PR #758 + # round-7 minor). + logger.debug("Storing refresh token:") + logger.debug(" user_id: %s", user_id) + logger.debug(" flow_type: flow2") + logger.debug(" token_audience: nextcloud") + logger.debug(" provisioning_client_id: %s...", state[:16]) + logger.debug(" scopes: %s", granted_scopes) + logger.debug(" expires_at: %s", refresh_expires_at) await storage.store_refresh_token( user_id=user_id, @@ -722,8 +725,8 @@ async def oauth_callback_nextcloud(request: Request): scopes=granted_scopes, expires_at=refresh_expires_at, ) - logger.info("✓ Stored Flow 2 master refresh token for user %s", user_id) - logger.info("=" * 60) + logger.debug("✓ Stored Flow 2 master refresh token for user %s", user_id) + logger.debug("=" * 60) # Return success HTML page success_html = """ diff --git a/nextcloud_mcp_server/auth/session_backend.py b/nextcloud_mcp_server/auth/session_backend.py index 70c45146..87364f09 100644 --- a/nextcloud_mcp_server/auth/session_backend.py +++ b/nextcloud_mcp_server/auth/session_backend.py @@ -101,6 +101,17 @@ class SessionAuthBackend(AuthenticationBackend): session_id[:8], user_id, ) + # Proactively evict the orphan so the table doesn't accumulate + # rows that the auth check will keep rejecting until TTL + # cleanup (PR #758 round-7 minor). + try: + await storage.delete_browser_session(session_id) + except Exception as e: + logger.warning( + "Failed to delete orphaned browser session %s…: %s", + session_id[:8], + e, + ) return None return AuthCredentials(["authenticated"]), SimpleUser(user_id) diff --git a/tests/unit/test_browser_oauth_routes.py b/tests/unit/test_browser_oauth_routes.py index 0c86bb04..6f3ae7f5 100644 --- a/tests/unit/test_browser_oauth_routes.py +++ b/tests/unit/test_browser_oauth_routes.py @@ -6,9 +6,18 @@ tests / direct ``settings.set`` calls can leave the raw string in place, and ``bool("false")`` is ``True``. """ -import pytest +import json +import tempfile +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch -from nextcloud_mcp_server.auth import browser_oauth_routes +import httpx +import pytest +from cryptography.fernet import Fernet + +from nextcloud_mcp_server.auth import browser_oauth_routes, token_utils +from nextcloud_mcp_server.auth.browser_oauth_routes import oauth_login_callback +from nextcloud_mcp_server.auth.storage import RefreshTokenStorage pytestmark = pytest.mark.unit @@ -71,3 +80,141 @@ def test_should_use_secure_cookies_falls_back_to_http_scheme(monkeypatch): ), ) assert browser_oauth_routes._should_use_secure_cookies() is False + + +# --------------------------------------------------------------------------- +# oauth_login_callback: missing refresh_token must NOT create a session +# --------------------------------------------------------------------------- +# +# Pins PR #758 round-7 medium 1: when the IdP returns no refresh_token, +# ``SessionAuthBackend`` would silently reject every subsequent request +# (because ``get_refresh_token`` returns None), bouncing the user back to +# ``/oauth/login`` in a loop. The callback now bails with a 400 error page +# *before* any browser_sessions row or Set-Cookie header is created. + + +@pytest.fixture +def _clear_oidc_caches(): + token_utils._discovery_cache.clear() + token_utils._jwks_cache.clear() + token_utils._fetch_locks.clear() + yield + token_utils._discovery_cache.clear() + token_utils._jwks_cache.clear() + token_utils._fetch_locks.clear() + + +@pytest.fixture +async def _no_refresh_storage(): + with tempfile.TemporaryDirectory() as tmpdir: + s = RefreshTokenStorage( + db_path=str(Path(tmpdir) / "norefresh.db"), + encryption_key=Fernet.generate_key().decode(), + ) + await s.initialize() + yield s + + +async def test_callback_rejects_token_response_without_refresh_token( + _clear_oidc_caches, _no_refresh_storage +): + storage = _no_refresh_storage + state = "state-norefresh" + + await storage.store_oauth_session( + session_id=state, + client_id="browser-ui", + client_redirect_uri="/app", + state=state, + code_challenge="cc", + code_challenge_method="S256", + mcp_authorization_code="cv", + flow_type="browser", + ttl_seconds=600, + ) + + discovery = { + "issuer": "http://idp.example", + "token_endpoint": "http://idp.example/token", + } + + def handler(request: httpx.Request) -> httpx.Response: + if request.url.path.endswith("/.well-known/openid-configuration"): + return httpx.Response( + 200, + content=json.dumps(discovery).encode(), + headers={"content-type": "application/json"}, + ) + if str(request.url) == "http://idp.example/token": + # Successful token exchange but no refresh_token (e.g. IdP + # config without offline_access). + return httpx.Response( + 200, + content=json.dumps( + { + "access_token": "at", + "id_token": "id-token-stub", + "token_type": "Bearer", + } + ).encode(), + headers={"content-type": "application/json"}, + ) + return httpx.Response(404) + + transport = httpx.MockTransport(handler) + + def fake_client(**kwargs): + kwargs["transport"] = transport + return httpx.AsyncClient(**kwargs) + + request = MagicMock() + request.query_params = {"code": "abc", "state": state} + request.cookies = {} + request.app.state.oauth_context = { + "storage": storage, + "oauth_client": None, + "config": { + "discovery_url": "http://idp.example/.well-known/openid-configuration", + "client_id": "test", + "client_secret": "secret", + "mcp_server_url": "http://localhost", + }, + } + request.url_for = MagicMock(return_value="/oauth/login") + + fake_userinfo = {"sub": "alice", "preferred_username": "alice"} + + with ( + patch( + "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", + side_effect=fake_client, + ), + patch( + "nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client", + side_effect=fake_client, + ), + patch( + "nextcloud_mcp_server.auth.browser_oauth_routes.verify_id_token", + new=AsyncMock(return_value=fake_userinfo), + ), + patch( + "nextcloud_mcp_server.auth.browser_oauth_routes._get_userinfo_endpoint", + new=AsyncMock(return_value=None), + ), + ): + response = await oauth_login_callback(request) + + assert response.status_code == 400 + body = response.body.decode() + assert "Login Failed" in body + assert "refresh token" in body.lower() + + # No browser session row may have been created. + assert await storage.get_browser_session_user("ignored") is None + # Nothing under the verified user_id either. + assert await storage.get_refresh_token("alice") is None + + # No Set-Cookie header — the user must not walk away with an unusable + # session cookie. + set_cookie = response.headers.get("set-cookie", "") + assert "mcp_session" not in set_cookie diff --git a/tests/unit/test_oauth_logout.py b/tests/unit/test_oauth_logout.py index 9eee223d..c27960f6 100644 --- a/tests/unit/test_oauth_logout.py +++ b/tests/unit/test_oauth_logout.py @@ -586,7 +586,12 @@ async def test_session_backend_rejects_unknown_session(storage): async def test_session_backend_rejects_session_without_refresh_token(storage): - """Defense-in-depth: session row exists but user has no refresh token.""" + """Defense-in-depth: session row exists but user has no refresh token. + + PR #758 round-7 minor: rejection now also evicts the orphaned + ``browser_sessions`` row so the table doesn't accumulate dead entries + that the auth check will keep rejecting until TTL cleanup. + """ await storage.create_browser_session(session_id="sid-B", user_id="bob") # Note: NO refresh token stored for bob @@ -594,6 +599,9 @@ async def test_session_backend_rejects_session_without_refresh_token(storage): conn = _build_conn(cookie="sid-B", oauth_context={"storage": storage}) assert await backend.authenticate(conn) is None + # Orphan must be evicted on rejection. + assert await storage.get_browser_session_user("sid-B") is None + async def test_session_backend_rejects_when_no_cookie(storage): backend = SessionAuthBackend(oauth_enabled=True)