From cafd318f3675f2d923a62c7ece1f8d75523ef58d Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 12 May 2026 19:34:13 +0200 Subject: [PATCH 1/9] refactor(config)!: rename OAUTH_SINGLE_AUDIENCE to LOGIN_FLOW, gate on ENABLE_LOGIN_FLOW MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The AuthMode.OAUTH_SINGLE_AUDIENCE enum was a vestige of ADR-021's original design where it co-existed with OAUTH_TOKEN_EXCHANGE. The un-augmented OAuth bearer pass-through it represented relied on Nextcloud-side patches to user_oidc (Bearer token validation on non-OCS endpoints) that were never merged upstream (see docs/authentication.md, docs/login-flow-v2.md). The working path — mcp-login-flow profile — sets ENABLE_LOGIN_FLOW=true on top of this mode so Login Flow v2 acquires per-user Nextcloud app passwords via a browser flow. With OAUTH_TOKEN_EXCHANGE removed in 57303135, the _AUDIENCE suffix in the Python name no longer disambiguates anything, and the enum value diverged from the env-var spelling. ADR-022 (now accepted) called for this rename as step 1 of consolidation. - nextcloud_mcp_server/config_validators.py: rename enum to LOGIN_FLOW with value "login_flow". The mode_map key is now "login_flow"; the MODE_REQUIREMENTS entry requires `enable_login_flow=True`. Added a validation gate so MCP_DEPLOYMENT_MODE=login_flow without ENABLE_LOGIN_FLOW=true errors with a clear message pointing at ADR-022. Default auto-detection fallback returns LOGIN_FLOW. - nextcloud_mcp_server/app.py: renamed three identifier references and switched the "Configuring MCP server for OAuth mode" log line to the uniform `mode.value` shape used by the other modes. - nextcloud_mcp_server/api/management.py: renamed identifier in the /api/v1/status mapping. The user-visible "auth_mode": "oauth" string is preserved — that's a stable Astrolabe contract. - nextcloud_mcp_server/config.py: updated Settings docstring. - tests/unit/test_config_validators.py: renamed class TestOAuthSingleAudienceValidation → TestLoginFlowValidation, individual test methods, env-var strings; added enable_login_flow=True to fixtures expecting success; added a new test (test_login_flow_requires_enable_login_flow_flag) that exercises the validation gate. - tests/unit/test_management_status_endpoint.py: renamed identifier. BREAKING CHANGE: MCP_DEPLOYMENT_MODE=oauth_single_audience is no longer accepted. Set MCP_DEPLOYMENT_MODE=login_flow (and keep ENABLE_LOGIN_FLOW=true) for the same deployment. The un-augmented OAuth path is no longer supported; if you previously ran the broken path, you can either configure Login Flow v2 (recommended) or switch to multi_user_basic / single_user_basic. Dead-code pruning of `oauth_enabled and not enable_login_flow` branches in app.py (lifespan, background sync) is deferred to a separate follow-up PR per the consolidation plan in ADR-022. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/api/management.py | 2 +- nextcloud_mcp_server/app.py | 13 ++-- nextcloud_mcp_server/config.py | 5 +- nextcloud_mcp_server/config_validators.py | 43 +++++++---- tests/unit/test_config_validators.py | 75 ++++++++++++------- tests/unit/test_management_status_endpoint.py | 2 +- 6 files changed, 90 insertions(+), 50 deletions(-) diff --git a/nextcloud_mcp_server/api/management.py b/nextcloud_mcp_server/api/management.py index 0f10d6b9..b621cec2 100644 --- a/nextcloud_mcp_server/api/management.py +++ b/nextcloud_mcp_server/api/management.py @@ -212,7 +212,7 @@ async def get_server_status(request: Request) -> JSONResponse: # Map deployment mode to auth_mode for API response # This helps clients (like Astrolabe) determine which auth flow to use - if mode == AuthMode.OAUTH_SINGLE_AUDIENCE: + if mode == AuthMode.LOGIN_FLOW: auth_mode = "oauth" elif mode == AuthMode.MULTI_USER_BASIC: auth_mode = "multi_user_basic" diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index 2fa6eff9..5bdedf86 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -1015,8 +1015,11 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = logger.info(f"✅ Configuration validated successfully for {mode.value} mode") logger.debug(f"Mode details:\n{get_mode_summary(mode)}") - # Derive helper variables for backward compatibility with existing code - oauth_enabled = mode == AuthMode.OAUTH_SINGLE_AUDIENCE + # Derive helper variables for backward compatibility with existing code. + # `oauth_enabled` is True for the LOGIN_FLOW (formerly OAUTH_SINGLE_AUDIENCE) + # multi-user OAuth mode — in this mode the MCP server is an OIDC relying + # party and Login Flow v2 acquires per-user Nextcloud app passwords. + oauth_enabled = mode == AuthMode.LOGIN_FLOW # Log hybrid authentication status for multi-user BasicAuth with offline access if mode == AuthMode.MULTI_USER_BASIC and settings.enable_offline_access: logger.info( @@ -1166,8 +1169,8 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = raise # Create MCP server based on detected mode - if mode == AuthMode.OAUTH_SINGLE_AUDIENCE: - logger.info("Configuring MCP server for OAuth mode") + if mode == AuthMode.LOGIN_FLOW: + logger.info("Configuring MCP server for %s mode", mode.value) # Asynchronously get the OAuth configuration ( @@ -1932,7 +1935,7 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = # Check authentication configuration # Report the deployment mode, not just whether OAuth is enabled # This helps clients (like Astrolabe) determine which auth flow to use - if mode == AuthMode.OAUTH_SINGLE_AUDIENCE: + if mode == AuthMode.LOGIN_FLOW: checks["auth_mode"] = "oauth" checks["auth_configured"] = "ok" elif mode == AuthMode.MULTI_USER_BASIC: diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 3a205110..dd806385 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -408,9 +408,10 @@ def get_document_processor_config() -> dict[str, Any]: class Settings: """Application settings from environment variables.""" - # Deployment mode (ADR-021: explicit mode selection) + # Deployment mode (ADR-021: explicit mode selection; updated by ADR-022) # Optional: If not set, mode is auto-detected from other settings - # Valid values: single_user_basic, multi_user_basic, oauth_single_audience + # Valid values: single_user_basic, multi_user_basic, login_flow + # (ADR-022: `oauth_single_audience` was renamed to `login_flow`.) deployment_mode: str | None = None # OAuth/OIDC settings diff --git a/nextcloud_mcp_server/config_validators.py b/nextcloud_mcp_server/config_validators.py index d2e4ff8a..1d497817 100644 --- a/nextcloud_mcp_server/config_validators.py +++ b/nextcloud_mcp_server/config_validators.py @@ -25,7 +25,7 @@ class AuthMode(Enum): SINGLE_USER_BASIC = "single_user_basic" MULTI_USER_BASIC = "multi_user_basic" - OAUTH_SINGLE_AUDIENCE = "oauth_single" + LOGIN_FLOW = "login_flow" @dataclass @@ -113,8 +113,8 @@ MODE_REQUIREMENTS: dict[AuthMode, ModeRequirements] = { "Users provide credentials in request headers. " "Optional background sync using app passwords stored via Astrolabe.", ), - AuthMode.OAUTH_SINGLE_AUDIENCE: ModeRequirements( - required=["nextcloud_host"], + AuthMode.LOGIN_FLOW: ModeRequirements( + required=["nextcloud_host", "enable_login_flow"], optional=[ # OAuth credentials (uses DCR if not provided) "oidc_client_id", @@ -149,9 +149,13 @@ MODE_REQUIREMENTS: dict[AuthMode, ModeRequirements] = { # enables background operations in multi-user modes. No explicit # enable_offline_access setting required. }, - description="OAuth multi-user deployment with single-audience tokens. " - "Tokens work for both MCP server and Nextcloud APIs (pass-through). " - "Uses Dynamic Client Registration if credentials not provided.", + description="OAuth multi-user deployment using Login Flow v2 to acquire " + "per-user Nextcloud app passwords via a browser flow. The MCP server " + "is an OIDC relying party of a configurable IdP (Nextcloud's built-in " + "OIDC by default; Keycloak, AWS Cognito, etc. via OIDC_DISCOVERY_URL). " + "Uses Dynamic Client Registration if credentials not provided. " + "Replaces the deprecated direct OAuth bearer-token pass-through which " + "required unmerged user_oidc patches (see ADR-022).", ), } @@ -159,11 +163,11 @@ MODE_REQUIREMENTS: dict[AuthMode, ModeRequirements] = { def detect_auth_mode(settings: Settings) -> AuthMode: """Detect authentication mode from configuration. - Mode detection priority (ADR-021): + 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 - 3. OAuth single-audience (default OAuth mode) + 3. Login Flow v2 (default — was OAuth single-audience pre-ADR-022) Args: settings: Application settings @@ -185,7 +189,7 @@ def detect_auth_mode(settings: Settings) -> AuthMode: mode_map = { "single_user_basic": AuthMode.SINGLE_USER_BASIC, "multi_user_basic": AuthMode.MULTI_USER_BASIC, - "oauth_single_audience": AuthMode.OAUTH_SINGLE_AUDIENCE, + "login_flow": AuthMode.LOGIN_FLOW, } if mode_str not in mode_map: @@ -208,9 +212,11 @@ def detect_auth_mode(settings: Settings) -> AuthMode: if settings.nextcloud_username and settings.nextcloud_password: return AuthMode.SINGLE_USER_BASIC - # Default: OAuth single-audience mode - # This is the safest multi-user mode (no credential storage) - return AuthMode.OAUTH_SINGLE_AUDIENCE + # 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); selecting LOGIN_FLOW without + # ENABLE_LOGIN_FLOW=true fails validation below. + return AuthMode.LOGIN_FLOW def validate_configuration(settings: Settings) -> tuple[AuthMode, list[str]]: @@ -301,7 +307,18 @@ def validate_configuration(settings: Settings) -> tuple[AuthMode, list[str]]: f"{settings.nextcloud_host}" ) - if mode == AuthMode.OAUTH_SINGLE_AUDIENCE: + if mode == AuthMode.LOGIN_FLOW: + # ADR-022: LOGIN_FLOW requires the Login Flow v2 layer. The un-augmented + # OAuth bearer pass-through (formerly OAUTH_SINGLE_AUDIENCE without + # ENABLE_LOGIN_FLOW) needed unmerged Nextcloud user_oidc patches and + # is no longer supported. + if not settings.enable_login_flow: + errors.append( + f"[{mode.value}] ENABLE_LOGIN_FLOW=true is required for " + "login_flow mode. The un-augmented OAuth path is no longer " + "supported — see ADR-022." + ) + # If OAuth credentials not provided, DCR must be available # (This is a runtime check, not a config check, so we just warn) if not settings.oidc_client_id or not settings.oidc_client_secret: diff --git a/tests/unit/test_config_validators.py b/tests/unit/test_config_validators.py index 3aad9eaa..5f633754 100644 --- a/tests/unit/test_config_validators.py +++ b/tests/unit/test_config_validators.py @@ -43,14 +43,14 @@ class TestModeDetection: mode = detect_auth_mode(settings) assert mode == AuthMode.SINGLE_USER_BASIC - def test_oauth_single_audience_default(self): - """Test OAuth single-audience is default mode.""" + def test_login_flow_default(self): + """Test Login Flow v2 is the default multi-user mode.""" settings = Settings( nextcloud_host="http://localhost", ) mode = detect_auth_mode(settings) - assert mode == AuthMode.OAUTH_SINGLE_AUDIENCE + assert mode == AuthMode.LOGIN_FLOW class TestSingleUserBasicValidation: @@ -108,7 +108,7 @@ class TestSingleUserBasicValidation: # Mode detection requires BOTH username AND password for single-user BasicAuth # If only one is present, it defaults to OAuth single-audience - assert mode == AuthMode.OAUTH_SINGLE_AUDIENCE + assert mode == AuthMode.LOGIN_FLOW # In OAuth mode, having a password set is forbidden assert any("nextcloud_password" in err.lower() for err in errors) @@ -123,7 +123,7 @@ class TestSingleUserBasicValidation: # Mode detection requires BOTH username AND password for single-user BasicAuth # If only one is present, it defaults to OAuth single-audience - assert mode == AuthMode.OAUTH_SINGLE_AUDIENCE + assert mode == AuthMode.LOGIN_FLOW # In OAuth mode, having a username set is forbidden assert any("nextcloud_username" in err.lower() for err in errors) @@ -285,37 +285,40 @@ class TestMultiUserBasicValidation: assert settings.enable_offline_access is True -class TestOAuthSingleAudienceValidation: - """Test validation for OAuth single-audience mode.""" +class TestLoginFlowValidation: + """Test validation for Login Flow v2 mode (formerly OAUTH_SINGLE_AUDIENCE).""" def test_valid_minimal_config(self): - """Test valid minimal OAuth single-audience config.""" + """Test valid minimal Login Flow v2 config.""" settings = Settings( nextcloud_host="http://localhost", + enable_login_flow=True, ) mode, errors = validate_configuration(settings) - assert mode == AuthMode.OAUTH_SINGLE_AUDIENCE + assert mode == AuthMode.LOGIN_FLOW assert len(errors) == 0 def test_valid_with_static_credentials(self): """Test valid config with static OAuth credentials.""" settings = Settings( nextcloud_host="http://localhost", + enable_login_flow=True, oidc_client_id="test-client", oidc_client_secret="test-secret", ) mode, errors = validate_configuration(settings) - assert mode == AuthMode.OAUTH_SINGLE_AUDIENCE + assert mode == AuthMode.LOGIN_FLOW assert len(errors) == 0 def test_valid_with_offline_access(self): """Test valid config with offline access.""" settings = Settings( nextcloud_host="http://localhost", + enable_login_flow=True, oidc_client_id="test-client", oidc_client_secret="test-secret", enable_offline_access=True, @@ -325,7 +328,7 @@ class TestOAuthSingleAudienceValidation: mode, errors = validate_configuration(settings) - assert mode == AuthMode.OAUTH_SINGLE_AUDIENCE + assert mode == AuthMode.LOGIN_FLOW assert len(errors) == 0 def test_forbidden_username_password(self): @@ -345,29 +348,45 @@ class TestOAuthSingleAudienceValidation: """Test error when offline access enabled but encryption key missing.""" settings = Settings( nextcloud_host="http://localhost", + enable_login_flow=True, enable_offline_access=True, token_storage_db="/tmp/tokens.db", ) mode, errors = validate_configuration(settings) - assert mode == AuthMode.OAUTH_SINGLE_AUDIENCE + assert mode == AuthMode.LOGIN_FLOW assert any("token_encryption_key" in err.lower() for err in errors) - def test_vector_sync_auto_enables_background_ops_in_oauth_mode(self): - """Test vector sync automatically enables background operations in OAuth mode (ADR-021).""" + def test_login_flow_requires_enable_login_flow_flag(self): + """ADR-022: LOGIN_FLOW mode must error when ENABLE_LOGIN_FLOW is not true.""" + settings = Settings( + nextcloud_host="http://localhost", + # enable_login_flow deliberately omitted (defaults to False) + ) + + mode, errors = validate_configuration(settings) + + assert mode == AuthMode.LOGIN_FLOW + assert any("ENABLE_LOGIN_FLOW" in err for err in errors), ( + f"Expected ENABLE_LOGIN_FLOW gate error, got: {errors}" + ) + + 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).""" # Before ADR-021: This would have failed validation (required explicit ENABLE_OFFLINE_ACCESS) # After ADR-021: vector_sync_enabled auto-enables background operations in multi-user modes with patch.dict( os.environ, { "NEXTCLOUD_HOST": "http://localhost:8080", + "ENABLE_LOGIN_FLOW": "true", "VECTOR_SYNC_ENABLED": "true", "QDRANT_LOCATION": ":memory:", "OLLAMA_BASE_URL": "http://ollama:11434", "TOKEN_ENCRYPTION_KEY": "test-key", "TOKEN_STORAGE_DB": "/tmp/test.db", - # Note: No username/password = OAuth mode + # Note: No username/password = Login Flow v2 multi-user OAuth mode }, clear=True, ): @@ -377,7 +396,7 @@ class TestOAuthSingleAudienceValidation: settings = get_settings() mode, errors = validate_configuration(settings) - assert mode == AuthMode.OAUTH_SINGLE_AUDIENCE + assert mode == AuthMode.LOGIN_FLOW # Should have no errors - background operations auto-enabled assert len(errors) == 0 # Verify background operations were auto-enabled @@ -515,8 +534,8 @@ class TestConfigurationConsolidation: settings = get_settings() assert settings.enable_offline_access is True - def test_semantic_search_auto_enables_background_ops_in_oauth_mode(self): - """Test ENABLE_SEMANTIC_SEARCH automatically enables background operations in OAuth mode.""" + def test_semantic_search_auto_enables_background_ops_in_login_flow_mode(self): + """Test ENABLE_SEMANTIC_SEARCH automatically enables background operations in Login Flow v2 mode.""" with patch.dict( os.environ, { @@ -707,13 +726,13 @@ class TestExplicitModeSelection: assert mode == AuthMode.MULTI_USER_BASIC - def test_explicit_oauth_single_audience_mode(self): - """Test explicit oauth_single_audience mode selection.""" + def test_explicit_login_flow_mode(self): + """Test explicit login_flow mode selection.""" with patch.dict( os.environ, { "NEXTCLOUD_HOST": "http://localhost:8080", - "MCP_DEPLOYMENT_MODE": "oauth_single_audience", + "MCP_DEPLOYMENT_MODE": "login_flow", }, clear=True, ): @@ -723,7 +742,7 @@ class TestExplicitModeSelection: settings = get_settings() mode = detect_auth_mode(settings) - assert mode == AuthMode.OAUTH_SINGLE_AUDIENCE + assert mode == AuthMode.LOGIN_FLOW def test_invalid_deployment_mode_raises_error(self): """Test invalid MCP_DEPLOYMENT_MODE raises ValueError.""" @@ -757,7 +776,7 @@ class TestExplicitModeSelection: "NEXTCLOUD_HOST": "http://localhost:8080", "NEXTCLOUD_USERNAME": "admin", # Would auto-detect as single_user_basic "NEXTCLOUD_PASSWORD": "password", - "MCP_DEPLOYMENT_MODE": "oauth_single_audience", # Explicit override + "MCP_DEPLOYMENT_MODE": "login_flow", # Explicit override }, clear=True, ): @@ -768,7 +787,7 @@ class TestExplicitModeSelection: mode = detect_auth_mode(settings) # Should use explicit mode, not auto-detected mode - assert mode == AuthMode.OAUTH_SINGLE_AUDIENCE + assert mode == AuthMode.LOGIN_FLOW def test_case_insensitive_mode_names(self): """Test MCP_DEPLOYMENT_MODE is case-insensitive.""" @@ -776,7 +795,7 @@ class TestExplicitModeSelection: os.environ, { "NEXTCLOUD_HOST": "http://localhost:8080", - "MCP_DEPLOYMENT_MODE": "OAUTH_SINGLE_AUDIENCE", # Uppercase + "MCP_DEPLOYMENT_MODE": "LOGIN_FLOW", # Uppercase }, clear=True, ): @@ -786,7 +805,7 @@ class TestExplicitModeSelection: settings = get_settings() mode = detect_auth_mode(settings) - assert mode == AuthMode.OAUTH_SINGLE_AUDIENCE + assert mode == AuthMode.LOGIN_FLOW def test_whitespace_in_mode_name_stripped(self): """Test whitespace in MCP_DEPLOYMENT_MODE is stripped.""" @@ -794,7 +813,7 @@ class TestExplicitModeSelection: os.environ, { "NEXTCLOUD_HOST": "http://localhost:8080", - "MCP_DEPLOYMENT_MODE": " oauth_single_audience ", # Whitespace + "MCP_DEPLOYMENT_MODE": " login_flow ", # Whitespace }, clear=True, ): @@ -804,4 +823,4 @@ class TestExplicitModeSelection: settings = get_settings() mode = detect_auth_mode(settings) - assert mode == AuthMode.OAUTH_SINGLE_AUDIENCE + assert mode == AuthMode.LOGIN_FLOW diff --git a/tests/unit/test_management_status_endpoint.py b/tests/unit/test_management_status_endpoint.py index f6ea5f40..313a12a7 100644 --- a/tests/unit/test_management_status_endpoint.py +++ b/tests/unit/test_management_status_endpoint.py @@ -173,7 +173,7 @@ class TestStatusEndpointOidcConfig: ), patch( "nextcloud_mcp_server.api.management.detect_auth_mode", - return_value=AuthMode.OAUTH_SINGLE_AUDIENCE, + return_value=AuthMode.LOGIN_FLOW, ), ): app = create_test_app() From c74ef014eec3080996a166e27e53ecba635ec719 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 12 May 2026 19:34:29 +0200 Subject: [PATCH 2/9] docs(adr-022): mark Accepted, update env/compose/migration docs for LOGIN_FLOW rename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the LOGIN_FLOW rename. The user-facing surface area — env.sample, docker-compose.yml mcp-login-flow profile, migration guide, ADR statuses, and the running.md boot-log examples — all need to refer to `login_flow` rather than the deprecated `oauth_single_audience` string. - docker-compose.yml: add explicit MCP_DEPLOYMENT_MODE=login_flow to the mcp-login-flow profile (no longer relying on auto-detection). - env.sample: update the deployment-mode list and example, dropping the removed `oauth_token_exchange` and pointing at ADR-022 for the rename rationale. - docs/ADR-022: flip Status to Accepted with a note that this PR implements step 1 (rename + validation gate). - docs/ADR-021: note that it has been partly superseded by ADR-022 (the oauth_single_audience naming is no longer accurate); cross-link. - docs/ADR-025: drop oauth_single_audience/keycloak from the dynaconf validator example and the [oauth_single_audience] TOML section. - docs/configuration-migration-v2.md: bulk-replace oauth_single_audience → login_flow throughout (sed -i). - docs/running.md: re-collapse the per-mode boot-log subsections (added during the closed PR #786 workaround) back into a uniform ""-substitution block — now correct after this PR's logging cleanup at app.py:1172. No code changes in this commit. Co-Authored-By: Claude Opus 4.7 (1M context) --- docker-compose.yml | 4 +++- docs/ADR-021-configuration-consolidation.md | 4 ++-- docs/ADR-022-deployment-mode-consolidation.md | 4 ++-- ...R-025-dynaconf-configuration-management.md | 8 ++++---- docs/configuration-migration-v2.md | 20 +++++++++---------- docs/running.md | 16 +++------------ env.sample | 10 +++++++--- 7 files changed, 31 insertions(+), 35 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index 46984db5..eb5d282e 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -275,7 +275,9 @@ services: #- NEXTCLOUD_MCP_SERVER_URL=https://nextcloud-mcp-dev.tail148d5.ts.net - NEXTCLOUD_PUBLIC_ISSUER_URL=http://localhost:8080 - # Login Flow v2 (ADR-022) + # Login Flow v2 (ADR-022) — explicit mode + the flag that enables the + # browser-based app-password acquisition layer + - MCP_DEPLOYMENT_MODE=login_flow - ENABLE_LOGIN_FLOW=true # Token storage (required for app password + session persistence). diff --git a/docs/ADR-021-configuration-consolidation.md b/docs/ADR-021-configuration-consolidation.md index 065ff936..2b47a724 100644 --- a/docs/ADR-021-configuration-consolidation.md +++ b/docs/ADR-021-configuration-consolidation.md @@ -1,9 +1,9 @@ # ADR-021: Configuration Consolidation and Simplification -**Status:** Accepted +**Status:** Accepted — partly superseded by ADR-022 (`oauth_single_audience` renamed to `login_flow`; `oauth_token_exchange` removed) **Date:** 2025-12-21 **Deciders:** Development Team -**Related:** ADR-020 (Deployment Modes), ADR-002 (Vector Sync), ADR-004 (Progressive Consent) +**Related:** ADR-020 (Deployment Modes), ADR-002 (Vector Sync), ADR-004 (Progressive Consent), ADR-022 (Deployment Mode Consolidation) ## Context diff --git a/docs/ADR-022-deployment-mode-consolidation.md b/docs/ADR-022-deployment-mode-consolidation.md index 9386fad6..6b958119 100644 --- a/docs/ADR-022-deployment-mode-consolidation.md +++ b/docs/ADR-022-deployment-mode-consolidation.md @@ -1,7 +1,7 @@ # ADR-022: Deployment Mode Consolidation via Login Flow v2 -**Status:** Proposed -**Date:** 2026-02-01 +**Status:** Accepted (step 1 — `OAUTH_SINGLE_AUDIENCE` → `LOGIN_FLOW` rename + validation gate. Dead-code pruning is a follow-up.) +**Date:** 2026-02-01 (accepted 2026-05-12) **Deciders:** Development Team **Related:** ADR-020 (Deployment Modes), ADR-021 (Configuration Consolidation), ADR-004 (Progressive Consent), Issue #521 diff --git a/docs/ADR-025-dynaconf-configuration-management.md b/docs/ADR-025-dynaconf-configuration-management.md index fd95e419..2dca9ca3 100644 --- a/docs/ADR-025-dynaconf-configuration-management.md +++ b/docs/ADR-025-dynaconf-configuration-management.md @@ -108,8 +108,9 @@ nextcloud_verify_ssl = true nextcloud_ca_bundle = "@none" # === Deployment Mode === -# Auto-detected if not set. Valid: single_user_basic, multi_user_basic, -# oauth_single_audience, login_flow, keycloak +# Auto-detected if not set. Valid: single_user_basic, multi_user_basic, login_flow +# (`oauth_single_audience` was renamed to `login_flow` in ADR-022; `keycloak` +# is a planned future mode.) # mcp_deployment_mode = "" # === Authentication Toggles === @@ -209,7 +210,7 @@ enable_token_exchange = true token_storage_db = "/app/data/tokens.db" token_exchange_cache_ttl = 300 -[oauth_single_audience] +[login_flow] token_storage_db = "/app/data/tokens.db" ``` @@ -281,7 +282,6 @@ validators = [ # Deployment mode validation — catch typos at startup Validator("MCP_DEPLOYMENT_MODE", is_in=[ "single_user_basic", "multi_user_basic", "login_flow", - "keycloak", "oauth_single_audience", ], when=Validator("MCP_DEPLOYMENT_MODE", must_exist=True)), # Type and range validation diff --git a/docs/configuration-migration-v2.md b/docs/configuration-migration-v2.md index 101c3bbf..9fa6020f 100644 --- a/docs/configuration-migration-v2.md +++ b/docs/configuration-migration-v2.md @@ -106,7 +106,7 @@ NEXTCLOUD_USERNAME= NEXTCLOUD_PASSWORD= # Optional: Explicit mode declaration -MCP_DEPLOYMENT_MODE=oauth_single_audience +MCP_DEPLOYMENT_MODE=login_flow # One variable does it all! ENABLE_SEMANTIC_SEARCH=true # Automatically enables background operations @@ -131,7 +131,7 @@ NEXTCLOUD_OIDC_CLIENT_SECRET=secret **Migration Steps:** 1. Replace `VECTOR_SYNC_ENABLED=true` with `ENABLE_SEMANTIC_SEARCH=true` 2. Remove `ENABLE_OFFLINE_ACCESS=true` (auto-enabled) -3. Optionally add `MCP_DEPLOYMENT_MODE=oauth_single_audience` +3. Optionally add `MCP_DEPLOYMENT_MODE=login_flow` 4. Restart server 5. Check logs for confirmation: "Automatically enabled background operations for semantic search" @@ -161,7 +161,7 @@ NEXTCLOUD_USERNAME= NEXTCLOUD_PASSWORD= # Optional: Explicit mode declaration -MCP_DEPLOYMENT_MODE=oauth_single_audience +MCP_DEPLOYMENT_MODE=login_flow # Renamed for clarity ENABLE_BACKGROUND_OPERATIONS=true # Previously ENABLE_OFFLINE_ACCESS @@ -178,7 +178,7 @@ NEXTCLOUD_OIDC_CLIENT_SECRET=secret **Migration Steps:** 1. Replace `ENABLE_OFFLINE_ACCESS=true` with `ENABLE_BACKGROUND_OPERATIONS=true` -2. Optionally add `MCP_DEPLOYMENT_MODE=oauth_single_audience` +2. Optionally add `MCP_DEPLOYMENT_MODE=login_flow` 3. Restart server --- @@ -321,7 +321,7 @@ Only needed when you want background operations **without** semantic search: ```bash # Example: OAuth mode with background operations but NO semantic search NEXTCLOUD_HOST=https://nextcloud.example.com -MCP_DEPLOYMENT_MODE=oauth_single_audience +MCP_DEPLOYMENT_MODE=login_flow # Explicitly enable background operations for future features ENABLE_BACKGROUND_OPERATIONS=true @@ -352,7 +352,7 @@ NEXTCLOUD_HOST=https://nextcloud.example.com # Is this OAuth or Multi-User BasicAuth? Not immediately clear. # With explicit mode: -MCP_DEPLOYMENT_MODE=oauth_single_audience +MCP_DEPLOYMENT_MODE=login_flow NEXTCLOUD_HOST=https://nextcloud.example.com # Clear: This is OAuth mode ``` @@ -363,7 +363,7 @@ NEXTCLOUD_HOST=https://nextcloud.example.com |-----------|-------------| | `single_user_basic` | Single-user with username/password | | `multi_user_basic` | Multi-user with BasicAuth pass-through | -| `oauth_single_audience` | Multi-user OAuth (recommended) | +| `login_flow` | Multi-user OAuth (recommended) | | `oauth_token_exchange` | Multi-user OAuth with token exchange | ### Mode Detection Priority @@ -430,7 +430,7 @@ WARNING: Both ENABLE_SEMANTIC_SEARCH and VECTOR_SYNC_ENABLED are set. Using ENAB **Symptom:** ``` -Error: [oauth_single_audience] TOKEN_ENCRYPTION_KEY is required when ENABLE_SEMANTIC_SEARCH is enabled +Error: [login_flow] TOKEN_ENCRYPTION_KEY is required when ENABLE_SEMANTIC_SEARCH is enabled ``` **Solution:** @@ -442,7 +442,7 @@ When semantic search is enabled in multi-user modes, you need: ### Issue: Unexpected Mode Detected **Symptom:** -Server activates `oauth_single_audience` mode when you expected `multi_user_basic` +Server activates `login_flow` mode when you expected `multi_user_basic` **Solution:** Add explicit mode declaration: @@ -483,7 +483,7 @@ docker-compose up mcp **Expected Log Output (Multi-User OAuth + Semantic Search):** ``` -INFO: Using explicit deployment mode: oauth_single_audience +INFO: Using explicit deployment mode: login_flow INFO: Automatically enabled background operations for semantic search in multi-user mode. INFO: Vector sync enabled. Starting background scanner... ``` diff --git a/docs/running.md b/docs/running.md index b71ad6a7..0509447a 100644 --- a/docs/running.md +++ b/docs/running.md @@ -336,29 +336,19 @@ messages in the container logs: **At server boot (all modes):** ``` INFO ✅ Configuration validated successfully for mode +INFO Configuring MCP server for mode INFO Health check endpoints enabled: /health/live, /health/ready ``` -`` is one of `single_user_basic`, `multi_user_basic`, or -`oauth_single`, matching the `MCP_DEPLOYMENT_MODE` setting. - -**Additional BasicAuth-mode messages (at server boot):** -``` -INFO Configuring MCP server for mode -``` - -Here `` is the enum value (`single_user_basic` or `multi_user_basic`). +`` is one of `single_user_basic`, `multi_user_basic`, or `login_flow`, +matching the `MCP_DEPLOYMENT_MODE` setting. **Additional OAuth-mode messages (at server boot):** ``` -INFO Configuring MCP server for OAuth mode INFO OAuth client ready: ... INFO OAuth configuration complete ``` -Note the OAuth boot line logs the literal string `OAuth mode`, not the enum -value `oauth_single`. - **Additional single-user BasicAuth messages (per MCP session):** These fire when the first MCP client connects, not at server boot — if you diff --git a/env.sample b/env.sample index bae9d96e..8e175bde 100644 --- a/env.sample +++ b/env.sample @@ -3,11 +3,15 @@ # ============================================ # Optional: Explicitly declare deployment mode (ADR-021) # If not set, mode is auto-detected from other settings -# Valid values: single_user_basic, multi_user_basic, oauth_single_audience, -# oauth_token_exchange +# Valid values: single_user_basic, multi_user_basic, login_flow +# +# Note: `login_flow` is the renamed successor of the former +# `oauth_single_audience` mode (see ADR-022). The un-augmented OAuth path +# required unmerged Nextcloud user_oidc patches and is no longer supported; +# `login_flow` mode now requires ENABLE_LOGIN_FLOW=true. # # Recommendation: Set this for clarity and to catch configuration errors early -#MCP_DEPLOYMENT_MODE=oauth_single_audience +#MCP_DEPLOYMENT_MODE=login_flow # ============================================ # COMMON SETTINGS (Required for all modes) From df4994e8605feca0116c54ca9709c0a8b51cd27f Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 12 May 2026 19:45:50 +0200 Subject: [PATCH 3/9] refactor(config)!: derive enable_login_flow from mode, remove ENABLE_LOGIN_FLOW env var MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Once OAUTH_SINGLE_AUDIENCE was renamed to LOGIN_FLOW and the validation gate ensured the only meaningful configuration was `MCP_DEPLOYMENT_MODE=login_flow + ENABLE_LOGIN_FLOW=true`, the two controls became redundant. Setting the mode is sufficient; the ENABLE_LOGIN_FLOW env var doesn't add information. This commit makes the deployment mode the single source of truth for the Login Flow v2 toggle: - `nextcloud_mcp_server/config.py`: drop the `ENABLE_LOGIN_FLOW` dynaconf env-var alias. The `enable_login_flow` field stays as an internal attribute so the 6 runtime call sites (app.py x4, context.py, auth/scope_authorization.py) keep working unchanged. Updated field docstring to flag it as derived. - `nextcloud_mcp_server/config_validators.py`: - Drop `enable_login_flow` from `MODE_REQUIREMENTS[LOGIN_FLOW].required`. - Drop the validation gate that required ENABLE_LOGIN_FLOW=true for LOGIN_FLOW mode (no longer possible to misconfigure — the flag is derived, not user input). - Add `_sync_derived_flags()` helper called at every return path of `detect_auth_mode` to set `settings.enable_login_flow` from the resolved mode. - `tests/unit/test_config_validators.py`: drop `enable_login_flow=True` from happy-path fixtures (no longer needed — detection sets it). Repurpose `test_login_flow_requires_enable_login_flow_flag` into `test_login_flow_mode_auto_derives_enable_login_flow_flag` which asserts the new auto-derivation behaviour for both LOGIN_FLOW and a non-LOGIN_FLOW mode. - `docker-compose.yml`: remove `ENABLE_LOGIN_FLOW=true` from the `mcp-login-flow` and `mcp-keycloak` profiles. - `env.sample`: remove the ENABLE_LOGIN_FLOW reference; the comment on `MCP_DEPLOYMENT_MODE` now notes the derived flag. - `docs/configuration.md`, `docs/authentication.md`, `docs/login-flow-v2.md`, `docs/auth-flows.md`, `docs/troubleshooting.md`, `docs/ADR-025-*.md`: replace ENABLE_LOGIN_FLOW=true examples and references with MCP_DEPLOYMENT_MODE=login_flow. BREAKING CHANGE: `ENABLE_LOGIN_FLOW` is no longer read from the environment. Anyone who relied on `ENABLE_LOGIN_FLOW=true` to activate Login Flow v2 should set `MCP_DEPLOYMENT_MODE=login_flow` instead (or rely on it being the default when no other auth env vars are set). Co-Authored-By: Claude Opus 4.7 (1M context) --- docker-compose.yml | 9 ++--- ...R-025-dynaconf-configuration-management.md | 9 +++-- docs/auth-flows.md | 2 +- docs/authentication.md | 2 +- docs/configuration.md | 7 ++-- docs/login-flow-v2.md | 8 ++-- docs/troubleshooting.md | 2 +- env.sample | 6 ++- nextcloud_mcp_server/config.py | 11 +++-- nextcloud_mcp_server/config_validators.py | 40 +++++++++++++------ tests/unit/test_config_validators.py | 38 ++++++++++-------- 11 files changed, 80 insertions(+), 54 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index eb5d282e..e6e237de 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -241,8 +241,8 @@ services: - ENABLE_TOKEN_EXCHANGE=true - TOKEN_EXCHANGE_CACHE_TTL=300 # Cache exchanged tokens for 5 minutes (default) - # Login Flow v2 (ADR-022) with external IdP - - ENABLE_LOGIN_FLOW=true + # Login Flow v2 (ADR-022) with external IdP — derived from the + # auto-detected LOGIN_FLOW deployment mode; no separate flag needed. - ENABLE_DCR=true # OAuth scopes (optional - uses defaults if not specified) @@ -275,10 +275,9 @@ services: #- NEXTCLOUD_MCP_SERVER_URL=https://nextcloud-mcp-dev.tail148d5.ts.net - NEXTCLOUD_PUBLIC_ISSUER_URL=http://localhost:8080 - # Login Flow v2 (ADR-022) — explicit mode + the flag that enables the - # browser-based app-password acquisition layer + # Login Flow v2 (ADR-022) — the deployment mode is the single switch; + # the browser-based app-password layer is derived automatically. - MCP_DEPLOYMENT_MODE=login_flow - - ENABLE_LOGIN_FLOW=true # Token storage (required for app password + session persistence). # Source the key from .env — see env.sample. To generate a fresh key: diff --git a/docs/ADR-025-dynaconf-configuration-management.md b/docs/ADR-025-dynaconf-configuration-management.md index 2dca9ca3..c4974c29 100644 --- a/docs/ADR-025-dynaconf-configuration-management.md +++ b/docs/ADR-025-dynaconf-configuration-management.md @@ -38,7 +38,7 @@ The nextcloud-mcp-server configuration system has grown to ~80+ environment vari |----------|-------------|---------| | Core Nextcloud | 6 | `NEXTCLOUD_HOST`, `NEXTCLOUD_USERNAME`, `NEXTCLOUD_VERIFY_SSL` | | OAuth/OIDC | 12 | `OIDC_DISCOVERY_URL`, `NEXTCLOUD_OIDC_CLIENT_ID`, `JWKS_URI` | -| Mode Selection | 4 | `MCP_DEPLOYMENT_MODE`, `ENABLE_LOGIN_FLOW`, `ENABLE_TOKEN_EXCHANGE` | +| Mode Selection | 2 | `MCP_DEPLOYMENT_MODE`, `ENABLE_MULTI_USER_BASIC_AUTH` | | Token Storage | 3 | `TOKEN_ENCRYPTION_KEY`, `TOKEN_STORAGE_DB` | | Semantic Search | 6 | `ENABLE_SEMANTIC_SEARCH`, `VECTOR_SYNC_SCAN_INTERVAL` | | Qdrant | 4 | `QDRANT_URL`, `QDRANT_LOCATION`, `QDRANT_API_KEY` | @@ -115,7 +115,8 @@ nextcloud_ca_bundle = "@none" # === Authentication Toggles === enable_multi_user_basic_auth = false -enable_login_flow = false +# `enable_login_flow` is derived from MCP_DEPLOYMENT_MODE=login_flow in +# detect_auth_mode (ADR-022 follow-up) — no separate toggle. enable_token_exchange = false # === Token Storage === @@ -202,7 +203,7 @@ enable_multi_user_basic_auth = true token_storage_db = "/app/data/tokens.db" [login_flow] -enable_login_flow = true +# enable_login_flow is now derived from the mode (ADR-022 follow-up). token_storage_db = "/app/data/tokens.db" [keycloak] @@ -266,7 +267,7 @@ Dynaconf merges configuration in this order (last wins): This means: - **File-based config is optional** — env vars alone still work (they override everything) -- **Mode-specific defaults reduce boilerplate** — `[login_flow]` sets `enable_login_flow=true` and `token_storage_db=/app/data/tokens.db` so deployers don't need to +- **Mode-specific defaults reduce boilerplate** — `[login_flow]` sets `token_storage_db=/app/data/tokens.db` so deployers don't need to - **Secrets are separated** — `.secrets.toml` holds `TOKEN_ENCRYPTION_KEY`, passwords, API keys - **Local dev overrides don't pollute** — `settings.local.toml` is gitignored diff --git a/docs/auth-flows.md b/docs/auth-flows.md index 4ebf4857..1b89372b 100644 --- a/docs/auth-flows.md +++ b/docs/auth-flows.md @@ -231,7 +231,7 @@ TOKEN_STORAGE_DB=/app/data/tokens.db ### Login Flow v2 ```bash NEXTCLOUD_HOST=https://nextcloud.example.com -ENABLE_LOGIN_FLOW=true +MCP_DEPLOYMENT_MODE=login_flow # Required for app-password storage TOKEN_ENCRYPTION_KEY= diff --git a/docs/authentication.md b/docs/authentication.md index bdc826b9..9eb402c8 100644 --- a/docs/authentication.md +++ b/docs/authentication.md @@ -75,7 +75,7 @@ The server detects the active mode from environment variables at startup: |------------------|---------------| | `NEXTCLOUD_USERNAME` + `NEXTCLOUD_PASSWORD` | Single-User (BasicAuth) | | `ENABLE_MULTI_USER_BASIC_AUTH=true` (no creds) | Multi-User (BasicAuth pass-through) | -| `ENABLE_LOGIN_FLOW=true` (no creds) | Multi-User (Login Flow v2) | +| `MCP_DEPLOYMENT_MODE=login_flow` or no auth env vars set | Multi-User (Login Flow v2) | You can also force a mode via CLI flag: diff --git a/docs/configuration.md b/docs/configuration.md index a1879596..064d716a 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -91,7 +91,7 @@ The recommended multi-user mode. MCP clients authenticate to the MCP server via ```dotenv NEXTCLOUD_HOST=https://your.nextcloud.instance.com -ENABLE_LOGIN_FLOW=true +MCP_DEPLOYMENT_MODE=login_flow # App-password storage (required) TOKEN_ENCRYPTION_KEY= @@ -105,7 +105,7 @@ NEXTCLOUD_PUBLIC_ISSUER_URL=https://your.nextcloud.instance.com | Variable | Required | Description | |----------|----------|-------------| | `NEXTCLOUD_HOST` | ✅ Yes | Internal URL of your Nextcloud instance (server-to-server) | -| `ENABLE_LOGIN_FLOW` | ✅ Yes | Set to `true` to enable Login Flow v2 | +| `MCP_DEPLOYMENT_MODE` | ✅ Yes | Set to `login_flow` to select this mode. The Login Flow v2 browser-app-password layer is derived from the mode automatically — no separate flag needed. | | `TOKEN_ENCRYPTION_KEY` | ✅ Yes | Fernet key for app-password encryption — generate with `python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())"` | | `TOKEN_STORAGE_DB` | ✅ Yes | Path to SQLite DB for stored app passwords (use a persistent volume) | | `NEXTCLOUD_MCP_SERVER_URL` | ✅ Yes | Public URL of the MCP server (used as the audience claim and for browser redirects) | @@ -197,8 +197,7 @@ OLLAMA_BASE_URL=http://ollama:11434 **Multi-User Login Flow v2 Mode:** ```dotenv NEXTCLOUD_HOST=https://nextcloud.example.com -MCP_DEPLOYMENT_MODE=login_flow_v2 -ENABLE_LOGIN_FLOW=true +MCP_DEPLOYMENT_MODE=login_flow # Enable semantic search # In multi-user modes, this AUTOMATICALLY enables background operations! diff --git a/docs/login-flow-v2.md b/docs/login-flow-v2.md index 10558169..7d7e74e8 100644 --- a/docs/login-flow-v2.md +++ b/docs/login-flow-v2.md @@ -58,8 +58,8 @@ NEXTCLOUD_HOST=https://your.nextcloud.example.com NEXTCLOUD_OIDC_CLIENT_ID= NEXTCLOUD_OIDC_CLIENT_SECRET= -# Enable Login Flow v2 (per-user Nextcloud app-password provisioning for the data leg) -ENABLE_LOGIN_FLOW=true +# Select Login Flow v2 mode (per-user Nextcloud app-password provisioning for the data leg) +MCP_DEPLOYMENT_MODE=login_flow # App-password storage (required for persistence across restarts) TOKEN_STORAGE_DB=/app/data/tokens.db @@ -172,7 +172,7 @@ mcp-login-flow: - NEXTCLOUD_HOST=http://app:80 - NEXTCLOUD_MCP_SERVER_URL=http://localhost:8004 - NEXTCLOUD_PUBLIC_ISSUER_URL=http://localhost:8080 - - ENABLE_LOGIN_FLOW=true + - MCP_DEPLOYMENT_MODE=login_flow # Dev-only inline value. In production, mount via Docker secret and read # from a *_FILE env var or a secrets-management init step. - TOKEN_ENCRYPTION_KEY= @@ -351,7 +351,7 @@ The user revoked it from **Settings → Security → Devices & Sessions**. Delet ### Multiple worker processes -The provisioning session store is in-memory; `ENABLE_LOGIN_FLOW=true` assumes a single worker. Running with `uvicorn --workers N` will cause provisioning sessions to randomly fail. For higher concurrency, scale horizontally (multiple containers behind a sticky-session load balancer) rather than within a single process. +The provisioning session store is in-memory; `MCP_DEPLOYMENT_MODE=login_flow` assumes a single worker. Running with `uvicorn --workers N` will cause provisioning sessions to randomly fail. For higher concurrency, scale horizontally (multiple containers behind a sticky-session load balancer) rather than within a single process. > **Sticky-session keying:** route on the **user identity** (e.g. the `sub` claim from the OAuth Bearer token) — **not** the raw token value, and **not** source IP. Bearer tokens rotate on refresh, which would silently break token-value affinity if a refresh lands between the request that initiates provisioning and the polling request that completes it. MCP clients may also not maintain stable IPs across those requests. A stable per-user identifier extracted from the `Authorization` header (e.g. `sub`) is the right key. diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index fd8e683a..e99e34d7 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -148,7 +148,7 @@ For multi-user deployment issues — provisioning loops, app-password storage, O ```bash # To Single-User BasicAuth: set NEXTCLOUD_USERNAME and NEXTCLOUD_PASSWORD # To Multi-User BasicAuth pass-through: ENABLE_MULTI_USER_BASIC_AUTH=true (no creds) -# To Login Flow v2: ENABLE_LOGIN_FLOW=true (no creds) +# To Login Flow v2: MCP_DEPLOYMENT_MODE=login_flow (no creds; also the default fallback) ``` Restart the server after changing modes. The active mode is logged at startup; you can also set `MCP_DEPLOYMENT_MODE` explicitly to fail fast if the env vars don't match. diff --git a/env.sample b/env.sample index 8e175bde..17e9ca92 100644 --- a/env.sample +++ b/env.sample @@ -7,8 +7,10 @@ # # Note: `login_flow` is the renamed successor of the former # `oauth_single_audience` mode (see ADR-022). The un-augmented OAuth path -# required unmerged Nextcloud user_oidc patches and is no longer supported; -# `login_flow` mode now requires ENABLE_LOGIN_FLOW=true. +# required unmerged Nextcloud user_oidc patches and is no longer supported. +# Setting MCP_DEPLOYMENT_MODE=login_flow is sufficient — the Login Flow v2 +# browser-based app-password layer is derived from the mode automatically +# (the previous ENABLE_LOGIN_FLOW=true env var has been removed). # # Recommendation: Set this for clarity and to catch configuration errors early #MCP_DEPLOYMENT_MODE=login_flow diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index dd806385..a1310c53 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -458,7 +458,10 @@ class Settings: # and passes them through to Nextcloud APIs (no storage, stateless) enable_multi_user_basic_auth: bool = False - # Login Flow v2 settings (ADR-022) + # Login Flow v2 derived flag (ADR-022). Internal — not user-settable. + # Auto-set by detect_auth_mode() when the resolved deployment mode is + # LOGIN_FLOW. Kept as a field for backward compat with the runtime call + # sites that read it (app.py, context.py, scope_authorization.py). enable_login_flow: bool = False # Token and webhook storage settings @@ -853,8 +856,10 @@ def get_settings() -> Settings: "userinfo_uri": "USERINFO_URI", # Multi-user BasicAuth pass-through mode "enable_multi_user_basic_auth": "ENABLE_MULTI_USER_BASIC_AUTH", - # Login Flow v2 settings (ADR-022) - "enable_login_flow": "ENABLE_LOGIN_FLOW", + # NOTE: `enable_login_flow` used to have an `ENABLE_LOGIN_FLOW` env-var + # alias here, but it was removed in the ADR-022 follow-up — the flag + # is now derived from MCP_DEPLOYMENT_MODE=login_flow and set by + # detect_auth_mode() so users only need to configure the mode. # Token and webhook storage settings "token_encryption_key": "TOKEN_ENCRYPTION_KEY", "token_storage_db": "TOKEN_STORAGE_DB", diff --git a/nextcloud_mcp_server/config_validators.py b/nextcloud_mcp_server/config_validators.py index 1d497817..1b409bf4 100644 --- a/nextcloud_mcp_server/config_validators.py +++ b/nextcloud_mcp_server/config_validators.py @@ -114,7 +114,7 @@ MODE_REQUIREMENTS: dict[AuthMode, ModeRequirements] = { "Optional background sync using app passwords stored via Astrolabe.", ), AuthMode.LOGIN_FLOW: ModeRequirements( - required=["nextcloud_host", "enable_login_flow"], + required=["nextcloud_host"], optional=[ # OAuth credentials (uses DCR if not provided) "oidc_client_id", @@ -201,24 +201,42 @@ 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 (existing behavior) # Check for multi-user BasicAuth if settings.enable_multi_user_basic_auth: + _sync_derived_flags(settings, AuthMode.MULTI_USER_BASIC) return AuthMode.MULTI_USER_BASIC # 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); selecting LOGIN_FLOW without - # ENABLE_LOGIN_FLOW=true fails validation below. + # 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. + + Specifically: `enable_login_flow` is now derived from + `mode == AuthMode.LOGIN_FLOW`. The ENABLE_LOGIN_FLOW env-var alias was + removed in the ADR-022 follow-up (PR #787). + """ + settings.enable_login_flow = mode == AuthMode.LOGIN_FLOW + + def validate_configuration(settings: Settings) -> tuple[AuthMode, list[str]]: """Validate configuration for detected mode. @@ -308,16 +326,12 @@ def validate_configuration(settings: Settings) -> tuple[AuthMode, list[str]]: ) if mode == AuthMode.LOGIN_FLOW: - # ADR-022: LOGIN_FLOW requires the Login Flow v2 layer. The un-augmented - # OAuth bearer pass-through (formerly OAUTH_SINGLE_AUDIENCE without - # ENABLE_LOGIN_FLOW) needed unmerged Nextcloud user_oidc patches and - # is no longer supported. - if not settings.enable_login_flow: - errors.append( - f"[{mode.value}] ENABLE_LOGIN_FLOW=true is required for " - "login_flow mode. The un-augmented OAuth path is no longer " - "supported — see ADR-022." - ) + # ADR-022 follow-up: the un-augmented OAuth bearer pass-through (the + # old OAUTH_SINGLE_AUDIENCE without ENABLE_LOGIN_FLOW) needed unmerged + # Nextcloud user_oidc patches and is no longer supported. The + # `enable_login_flow` flag is now derived from the resolved mode by + # `_sync_derived_flags`, so users only configure the mode — no + # separate ENABLE_LOGIN_FLOW env var is needed. # If OAuth credentials not provided, DCR must be available # (This is a runtime check, not a config check, so we just warn) diff --git a/tests/unit/test_config_validators.py b/tests/unit/test_config_validators.py index 5f633754..3c63b691 100644 --- a/tests/unit/test_config_validators.py +++ b/tests/unit/test_config_validators.py @@ -289,22 +289,22 @@ class TestLoginFlowValidation: """Test validation for Login Flow v2 mode (formerly OAUTH_SINGLE_AUDIENCE).""" def test_valid_minimal_config(self): - """Test valid minimal Login Flow v2 config.""" + """Test valid minimal Login Flow v2 config — enable_login_flow is now derived.""" settings = Settings( nextcloud_host="http://localhost", - enable_login_flow=True, ) mode, errors = validate_configuration(settings) assert mode == AuthMode.LOGIN_FLOW assert len(errors) == 0 + # ADR-022 follow-up: enable_login_flow is derived from the resolved mode. + assert settings.enable_login_flow is True def test_valid_with_static_credentials(self): """Test valid config with static OAuth credentials.""" settings = Settings( nextcloud_host="http://localhost", - enable_login_flow=True, oidc_client_id="test-client", oidc_client_secret="test-secret", ) @@ -318,7 +318,6 @@ class TestLoginFlowValidation: """Test valid config with offline access.""" settings = Settings( nextcloud_host="http://localhost", - enable_login_flow=True, oidc_client_id="test-client", oidc_client_secret="test-secret", enable_offline_access=True, @@ -348,7 +347,6 @@ class TestLoginFlowValidation: """Test error when offline access enabled but encryption key missing.""" settings = Settings( nextcloud_host="http://localhost", - enable_login_flow=True, enable_offline_access=True, token_storage_db="/tmp/tokens.db", ) @@ -358,19 +356,28 @@ class TestLoginFlowValidation: assert mode == AuthMode.LOGIN_FLOW assert any("token_encryption_key" in err.lower() for err in errors) - def test_login_flow_requires_enable_login_flow_flag(self): - """ADR-022: LOGIN_FLOW mode must error when ENABLE_LOGIN_FLOW is not true.""" - settings = Settings( - nextcloud_host="http://localhost", - # enable_login_flow deliberately omitted (defaults to False) - ) - - mode, errors = validate_configuration(settings) + 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. + 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. + """ + # 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 any("ENABLE_LOGIN_FLOW" in err for err in errors), ( - f"Expected ENABLE_LOGIN_FLOW gate error, got: {errors}" + assert settings.enable_login_flow is True + + # Non-LOGIN_FLOW mode should leave the flag False. + 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 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).""" @@ -380,7 +387,6 @@ class TestLoginFlowValidation: os.environ, { "NEXTCLOUD_HOST": "http://localhost:8080", - "ENABLE_LOGIN_FLOW": "true", "VECTOR_SYNC_ENABLED": "true", "QDRANT_LOCATION": ":memory:", "OLLAMA_BASE_URL": "http://ollama:11434", From 282c245da1317266a03535bea21ac52be93f08a7 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 12 May 2026 20:06:16 +0200 Subject: [PATCH 4/9] refactor(config)!: drop ENABLE_MULTI_USER_BASIC_AUTH env var, fail loud on legacy aliases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same pattern as the ENABLE_LOGIN_FLOW removal in the previous commit: the deployment mode (MCP_DEPLOYMENT_MODE) is the single source of truth for selecting an auth flow. The ENABLE_MULTI_USER_BASIC_AUTH env-var alias is redundant with `MCP_DEPLOYMENT_MODE=multi_user_basic`. Unlike the ENABLE_LOGIN_FLOW removal — where silent removal was safe because Login Flow v2 is the auto-detection default — silent removal here would be a surprise: a user with only ENABLE_MULTI_USER_BASIC_AUTH=true in their .env would auto-detect into LOGIN_FLOW after upgrade (wrong runtime mode). Mitigation: detect_auth_mode now reads os.environ directly for both legacy aliases and raises ValueError with a one-line migration message if either is set. Applied retroactively to ENABLE_LOGIN_FLOW as well — loud is better than silent. - nextcloud_mcp_server/config.py: - Drop the dynaconf env-var alias entry for ENABLE_MULTI_USER_BASIC_AUTH. - Update the `enable_multi_user_basic_auth` field docstring to mark it as derived / not user-settable. - `_is_multi_user_mode()` (early-config helper, runs before Settings is built) switched to checking MCP_DEPLOYMENT_MODE directly. Now consistent with the canonical detection in detect_auth_mode. - nextcloud_mcp_server/config_validators.py: - Drop the auto-detection branch (`if settings.enable_multi_user_basic_auth`). Selection of MULTI_USER_BASIC is now exclusively via the explicit MCP_DEPLOYMENT_MODE branch. - Add `enable_multi_user_basic_auth` to `_sync_derived_flags` alongside `enable_login_flow` — both flags are now derived from the resolved mode. - Drop `enable_multi_user_basic_auth` from `MODE_REQUIREMENTS[MULTI_USER_BASIC].required` and from the `forbidden` lists of SINGLE_USER_BASIC and LOGIN_FLOW (no longer user input → no meaningful forbidden check). - Add loud-deprecation `ValueError` block at the top of detect_auth_mode that errors with a clear migration message when ENABLE_MULTI_USER_BASIC_AUTH or ENABLE_LOGIN_FLOW is found in os.environ. - tests/unit/test_config_validators.py: - Switch ~10 fixtures from `enable_multi_user_basic_auth=True` to `deployment_mode="multi_user_basic"` (mirrors `enable_login_flow` treatment from the previous commit). - Switch two `patch.dict(os.environ, {"ENABLE_MULTI_USER_BASIC_AUTH": "true"})` blocks to use MCP_DEPLOYMENT_MODE. - Rename `test_forbidden_multi_user_basic_auth` to `test_forbidden_multi_user_basic_when_credentials_present` — the scenario is now an explicit-mode + credentials conflict, not an env-var-flag conflict. - Add `test_legacy_enable_multi_user_basic_auth_env_var_errors` and `test_legacy_enable_login_flow_env_var_errors` to exercise the new loud-deprecation ValueError path. - docker-compose.yml: mcp-multi-user-basic profile switched to `MCP_DEPLOYMENT_MODE=multi_user_basic`. - env.sample: replaced `#ENABLE_MULTI_USER_BASIC_AUTH=true` example with `#MCP_DEPLOYMENT_MODE=multi_user_basic`. - docs/authentication.md, configuration.md, troubleshooting.md, auth-flows.md, webhook-management-guide.md, configuration-migration-v2.md, ADR-025: replaced env-var examples with the canonical MCP_DEPLOYMENT_MODE form. - docs/ADR-020: marked partly superseded by ADR-022. - CLAUDE.md: Multi-User BasicAuth section updated to set MCP_DEPLOYMENT_MODE. - nextcloud_mcp_server/vector/oauth_sync.py: module docstring updated. BREAKING CHANGE: ENABLE_MULTI_USER_BASIC_AUTH is no longer read from the environment, and setting it now raises a startup ValueError with a migration message. Replace `ENABLE_MULTI_USER_BASIC_AUTH=true` with `MCP_DEPLOYMENT_MODE=multi_user_basic`. The same loud-deprecation check is also applied to the recently-removed ENABLE_LOGIN_FLOW — replace with `MCP_DEPLOYMENT_MODE=login_flow` (or drop both; `login_flow` is the auto-detect default when no other auth env vars are set). Co-Authored-By: Claude Opus 4.7 (1M context) --- CLAUDE.md | 2 +- docker-compose.yml | 4 +- ...ment-modes-and-configuration-validation.md | 4 +- ...R-025-dynaconf-configuration-management.md | 21 ++-- docs/auth-flows.md | 2 +- docs/authentication.md | 4 +- docs/configuration-migration-v2.md | 6 +- docs/configuration.md | 2 +- docs/troubleshooting.md | 2 +- docs/webhook-management-guide.md | 2 +- env.sample | 5 +- nextcloud_mcp_server/config.py | 43 +++++--- nextcloud_mcp_server/config_validators.py | 37 ++++--- nextcloud_mcp_server/vector/oauth_sync.py | 2 +- tests/unit/test_config_validators.py | 98 +++++++++++++++---- 15 files changed, 166 insertions(+), 68 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 96fb337c..332bb36f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -338,7 +338,7 @@ The server supports three deployment modes, controlled by environment variables - Best for: personal instances, local development **2. Multi-User BasicAuth** (profile: `multi-user-basic`) -- Set `ENABLE_MULTI_USER_BASIC_AUTH=true` +- Set `MCP_DEPLOYMENT_MODE=multi_user_basic` - Each MCP client provides credentials via HTTP Authorization header - Per-request client creation from extracted credentials - Best for: internal deployments where users manage their own Nextcloud credentials diff --git a/docker-compose.yml b/docker-compose.yml index e6e237de..041da0ff 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -146,11 +146,11 @@ services: ports: - 127.0.0.1:8003:8000 environment: - # Multi-user BasicAuth pass-through mode (ADR-020) + # Multi-user BasicAuth pass-through mode (ADR-020, ADR-022) - NEXTCLOUD_HOST=http://app:80 - NEXTCLOUD_MCP_SERVER_URL=http://localhost:8003 - NEXTCLOUD_PUBLIC_ISSUER_URL=http://localhost:8080 - - ENABLE_MULTI_USER_BASIC_AUTH=true + - MCP_DEPLOYMENT_MODE=multi_user_basic - ENABLE_BACKGROUND_OPERATIONS=true # Token storage (required for middleware initialization). diff --git a/docs/ADR-020-deployment-modes-and-configuration-validation.md b/docs/ADR-020-deployment-modes-and-configuration-validation.md index f81215c7..861bb823 100644 --- a/docs/ADR-020-deployment-modes-and-configuration-validation.md +++ b/docs/ADR-020-deployment-modes-and-configuration-validation.md @@ -1,9 +1,9 @@ # ADR-020: Deployment Modes and Configuration Validation -**Status:** Accepted +**Status:** Accepted — partly superseded by ADR-022 (`oauth_single_audience` renamed to `login_flow`; the `ENABLE_MULTI_USER_BASIC_AUTH` and `ENABLE_LOGIN_FLOW` env-var aliases were removed in favour of `MCP_DEPLOYMENT_MODE` as the single source of truth) **Date:** 2025-12-20 **Deciders:** Development Team -**Related:** ADR-002 (Vector Sync), ADR-004 (Progressive Consent), ADR-019 (Multi-user BasicAuth) +**Related:** ADR-002 (Vector Sync), ADR-004 (Progressive Consent), ADR-019 (Multi-user BasicAuth), ADR-022 (Deployment Mode Consolidation) ## Context diff --git a/docs/ADR-025-dynaconf-configuration-management.md b/docs/ADR-025-dynaconf-configuration-management.md index c4974c29..b905335f 100644 --- a/docs/ADR-025-dynaconf-configuration-management.md +++ b/docs/ADR-025-dynaconf-configuration-management.md @@ -38,7 +38,7 @@ The nextcloud-mcp-server configuration system has grown to ~80+ environment vari |----------|-------------|---------| | Core Nextcloud | 6 | `NEXTCLOUD_HOST`, `NEXTCLOUD_USERNAME`, `NEXTCLOUD_VERIFY_SSL` | | OAuth/OIDC | 12 | `OIDC_DISCOVERY_URL`, `NEXTCLOUD_OIDC_CLIENT_ID`, `JWKS_URI` | -| Mode Selection | 2 | `MCP_DEPLOYMENT_MODE`, `ENABLE_MULTI_USER_BASIC_AUTH` | +| Mode Selection | 1 | `MCP_DEPLOYMENT_MODE` | | Token Storage | 3 | `TOKEN_ENCRYPTION_KEY`, `TOKEN_STORAGE_DB` | | Semantic Search | 6 | `ENABLE_SEMANTIC_SEARCH`, `VECTOR_SYNC_SCAN_INTERVAL` | | Qdrant | 4 | `QDRANT_URL`, `QDRANT_LOCATION`, `QDRANT_API_KEY` | @@ -114,9 +114,10 @@ nextcloud_ca_bundle = "@none" # mcp_deployment_mode = "" # === Authentication Toggles === -enable_multi_user_basic_auth = false -# `enable_login_flow` is derived from MCP_DEPLOYMENT_MODE=login_flow in -# detect_auth_mode (ADR-022 follow-up) — no separate toggle. +# Both `enable_multi_user_basic_auth` and `enable_login_flow` are derived +# from MCP_DEPLOYMENT_MODE in detect_auth_mode (ADR-022 follow-up) — no +# separate toggles. Only ENABLE_TOKEN_EXCHANGE remains as an independent +# flag (separate cleanup). enable_token_exchange = false # === Token Storage === @@ -199,7 +200,7 @@ nextcloud_mcp_port = 8000 # nextcloud_password = "" (in .secrets.toml) [multi_user_basic] -enable_multi_user_basic_auth = true +# enable_multi_user_basic_auth is now derived from the mode (ADR-022 follow-up). token_storage_db = "/app/data/tokens.db" [login_flow] @@ -344,10 +345,16 @@ In **Phase 4**, this could migrate to a post-hook: # Phase 4 target (not implemented in Phases 1-3) def resolve_dependencies(settings): """Auto-enable background operations for semantic search in multi-user modes.""" + mode = (settings.get("MCP_DEPLOYMENT_MODE", "") or "").lower().strip() is_multi_user = ( - settings.get("ENABLE_MULTI_USER_BASIC_AUTH", False) + mode in {"multi_user_basic", "login_flow"} or settings.get("ENABLE_TOKEN_EXCHANGE", False) - or (not settings.get("NEXTCLOUD_USERNAME") and not settings.get("NEXTCLOUD_PASSWORD")) + or ( + mode != "single_user_basic" + and not ( + settings.get("NEXTCLOUD_USERNAME") and settings.get("NEXTCLOUD_PASSWORD") + ) + ) ) if settings.get("ENABLE_SEMANTIC_SEARCH", False) and is_multi_user: if not settings.get("ENABLE_BACKGROUND_OPERATIONS", False): diff --git a/docs/auth-flows.md b/docs/auth-flows.md index 1b89372b..7997c6f4 100644 --- a/docs/auth-flows.md +++ b/docs/auth-flows.md @@ -221,7 +221,7 @@ NEXTCLOUD_PASSWORD= ### Multi-User BasicAuth ```bash NEXTCLOUD_HOST=https://nextcloud.example.com -ENABLE_MULTI_USER_BASIC_AUTH=true +MCP_DEPLOYMENT_MODE=multi_user_basic # Optional: app-password storage for background sync TOKEN_ENCRYPTION_KEY= diff --git a/docs/authentication.md b/docs/authentication.md index 9eb402c8..25c059a9 100644 --- a/docs/authentication.md +++ b/docs/authentication.md @@ -43,7 +43,7 @@ Each MCP client sends its own credentials in an HTTP `Authorization: Basic` head ```bash NEXTCLOUD_HOST=https://your.nextcloud.example.com -ENABLE_MULTI_USER_BASIC_AUTH=true +MCP_DEPLOYMENT_MODE=multi_user_basic ``` `NEXTCLOUD_USERNAME` and `NEXTCLOUD_PASSWORD` must NOT be set in this mode. @@ -74,7 +74,7 @@ The server detects the active mode from environment variables at startup: | Env vars present | Detected mode | |------------------|---------------| | `NEXTCLOUD_USERNAME` + `NEXTCLOUD_PASSWORD` | Single-User (BasicAuth) | -| `ENABLE_MULTI_USER_BASIC_AUTH=true` (no creds) | Multi-User (BasicAuth pass-through) | +| `MCP_DEPLOYMENT_MODE=multi_user_basic` | Multi-User (BasicAuth pass-through) | | `MCP_DEPLOYMENT_MODE=login_flow` or no auth env vars set | Multi-User (Login Flow v2) | You can also force a mode via CLI flag: diff --git a/docs/configuration-migration-v2.md b/docs/configuration-migration-v2.md index 9fa6020f..8d288b78 100644 --- a/docs/configuration-migration-v2.md +++ b/docs/configuration-migration-v2.md @@ -188,7 +188,7 @@ NEXTCLOUD_OIDC_CLIENT_SECRET=secret **Before (v0.57.x):** ```bash NEXTCLOUD_HOST=https://nextcloud.example.com -ENABLE_MULTI_USER_BASIC_AUTH=true +MCP_DEPLOYMENT_MODE=multi_user_basic # Both required - redundant ENABLE_OFFLINE_ACCESS=true @@ -205,7 +205,7 @@ NEXTCLOUD_OIDC_CLIENT_SECRET=secret **After (v0.58.0+ - Simplified):** ```bash NEXTCLOUD_HOST=https://nextcloud.example.com -ENABLE_MULTI_USER_BASIC_AUTH=true +MCP_DEPLOYMENT_MODE=multi_user_basic # Optional: Explicit mode declaration MCP_DEPLOYMENT_MODE=multi_user_basic @@ -448,7 +448,7 @@ Server activates `login_flow` mode when you expected `multi_user_basic` Add explicit mode declaration: ```bash MCP_DEPLOYMENT_MODE=multi_user_basic -ENABLE_MULTI_USER_BASIC_AUTH=true +MCP_DEPLOYMENT_MODE=multi_user_basic ``` --- diff --git a/docs/configuration.md b/docs/configuration.md index 064d716a..128a768e 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -74,7 +74,7 @@ Each MCP client sends its own Nextcloud credentials in an `Authorization: Basic` ```dotenv NEXTCLOUD_HOST=https://your.nextcloud.instance.com -ENABLE_MULTI_USER_BASIC_AUTH=true +MCP_DEPLOYMENT_MODE=multi_user_basic # Optional: enable per-user app-password storage for background sync TOKEN_ENCRYPTION_KEY= diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index e99e34d7..6fb05915 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -147,7 +147,7 @@ For multi-user deployment issues — provisioning loops, app-password storage, O ```bash # To Single-User BasicAuth: set NEXTCLOUD_USERNAME and NEXTCLOUD_PASSWORD -# To Multi-User BasicAuth pass-through: ENABLE_MULTI_USER_BASIC_AUTH=true (no creds) +# To Multi-User BasicAuth pass-through: MCP_DEPLOYMENT_MODE=multi_user_basic (no creds) # To Login Flow v2: MCP_DEPLOYMENT_MODE=login_flow (no creds; also the default fallback) ``` diff --git a/docs/webhook-management-guide.md b/docs/webhook-management-guide.md index 0d6dc54e..c0e55003 100644 --- a/docs/webhook-management-guide.md +++ b/docs/webhook-management-guide.md @@ -77,7 +77,7 @@ php occ webhook_listeners:remove **Configuration:** ```bash NEXTCLOUD_HOST=http://nextcloud.example.com -ENABLE_MULTI_USER_BASIC_AUTH=true +MCP_DEPLOYMENT_MODE=multi_user_basic ENABLE_BACKGROUND_OPERATIONS=true TOKEN_ENCRYPTION_KEY= TOKEN_STORAGE_DB=/app/data/tokens.db diff --git a/env.sample b/env.sample index 17e9ca92..600c2ffc 100644 --- a/env.sample +++ b/env.sample @@ -49,8 +49,9 @@ NEXTCLOUD_PASSWORD= # Users provide credentials in request headers (pass-through) # Use for: Multi-user without OAuth, simple shared deployments # -# Required: -#ENABLE_MULTI_USER_BASIC_AUTH=true +# Required (sets the deployment mode; the legacy ENABLE_MULTI_USER_BASIC_AUTH +# env var was removed in the ADR-022 follow-up): +#MCP_DEPLOYMENT_MODE=multi_user_basic # # Optional - Background Operations (for semantic search, future features): # Enable background token storage using app passwords (via Astrolabe) diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index a1310c53..c0aeab42 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -453,9 +453,13 @@ class Settings: # Progressive Consent settings (always enabled - no flag needed) enable_offline_access: bool = False - # Multi-user BasicAuth pass-through mode (ADR-019 interim solution) - # When enabled, MCP server extracts BasicAuth credentials from request headers - # and passes them through to Nextcloud APIs (no storage, stateless) + # Multi-user BasicAuth pass-through mode (ADR-019 interim solution). + # Internal — not user-settable; the ENABLE_MULTI_USER_BASIC_AUTH env-var + # alias was removed in the ADR-022 follow-up. Auto-set by + # detect_auth_mode() when MCP_DEPLOYMENT_MODE=multi_user_basic. When True, + # the MCP server extracts BasicAuth credentials from request headers and + # passes them through to Nextcloud APIs (no storage, stateless). Kept + # as a field for backward compat with the runtime call sites that read it. enable_multi_user_basic_auth: bool = False # Login Flow v2 derived flag (ADR-022). Internal — not user-settable. @@ -723,20 +727,31 @@ def _get_semantic_search_enabled() -> bool: def _is_multi_user_mode() -> bool: """Detect if this is a multi-user deployment mode. + Runs early in config setup (before Settings is fully built) for + mode-conditional defaults. Must match the canonical detection in + `config_validators.detect_auth_mode`, but works directly against the + raw dynaconf store since Settings doesn't exist yet. + Multi-user modes are: - - Multi-user BasicAuth (ENABLE_MULTI_USER_BASIC_AUTH=true) - - OAuth Single-Audience (no username/password set) + - Multi-user BasicAuth (MCP_DEPLOYMENT_MODE=multi_user_basic) + - Login Flow v2 / default OAuth (MCP_DEPLOYMENT_MODE=login_flow, or no + username/password and no explicit mode) - OAuth Token Exchange (ENABLE_TOKEN_EXCHANGE=true) - Single-user modes are: + Single-user mode is: - Single-user BasicAuth (username and password both set) Returns: True if multi-user mode detected """ - # Multi-user BasicAuth explicitly enabled - if _dynaconf.get("ENABLE_MULTI_USER_BASIC_AUTH", False): + # Explicit deployment mode wins. The ENABLE_MULTI_USER_BASIC_AUTH env-var + # alias was removed in the ADR-022 follow-up; selection is now via + # MCP_DEPLOYMENT_MODE. + explicit_mode = str(_dynaconf.get("MCP_DEPLOYMENT_MODE", "") or "").lower().strip() + if explicit_mode in {"multi_user_basic", "login_flow"}: return True + if explicit_mode == "single_user_basic": + return False # Token exchange implies OAuth multi-user if _dynaconf.get("ENABLE_TOKEN_EXCHANGE", False): @@ -748,7 +763,7 @@ def _is_multi_user_mode() -> bool: if has_username and has_password: return False - # Otherwise, assume OAuth multi-user (default when no credentials provided) + # Otherwise, assume multi-user (default when no credentials provided) return True @@ -854,12 +869,10 @@ def get_settings() -> Settings: "jwks_uri": "JWKS_URI", "introspection_uri": "INTROSPECTION_URI", "userinfo_uri": "USERINFO_URI", - # Multi-user BasicAuth pass-through mode - "enable_multi_user_basic_auth": "ENABLE_MULTI_USER_BASIC_AUTH", - # NOTE: `enable_login_flow` used to have an `ENABLE_LOGIN_FLOW` env-var - # alias here, but it was removed in the ADR-022 follow-up — the flag - # is now derived from MCP_DEPLOYMENT_MODE=login_flow and set by - # detect_auth_mode() so users only need to configure the mode. + # NOTE: `enable_multi_user_basic_auth` and `enable_login_flow` no + # longer have env-var aliases — both are derived from the resolved + # MCP_DEPLOYMENT_MODE in detect_auth_mode() so users only configure + # the mode (ADR-022 follow-up). # Token and webhook storage settings "token_encryption_key": "TOKEN_ENCRYPTION_KEY", "token_storage_db": "TOKEN_STORAGE_DB", diff --git a/nextcloud_mcp_server/config_validators.py b/nextcloud_mcp_server/config_validators.py index 1b409bf4..325deeb5 100644 --- a/nextcloud_mcp_server/config_validators.py +++ b/nextcloud_mcp_server/config_validators.py @@ -9,6 +9,7 @@ See ADR-020 for detailed architecture and deployment mode documentation. """ import logging +import os from dataclasses import dataclass from enum import Enum @@ -64,7 +65,6 @@ MODE_REQUIREMENTS: dict[AuthMode, ModeRequirements] = { "document_chunk_overlap", ], forbidden=[ - "enable_multi_user_basic_auth", "oidc_client_id", "oidc_client_secret", ], @@ -78,7 +78,7 @@ MODE_REQUIREMENTS: dict[AuthMode, ModeRequirements] = { "Suitable for personal Nextcloud instances and local development.", ), AuthMode.MULTI_USER_BASIC: ModeRequirements( - required=["nextcloud_host", "enable_multi_user_basic_auth"], + required=["nextcloud_host"], optional=[ # Background sync with app passwords (via Astrolabe) "enable_offline_access", @@ -138,7 +138,6 @@ MODE_REQUIREMENTS: dict[AuthMode, ModeRequirements] = { forbidden=[ "nextcloud_username", "nextcloud_password", - "enable_multi_user_basic_auth", ], conditional={ "enable_offline_access": [ @@ -181,6 +180,21 @@ def detect_auth_mode(settings: Settings) -> AuthMode: 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 if settings.deployment_mode: mode_str = settings.deployment_mode.lower().strip() @@ -204,11 +218,11 @@ def detect_auth_mode(settings: Settings) -> AuthMode: _sync_derived_flags(settings, explicit_mode) return explicit_mode - # Auto-detection (existing behavior) - # Check for multi-user BasicAuth - if settings.enable_multi_user_basic_auth: - _sync_derived_flags(settings, AuthMode.MULTI_USER_BASIC) - return AuthMode.MULTI_USER_BASIC + # 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. # Check for single-user BasicAuth (explicit credentials) if settings.nextcloud_username and settings.nextcloud_password: @@ -228,13 +242,12 @@ def _sync_derived_flags(settings: Settings, mode: AuthMode) -> None: 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. - - Specifically: `enable_login_flow` is now derived from - `mode == AuthMode.LOGIN_FLOW`. The ENABLE_LOGIN_FLOW env-var alias was + 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]]: diff --git a/nextcloud_mcp_server/vector/oauth_sync.py b/nextcloud_mcp_server/vector/oauth_sync.py index f29443e9..7883c4db 100644 --- a/nextcloud_mcp_server/vector/oauth_sync.py +++ b/nextcloud_mcp_server/vector/oauth_sync.py @@ -7,7 +7,7 @@ Manages background vector sync for multi-user deployments: Authentication strategies are mutually exclusive by deployment mode: -Multi-user BasicAuth mode (ENABLE_MULTI_USER_BASIC_AUTH=true): +Multi-user BasicAuth mode (MCP_DEPLOYMENT_MODE=multi_user_basic): - Uses app passwords stored locally in MCP server's database - Users provision via Astrolabe personal settings, which sends to MCP API - OAuth is NOT used diff --git a/tests/unit/test_config_validators.py b/tests/unit/test_config_validators.py index 3c63b691..f8d7f907 100644 --- a/tests/unit/test_config_validators.py +++ b/tests/unit/test_config_validators.py @@ -10,6 +10,8 @@ Tests cover: import os from unittest.mock import patch +import pytest + from nextcloud_mcp_server.config import Settings, _reload_config from nextcloud_mcp_server.config_validators import ( AuthMode, @@ -23,14 +25,21 @@ class TestModeDetection: """Test auth mode detection from configuration.""" def test_multi_user_basic_mode_detection(self): - """Test multi-user BasicAuth mode is detected.""" + """Test multi-user BasicAuth mode is selected via explicit deployment_mode. + + ADR-022 follow-up: the ENABLE_MULTI_USER_BASIC_AUTH auto-detection branch + was removed; the only way to opt in is `MCP_DEPLOYMENT_MODE=multi_user_basic`. + Coverage for the explicit-mode path also lives in + TestExplicitModeSelection::test_explicit_multi_user_basic_mode. + """ settings = Settings( nextcloud_host="http://localhost", - enable_multi_user_basic_auth=True, + deployment_mode="multi_user_basic", ) mode = detect_auth_mode(settings) assert mode == AuthMode.MULTI_USER_BASIC + assert settings.enable_multi_user_basic_auth is True def test_single_user_basic_mode_detection(self): """Test single-user BasicAuth mode is detected.""" @@ -127,20 +136,25 @@ class TestSingleUserBasicValidation: # In OAuth mode, having a username set is forbidden assert any("nextcloud_username" in err.lower() for err in errors) - def test_forbidden_multi_user_basic_auth(self): - """Test error when ENABLE_MULTI_USER_BASIC_AUTH is set.""" + def test_forbidden_multi_user_basic_when_credentials_present(self): + """Test multi-user mode rejects single-user credentials. + + When MCP_DEPLOYMENT_MODE=multi_user_basic is set explicitly but + NEXTCLOUD_USERNAME/PASSWORD are also set (a misconfiguration), + the explicit mode wins and validation reports the credentials as + forbidden. + """ settings = Settings( nextcloud_host="http://localhost", nextcloud_username="admin", nextcloud_password="password", - enable_multi_user_basic_auth=True, + deployment_mode="multi_user_basic", ) - # Note: This will detect as MULTI_USER_BASIC due to priority mode, errors = validate_configuration(settings) assert mode == AuthMode.MULTI_USER_BASIC - # It will fail multi-user validation because username/password are forbidden + # Should report errors for forbidden username/password assert len(errors) > 0 def test_vector_sync_without_embedding_provider_uses_fallback(self): @@ -167,7 +181,7 @@ class TestMultiUserBasicValidation: """Test valid minimal multi-user BasicAuth config.""" settings = Settings( nextcloud_host="http://localhost", - enable_multi_user_basic_auth=True, + deployment_mode="multi_user_basic", ) mode, errors = validate_configuration(settings) @@ -179,7 +193,7 @@ class TestMultiUserBasicValidation: """Test valid config with offline access enabled.""" settings = Settings( nextcloud_host="http://localhost", - enable_multi_user_basic_auth=True, + deployment_mode="multi_user_basic", enable_offline_access=True, oidc_client_id="test-client", oidc_client_secret="test-secret", @@ -195,7 +209,7 @@ class TestMultiUserBasicValidation: def test_missing_required_host(self): """Test error when NEXTCLOUD_HOST is missing.""" settings = Settings( - enable_multi_user_basic_auth=True, + deployment_mode="multi_user_basic", ) mode, errors = validate_configuration(settings) @@ -209,13 +223,12 @@ class TestMultiUserBasicValidation: nextcloud_host="http://localhost", nextcloud_username="admin", nextcloud_password="password", - enable_multi_user_basic_auth=True, + deployment_mode="multi_user_basic", ) mode, errors = validate_configuration(settings) - # Multi-user BasicAuth has higher priority than single-user in detection - # (explicit flags come before credentials) + # Explicit MCP_DEPLOYMENT_MODE wins over auto-detection from credentials assert mode == AuthMode.MULTI_USER_BASIC # Should report errors for forbidden username/password assert any("nextcloud_username" in err.lower() for err in errors) @@ -225,7 +238,7 @@ class TestMultiUserBasicValidation: """Test that offline access works without OAuth credentials (will use DCR).""" settings = Settings( nextcloud_host="http://localhost", - enable_multi_user_basic_auth=True, + deployment_mode="multi_user_basic", enable_offline_access=True, token_encryption_key="test-key-" + "a" * 32, token_storage_db="/tmp/tokens.db", @@ -241,7 +254,7 @@ class TestMultiUserBasicValidation: """Test error when offline access enabled but encryption key missing.""" settings = Settings( nextcloud_host="http://localhost", - enable_multi_user_basic_auth=True, + deployment_mode="multi_user_basic", enable_offline_access=True, oidc_client_id="test-client", oidc_client_secret="test-secret", @@ -261,7 +274,7 @@ class TestMultiUserBasicValidation: os.environ, { "NEXTCLOUD_HOST": "http://localhost:8080", - "ENABLE_MULTI_USER_BASIC_AUTH": "true", + "MCP_DEPLOYMENT_MODE": "multi_user_basic", "VECTOR_SYNC_ENABLED": "true", # Using old name for backward compat test "QDRANT_LOCATION": ":memory:", "OLLAMA_BASE_URL": "http://ollama:11434", @@ -659,7 +672,7 @@ class TestConfigurationConsolidation: os.environ, { "NEXTCLOUD_HOST": "http://localhost:8080", - "ENABLE_MULTI_USER_BASIC_AUTH": "true", + "MCP_DEPLOYMENT_MODE": "multi_user_basic", "ENABLE_SEMANTIC_SEARCH": "true", "QDRANT_LOCATION": ":memory:", "TOKEN_ENCRYPTION_KEY": "test-key", @@ -830,3 +843,54 @@ class TestExplicitModeSelection: mode = detect_auth_mode(settings) assert mode == AuthMode.LOGIN_FLOW + + def test_legacy_enable_multi_user_basic_auth_env_var_errors(self): + """ADR-022 follow-up: ENABLE_MULTI_USER_BASIC_AUTH=true must fail loudly. + + 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. + """ + with patch.dict( + os.environ, + { + "NEXTCLOUD_HOST": "http://localhost:8080", + "ENABLE_MULTI_USER_BASIC_AUTH": "true", + }, + clear=True, + ): + from nextcloud_mcp_server.config import get_settings + + _reload_config() + settings = get_settings() + + with pytest.raises(ValueError) as exc: + detect_auth_mode(settings) + + assert "ENABLE_MULTI_USER_BASIC_AUTH" in str(exc.value) + assert "multi_user_basic" in str(exc.value) + + def test_legacy_enable_login_flow_env_var_errors(self): + """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. + """ + with patch.dict( + os.environ, + { + "NEXTCLOUD_HOST": "http://localhost:8080", + "ENABLE_LOGIN_FLOW": "true", + }, + clear=True, + ): + from nextcloud_mcp_server.config import get_settings + + _reload_config() + settings = get_settings() + + with pytest.raises(ValueError) as exc: + detect_auth_mode(settings) + + assert "ENABLE_LOGIN_FLOW" in str(exc.value) + assert "login_flow" in str(exc.value) From 6e7c821761d59b7ce468e760f1e711ebf2039b44 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 12 May 2026 20:44:17 +0200 Subject: [PATCH 5/9] 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 From ade42b55dc5dbebc42cc32167ffd86db4b1c6443 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 12 May 2026 21:25:51 +0200 Subject: [PATCH 6/9] =?UTF-8?q?docs:=20clear=20review-round-3=20nits=20?= =?UTF-8?q?=E2=80=94=20stale=20login=5Fflow=5Fv2,=20duplicates,=20field=20?= =?UTF-8?q?comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five small findings from the reviewer's third round, plus a SonarCloud quality-gate failure on a test fixture. - docs/troubleshooting.md, docs/configuration.md: six pre-PR references to a non-existent `login_flow_v2` mode value (the actual enum value is `login_flow`). They predated this PR but became actively misleading once `detect_auth_mode` started raising ValueError for anything not in the mode_map. Replaced with `login_flow` via sed. - docs/configuration-migration-v2.md: removed a duplicate `MCP_DEPLOYMENT_MODE=multi_user_basic` line in the troubleshooting section (around line 447) — same shape as the round-2 duplicate caught earlier in the migration-steps section. Also dropped the `oauth_token_exchange` row from the mode-value table around line 364 (that enum value was removed in 57303135 and would now raise ValueError from detect_auth_mode). - nextcloud_mcp_server/config.py: field comments for `enable_multi_user_basic_auth` and `enable_login_flow` said "Auto-set by detect_auth_mode()" but the derivation moved into `Settings.__post_init__` in the previous commit. Updated both. - tests/unit/test_config_validators.py: SonarCloud's python:S2068 flagged `nextcloud_password="hunter2"` in the `test_login_flow_mode_auto_derives_enable_login_flow_flag` fixture I added in commit 5 as a potentially hard-coded credential. Other fixtures in the same file use the literal `"password"` and aren't flagged (they predate the PR and SonarCloud only checks new-code). Switched to `"password"` to match the existing convention. No functional changes; all 1009 unit tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/configuration-migration-v2.md | 4 +--- docs/configuration.md | 4 ++-- docs/troubleshooting.md | 8 ++++---- nextcloud_mcp_server/config.py | 4 ++-- tests/unit/test_config_validators.py | 2 +- 5 files changed, 10 insertions(+), 12 deletions(-) diff --git a/docs/configuration-migration-v2.md b/docs/configuration-migration-v2.md index e1c62db8..3aac3740 100644 --- a/docs/configuration-migration-v2.md +++ b/docs/configuration-migration-v2.md @@ -360,8 +360,7 @@ NEXTCLOUD_HOST=https://nextcloud.example.com |-----------|-------------| | `single_user_basic` | Single-user with username/password | | `multi_user_basic` | Multi-user with BasicAuth pass-through | -| `login_flow` | Multi-user OAuth (recommended) | -| `oauth_token_exchange` | Multi-user OAuth with token exchange | +| `login_flow` | Multi-user OAuth via Login Flow v2 (recommended) | ### Mode Detection Priority @@ -445,7 +444,6 @@ Server activates `login_flow` mode when you expected `multi_user_basic` Add explicit mode declaration: ```bash MCP_DEPLOYMENT_MODE=multi_user_basic -MCP_DEPLOYMENT_MODE=multi_user_basic ``` --- diff --git a/docs/configuration.md b/docs/configuration.md index 128a768e..63577c9c 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -38,12 +38,12 @@ The server supports three deployment modes. See [Authentication](authentication. |------|-------------| | `single_user_basic` | Personal use, dev — credentials in env vars | | `multi_user_basic` | Internal deployments — clients send credentials via `Authorization: Basic` header | -| `login_flow_v2` | Hosted / OAuth-based MCP clients (claude.ai, Astrolabe Cloud) — recommended for multi-user | +| `login_flow` | Hosted / OAuth-based MCP clients (claude.ai, Astrolabe Cloud) — recommended for multi-user | You can declare the mode explicitly: ```dotenv -MCP_DEPLOYMENT_MODE=login_flow_v2 +MCP_DEPLOYMENT_MODE=login_flow ``` If `MCP_DEPLOYMENT_MODE` is not set, the server auto-detects from the other env vars below. diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 6fb05915..d7e6b3a5 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -69,7 +69,7 @@ ENABLE_BACKGROUND_OPERATIONS=true **Symptom:** ``` -ValueError: Invalid MCP_DEPLOYMENT_MODE: 'oauth'. Valid values: single_user_basic, multi_user_basic, login_flow_v2 +ValueError: Invalid MCP_DEPLOYMENT_MODE: 'oauth'. Valid values: single_user_basic, multi_user_basic, login_flow ``` **Cause:** Invalid value for `MCP_DEPLOYMENT_MODE`. @@ -79,7 +79,7 @@ Use one of the valid mode values: ```bash MCP_DEPLOYMENT_MODE=single_user_basic # Single-user with username/app password MCP_DEPLOYMENT_MODE=multi_user_basic # Multi-user BasicAuth pass-through -MCP_DEPLOYMENT_MODE=login_flow_v2 # Multi-user via Login Flow v2 (recommended) +MCP_DEPLOYMENT_MODE=login_flow # Multi-user via Login Flow v2 (recommended) ``` Or remove `MCP_DEPLOYMENT_MODE` to use automatic detection. @@ -90,7 +90,7 @@ Or remove `MCP_DEPLOYMENT_MODE` to use automatic detection. **Symptom:** ``` -Error: [login_flow_v2] TOKEN_ENCRYPTION_KEY is required when ENABLE_SEMANTIC_SEARCH is enabled +Error: [login_flow] TOKEN_ENCRYPTION_KEY is required when ENABLE_SEMANTIC_SEARCH is enabled ``` **Cause:** In multi-user modes, semantic search automatically enables background operations, which require encrypted token storage. @@ -441,7 +441,7 @@ If problems persist, open an issue on the [GitHub repository](https://github.com - **Server logs** (with `--log-level debug`) - **Nextcloud version** -- **Deployment mode** (single_user_basic / multi_user_basic / login_flow_v2) +- **Deployment mode** (single_user_basic / multi_user_basic / login_flow) - **Error messages** - **Steps to reproduce** - **Environment details** (OS, Python version, Docker vs local) diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index c345ce27..ece2d641 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -456,14 +456,14 @@ class Settings: # Multi-user BasicAuth pass-through mode (ADR-019 interim solution). # Internal — not user-settable; the ENABLE_MULTI_USER_BASIC_AUTH env-var # alias was removed in the ADR-022 follow-up. Auto-set by - # detect_auth_mode() when MCP_DEPLOYMENT_MODE=multi_user_basic. When True, + # Settings.__post_init__ when MCP_DEPLOYMENT_MODE=multi_user_basic. When True, # the MCP server extracts BasicAuth credentials from request headers and # passes them through to Nextcloud APIs (no storage, stateless). Kept # as a field for backward compat with the runtime call sites that read it. enable_multi_user_basic_auth: bool = False # Login Flow v2 derived flag (ADR-022). Internal — not user-settable. - # Auto-set by detect_auth_mode() when the resolved deployment mode is + # Auto-set by Settings.__post_init__ when the resolved deployment mode is # LOGIN_FLOW. Kept as a field for backward compat with the runtime call # sites that read it (app.py, context.py, scope_authorization.py). enable_login_flow: bool = False diff --git a/tests/unit/test_config_validators.py b/tests/unit/test_config_validators.py index 7c0781f7..a814c66f 100644 --- a/tests/unit/test_config_validators.py +++ b/tests/unit/test_config_validators.py @@ -387,7 +387,7 @@ class TestLoginFlowValidation: basic_settings = Settings( nextcloud_host="http://localhost", nextcloud_username="alice", - nextcloud_password="hunter2", + nextcloud_password="password", ) assert basic_settings.enable_login_flow is False assert basic_settings.enable_multi_user_basic_auth is False From 1fa4c82fd28d0f93c4cfebf03106535021a64c85 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 12 May 2026 22:35:50 +0200 Subject: [PATCH 7/9] =?UTF-8?q?chore:=20address=20review-round-4=20nits=20?= =?UTF-8?q?=E2=80=94=20stale=20delenv,=20upgrade=20hint,=20in-sync=20notes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four small follow-ups from the reviewer's latest pass: - tests/unit/test_stdio.py:18: the single_user_env fixture used monkeypatch.delenv("ENABLE_MULTI_USER_BASIC_AUTH", ...). That env var is no longer read after the ADR-022 follow-up; switched to delenv of MCP_DEPLOYMENT_MODE which is the canonical mode-selection input today. Comment updated to match. - config_validators.py: when detect_auth_mode rejects an invalid MCP_DEPLOYMENT_MODE, surface a one-line ADR-022 migration hint if the rejected value is exactly "oauth_single_audience" (the most common upgrade pain — users carrying that value over from ADR-021 .env files). Other invalid values get the regular "Valid values: …" message unchanged. - config.py + config_validators.py: added cross-reference comments on both mode-resolution sites (Settings.__post_init__ and detect_auth_mode) noting that they each compute the canonical mode independently and must be kept in sync when a new mode is added. Surfaces the parallel-duplication intentionally so the next maintainer doesn't have to discover it. - docs/ADR-021-configuration-consolidation.md:92: appended a trailing comment to the historical "valid values" example, marking oauth_single_audience and oauth_token_exchange as removed in ADR-022. ADR-021 stays as the historical record; the trailer points future readers at the current state. No functional changes; 1009 unit tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/ADR-021-configuration-consolidation.md | 1 + nextcloud_mcp_server/config.py | 8 ++++++++ nextcloud_mcp_server/config_validators.py | 16 +++++++++++++++- tests/unit/test_stdio.py | 4 ++-- 4 files changed, 26 insertions(+), 3 deletions(-) diff --git a/docs/ADR-021-configuration-consolidation.md b/docs/ADR-021-configuration-consolidation.md index 513f8448..6298a754 100644 --- a/docs/ADR-021-configuration-consolidation.md +++ b/docs/ADR-021-configuration-consolidation.md @@ -90,6 +90,7 @@ MCP_DEPLOYMENT_MODE=login_flow # Valid values: single_user_basic, multi_user_basic, # oauth_single_audience, oauth_token_exchange +# (both OAuth values removed in ADR-022 — current value: login_flow) ``` **Detection logic**: diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index ece2d641..62eca989 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -636,6 +636,14 @@ class Settings: "of truth for selecting an auth flow." ) + # NOTE: this block mirrors the resolution logic in + # `config_validators.detect_auth_mode` (which works on strings via a + # `mode_map`). Both call sites resolve the deployment mode + # independently — the canonical AuthMode enum in detect_auth_mode, + # and the boolean derived flags here. **Keep them in sync when + # adding a new mode**: a new entry must be added in both places, in + # addition to `mode_map` (`config_validators.py`) and any + # MODE_REQUIREMENTS entry. resolved_mode = (self.deployment_mode or "").strip().lower() if not resolved_mode: if self.nextcloud_username and self.nextcloud_password: diff --git a/nextcloud_mcp_server/config_validators.py b/nextcloud_mcp_server/config_validators.py index 78af22d7..0b190659 100644 --- a/nextcloud_mcp_server/config_validators.py +++ b/nextcloud_mcp_server/config_validators.py @@ -172,6 +172,11 @@ def detect_auth_mode(settings: Settings) -> AuthMode: `Settings.__post_init__` so every Settings instance carries correct flags regardless of how it was constructed. + Keep the resolution logic here in sync with `Settings.__post_init__`: + both compute the canonical mode from `deployment_mode` (+ credentials + as a fallback). When adding a new mode, update `mode_map` *and* the + `__post_init__` resolution block in `config.py`. + Args: settings: Application settings @@ -196,9 +201,18 @@ def detect_auth_mode(settings: Settings) -> AuthMode: if mode_str not in mode_map: valid_modes = ", ".join(mode_map.keys()) + # ADR-022 migration hint: the most common upgrade pain is users + # carrying MCP_DEPLOYMENT_MODE=oauth_single_audience over from + # ADR-021. Surface a one-liner so they don't have to grep the + # changelog. + hint = ( + " (Note: 'oauth_single_audience' was renamed to 'login_flow' in ADR-022.)" + if mode_str == "oauth_single_audience" + else "" + ) raise ValueError( f"Invalid MCP_DEPLOYMENT_MODE: '{settings.deployment_mode}'. " - f"Valid values: {valid_modes}" + f"Valid values: {valid_modes}.{hint}" ) explicit_mode = mode_map[mode_str] diff --git a/tests/unit/test_stdio.py b/tests/unit/test_stdio.py index a70053be..cd2418ae 100644 --- a/tests/unit/test_stdio.py +++ b/tests/unit/test_stdio.py @@ -14,8 +14,8 @@ def single_user_env(monkeypatch): monkeypatch.setenv("NEXTCLOUD_HOST", "https://cloud.example.com") monkeypatch.setenv("NEXTCLOUD_USERNAME", "admin") monkeypatch.setenv("NEXTCLOUD_PASSWORD", "secret") - # Ensure multi-user mode is off (may leak from other tests) - monkeypatch.delenv("ENABLE_MULTI_USER_BASIC_AUTH", raising=False) + # Ensure no explicit deployment mode leaks from other tests + monkeypatch.delenv("MCP_DEPLOYMENT_MODE", raising=False) _reload_config() yield _reload_config() From 0fb21b5c6dfc49468a6d45e830644b8a23a09e49 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 12 May 2026 22:57:35 +0200 Subject: [PATCH 8/9] =?UTF-8?q?chore:=20address=20review-round-5=20?= =?UTF-8?q?=E2=80=94=20stale=20=5Fsync=5Fderived=5Fflags=20reference=20+?= =?UTF-8?q?=20missing=20migration-hint=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the reviewer's latest pass: - config_validators.py:329: comment in the LOGIN_FLOW validation block still referenced `_sync_derived_flags` (removed in commit 5; derivation now lives in `Settings.__post_init__`). Updated the comment to point at the correct location so a future reader grepping for the function name doesn't come up empty. - tests/unit/test_config_validators.py: added `test_oauth_single_audience_migration_hint` next to the existing `test_invalid_deployment_mode_raises_error`. The new test pins the ADR-022 rename-hint branch in `detect_auth_mode` by setting `MCP_DEPLOYMENT_MODE=oauth_single_audience` and asserting the ValueError mentions both the old and new mode names plus "ADR-022". Without this, a future refactor could drop the hint without any test catching it (the prior `invalid_mode` test only asserts the generic "Valid values:" prefix). No functional changes; 1010 unit tests now pass (+1 from the new hint test). Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/config_validators.py | 4 ++-- tests/unit/test_config_validators.py | 28 +++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/nextcloud_mcp_server/config_validators.py b/nextcloud_mcp_server/config_validators.py index 0b190659..7434afa9 100644 --- a/nextcloud_mcp_server/config_validators.py +++ b/nextcloud_mcp_server/config_validators.py @@ -325,8 +325,8 @@ def validate_configuration(settings: Settings) -> tuple[AuthMode, list[str]]: # ADR-022 follow-up: the un-augmented OAuth bearer pass-through (the # old OAUTH_SINGLE_AUDIENCE without ENABLE_LOGIN_FLOW) needed unmerged # Nextcloud user_oidc patches and is no longer supported. The - # `enable_login_flow` flag is now derived from the resolved mode by - # `_sync_derived_flags`, so users only configure the mode — no + # `enable_login_flow` flag is now derived from the resolved mode in + # `Settings.__post_init__`, so users only configure the mode — no # separate ENABLE_LOGIN_FLOW env var is needed. # If OAuth credentials not provided, DCR must be available diff --git a/tests/unit/test_config_validators.py b/tests/unit/test_config_validators.py index a814c66f..f7e97dec 100644 --- a/tests/unit/test_config_validators.py +++ b/tests/unit/test_config_validators.py @@ -788,6 +788,34 @@ class TestExplicitModeSelection: assert "invalid_mode" in str(e) assert "Valid values:" in str(e) + def test_oauth_single_audience_migration_hint(self): + """ADR-022: rejecting `oauth_single_audience` surfaces a rename hint. + + Pins the special-case branch in detect_auth_mode that helps users + upgrading from ADR-021 configurations spot the rename without + having to grep the changelog. + """ + with patch.dict( + os.environ, + { + "NEXTCLOUD_HOST": "http://localhost:8080", + "MCP_DEPLOYMENT_MODE": "oauth_single_audience", + }, + clear=True, + ): + from nextcloud_mcp_server.config import get_settings + + _reload_config() + settings = get_settings() + + with pytest.raises(ValueError) as exc: + detect_auth_mode(settings) + + msg = str(exc.value) + assert "oauth_single_audience" in msg + assert "login_flow" in msg + assert "ADR-022" in msg + def test_explicit_mode_overrides_auto_detection(self): """Test explicit mode takes precedence over auto-detection.""" with patch.dict( From bf424135a5c7eff3029b6c44b5a613f09f99f45e Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 12 May 2026 23:27:17 +0200 Subject: [PATCH 9/9] docs(adr-025): remove duplicate [login_flow] section in TOML example MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 4 renamed `[oauth_single_audience]` → `[login_flow]` via a global sed pass, but ADR-025's example settings.toml already had a `[login_flow]` section just above the `[keycloak]` block. The rename produced two back-to-back `[login_flow]` headers with identical contents — TOML parsers either reject the file or silently override, and a reader copying the example would land on either outcome. Dropped the now-duplicate second `[login_flow]` section (the renamed one). The earlier `[login_flow]` section retains the same content + a comment explaining the ADR-022 derivation, so no information is lost. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/ADR-025-dynaconf-configuration-management.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/docs/ADR-025-dynaconf-configuration-management.md b/docs/ADR-025-dynaconf-configuration-management.md index b905335f..33defa6c 100644 --- a/docs/ADR-025-dynaconf-configuration-management.md +++ b/docs/ADR-025-dynaconf-configuration-management.md @@ -211,9 +211,6 @@ token_storage_db = "/app/data/tokens.db" enable_token_exchange = true token_storage_db = "/app/data/tokens.db" token_exchange_cache_ttl = 300 - -[login_flow] -token_storage_db = "/app/data/tokens.db" ``` **`.secrets.toml.example`** — Template, checked into git (actual `.secrets.toml` is gitignored):