fix(auth): address PR #758 round-2 review
- oauth_login_callback's integrated-mode token-exchange branch now reuses the shared discovery cache via get_oidc_discovery (round-2 finding 1). - AS proxy flow now generates an OIDC nonce in oauth_authorize, stores it on ASProxySession, forwards it to the IdP, and passes it as expected_nonce to verify_id_token in _oauth_callback_as_proxy (round-2 finding 2). - Consolidate the two parallel discovery caches: oauth_routes' local _discovery_cache and _get_cached_discovery are removed; all callers now go through token_utils.get_oidc_discovery, which acquires the follow_redirects=True knob it needs for Nextcloud installs without pretty URLs (round-2 finding 3). - Demote per-user INFO logs in oauth_tools.py (check_logged_in, get_provisioning_status) to DEBUG; the elicitation auth URL is no longer logged because it contains a sensitive state token (round-2 finding 4). Also pin nonce binding behaviour with a new unit test that asserts _oauth_callback_as_proxy forwards session.nonce to verify_id_token, and update test mocks to track the cache consolidation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
4c84d82984
commit
c33d52ea91
@@ -407,11 +407,11 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo
|
||||
else:
|
||||
# Integrated mode (Nextcloud OIDC)
|
||||
discovery_url = oauth_config.get("discovery_url")
|
||||
async with nextcloud_httpx_client() as http_client:
|
||||
response = await http_client.get(discovery_url)
|
||||
response.raise_for_status()
|
||||
discovery = response.json()
|
||||
token_endpoint = discovery["token_endpoint"]
|
||||
# Use the shared 5-minute discovery cache; oauth_login() above
|
||||
# has already populated it for this discovery_url so the
|
||||
# callback should hit the cache rather than re-fetching.
|
||||
discovery = await get_oidc_discovery(discovery_url)
|
||||
token_endpoint = discovery["token_endpoint"]
|
||||
|
||||
token_params = {
|
||||
"grant_type": "authorization_code",
|
||||
|
||||
@@ -38,6 +38,7 @@ from nextcloud_mcp_server.auth.client_registry import get_client_registry
|
||||
from nextcloud_mcp_server.auth.storage import RefreshTokenStorage
|
||||
from nextcloud_mcp_server.auth.token_utils import (
|
||||
IdTokenVerificationError,
|
||||
get_oidc_discovery,
|
||||
verify_id_token,
|
||||
)
|
||||
from nextcloud_mcp_server.config import get_settings
|
||||
@@ -91,6 +92,7 @@ class ASProxySession:
|
||||
code_challenge: str
|
||||
code_challenge_method: str
|
||||
requested_scopes: str
|
||||
nonce: str = ""
|
||||
created_at: float = field(default_factory=time.time)
|
||||
expires_at: float = field(default_factory=lambda: time.time() + 600)
|
||||
|
||||
@@ -103,10 +105,6 @@ class ASProxySession:
|
||||
_proxy_codes: dict[str, ProxyCodeEntry] = {}
|
||||
_as_proxy_sessions: dict[str, ASProxySession] = {}
|
||||
|
||||
# OIDC discovery document cache (URL → (expires_at, data))
|
||||
_discovery_cache: dict[str, tuple[float, dict[str, Any]]] = {}
|
||||
_DISCOVERY_CACHE_TTL = 300 # 5 minutes
|
||||
|
||||
# DCR rate limiting (IP → [timestamps])
|
||||
_dcr_rate_limit: dict[str, list[float]] = {}
|
||||
_DCR_RATE_LIMIT_MAX = 10 # max requests
|
||||
@@ -138,26 +136,6 @@ def _transform_scopes_for_idp(scopes: str, resource_server_id: str) -> str:
|
||||
)
|
||||
|
||||
|
||||
async def _get_cached_discovery(url: str) -> dict[str, Any]:
|
||||
"""Fetch OIDC discovery document with caching (5-minute TTL).
|
||||
|
||||
Follows redirects so the configured discovery URL works against Nextcloud
|
||||
instances without pretty URLs enabled, where ``/.well-known/openid-configuration``
|
||||
issues a 301 to ``/index.php/.well-known/openid-configuration``.
|
||||
"""
|
||||
now = time.time()
|
||||
if url in _discovery_cache:
|
||||
expires_at, data = _discovery_cache[url]
|
||||
if now < expires_at:
|
||||
return data
|
||||
async with nextcloud_httpx_client(follow_redirects=True) as http_client:
|
||||
response = await http_client.get(url)
|
||||
response.raise_for_status()
|
||||
data = response.json()
|
||||
_discovery_cache[url] = (now + _DISCOVERY_CACHE_TTL, data)
|
||||
return data
|
||||
|
||||
|
||||
def _cleanup_expired_proxy_codes() -> None:
|
||||
"""Remove expired proxy codes and sessions."""
|
||||
now = time.time()
|
||||
@@ -316,6 +294,10 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse:
|
||||
# We do NOT forward PKCE to Nextcloud — the MCP server is a confidential client.
|
||||
server_state = secrets.token_urlsafe(32)
|
||||
|
||||
# OIDC nonce binds the IdP's ID token to THIS authorization request,
|
||||
# blocking ID-token replay across flows (PR #758 round-2 finding 2).
|
||||
server_nonce = secrets.token_urlsafe(32)
|
||||
|
||||
requested_scope = request.query_params.get("scope", "")
|
||||
default_scopes = "openid profile email"
|
||||
resource_scopes = oauth_config.get("scopes", "")
|
||||
@@ -333,6 +315,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse:
|
||||
code_challenge=code_challenge,
|
||||
code_challenge_method=code_challenge_method,
|
||||
requested_scopes=scopes,
|
||||
nonce=server_nonce,
|
||||
)
|
||||
|
||||
# Use MCP server's own client_id with Nextcloud
|
||||
@@ -359,7 +342,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse:
|
||||
status_code=500,
|
||||
)
|
||||
|
||||
discovery = await _get_cached_discovery(discovery_url)
|
||||
discovery = await get_oidc_discovery(discovery_url)
|
||||
authorization_endpoint = discovery["authorization_endpoint"]
|
||||
|
||||
# Replace internal Docker hostname with public URL for browser access
|
||||
@@ -395,6 +378,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse:
|
||||
"response_type": "code",
|
||||
"scope": idp_scope_str,
|
||||
"state": server_state,
|
||||
"nonce": server_nonce,
|
||||
"prompt": "consent",
|
||||
"resource": f"{mcp_server_url}/mcp", # MCP server audience
|
||||
}
|
||||
@@ -469,7 +453,7 @@ async def oauth_authorize_nextcloud(
|
||||
# supporting the offline_access scope.
|
||||
discovery_url = oauth_config.get("discovery_url")
|
||||
if discovery_url:
|
||||
disc = await _get_cached_discovery(discovery_url)
|
||||
disc = await get_oidc_discovery(discovery_url)
|
||||
scopes_supported = disc.get("scopes_supported")
|
||||
if scopes_supported is None or "offline_access" in scopes_supported:
|
||||
scopes += " offline_access"
|
||||
@@ -506,7 +490,7 @@ async def oauth_authorize_nextcloud(
|
||||
status_code=500,
|
||||
)
|
||||
|
||||
discovery = await _get_cached_discovery(discovery_url)
|
||||
discovery = await get_oidc_discovery(discovery_url)
|
||||
authorization_endpoint = discovery["authorization_endpoint"]
|
||||
|
||||
# Fix internal hostname for browser access
|
||||
@@ -623,7 +607,7 @@ async def oauth_callback_nextcloud(request: Request):
|
||||
status_code=500,
|
||||
)
|
||||
|
||||
discovery = await _get_cached_discovery(discovery_url)
|
||||
discovery = await get_oidc_discovery(discovery_url)
|
||||
token_endpoint = discovery["token_endpoint"]
|
||||
|
||||
# Build token exchange params
|
||||
@@ -917,7 +901,7 @@ async def _oauth_callback_as_proxy(
|
||||
status_code=500,
|
||||
)
|
||||
|
||||
discovery = await _get_cached_discovery(discovery_url)
|
||||
discovery = await get_oidc_discovery(discovery_url)
|
||||
token_endpoint = discovery["token_endpoint"]
|
||||
|
||||
# Exchange auth code with Nextcloud (server-side, confidential client, no PKCE)
|
||||
@@ -959,12 +943,18 @@ async def _oauth_callback_as_proxy(
|
||||
# transport could plant arbitrary identity claims into the proxy code
|
||||
# entry that gets handed back to the MCP client. Mirrors the
|
||||
# verification done in oauth_callback_nextcloud.
|
||||
#
|
||||
# ``expected_nonce`` is the per-request nonce we forwarded to the IdP
|
||||
# in oauth_authorize (PR #758 round-2 finding 2); falsy → skip nonce
|
||||
# check for backward compatibility with sessions stored before the
|
||||
# nonce field was added.
|
||||
id_token = nc_token_response.get("id_token")
|
||||
try:
|
||||
await verify_id_token(
|
||||
id_token,
|
||||
discovery_url=discovery_url,
|
||||
expected_audience=mcp_server_client_id,
|
||||
expected_nonce=session.nonce or None,
|
||||
)
|
||||
except IdTokenVerificationError as e:
|
||||
logger.error("AS proxy: ID token verification failed: %s", e)
|
||||
@@ -1252,7 +1242,7 @@ async def _token_refresh(request: Request, form) -> JSONResponse:
|
||||
status_code=500,
|
||||
)
|
||||
|
||||
discovery = await _get_cached_discovery(discovery_url)
|
||||
discovery = await get_oidc_discovery(discovery_url)
|
||||
token_endpoint = discovery["token_endpoint"]
|
||||
|
||||
# Proxy refresh request to Nextcloud
|
||||
@@ -1341,7 +1331,7 @@ async def oauth_register_proxy(request: Request) -> JSONResponse:
|
||||
registration_endpoint = None
|
||||
if discovery_url:
|
||||
try:
|
||||
discovery = await _get_cached_discovery(discovery_url)
|
||||
discovery = await get_oidc_discovery(discovery_url)
|
||||
registration_endpoint = discovery.get("registration_endpoint")
|
||||
except Exception:
|
||||
logger.warning("Failed to fetch OIDC discovery for DCR endpoint")
|
||||
|
||||
@@ -21,10 +21,11 @@ from ..http import nextcloud_httpx_client
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
# OIDC discovery + JWKS caches keyed by URL → (expires_at, data). Mirrors the
|
||||
# pattern in oauth_routes._get_cached_discovery so that ID-token verification
|
||||
# during the OAuth callback doesn't make two extra round-trips per login (PR
|
||||
# #758 finding 4). 5-minute TTL matches oauth_routes.
|
||||
# OIDC discovery + JWKS caches keyed by URL → (expires_at, data). Single
|
||||
# source of truth for the codebase: oauth_routes / browser_oauth_routes both
|
||||
# go through ``get_oidc_discovery`` which reads/writes _discovery_cache, so
|
||||
# the first discovery fetch primes the cache for all later callers (PR #758
|
||||
# round-2 nit 3). 5-minute TTL.
|
||||
_discovery_cache: dict[str, tuple[float, dict[str, Any]]] = {}
|
||||
_jwks_cache: dict[str, tuple[float, dict[str, Any]]] = {}
|
||||
_OIDC_CACHE_TTL = 300
|
||||
@@ -35,16 +36,26 @@ class IdTokenVerificationError(Exception):
|
||||
|
||||
|
||||
async def _get_cached(
|
||||
cache: dict[str, tuple[float, dict[str, Any]]], url: str
|
||||
cache: dict[str, tuple[float, dict[str, Any]]],
|
||||
url: str,
|
||||
*,
|
||||
follow_redirects: bool = False,
|
||||
) -> dict[str, Any]:
|
||||
"""Return cached JSON response for *url* or fetch + cache on miss/expiry."""
|
||||
"""Return cached JSON response for *url* or fetch + cache on miss/expiry.
|
||||
|
||||
``follow_redirects`` is forwarded to ``nextcloud_httpx_client``: discovery
|
||||
fetches against Nextcloud without pretty URLs need it (the configured
|
||||
``/.well-known/openid-configuration`` path issues a 301), but JWKS
|
||||
fetches deliberately stay strict — the URL came from the discovery
|
||||
document we already trust, so a redirect there would be suspicious.
|
||||
"""
|
||||
now = time.time()
|
||||
entry = cache.get(url)
|
||||
if entry is not None:
|
||||
expires_at, data = entry
|
||||
if now < expires_at:
|
||||
return data
|
||||
async with nextcloud_httpx_client() as http_client:
|
||||
async with nextcloud_httpx_client(follow_redirects=follow_redirects) as http_client:
|
||||
response = await http_client.get(url)
|
||||
response.raise_for_status()
|
||||
data = response.json()
|
||||
@@ -57,10 +68,13 @@ async def get_oidc_discovery(discovery_url: str) -> dict[str, Any]:
|
||||
|
||||
Shares the 5-minute discovery cache used by `verify_id_token`, so a
|
||||
callback that does discovery → token-exchange → ID-token verification
|
||||
reuses one HTTP round-trip instead of three. Public alias for `_get_cached`
|
||||
against `_discovery_cache` (PR #758 nits 5 & 6).
|
||||
reuses one HTTP round-trip instead of three. The fetch follows
|
||||
redirects because Nextcloud without pretty URLs returns 301 from
|
||||
``/.well-known/openid-configuration`` to ``/index.php/.well-known/...``.
|
||||
Single source of truth for OIDC discovery in the codebase
|
||||
(PR #758 round-2 nit 3).
|
||||
"""
|
||||
return await _get_cached(_discovery_cache, discovery_url)
|
||||
return await _get_cached(_discovery_cache, discovery_url, follow_redirects=True)
|
||||
|
||||
|
||||
async def verify_id_token(
|
||||
@@ -103,7 +117,7 @@ async def verify_id_token(
|
||||
raise IdTokenVerificationError("ID token missing from token response")
|
||||
|
||||
try:
|
||||
discovery = await _get_cached(_discovery_cache, discovery_url)
|
||||
discovery = await get_oidc_discovery(discovery_url)
|
||||
|
||||
issuer = discovery.get("issuer")
|
||||
jwks_uri = discovery.get("jwks_uri")
|
||||
|
||||
@@ -105,8 +105,12 @@ async def get_provisioning_status(ctx: Context, user_id: str) -> ProvisioningSta
|
||||
status = await astrolabe.get_background_sync_status(user_id)
|
||||
|
||||
if status.get("has_access"):
|
||||
logger.info(
|
||||
f" get_provisioning_status: ✓ App password FOUND for user_id={user_id}"
|
||||
# Demoted to debug (PR #758 round-2 nit 4): user_id ends up
|
||||
# in log aggregation on every call, which is noise in a
|
||||
# multi-tenant deployment.
|
||||
logger.debug(
|
||||
" get_provisioning_status: app password FOUND for user_id=%s",
|
||||
user_id,
|
||||
)
|
||||
provisioned_at_str = status.get("provisioned_at")
|
||||
return ProvisioningStatus(
|
||||
@@ -115,28 +119,28 @@ async def get_provisioning_status(ctx: Context, user_id: str) -> ProvisioningSta
|
||||
credential_type="app_password",
|
||||
)
|
||||
except Exception as e:
|
||||
logger.debug(f" App password check failed for {user_id}: {e}")
|
||||
logger.debug(" App password check failed for %s: %s", user_id, e)
|
||||
|
||||
# Check for OAuth refresh token (fallback)
|
||||
logger.info(
|
||||
f" get_provisioning_status: Looking up refresh token for user_id={user_id}"
|
||||
logger.debug(
|
||||
" get_provisioning_status: looking up refresh token for user_id=%s", user_id
|
||||
)
|
||||
storage = await get_shared_storage()
|
||||
|
||||
token_data = await storage.get_refresh_token(user_id)
|
||||
|
||||
if not token_data:
|
||||
logger.info(
|
||||
f" get_provisioning_status: ✗ No credentials found for user_id={user_id}"
|
||||
logger.debug(
|
||||
" get_provisioning_status: no credentials found for user_id=%s", user_id
|
||||
)
|
||||
return ProvisioningStatus(is_provisioned=False)
|
||||
|
||||
logger.info(
|
||||
f" get_provisioning_status: ✓ Refresh token FOUND for user_id={user_id}"
|
||||
)
|
||||
logger.info(f" flow_type: {token_data.get('flow_type')}")
|
||||
logger.info(
|
||||
f" provisioning_client_id: {token_data.get('provisioning_client_id', 'N/A')}"
|
||||
logger.debug(
|
||||
" get_provisioning_status: refresh token FOUND for user_id=%s "
|
||||
"flow_type=%s provisioning_client_id=%s",
|
||||
user_id,
|
||||
token_data.get("flow_type"),
|
||||
token_data.get("provisioning_client_id", "N/A"),
|
||||
)
|
||||
|
||||
# Convert timestamp to ISO format if present
|
||||
@@ -370,18 +374,22 @@ async def check_logged_in(ctx: Context, user_id: str) -> str:
|
||||
"yes" if logged in, or elicitation prompting for login
|
||||
"""
|
||||
try:
|
||||
# Check if already logged in
|
||||
logger.info(f"Checking provisioning status for user_id: {user_id}")
|
||||
# Demoted to debug (PR #758 round-2 nit 4): per-user logging at INFO
|
||||
# ends up in log aggregation on every check_logged_in call, which is
|
||||
# noise in a hosted multi-tenant deployment.
|
||||
logger.debug("Checking provisioning status for user_id=%s", user_id)
|
||||
status = await get_provisioning_status(ctx, user_id)
|
||||
logger.info(f" Provisioning status: is_provisioned={status.is_provisioned}")
|
||||
logger.debug(
|
||||
" Provisioning status for %s: is_provisioned=%s",
|
||||
user_id,
|
||||
status.is_provisioned,
|
||||
)
|
||||
|
||||
if status.is_provisioned:
|
||||
logger.info(f"✓ User {user_id} is already logged in - returning 'yes'")
|
||||
logger.info("=" * 60)
|
||||
logger.debug("User %s already logged in", user_id)
|
||||
return "yes"
|
||||
|
||||
logger.info(f"✗ User {user_id} is NOT logged in - triggering elicitation")
|
||||
logger.info("=" * 60)
|
||||
logger.debug("User %s NOT logged in — triggering elicitation", user_id)
|
||||
|
||||
# Not logged in - generate OAuth URL for Flow 2
|
||||
# Use settings (handles both ENABLE_BACKGROUND_OPERATIONS and ENABLE_OFFLINE_ACCESS)
|
||||
@@ -462,8 +470,11 @@ async def check_logged_in(ctx: Context, user_id: str) -> str:
|
||||
scopes=scopes,
|
||||
)
|
||||
|
||||
# Use elicitation to prompt user to login
|
||||
logger.info(f"Eliciting login for user {user_id} with URL: {auth_url}")
|
||||
# Use elicitation to prompt user to login. Logged at debug (PR #758
|
||||
# round-2 nit 4): the auth URL contains the per-request ``state``
|
||||
# token, which is sensitive enough that it shouldn't land in
|
||||
# multi-tenant log aggregation by default.
|
||||
logger.debug("Eliciting login for user %s (URL omitted)", user_id)
|
||||
|
||||
result = await ctx.elicit(
|
||||
message=f"Please log in to Nextcloud at the following URL:\n\n{auth_url}\n\nAfter completing the login, check the box below and click OK.",
|
||||
@@ -472,10 +483,15 @@ async def check_logged_in(ctx: Context, user_id: str) -> str:
|
||||
|
||||
if result.action == "accept":
|
||||
# Check if login was successful by looking for refresh token
|
||||
# Strategy: Try multiple lookup methods to handle both flows
|
||||
logger.info("User accepted login prompt, checking for refresh token")
|
||||
logger.info(f" State parameter: {state[:16]}...")
|
||||
logger.info(f" User ID: {user_id}")
|
||||
# Strategy: Try multiple lookup methods to handle both flows.
|
||||
# Demoted to debug (PR #758 round-2 nit 4): user_id + state
|
||||
# appear here on every elicitation accept.
|
||||
logger.debug(
|
||||
"User accepted login prompt; looking up refresh token "
|
||||
"(user_id=%s state=%s...)",
|
||||
user_id,
|
||||
state[:16],
|
||||
)
|
||||
|
||||
# First, try to find token by provisioning_client_id (Flow 2 from elicitation)
|
||||
refresh_token_data = (
|
||||
@@ -483,45 +499,39 @@ async def check_logged_in(ctx: Context, user_id: str) -> str:
|
||||
)
|
||||
|
||||
if refresh_token_data:
|
||||
logger.info("✓ Refresh token found via provisioning_client_id lookup")
|
||||
logger.info(
|
||||
f" Flow type: {refresh_token_data.get('flow_type', 'unknown')}"
|
||||
)
|
||||
logger.info(
|
||||
f" Provisioned at: {refresh_token_data.get('provisioned_at', 'unknown')}"
|
||||
logger.debug(
|
||||
"Refresh token found via provisioning_client_id lookup "
|
||||
"(flow_type=%s provisioned_at=%s)",
|
||||
refresh_token_data.get("flow_type", "unknown"),
|
||||
refresh_token_data.get("provisioned_at", "unknown"),
|
||||
)
|
||||
return "yes"
|
||||
|
||||
# Fallback: Try to find token by user_id (browser login or any other flow)
|
||||
logger.info(f"✗ No token found with provisioning_client_id={state[:16]}...")
|
||||
logger.info(f" Trying fallback lookup by user_id: {user_id}")
|
||||
logger.debug(
|
||||
"No token via provisioning_client_id=%s...; falling back to user_id=%s",
|
||||
state[:16],
|
||||
user_id,
|
||||
)
|
||||
|
||||
refresh_token_data = await storage.get_refresh_token(user_id)
|
||||
|
||||
if refresh_token_data:
|
||||
logger.info("✓ Refresh token found via user_id lookup")
|
||||
logger.info(
|
||||
f" Flow type: {refresh_token_data.get('flow_type', 'unknown')}"
|
||||
)
|
||||
logger.info(
|
||||
f" Provisioned at: {refresh_token_data.get('provisioned_at', 'unknown')}"
|
||||
)
|
||||
logger.info(
|
||||
f" Provisioning client ID: {refresh_token_data.get('provisioning_client_id', 'NULL')}"
|
||||
)
|
||||
logger.info(
|
||||
" Note: This token was created via browser login or different flow"
|
||||
logger.debug(
|
||||
"Refresh token found via user_id lookup "
|
||||
"(flow_type=%s provisioned_at=%s provisioning_client_id=%s)",
|
||||
refresh_token_data.get("flow_type", "unknown"),
|
||||
refresh_token_data.get("provisioned_at", "unknown"),
|
||||
refresh_token_data.get("provisioning_client_id", "NULL"),
|
||||
)
|
||||
return "yes"
|
||||
|
||||
# No token found by either method
|
||||
logger.warning(f"✗ No refresh token found for user {user_id}")
|
||||
logger.warning(
|
||||
f" Checked provisioning_client_id={state[:16]}... - NOT FOUND"
|
||||
)
|
||||
logger.warning(f" Checked user_id={user_id} - NOT FOUND")
|
||||
logger.warning(
|
||||
" This may indicate the user completed login but token wasn't stored"
|
||||
"No refresh token found for user_id=%s (checked provisioning_client_id=%s... and user_id) — "
|
||||
"user completed elicitation but token wasn't stored",
|
||||
user_id,
|
||||
state[:16],
|
||||
)
|
||||
|
||||
return (
|
||||
|
||||
Reference in New Issue
Block a user