`tests/server/login_flow/test_management_api.py` had
`[pytest.mark.integration, pytest.mark.oauth]` while every other test in
`tests/server/login_flow/` uses `[pytest.mark.integration,
pytest.mark.login_flow]`. The single-user CI matrix filter is
`(integration and not keycloak and not login_flow and not
multi_user_basic)`, so the missing `login_flow` mark let this test
collect and run under single-user mode against `localhost:8004` (which
isn't up there, hitting the bug being reported), even though it's
specifically driving the login-flow MCP server.
Also `oauth` isn't a registered marker (see `[tool.pytest.ini_options]`
in pyproject.toml), so it was emitting an unregistered-marker warning.
Replacing the marker aligns this file with its siblings: single-user /
multi-user-basic / keycloak filters all deselect it now, and the
login-flow filter still picks it up.
Verified: `pytest --collect-only -m "<single-user filter>"` reports 2
deselected; `-m login_flow` collects both tests.
The Astrolabe webhooks UI hits /api/v1/apps on the MCP server, which
forwarded the OAuth bearer token to /ocs/v1.php/cloud/apps?filter=enabled.
That OCS endpoint is admin-only AND @PasswordConfirmationRequired —
neither requirement is satisfiable via an OAuth bearer token, so even an
admin user's token returns a silent 401 (no entry in nextcloud.log).
Switch to /ocs/v2.php/cloud/capabilities, which has no admin or password-
confirmation gate, accepts the existing bearer token, and returns a
capabilities map keyed by app id (notes, files, tables, forms, etc.).
This is sufficient for the webhook presets UI to gate available presets
against the running Nextcloud instance's enabled apps.
Bearer is preserved on the outbound call because anonymous capabilities
omits notes/tables/forms — only authenticated capabilities exposes them.
Tests:
- New unit test covers the regression (asserts /ocs/v2.php/cloud/capabilities
is hit, NOT /cloud/apps), response parsing, sanitized error messages,
and missing-config paths.
- New integration test under tests/server/login_flow/ drives a real
OAuth flow against mcp-login-flow with a static OIDC client
(nextcloudMcpServerUIPublicClient) and asserts /api/v1/apps returns 200
with core/files in the response.
docker-compose.yml: aligns mcp-login-flow's ALLOWED_MGMT_CLIENT with
mcp-multi-user-basic so the same static-client test fixture works for both.
Follow-up to homelab-argocd #1608, which set ALLOWED_MGMT_CLIENT in
production but didn't unblock the webhooks flow.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Gate browser session creation on a successful refresh token. When the
IdP returns no refresh token, SessionAuthBackend would silently reject
every subsequent request and bounce the user back to /oauth/login in a
loop. The callback now bails with a 400 + correlation ID + actionable
hint about offline_access *before* writing browser_sessions or setting
the cookie. Pinned by a new end-to-end unit test.
- Evict orphaned browser_sessions rows in SessionAuthBackend when the
associated refresh token is gone, instead of letting them accumulate
until TTL cleanup. Best-effort; deletion errors stay non-fatal.
- Demote identity-bearing logs in the Flow 2 OAuth callback (user_id,
scopes, audience, expires_at) from INFO to DEBUG so they don't leak
into multi-tenant log aggregation on every provision.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Coerce refresh_expires_in to int before arithmetic in both callback
paths so IdPs that serialize the field as a JSON string (e.g. AWS
Cognito) don't trigger an unhandled TypeError 500.
- Drop the orphaned oauth_session row written by _check_logged_in. The
canonical Flow 2 row is created by generate_oauth_url_for_flow2 keyed
by `state`, which is what the unified callback looks up; the
flow2_<hex> session_id was never matched and just churned the table
for 10 minutes per call.
- Match delete_cookie attributes (httponly, secure, samesite) to the
set_cookie call on logout so browsers reliably evict the cookie even
on implementations that consider security flags during deletion.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five findings from the latest review on #758 (2 medium, 3 nit):
Medium:
- browser_oauth_routes.oauth_login_callback + oauth_routes.oauth_callback_nextcloud:
fail closed with 400 when the oauth_session row is unknown/expired. Previously
both callbacks fell through with code_verifier="" and expected_nonce=None,
silently bypassing the PKCE + nonce protections introduced in earlier rounds.
Symmetric unit tests pin both contracts.
- token_utils.verify_id_token: use secrets.compare_digest for the nonce check
instead of short-circuit !=. Mirrors the sibling PKCE verifier comparison;
closes the last secret-equality timing-side-channel surface in the auth path.
Nit:
- Tighten the comment at all 4 mcp_authorization_code/code_verifier store +
retrieve sites so a future refactor sees the field reuse immediately
(renaming the column requires a schema migration).
- _should_use_secure_cookies: explicit string normalisation instead of
bool(settings.cookie_secure). Dynaconf normally coerces but tests / direct
settings.set calls can leave the raw string in place — bool("false") is True.
New parametrized unit tests cover the coercion matrix + http/https fallback.
- oauth_routes.py:591 f-string log converted to lazy %s formatting (folded into
the Flow 2 callback rewrite).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three findings from the latest review on #758 (1 medium, 2 low):
Medium:
- browser_oauth_routes.oauth_logout: move delete_browser_session into a
finally block so an error from delete_refresh_token can no longer leave
an orphan browser_sessions row. The orphan was not exploitable
(SessionAuthBackend rejects sessions without a live refresh token), but
it lingered until the hourly cleanup cron — a correctness gap. New
regression test pins the fix.
Low:
- oauth_callback_nextcloud: drop redundant ``or None`` from
``expected_nonce=nonce``. ``nonce`` is already ``str | None`` and
``secrets.token_urlsafe`` never produces an empty string, so the
coercion was a no-op that could mislead future readers into thinking
empty-string was a valid skip-the-check path.
- storage.RefreshTokenStorage.initialize: fail fast at startup when
SQLite < 3.35, since ``DELETE ... RETURNING`` (used in
``delete_browser_session``) needs that minimum. Ubuntu 20.04 ships
3.31 and would otherwise hit OperationalError on every logout.
Prerequisite also documented in docs/installation.md.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Drop "(in-place)" from filter-helper docstrings; callers should
consume the return value, mutation is an implementation detail.
- Document that deck_get_archived_stacks always returns cards (an
archived stack without its cards has no audit value); point to
description_max_length for size control.
- Document that deck_get_cards applies filtering client-side, so it
is network-equivalent to deck_get_stack(include_cards=True).
- Pin the empty-list contract: a stack with all-archived cards and
include_archived_cards=False yields cards == [] (loaded but empty),
not cards is None (explicitly suppressed).
- Add explicit one-character-over-limit truncation test alongside the
existing exact-boundary test.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Seven findings from the latest review on #758 (3 medium, 4 low/nit):
Medium:
- storage.py: replace 5 ``assert self.cipher is not None`` sites with
explicit ``RuntimeError`` so missing TOKEN_ENCRYPTION_KEY can't silently
become an AttributeError under ``python -O``
- session_backend.py: document the silent-invalidation invariant —
refresh-token TTL expiry without explicit logout deliberately makes
the browser session unusable; future readers must not relax it
- server/oauth_tools.py: drop user_id from the Flow 2 session_id
identifier — use ``flow2_{secrets.token_hex(16)}`` so audit logs and
DB rows don't carry user_id in the session_id field
Low / nit:
- token_utils.py: drop _fetch_locks dict entry in finally so a probed
deployment can't grow the lock dict without bound; coalescing test
now pins the invariant with len(_fetch_locks) == 0
- browser_oauth_routes.py: strip trailing slash from settings.nextcloud_host
before constructing the well-known URL so a host configured as
``https://cloud.example.com/`` doesn't produce a double-slash
- browser_oauth_routes.py: add comment explaining the three-layer CSRF
policy on the mcp_session cookie set (SameSite=Lax + POST-only logout
+ Origin/Referer check)
- oauth_routes.py: convert all 23 f-string log calls to lazy %-style
per the CLAUDE.md / memory feedback_lazy_logging convention
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Move description_max_length validation to tool layer
(_validate_description_max_length), matching the existing
_validate_comment_message pattern; helper now trusts callers per
CLAUDE.md ("validate at system boundaries only").
- Fix mutation/return inconsistency: deck_get_stacks now uses a list
comprehension to capture _apply_stack_filters' return, matching
deck_get_stack / deck_get_archived_stacks.
- Rename include_archived -> include_archived_cards on deck_get_cards
and _apply_card_filters for consistency with deck_get_stacks.
- Route deck_get_archived_stacks through _apply_stack_filters so
future filters apply uniformly to active + archived paths.
- Trim _truncate_card_descriptions docstring to one line; add inline
comment in _apply_stack_filters explaining the breaking-change
default (mirrors Deck UI archived-card filtering).
- Replace fragile call_args[0][1] with call_args.args[1] in the
archived-stacks client test.
- Modernize Optional[X] -> X | None throughout deck.py (adjacent
cleanup called out in the review).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Validate description_max_length is positive (raises ValueError on 0
or negative); the prior code would have wiped descriptions to a
single ellipsis character on description_max_length=0.
- Extract filter logic into testable module-level helpers
(_apply_board_filters, _apply_stack_filters, _apply_card_filters)
and replace the dense `continue`-based loop in deck_get_stacks with
the reviewer's elif form.
- Document the truncation length quirk in the helper docstring (result
is description_max_length + 1 chars when truncation fires).
- Add 13 new unit tests covering the include/exclude flags on board,
stacks, and flat card lists, plus the new validation paths and an
explicit "description fits within limit, no ellipsis" case.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add filtering options to deck read tools to keep responses compact on
boards with accumulated cards/comments, and expose archived stacks so
agents can audit completed work that has been archived off the active
board.
- deck_get_board: include_acl, include_users, include_labels
- deck_get_stacks/deck_get_stack: include_cards, include_archived_cards,
description_max_length
- deck_get_cards: include_archived, description_max_length
- New deck_get_archived_stacks tool wrapping the existing client method
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Seven findings from the latest review on #758, plus a regression test
catching the substance of the cache-stampede fix:
- verify_id_token: widen id_token annotation to str | None to match
callers passing nc_token_response.get("id_token")
- extract_user_id_from_token: use JSON-RPC reserved error code -32001
instead of -1
- _get_cached: per-URL anyio.Lock dict + meta-lock coalesces concurrent
cache misses into a single IdP fetch (mirrors token_broker.py idiom)
- delete_browser_session: collapse SELECT+DELETE into atomic
DELETE ... RETURNING user_id (SQLite >= 3.35)
- new test_origin_normalise.py: parametrized port/scheme/host equivalence
cases for the CSRF Origin guard
- browser_oauth_routes: correct misleading "PR #758 finding 5" cross-
references (finding 5 was Fernet-key hardening, not CSRF)
- ASProxySession.nonce: make required, drop spurious "legacy session"
default; reword the in-flight `or None` comment to reflect that
ASProxySession is purely in-memory
- new test_get_cached_coalesces_concurrent_misses: pins the
cache-stampede protection — fires 10 concurrent _get_cached calls and
asserts exactly one HTTP fetch
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Flow 2 (oauth_authorize_nextcloud) now generates a nonce, stores it on
the oauth_session row, forwards it to the IdP, and verifies it via
expected_nonce in oauth_callback_nextcloud — closes the last replay-
protection gap (round-3 finding 1).
- _origin_matches_self fails closed when mcp_server_url is missing
instead of allowing the logout, and the diagnostic log is promoted
from warning to error so the misconfiguration is monitorable
(round-3 finding 2). New regression test pins the new behaviour.
- The five user_id-accepting helpers in oauth_tools.py (get_provisioning_status,
provision_nextcloud_access, revoke_nextcloud_access, check_provisioning_status,
check_logged_in) are renamed with leading underscores to make the
trust boundary structural rather than documentary
(round-3 finding 3).
- create_browser_session and delete_browser_session now emit audit_log
rows so session establishment / teardown match the pattern used by
the rest of the security-relevant storage operations
(round-3 nit 5). delete_browser_session selects user_id before delete
so the audit row is attributable.
- oauth_login_callback no longer reflects raw IdP-error text or
exception strings into the HTML failure page; users see a generic
"internal error occurred" message + a correlation ID, with the
detail logged server-side keyed by the same ID (round-3 nit 6).
The XSS regression test is updated to pin the stricter contract.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- oauth_login_callback's integrated-mode token-exchange branch now reuses
the shared discovery cache via get_oidc_discovery (round-2 finding 1).
- AS proxy flow now generates an OIDC nonce in oauth_authorize, stores it
on ASProxySession, forwards it to the IdP, and passes it as
expected_nonce to verify_id_token in _oauth_callback_as_proxy
(round-2 finding 2).
- Consolidate the two parallel discovery caches: oauth_routes' local
_discovery_cache and _get_cached_discovery are removed; all callers
now go through token_utils.get_oidc_discovery, which acquires the
follow_redirects=True knob it needs for Nextcloud installs without
pretty URLs (round-2 finding 3).
- Demote per-user INFO logs in oauth_tools.py (check_logged_in,
get_provisioning_status) to DEBUG; the elicitation auth URL is no
longer logged because it contains a sensitive state token
(round-2 finding 4).
Also pin nonce binding behaviour with a new unit test that asserts
_oauth_callback_as_proxy forwards session.nonce to verify_id_token, and
update test mocks to track the cache consolidation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Blocking:
- AS proxy callback now calls verify_id_token before caching the proxy
code so a tampered IdP response can't smuggle identity claims.
Important:
- Browser OAuth flow generates and verifies an OIDC nonce; new alembic
migration 006 adds the nonce column to oauth_sessions.
- _origin_matches_self logs a warning when CSRF check is bypassed.
- oauth_tools.py uses get_shared_storage instead of fresh handles.
Nits:
- New token_utils.get_oidc_discovery shares the 5-minute cache with
verify_id_token; oauth_login (integrated) and _revoke_refresh_token_at_idp
now use it instead of issuing fresh discovery fetches.
- Drop typing.Optional from oauth_tools.py in favour of X | None.
CI:
- test.yml generates an ephemeral Fernet TOKEN_ENCRYPTION_KEY per run
with openssl, removing the dependency on a missing repo secret.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the two remaining 🟡 findings from the PR #758 follow-up review:
1. extract_user_id_from_token previously fell back to "default_user" when
the verified access token had no sub claim. In a multi-tenant deployment
a malformed IdP token could have bucketed every request under a single
sentinel user, risking cross-tenant data exposure. The function now
raises McpError on that branch; the BasicAuth no-token sentinel path is
preserved.
2. oauth_callback_nextcloud (Flow 2) read the PKCE code_verifier from
oauth_sessions but never deleted the row, leaving the verifier valid for
the full 10-minute TTL. The row is now deleted eagerly inside the same
branch, mirroring oauth_login_callback in browser_oauth_routes.
Also wires TOKEN_ENCRYPTION_KEY through the docker-compose step in the CI
test workflow so the integration matrix can boot — every job had been
failing fast on the ${TOKEN_ENCRYPTION_KEY:?...} interpolation guard added
in PR #758 finding 5.
Tests pin both fixes (test_token_utils_user_id.py,
test_oauth_callback_session_cleanup.py).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Six findings from the latest claude-bot review on PR #758:
- JWKS cache had no kid-miss refresh path (Medium): on IdP key
rotation every login failed for up to _OIDC_CACHE_TTL. Evict
and refetch once before raising, per OIDC core §10.1.1.
- _should_use_secure_cookies fell back to nextcloud_host scheme,
but the cookie is issued by the MCP server. Switch to
settings.nextcloud_mcp_server_url so split-scheme deployments
get the right Secure flag.
- _origin_matches_self compared raw netloc strings, which include
the port. Browsers omit default ports per RFC 6454 §6.2; an
mcp_server_url like :443 falsely 403'd every legitimate logout.
Normalise (scheme, host, port) tuples with default ports stripped.
- delete_oauth_session exists in storage.py — drop the stale
"we don't have this method" comment and call it eagerly so
replays can't be processed and the table doesn't accumulate
completed-but-not-yet-expired browser-login rows.
- extract_user_id_from_token's unused ctx param renamed to _ctx
to signal "intentionally unused" at the signature level.
- provisioning_decorator instantiated RefreshTokenStorage per
call. Switch to get_shared_storage() for the lock-protected
process-wide singleton.
Plus pre-push self-review catch: lazy-logging on the unchanged
except arm in session_backend.py.
Adds 5 regression tests:
- JWKS rotation: success on refetch
- JWKS rotation: still-missing-kid surfaces original error
- JWKS rotation: network error during refresh wrapped as
IdTokenVerificationError
- default-port CSRF: explicit :443 in config + portless Origin
- default-port CSRF: portless config + explicit :443 in Origin
- scheme-mismatch CSRF: same host, different scheme rejected
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After merging master, _should_use_secure_cookies was refactored to read
from Settings instead of os.getenv, which dropped `import os` from
browser_oauth_routes.py — leaving _revoke_refresh_token_at_idp's four
remaining os.getenv() calls undefined (CI ruff F821).
Migrate the helper to the same Settings-based pattern:
- oidc_discovery_url → settings.oidc_discovery_url
- OIDC_CLIENT_ID → settings.oidc_client_id
- OIDC_CLIENT_SECRET → settings.oidc_client_secret
- NEXTCLOUD_HOST → settings.nextcloud_host
Drive-by: the previous fallback read OIDC_CLIENT_ID, but the canonical
env var per env.sample / docker-compose is NEXTCLOUD_OIDC_CLIENT_ID.
The Settings layer handles this mapping via dynaconf, so the corrected
name is now used automatically.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses all 9 findings from the review on PR #758:
Blocking:
- _revoke_refresh_token_at_idp now reads config from oauth_ctx["config"]
(the production-shaped nested dict). Previously read flat keys, causing
IdP revocation to silently no-op in production. Test fixtures rebuilt
to the realistic nested shape so the bug can't regress unnoticed.
- HTML error responses in oauth_login_callback now wrap IdP-controlled
error_body, str(e), and the attacker-controlled error/error_description
query params in html_escape. New test_browser_oauth_xss.py pins this.
Important:
- New _safe_next_url helper validates the ?next= query param at write
time (oauth_login), in oauth_logout, and on read from the session row
in oauth_login_callback. Blocks https://, // (protocol-relative), and
CRLF/whitespace injection.
- verify_id_token now caches discovery + JWKS (5-min TTL) using the
same pattern as oauth_routes._get_cached_discovery. New caching
regression test pins to one fetch per URL across multiple calls.
- /oauth/logout is now POST-only at the route layer (defeats passive
CSRF via <img src>). oauth_logout also validates Origin/Referer
against the configured mcp_server_url. Logout UI in user_info.html
converted from <a href> to <form method="post">.
- New storage.cleanup_expired_browser_sessions() called from the hourly
cleanup loop in app.py — previously these rows accumulated for users
who never explicitly logged out.
Nits:
- Demoted INFO logs that leaked oauth_config.keys() / client_id /
token-storage state to DEBUG. Operator-relevant outcome lines
(login successful, refresh token stored, logged out) stay INFO.
- verify_id_token algorithms widened to RS256, PS256, ES256 — covers
Azure AD (PS256) and Cognito/some Keycloak realms (ES256). Symmetric
and "none" remain off the allowlist.
- Migrated all Optional[X] usages in auth/storage.py to X | None per
CLAUDE.md.
Breaking change: GET /oauth/logout now returns 405. The in-tree logout
UI was migrated to a POST form; any external bookmark or curl-based
caller that relied on GET will need to switch.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three review items from the third-round review on PR #757:
- scope_authorization: split the combined logger.warning(error_msg) in
the require_scopes decorator's missing-app-password branch into two
lazy %-style logger calls (one per branch), keeping the f-string
error_msg for the exception only. The else branch also logs the
elicit_result for diagnostics. Bypassing lazy %-interpolation in
security-sensitive code formatted the message regardless of log level
and matched the repo-wide lazy-logging preference; the new code now
conforms.
- config + browser_oauth_routes: wire COOKIE_SECURE through Settings
(cookie_secure: bool | None = None) so _should_use_secure_cookies()
reads it via get_settings() rather than os.getenv. Completes the
consolidation pass that touched this file in commit 7464340 and
removes the last raw os.getenv from browser_oauth_routes.py
(import os dropped). Dynaconf auto-coerces "true"/"false" → bool;
"1"/"0" arrive as int and are normalised by an explicit bool() at
the consumer.
- elicitation: clarify the _astrolabe_settings_url docstring to call
out that the empty-string case is also a None-return path (matches
the existing `if not base:` guard).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-launch hardening for the hosted Astrolabe Cloud offering. Addresses
all five findings raised in #626 (Tim Kaufmann, code review of v0.65.0).
Re-verified against master before fixing.
Finding 3 (LLM-controllable user_id) — drop user_id from the public
signatures of provision_nextcloud_access, revoke_nextcloud_access,
check_provisioning_status, check_logged_in. Tool wrappers now always
derive identity from the verified AccessToken; user_id is no longer
accepted as MCP input. Adds parameterized CI-guard test that locks the
schema.
Finding 2 (predictable session cookie) — replace mcp_session=<user_id>
cookie with a cryptographically random session_id mapped server-side
(new browser_sessions table, alembic 005). Cookie value is opaque,
expires, revocable. SessionAuthBackend looks up user_id via the new
mapping and additionally requires a refresh token to fail closed.
Finding 4 (logout doesn't revoke refresh token) — oauth_logout now
calls the IdP revocation_endpoint (RFC 7009) when advertised, deletes
the stored refresh token regardless, and clears the browser_sessions
row. Cleanup is best-effort: logout always 302s.
Finding 1 (unverified ID token decodes) — verify_id_token helper does
JWKS signature + issuer + audience + exp + nonce checks per OIDC core
3.1.3.7. Used by both OAuth callback handlers (browser + MCP). Removes
the four "verify_signature: False" decodes that previously trusted IdP
claims unconditionally. Drops dead-code _validate_token_audience in
token_broker. Refactors token_utils + provisioning_decorator to read
user_id from the verified AccessToken instead of re-decoding the JWT.
Finding 5 (hardcoded Fernet keys in docker-compose.yml) — replace the
three inline TOKEN_ENCRYPTION_KEY values with required env var
interpolation; document in env.sample.
Test coverage: 4 new unit test modules (signature pinning, browser
sessions, ID-token verification, logout + revoke + session backend).
693 unit tests pass; ruff/format/ty clean.
Migration note: existing browser admin-UI sessions become invalid on
rollout (cookies are looked up against the new browser_sessions table,
which starts empty). Users re-login. MCP API access is unaffected.
Tracked on Astrolabe Cloud POC board card #37.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The elicitation flow points users to the Astrolabe web route or the
BasicAuth REST endpoint to provision their app password. Both paths
stored the password without clearing the in-process scope cache, so a
user who provisioned through them would keep hitting
ProvisioningRequiredError for up to _SCOPE_CACHE_TTL (5 min) afterwards.
Add invalidate_scope_cache(user_id) to both write-paths (matching the
existing pattern in nc_auth_check_status), correct the now-misleading
comment in scope_authorization.py to name all three invalidation paths,
and add a one-line hint above the first elicitation patch in the test
file so future authors don't "fix" the patch target to the wrong module.
Addresses PR #757 round-3 review feedback.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four review items from the second-round review on PR #757:
- scope_authorization: broaden the post-elicit retry message to acknowledge
the 5-minute scope-cache TTL — if the LFv2 poller is still in-flight at
acknowledge-time, the immediate retry can still hit a stale cache.
- elicitation: extract a shared `_run_elicit(ctx, message, schema, *,
log_label)` helper so `present_login_url` and
`present_provisioning_required` no longer duplicate the
hasattr-guard / try-NotImplementedError / try-Exception fallback block.
The data-acknowledged warning specific to login-flow stays in
`present_login_url` so behaviour is preserved exactly.
- elicitation: detect missing http:// / https:// scheme in
`_astrolabe_settings_url`, log a warning, and return None — caller
renders the safe tool-only fallback instead of producing a broken link.
New unit test locks this in.
- browser_oauth_routes: replace the stray
`os.getenv(\"NEXTCLOUD_HOST\")` in `_should_use_secure_cookies` with
`get_settings().nextcloud_host` for consistency with the rest of the
file (PR #757 review nit).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lift NEXTCLOUD_PUBLIC_ISSUER_URL out of raw os.getenv reads into
Settings.nextcloud_public_issuer_url across all 8 production call sites
(app.py x2, oauth_routes.py x2, browser_oauth_routes.py,
provision_routes.py, userinfo_routes.py, elicitation.py). cli.py
remains the env-write source so the existing config-by-flag pipeline
still works.
Also addresses remaining PR #757 review nits:
- elicitation.py: align URL-present/absent wording on "open in your
browser" so users don't try clicking in the terminal
- test_scope_authorization_stored.py: lock in the deliberately-shared
fall-through branch with explicit declined/cancelled decorator tests
- test_elicitation.py: switch from monkeypatch.setenv to
patch(get_settings) since Settings is now the canonical surface
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Branch the ProvisioningRequiredError message on the elicit result so a
user who acknowledged the prompt isn't told to call
nc_auth_provision_access (which would loop an LLM that just confirmed
via elicitation). Other paths keep the existing instruction.
- Convert present_login_url's f-string logger.warning to lazy %s, matching
present_provisioning_required and the repo's lazy-logging preference.
- Add a test for NEXTCLOUD_PUBLIC_ISSUER_URL trailing-slash normalization.
- Strengthen the decorator-elicits test: split into the "accepted" and
"message_only" branches so the error-message change is regression-tested.
Refs: cbcoutinho/nextcloud-mcp-server#757#issuecomment-4363552487
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Calls out the apps-to-install matrix (user_oidc required, oidc skip,
astrolabe optional), the OIDC clients to register and what each is for,
the per-app scope advertisement requirement on the IdP side, and the
"OAuth succeeded but Nextcloud returns 401" diagnosis path.
Mined from the cbcoutinho/nextcloud-mcp-server#752 thread.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When a tool requiring Nextcloud access is called without a stored app
password (Login Flow v2 mode), the @require_scopes decorator now invokes
MCP elicitation with a clickable Astrolabe settings URL — reconstructed
from NEXTCLOUD_PUBLIC_ISSUER_URL / NEXTCLOUD_HOST — before raising
ProvisioningRequiredError. Clients without elicitation support fall back
to the existing text error.
Surfaced by cbcoutinho/nextcloud-mcp-server#752, where users hit a 401
after OAuth and had no clickable URL to start Login Flow v2 from.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switch the diff range from $BASE..HEAD to $BASE so the review covers
working-tree changes (committed + staged + unstaged), letting the skill
run usefully on in-progress work without requiring a commit first.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Six review items raised; four required code changes (#3, #4, #5, #6) and
two were resolved without code changes (#1 audit-only, #2 informational).
* search/verification.py — clarify the granularity asymmetry between the
whole-batch fail-open (structural API failure) and the per-item fail-open
(single bad stored doc_id). Future readers no longer need to derive why
the two paths have different blast radii from the code alone.
* models/semantic.py — `dropped_document_count` description now explicitly
notes that subtracting it from `verified_chunk_count` is not a meaningful
operation, since the two fields count different units (documents vs
chunks). Surfaces the unit mismatch where MCP clients actually see it.
* server/semantic.py — clarify the per-doc_type over-fetch comment so the
N×2 pre-merge Qdrant cost (vs the cross-app branch's 1×2) is explicit
rather than implied by "same 2× over-fetch budget".
* tests/unit/search/test_verification.py — add four new 429 unit tests
(notes/news/files/deck) mirroring the existing 5xx-keeps pattern. Locks
in that `_is_definitive_404_or_403` returns False for 429 so a future
refactor cannot accidentally treat rate-limit responses as permanent
revocations.
Audit confirmation for review item #1: all four `WebDAVClient.get_file_info`
call sites already handle the new `HTTPStatusError`-on-404 contract
(verification.py:156, tests/integration/test_rag.py:139,
tests/unit/client/test_webdav.py:153/190). No silent breakage internal to
this repo.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Deployer role:
- Add servicediscovery actions; module always creates Cloud Map namespace
and service so the policy must grant CreatePrivateDnsNamespace etc.
- Make Route53 + ACM permissions unconditional. The server module always
issues an ACM cert and writes Route53 records (no CloudFront default-cert
path exists), so gating these on route53_zone_ids was broken. Split
Route53 into hosted-zone management (always) plus record-set mutation
(scoped to caller-supplied zones, falls back to *).
- Remove unused cloudfront:* statement; no CloudFront resources in module.
- Replace acm:* wildcard with explicit cert-management action set.
Server module:
- qdrant_image_tag is now nullable with default null and validated against
use_external_qdrant, so external-qdrant callers can omit it instead of
passing a sentinel "unused" value.
- task_role_arn and efs_id outputs marked sensitive; qdrant_dns_name returns
null when use_external_qdrant = true.
- ALB SG now has matching IPv6 egress rule (was v4-only).
- nextcloud_url validates the https:// scheme.
- random_pet.subdomain keeper includes zone_name so a zone migration that
preserves zone_id still triggers regeneration.
- Pin required_version >= 1.9 on both modules.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three minor fixes from the round-11 review on PR #750:
- bm25_hybrid.py:209 — Comment said `doc_id` is `int (notes) or str (files)`,
which is backwards. Notes, news_items, and deck_cards are stored as `str`
(scanner.py:241, 666, 867); files are stored as `int` (scanner.py:425).
Updated to point readers at scanner.py as the source of truth.
- verification.py:338 — Lowered the News-API 403/404 log line from `info`
to `debug`. The News app being uninstalled or disabled is a predictable
operational state (matching the other verifiers' debug-on-not-found
paths), so this should not generate operator-dashboard noise. Transient
errors immediately below stay at `warning` because they're unexpected.
- semantic.py:809 — `nc_get_vector_sync_status` was reading
`document_receive_stream` via `getattr(..., None)`, but the attribute is
guaranteed-defined on both `AppContext` and `OAuthAppContext` (as a
field with `None` default). The defensive `getattr` masked typos that
the eviction_task_group access at semantic.py:197-199 deliberately
surfaces. Switched to direct access; the `if … is None:` value-check
below is preserved (the attribute can legitimately be None before sync
starts).
Items deliberately deferred (with rationale in the plan file):
- News verifier semaphore-hold during get_items (reviewer: "not required
here, just worth tracking"; ADR already lists follow-ups).
- Hardcoded 2× over-fetch / VERIFICATION_OVERFETCH (TODO already in code).
- Integration test for the real Qdrant eviction filter (reviewer marked
low-priority; type-preservation chain is unit-tested).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Tighten verify_search_results signature: client: Any → NextcloudClientProtocol
- Collapse 3 copy-pasted lock-justification comments to a single-line pointer
- Add logger.debug timing around the verify_search_results call site
- Add logger.debug timing around the unbounded news.get_items fetch
- Rename SemanticSearchResponse.dropped_count → dropped_document_count to make
the chunks-vs-documents unit asymmetry explicit at the API boundary
- Drop unreachable duplicate 409 branch in WebDAVClient.move_resource
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
