fix(config): derive mode flags in Settings.__post_init__; address review round 2
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) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
282c245da1
commit
6e7c821761
+1
-1
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user