- ProvisionSignal.wait() re-arms in a finally so a cancelled wait (shutdown
racing the doorbell) leaves a fresh unset event, not a stale set-but-consumed
one; preserves the no-await-before-swap lost-wakeup guarantee.
- Hoist user_manager_task's _wake_on helper out of the while loop (one object,
not one per iteration).
- Test: assert ProvisionSignal via its public wait() contract instead of the
private _event attribute.
- Add test_provision_app_password_wakes_user_manager covering the
api/passwords.py wake path (previously only LFv2 web + MCP tool were tested).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #825 review round 2:
- _validate_nextcloud_credentials now only maps OCS HTTP 401/403 to a 401
"invalid credential"; any other non-200 (5xx, 503 maintenance mode) surfaces
as 502 "Nextcloud returned a server error" so ops don't chase a phantom bad
password when Nextcloud is actually down.
- The client-facing 401 message is now a parameter, so delete_app_password keeps
its "Invalid credentials" wording without unwrapping/rebuilding the helper's
JSONResponse.
- Body parsing catches (ValueError, UnicodeDecodeError) instead of bare
Exception, and guards body.get behind isinstance(body, dict) — no longer
swallows RuntimeError/AttributeError or a non-object JSON body.
- Add a unit test asserting 500/503 -> 502.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review of PR #825 surfaced an auth bypass introduced by adding loginName
support to delete_app_password: with the OCS-resolved UID discarded, a user
could authenticate as their own loginName (via the request body) while
targeting another user's path and delete the victim's stored app password.
Add the same UID-mismatch guard provisioning already has, so the
authenticated account must own the path UID (403 otherwise).
Also:
- integration test: build the BasicAuth header via base64 instead of
httpx.BasicAuth._auth_header (private attribute); mark the throwaway test
credential NOSONAR(S2068).
- unit tests: cover the httpx.RequestError -> 502 branch, the standard OCS v2
success shape (meta.statuscode 200), and the cross-user delete 403 guard.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
provision_app_password validated credentials against OCS v1
(/ocs/v1.php/cloud/user), which always returns HTTP 200 — even on auth
failure, where the real status lives in ocs.meta.statuscode (997) and
ocs.data comes back as an empty list []. The status_code != 200 guard
therefore never fired, execution fell through to [].get("id"), and the
resulting AttributeError escaped as an unhandled 500. This blocked
background vector indexing for any user whose supplied loginName didn't
resolve (e.g. display name "Admin" vs loginName "admin").
Extract a shared _validate_nextcloud_credentials helper that:
- queries OCS v2 (/ocs/v2.php), which maps the OCS status onto the HTTP
status, so a failed credential is a real 401;
- parses the payload defensively (isinstance guards) so a non-dict
ocs.data can never raise;
- returns a clean 502 for an unreachable Nextcloud or a non-JSON body.
delete_app_password shared the same v1.php dead-guard bug, which made its
credential check a no-op (any valid-format password passed) — an auth
bypass on deletion. Route it through the same helper and accept the
loginName from the request body (mirroring provisioning) so OIDC users
whose UID differs from their loginName are not regressed.
Adds unit regression tests for the OCS failure payload, non-dict data,
and non-JSON response, plus a login-flow integration test that provisions
with capitalized ("Admin") and spaced ("Test User") loginNames and asserts
a 401 rather than a 500.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Nextcloud authenticates app passwords against the *loginName*, which differs
from the UID for OIDC-provisioned users (e.g. user_oidc makes the UID the
display name: UID "Ada Lovelace", loginName "ada@example.com"). The runtime
consumers of stored app passwords bound the UID as the BasicAuth username, so
every Notes/Files/Shares/CalDAV call returned HTTP 401.
PR #818 fixed only the provisioning endpoint; the consuming paths were missed.
Observed on a login_flow tenant (NC's own OIDC app as IdP): the background-sync
scan loop never started ("Credential validation failed ... HTTP 401") and
semantic search returned 0 results because the ACL shared_with_me lookup 401'd
and degraded to a self-only owner filter.
Root cause: NextcloudClient / CalendarClient conflated two identities — the
DAV/URL path identity (the user_id the whole system keys on = NC UID) and the
auth-credential username (the loginName). Decouple them:
- Thread a keyword-only auth_username through NextcloudClient -> CalendarClient
(defaults to username, so single-user / OAuth where UID == loginName is
unchanged).
- get_user_client_basic_auth (background sync + the /api/v1/vector-viz/search
endpoint) authenticates as the stored loginName, UID for paths.
- _get_client_from_login_flow (the get_client(ctx) MCP-tool path) does the same.
- cleanup_invalid_app_passwords validates with the loginName, so it no longer
401s and wrongly deletes a valid OIDC user's password.
The loginName is already persisted in app_passwords.username and returned by
get_app_password_with_scopes. Adds unit tests covering the UID != loginName
split for both client builders, the calendar credential/path split, and the
cleanup validation. Also genericises the example user in the #818 comment/test
(real name/email -> Ada Lovelace / ada@example.com).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
provision_app_password validated the supplied app password by calling the
OCS cloud/user endpoint with BasicAuth as the *path user_id* (the UID).
Nextcloud keys app-password BasicAuth on the loginName, which differs from
the UID for OIDC-provisioned accounts whose UID is their display name
(UID "Chris Coutinho", loginName "chris@coutinho.io"). Authenticating as
the UID is rejected with HTTP 401 ("App password validation failed"), so
provisioning never completes.
Parse the request body up front and authenticate the OCS validation as the
body's `username` (the Nextcloud loginName), falling back to the path
user_id for legacy callers where UID == loginName. The OCS-returned account
id is still checked against the path user_id (the UID), and the password is
still stored keyed by UID with the loginName alongside.
Note this is not an encoding issue: BasicAuth places the user-id literally
in the header (RFC 7617, no URL-encoding); %20/+/literal-space forms of the
UID all fail — only the loginName authenticates.
Adds a regression test asserting the OCS BasicAuth uses the loginName while
storage is keyed by the UID, plus a backward-compat assertion that callers
without a loginName fall back to the UID.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two fixes surfaced while testing Login Flow v2 provisioning behind a split
internal/external host (Docker: server↔Nextcloud over http://app, browser
over http://localhost:8080):
1. login_url pointed at the internal host. Nextcloud builds the login URL
from the request host, so the browser-facing URL came back as
http://app/login/v2/flow/... — unreachable from the user's browser. The
poll endpoint was already rewritten to the internal host (correct, the
server polls it); now LoginFlowV2Client also rewrites the login_url origin
to settings.nextcloud_public_issuer_url when set (passed at all 5
construction sites). When unset, behaviour is unchanged.
2. The app-password format guard rejected raw session tokens. core/
getapppassword returns a long alphanumeric token, not the dashed 25-char
Security-settings format, so the dashed-only regex 400'd the one-click
opt-in handoff. Relax APP_PASSWORD_PATTERN to `^[a-zA-Z0-9-]{20,256}$`;
the authoritative validation is still the BasicAuth check against Nextcloud.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Unit test fixes:
- test_userinfo_routes: patch nextcloud_httpx_client instead of httpx.AsyncClient
- test_instrument_tool: patch trace_operation in metrics module (where imported)
- test_management_app_password_endpoints: patch nextcloud_httpx_client and
get_settings at correct import locations
- test_management_status_endpoint: patch detect_auth_mode and get_settings at
correct import locations (api.management, not config/config_validators)
- test_token_exchange: fix TokenBrokerService constructor args (client_id/
client_secret instead of encryption_key)
CI:
- Add Node.js setup and astrolabe build step (composer + npm ci + npm run build)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
httpx emits a DeprecationWarning when verify=<str> is passed, recommending
ssl.SSLContext instead. This affected both our httpx client factories and
the caldav library passthrough.
Changed get_nextcloud_ssl_verify() to return bool | ssl.SSLContext instead
of bool | str by constructing an SSLContext when NEXTCLOUD_CA_BUNDLE is set.
All downstream consumers (httpx, caldav) natively accept ssl.SSLContext.
Also fixed app password endpoint tests that used overly broad MagicMock
(auto-generated truthy nextcloud_ca_bundle attribute).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split the monolithic management.py (1988 lines) into 4 focused modules:
- management.py: Server status, user sessions, shared helpers (~520 lines)
- passwords.py: App password provisioning for BasicAuth mode (~300 lines)
- webhooks.py: Webhook registration management (~290 lines)
- visualization.py: Search and PDF preview endpoints (~810 lines)
Backward compatibility maintained via __init__.py re-exports.
Updated test imports to use new module paths.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Security improvements:
- Add in-memory rate limiter for app password provisioning (5 attempts/hour/user)
- Returns 429 Too Many Requests with Retry-After header when limit exceeded
- Rate limiting is per-user to prevent cross-user DoS
Code quality improvements:
- Extract _extract_basic_auth() helper to reduce duplication across 3 endpoints
- Move base64, re imports to module level
- Add APP_PASSWORD_PATTERN constant for regex validation
- Add NEXTCLOUD_VALIDATION_TIMEOUT constant (10s)
Test coverage:
- Add test_provision_app_password_rate_limiting
- Add test_rate_limiting_is_per_user
- Add autouse fixture to clear rate limit state between tests
- Total: 15 tests for management API endpoints
Addresses reviewer feedback on PR #473.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
