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)