fix(config): correct OIDC token-type/scopes env keys; address review round 1

- _DEFAULTS keys for NEXTCLOUD_OIDC_TOKEN_TYPE / NEXTCLOUD_OIDC_SCOPES were
  registered as oidc_* (uppercasing to OIDC_*), so dynaconf
  (ignore_unknown_envvars) never read the NEXTCLOUD_-prefixed env vars and the
  fields stayed at their defaults. Prefix the keys to match _field_map; add a
  regression test.
- Add gte=1 validator for HEALTH_READY_REFRESH_INTERVAL and a 1..65535 range
  validator for PORT.
- Tie ReadinessCache.ttl_seconds to 2x the configured refresh interval so
  is_stale() stays meaningful when the interval is tuned.
- Raise the refresh-loop exception log from DEBUG to WARNING.
- Make health_ready a sync handler (no awaits); dedupe the localhost fallback
  into _DEFAULT_MCP_SERVER_URL; use pytest.approx for the float default.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-06-10 21:45:02 +02:00
co-authored by Claude Opus 4.8
parent 6ef7786cec
commit cc2ce6e853
5 changed files with 47 additions and 14 deletions
+17 -9
View File
@@ -431,6 +431,9 @@ def _clear_vector_sync_state() -> None:
# background loop, cached, and *reported but non-gating* — and the probe path
# never performs external I/O.
# Fallback public URL when NEXTCLOUD_MCP_SERVER_URL is unset (dev/local).
_DEFAULT_MCP_SERVER_URL = "http://localhost:8000"
_readiness_cache = ReadinessCache(ttl_seconds=30.0)
@@ -501,6 +504,9 @@ async def _readiness_refresh_loop() -> None:
Started in the shared lifespan task group; cancelled on shutdown.
"""
interval = get_settings().health_ready_refresh_interval
# Keep the staleness window in step with the configured cadence so
# is_stale() stays meaningful when the interval is tuned off its default.
_readiness_cache.ttl_seconds = interval * 2
logger.info(
"Readiness dependency-health refresh loop started (every %ss)", interval
)
@@ -508,7 +514,9 @@ async def _readiness_refresh_loop() -> None:
try:
await _refresh_dependency_health()
except Exception: # noqa: BLE001 - never let the loop die
logger.debug("Readiness dependency refresh iteration failed", exc_info=True)
logger.warning(
"Readiness dependency refresh iteration failed", exc_info=True
)
await anyio.sleep(interval)
@@ -657,7 +665,7 @@ async def load_oauth_client_credentials(
if registration_endpoint:
logger.info("Dynamic client registration available")
mcp_server_url = (
get_settings().nextcloud_mcp_server_url or "http://localhost:8000"
get_settings().nextcloud_mcp_server_url or _DEFAULT_MCP_SERVER_URL
)
redirect_uris = [
f"{mcp_server_url}/oauth/callback", # Unified callback (flow determined by query param)
@@ -954,7 +962,7 @@ async def setup_oauth_config():
public_issuer_url = settings.nextcloud_public_issuer_url
client_issuer = public_issuer_url if public_issuer_url else issuer
# Get MCP server URL for audience validation
mcp_server_url = settings.nextcloud_mcp_server_url or "http://localhost:8000"
mcp_server_url = settings.nextcloud_mcp_server_url or _DEFAULT_MCP_SERVER_URL
nextcloud_resource_uri = settings.nextcloud_resource_uri or nextcloud_host
# Warn if resource URIs are not configured (required for ADR-005 compliance)
@@ -1018,7 +1026,7 @@ async def setup_oauth_config():
oauth_client = None
# Create auth settings
mcp_server_url = settings.nextcloud_mcp_server_url or "http://localhost:8000"
mcp_server_url = settings.nextcloud_mcp_server_url or _DEFAULT_MCP_SERVER_URL
# Note: We don't set required_scopes here anymore.
# Scopes are now advertised via PRM endpoint and enforced per-tool.
@@ -1139,7 +1147,7 @@ async def setup_oauth_config_for_multi_user_basic(
logger.info(" Introspection: %s", introspection_uri)
# Get MCP server URL for audience validation
mcp_server_url = settings.nextcloud_mcp_server_url or "http://localhost:8000"
mcp_server_url = settings.nextcloud_mcp_server_url or _DEFAULT_MCP_SERVER_URL
nextcloud_resource_uri = settings.nextcloud_resource_uri or nextcloud_host
# Use public issuer URL for JWT validation if set (handles Docker internal/external URL mismatch)
@@ -1664,7 +1672,7 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None =
raise ValueError("NEXTCLOUD_HOST is required for OAuth mode")
mcp_server_url = (
settings.nextcloud_mcp_server_url or "http://localhost:8000"
settings.nextcloud_mcp_server_url or _DEFAULT_MCP_SERVER_URL
)
nextcloud_resource_uri = (
settings.nextcloud_resource_uri or nextcloud_host_for_context
@@ -1729,7 +1737,7 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None =
# Create oauth_context for management API authentication
nextcloud_host_for_context = settings.nextcloud_host
mcp_server_url = (
settings.nextcloud_mcp_server_url or "http://localhost:8000"
settings.nextcloud_mcp_server_url or _DEFAULT_MCP_SERVER_URL
)
discovery_url = (
settings.oidc_discovery_url
@@ -2193,7 +2201,7 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None =
}
)
async def health_ready(request):
def health_ready(request):
"""Readiness probe endpoint.
Gates **only** on local, cheap configuration checks (that the process is
@@ -2732,7 +2740,7 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None =
@app.exception_handler(InsufficientScopeError)
async def handle_insufficient_scope(request, exc: InsufficientScopeError):
"""Return 403 with WWW-Authenticate header for scope challenges."""
resource_url = settings.nextcloud_mcp_server_url or "http://localhost:8000"
resource_url = settings.nextcloud_mcp_server_url or _DEFAULT_MCP_SERVER_URL
scope_str = " ".join(exc.missing_scopes)
return JSONResponse(
+6 -2
View File
@@ -37,8 +37,10 @@ _DEFAULTS: dict[str, Any] = {
"cookie_secure": None,
# OAuth/OIDC
"oidc_discovery_url": None,
"oidc_token_type": "Bearer",
"oidc_scopes": "",
# Keys must uppercase to the env var dynaconf reads (ignore_unknown_envvars):
# NEXTCLOUD_OIDC_TOKEN_TYPE / NEXTCLOUD_OIDC_SCOPES, matching _field_map.
"nextcloud_oidc_token_type": "Bearer",
"nextcloud_oidc_scopes": "",
"port": 8000,
"nextcloud_oidc_client_id": None,
"nextcloud_oidc_client_secret": None,
@@ -312,6 +314,8 @@ _dynaconf = Dynaconf(
Validator("VECTOR_SYNC_QUEUE_MAX_SIZE", gte=1),
Validator("VECTOR_SYNC_METRICS_REFRESH_INTERVAL", gte=1),
Validator("VECTOR_SYNC_USER_POLL_INTERVAL", gte=1),
Validator("HEALTH_READY_REFRESH_INTERVAL", gte=1),
Validator("PORT", gte=1, lte=65535),
Validator("VERIFICATION_CONCURRENCY", gte=1),
Validator("DOCUMENT_CHUNK_SIZE", gte=1),
Validator("DOCUMENT_PARSE_TIMEOUT_SECONDS", gte=1),
@@ -38,8 +38,9 @@ class ReadinessCache(BaseModel):
"""Time-bounded snapshot of external dependency health.
Written only by the background refresh loop and read only by the readiness
handler. No locking: assignment into ``statuses`` is atomic under the GIL,
and a reader tolerates seeing the previous value for one entry.
handler. The single-writer invariant (one refresh loop) is what makes this
safe without a lock; a reader simply tolerates seeing the previous value for
one entry until the next refresh.
"""
ttl_seconds: float = 30.0
+20
View File
@@ -95,6 +95,26 @@ class TestGetSettings:
assert settings.qdrant_api_key == "test-key"
assert settings.qdrant_location is None
@patch.dict(
os.environ,
{
"NEXTCLOUD_OIDC_TOKEN_TYPE": "jwt",
"NEXTCLOUD_OIDC_SCOPES": "openid profile",
},
clear=True,
)
def test_get_settings_oidc_token_type_and_scopes_from_env(self):
"""NEXTCLOUD_OIDC_TOKEN_TYPE / _SCOPES must reach settings (regression).
The settings migration first registered these under _DEFAULTS keys that
uppercased to OIDC_* instead of NEXTCLOUD_OIDC_*, so dynaconf silently
ignored the env vars and always returned the defaults.
"""
_reload_config()
settings = get_settings()
assert settings.oidc_token_type == "jwt"
assert settings.oidc_scopes == "openid profile"
@patch.dict(
os.environ,
{"QDRANT_LOCATION": "/app/data/qdrant"},
+1 -1
View File
@@ -18,7 +18,7 @@ def test_dependency_status_defaults():
status = DependencyStatus(name="nextcloud")
assert status.healthy is None # not yet checked
assert status.detail == "pending"
assert status.checked_at == 0.0
assert status.checked_at == pytest.approx(0.0)
def test_update_and_snapshot_round_trip():