From 6e7c821761d59b7ce468e760f1e711ebf2039b44 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 12 May 2026 20:44:17 +0200 Subject: [PATCH] fix(config): derive mode flags in Settings.__post_init__; address review round 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The integration jobs for `mcp-multi-user-basic` and `mcp-login-flow` were failing with HTTP 500s. Root cause: `get_settings()` builds a fresh Settings on every call (not cached). Commits 3 and 4 set the derived `enable_login_flow` / `enable_multi_user_basic_auth` flags as a side effect of `detect_auth_mode`. detect_auth_mode runs once at startup, against the Settings instance owned by `validate_configuration`. Every per-request call site that does `settings = get_settings()` got a fresh Settings with both flags at their default `False` (since the env-var aliases were dropped), causing the multi-user dispatcher in `context.py` to take the wrong branch and crash. Fix: move the derivation into `Settings.__post_init__`. Every Settings instance now carries correct flags from the moment it's constructed — no caching needed, no mutation-after-construction race. detect_auth_mode becomes a pure reader of the already-derived state. The legacy env-var deprecation check moves with it. It also picks up the reviewer's truthy-string fix: previously `os.getenv(legacy)` fired for the literal string "false" (a non-empty Python string is truthy), which would have errored on any user with a leftover `ENABLE_LOGIN_FLOW=false` in their `.env`. The check now only fires when the value lowercases to one of {"1", "true", "yes", "on"}. - nextcloud_mcp_server/config.py: extend Settings.__post_init__ with the legacy-deprecation block and the derived-flag derivation (resolve mode from deployment_mode + username/password, set flags). - nextcloud_mcp_server/config_validators.py: drop the `_sync_derived_flags` helper (superseded by __post_init__). Drop the legacy-env-var deprecation block (moved). `detect_auth_mode` is now pure — no mutation. Drop the now-unused `import os`. - tests/unit/test_config_validators.py: legacy-env-var tests now expect `ValueError` at `Settings(...)` construction (via `get_settings()`), not at `detect_auth_mode` call. Added two new tests: * `test_legacy_env_var_check_ignores_falsy_strings` — pins the truthy-string fix (reviewer round 2 finding). * `test_derived_flags_stable_across_get_settings_calls` — regression test pinning the integration-test fix (two consecutive `get_settings()` calls return Settings instances with the same derived flags). Also reworked `test_login_flow_mode_auto_derives_enable_login_flow_flag` to assert at-construction derivation (not the old mutation pattern). - docs/configuration-migration-v2.md: dropped the duplicate `MCP_DEPLOYMENT_MODE=multi_user_basic` line (review round 2 nit — a sed artifact from commit 4). - docs/ADR-021-configuration-consolidation.md: sed-replaced the in-body `MCP_DEPLOYMENT_MODE=oauth_single_audience` examples with `login_flow` (review round 2 nit — only the status header was updated in commit 4). - tests/conftest.py: docstring comment for the multi-user-basic fixture switched from `ENABLE_MULTI_USER_BASIC_AUTH=true` to `MCP_DEPLOYMENT_MODE=multi_user_basic` (review round 2 nit). Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/ADR-021-configuration-consolidation.md | 10 +-- docs/configuration-migration-v2.md | 3 - nextcloud_mcp_server/config.py | 43 ++++++++++ nextcloud_mcp_server/config_validators.py | 59 ++++---------- tests/conftest.py | 2 +- tests/unit/test_config_validators.py | 88 +++++++++++++++++---- 6 files changed, 137 insertions(+), 68 deletions(-) diff --git a/docs/ADR-021-configuration-consolidation.md b/docs/ADR-021-configuration-consolidation.md index 2b47a724..513f8448 100644 --- a/docs/ADR-021-configuration-consolidation.md +++ b/docs/ADR-021-configuration-consolidation.md @@ -86,7 +86,7 @@ Add `MCP_DEPLOYMENT_MODE` environment variable to remove detection ambiguity: ```bash # Optional: Explicitly declare deployment mode -MCP_DEPLOYMENT_MODE=oauth_single_audience +MCP_DEPLOYMENT_MODE=login_flow # Valid values: single_user_basic, multi_user_basic, # oauth_single_audience, oauth_token_exchange @@ -114,7 +114,7 @@ TOKEN_STORAGE_DB=/path/to/tokens.db ```bash # Multi-user OAuth with semantic search NEXTCLOUD_HOST=https://nextcloud.example.com -MCP_DEPLOYMENT_MODE=oauth_single_audience # Explicit (optional) +MCP_DEPLOYMENT_MODE=login_flow # Explicit (optional) ENABLE_SEMANTIC_SEARCH=true # Auto-enables background ops QDRANT_URL=http://qdrant:6333 TOKEN_ENCRYPTION_KEY= @@ -271,7 +271,7 @@ def enable_semantic_search(self) -> bool: - `ENABLE_SEMANTIC_SEARCH=false` → `enable_background_operations=false` (unless explicitly set) **Mode selection tests**: -- `MCP_DEPLOYMENT_MODE=oauth_single_audience` → mode correctly detected +- `MCP_DEPLOYMENT_MODE=login_flow` → mode correctly detected - `MCP_DEPLOYMENT_MODE` conflicts with detected mode → validation error - No `MCP_DEPLOYMENT_MODE` → auto-detection works as before @@ -363,7 +363,7 @@ QDRANT_URL=http://qdrant:6333 **After** (simplified): ```bash NEXTCLOUD_HOST=https://nextcloud.example.com -MCP_DEPLOYMENT_MODE=oauth_single_audience # Explicit (optional) +MCP_DEPLOYMENT_MODE=login_flow # Explicit (optional) ENABLE_SEMANTIC_SEARCH=true # Auto-enables background operations TOKEN_ENCRYPTION_KEY= TOKEN_STORAGE_DB=/path/to/tokens.db @@ -384,7 +384,7 @@ TOKEN_STORAGE_DB=/path/to/tokens.db **After** (optional migration): ```bash NEXTCLOUD_HOST=https://nextcloud.example.com -MCP_DEPLOYMENT_MODE=oauth_single_audience +MCP_DEPLOYMENT_MODE=login_flow ENABLE_BACKGROUND_OPERATIONS=true # Renamed for clarity TOKEN_ENCRYPTION_KEY= TOKEN_STORAGE_DB=/path/to/tokens.db diff --git a/docs/configuration-migration-v2.md b/docs/configuration-migration-v2.md index 8d288b78..e1c62db8 100644 --- a/docs/configuration-migration-v2.md +++ b/docs/configuration-migration-v2.md @@ -207,9 +207,6 @@ NEXTCLOUD_OIDC_CLIENT_SECRET=secret NEXTCLOUD_HOST=https://nextcloud.example.com MCP_DEPLOYMENT_MODE=multi_user_basic -# Optional: Explicit mode declaration -MCP_DEPLOYMENT_MODE=multi_user_basic - # One variable handles both! ENABLE_SEMANTIC_SEARCH=true # Auto-enables background operations diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index c0aeab42..c345ce27 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -607,6 +607,49 @@ class Settings: f"Smaller chunks may lose context. Consider using at least 1024 characters." ) + # --- ADR-022 follow-up: deployment mode is the single source of truth --- + # The ENABLE_MULTI_USER_BASIC_AUTH and ENABLE_LOGIN_FLOW env vars were + # removed in favour of MCP_DEPLOYMENT_MODE. We do TWO things here: + # + # 1. Loud-fail if a user still has either legacy env var set to a + # truthy value (silent removal would have flipped them into the + # wrong runtime mode). Only fires for truthy strings, so an + # explicit `ENABLE_LOGIN_FLOW=false` in a leftover .env passes + # through harmlessly. + # 2. Derive `enable_login_flow` and `enable_multi_user_basic_auth` + # from the resolved deployment mode here, in __post_init__, so + # every Settings instance carries correct flags. (`get_settings()` + # builds a fresh Settings on each call — without this, the + # mutation that used to live in detect_auth_mode would only stick + # on the startup Settings instance, leaving per-request handlers + # with default False values.) + _truthy = {"1", "true", "yes", "on"} + for _legacy, _replacement in ( + ("ENABLE_MULTI_USER_BASIC_AUTH", "multi_user_basic"), + ("ENABLE_LOGIN_FLOW", "login_flow"), + ): + if os.environ.get(_legacy, "").strip().lower() in _truthy: + raise ValueError( + f"{_legacy} is no longer read from the environment. " + f"Set MCP_DEPLOYMENT_MODE={_replacement} instead " + "(ADR-022). The deployment mode is the single source " + "of truth for selecting an auth flow." + ) + + resolved_mode = (self.deployment_mode or "").strip().lower() + if not resolved_mode: + if self.nextcloud_username and self.nextcloud_password: + resolved_mode = "single_user_basic" + else: + # Default multi-user mode is Login Flow v2 (browser-based + # app-password acquisition); the un-augmented OAuth bearer + # pass-through it replaced needed unmerged Nextcloud + # user_oidc patches and is no longer supported. + resolved_mode = "login_flow" + + self.enable_multi_user_basic_auth = resolved_mode == "multi_user_basic" + self.enable_login_flow = resolved_mode == "login_flow" + def get_embedding_model_name(self) -> str: """ Get the active embedding model name based on provider priority. diff --git a/nextcloud_mcp_server/config_validators.py b/nextcloud_mcp_server/config_validators.py index 325deeb5..78af22d7 100644 --- a/nextcloud_mcp_server/config_validators.py +++ b/nextcloud_mcp_server/config_validators.py @@ -9,7 +9,6 @@ See ADR-020 for detailed architecture and deployment mode documentation. """ import logging -import os from dataclasses import dataclass from enum import Enum @@ -163,11 +162,16 @@ def detect_auth_mode(settings: Settings) -> AuthMode: """Detect authentication mode from configuration. Mode detection priority (ADR-021, updated for ADR-022): - 0. Explicit MCP_DEPLOYMENT_MODE (if set) - NEW in ADR-021 - 1. Multi-user BasicAuth - 2. Single-user BasicAuth + 0. Explicit MCP_DEPLOYMENT_MODE (if set) — NEW in ADR-021 + 1. Multi-user BasicAuth (only via explicit mode after ADR-022 follow-up) + 2. Single-user BasicAuth (auto-detected from credentials) 3. Login Flow v2 (default — was OAuth single-audience pre-ADR-022) + Pure function — the legacy-env-var deprecation and the derivation of + `enable_login_flow` / `enable_multi_user_basic_auth` now happen in + `Settings.__post_init__` so every Settings instance carries correct + flags regardless of how it was constructed. + Args: settings: Application settings @@ -175,31 +179,15 @@ def detect_auth_mode(settings: Settings) -> AuthMode: Detected AuthMode Raises: - ValueError: If explicit deployment_mode is invalid or conflicts with detected mode + ValueError: If explicit deployment_mode is unrecognised. """ logger = logging.getLogger(__name__) - # ADR-022 follow-up: fail loudly if a caller is still relying on the - # removed env-var aliases. Bypass dynaconf and read os.environ directly - # so the check survives even though the aliases are gone. - for legacy, replacement in ( - ("ENABLE_MULTI_USER_BASIC_AUTH", "multi_user_basic"), - ("ENABLE_LOGIN_FLOW", "login_flow"), - ): - if os.getenv(legacy): - raise ValueError( - f"{legacy} is no longer read from the environment. " - f"Set MCP_DEPLOYMENT_MODE={replacement} instead " - "(ADR-022). The deployment mode is the single source of " - "truth for selecting an auth flow." - ) - - # ADR-021: Check for explicit deployment mode first + # ADR-021: explicit deployment mode wins if settings.deployment_mode: mode_str = settings.deployment_mode.lower().strip() - # Map string to AuthMode enum mode_map = { "single_user_basic": AuthMode.SINGLE_USER_BASIC, "multi_user_basic": AuthMode.MULTI_USER_BASIC, @@ -215,41 +203,22 @@ def detect_auth_mode(settings: Settings) -> AuthMode: explicit_mode = mode_map[mode_str] logger.info(f"Using explicit deployment mode: {explicit_mode.value}") - _sync_derived_flags(settings, explicit_mode) return explicit_mode # Auto-detection (no explicit deployment_mode). # MULTI_USER_BASIC is no longer auto-detectable — the ENABLE_MULTI_USER_BASIC_AUTH - # env-var alias was dropped in the ADR-022 follow-up, so the only way to - # opt into that mode is `MCP_DEPLOYMENT_MODE=multi_user_basic` (handled - # above). The legacy env var fails loudly at the top of this function. + # env-var alias was dropped in the ADR-022 follow-up, so the only way + # to opt into that mode is `MCP_DEPLOYMENT_MODE=multi_user_basic` + # (handled above). The legacy env var fails loudly in + # `Settings.__post_init__`. - # Check for single-user BasicAuth (explicit credentials) if settings.nextcloud_username and settings.nextcloud_password: - _sync_derived_flags(settings, AuthMode.SINGLE_USER_BASIC) return AuthMode.SINGLE_USER_BASIC # Default: Login Flow v2 multi-user mode (browser-based app-password flow). - # The un-augmented OAuth bearer pass-through it replaced required unmerged - # Nextcloud user_oidc patches (see ADR-022). - _sync_derived_flags(settings, AuthMode.LOGIN_FLOW) return AuthMode.LOGIN_FLOW -def _sync_derived_flags(settings: Settings, mode: AuthMode) -> None: - """Derive internal feature flags from the resolved deployment mode. - - Some runtime call sites (app.py, context.py, auth/scope_authorization.py) - still read individual boolean flags rather than passing the mode around. - Keep those flags in sync with the mode here so the mode is the single - source of truth and users don't have to set redundant env vars. The - ENABLE_LOGIN_FLOW and ENABLE_MULTI_USER_BASIC_AUTH env-var aliases were - removed in the ADR-022 follow-up (PR #787). - """ - settings.enable_login_flow = mode == AuthMode.LOGIN_FLOW - settings.enable_multi_user_basic_auth = mode == AuthMode.MULTI_USER_BASIC - - def validate_configuration(settings: Settings) -> tuple[AuthMode, list[str]]: """Validate configuration for detected mode. diff --git a/tests/conftest.py b/tests/conftest.py index e8aade3f..f131c7e9 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -265,7 +265,7 @@ async def nc_mcp_basic_auth_client( ) -> AsyncGenerator[ClientSession, Any]: """ Fixture to create an MCP client session with BasicAuth credentials. - Connects to the multi-user BasicAuth MCP server on port 8003 with ENABLE_MULTI_USER_BASIC_AUTH=true. + Connects to the multi-user BasicAuth MCP server on port 8003 with MCP_DEPLOYMENT_MODE=multi_user_basic. Uses BasicAuth credentials for multi-user pass-through mode (ADR-020). Credentials are passed in Authorization header and forwarded to Nextcloud APIs. diff --git a/tests/unit/test_config_validators.py b/tests/unit/test_config_validators.py index f8d7f907..7c0781f7 100644 --- a/tests/unit/test_config_validators.py +++ b/tests/unit/test_config_validators.py @@ -370,27 +370,28 @@ class TestLoginFlowValidation: assert any("token_encryption_key" in err.lower() for err in errors) def test_login_flow_mode_auto_derives_enable_login_flow_flag(self): - """ADR-022 follow-up: setting MCP_DEPLOYMENT_MODE=login_flow auto-derives the flag. + """ADR-022 follow-up: deployment_mode=login_flow auto-derives the flag. - Users no longer need to set ENABLE_LOGIN_FLOW=true (env var was removed); - detect_auth_mode populates settings.enable_login_flow from the resolved mode. + Users no longer need to set ENABLE_LOGIN_FLOW=true (env var was + removed); Settings.__post_init__ populates settings.enable_login_flow + from the resolved mode at construction time, so every Settings + instance carries correct flags regardless of how it was built. """ # Default-fallback case: no auth env vars → LOGIN_FLOW. settings = Settings(nextcloud_host="http://localhost") - assert settings.enable_login_flow is False # default before detection - mode = detect_auth_mode(settings) - assert mode == AuthMode.LOGIN_FLOW assert settings.enable_login_flow is True + assert settings.enable_multi_user_basic_auth is False + assert detect_auth_mode(settings) == AuthMode.LOGIN_FLOW - # Non-LOGIN_FLOW mode should leave the flag False. + # Single-user BasicAuth (credentials set) → neither derived flag. basic_settings = Settings( nextcloud_host="http://localhost", nextcloud_username="alice", nextcloud_password="hunter2", ) - basic_mode = detect_auth_mode(basic_settings) - assert basic_mode == AuthMode.SINGLE_USER_BASIC assert basic_settings.enable_login_flow is False + assert basic_settings.enable_multi_user_basic_auth is False + assert detect_auth_mode(basic_settings) == AuthMode.SINGLE_USER_BASIC def test_vector_sync_auto_enables_background_ops_in_login_flow_mode(self): """Test vector sync automatically enables background operations in Login Flow v2 mode (ADR-021).""" @@ -850,6 +851,8 @@ class TestExplicitModeSelection: The env-var alias was removed; users must migrate to `MCP_DEPLOYMENT_MODE=multi_user_basic`. Silent removal would have switched users to LOGIN_FLOW (the default) — wrong runtime mode. + The check lives in Settings.__post_init__ so it fires at config + load time, not only when detect_auth_mode is reached. """ with patch.dict( os.environ, @@ -862,10 +865,9 @@ class TestExplicitModeSelection: from nextcloud_mcp_server.config import get_settings _reload_config() - settings = get_settings() with pytest.raises(ValueError) as exc: - detect_auth_mode(settings) + get_settings() assert "ENABLE_MULTI_USER_BASIC_AUTH" in str(exc.value) assert "multi_user_basic" in str(exc.value) @@ -874,7 +876,7 @@ class TestExplicitModeSelection: """ADR-022 follow-up: ENABLE_LOGIN_FLOW=true must fail loudly. Mirrors the ENABLE_MULTI_USER_BASIC_AUTH check — both legacy aliases - now error with a one-line migration message. + now error with a one-line migration message at Settings construction. """ with patch.dict( os.environ, @@ -887,10 +889,68 @@ class TestExplicitModeSelection: from nextcloud_mcp_server.config import get_settings _reload_config() - settings = get_settings() with pytest.raises(ValueError) as exc: - detect_auth_mode(settings) + get_settings() assert "ENABLE_LOGIN_FLOW" in str(exc.value) assert "login_flow" in str(exc.value) + + def test_legacy_env_var_check_ignores_falsy_strings(self): + """ADR-022 follow-up: a leftover ENABLE_LOGIN_FLOW=false must NOT error. + + Reviewer round 2 found that the legacy check used `os.getenv(legacy)` + which is truthy for the literal string 'false'. A user who had + explicitly set the flag to false (meaning 'I don't want this') would + get a startup ValueError after upgrading, which is wrong. The fix + only fires for explicitly-truthy values. + """ + with patch.dict( + os.environ, + { + "NEXTCLOUD_HOST": "http://localhost:8080", + "ENABLE_LOGIN_FLOW": "false", + "ENABLE_MULTI_USER_BASIC_AUTH": "0", + }, + clear=True, + ): + from nextcloud_mcp_server.config import get_settings + + _reload_config() + settings = get_settings() # Must not raise. + + # Falls through to LOGIN_FLOW (the auto-detect default) since + # no credentials are set. + assert detect_auth_mode(settings) == AuthMode.LOGIN_FLOW + + def test_derived_flags_stable_across_get_settings_calls(self): + """Regression: derived flags must persist across get_settings() calls. + + `get_settings()` builds a fresh Settings on each invocation. Earlier + commits set the derived flags as a side effect of detect_auth_mode, + which meant the *next* get_settings() call started with default + False — breaking per-request handlers in the integration tests + for `mcp-multi-user-basic` and `mcp-login-flow`. The fix moves the + derivation into Settings.__post_init__ so every instance carries + correct flags. + """ + with patch.dict( + os.environ, + { + "NEXTCLOUD_HOST": "http://localhost:8080", + "MCP_DEPLOYMENT_MODE": "multi_user_basic", + }, + clear=True, + ): + from nextcloud_mcp_server.config import get_settings + + _reload_config() + + s1 = get_settings() + s2 = get_settings() + + assert s1 is not s2 # fresh instance each call + assert s1.enable_multi_user_basic_auth is True + assert s2.enable_multi_user_basic_auth is True + assert s1.enable_login_flow is False + assert s2.enable_login_flow is False