Incorporate reviewer feedback across three review rounds:
- Remove post_hooks from Phase 1 constructor; defer to Phase 4
- Fix Validator syntax: use condition=lambda instead of ne= kwarg
- Add MCP_DEPLOYMENT_MODE validator to catch typos at startup
- Add CRITICAL to LOG_LEVEL validator enum
- Make OTEL_TRACES_SAMPLER_ARG validation conditional on ratio samplers
- Add all missing provider env vars to settings.toml (Bedrock, Anthropic, Ollama, Simple)
- Add provider secrets to .secrets.toml.example
- Fix DynaconfDict import to stable public API path
- Strengthen ignore_unknown_envvars risk: CI lint check mandatory before Phase 2
- Document ValidationError vs ValueError breaking change in Phase 3
- Acknowledge environments=True legacy risk with mitigation
- Address root_path pip-install concern (intentional: pip uses env vars)
- Add enable_token_exchange to adapter example; note exhaustive field mapping
- Clarify Provider Registry is Phase 6 with explanation of os.getenv coexistence
- Improve test isolation fixture with teardown reload + _dynaconf visibility note
- Add Docker Compose volume mount host-file existence note
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The root_path for dynaconf resolved to site-packages instead of the
application root when installed non-editable (Docker). This caused all
settings without env var overrides to be None, crashing on startup with
a TypeError in chunk size validation.
Fix root_path to fall back to CWD when settings.toml isn't at the
source-tree path, and refactor get_settings() to only pass values
dynaconf actually has — letting Settings dataclass defaults apply for
unconfigured keys. Mount settings.toml into all docker-compose MCP
services as a read-only volume.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Many identity providers (AWS Cognito, Okta, Azure AD) reject or mishandle
colons in OAuth scope names. This migrates all custom scopes from
`resource:action` to `resource.action` format (e.g., `notes:read` →
`notes.read`), which is universally accepted and aligns with industry
conventions (Microsoft, Google).
Includes Alembic migration 004 for stored scope strings and ADR-024
documenting the rationale and RFC references.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Document wildcard scope policy in ClientRegistry class docstring
- Add hostname None guard and IPv6 loopback (::1) to redirect URI validation
- Simplify redirect URI scheme validation into single guard clause
- Add try/finally cleanup to DCR client deletion test
- Validate 302 Location header in unknown client rejection test
- Add unit tests for IPv6 loopback, malformed URIs, and DCR proxy paths
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address all 9 review points from PR #680:
- Fix post_hooks code examples to use correct return-dict signature
- Expand test isolation section with fixture factory, DynaconfDict, and
reload patterns
- Document ignore_unknown_envvars silent failure mode in Negative
Consequences and add env var audit to Phase 1 checklist
- Fix NEXTCLOUD_HOST validator to be unconditional (required in all modes)
- Document environments=True edge cases (unset mode, ENV_FOR_DYNACONF
shadowing)
- Add upper bound to dynaconf version pin (>=3.2.13,<4.0)
- Tighten Pydantic Settings comparison to acknowledge 2.x TOML support
- Make .gitignore additions explicit in Phase 1 checklist
- Clarify that shell-level .env loading still works with load_dotenv=False
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove the unused ALLOWED_MCP_CLOUD_CLIENTS env var — all clients are
defined via ALLOWED_MCP_CLIENTS or the static well-known defaults.
Add keycloak as an integration test profile in CI now that login-flow
replaces the old bearer token approach for external IdPs.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix incorrect hook syntax (@hookable.post → Dynaconf(post_hooks=[...])),
broken Qdrant mutual exclusivity validator, missing root_path for settings
file resolution, and empty string defaults that bypass validators. Add test
isolation section, mark Phase 4 as optional/future with risk note, and
correct Pydantic comparison (already a project dependency).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Merge ALLOWED_MCP_CLOUD_CLIENTS into a single ALLOWED_MCP_CLIENTS env var
that supports both simple client IDs and pipe-separated client_id|redirect_uri
entries. Enforce HTTPS for non-localhost redirect URIs, warn on malformed
entries, and use wildcard scopes for all static clients (upstream IdP enforces
actual scopes). Add deprecation warning for the old env var.
Also fixes DCR proxy error messages to reference only ALLOWED_MCP_CLIENTS and
use "Upstream" instead of "Nextcloud" for IdP-agnostic language. Enables
Login Flow v2 + DCR on the mcp-keycloak docker-compose service.
Adds 17 unit tests for ClientRegistry parsing/validation and 7 keycloak
integration tests for DCR lifecycle, AS metadata, and client authorization.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Propose migrating from manual os.getenv() calls to dynaconf for
file-based configuration. Key decisions: envvar_prefix=False for
backward compatibility, MCP_DEPLOYMENT_MODE as environment switcher,
TOML settings files with secret separation, and incremental migration
via adapter pattern.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude AI (web) sends a Cognito-issued client_id with an HTTPS redirect
URI, but the client registry only supported localhost redirect URIs via
ALLOWED_MCP_CLIENTS. Add ALLOWED_MCP_CLOUD_CLIENTS env var for web-based
clients with format "client_id|redirect_uri".
Also fix the DCR proxy to return a clear error when the upstream IdP
(e.g. Cognito) doesn't support dynamic client registration, instead of
silently falling back to a Nextcloud-specific endpoint that fails.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove all RFC 8693 token exchange tests (integration, manual, keycloak)
since Nextcloud doesn't support bearer tokens without upstream patches
- Remove manual impersonation/ADR-004 scripts and their docs
- Clean up token_exchange singleton from integration conftest
- Improve logging in _complete_login_flow_v2_as_user with step-by-step
[username] prefixed messages matching _complete_login_flow_v2 style
- Remove unnecessary time staggering from all_login_flow_user_tokens;
concurrent token acquisition works without artificial delays
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The OAuth profile removal dropped cross-user permission tests (deck, files,
notes) that validated Nextcloud sharing/ACL enforcement through MCP tools.
These tested general functionality, not OAuth-specific behavior.
Restores coverage with login-flow fixtures and 9 tests covering file share
read/write enforcement, folder sharing, Deck board ACL view/edit, and
per-user resource isolation for files, boards, and notes.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace 4 stale mcp-oauth references in CLAUDE.md with mcp-login-flow
- Import and catch playwright.async_api.TimeoutError in consent retry loop
(Playwright's TimeoutError doesn't inherit from Python's built-in)
- Replace unreachable `return True` with explicit RuntimeError raise
- Add clarifying comment for hardcoded login-flow port default
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The login-flow MCP server exposes 3 additional auth tools
(nc_auth_provision_access, nc_auth_check_status, nc_auth_update_scopes)
from ADR-022 that require only 'openid' scope. Update the
no-custom-scopes test to expect 7 auth tools instead of 4.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Discourage raw docker exec for database queries; use scripts/dbquery.py
and scripts/sqlitequery.py exclusively
- Ensure all python commands use uv run prefix (mcp run, pytest, etc.)
- Replace Progressive Consent section with concise Deployment Modes
overview (single-user, multi-user BasicAuth, Login Flow v2)
- Normalize docker-compose to docker compose throughout
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
