From 292cbb3292b6bc16c22e738299de132e3a27efd9 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 16 May 2026 18:06:42 +0200 Subject: [PATCH 1/4] feat(storage): pluggable database backend via DATABASE_URL (ADR-026) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a `DATABASE_URL` setting that lets `RefreshTokenStorage` run against any SQLAlchemy async backend, primarily `postgresql+asyncpg://...` for HA k8s deployments. Default behavior is unchanged: when `DATABASE_URL` is unset the server falls back to the existing `TOKEN_STORAGE_DB` path / ephemeral SQLite tempfile. Why --- Today every MCP pod needs its own PVC to hold the SQLite file, which pins the Deployment to one replica and blocks horizontal scaling. With this change, operators can point all replicas at a shared Postgres (CNPG, RDS, etc.) and the pods become stateless. Encryption stays in Python (Fernet); the database only sees ciphertext. What changed ------------ - `config.get_database_url()` resolves DATABASE_URL → TOKEN_STORAGE_DB → ephemeral tempfile in that priority order. - `RefreshTokenStorage` builds a process-shared `AsyncEngine` in `initialize()`. SQLite gets NullPool; Postgres gets pool_size=10, max_overflow=20, pool_pre_ping=True. 30 aiosqlite call sites adapted via a thin `_DBConn` / `_Cursor` / `_Row` / `_ExecuteCtx` shim so existing method bodies need no churn beyond the connection context-manager swap. - 7 `INSERT OR REPLACE` statements rewritten as portable `INSERT ... ON CONFLICT (...) DO UPDATE` (SQLite ≥ 3.24, Postgres ≥ 9.5). - `sqlite_master` legacy-detection lookup replaced with SQLAlchemy inspector so the path works against either backend. - File-permission hardening + parent-dir creation gated on `is_sqlite_url(...)` — centralized backends manage their own filesystem. - Alembic migrations 001/002/003/005 converted from raw `op.execute(SQL)` to portable `op.create_table()` / `op.create_index()` with SQLAlchemy types. All timestamp columns are `sa.BigInteger` so Postgres allocates BIGINT (unix epochs don't fit in INT4). SQLite treats BIGINT as INTEGER, so existing deployments at revision 006 see no schema drift. - `migrations.py` + CLI take URLs; `db {upgrade,downgrade,current,history}` gain `--database-url / -u` alongside the legacy `--database-path / -d`. `get_current_revision()` uses SQLAlchemy inspector instead of raw sqlite3, so the CLI works against Postgres too. - `docker-compose.yml` adds a `postgres-test` service under the `postgres` profile (pinned `postgres:16-alpine` digest) for integration testing. - Unit storage tests parametrized over backends via shared `tests/fixtures/storage_backend.py` — every test in `test_app_password_storage.py` and `test_webhook_storage.py` runs once per backend that is available. Postgres is opted in by `TEST_DATABASE_URL`. - New `tests/integration/test_storage_postgres.py` (5 tests, marked `postgres` + `integration`) covers refresh-token, app-password, OAuth-session, webhook, and audit-log paths end-to-end on Postgres. - New `docs/ADR-026-pluggable-database-backend.md` records the decision; `docs/configuration.md` documents `DATABASE_URL` with examples. Out of scope ------------ - No SQLite → Postgres data migration tool (clean cutover; tokens reissue on next login, webhooks re-register on next sync tick). - This repo does not provision Postgres. The matching helm chart change lives in cbcoutinho/helm-charts (database.url / existingSecret values). Verification ------------ - `uv run pytest tests/unit/` — 1012 passed, SQLite path unchanged. - `docker compose --profile postgres up -d postgres-test` - `TEST_DATABASE_URL=... uv run pytest tests/integration/test_storage_postgres.py -m postgres -v` — 5 passed. - `TEST_DATABASE_URL=... uv run pytest tests/unit/test_app_password_storage.py tests/unit/test_webhook_storage.py` — 50 passed (25 per backend). - `uv run ruff check && uv run ruff format --check && uv run ty check -- nextcloud_mcp_server` — clean. Tracked on Astrolabe Cloud POC board, card #99. --- _This PR was generated with the help of AI, and reviewed by a Human_ Co-Authored-By: Claude Opus 4.7 (1M context) --- docker-compose.yml | 23 + docs/ADR-026-pluggable-database-backend.md | 169 +++++ docs/configuration.md | 42 ++ .../20251217_2200_001_initial_schema.py | 245 +++---- .../20260113_1200_002_add_app_passwords.py | 32 +- ...0227_1200_003_add_scopes_and_login_flow.py | 89 +-- .../20260502_1500_005_add_browser_sessions.py | 38 +- nextcloud_mcp_server/auth/storage.py | 618 +++++++++++++----- nextcloud_mcp_server/cli.py | 153 ++--- nextcloud_mcp_server/config.py | 28 + nextcloud_mcp_server/migrations.py | 166 +++-- pyproject.toml | 3 + tests/fixtures/__init__.py | 0 tests/fixtures/storage_backend.py | 97 +++ tests/integration/test_storage_postgres.py | 164 +++++ tests/unit/conftest.py | 5 + tests/unit/test_app_password_storage.py | 35 +- tests/unit/test_webhook_storage.py | 26 +- uv.lock | 57 ++ 19 files changed, 1402 insertions(+), 588 deletions(-) create mode 100644 docs/ADR-026-pluggable-database-backend.md create mode 100644 tests/fixtures/__init__.py create mode 100644 tests/fixtures/storage_backend.py create mode 100644 tests/integration/test_storage_postgres.py diff --git a/docker-compose.yml b/docker-compose.yml index cddd583a..1d7077a3 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -352,6 +352,29 @@ services: profiles: - qdrant + # Centralized Postgres backend for token storage. Used by: + # 1. Integration tests gated on @pytest.mark.postgres — they read + # TEST_DATABASE_URL=postgresql+asyncpg://mcp:mcp@localhost:5433/mcp. + # 2. Manual smoke testing of the HA (multi-replica) deployment story. + # Bring up with: docker compose --profile postgres up -d postgres-test + # See ADR-026 for the pluggable-database-backend design. + postgres-test: + image: docker.io/library/postgres:16-alpine@sha256:16bc17c64a573ef34162af9298258d1aec548232985b33ed7b1eac33ba35c229 + restart: unless-stopped + environment: + POSTGRES_USER: mcp + POSTGRES_PASSWORD: mcp + POSTGRES_DB: mcp + ports: + - 127.0.0.1:5433:5432 + healthcheck: + test: ["CMD-SHELL", "pg_isready -U mcp -d mcp"] + interval: 5s + timeout: 3s + retries: 10 + profiles: + - postgres + volumes: nextcloud: db: diff --git a/docs/ADR-026-pluggable-database-backend.md b/docs/ADR-026-pluggable-database-backend.md new file mode 100644 index 00000000..3e39b76c --- /dev/null +++ b/docs/ADR-026-pluggable-database-backend.md @@ -0,0 +1,169 @@ +# ADR-026: Pluggable database backend (DATABASE_URL) + +## Status + +Accepted — 2026-05-16 + +## Context + +`RefreshTokenStorage` (in `nextcloud_mcp_server/auth/storage.py`) holds all +of the MCP server's persistent state: refresh tokens, OAuth client +credentials, OAuth sessions, browser sessions, app passwords, login-flow +sessions, audit logs, and webhook registrations. Until this ADR it was +backed by a single SQLite file, with the path configured by +`TOKEN_STORAGE_DB`. + +This works well for single-user deployments but blocks horizontal scaling +in Kubernetes: + +- Every pod needs its own PVC (ReadWriteOnce) or a ReadWriteMany volume. +- Tokens stored on pod A are invisible to pod B, so a Service can only + route traffic to one pod at a time. +- Restart / re-deploy cycles either drop the volume (token loss) or + require coordinated PVC handling. +- Backup, encryption-at-rest, and multi-region replication become + per-pod concerns rather than centrally managed DB concerns. + +We needed a way for pods to be stateless and share a centralized store +without giving up the zero-config SQLite path that single-user installs and +local development rely on. + +## Decision + +Introduce a `DATABASE_URL` setting that accepts any SQLAlchemy async URL, +with `sqlite+aiosqlite:///...` remaining the default. The runtime keeps a +single linear migration history and a single `RefreshTokenStorage` class — +the backend is selected purely by the URL. + +### Resolution order + +`get_database_url()` (in `nextcloud_mcp_server/config.py`) returns: + +1. `DATABASE_URL` if set — wins over everything. +2. Otherwise `sqlite+aiosqlite:///{get_token_db_path()}`, so the legacy + `TOKEN_STORAGE_DB` env var and the process-local ephemeral tempfile + fallback both keep working unchanged. + +### Why SQLAlchemy Core + async engine, not an ABC with parallel drivers + +Two alternatives were considered: + +| Option | Why rejected | +|---|---| +| Define a `Storage` ABC with `SQLiteStorage` (aiosqlite) and `PostgresStorage` (asyncpg) implementations | Doubles the surface area — every schema change has to land in two backends, with two sets of migrations, two SQL dialects, two upsert idioms. Diverges over time. | +| Switch to a full SQLAlchemy ORM (declarative models) | Larger refactor; the existing explicit-SQL style is intentional and well-understood by reviewers. | +| **Keep `RefreshTokenStorage` and put SQLAlchemy Core under it** *(chosen)* | One method body per operation, one migration history (Alembic is already SQLAlchemy-based). The URL drives dialect, pool, and DDL. | + +A thin compatibility shim (`_DBConn` / `_Cursor` / `_Row` in `storage.py`) +adapts the `async with aiosqlite.connect(...) as db: async with +db.execute(...) as cursor: ...` idiom to SQLAlchemy `AsyncEngine` / +`AsyncConnection`. Existing method bodies needed only their connection +context-manager swapped; `?` placeholders are rewritten to named binds on +the fly. The seven `INSERT OR REPLACE` statements were rewritten as +portable `INSERT ... ON CONFLICT (...) DO UPDATE` (SQLite ≥ 3.24, Postgres +≥ 9.5; we already require SQLite ≥ 3.35 elsewhere). + +### Encryption stays in Python (Fernet), not the DB + +The DB only ever sees ciphertext for sensitive columns +(`encrypted_token`, `encrypted_client_secret`, `encrypted_password`, +`encrypted_poll_token`). The Fernet key remains a `TOKEN_ENCRYPTION_KEY` +env var, applied in Python before INSERT and after SELECT. This means: + +- Switching backends does not invalidate or re-key existing data. +- Postgres-level features like `pgcrypto` are not required. +- Operators rotating the encryption key still go through the existing + Python path. + +### DDL portability + +All Alembic migrations were rewritten from raw `op.execute("CREATE TABLE +...")` strings to `op.create_table()` / `op.create_index()` calls with +SQLAlchemy types. Notable choices: + +- All `*_at` / expiration / timestamp columns use `sa.BigInteger` — + Postgres `INTEGER` is 32-bit and unix epochs are already past that range. + SQLite treats `BIGINT` and `INTEGER` identically (dynamic typing) so + this is backwards compatible. +- `BLOB` → `sa.LargeBinary` (becomes `BYTEA` on Postgres). +- `BOOLEAN DEFAULT FALSE` → `sa.Boolean, server_default=sa.false()`. +- Existing SQLite deployments are at revision `006` and skip the + rewritten migrations entirely — content rewrites are safe. + +### No data migration, no shipped Postgres + +Two scope decisions worth recording: + +1. **Clean cutover, no SQLite → Postgres data migration tool.** Tokens + are reissued on the next login; webhooks re-register on the next sync + tick. Acceptable because the ephemeral-default already implies this, + and the data being preserved (audit logs, OAuth sessions) is either + short-lived or reconstructible. +2. **Bring-your-own database.** The MCP server consumes a + `DATABASE_URL`; it does not provision Postgres itself. Operators use + CNPG, RDS, the project's existing Helm chart with a sub-chart, etc. + The `postgres-test` service in `docker-compose.yml` exists only for + integration tests and manual HA smoke testing — it is gated on the + `postgres` profile and is not the recommended production pattern. + +### CLI changes + +The `nextcloud-mcp-server db {upgrade,downgrade,current,history}` commands +gain a `--database-url / -u` flag (env `DATABASE_URL`) alongside the +existing `--database-path / -d` (env `TOKEN_STORAGE_DB`). `-u` wins over +`-d`; both fall back to `get_database_url()`. + +## Consequences + +### Positive + +- MCP server pods become stateless. A Kubernetes Deployment can run with + `replicas: 3` behind a Service, with all pods pointed at the same + Postgres URL — tokens written by pod A are immediately visible to pod B. +- Centralized DB operations (backup, restore, replication, encryption at + rest, monitoring) are handled by the operator's existing Postgres + infrastructure rather than duplicated per-pod. +- No regression for single-user / local-development / docker-compose + installs — the SQLite tempfile path is unchanged and remains the default + when `DATABASE_URL` is unset. +- Test coverage doubles automatically: every test that uses the + `temp_storage` fixture now runs against both SQLite and Postgres when + `TEST_DATABASE_URL` is exported. + +### Negative + +- One more thing operators have to think about for HA deployments + (Postgres connection string, credentials secret, network policies). +- Adds SQLAlchemy + asyncpg to the runtime dependency set. SQLAlchemy was + already transitively present via Alembic; asyncpg is genuinely new. +- The compatibility shim in `storage.py` is a small piece of bespoke code + that future contributors need to understand. The alternative — rewriting + every method body to SQLAlchemy idioms — was rejected as too risky for + this PR but might be revisited. + +### Neutral + +- The Alembic migration history was content-rewritten but its revision + graph is unchanged (still `001 → 006`), so existing SQLite deployments + do not re-run anything. +- `TOKEN_STORAGE_DB` still works exactly as before; deployments that + already set it require no changes. + +## Related + +- [ADR-022 Login Flow v2](ADR-022-deployment-mode-consolidation.md) — + defines the per-user app password storage that this ADR centralizes. +- [ADR-002 Vector sync authentication](ADR-002-vector-sync-authentication.md) + — explains the offline-access tokens that benefit most from HA storage. + +## Verification + +1. `uv run pytest tests/unit/` — SQLite path unchanged (1012 tests). +2. `docker compose --profile postgres up -d postgres-test` then + `TEST_DATABASE_URL=postgresql+asyncpg://mcp:mcp@localhost:5433/mcp uv run pytest tests/unit/test_app_password_storage.py tests/unit/test_webhook_storage.py` + — every test runs once per backend. +3. Manual end-to-end smoke against `mcp-login-flow` with a Postgres URL + (commands in `/home/chris/.claude/plans/spicy-enchanting-flurry.md` → + Verification). +4. k8s HA validation (after merge in `homelab-argocd`): `replicas: 3`, + confirm session continuity through the Service. diff --git a/docs/configuration.md b/docs/configuration.md index 63577c9c..0beb1150 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -118,6 +118,48 @@ See [Login Flow v2](login-flow-v2.md) for full setup, scope reference, and troub --- +## Centralized Token Storage (DATABASE_URL, Optional) + +By default the MCP server stores tokens / sessions / app passwords in a +local SQLite file (`TOKEN_STORAGE_DB`, falling back to a per-process +tempfile). For HA Kubernetes deployments where you need multiple +stateless pods to share state, point the server at a centralized +database via `DATABASE_URL`. + +```env +# Centralized Postgres backend (HA k8s deployments) +DATABASE_URL=postgresql+asyncpg://mcp:secret@postgres.svc.cluster.local:5432/mcp +TOKEN_ENCRYPTION_KEY= +``` + +| Variable | Required | Description | +|----------|----------|-------------| +| `DATABASE_URL` | Optional | SQLAlchemy async URL for any supported backend. When set, wins over `TOKEN_STORAGE_DB`. Primary supported targets: `postgresql+asyncpg://...` (recommended for HA) and `sqlite+aiosqlite:///...` (development). | +| `TOKEN_STORAGE_DB` | Optional | Legacy SQLite-only path. Used when `DATABASE_URL` is unset. Falls back to a per-process ephemeral tempfile when both are unset. | + +Notes: + +- **Bring-your-own DB.** The MCP server doesn't provision the database; + it just consumes the URL. Use CNPG, RDS, your existing Helm chart's + Postgres sub-chart, etc. +- **Encryption stays in the app.** `TOKEN_ENCRYPTION_KEY` (Fernet) is + applied in Python; the database only ever sees ciphertext for + sensitive columns. You don't need `pgcrypto`. +- **Schema is managed automatically.** On startup the server runs + Alembic migrations against the configured backend. Existing SQLite + deployments are stamped at the current revision and skip re-execution. +- **No data migration tool.** Moving from SQLite to Postgres is a clean + cutover — tokens are reissued on the next login, webhooks + re-register on the next sync tick. +- **Testing a Postgres backend locally:** `docker compose --profile + postgres up -d postgres-test` then export + `DATABASE_URL=postgresql+asyncpg://mcp:mcp@localhost:5433/mcp`. + +See [ADR-026 Pluggable database backend](ADR-026-pluggable-database-backend.md) +for the architecture rationale. + +--- + ## SSL/TLS Configuration (Optional) If your Nextcloud instance uses a self-signed certificate or a private CA (common with reverse proxies like Traefik or Caddy), the MCP server will reject the connection by default. Use these settings to configure certificate verification. diff --git a/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py b/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py index 3f1110c0..36eed2c5 100644 --- a/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py +++ b/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py @@ -8,12 +8,19 @@ This migration creates the initial database schema including: - registered_webhooks: Webhook registration tracking (both OAuth and BasicAuth) - schema_version: Legacy schema version tracking (deprecated, use alembic_version) +Uses Alembic's portable schema-DDL helpers (``op.create_table`` / +``op.create_index``) with SQLAlchemy types so the DDL is emitted correctly +for both SQLite (BLOB / INTEGER PRIMARY KEY AUTOINCREMENT) and Postgres +(BYTEA / SERIAL). See ADR-026. + Revision ID: 001 Revises: Create Date: 2025-12-17 22:00:00.000000 """ +import sqlalchemy as sa + from alembic import op # revision identifiers, used by Alembic. @@ -24,143 +31,104 @@ depends_on = None def upgrade() -> None: - """Create initial database schema.""" + """Create initial database schema. - # Refresh tokens table (OAuth mode only, for background jobs) - op.execute( - """ - CREATE TABLE IF NOT EXISTS refresh_tokens ( - user_id TEXT PRIMARY KEY, - encrypted_token BLOB NOT NULL, - expires_at INTEGER, - created_at INTEGER NOT NULL, - updated_at INTEGER NOT NULL, - -- ADR-004 Progressive Consent fields - flow_type TEXT DEFAULT 'hybrid', - token_audience TEXT DEFAULT 'nextcloud', - provisioned_at INTEGER, - provisioning_client_id TEXT, - scopes TEXT, - -- Browser session profile cache - user_profile TEXT, - profile_cached_at INTEGER - ) - """ + All ``*_at`` / expiration / timestamp columns use :class:`sa.BigInteger` + so Postgres allocates BIGINT (8-byte) and unix epoch values don't + overflow the 32-bit int32 INTEGER range on long-lived sessions. SQLite + treats BIGINT and INTEGER identically (dynamic typing), so this is + backwards compatible. + """ + + op.create_table( + "refresh_tokens", + sa.Column("user_id", sa.Text, primary_key=True), + sa.Column("encrypted_token", sa.LargeBinary, nullable=False), + sa.Column("expires_at", sa.BigInteger), + sa.Column("created_at", sa.BigInteger, nullable=False), + sa.Column("updated_at", sa.BigInteger, nullable=False), + # ADR-004 Progressive Consent fields + sa.Column("flow_type", sa.Text, server_default="hybrid"), + sa.Column("token_audience", sa.Text, server_default="nextcloud"), + sa.Column("provisioned_at", sa.BigInteger), + sa.Column("provisioning_client_id", sa.Text), + sa.Column("scopes", sa.Text), + # Browser session profile cache + sa.Column("user_profile", sa.Text), + sa.Column("profile_cached_at", sa.BigInteger), ) - # Audit logs table (both OAuth and BasicAuth modes) - op.execute( - """ - CREATE TABLE IF NOT EXISTS audit_logs ( - id INTEGER PRIMARY KEY AUTOINCREMENT, - timestamp INTEGER NOT NULL, - event TEXT NOT NULL, - user_id TEXT NOT NULL, - resource_type TEXT, - resource_id TEXT, - auth_method TEXT, - hostname TEXT - ) - """ + op.create_table( + "audit_logs", + sa.Column("id", sa.Integer, primary_key=True, autoincrement=True), + sa.Column("timestamp", sa.BigInteger, nullable=False), + sa.Column("event", sa.Text, nullable=False), + sa.Column("user_id", sa.Text, nullable=False), + sa.Column("resource_type", sa.Text), + sa.Column("resource_id", sa.Text), + sa.Column("auth_method", sa.Text), + sa.Column("hostname", sa.Text), + ) + op.create_index("idx_audit_user_timestamp", "audit_logs", ["user_id", "timestamp"]) + + op.create_table( + "oauth_clients", + sa.Column("id", sa.Integer, primary_key=True, autoincrement=False), + sa.Column("client_id", sa.Text, nullable=False, unique=True), + sa.Column("encrypted_client_secret", sa.LargeBinary, nullable=False), + sa.Column("client_id_issued_at", sa.BigInteger, nullable=False), + sa.Column("client_secret_expires_at", sa.BigInteger, nullable=False), + sa.Column("redirect_uris", sa.Text, nullable=False), + sa.Column("encrypted_registration_access_token", sa.LargeBinary), + sa.Column("registration_client_uri", sa.Text), + sa.Column("created_at", sa.BigInteger, nullable=False), + sa.Column("updated_at", sa.BigInteger, nullable=False), ) - # Index on audit logs for efficient queries - op.execute( - """ - CREATE INDEX IF NOT EXISTS idx_audit_user_timestamp - ON audit_logs(user_id, timestamp) - """ + op.create_table( + "oauth_sessions", + sa.Column("session_id", sa.Text, primary_key=True), + sa.Column("client_id", sa.Text), + sa.Column("client_redirect_uri", sa.Text, nullable=False), + sa.Column("state", sa.Text), + sa.Column("code_challenge", sa.Text), + sa.Column("code_challenge_method", sa.Text), + sa.Column("mcp_authorization_code", sa.Text, unique=True), + sa.Column("idp_access_token", sa.Text), + sa.Column("idp_refresh_token", sa.Text), + sa.Column("user_id", sa.Text), + sa.Column("created_at", sa.BigInteger, nullable=False), + sa.Column("expires_at", sa.BigInteger, nullable=False), + # ADR-004 Progressive Consent fields + sa.Column("flow_type", sa.Text, server_default="hybrid"), + sa.Column("requested_scopes", sa.Text), + sa.Column("granted_scopes", sa.Text), + sa.Column("is_provisioning", sa.Boolean, server_default=sa.false()), + ) + op.create_index( + "idx_oauth_sessions_mcp_code", + "oauth_sessions", + ["mcp_authorization_code"], ) - # OAuth client credentials storage (OAuth mode only) - op.execute( - """ - CREATE TABLE IF NOT EXISTS oauth_clients ( - id INTEGER PRIMARY KEY, - client_id TEXT UNIQUE NOT NULL, - encrypted_client_secret BLOB NOT NULL, - client_id_issued_at INTEGER NOT NULL, - client_secret_expires_at INTEGER NOT NULL, - redirect_uris TEXT NOT NULL, - encrypted_registration_access_token BLOB, - registration_client_uri TEXT, - created_at INTEGER NOT NULL, - updated_at INTEGER NOT NULL - ) - """ + # Legacy schema-version table; superseded by alembic_version. Retained + # so pre-Alembic databases that get stamped into the migration chain + # still match the schema fingerprint they had on disk. + op.create_table( + "schema_version", + sa.Column("version", sa.Integer, primary_key=True, autoincrement=False), + sa.Column("applied_at", sa.Float, nullable=False), ) - # OAuth flow sessions (ADR-004 Progressive Consent) - op.execute( - """ - CREATE TABLE IF NOT EXISTS oauth_sessions ( - session_id TEXT PRIMARY KEY, - client_id TEXT, - client_redirect_uri TEXT NOT NULL, - state TEXT, - code_challenge TEXT, - code_challenge_method TEXT, - mcp_authorization_code TEXT UNIQUE, - idp_access_token TEXT, - idp_refresh_token TEXT, - user_id TEXT, - created_at INTEGER NOT NULL, - expires_at INTEGER NOT NULL, - -- ADR-004 Progressive Consent fields - flow_type TEXT DEFAULT 'hybrid', - requested_scopes TEXT, - granted_scopes TEXT, - is_provisioning BOOLEAN DEFAULT FALSE - ) - """ - ) - - # Index for MCP authorization code lookups - op.execute( - """ - CREATE INDEX IF NOT EXISTS idx_oauth_sessions_mcp_code - ON oauth_sessions(mcp_authorization_code) - """ - ) - - # Legacy schema version tracking table - # NOTE: This is deprecated in favor of Alembic's alembic_version table - # Kept for backward compatibility with pre-Alembic databases - op.execute( - """ - CREATE TABLE IF NOT EXISTS schema_version ( - version INTEGER PRIMARY KEY, - applied_at REAL NOT NULL - ) - """ - ) - - # Registered webhooks tracking (both BasicAuth and OAuth modes) - op.execute( - """ - CREATE TABLE IF NOT EXISTS registered_webhooks ( - id INTEGER PRIMARY KEY AUTOINCREMENT, - webhook_id INTEGER NOT NULL UNIQUE, - preset_id TEXT NOT NULL, - created_at REAL NOT NULL - ) - """ - ) - - # Indexes for efficient webhook queries - op.execute( - """ - CREATE INDEX IF NOT EXISTS idx_webhooks_preset - ON registered_webhooks(preset_id) - """ - ) - - op.execute( - """ - CREATE INDEX IF NOT EXISTS idx_webhooks_created - ON registered_webhooks(created_at) - """ + op.create_table( + "registered_webhooks", + sa.Column("id", sa.Integer, primary_key=True, autoincrement=True), + sa.Column("webhook_id", sa.Integer, nullable=False, unique=True), + sa.Column("preset_id", sa.Text, nullable=False), + sa.Column("created_at", sa.Float, nullable=False), ) + op.create_index("idx_webhooks_preset", "registered_webhooks", ["preset_id"]) + op.create_index("idx_webhooks_created", "registered_webhooks", ["created_at"]) def downgrade() -> None: @@ -170,16 +138,13 @@ def downgrade() -> None: Use with extreme caution. """ - # Drop indexes first - op.execute("DROP INDEX IF EXISTS idx_webhooks_created") - op.execute("DROP INDEX IF EXISTS idx_webhooks_preset") - op.execute("DROP INDEX IF EXISTS idx_oauth_sessions_mcp_code") - op.execute("DROP INDEX IF EXISTS idx_audit_user_timestamp") - - # Drop tables - op.execute("DROP TABLE IF EXISTS registered_webhooks") - op.execute("DROP TABLE IF EXISTS schema_version") - op.execute("DROP TABLE IF EXISTS oauth_sessions") - op.execute("DROP TABLE IF EXISTS oauth_clients") - op.execute("DROP TABLE IF EXISTS audit_logs") - op.execute("DROP TABLE IF EXISTS refresh_tokens") + op.drop_index("idx_webhooks_created", table_name="registered_webhooks") + op.drop_index("idx_webhooks_preset", table_name="registered_webhooks") + op.drop_table("registered_webhooks") + op.drop_table("schema_version") + op.drop_index("idx_oauth_sessions_mcp_code", table_name="oauth_sessions") + op.drop_table("oauth_sessions") + op.drop_table("oauth_clients") + op.drop_index("idx_audit_user_timestamp", table_name="audit_logs") + op.drop_table("audit_logs") + op.drop_table("refresh_tokens") diff --git a/nextcloud_mcp_server/alembic/versions/20260113_1200_002_add_app_passwords.py b/nextcloud_mcp_server/alembic/versions/20260113_1200_002_add_app_passwords.py index 9e7b36c0..25aa17f4 100644 --- a/nextcloud_mcp_server/alembic/versions/20260113_1200_002_add_app_passwords.py +++ b/nextcloud_mcp_server/alembic/versions/20260113_1200_002_add_app_passwords.py @@ -10,6 +10,8 @@ Create Date: 2026-01-13 12:00:00.000000 """ +import sqlalchemy as sa + from alembic import op # revision identifiers, used by Alembic. @@ -22,29 +24,19 @@ depends_on = None def upgrade() -> None: """Add app_passwords table for multi-user BasicAuth mode.""" - # App passwords table for multi-user BasicAuth background sync - op.execute( - """ - CREATE TABLE IF NOT EXISTS app_passwords ( - user_id TEXT PRIMARY KEY, - encrypted_password BLOB NOT NULL, - created_at INTEGER NOT NULL, - updated_at INTEGER NOT NULL - ) - """ - ) - - # Index for efficient user lookups - op.execute( - """ - CREATE INDEX IF NOT EXISTS idx_app_passwords_updated - ON app_passwords(updated_at) - """ + op.create_table( + "app_passwords", + sa.Column("user_id", sa.Text, primary_key=True), + sa.Column("encrypted_password", sa.LargeBinary, nullable=False), + # BigInteger to keep unix epochs in range on Postgres (see 001). + sa.Column("created_at", sa.BigInteger, nullable=False), + sa.Column("updated_at", sa.BigInteger, nullable=False), ) + op.create_index("idx_app_passwords_updated", "app_passwords", ["updated_at"]) def downgrade() -> None: """Drop app_passwords table.""" - op.execute("DROP INDEX IF EXISTS idx_app_passwords_updated") - op.execute("DROP TABLE IF EXISTS app_passwords") + op.drop_index("idx_app_passwords_updated", table_name="app_passwords") + op.drop_table("app_passwords") diff --git a/nextcloud_mcp_server/alembic/versions/20260227_1200_003_add_scopes_and_login_flow.py b/nextcloud_mcp_server/alembic/versions/20260227_1200_003_add_scopes_and_login_flow.py index 5f14e678..1f049155 100644 --- a/nextcloud_mcp_server/alembic/versions/20260227_1200_003_add_scopes_and_login_flow.py +++ b/nextcloud_mcp_server/alembic/versions/20260227_1200_003_add_scopes_and_login_flow.py @@ -12,6 +12,8 @@ Create Date: 2026-02-27 12:00:00.000000 """ +import sqlalchemy as sa + from alembic import op # revision identifiers, used by Alembic. @@ -24,72 +26,37 @@ depends_on = None def upgrade() -> None: """Add scopes/username to app_passwords and create login_flow_sessions.""" - # Add scopes column (nullable JSON array, NULL = all scopes allowed) - op.execute( - """ - ALTER TABLE app_passwords ADD COLUMN scopes TEXT - """ - ) + # Nullable scope columns on the existing app_passwords table. + op.add_column("app_passwords", sa.Column("scopes", sa.Text)) + op.add_column("app_passwords", sa.Column("username", sa.Text)) - # Add username column (Nextcloud loginName from Login Flow v2) - op.execute( - """ - ALTER TABLE app_passwords ADD COLUMN username TEXT - """ + op.create_table( + "login_flow_sessions", + sa.Column("user_id", sa.Text, primary_key=True), + sa.Column("encrypted_poll_token", sa.LargeBinary, nullable=False), + sa.Column("poll_endpoint", sa.Text, nullable=False), + sa.Column("requested_scopes", sa.Text), + # BigInteger to keep unix epochs in range on Postgres (see 001). + sa.Column("created_at", sa.BigInteger, nullable=False), + sa.Column("expires_at", sa.BigInteger, nullable=False), ) - - # Login Flow v2 session tracking - op.execute( - """ - CREATE TABLE IF NOT EXISTS login_flow_sessions ( - user_id TEXT PRIMARY KEY, - encrypted_poll_token BLOB NOT NULL, - poll_endpoint TEXT NOT NULL, - requested_scopes TEXT, - created_at INTEGER NOT NULL, - expires_at INTEGER NOT NULL - ) - """ - ) - - # Index for efficient cleanup of expired sessions - op.execute( - """ - CREATE INDEX IF NOT EXISTS idx_login_flow_sessions_expires - ON login_flow_sessions(expires_at) - """ + op.create_index( + "idx_login_flow_sessions_expires", + "login_flow_sessions", + ["expires_at"], ) def downgrade() -> None: - """Drop login_flow_sessions and remove added columns.""" + """Drop login_flow_sessions and remove added columns. - op.execute("DROP INDEX IF EXISTS idx_login_flow_sessions_expires") - op.execute("DROP TABLE IF EXISTS login_flow_sessions") + ``batch_alter_table`` handles SQLite's pre-3.35 lack of ``DROP COLUMN`` + by recreating the table; on Postgres it issues a native ``DROP COLUMN``. + """ - # SQLite doesn't support DROP COLUMN before 3.35.0 - # Recreate app_passwords without the new columns - op.execute( - """ - CREATE TABLE app_passwords_backup ( - user_id TEXT PRIMARY KEY, - encrypted_password BLOB NOT NULL, - created_at INTEGER NOT NULL, - updated_at INTEGER NOT NULL - ) - """ - ) - op.execute( - """ - INSERT INTO app_passwords_backup (user_id, encrypted_password, created_at, updated_at) - SELECT user_id, encrypted_password, created_at, updated_at FROM app_passwords - """ - ) - op.execute("DROP TABLE app_passwords") - op.execute("ALTER TABLE app_passwords_backup RENAME TO app_passwords") - op.execute( - """ - CREATE INDEX IF NOT EXISTS idx_app_passwords_updated - ON app_passwords(updated_at) - """ - ) + op.drop_index("idx_login_flow_sessions_expires", table_name="login_flow_sessions") + op.drop_table("login_flow_sessions") + + with op.batch_alter_table("app_passwords") as batch_op: + batch_op.drop_column("username") + batch_op.drop_column("scopes") diff --git a/nextcloud_mcp_server/alembic/versions/20260502_1500_005_add_browser_sessions.py b/nextcloud_mcp_server/alembic/versions/20260502_1500_005_add_browser_sessions.py index b85f20d1..a8aaea7d 100644 --- a/nextcloud_mcp_server/alembic/versions/20260502_1500_005_add_browser_sessions.py +++ b/nextcloud_mcp_server/alembic/versions/20260502_1500_005_add_browser_sessions.py @@ -10,6 +10,8 @@ Revises: 004 Create Date: 2026-05-02 15:00:00.000000 """ +import sqlalchemy as sa + from alembic import op revision = "005" @@ -19,31 +21,19 @@ depends_on = None def upgrade() -> None: - op.execute( - """ - CREATE TABLE IF NOT EXISTS browser_sessions ( - session_id TEXT PRIMARY KEY, - user_id TEXT NOT NULL, - created_at INTEGER NOT NULL, - expires_at INTEGER NOT NULL - ) - """ - ) - op.execute( - """ - CREATE INDEX IF NOT EXISTS idx_browser_sessions_user - ON browser_sessions(user_id) - """ - ) - op.execute( - """ - CREATE INDEX IF NOT EXISTS idx_browser_sessions_expires - ON browser_sessions(expires_at) - """ + op.create_table( + "browser_sessions", + sa.Column("session_id", sa.Text, primary_key=True), + sa.Column("user_id", sa.Text, nullable=False), + # BigInteger to keep unix epochs in range on Postgres (see 001). + sa.Column("created_at", sa.BigInteger, nullable=False), + sa.Column("expires_at", sa.BigInteger, nullable=False), ) + op.create_index("idx_browser_sessions_user", "browser_sessions", ["user_id"]) + op.create_index("idx_browser_sessions_expires", "browser_sessions", ["expires_at"]) def downgrade() -> None: - op.execute("DROP INDEX IF EXISTS idx_browser_sessions_expires") - op.execute("DROP INDEX IF EXISTS idx_browser_sessions_user") - op.execute("DROP TABLE IF EXISTS browser_sessions") + op.drop_index("idx_browser_sessions_expires", table_name="browser_sessions") + op.drop_index("idx_browser_sessions_user", table_name="browser_sessions") + op.drop_table("browser_sessions") diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index 4090aa25..c31d9efe 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -31,22 +31,232 @@ import os import socket import sqlite3 import time +from contextlib import asynccontextmanager from pathlib import Path from typing import Any -import aiosqlite import anyio import httpx +import sqlalchemy as sa from anyio import to_thread from cryptography.fernet import Fernet +from sqlalchemy.ext.asyncio import AsyncConnection, AsyncEngine, create_async_engine +from sqlalchemy.pool import NullPool -from nextcloud_mcp_server.config import get_token_db_path, is_ephemeral_token_db +from nextcloud_mcp_server.config import ( + get_database_url, + is_ephemeral_token_db, + is_sqlite_url, +) from nextcloud_mcp_server.migrations import stamp_database, upgrade_database from nextcloud_mcp_server.observability.metrics import record_db_operation logger = logging.getLogger(__name__) +def _qmark_to_named(sql: str) -> tuple[str, list[str]]: + """Rewrite ``?`` positional placeholders to ``:p0, :p1, ...`` named binds. + + SQLAlchemy's :func:`text` only supports named bind parameters, so the + aiosqlite-style call sites (which use ``?``) are translated as they + cross the shim. The rewriter preserves ``?`` characters inside SQL + string literals; comments are not currently respected but the storage + layer doesn't put ``?`` inside comments. + """ + out: list[str] = [] + names: list[str] = [] + i = 0 + in_str = False + quote = "" + n = 0 + while i < len(sql): + ch = sql[i] + if in_str: + out.append(ch) + if ch == quote: + # SQL string escapes ('' or "") — stay in string mode. + if i + 1 < len(sql) and sql[i + 1] == quote: + out.append(quote) + i += 2 + continue + in_str = False + i += 1 + continue + if ch in ("'", '"'): + in_str = True + quote = ch + out.append(ch) + i += 1 + continue + if ch == "?": + name = f"p{n}" + out.append(f":{name}") + names.append(name) + n += 1 + i += 1 + continue + out.append(ch) + i += 1 + return "".join(out), names + + +class _Row: + """Hybrid tuple/dict row mirroring ``aiosqlite.Row`` semantics. + + The legacy SQLite-direct call sites use a mix of access patterns: + positional unpacking (``a, b, c = row``), indexed access (``row[0]``), + and dict-like access (``row["col"]``, ``dict(row)``) when + ``db.row_factory = aiosqlite.Row`` is set. To avoid touching every call + site, every row returned by :class:`_Cursor` is wrapped in this hybrid + object so all three patterns keep working. + """ + + __slots__ = ("_values", "_mapping") + + def __init__(self, values: tuple, mapping: dict) -> None: + self._values = values + self._mapping = mapping + + def __getitem__(self, key): + if isinstance(key, (int, slice)): + return self._values[key] + return self._mapping[key] + + def __iter__(self): + return iter(self._values) + + def __len__(self) -> int: + return len(self._values) + + def keys(self): + return self._mapping.keys() + + def values(self): + return self._mapping.values() + + def items(self): + return self._mapping.items() + + +def _wrap_row(row) -> _Row | None: + if row is None: + return None + return _Row(tuple(row), dict(row._mapping)) + + +def _wrap_rows(rows) -> list[_Row]: + """Wrap a list of SQLAlchemy rows; iterator never yields ``None``.""" + return [_Row(tuple(r), dict(r._mapping)) for r in rows] + + +class _Cursor: + """aiosqlite-compatible cursor view over a SQLAlchemy CursorResult. + + Existing storage methods iterate cursors via ``async with db.execute(...) + as cursor: row = await cursor.fetchone()``. SQLAlchemy returns a + synchronous :class:`Result` from an async ``execute``; this shim adds the + async context-manager / async-fetch surface so call sites are unchanged. + + ``rowcount`` is captured eagerly at construction time. ``lastrowid`` is + intentionally NOT exposed: accessing ``CursorResult.lastrowid`` on the + asyncpg dialect consumes the result buffer, which would silently turn + every subsequent ``fetchall()`` into an empty list (a real bug hit + during the Postgres port). + """ + + __slots__ = ("_result", "rowcount") + + def __init__(self, result: sa.CursorResult) -> None: + self._result = result + # ``rowcount`` is -1 for SELECTs in SQLAlchemy; existing code only + # reads it after writes (DELETE/UPDATE) where it is accurate. + self.rowcount = result.rowcount + + async def fetchone(self) -> _Row | None: + return _wrap_row(self._result.fetchone()) + + async def fetchall(self) -> list[_Row]: + return _wrap_rows(self._result.fetchall()) + + async def __aenter__(self) -> "_Cursor": + return self + + async def __aexit__(self, *exc: object) -> None: + # SQLAlchemy Result closes when the connection closes; no-op here. + return None + + +class _ExecuteCtx: + """Hybrid awaitable + async context manager for ``db.execute(...)``. + + Aiosqlite call sites use both forms interchangeably:: + + cursor = await db.execute(sql, params) + async with db.execute(sql, params) as cursor: ... + + so the return value must be awaitable (resolves to a cursor) AND a + one-shot async context manager (executes on ``__aenter__`` and returns + the cursor). This wrapper provides both surfaces without executing the + SQL twice — the cursor is cached after the first resolution. + """ + + __slots__ = ("_conn", "_sql", "_params", "_cursor") + + def __init__(self, conn: AsyncConnection, sql: str, params: tuple | list) -> None: + self._conn = conn + self._sql = sql + self._params = params + self._cursor: _Cursor | None = None + + async def _resolve(self) -> _Cursor: + if self._cursor is not None: + return self._cursor + text_sql, names = _qmark_to_named(self._sql) + if len(names) != len(self._params): + raise ValueError( + f"Placeholder count mismatch: SQL has {len(names)} '?' " + f"but got {len(self._params)} params" + ) + bind = dict(zip(names, self._params, strict=True)) + result = await self._conn.execute(sa.text(text_sql), bind) + self._cursor = _Cursor(result) + return self._cursor + + def __await__(self): + return self._resolve().__await__() + + async def __aenter__(self) -> _Cursor: + return await self._resolve() + + async def __aexit__(self, *exc: object) -> None: + return None + + +class _DBConn: + """aiosqlite-compatible wrapper around a SQLAlchemy AsyncConnection. + + Provides ``execute`` (with ``?`` placeholders, returning a hybrid + awaitable/context-manager :class:`_ExecuteCtx`) and ``commit`` so the + existing storage method bodies need no churn beyond swapping the + connection context-manager. Wraps results in :class:`_Cursor` for the + fetchone/fetchall/rowcount surface the call sites already use. + + ``row_factory`` is accepted as a setter for source compatibility with + aiosqlite call sites but is ignored: every row is already wrapped in + :class:`_Row` so dict-style access works unconditionally. + """ + + def __init__(self, conn: AsyncConnection) -> None: + self._conn = conn + self.row_factory = None # set by aiosqlite-shaped call sites; ignored + + def execute(self, sql: str, params: tuple | list = ()) -> _ExecuteCtx: + return _ExecuteCtx(self._conn, sql, params) + + async def commit(self) -> None: + await self._conn.commit() + + class RefreshTokenStorage: """Persistent storage for MCP server state (tokens, webhooks, and future features). @@ -65,17 +275,41 @@ class RefreshTokenStorage: Token-related operations require TOKEN_ENCRYPTION_KEY, but webhook operations do not. """ - def __init__(self, db_path: str, encryption_key: bytes | None = None): + def __init__( + self, + database_url: str | None = None, + encryption_key: bytes | None = None, + *, + db_path: str | None = None, + ): """ Initialize persistent storage. Args: - db_path: Path to SQLite database file + database_url: SQLAlchemy URL (``sqlite+aiosqlite:///...`` or + ``postgresql+asyncpg://...``). When omitted, falls back to + :func:`get_database_url` (honors ``DATABASE_URL`` env, then + ``TOKEN_STORAGE_DB``). encryption_key: Optional Fernet encryption key (32 bytes, base64-encoded). - Required for token storage operations, not required for webhook tracking. + Required for token storage operations, not required for webhook tracking. + db_path: Deprecated SQLite-only constructor argument retained for + tests that pass a tempfile path. Internally converted to + ``sqlite+aiosqlite:///{db_path}``. """ - self.db_path = db_path + if database_url is None and db_path is not None: + database_url = f"sqlite+aiosqlite:///{db_path}" + if database_url is None: + database_url = get_database_url() + self.database_url = database_url + # Legacy attribute retained for sqlite-only code paths (file perms, + # ephemeral tempfile detection, log messages). Empty string for + # non-sqlite URLs so accidental file ops fail loudly. + self.db_path = ( + database_url.split("///", 1)[1] if is_sqlite_url(database_url) else "" + ) self.cipher = Fernet(encryption_key) if encryption_key else None + self.engine: AsyncEngine | None = None + self._dialect: str = "unknown" self._initialized = False @classmethod @@ -84,10 +318,14 @@ class RefreshTokenStorage: Create storage instance from environment variables. Environment variables: - TOKEN_STORAGE_DB: Path to database file. If unset, a per-process - tempfile is allocated and deleted at interpreter exit — - tokens are ephemeral and wiped on restart. Set this to a - filesystem path to persist tokens across restarts. + DATABASE_URL: SQLAlchemy URL for any supported backend. Wins + over ``TOKEN_STORAGE_DB`` when set. Use + ``postgresql+asyncpg://user:pw@host/db`` for HA k8s + deployments. See ADR-026. + TOKEN_STORAGE_DB: Legacy SQLite-only path. If unset and + ``DATABASE_URL`` is also unset, a per-process tempfile is + allocated and deleted at interpreter exit — tokens are + ephemeral and wiped on restart. TOKEN_ENCRYPTION_KEY: Optional base64-encoded Fernet key (required for token storage) Returns: @@ -97,13 +335,17 @@ class RefreshTokenStorage: If TOKEN_ENCRYPTION_KEY is not set, token storage operations will fail, but webhook tracking will still work. """ - db_path = get_token_db_path() - if is_ephemeral_token_db(db_path): - logger.info( - "Using ephemeral token storage at %s " - "(set TOKEN_STORAGE_DB to persist tokens across restarts)", - db_path, - ) + database_url = get_database_url() + if is_sqlite_url(database_url): + sqlite_path = database_url.split("///", 1)[1] + if is_ephemeral_token_db(sqlite_path): + logger.info( + "Using ephemeral token storage at %s " + "(set DATABASE_URL or TOKEN_STORAGE_DB to persist tokens across restarts)", + sqlite_path, + ) + else: + logger.info("Using centralized token storage at %s", database_url) encryption_key_b64 = os.getenv("TOKEN_ENCRYPTION_KEY") encryption_key = None @@ -130,7 +372,7 @@ class RefreshTokenStorage: "but webhook tracking will still work" ) - return cls(db_path=db_path, encryption_key=encryption_key) + return cls(database_url=database_url, encryption_key=encryption_key) async def initialize(self) -> None: """ @@ -151,7 +393,9 @@ class RefreshTokenStorage: if self._initialized: return - if sqlite3.sqlite_version_info < (3, 35): + is_sqlite = is_sqlite_url(self.database_url) + + if is_sqlite and sqlite3.sqlite_version_info < (3, 35): raise RuntimeError( "SQLite >= 3.35 is required (DELETE ... RETURNING is used " "by delete_browser_session); detected " @@ -159,63 +403,85 @@ class RefreshTokenStorage: "image with a newer bundled libsqlite3." ) - # Ensure directory exists - db_dir = Path(self.db_path).parent - db_dir.mkdir(parents=True, exist_ok=True) + if is_sqlite: + # File-permission hardening + parent dir creation is sqlite-only; + # centralized backends manage their own filesystem. + db_dir = Path(self.db_path).parent + db_dir.mkdir(parents=True, exist_ok=True) + if Path(self.db_path).exists(): + os.chmod(self.db_path, 0o600) - # Set restrictive permissions on database file if it exists - if Path(self.db_path).exists(): - os.chmod(self.db_path, 0o600) - - # Check database state and run appropriate migration strategy - async with aiosqlite.connect(self.db_path) as db: - # Check if database is managed by Alembic - cursor = await db.execute( - "SELECT name FROM sqlite_master WHERE type='table' AND name='alembic_version'" + # Create the shared async engine for the chosen backend. SQLite uses + # NullPool (per-call connections, matches the prior aiosqlite-direct + # behavior); Postgres uses the default pool with pre-ping so dropped + # connections from idle k8s networks are retried transparently. + if is_sqlite: + self.engine = create_async_engine( + self.database_url, + poolclass=NullPool, + connect_args={"check_same_thread": False}, + future=True, ) - has_alembic = await cursor.fetchone() is not None + else: + self.engine = create_async_engine( + self.database_url, + pool_size=10, + max_overflow=20, + pool_pre_ping=True, + future=True, + ) + self._dialect = self.engine.dialect.name - if not has_alembic: - # Check if this is a pre-Alembic database with existing schema - cursor = await db.execute( - "SELECT name FROM sqlite_master WHERE type='table' AND name='refresh_tokens'" - ) - has_schema = await cursor.fetchone() is not None + # Check database state with the SQLAlchemy inspector so the legacy + # ``sqlite_master`` lookup works against either backend. + def _inspect(sync_conn: sa.Connection) -> tuple[bool, bool]: + insp = sa.inspect(sync_conn) + tables = set(insp.get_table_names()) + return ("alembic_version" in tables), ("refresh_tokens" in tables) - if has_schema: - logger.info( - "Detected pre-Alembic database at %s, stamping with initial revision", - self.db_path, - ) - else: - logger.info( - "Initializing new database at %s with migrations", self.db_path - ) + async with self.engine.connect() as conn: + has_alembic, has_schema = await conn.run_sync(_inspect) - # Run migrations in a worker thread using anyio.to_thread - # This allows Alembic to run its own async operations in a separate context if not has_alembic: if has_schema: - # Stamp existing database without running migrations - await to_thread.run_sync(stamp_database, self.db_path, "001") + logger.info( + "Detected pre-Alembic database at %s, stamping with initial revision", + self.database_url, + ) + await to_thread.run_sync(stamp_database, self.database_url, "001") logger.info( "Pre-Alembic database stamped successfully. " "Future schema changes will use migrations." ) else: - # New database - run migrations - await to_thread.run_sync(upgrade_database, self.db_path, "head") + logger.info( + "Initializing new database at %s with migrations", + self.database_url, + ) + await to_thread.run_sync(upgrade_database, self.database_url, "head") logger.info("Database initialized with migrations") else: - # Alembic-managed database - upgrade to latest - await to_thread.run_sync(upgrade_database, self.db_path, "head") + await to_thread.run_sync(upgrade_database, self.database_url, "head") logger.info("Database upgraded to latest version") - # Set restrictive permissions after initialization - os.chmod(self.db_path, 0o600) + if is_sqlite: + os.chmod(self.db_path, 0o600) self._initialized = True - logger.info("Initialized refresh token storage at %s", self.db_path) + logger.info("Initialized refresh token storage at %s", self.database_url) + + @asynccontextmanager + async def _db(self): + """Open a backend-agnostic connection. + + Yields a :class:`_DBConn` that mimics aiosqlite's API (``execute`` with + ``?`` placeholders, ``commit``, cursor with ``fetchone`` / + ``fetchall`` / ``rowcount``) so the existing storage method bodies + work against either SQLite or Postgres without per-call rewrites. + """ + assert self.engine is not None, "RefreshTokenStorage.initialize() not called" + async with self.engine.connect() as conn: + yield _DBConn(conn) async def store_refresh_token( self, @@ -260,20 +526,30 @@ class RefreshTokenStorage: start_time = time.time() try: - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: + # ON CONFLICT DO UPDATE preserves ``created_at`` (it's not + # listed in the update clause) so the original + # COALESCE(...)-based INSERT OR REPLACE semantics are kept. await db.execute( """ - INSERT OR REPLACE INTO refresh_tokens + INSERT INTO refresh_tokens (user_id, encrypted_token, expires_at, created_at, updated_at, flow_type, token_audience, provisioned_at, provisioning_client_id, scopes) - VALUES (?, ?, ?, COALESCE((SELECT created_at FROM refresh_tokens WHERE user_id = ?), ?), ?, - ?, ?, ?, ?, ?) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + ON CONFLICT (user_id) DO UPDATE SET + encrypted_token = EXCLUDED.encrypted_token, + expires_at = EXCLUDED.expires_at, + updated_at = EXCLUDED.updated_at, + flow_type = EXCLUDED.flow_type, + token_audience = EXCLUDED.token_audience, + provisioned_at = EXCLUDED.provisioned_at, + provisioning_client_id = EXCLUDED.provisioning_client_id, + scopes = EXCLUDED.scopes """, ( user_id, encrypted_token, expires_at, - user_id, now, now, flow_type, @@ -285,7 +561,7 @@ class RefreshTokenStorage: ) await db.commit() duration = time.time() - start_time - record_db_operation("sqlite", "insert", duration, "success") + record_db_operation(self._dialect, "insert", duration, "success") logger.info( f"Stored refresh token for user {user_id}" @@ -293,7 +569,7 @@ class RefreshTokenStorage: ) except Exception: duration = time.time() - start_time - record_db_operation("sqlite", "insert", duration, "error") + record_db_operation(self._dialect, "insert", duration, "error") raise # Audit log @@ -322,7 +598,7 @@ class RefreshTokenStorage: profile_json = json.dumps(profile_data) now = int(time.time()) - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: await db.execute( """ UPDATE refresh_tokens @@ -351,7 +627,7 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: async with db.execute( """ SELECT user_profile, profile_cached_at @@ -407,7 +683,7 @@ class RefreshTokenStorage: start_time = time.time() try: - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: async with db.execute( """ SELECT encrypted_token, expires_at, flow_type, token_audience, @@ -421,7 +697,7 @@ class RefreshTokenStorage: if not row: logger.debug("No refresh token found for user %s", user_id) duration = time.time() - start_time - record_db_operation("sqlite", "select", duration, "success") + record_db_operation(self._dialect, "select", duration, "success") return None ( @@ -443,7 +719,7 @@ class RefreshTokenStorage: ) await self.delete_refresh_token(user_id) duration = time.time() - start_time - record_db_operation("sqlite", "select", duration, "success") + record_db_operation(self._dialect, "select", duration, "success") return None decrypted_token = self.cipher.decrypt(encrypted_token).decode() @@ -456,7 +732,7 @@ class RefreshTokenStorage: ) duration = time.time() - start_time - record_db_operation("sqlite", "select", duration, "success") + record_db_operation(self._dialect, "select", duration, "success") return { "refresh_token": decrypted_token, @@ -470,7 +746,7 @@ class RefreshTokenStorage: } except Exception as e: duration = time.time() - start_time - record_db_operation("sqlite", "select", duration, "error") + record_db_operation(self._dialect, "select", duration, "error") logger.error("Failed to decrypt refresh token for user %s: %s", user_id, e) return None @@ -502,7 +778,7 @@ class RefreshTokenStorage: "TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable" ) - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: async with db.execute( """ SELECT user_id, encrypted_token, expires_at, flow_type, token_audience, @@ -582,7 +858,7 @@ class RefreshTokenStorage: start_time = time.time() try: - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: cursor = await db.execute( "DELETE FROM refresh_tokens WHERE user_id = ?", (user_id,), @@ -591,7 +867,7 @@ class RefreshTokenStorage: deleted = cursor.rowcount > 0 duration = time.time() - start_time - record_db_operation("sqlite", "delete", duration, "success") + record_db_operation(self._dialect, "delete", duration, "success") if deleted: logger.info("Deleted refresh token for user %s", user_id) @@ -606,7 +882,7 @@ class RefreshTokenStorage: return deleted except Exception: duration = time.time() - start_time - record_db_operation("sqlite", "delete", duration, "error") + record_db_operation(self._dialect, "delete", duration, "error") raise async def get_all_user_ids(self) -> list[str]: @@ -619,7 +895,7 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: async with db.execute( "SELECT user_id FROM refresh_tokens ORDER BY updated_at DESC" ) as cursor: @@ -641,7 +917,7 @@ class RefreshTokenStorage: now = int(time.time()) - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: cursor = await db.execute( "DELETE FROM refresh_tokens WHERE expires_at IS NOT NULL AND expires_at < ?", (now,), @@ -700,18 +976,25 @@ class RefreshTokenStorage: redirect_uris_json = json.dumps(redirect_uris) now = int(time.time()) - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: + # Singleton row pinned at id=1; ON CONFLICT preserves the + # original ``created_at`` because it's omitted from the update. await db.execute( """ - INSERT OR REPLACE INTO oauth_clients + INSERT INTO oauth_clients (id, client_id, encrypted_client_secret, client_id_issued_at, client_secret_expires_at, redirect_uris, encrypted_registration_access_token, registration_client_uri, created_at, updated_at) - VALUES ( - 1, ?, ?, ?, ?, ?, ?, ?, - COALESCE((SELECT created_at FROM oauth_clients WHERE id = 1), ?), - ? - ) + VALUES (1, ?, ?, ?, ?, ?, ?, ?, ?, ?) + ON CONFLICT (id) DO UPDATE SET + client_id = EXCLUDED.client_id, + encrypted_client_secret = EXCLUDED.encrypted_client_secret, + client_id_issued_at = EXCLUDED.client_id_issued_at, + client_secret_expires_at = EXCLUDED.client_secret_expires_at, + redirect_uris = EXCLUDED.redirect_uris, + encrypted_registration_access_token = EXCLUDED.encrypted_registration_access_token, + registration_client_uri = EXCLUDED.registration_client_uri, + updated_at = EXCLUDED.updated_at """, ( client_id, @@ -768,7 +1051,7 @@ class RefreshTokenStorage: "TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable" ) - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: async with db.execute( """ SELECT client_id, encrypted_client_secret, client_id_issued_at, @@ -841,7 +1124,7 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: cursor = await db.execute("DELETE FROM oauth_clients WHERE id = 1") await db.commit() deleted = cursor.rowcount > 0 @@ -868,7 +1151,7 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: async with db.execute( "SELECT client_secret_expires_at FROM oauth_clients WHERE id = 1" ) as cursor: @@ -902,7 +1185,7 @@ class RefreshTokenStorage: hostname = socket.gethostname() timestamp = int(time.time()) - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: await db.execute( """ INSERT INTO audit_logs @@ -955,8 +1238,7 @@ class RefreshTokenStorage: query += " ORDER BY timestamp DESC LIMIT ?" params.append(limit) - async with aiosqlite.connect(self.db_path) as db: - db.row_factory = aiosqlite.Row + async with self._db() as db: async with db.execute(query, params) as cursor: rows = await cursor.fetchall() @@ -1001,7 +1283,7 @@ class RefreshTokenStorage: now = int(time.time()) expires_at = now + ttl_seconds - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: await db.execute( """ INSERT INTO oauth_sessions @@ -1042,8 +1324,7 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - async with aiosqlite.connect(self.db_path) as db: - db.row_factory = aiosqlite.Row + async with self._db() as db: async with db.execute( "SELECT * FROM oauth_sessions WHERE session_id = ?", (session_id,) ) as cursor: @@ -1074,8 +1355,7 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - async with aiosqlite.connect(self.db_path) as db: - db.row_factory = aiosqlite.Row + async with self._db() as db: async with db.execute( "SELECT * FROM oauth_sessions WHERE mcp_authorization_code = ?", (mcp_authorization_code,), @@ -1134,7 +1414,7 @@ class RefreshTokenStorage: params.append(session_id) - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: cursor = await db.execute( f""" UPDATE oauth_sessions @@ -1161,7 +1441,7 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: cursor = await db.execute( "DELETE FROM oauth_sessions WHERE session_id = ?", (session_id,) ) @@ -1185,7 +1465,7 @@ class RefreshTokenStorage: now = int(time.time()) - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: cursor = await db.execute( "DELETE FROM oauth_sessions WHERE expires_at < ?", (now,) ) @@ -1220,12 +1500,16 @@ class RefreshTokenStorage: now = int(time.time()) expires_at = now + ttl_seconds - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: await db.execute( """ - INSERT OR REPLACE INTO browser_sessions + INSERT INTO browser_sessions (session_id, user_id, created_at, expires_at) VALUES (?, ?, ?, ?) + ON CONFLICT (session_id) DO UPDATE SET + user_id = EXCLUDED.user_id, + created_at = EXCLUDED.created_at, + expires_at = EXCLUDED.expires_at """, (session_id, user_id, now, expires_at), ) @@ -1257,8 +1541,7 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - async with aiosqlite.connect(self.db_path) as db: - db.row_factory = aiosqlite.Row + async with self._db() as db: async with db.execute( "SELECT user_id, expires_at FROM browser_sessions WHERE session_id = ?", (session_id,), @@ -1285,7 +1568,7 @@ class RefreshTokenStorage: # concurrent delete that empties the row between SELECT and DELETE # (PR #758 round-3 review). user_id: str | None = None - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: async with db.execute( "DELETE FROM browser_sessions WHERE session_id = ? RETURNING user_id", (session_id,), @@ -1319,7 +1602,7 @@ class RefreshTokenStorage: now = int(time.time()) - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: cursor = await db.execute( "DELETE FROM browser_sessions WHERE expires_at < ?", (now,) ) @@ -1346,9 +1629,15 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: await db.execute( - "INSERT OR REPLACE INTO registered_webhooks (webhook_id, preset_id, created_at) VALUES (?, ?, ?)", + """ + INSERT INTO registered_webhooks (webhook_id, preset_id, created_at) + VALUES (?, ?, ?) + ON CONFLICT (webhook_id) DO UPDATE SET + preset_id = EXCLUDED.preset_id, + created_at = EXCLUDED.created_at + """, (webhook_id, preset_id, time.time()), ) await db.commit() @@ -1368,7 +1657,7 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: cursor = await db.execute( "SELECT webhook_id FROM registered_webhooks WHERE preset_id = ?", (preset_id,), @@ -1390,7 +1679,7 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: cursor = await db.execute( "DELETE FROM registered_webhooks WHERE webhook_id = ?", (webhook_id,) ) @@ -1412,7 +1701,7 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: cursor = await db.execute( "SELECT webhook_id, preset_id, created_at FROM registered_webhooks ORDER BY created_at DESC" ) @@ -1436,7 +1725,7 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: cursor = await db.execute( "DELETE FROM registered_webhooks WHERE preset_id = ?", (preset_id,) ) @@ -1478,29 +1767,27 @@ class RefreshTokenStorage: start_time = time.time() try: - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: await db.execute( """ - INSERT OR REPLACE INTO app_passwords + INSERT INTO app_passwords (user_id, encrypted_password, created_at, updated_at) - VALUES ( - ?, - ?, - COALESCE((SELECT created_at FROM app_passwords WHERE user_id = ?), ?), - ? - ) + VALUES (?, ?, ?, ?) + ON CONFLICT (user_id) DO UPDATE SET + encrypted_password = EXCLUDED.encrypted_password, + updated_at = EXCLUDED.updated_at """, - (user_id, encrypted_password, user_id, now, now), + (user_id, encrypted_password, now, now), ) await db.commit() duration = time.time() - start_time - record_db_operation("sqlite", "insert", duration, "success") + record_db_operation(self._dialect, "insert", duration, "success") logger.info("Stored app password for user %s", user_id) except Exception: duration = time.time() - start_time - record_db_operation("sqlite", "insert", duration, "error") + record_db_operation(self._dialect, "insert", duration, "error") raise # Audit log @@ -1531,7 +1818,7 @@ class RefreshTokenStorage: start_time = time.time() try: - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: async with db.execute( "SELECT encrypted_password FROM app_passwords WHERE user_id = ?", (user_id,), @@ -1541,21 +1828,21 @@ class RefreshTokenStorage: if not row: logger.debug("No app password found for user %s", user_id) duration = time.time() - start_time - record_db_operation("sqlite", "select", duration, "success") + record_db_operation(self._dialect, "select", duration, "success") return None encrypted_password = row[0] decrypted_password = self.cipher.decrypt(encrypted_password).decode() duration = time.time() - start_time - record_db_operation("sqlite", "select", duration, "success") + record_db_operation(self._dialect, "select", duration, "success") logger.debug("Retrieved app password for user %s", user_id) return decrypted_password except Exception as e: duration = time.time() - start_time - record_db_operation("sqlite", "select", duration, "error") + record_db_operation(self._dialect, "select", duration, "error") logger.error("Failed to decrypt app password for user %s: %s", user_id, e) return None @@ -1574,7 +1861,7 @@ class RefreshTokenStorage: start_time = time.time() try: - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: cursor = await db.execute( "DELETE FROM app_passwords WHERE user_id = ?", (user_id,), @@ -1583,7 +1870,7 @@ class RefreshTokenStorage: deleted = cursor.rowcount > 0 duration = time.time() - start_time - record_db_operation("sqlite", "delete", duration, "success") + record_db_operation(self._dialect, "delete", duration, "success") if deleted: logger.info("Deleted app password for user %s", user_id) @@ -1599,7 +1886,7 @@ class RefreshTokenStorage: except Exception: duration = time.time() - start_time - record_db_operation("sqlite", "delete", duration, "error") + record_db_operation(self._dialect, "delete", duration, "error") raise async def get_all_app_password_user_ids(self) -> list[str]: @@ -1612,7 +1899,7 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: async with db.execute( "SELECT user_id FROM app_passwords ORDER BY updated_at DESC" ) as cursor: @@ -1732,24 +2019,21 @@ class RefreshTokenStorage: start_time = time.time() try: - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: await db.execute( """ - INSERT OR REPLACE INTO app_passwords + INSERT INTO app_passwords (user_id, encrypted_password, created_at, updated_at, scopes, username) - VALUES ( - ?, - ?, - COALESCE((SELECT created_at FROM app_passwords WHERE user_id = ?), ?), - ?, - ?, - ? - ) + VALUES (?, ?, ?, ?, ?, ?) + ON CONFLICT (user_id) DO UPDATE SET + encrypted_password = EXCLUDED.encrypted_password, + updated_at = EXCLUDED.updated_at, + scopes = EXCLUDED.scopes, + username = EXCLUDED.username """, ( user_id, encrypted_password, - user_id, now, now, scopes_json, @@ -1759,7 +2043,7 @@ class RefreshTokenStorage: await db.commit() duration = time.time() - start_time - record_db_operation("sqlite", "insert", duration, "success") + record_db_operation(self._dialect, "insert", duration, "success") logger.info( "Stored scoped app password for user %s (scopes=%s, username=%s)", user_id, @@ -1769,7 +2053,7 @@ class RefreshTokenStorage: except Exception: duration = time.time() - start_time - record_db_operation("sqlite", "insert", duration, "error") + record_db_operation(self._dialect, "insert", duration, "error") raise await self._audit_log( @@ -1799,7 +2083,7 @@ class RefreshTokenStorage: start_time = time.time() try: - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: async with db.execute( """ SELECT encrypted_password, scopes, username, created_at, updated_at @@ -1812,7 +2096,7 @@ class RefreshTokenStorage: if not row: logger.debug("No app password found for user %s", user_id) duration = time.time() - start_time - record_db_operation("sqlite", "select", duration, "success") + record_db_operation(self._dialect, "select", duration, "success") return None encrypted_password, scopes_json, username, created_at, updated_at = row @@ -1820,7 +2104,7 @@ class RefreshTokenStorage: scopes = json.loads(scopes_json) if scopes_json else None duration = time.time() - start_time - record_db_operation("sqlite", "select", duration, "success") + record_db_operation(self._dialect, "select", duration, "success") return { "app_password": decrypted_password, @@ -1832,7 +2116,7 @@ class RefreshTokenStorage: except Exception: duration = time.time() - start_time - record_db_operation("sqlite", "select", duration, "error") + record_db_operation(self._dialect, "select", duration, "error") raise async def update_app_password_scopes(self, user_id: str, scopes: list[str]) -> bool: @@ -1852,7 +2136,7 @@ class RefreshTokenStorage: now = int(time.time()) start_time = time.time() try: - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: cursor = await db.execute( "UPDATE app_passwords SET scopes = ?, updated_at = ? WHERE user_id = ?", (scopes_json, now, user_id), @@ -1861,7 +2145,7 @@ class RefreshTokenStorage: updated = cursor.rowcount > 0 duration = time.time() - start_time - record_db_operation("sqlite", "update", duration, "success") + record_db_operation(self._dialect, "update", duration, "success") if updated: await self._audit_log( @@ -1874,7 +2158,7 @@ class RefreshTokenStorage: except Exception: duration = time.time() - start_time - record_db_operation("sqlite", "update", duration, "error") + record_db_operation(self._dialect, "update", duration, "error") raise # ── Login Flow v2: Session Tracking ────────────────────────────────── @@ -1913,13 +2197,19 @@ class RefreshTokenStorage: start_time = time.time() try: - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: await db.execute( """ - INSERT OR REPLACE INTO login_flow_sessions + INSERT INTO login_flow_sessions (user_id, encrypted_poll_token, poll_endpoint, requested_scopes, created_at, expires_at) VALUES (?, ?, ?, ?, ?, ?) + ON CONFLICT (user_id) DO UPDATE SET + encrypted_poll_token = EXCLUDED.encrypted_poll_token, + poll_endpoint = EXCLUDED.poll_endpoint, + requested_scopes = EXCLUDED.requested_scopes, + created_at = EXCLUDED.created_at, + expires_at = EXCLUDED.expires_at """, ( user_id, @@ -1933,12 +2223,12 @@ class RefreshTokenStorage: await db.commit() duration = time.time() - start_time - record_db_operation("sqlite", "insert", duration, "success") + record_db_operation(self._dialect, "insert", duration, "success") logger.info("Stored login flow session for user %s", user_id) except Exception: duration = time.time() - start_time - record_db_operation("sqlite", "insert", duration, "error") + record_db_operation(self._dialect, "insert", duration, "error") raise async def get_login_flow_session(self, user_id: str) -> dict[str, Any] | None: @@ -1965,7 +2255,7 @@ class RefreshTokenStorage: now = int(time.time()) start_time = time.time() try: - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: async with db.execute( """ SELECT encrypted_poll_token, poll_endpoint, requested_scopes, @@ -1979,7 +2269,7 @@ class RefreshTokenStorage: if not row: duration = time.time() - start_time - record_db_operation("sqlite", "select", duration, "success") + record_db_operation(self._dialect, "select", duration, "success") return None encrypted_token, poll_endpoint, scopes_json, created_at, expires_at = row @@ -1987,7 +2277,7 @@ class RefreshTokenStorage: requested_scopes = json.loads(scopes_json) if scopes_json else None duration = time.time() - start_time - record_db_operation("sqlite", "select", duration, "success") + record_db_operation(self._dialect, "select", duration, "success") return { "poll_token": poll_token, @@ -1999,7 +2289,7 @@ class RefreshTokenStorage: except Exception as e: duration = time.time() - start_time - record_db_operation("sqlite", "select", duration, "error") + record_db_operation(self._dialect, "select", duration, "error") logger.error( "Failed to retrieve login flow session for user %s: %s", user_id, e ) @@ -2019,7 +2309,7 @@ class RefreshTokenStorage: start_time = time.time() try: - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: cursor = await db.execute( "DELETE FROM login_flow_sessions WHERE user_id = ?", (user_id,), @@ -2028,7 +2318,7 @@ class RefreshTokenStorage: deleted = cursor.rowcount > 0 duration = time.time() - start_time - record_db_operation("sqlite", "delete", duration, "success") + record_db_operation(self._dialect, "delete", duration, "success") if deleted: logger.info("Deleted login flow session for user %s", user_id) @@ -2042,7 +2332,7 @@ class RefreshTokenStorage: except Exception: duration = time.time() - start_time - record_db_operation("sqlite", "delete", duration, "error") + record_db_operation(self._dialect, "delete", duration, "error") raise async def delete_expired_login_flow_sessions(self) -> int: @@ -2057,7 +2347,7 @@ class RefreshTokenStorage: now = int(time.time()) start_time = time.time() try: - async with aiosqlite.connect(self.db_path) as db: + async with self._db() as db: cursor = await db.execute( "DELETE FROM login_flow_sessions WHERE expires_at <= ?", (now,), @@ -2066,7 +2356,7 @@ class RefreshTokenStorage: count = cursor.rowcount duration = time.time() - start_time - record_db_operation("sqlite", "delete", duration, "success") + record_db_operation(self._dialect, "delete", duration, "success") if count > 0: logger.info("Cleaned up %s expired login flow sessions", count) @@ -2080,7 +2370,7 @@ class RefreshTokenStorage: except Exception: duration = time.time() - start_time - record_db_operation("sqlite", "delete", duration, "error") + record_db_operation(self._dialect, "delete", duration, "error") raise diff --git a/nextcloud_mcp_server/cli.py b/nextcloud_mcp_server/cli.py index ee6b045c..15e7de1f 100644 --- a/nextcloud_mcp_server/cli.py +++ b/nextcloud_mcp_server/cli.py @@ -5,8 +5,8 @@ import click import uvicorn from nextcloud_mcp_server.config import ( + get_database_url, get_settings, - get_token_db_path, is_ephemeral_token_db, ) from nextcloud_mcp_server.migrations import ( @@ -287,27 +287,67 @@ def db(): pass -def _warn_if_ephemeral(database_path: str) -> None: - if is_ephemeral_token_db(database_path): +def _resolve_db_url(database_url: str | None, database_path: str | None) -> str: + """Pick the database URL for a CLI subcommand. + + Priority: explicit ``--database-url`` > legacy ``--database-path`` + (treated as a SQLite file) > :func:`get_database_url` (honors + ``DATABASE_URL`` env or falls back to the ephemeral SQLite tempfile). + """ + if database_url: + return database_url + if database_path: + return f"sqlite+aiosqlite:///{database_path}" + return get_database_url() + + +def _warn_if_ephemeral(database_url: str) -> None: + """Warn when the resolved URL is the per-process SQLite tempfile.""" + if not database_url.startswith( + "sqlite+aiosqlite:///" + ) and not database_url.startswith("sqlite:///"): + return + path = database_url.split("///", 1)[1] + if is_ephemeral_token_db(path): click.echo( click.style( - f"⚠ Using ephemeral tempfile {database_path}; changes " - "will be lost on exit. Pass --database-path or set " - "TOKEN_STORAGE_DB to operate on a persistent database.", + f"⚠ Using ephemeral tempfile {path}; changes " + "will be lost on exit. Pass --database-url / --database-path " + "or set DATABASE_URL / TOKEN_STORAGE_DB to operate on a " + "persistent database.", fg="yellow", ), err=True, ) +def _db_target_options(fn): + """Attach the shared ``--database-url`` / ``--database-path`` options. + + Using a decorator factory rather than ``**kwargs`` dict-expansion so + static type checkers (ty) see ``click.option`` called with literal + keyword arguments, which is the only form it's typed to accept. + """ + fn = click.option( + "--database-path", + "-d", + envvar="TOKEN_STORAGE_DB", + default=None, + help="SQLite database file path. Equivalent to " + "--database-url sqlite+aiosqlite:///.", + )(fn) + fn = click.option( + "--database-url", + "-u", + envvar="DATABASE_URL", + default=None, + help="SQLAlchemy URL (e.g. postgresql+asyncpg://...). Wins over --database-path.", + )(fn) + return fn + + @db.command() -@click.option( - "--database-path", - "-d", - envvar="TOKEN_STORAGE_DB", - default=None, - help="Path to token storage database (can also use TOKEN_STORAGE_DB env var)", -) +@_db_target_options @click.option( "--revision", "-r", @@ -315,7 +355,7 @@ def _warn_if_ephemeral(database_path: str) -> None: show_default=True, help="Target revision (default: head for latest)", ) -def upgrade(database_path: str | None, revision: str): +def upgrade(database_url: str | None, database_path: str | None, revision: str): """Upgrade database to a specific revision. \b @@ -323,17 +363,17 @@ def upgrade(database_path: str | None, revision: str): # Upgrade to latest version $ nextcloud-mcp-server db upgrade - # Upgrade to specific revision - $ nextcloud-mcp-server db upgrade --revision 001 + # Upgrade a Postgres backend + $ nextcloud-mcp-server db upgrade -u postgresql+asyncpg://mcp:mcp@db/mcp - # Use custom database path + # Use custom SQLite path $ nextcloud-mcp-server db upgrade -d /path/to/tokens.db """ - database_path = database_path or get_token_db_path() - _warn_if_ephemeral(database_path) + url = _resolve_db_url(database_url, database_path) + _warn_if_ephemeral(url) try: click.echo(f"Upgrading database to revision: {revision}") - upgrade_database(database_path, revision) + upgrade_database(url, revision) click.echo(click.style("✓ Database upgraded successfully", fg="green")) except Exception as e: click.echo(click.style(f"✗ Upgrade failed: {e}", fg="red"), err=True) @@ -341,13 +381,7 @@ def upgrade(database_path: str | None, revision: str): @db.command() -@click.option( - "--database-path", - "-d", - envvar="TOKEN_STORAGE_DB", - default=None, - help="Path to token storage database", -) +@_db_target_options @click.option( "--revision", "-r", @@ -358,27 +392,16 @@ def upgrade(database_path: str | None, revision: str): @click.confirmation_option( prompt="Are you sure you want to downgrade the database? This may result in data loss." ) -def downgrade(database_path: str | None, revision: str): +def downgrade(database_url: str | None, database_path: str | None, revision: str): """Downgrade database to a specific revision. WARNING: This may result in data loss! Use with caution. - - \b - Examples: - # Downgrade by one version - $ nextcloud-mcp-server db downgrade - - # Downgrade to specific revision - $ nextcloud-mcp-server db downgrade --revision 001 - - # Downgrade to base (empty database) - $ nextcloud-mcp-server db downgrade --revision base """ - database_path = database_path or get_token_db_path() - _warn_if_ephemeral(database_path) + url = _resolve_db_url(database_url, database_path) + _warn_if_ephemeral(url) try: click.echo(f"Downgrading database to revision: {revision}") - downgrade_database(database_path, revision) + downgrade_database(url, revision) click.echo(click.style("✓ Database downgraded successfully", fg="green")) except Exception as e: click.echo(click.style(f"✗ Downgrade failed: {e}", fg="red"), err=True) @@ -386,24 +409,13 @@ def downgrade(database_path: str | None, revision: str): @db.command() -@click.option( - "--database-path", - "-d", - envvar="TOKEN_STORAGE_DB", - default=None, - help="Path to token storage database", -) -def current(database_path: str | None): - """Show current database revision. - - \b - Example: - $ nextcloud-mcp-server db current - """ - database_path = database_path or get_token_db_path() - _warn_if_ephemeral(database_path) +@_db_target_options +def current(database_url: str | None, database_path: str | None): + """Show current database revision.""" + url = _resolve_db_url(database_url, database_path) + _warn_if_ephemeral(url) try: - revision = get_current_revision(database_path) + revision = get_current_revision(url) if revision: click.echo(f"Current revision: {click.style(revision, fg='cyan')}") else: @@ -420,25 +432,14 @@ def current(database_path: str | None): @db.command() -@click.option( - "--database-path", - "-d", - envvar="TOKEN_STORAGE_DB", - default=None, - help="Path to token storage database", -) -def history(database_path: str | None): - """Show migration history. - - \b - Example: - $ nextcloud-mcp-server db history - """ - database_path = database_path or get_token_db_path() - _warn_if_ephemeral(database_path) +@_db_target_options +def history(database_url: str | None, database_path: str | None): + """Show migration history.""" + url = _resolve_db_url(database_url, database_path) + _warn_if_ephemeral(url) try: click.echo("Migration history:") - show_migration_history(database_path) + show_migration_history(url) except Exception as e: click.echo(click.style(f"✗ Failed to show history: {e}", fg="red"), err=True) raise click.ClickException(str(e)) diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 85a1d6df..7a3c3b4c 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -56,6 +56,10 @@ _DEFAULTS: dict[str, Any] = { # None = ephemeral per-process tempfile (see get_token_db_path()). # Set TOKEN_STORAGE_DB to persist tokens across restarts. "token_storage_db": None, + # Centralized backend (any SQLAlchemy URL). Wins over TOKEN_STORAGE_DB + # when set. Use postgresql+asyncpg://user:pw@host/db for HA k8s + # deployments so pods can be stateless. See ADR-026. + "database_url": None, # Webhook delivery authentication (ADR-010): when set, registrations # tell NC to add `Authorization: Bearer ` to webhook deliveries # and the receiver rejects unauthenticated requests. @@ -279,6 +283,30 @@ def is_ephemeral_token_db(path: str) -> bool: return path == _ephemeral_db_path +def get_database_url() -> str: + """Resolve the SQLAlchemy database URL for token storage. + + Priority: + 1. ``DATABASE_URL`` if set — any SQLAlchemy URL is accepted; the primary + supported backends are ``postgresql+asyncpg://...`` for HA k8s + deployments and ``sqlite+aiosqlite:///...`` for development. + 2. Otherwise build ``sqlite+aiosqlite:///{get_token_db_path()}`` so the + legacy ``TOKEN_STORAGE_DB`` env var and the ephemeral-tempfile + fallback both keep working unchanged. + """ + explicit = _dynaconf.get("DATABASE_URL") + if explicit: + return str(explicit) + return f"sqlite+aiosqlite:///{get_token_db_path()}" + + +def is_sqlite_url(url: str) -> bool: + """Return True for SQLite SQLAlchemy URLs (used to gate sqlite-only logic + like file-permission hardening and ``sqlite_master`` legacy lookups). + """ + return url.startswith("sqlite") + + LOGGING_CONFIG = { "version": 1, "disable_existing_loggers": False, diff --git a/nextcloud_mcp_server/migrations.py b/nextcloud_mcp_server/migrations.py index 15b5274c..6b2cff41 100644 --- a/nextcloud_mcp_server/migrations.py +++ b/nextcloud_mcp_server/migrations.py @@ -3,22 +3,49 @@ This module provides helper functions for managing Alembic database migrations programmatically. It enables automatic migration on application startup and provides CLI integration. + +All helpers accept a SQLAlchemy URL (``sqlite+aiosqlite:///...`` or +``postgresql+asyncpg://...``). When called without an explicit URL they fall +back to :func:`nextcloud_mcp_server.config.get_database_url`. """ import logging -import sqlite3 from pathlib import Path from alembic.config import Config +from sqlalchemy import create_engine, inspect, text import nextcloud_mcp_server.alembic as alembic_package from alembic import command -from nextcloud_mcp_server.config import get_token_db_path +from nextcloud_mcp_server.config import get_database_url logger = logging.getLogger(__name__) -def get_alembic_config(database_path: str | Path | None = None) -> Config: +def _coerce_url(database_url: str | Path | None) -> str: + """Accept either a URL string, a Path (legacy SQLite path), or None. + + A bare ``Path`` is interpreted as a SQLite database file for backward + compatibility with the prior path-based API. + """ + if database_url is None: + return get_database_url() + if isinstance(database_url, Path): + return f"sqlite+aiosqlite:///{database_url.resolve()}" + return database_url + + +def _to_sync_url(database_url: str) -> str: + """Map an async driver URL to its sync equivalent for blocking inspection. + + SQLAlchemy's :func:`inspect` and :func:`create_engine` used below are + synchronous APIs. The runtime uses async drivers (``aiosqlite``, + ``asyncpg``) but Alembic and these utility queries don't need them. + """ + return database_url.replace("+aiosqlite", "").replace("+asyncpg", "") + + +def get_alembic_config(database_url: str | Path | None = None) -> Config: """ Get Alembic configuration for programmatic use. @@ -26,145 +53,102 @@ def get_alembic_config(database_path: str | Path | None = None) -> Config: package location instead of alembic.ini file. Args: - database_path: Path to SQLite database file. If None, resolves via - config.get_token_db_path() (ephemeral tempfile unless - TOKEN_STORAGE_DB is set). + database_url: SQLAlchemy URL. If None, resolves via + :func:`get_database_url` (DATABASE_URL env var, falling back + to the ephemeral SQLite tempfile under ``TOKEN_STORAGE_DB``). + For backward compatibility a ``Path`` is treated as a SQLite + file path. Returns: - Alembic Config object configured for the specified database + Alembic Config object configured for the resolved URL. """ - # Use package location (works in both editable and installed modes) if alembic_package.__file__ is None: raise RuntimeError("alembic package __file__ is None") script_location = Path(alembic_package.__file__).parent - # Create config programmatically (no alembic.ini needed at runtime) config = Config() config.set_main_option("script_location", str(script_location)) - config.set_main_option("path_separator", "os") # Suppress deprecation warning + config.set_main_option("path_separator", "os") - # Set database URL - if database_path: - db_path = Path(database_path).resolve() - else: - db_path = Path(get_token_db_path()).resolve() - - url = f"sqlite+aiosqlite:///{db_path}" + url = _coerce_url(database_url) config.set_main_option("sqlalchemy.url", url) logger.debug("Alembic script location: %s", script_location) - logger.debug("Database: %s", db_path) + logger.debug("Database URL: %s", url) return config def upgrade_database( - database_path: str | Path | None = None, revision: str = "head" + database_url: str | Path | None = None, revision: str = "head" ) -> None: - """ - Upgrade database to a specific revision. - - Args: - database_path: Path to SQLite database file - revision: Target revision (default: "head" for latest) - """ - config = get_alembic_config(database_path) + """Upgrade database to a specific revision (default: latest).""" + config = get_alembic_config(database_url) logger.info("Upgrading database to revision: %s", revision) command.upgrade(config, revision) logger.info("Database upgrade completed successfully") def downgrade_database( - database_path: str | Path | None = None, revision: str = "-1" + database_url: str | Path | None = None, revision: str = "-1" ) -> None: - """ - Downgrade database to a specific revision. - - Args: - database_path: Path to SQLite database file - revision: Target revision (default: "-1" for previous version) - """ - config = get_alembic_config(database_path) + """Downgrade database to a specific revision (default: previous).""" + config = get_alembic_config(database_url) logger.warning("Downgrading database to revision: %s", revision) command.downgrade(config, revision) logger.info("Database downgrade completed successfully") -def get_current_revision(database_path: str | Path | None = None) -> str | None: +def get_current_revision(database_url: str | Path | None = None) -> str | None: """ - Get the current database revision by directly querying the alembic_version table. + Get the current database revision by reading the ``alembic_version`` table. - Args: - database_path: Path to SQLite database file - - Returns: - Current revision ID or None if not versioned + Returns ``None`` when the database does not exist or has no + ``alembic_version`` table (i.e. has never been migrated). """ + url = _to_sync_url(_coerce_url(database_url)) - if database_path is None: - database_path = get_token_db_path() - - db_path = Path(database_path).resolve() - - if not db_path.exists(): - logger.debug("Database does not exist: %s", db_path) - return None - - try: - # Query alembic_version table directly - conn = sqlite3.connect(str(db_path)) - cursor = conn.cursor() - - # Check if alembic_version table exists - cursor.execute( - "SELECT name FROM sqlite_master WHERE type='table' AND name='alembic_version'" - ) - has_table = cursor.fetchone() is not None - - if not has_table: - conn.close() + if url.startswith("sqlite:///"): + path = url[len("sqlite:///") :] + if path and not Path(path).exists(): + logger.debug("Database does not exist: %s", path) return None - # Get current version - cursor.execute("SELECT version_num FROM alembic_version") - row = cursor.fetchone() - conn.close() - - return row[0] if row else None - + try: + engine = create_engine(url, future=True) + try: + inspector = inspect(engine) + if not inspector.has_table("alembic_version"): + return None + with engine.connect() as conn: + row = conn.execute( + text("SELECT version_num FROM alembic_version") + ).fetchone() + return row[0] if row else None + finally: + engine.dispose() except Exception as e: logger.error("Failed to get current revision: %s", e) return None def stamp_database( - database_path: str | Path | None = None, revision: str = "head" + database_url: str | Path | None = None, revision: str = "head" ) -> None: """ Stamp database with a specific revision without running migrations. - This is useful for marking existing databases that were created before - Alembic was introduced. It tells Alembic "this database is at revision X" - without actually running the migration. - - Args: - database_path: Path to SQLite database file - revision: Revision to stamp (default: "head" for latest) + Useful for marking pre-Alembic databases as already at a known revision. """ - config = get_alembic_config(database_path) + config = get_alembic_config(database_url) logger.info("Stamping database with revision: %s", revision) command.stamp(config, revision) logger.info("Database stamped successfully") -def show_migration_history(database_path: str | Path | None = None) -> None: - """ - Display migration history. - - Args: - database_path: Path to SQLite database file - """ - config = get_alembic_config(database_path) +def show_migration_history(database_url: str | Path | None = None) -> None: + """Display migration history.""" + config = get_alembic_config(database_url) command.history(config, verbose=True) @@ -178,7 +162,9 @@ def create_migration(message: str, autogenerate: bool = False) -> None: Note: Since we don't use SQLAlchemy models, autogenerate will be disabled - and migrations must be written manually. + and migrations must be written manually using portable Alembic + operations (``op.create_table``, ``op.add_column`` …) rather than + raw SQL so they work on both SQLite and Postgres. """ config = get_alembic_config() logger.info("Creating new migration: %s", message) diff --git a/pyproject.toml b/pyproject.toml index 1e5c205d..9bd6a67e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -45,6 +45,8 @@ dependencies = [ "openai>=2.8.1", "dynaconf>=3.2.13,<4.0", "mistralai>=2.4.5", + "sqlalchemy[asyncio]>=2.0", + "asyncpg>=0.29", ] classifiers = [ "Development Status :: 4 - Beta", @@ -78,6 +80,7 @@ markers = [ "keycloak: OAuth tests that utilize keycloak external identity provider", "login_flow: Login Flow v2 integration tests (ADR-022)", "multi_user_basic: Multi-user BasicAuth pass-through tests (ADR-020)", + "postgres: Tests requiring the docker-compose postgres-test service (ADR-026)", ] testpaths = [ "tests", diff --git a/tests/fixtures/__init__.py b/tests/fixtures/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/fixtures/storage_backend.py b/tests/fixtures/storage_backend.py new file mode 100644 index 00000000..22a7f2e1 --- /dev/null +++ b/tests/fixtures/storage_backend.py @@ -0,0 +1,97 @@ +"""Shared pytest fixture for parametrizing storage tests over backends. + +Tests for ``RefreshTokenStorage`` are exercised against every backend that is +available in the current environment: + +- ``sqlite`` — always available; uses a per-test tempfile. +- ``postgres`` — opt-in. Bring up the test instance with:: + + docker compose --profile postgres up -d postgres-test + + and export the URL so the fixture picks it up:: + + export TEST_DATABASE_URL=postgresql+asyncpg://mcp:mcp@localhost:5433/mcp + + When ``TEST_DATABASE_URL`` is unset (or the host is unreachable), the + Postgres parametrization is skipped automatically so the suite still runs + cleanly without Docker. + +Each Postgres test runs against an isolated schema that is dropped and +recreated between tests, mirroring the per-tempfile isolation that the +SQLite path gets for free. +""" + +from __future__ import annotations + +import os +from typing import Any + +import pytest + + +def _postgres_url() -> str | None: + """Resolve the Postgres URL for tests, or ``None`` when opted out.""" + return os.environ.get("TEST_DATABASE_URL") or None + + +def _postgres_reachable(url: str) -> bool: + """Return ``True`` if the configured Postgres accepts TCP connections. + + A lightweight socket probe is used rather than a full DB handshake so + we don't have to ship a sync Postgres driver (psycopg2) just for the + test gate — asyncpg only works inside an event loop. + """ + import socket + from urllib.parse import urlparse + + try: + parsed = urlparse(url) + host = parsed.hostname or "localhost" + port = parsed.port or 5432 + with socket.create_connection((host, port), timeout=1.0): + return True + except OSError: + return False + + +def _backend_params() -> list[Any]: + """Build the pytest parametrize list, gating Postgres on availability.""" + params: list[Any] = [pytest.param("sqlite", id="sqlite")] + url = _postgres_url() + if url and _postgres_reachable(url): + params.append(pytest.param(url, id="postgres")) + return params + + +@pytest.fixture(params=_backend_params()) +def storage_backend(request): + """Yield ``{"kind": ..., "url": ..., "reset": }`` per backend. + + For ``sqlite`` the test fixture builds its own tempfile path; only the + ``kind`` discriminator is used. For ``postgres`` the URL is forwarded + and a ``reset()`` coroutine is provided so test fixtures can wipe the + schema between parametrized runs. + """ + if request.param == "sqlite": + yield {"kind": "sqlite"} + return + + url = request.param + + async def reset() -> None: + # Use a fresh async engine so we don't fight an async connection + # the test might still be holding open at teardown time. asyncpg + # is the only driver we ship for Postgres, so the reset path stays + # event-loop-only (no psycopg2 dependency required). + from sqlalchemy import text + from sqlalchemy.ext.asyncio import create_async_engine + + engine = create_async_engine(url, future=True) + try: + async with engine.begin() as conn: + await conn.execute(text("DROP SCHEMA public CASCADE")) + await conn.execute(text("CREATE SCHEMA public")) + finally: + await engine.dispose() + + yield {"kind": "postgres", "url": url, "reset": reset} diff --git a/tests/integration/test_storage_postgres.py b/tests/integration/test_storage_postgres.py new file mode 100644 index 00000000..8cdee460 --- /dev/null +++ b/tests/integration/test_storage_postgres.py @@ -0,0 +1,164 @@ +"""End-to-end Postgres backend smoke for RefreshTokenStorage (ADR-026). + +Exercises every storage method touched by the SQLAlchemy / asyncpg port +against a fresh Postgres schema. The test is opt-in: it requires the +``postgres-test`` docker-compose service to be running and +``TEST_DATABASE_URL`` to be exported. + +Bring up the dependency once:: + + docker compose --profile postgres up -d postgres-test + export TEST_DATABASE_URL=postgresql+asyncpg://mcp:mcp@localhost:5433/mcp + +Then run:: + + uv run pytest tests/integration/test_storage_postgres.py -v -m postgres + +When ``TEST_DATABASE_URL`` is unset (or the service is unreachable) the +test is skipped so the full suite still passes locally without Docker. +""" + +from __future__ import annotations + +import os +import socket +from urllib.parse import urlparse + +import pytest +from cryptography.fernet import Fernet + +from nextcloud_mcp_server.auth.storage import RefreshTokenStorage + +pytestmark = [pytest.mark.integration, pytest.mark.postgres] + + +def _postgres_url() -> str | None: + return os.environ.get("TEST_DATABASE_URL") or None + + +def _reachable(url: str) -> bool: + parsed = urlparse(url) + try: + with socket.create_connection( + (parsed.hostname or "localhost", parsed.port or 5432), timeout=1.0 + ): + return True + except OSError: + return False + + +@pytest.fixture +def postgres_url() -> str: + url = _postgres_url() + if not url: + pytest.skip( + "TEST_DATABASE_URL not set — run " + "`docker compose --profile postgres up -d postgres-test` and export " + "TEST_DATABASE_URL=postgresql+asyncpg://mcp:mcp@localhost:5433/mcp" + ) + if not _reachable(url): + pytest.skip(f"Postgres at {url} is not reachable") + return url + + +@pytest.fixture +async def reset_schema(postgres_url: str): + """Drop+recreate the public schema before and after each test.""" + from sqlalchemy import text + from sqlalchemy.ext.asyncio import create_async_engine + + async def _reset() -> None: + engine = create_async_engine(postgres_url, future=True) + try: + async with engine.begin() as conn: + await conn.execute(text("DROP SCHEMA public CASCADE")) + await conn.execute(text("CREATE SCHEMA public")) + finally: + await engine.dispose() + + await _reset() + yield + await _reset() + + +@pytest.fixture +async def storage(postgres_url: str, reset_schema): + key = Fernet.generate_key() + s = RefreshTokenStorage(database_url=postgres_url, encryption_key=key) + await s.initialize() + yield s + + +async def test_refresh_token_roundtrip(storage: RefreshTokenStorage): + """Store + retrieve + upsert + delete a refresh token end-to-end.""" + await storage.store_refresh_token( + user_id="alice", refresh_token="rt-1", expires_at=9_999_999_999 + ) + tok = await storage.get_refresh_token("alice") + assert tok is not None + assert tok["refresh_token"] == "rt-1" + assert tok["expires_at"] == 9_999_999_999 + + # Upsert preserves user_id, swaps token contents. + await storage.store_refresh_token( + user_id="alice", refresh_token="rt-2", expires_at=9_999_999_999 + ) + tok = await storage.get_refresh_token("alice") + assert tok is not None and tok["refresh_token"] == "rt-2" + + assert await storage.delete_refresh_token("alice") is True + assert await storage.get_refresh_token("alice") is None + + +async def test_app_password_roundtrip(storage: RefreshTokenStorage): + """Store + retrieve + replace + delete a scoped app password.""" + await storage.store_app_password(user_id="bob", app_password="pw-1") + assert await storage.get_app_password("bob") == "pw-1" + + # Replace path exercises the ON CONFLICT DO UPDATE on the singleton row. + await storage.store_app_password(user_id="bob", app_password="pw-2") + assert await storage.get_app_password("bob") == "pw-2" + + assert await storage.delete_app_password("bob") is True + assert await storage.get_app_password("bob") is None + + +async def test_oauth_session_lifecycle(storage: RefreshTokenStorage): + """Cover the ADR-004 progressive-consent session table.""" + await storage.store_oauth_session( + session_id="sess-1", + client_redirect_uri="http://localhost:12345/callback", + mcp_authorization_code="mcp-code-abc", + flow_type="hybrid", + ttl_seconds=600, + ) + fetched = await storage.get_oauth_session("sess-1") + assert fetched is not None + assert fetched["mcp_authorization_code"] == "mcp-code-abc" + + by_code = await storage.get_oauth_session_by_mcp_code("mcp-code-abc") + assert by_code is not None and by_code["session_id"] == "sess-1" + + +async def test_webhook_tracking(storage: RefreshTokenStorage): + """Tracks webhook ↔ preset mappings via ON CONFLICT upserts.""" + await storage.store_webhook(webhook_id=101, preset_id="notes_sync") + await storage.store_webhook(webhook_id=202, preset_id="notes_sync") + await storage.store_webhook(webhook_id=303, preset_id="calendar_sync") + + assert sorted(await storage.get_webhooks_by_preset("notes_sync")) == [101, 202] + assert await storage.get_webhooks_by_preset("calendar_sync") == [303] + + # Re-storing the same webhook_id is a no-op upsert. + await storage.store_webhook(webhook_id=101, preset_id="notes_sync") + assert sorted(await storage.get_webhooks_by_preset("notes_sync")) == [101, 202] + + assert await storage.delete_webhook(webhook_id=101) is True + assert await storage.get_webhooks_by_preset("notes_sync") == [202] + + +async def test_audit_log_capture(storage: RefreshTokenStorage): + """Audit events from upstream methods land in audit_logs.""" + await storage.store_app_password(user_id="carol", app_password="x") + logs = await storage.get_audit_logs(user_id="carol", limit=10) + assert any(entry["event"] == "store_app_password" for entry in logs) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 29b3b160..51ba62a8 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -2,6 +2,11 @@ import pytest +# Re-export the parametrized storage backend fixture so it's auto-discovered +# by every unit test that names it as a parameter, without each test module +# having to import it explicitly. +from tests.fixtures.storage_backend import storage_backend # noqa: F401 + @pytest.fixture(autouse=True) def _reload_dynaconf_after_test(): diff --git a/tests/unit/test_app_password_storage.py b/tests/unit/test_app_password_storage.py index 37431974..ed2f425f 100644 --- a/tests/unit/test_app_password_storage.py +++ b/tests/unit/test_app_password_storage.py @@ -3,6 +3,12 @@ Unit tests for App Password Storage functionality. Tests the app password methods in RefreshTokenStorage for multi-user BasicAuth mode background sync. + +These tests are parametrized over both supported backends so the storage +layer is exercised against SQLite (default, always runs) and Postgres (gated +on ``TEST_DATABASE_URL``; bring up ``docker compose --profile postgres up +-d postgres-test`` and export ``TEST_DATABASE_URL=postgresql+asyncpg://mcp:mcp@localhost:5433/mcp`` +to opt in). """ import tempfile @@ -23,15 +29,32 @@ def encryption_key(): @pytest.fixture -async def temp_storage(encryption_key): - """Create temporary storage instance with encryption for testing.""" - with tempfile.TemporaryDirectory() as tmpdir: - db_path = Path(tmpdir) / "test_app_passwords.db" +async def temp_storage(encryption_key, storage_backend): + """Create a storage instance backed by either SQLite or Postgres. + + The ``storage_backend`` fixture is parametrized by pytest, so every test + that uses ``temp_storage`` runs once per backend that is available in + the current environment. + """ + if storage_backend["kind"] == "sqlite": + with tempfile.TemporaryDirectory() as tmpdir: + db_path = Path(tmpdir) / "test_app_passwords.db" + storage = RefreshTokenStorage( + db_path=str(db_path), encryption_key=encryption_key + ) + await storage.initialize() + yield storage + else: storage = RefreshTokenStorage( - db_path=str(db_path), encryption_key=encryption_key + database_url=storage_backend["url"], encryption_key=encryption_key ) await storage.initialize() - yield storage + try: + yield storage + finally: + # Each test gets an isolated schema; tear it down so the next + # parametrized run starts clean. + await storage_backend["reset"]() async def test_store_app_password(temp_storage): diff --git a/tests/unit/test_webhook_storage.py b/tests/unit/test_webhook_storage.py index e07d485e..be41347f 100644 --- a/tests/unit/test_webhook_storage.py +++ b/tests/unit/test_webhook_storage.py @@ -3,6 +3,9 @@ Unit tests for Webhook Storage functionality. Tests the webhook tracking methods in RefreshTokenStorage without requiring real database connections or network calls. + +Runs against both SQLite and Postgres backends — see the docstring on +``tests.fixtures.storage_backend`` for opt-in instructions. """ import tempfile @@ -17,14 +20,23 @@ pytestmark = pytest.mark.unit @pytest.fixture -async def temp_storage(): - """Create temporary storage instance for testing.""" - with tempfile.TemporaryDirectory() as tmpdir: - db_path = Path(tmpdir) / "test_webhooks.db" - # No encryption key needed for webhook tracking - storage = RefreshTokenStorage(db_path=str(db_path), encryption_key=None) +async def temp_storage(storage_backend): + """Create a storage instance backed by either SQLite or Postgres.""" + if storage_backend["kind"] == "sqlite": + with tempfile.TemporaryDirectory() as tmpdir: + db_path = Path(tmpdir) / "test_webhooks.db" + storage = RefreshTokenStorage(db_path=str(db_path), encryption_key=None) + await storage.initialize() + yield storage + else: + storage = RefreshTokenStorage( + database_url=storage_backend["url"], encryption_key=None + ) await storage.initialize() - yield storage + try: + yield storage + finally: + await storage_backend["reset"]() async def test_store_webhook(temp_storage): diff --git a/uv.lock b/uv.lock index b41bbc0d..c0bb2d9d 100644 --- a/uv.lock +++ b/uv.lock @@ -226,6 +226,54 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/d2/39/e7eaf1799466a4aef85b6a4fe7bd175ad2b1c6345066aa33f1f58d4b18d0/asttokens-3.0.1-py3-none-any.whl", hash = "sha256:15a3ebc0f43c2d0a50eeafea25e19046c68398e487b9f1f5b517f7c0f40f976a", size = 27047, upload-time = "2025-11-15T16:43:16.109Z" }, ] +[[package]] +name = "asyncpg" +version = "0.31.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/fe/cc/d18065ce2380d80b1bcce927c24a2642efd38918e33fd724bc4bca904877/asyncpg-0.31.0.tar.gz", hash = "sha256:c989386c83940bfbd787180f2b1519415e2d3d6277a70d9d0f0145ac73500735", size = 993667, upload-time = "2025-11-24T23:27:00.812Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/08/17/cc02bc49bc350623d050fa139e34ea512cd6e020562f2a7312a7bcae4bc9/asyncpg-0.31.0-cp311-cp311-macosx_10_9_x86_64.whl", hash = "sha256:eee690960e8ab85063ba93af2ce128c0f52fd655fdff9fdb1a28df01329f031d", size = 643159, upload-time = "2025-11-24T23:25:36.443Z" }, + { url = "https://files.pythonhosted.org/packages/a4/62/4ded7d400a7b651adf06f49ea8f73100cca07c6df012119594d1e3447aa6/asyncpg-0.31.0-cp311-cp311-macosx_11_0_arm64.whl", hash = "sha256:2657204552b75f8288de08ca60faf4a99a65deef3a71d1467454123205a88fab", size = 638157, upload-time = "2025-11-24T23:25:37.89Z" }, + { url = "https://files.pythonhosted.org/packages/d6/5b/4179538a9a72166a0bf60ad783b1ef16efb7960e4d7b9afe9f77a5551680/asyncpg-0.31.0-cp311-cp311-manylinux2014_aarch64.manylinux_2_17_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:a429e842a3a4b4ea240ea52d7fe3f82d5149853249306f7ff166cb9948faa46c", size = 2918051, upload-time = "2025-11-24T23:25:39.461Z" }, + { url = "https://files.pythonhosted.org/packages/e6/35/c27719ae0536c5b6e61e4701391ffe435ef59539e9360959240d6e47c8c8/asyncpg-0.31.0-cp311-cp311-manylinux2014_x86_64.manylinux_2_17_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:c0807be46c32c963ae40d329b3a686356e417f674c976c07fa49f1b30303f109", size = 2972640, upload-time = "2025-11-24T23:25:41.512Z" }, + { url = "https://files.pythonhosted.org/packages/43/f4/01ebb9207f29e645a64699b9ce0eefeff8e7a33494e1d29bb53736f7766b/asyncpg-0.31.0-cp311-cp311-musllinux_1_2_aarch64.whl", hash = "sha256:e5d5098f63beeae93512ee513d4c0c53dc12e9aa2b7a1af5a81cddf93fe4e4da", size = 2851050, upload-time = "2025-11-24T23:25:43.153Z" }, + { url = "https://files.pythonhosted.org/packages/3e/f4/03ff1426acc87be0f4e8d40fa2bff5c3952bef0080062af9efc2212e3be8/asyncpg-0.31.0-cp311-cp311-musllinux_1_2_x86_64.whl", hash = "sha256:37fc6c00a814e18eef51833545d1891cac9aa69140598bb076b4cd29b3e010b9", size = 2962574, upload-time = "2025-11-24T23:25:44.942Z" }, + { url = "https://files.pythonhosted.org/packages/c7/39/cc788dfca3d4060f9d93e67be396ceec458dfc429e26139059e58c2c244d/asyncpg-0.31.0-cp311-cp311-win32.whl", hash = "sha256:5a4af56edf82a701aece93190cc4e094d2df7d33f6e915c222fb09efbb5afc24", size = 521076, upload-time = "2025-11-24T23:25:46.486Z" }, + { url = "https://files.pythonhosted.org/packages/28/fc/735af5384c029eb7f1ca60ccb8fa95521dbdaeef788edf4cecfc604c3cab/asyncpg-0.31.0-cp311-cp311-win_amd64.whl", hash = "sha256:480c4befbdf079c14c9ca43c8c5e1fe8b6296c96f1f927158d4f1e750aacc047", size = 584980, upload-time = "2025-11-24T23:25:47.938Z" }, + { url = "https://files.pythonhosted.org/packages/2a/a6/59d0a146e61d20e18db7396583242e32e0f120693b67a8de43f1557033e2/asyncpg-0.31.0-cp312-cp312-macosx_10_13_x86_64.whl", hash = "sha256:b44c31e1efc1c15188ef183f287c728e2046abb1d26af4d20858215d50d91fad", size = 662042, upload-time = "2025-11-24T23:25:49.578Z" }, + { url = "https://files.pythonhosted.org/packages/36/01/ffaa189dcb63a2471720615e60185c3f6327716fdc0fc04334436fbb7c65/asyncpg-0.31.0-cp312-cp312-macosx_11_0_arm64.whl", hash = "sha256:0c89ccf741c067614c9b5fc7f1fc6f3b61ab05ae4aaa966e6fd6b93097c7d20d", size = 638504, upload-time = "2025-11-24T23:25:51.501Z" }, + { url = "https://files.pythonhosted.org/packages/9f/62/3f699ba45d8bd24c5d65392190d19656d74ff0185f42e19d0bbd973bb371/asyncpg-0.31.0-cp312-cp312-manylinux_2_28_aarch64.whl", hash = "sha256:12b3b2e39dc5470abd5e98c8d3373e4b1d1234d9fbdedf538798b2c13c64460a", size = 3426241, upload-time = "2025-11-24T23:25:53.278Z" }, + { url = "https://files.pythonhosted.org/packages/8c/d1/a867c2150f9c6e7af6462637f613ba67f78a314b00db220cd26ff559d532/asyncpg-0.31.0-cp312-cp312-manylinux_2_28_x86_64.whl", hash = "sha256:aad7a33913fb8bcb5454313377cc330fbb19a0cd5faa7272407d8a0c4257b671", size = 3520321, upload-time = "2025-11-24T23:25:54.982Z" }, + { url = "https://files.pythonhosted.org/packages/7a/1a/cce4c3f246805ecd285a3591222a2611141f1669d002163abef999b60f98/asyncpg-0.31.0-cp312-cp312-musllinux_1_2_aarch64.whl", hash = "sha256:3df118d94f46d85b2e434fd62c84cb66d5834d5a890725fe625f498e72e4d5ec", size = 3316685, upload-time = "2025-11-24T23:25:57.43Z" }, + { url = "https://files.pythonhosted.org/packages/40/ae/0fc961179e78cc579e138fad6eb580448ecae64908f95b8cb8ee2f241f67/asyncpg-0.31.0-cp312-cp312-musllinux_1_2_x86_64.whl", hash = "sha256:bd5b6efff3c17c3202d4b37189969acf8927438a238c6257f66be3c426beba20", size = 3471858, upload-time = "2025-11-24T23:25:59.636Z" }, + { url = "https://files.pythonhosted.org/packages/52/b2/b20e09670be031afa4cbfabd645caece7f85ec62d69c312239de568e058e/asyncpg-0.31.0-cp312-cp312-win32.whl", hash = "sha256:027eaa61361ec735926566f995d959ade4796f6a49d3bde17e5134b9964f9ba8", size = 527852, upload-time = "2025-11-24T23:26:01.084Z" }, + { url = "https://files.pythonhosted.org/packages/b5/f0/f2ed1de154e15b107dc692262395b3c17fc34eafe2a78fc2115931561730/asyncpg-0.31.0-cp312-cp312-win_amd64.whl", hash = "sha256:72d6bdcbc93d608a1158f17932de2321f68b1a967a13e014998db87a72ed3186", size = 597175, upload-time = "2025-11-24T23:26:02.564Z" }, + { url = "https://files.pythonhosted.org/packages/95/11/97b5c2af72a5d0b9bc3fa30cd4b9ce22284a9a943a150fdc768763caf035/asyncpg-0.31.0-cp313-cp313-macosx_10_13_x86_64.whl", hash = "sha256:c204fab1b91e08b0f47e90a75d1b3c62174dab21f670ad6c5d0f243a228f015b", size = 661111, upload-time = "2025-11-24T23:26:04.467Z" }, + { url = "https://files.pythonhosted.org/packages/1b/71/157d611c791a5e2d0423f09f027bd499935f0906e0c2a416ce712ba51ef3/asyncpg-0.31.0-cp313-cp313-macosx_11_0_arm64.whl", hash = "sha256:54a64f91839ba59008eccf7aad2e93d6e3de688d796f35803235ea1c4898ae1e", size = 636928, upload-time = "2025-11-24T23:26:05.944Z" }, + { url = "https://files.pythonhosted.org/packages/2e/fc/9e3486fb2bbe69d4a867c0b76d68542650a7ff1574ca40e84c3111bb0c6e/asyncpg-0.31.0-cp313-cp313-manylinux2014_aarch64.manylinux_2_17_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:c0e0822b1038dc7253b337b0f3f676cadc4ac31b126c5d42691c39691962e403", size = 3424067, upload-time = "2025-11-24T23:26:07.957Z" }, + { url = "https://files.pythonhosted.org/packages/12/c6/8c9d076f73f07f995013c791e018a1cd5f31823c2a3187fc8581706aa00f/asyncpg-0.31.0-cp313-cp313-manylinux2014_x86_64.manylinux_2_17_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:bef056aa502ee34204c161c72ca1f3c274917596877f825968368b2c33f585f4", size = 3518156, upload-time = "2025-11-24T23:26:09.591Z" }, + { url = "https://files.pythonhosted.org/packages/ae/3b/60683a0baf50fbc546499cfb53132cb6835b92b529a05f6a81471ab60d0c/asyncpg-0.31.0-cp313-cp313-musllinux_1_2_aarch64.whl", hash = "sha256:0bfbcc5b7ffcd9b75ab1558f00db2ae07db9c80637ad1b2469c43df79d7a5ae2", size = 3319636, upload-time = "2025-11-24T23:26:11.168Z" }, + { url = "https://files.pythonhosted.org/packages/50/dc/8487df0f69bd398a61e1792b3cba0e47477f214eff085ba0efa7eac9ce87/asyncpg-0.31.0-cp313-cp313-musllinux_1_2_x86_64.whl", hash = "sha256:22bc525ebbdc24d1261ecbf6f504998244d4e3be1721784b5f64664d61fbe602", size = 3472079, upload-time = "2025-11-24T23:26:13.164Z" }, + { url = "https://files.pythonhosted.org/packages/13/a1/c5bbeeb8531c05c89135cb8b28575ac2fac618bcb60119ee9696c3faf71c/asyncpg-0.31.0-cp313-cp313-win32.whl", hash = "sha256:f890de5e1e4f7e14023619399a471ce4b71f5418cd67a51853b9910fdfa73696", size = 527606, upload-time = "2025-11-24T23:26:14.78Z" }, + { url = "https://files.pythonhosted.org/packages/91/66/b25ccb84a246b470eb943b0107c07edcae51804912b824054b3413995a10/asyncpg-0.31.0-cp313-cp313-win_amd64.whl", hash = "sha256:dc5f2fa9916f292e5c5c8b2ac2813763bcd7f58e130055b4ad8a0531314201ab", size = 596569, upload-time = "2025-11-24T23:26:16.189Z" }, + { url = "https://files.pythonhosted.org/packages/3c/36/e9450d62e84a13aea6580c83a47a437f26c7ca6fa0f0fd40b6670793ea30/asyncpg-0.31.0-cp314-cp314-macosx_10_15_x86_64.whl", hash = "sha256:f6b56b91bb0ffc328c4e3ed113136cddd9deefdf5f79ab448598b9772831df44", size = 660867, upload-time = "2025-11-24T23:26:17.631Z" }, + { url = "https://files.pythonhosted.org/packages/82/4b/1d0a2b33b3102d210439338e1beea616a6122267c0df459ff0265cd5807a/asyncpg-0.31.0-cp314-cp314-macosx_11_0_arm64.whl", hash = "sha256:334dec28cf20d7f5bb9e45b39546ddf247f8042a690bff9b9573d00086e69cb5", size = 638349, upload-time = "2025-11-24T23:26:19.689Z" }, + { url = "https://files.pythonhosted.org/packages/41/aa/e7f7ac9a7974f08eff9183e392b2d62516f90412686532d27e196c0f0eeb/asyncpg-0.31.0-cp314-cp314-manylinux2014_aarch64.manylinux_2_17_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:98cc158c53f46de7bb677fd20c417e264fc02b36d901cc2a43bd6cb0dc6dbfd2", size = 3410428, upload-time = "2025-11-24T23:26:21.275Z" }, + { url = "https://files.pythonhosted.org/packages/6f/de/bf1b60de3dede5c2731e6788617a512bc0ebd9693eac297ee74086f101d7/asyncpg-0.31.0-cp314-cp314-manylinux2014_x86_64.manylinux_2_17_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:9322b563e2661a52e3cdbc93eed3be7748b289f792e0011cb2720d278b366ce2", size = 3471678, upload-time = "2025-11-24T23:26:23.627Z" }, + { url = "https://files.pythonhosted.org/packages/46/78/fc3ade003e22d8bd53aaf8f75f4be48f0b460fa73738f0391b9c856a9147/asyncpg-0.31.0-cp314-cp314-musllinux_1_2_aarch64.whl", hash = "sha256:19857a358fc811d82227449b7ca40afb46e75b33eb8897240c3839dd8b744218", size = 3313505, upload-time = "2025-11-24T23:26:25.235Z" }, + { url = "https://files.pythonhosted.org/packages/bf/e9/73eb8a6789e927816f4705291be21f2225687bfa97321e40cd23055e903a/asyncpg-0.31.0-cp314-cp314-musllinux_1_2_x86_64.whl", hash = "sha256:ba5f8886e850882ff2c2ace5732300e99193823e8107e2c53ef01c1ebfa1e85d", size = 3434744, upload-time = "2025-11-24T23:26:26.944Z" }, + { url = "https://files.pythonhosted.org/packages/08/4b/f10b880534413c65c5b5862f79b8e81553a8f364e5238832ad4c0af71b7f/asyncpg-0.31.0-cp314-cp314-win32.whl", hash = "sha256:cea3a0b2a14f95834cee29432e4ddc399b95700eb1d51bbc5bfee8f31fa07b2b", size = 532251, upload-time = "2025-11-24T23:26:28.404Z" }, + { url = "https://files.pythonhosted.org/packages/d3/2d/7aa40750b7a19efa5d66e67fc06008ca0f27ba1bd082e457ad82f59aba49/asyncpg-0.31.0-cp314-cp314-win_amd64.whl", hash = "sha256:04d19392716af6b029411a0264d92093b6e5e8285ae97a39957b9a9c14ea72be", size = 604901, upload-time = "2025-11-24T23:26:30.34Z" }, + { url = "https://files.pythonhosted.org/packages/ce/fe/b9dfe349b83b9dee28cc42360d2c86b2cdce4cb551a2c2d27e156bcac84d/asyncpg-0.31.0-cp314-cp314t-macosx_10_15_x86_64.whl", hash = "sha256:bdb957706da132e982cc6856bb2f7b740603472b54c3ebc77fe60ea3e57e1bd2", size = 702280, upload-time = "2025-11-24T23:26:32Z" }, + { url = "https://files.pythonhosted.org/packages/6a/81/e6be6e37e560bd91e6c23ea8a6138a04fd057b08cf63d3c5055c98e81c1d/asyncpg-0.31.0-cp314-cp314t-macosx_11_0_arm64.whl", hash = "sha256:6d11b198111a72f47154fa03b85799f9be63701e068b43f84ac25da0bda9cb31", size = 682931, upload-time = "2025-11-24T23:26:33.572Z" }, + { url = "https://files.pythonhosted.org/packages/a6/45/6009040da85a1648dd5bc75b3b0a062081c483e75a1a29041ae63a0bf0dc/asyncpg-0.31.0-cp314-cp314t-manylinux2014_aarch64.manylinux_2_17_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:18c83b03bc0d1b23e6230f5bf8d4f217dc9bc08644ce0502a9d91dc9e634a9c7", size = 3581608, upload-time = "2025-11-24T23:26:35.638Z" }, + { url = "https://files.pythonhosted.org/packages/7e/06/2e3d4d7608b0b2b3adbee0d0bd6a2d29ca0fc4d8a78f8277df04e2d1fd7b/asyncpg-0.31.0-cp314-cp314t-manylinux2014_x86_64.manylinux_2_17_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:e009abc333464ff18b8f6fd146addffd9aaf63e79aa3bb40ab7a4c332d0c5e9e", size = 3498738, upload-time = "2025-11-24T23:26:37.275Z" }, + { url = "https://files.pythonhosted.org/packages/7d/aa/7d75ede780033141c51d83577ea23236ba7d3a23593929b32b49db8ed36e/asyncpg-0.31.0-cp314-cp314t-musllinux_1_2_aarch64.whl", hash = "sha256:3b1fbcb0e396a5ca435a8826a87e5c2c2cc0c8c68eb6fadf82168056b0e53a8c", size = 3401026, upload-time = "2025-11-24T23:26:39.423Z" }, + { url = "https://files.pythonhosted.org/packages/ba/7a/15e37d45e7f7c94facc1e9148c0e455e8f33c08f0b8a0b1deb2c5171771b/asyncpg-0.31.0-cp314-cp314t-musllinux_1_2_x86_64.whl", hash = "sha256:8df714dba348efcc162d2adf02d213e5fab1bd9f557e1305633e851a61814a7a", size = 3429426, upload-time = "2025-11-24T23:26:41.032Z" }, + { url = "https://files.pythonhosted.org/packages/13/d5/71437c5f6ae5f307828710efbe62163974e71237d5d46ebd2869ea052d10/asyncpg-0.31.0-cp314-cp314t-win32.whl", hash = "sha256:1b41f1afb1033f2b44f3234993b15096ddc9cd71b21a42dbd87fc6a57b43d65d", size = 614495, upload-time = "2025-11-24T23:26:42.659Z" }, + { url = "https://files.pythonhosted.org/packages/3c/d7/8fb3044eaef08a310acfe23dae9a8e2e07d305edc29a53497e52bc76eca7/asyncpg-0.31.0-cp314-cp314t-win_amd64.whl", hash = "sha256:bd4107bb7cdd0e9e65fae66a62afd3a249663b844fa34d479f6d5b3bef9c04c3", size = 706062, upload-time = "2025-11-24T23:26:44.086Z" }, +] + [[package]] name = "attrs" version = "25.4.0" @@ -2129,6 +2177,7 @@ dependencies = [ { name = "aiosqlite" }, { name = "alembic" }, { name = "anthropic" }, + { name = "asyncpg" }, { name = "authlib" }, { name = "boto3" }, { name = "caldav" }, @@ -2159,6 +2208,7 @@ dependencies = [ { name = "pythonvcard4" }, { name = "qdrant-client" }, { name = "recurring-ical-events" }, + { name = "sqlalchemy", extra = ["asyncio"] }, { name = "starlette" }, ] @@ -2184,6 +2234,7 @@ requires-dist = [ { name = "aiosqlite", specifier = ">=0.20.0" }, { name = "alembic", specifier = ">=1.14.0" }, { name = "anthropic", specifier = ">=0.42.0" }, + { name = "asyncpg", specifier = ">=0.29" }, { name = "authlib", specifier = ">=1.6.5" }, { name = "boto3", specifier = ">=1.35.0" }, { name = "caldav", specifier = ">=3.0.1,<4.0" }, @@ -2214,6 +2265,7 @@ requires-dist = [ { name = "pythonvcard4", specifier = ">=0.2.0" }, { name = "qdrant-client", specifier = ">=1.17.0" }, { name = "recurring-ical-events", specifier = ">=3.8.0,<4.0" }, + { name = "sqlalchemy", extras = ["asyncio"], specifier = ">=2.0" }, { name = "starlette", specifier = "<1.0" }, ] @@ -3944,6 +3996,11 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/bf/e1/3ccb13c643399d22289c6a9786c1a91e3dcbb68bce4beb44926ac2c557bf/sqlalchemy-2.0.45-py3-none-any.whl", hash = "sha256:5225a288e4c8cc2308dbdd874edad6e7d0fd38eac1e9e5f23503425c8eee20d0", size = 1936672, upload-time = "2025-12-09T21:54:52.608Z" }, ] +[package.optional-dependencies] +asyncio = [ + { name = "greenlet" }, +] + [[package]] name = "sse-starlette" version = "3.0.3" From f2b7bf132f31bf8084dc2f37d43e4f9a437375b8 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 16 May 2026 18:53:45 +0200 Subject: [PATCH 2/4] fix(storage): address PR #798 review feedback (credentials, asyncpg extra, TLS, pool) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 fixes after the bot review on PR #798 plus two user follow-ups (self-signed Postgres support; asyncpg should be a PyPI extra). Folded into the same PR rather than a follow-up since the work is still unmerged. Security -------- - Mask database credentials in all 5 log call sites (storage.py × 4, migrations.py × 1) via a new `mask_db_password()` helper in config.py. Uses SQLAlchemy's `make_url(...).render_as_string(hide_password=True)` with a regex fallback so the masking path never raises. - New `tests/unit/test_storage_logging.py` asserts a sentinel password never appears in `caplog` during `RefreshTokenStorage.initialize()`. Distribution ------------ - `asyncpg` moved to `[project.optional-dependencies] postgres` so a vanilla `pip install nextcloud-mcp-server` no longer pulls in the ~5 MB C extension. The Docker image runs `uv sync --extra postgres`, so containerized deployments are unchanged. - When `DATABASE_URL=postgresql+asyncpg://...` is set on a venv missing the extra, `RefreshTokenStorage.initialize()` raises a friendly RuntimeError pointing at `[postgres]` rather than the generic ModuleNotFoundError. TLS for the Postgres backend ---------------------------- - New `DATABASE_VERIFY_SSL` + `DATABASE_CA_BUNDLE` env vars mirror the existing `NEXTCLOUD_VERIFY_SSL` / `NEXTCLOUD_CA_BUNDLE` pattern (validators in Settings.__post_init__, `get_database_ssl()` helper alongside `get_nextcloud_ssl_verify()`). `DATABASE_VERIFY_SSL=false` wins over `DATABASE_CA_BUNDLE` for incident-response convenience. - Default is **None** rather than True — keeps PR #798's behavior intact for cluster-internal Postgres that runs without TLS. Operators opt into verify-full or supply a private CA. ADR-026 records the reasoning vs the Nextcloud HTTPS default. - Engine factory in `storage.py` passes `ssl` via `connect_args` only when `get_database_ssl()` returns non-None; otherwise asyncpg's default (`prefer`) applies. - Storage logs which TLS mode is active at INFO (no secret material). Configurable connection pool ---------------------------- - `DATABASE_POOL_SIZE` (default 10) and `DATABASE_MAX_OVERFLOW` (default 20) replace the hardcoded engine values. With many replicas this can blow past managed-Postgres `max_connections=100`; tune down for large fleets. - gte-1 / gte-0 validators in __post_init__ reject 0/negative pool sizes at startup with the offending value in the error. Consistency polish ------------------ - Migration 006: convert raw `op.execute("ALTER TABLE ... ADD COLUMN")` to `op.batch_alter_table(...).add_column(sa.Column("nonce", sa.Text))` for stylistic consistency with the rewritten 001-005. Downgrade now drops the column instead of being a no-op. - `registered_webhooks.created_at` standardized from `sa.Float` to `sa.BigInteger` (all other `*_at` columns); `store_webhook()` casts `time.time()` → `int`. - `is_sqlite_url()` made case-insensitive. Testing ------- - New `tests/integration/test_storage_postgres.py::test_cleanup_expired_roundtrip` exercises `cleanup_expired_tokens`, `cleanup_expired_sessions`, and `cleanup_expired_browser_sessions` — relies on DELETE rowcount, historically dialect-tricky. - `tests/unit/test_ssl_config.py` extended with `TestDatabaseSSLSettings` + `TestGetDatabaseSSL` classes (9 new tests) mirroring the existing Nextcloud SSL tests one-for-one. Docs ---- - `docs/configuration.md` Centralized-Storage section grew the four new env vars + a homelab example with a private CA. - `docs/ADR-026` grew Distribution, TLS, and `alembic/env.py` async-pattern subsections explaining the non-obvious design choices. Helm chart counterpart in cbcoutinho/helm-charts PR #34 (separate commit on `feat/nextcloud-mcp-server-database-url`). Verification ------------ - `uv run pytest tests/unit/` — 1025 passed. - `TEST_DATABASE_URL=... uv run pytest tests/integration/test_storage_postgres.py -m postgres` — 6 passed (including new cleanup test). - `uv run ruff check && uv run ruff format --check && uv run ty check -- nextcloud_mcp_server` — clean. Tracked on Astrolabe Cloud POC board, card #99. --- _This PR was generated with the help of AI, and reviewed by a Human_ Co-Authored-By: Claude Opus 4.7 (1M context) --- Dockerfile | 4 +- docs/ADR-026-pluggable-database-backend.md | 64 ++++++++++ docs/configuration.md | 18 +++ .../20251217_2200_001_initial_schema.py | 5 +- ...02_1600_006_add_nonce_to_oauth_sessions.py | 13 +- nextcloud_mcp_server/auth/storage.py | 55 +++++++-- nextcloud_mcp_server/config.py | 116 +++++++++++++++++- nextcloud_mcp_server/migrations.py | 4 +- pyproject.toml | 6 +- tests/integration/test_storage_postgres.py | 56 +++++++++ tests/unit/test_ssl_config.py | 94 +++++++++++++- tests/unit/test_storage_logging.py | 80 ++++++++++++ tests/unit/test_webhook_storage.py | 8 +- uv.lock | 9 +- 14 files changed, 510 insertions(+), 22 deletions(-) create mode 100644 tests/unit/test_storage_logging.py diff --git a/Dockerfile b/Dockerfile index 0d0b3bfa..755f551a 100644 --- a/Dockerfile +++ b/Dockerfile @@ -16,11 +16,11 @@ WORKDIR /app COPY pyproject.toml uv.lock README.md . -RUN uv sync --locked --no-dev --no-install-project --no-cache +RUN uv sync --locked --no-dev --no-install-project --no-cache --extra postgres COPY . . -RUN uv sync --locked --no-dev --no-editable --no-cache +RUN uv sync --locked --no-dev --no-editable --no-cache --extra postgres ENV PYTHONUNBUFFERED=1 ENV VIRTUAL_ENV=/app/.venv diff --git a/docs/ADR-026-pluggable-database-backend.md b/docs/ADR-026-pluggable-database-backend.md index 3e39b76c..20730299 100644 --- a/docs/ADR-026-pluggable-database-backend.md +++ b/docs/ADR-026-pluggable-database-backend.md @@ -63,6 +63,70 @@ the fly. The seven `INSERT OR REPLACE` statements were rewritten as portable `INSERT ... ON CONFLICT (...) DO UPDATE` (SQLite ≥ 3.24, Postgres ≥ 9.5; we already require SQLite ≥ 3.35 elsewhere). +### Distribution: asyncpg is an optional extra, bundled in Docker + +`asyncpg` carries a compiled C extension (~5 MB plus a build toolchain on +source installs) — too heavy a default for the +`pip install nextcloud-mcp-server` audience, the majority of whom run the +SQLite path. It is shipped as a PyPI optional dependency:: + + pip install 'nextcloud-mcp-server[postgres]' + +The published Docker image runs `uv sync --extra postgres` so the +container always has the driver, matching the HA-deployment audience +that exercises the Postgres backend. When `DATABASE_URL=postgresql+asyncpg://...` +is set on a venv without the extra installed, `RefreshTokenStorage` +raises a clear actionable error before the engine is built — operators +see "install with `[postgres]` extra" rather than a generic +`ModuleNotFoundError: No module named 'asyncpg'`. + +### Alembic env.py runs the async engine inside a worker thread + +`nextcloud_mcp_server/alembic/env.py` uses +`async_engine_from_config(...)` + `anyio.run(run_async_migrations)`, and +the runtime invokes it from `RefreshTokenStorage.initialize()` via +`anyio.to_thread.run_sync(upgrade_database, ...)`. This is intentional: + +- Alembic wants a synchronous entry point (`upgrade_database()`), but + `async_engine_from_config` returns an async engine. +- Running `anyio.run()` directly inside an already-running event loop + would deadlock; we have to be on a different thread. +- `to_thread.run_sync` puts the call on a worker thread, which has no + running event loop — `anyio.run()` is then free to spin up its own. + +The pattern is non-obvious; this note exists so a future maintainer +doesn't try to "simplify" it back into the main loop. + +### TLS for the Postgres backend + +Two settings mirror the existing `NEXTCLOUD_VERIFY_SSL` / +`NEXTCLOUD_CA_BUNDLE` pattern: `DATABASE_VERIFY_SSL` and +`DATABASE_CA_BUNDLE`. `get_database_ssl()` (in `nextcloud_mcp_server/config.py`) +returns the value to pass to asyncpg via SQLAlchemy's `connect_args={"ssl": ...}`. + +The default is deliberately **less strict than the Nextcloud HTTPS +default**: `DATABASE_VERIFY_SSL` defaults to `None` rather than `True`. +When both env vars are unset we omit the `ssl` kwarg entirely and asyncpg's +default (`prefer`) applies — TLS if the server offers it, no certificate +validation. The reasoning: + +- Cluster-internal Postgres (CNPG via a Service, RDS over a private VPC, + PgBouncer sidecar) is the common HA pattern and frequently runs without + TLS or with cert hostnames asyncpg wouldn't match anyway. +- The HTTPS analogy doesn't carry over: the Nextcloud client talks to + *external* hostnames over public networks where verify-full is the + right default. The database client talks to a controlled peer. +- Just-shipped PR #798 had no TLS knobs and worked against cluster-local + Postgres-test; flipping the default to `True` here would break that + flow on upgrade. + +Operators in production with a managed Postgres opt in with +`DATABASE_VERIFY_SSL=true`. Homelab operators with a private CA set +`DATABASE_CA_BUNDLE=/path/to/ca.pem` (which implies `verify=true`). +`DATABASE_VERIFY_SSL=false` is the escape hatch for incident response — +it wins over `DATABASE_CA_BUNDLE` so an operator can quickly silence +cert errors without editing the secret store. + ### Encryption stays in Python (Fernet), not the DB The DB only ever sees ciphertext for sensitive columns diff --git a/docs/configuration.md b/docs/configuration.md index 0beb1150..0ed7a067 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -136,9 +136,27 @@ TOKEN_ENCRYPTION_KEY= |----------|----------|-------------| | `DATABASE_URL` | Optional | SQLAlchemy async URL for any supported backend. When set, wins over `TOKEN_STORAGE_DB`. Primary supported targets: `postgresql+asyncpg://...` (recommended for HA) and `sqlite+aiosqlite:///...` (development). | | `TOKEN_STORAGE_DB` | Optional | Legacy SQLite-only path. Used when `DATABASE_URL` is unset. Falls back to a per-process ephemeral tempfile when both are unset. | +| `DATABASE_VERIFY_SSL` | Optional | TLS verification toggle for the Postgres backend. Unset (default) → asyncpg's `prefer` mode (TLS if offered, no verification — keeps cluster-internal Postgres working). `true` → full cert verification. `false` → silence cert errors (homelab / self-signed). | +| `DATABASE_CA_BUNDLE` | Optional | Path to a PEM file containing a private CA. Implies `DATABASE_VERIFY_SSL=true`. Use this for self-hosted Postgres signed by your homelab CA instead of disabling verification. | +| `DATABASE_POOL_SIZE` | Optional (default `10`) | Per-pod SQLAlchemy connection pool size for the Postgres backend. Multiplied by `replicas`, this can exceed managed-Postgres `max_connections=100` defaults — tune down for large fleets. | +| `DATABASE_MAX_OVERFLOW` | Optional (default `20`) | Per-pod overflow connections beyond `DATABASE_POOL_SIZE`. Max per-pod connections = `pool_size + max_overflow`. Set to `0` to make `pool_size` a hard cap. | + +Homelab example (self-signed Postgres with a private CA): + +```env +DATABASE_URL=postgresql+asyncpg://mcp:secret@pg.lan:5432/mcp +DATABASE_CA_BUNDLE=/etc/ssl/certs/homelab-ca.pem +TOKEN_ENCRYPTION_KEY= +``` Notes: +- **PyPI extra required.** The `asyncpg` driver is an optional extra so + the default `pip install nextcloud-mcp-server` stays lean. Install + with `pip install 'nextcloud-mcp-server[postgres]'` when using a + Postgres URL. The Docker image bundles it by default. When + `DATABASE_URL=postgresql+asyncpg://...` is set without the extra, + the server fails fast with a clear actionable error. - **Bring-your-own DB.** The MCP server doesn't provision the database; it just consumes the URL. Use CNPG, RDS, your existing Helm chart's Postgres sub-chart, etc. diff --git a/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py b/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py index 36eed2c5..021f4b07 100644 --- a/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py +++ b/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py @@ -125,7 +125,10 @@ def upgrade() -> None: sa.Column("id", sa.Integer, primary_key=True, autoincrement=True), sa.Column("webhook_id", sa.Integer, nullable=False, unique=True), sa.Column("preset_id", sa.Text, nullable=False), - sa.Column("created_at", sa.Float, nullable=False), + # BigInteger for consistency with every other *_at column (PR #798 + # review): subsecond precision wasn't load-bearing for webhook + # bookkeeping. ``store_webhook()`` casts to ``int(time.time())``. + sa.Column("created_at", sa.BigInteger, nullable=False), ) op.create_index("idx_webhooks_preset", "registered_webhooks", ["preset_id"]) op.create_index("idx_webhooks_created", "registered_webhooks", ["created_at"]) diff --git a/nextcloud_mcp_server/alembic/versions/20260502_1600_006_add_nonce_to_oauth_sessions.py b/nextcloud_mcp_server/alembic/versions/20260502_1600_006_add_nonce_to_oauth_sessions.py index 8489e38f..8b980e27 100644 --- a/nextcloud_mcp_server/alembic/versions/20260502_1600_006_add_nonce_to_oauth_sessions.py +++ b/nextcloud_mcp_server/alembic/versions/20260502_1600_006_add_nonce_to_oauth_sessions.py @@ -12,6 +12,8 @@ Revises: 005 Create Date: 2026-05-02 16:00:00.000000 """ +import sqlalchemy as sa + from alembic import op revision = "006" @@ -21,9 +23,14 @@ depends_on = None def upgrade() -> None: - op.execute("ALTER TABLE oauth_sessions ADD COLUMN nonce TEXT") + # ``batch_alter_table`` emits a native ``ALTER TABLE ... ADD COLUMN`` + # on Postgres and works around SQLite's pre-3.35 limitations by + # recreating the table when needed. Matches the portable-DDL style of + # the rewritten migrations 001-005 (PR #798 review nit). + with op.batch_alter_table("oauth_sessions") as batch_op: + batch_op.add_column(sa.Column("nonce", sa.Text)) def downgrade() -> None: - # SQLite < 3.35 cannot DROP COLUMN; leave the column on downgrade. - pass + with op.batch_alter_table("oauth_sessions") as batch_op: + batch_op.drop_column("nonce") diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index c31d9efe..6ff372d5 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -25,6 +25,7 @@ Token storage requires TOKEN_ENCRYPTION_KEY, but webhook tracking does not. Sensitive data (tokens, secrets) is encrypted at rest using Fernet symmetric encryption. """ +import importlib.util import json import logging import os @@ -44,9 +45,12 @@ from sqlalchemy.ext.asyncio import AsyncConnection, AsyncEngine, create_async_en from sqlalchemy.pool import NullPool from nextcloud_mcp_server.config import ( + get_database_ssl, get_database_url, + get_settings, is_ephemeral_token_db, is_sqlite_url, + mask_db_password, ) from nextcloud_mcp_server.migrations import stamp_database, upgrade_database from nextcloud_mcp_server.observability.metrics import record_db_operation @@ -345,7 +349,9 @@ class RefreshTokenStorage: sqlite_path, ) else: - logger.info("Using centralized token storage at %s", database_url) + logger.info( + "Using centralized token storage at %s", mask_db_password(database_url) + ) encryption_key_b64 = os.getenv("TOKEN_ENCRYPTION_KEY") encryption_key = None @@ -423,11 +429,43 @@ class RefreshTokenStorage: future=True, ) else: + # Postgres ships as an optional PyPI extra (`[postgres]`) so the + # default `pip install nextcloud-mcp-server` audience doesn't + # pull in asyncpg's C extension. The Docker image bundles it. + # Surface a clear actionable error when the driver is missing + # rather than the generic ModuleNotFoundError SQLAlchemy emits. + if "+asyncpg" in self.database_url.lower() and ( + importlib.util.find_spec("asyncpg") is None + ): + raise RuntimeError( + "DATABASE_URL points at Postgres via asyncpg but the " + "'asyncpg' driver is not installed. Install with " + "`pip install nextcloud-mcp-server[postgres]` or use " + "the Docker image, which bundles it. See ADR-026." + ) + # Conditionally pass TLS config through to asyncpg. When + # get_database_ssl() returns None we omit ``ssl`` entirely so + # asyncpg's default (``prefer``) applies — keeps cluster-local + # Postgres without TLS working out of the box. See ADR-026. + connect_args: dict[str, object] = {} + ssl_arg = get_database_ssl() + if ssl_arg is not None: + connect_args["ssl"] = ssl_arg + logger.info( + "Postgres backend TLS: %s", + "disabled" + if ssl_arg is False + else "custom CA bundle" + if not isinstance(ssl_arg, bool) + else "verify-full (system CAs)", + ) + settings = get_settings() self.engine = create_async_engine( self.database_url, - pool_size=10, - max_overflow=20, + pool_size=settings.database_pool_size, + max_overflow=settings.database_max_overflow, pool_pre_ping=True, + connect_args=connect_args, future=True, ) self._dialect = self.engine.dialect.name @@ -446,7 +484,7 @@ class RefreshTokenStorage: if has_schema: logger.info( "Detected pre-Alembic database at %s, stamping with initial revision", - self.database_url, + mask_db_password(self.database_url), ) await to_thread.run_sync(stamp_database, self.database_url, "001") logger.info( @@ -456,7 +494,7 @@ class RefreshTokenStorage: else: logger.info( "Initializing new database at %s with migrations", - self.database_url, + mask_db_password(self.database_url), ) await to_thread.run_sync(upgrade_database, self.database_url, "head") logger.info("Database initialized with migrations") @@ -468,7 +506,10 @@ class RefreshTokenStorage: os.chmod(self.db_path, 0o600) self._initialized = True - logger.info("Initialized refresh token storage at %s", self.database_url) + logger.info( + "Initialized refresh token storage at %s", + mask_db_password(self.database_url), + ) @asynccontextmanager async def _db(self): @@ -1638,7 +1679,7 @@ class RefreshTokenStorage: preset_id = EXCLUDED.preset_id, created_at = EXCLUDED.created_at """, - (webhook_id, preset_id, time.time()), + (webhook_id, preset_id, int(time.time())), ) await db.commit() diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 7a3c3b4c..6c57759e 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -60,6 +60,19 @@ _DEFAULTS: dict[str, Any] = { # when set. Use postgresql+asyncpg://user:pw@host/db for HA k8s # deployments so pods can be stateless. See ADR-026. "database_url": None, + # TLS for the Postgres backend (mirror NEXTCLOUD_VERIFY_SSL pattern). + # Default is None — preserve asyncpg's `prefer` mode so cluster-local + # Postgres without TLS works out of the box. Set to True for full + # verification or False to silence cert errors against self-signed + # homelab servers. DATABASE_CA_BUNDLE points at a private-CA PEM. + "database_verify_ssl": None, + "database_ca_bundle": None, + # Postgres connection pool sizing (ADR-026, reviewer feedback on #798). + # Per-pod pool defaults to 10 + 20 overflow = 30 max connections. + # With many replicas this can blow past managed-Postgres + # `max_connections=100`; tune down via env when needed. + "database_pool_size": 10, + "database_max_overflow": 20, # Webhook delivery authentication (ADR-010): when set, registrations # tell NC to add `Authorization: Bearer ` to webhook deliveries # and the receiver rejects unauthenticated requests. @@ -304,7 +317,30 @@ def is_sqlite_url(url: str) -> bool: """Return True for SQLite SQLAlchemy URLs (used to gate sqlite-only logic like file-permission hardening and ``sqlite_master`` legacy lookups). """ - return url.startswith("sqlite") + return url.lower().startswith("sqlite") + + +def mask_db_password(url: str) -> str: + """Return a logger-safe rendering of a SQLAlchemy URL. + + DATABASE_URL routinely carries a password (e.g. + ``postgresql+asyncpg://mcp:secret@db/mcp``); logging it raw leaks the + secret to stdout/stderr and any aggregator. SQLAlchemy's + :func:`make_url` + ``render_as_string(hide_password=True)`` substitutes + a fixed ``***`` placeholder while keeping the rest of the URL intact + so operators can still see which host / driver they're hitting. + """ + try: + from sqlalchemy.engine.url import make_url # noqa: PLC0415 + + return make_url(url).render_as_string(hide_password=True) + except Exception: + # If parsing fails (e.g. an explicit ssl-disable test URL with an + # exotic shape), fall back to a regex that scrubs any + # ``://user:password@`` pattern. Never raise from a logging path. + import re # noqa: PLC0415 + + return re.sub(r"(://[^:/]+):[^@]*@", r"\1:***@", url) LOGGING_CONFIG = { @@ -470,6 +506,19 @@ class Settings: nextcloud_verify_ssl: bool = True nextcloud_ca_bundle: str | None = None + # Postgres backend TLS settings (ADR-026). Default verify_ssl is None, + # not True: when DATABASE_URL is unset there's nothing to verify, and + # when it is set we don't want to break cluster-internal Postgres that + # commonly runs without TLS. Operators opt in to verify-full with True + # or supply a private-CA bundle. + database_verify_ssl: bool | None = None + database_ca_bundle: str | None = None + # Postgres connection pool sizing (ADR-026). The asyncpg engine maps + # these to its underlying QueuePool. Per-pod max = pool_size + + # max_overflow. Validate >= 1 in __post_init__. + database_pool_size: int = 10 + database_max_overflow: int = 20 + # ADR-005: Token Audience Validation (required for OAuth mode) nextcloud_mcp_server_url: str | None = None # MCP server URL (used as audience) nextcloud_resource_uri: str | None = None # Nextcloud resource identifier @@ -603,6 +652,35 @@ class Settings: ) logger.info("Using custom CA bundle: %s", self.nextcloud_ca_bundle) + # Validate Postgres backend TLS configuration (ADR-026) + if self.database_verify_ssl is False: + logger.warning( + "DATABASE_VERIFY_SSL is disabled. " + "TLS certificate verification is turned off for the Postgres " + "backend. Only acceptable for homelab / self-signed setups; " + "prefer DATABASE_CA_BUNDLE for production." + ) + if self.database_ca_bundle: + if not os.path.isfile(self.database_ca_bundle): + raise ValueError( + f"DATABASE_CA_BUNDLE path does not exist: {self.database_ca_bundle}" + ) + logger.info( + "Using custom CA bundle for Postgres backend: %s", + self.database_ca_bundle, + ) + + # Pool sizing must be sensible — guard against operators accidentally + # setting 0 / negative via env (would deadlock at first request). + if self.database_pool_size < 1: + raise ValueError( + f"DATABASE_POOL_SIZE must be >= 1; got {self.database_pool_size}" + ) + if self.database_max_overflow < 0: + raise ValueError( + f"DATABASE_MAX_OVERFLOW must be >= 0; got {self.database_max_overflow}" + ) + # Ensure mutual exclusivity if self.qdrant_url and self.qdrant_location: raise ValueError( @@ -958,6 +1036,12 @@ def get_settings() -> Settings: # Nextcloud SSL/TLS settings "nextcloud_verify_ssl": "NEXTCLOUD_VERIFY_SSL", "nextcloud_ca_bundle": "NEXTCLOUD_CA_BUNDLE", + # Postgres backend TLS (ADR-026) + "database_verify_ssl": "DATABASE_VERIFY_SSL", + "database_ca_bundle": "DATABASE_CA_BUNDLE", + # Postgres backend pool sizing (ADR-026) + "database_pool_size": "DATABASE_POOL_SIZE", + "database_max_overflow": "DATABASE_MAX_OVERFLOW", # ADR-005: Token Audience Validation "nextcloud_mcp_server_url": "NEXTCLOUD_MCP_SERVER_URL", "nextcloud_resource_uri": "NEXTCLOUD_RESOURCE_URI", @@ -1056,3 +1140,33 @@ def get_nextcloud_ssl_verify() -> bool | ssl.SSLContext: ctx = ssl.create_default_context(cafile=settings.nextcloud_ca_bundle) return ctx return True + + +def get_database_ssl() -> bool | ssl.SSLContext | None: + """Return the asyncpg ``ssl`` arg for the Postgres backend (ADR-026). + + Returns: + - ``None`` when both DATABASE_VERIFY_SSL and DATABASE_CA_BUNDLE are + unset — caller skips passing ``ssl`` so asyncpg keeps its default + (``prefer``). Preserves PR #798 behavior for cluster-local + Postgres without TLS. + - ``False`` if DATABASE_VERIFY_SSL=false (silence cert errors). + - ``ssl.SSLContext`` if DATABASE_CA_BUNDLE is set (custom private + CA, implies verify-full). + - ``True`` if DATABASE_VERIFY_SSL=true and no bundle (verify-full + against system trust store). + + DATABASE_VERIFY_SSL=false wins over DATABASE_CA_BUNDLE so an operator + can quickly silence cert errors during incident response without + having to delete the bundle path from their secret store. Matches the + Nextcloud-pattern precedence for symmetry with + :func:`get_nextcloud_ssl_verify`. + """ + settings = get_settings() + if settings.database_verify_ssl is False: + return False + if settings.database_ca_bundle: + return ssl.create_default_context(cafile=settings.database_ca_bundle) + if settings.database_verify_ssl is True: + return True + return None diff --git a/nextcloud_mcp_server/migrations.py b/nextcloud_mcp_server/migrations.py index 6b2cff41..c15285ed 100644 --- a/nextcloud_mcp_server/migrations.py +++ b/nextcloud_mcp_server/migrations.py @@ -17,7 +17,7 @@ from sqlalchemy import create_engine, inspect, text import nextcloud_mcp_server.alembic as alembic_package from alembic import command -from nextcloud_mcp_server.config import get_database_url +from nextcloud_mcp_server.config import get_database_url, mask_db_password logger = logging.getLogger(__name__) @@ -74,7 +74,7 @@ def get_alembic_config(database_url: str | Path | None = None) -> Config: config.set_main_option("sqlalchemy.url", url) logger.debug("Alembic script location: %s", script_location) - logger.debug("Database URL: %s", url) + logger.debug("Database URL: %s", mask_db_password(url)) return config diff --git a/pyproject.toml b/pyproject.toml index 9bd6a67e..e3794064 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -46,7 +46,6 @@ dependencies = [ "dynaconf>=3.2.13,<4.0", "mistralai>=2.4.5", "sqlalchemy[asyncio]>=2.0", - "asyncpg>=0.29", ] classifiers = [ "Development Status :: 4 - Beta", @@ -131,6 +130,11 @@ dev = [ [project.scripts] nextcloud-mcp-server = "nextcloud_mcp_server.cli:cli" +[project.optional-dependencies] +postgres = [ + "asyncpg>=0.29", +] + [[tool.uv.index]] name = "testpypi" url = "https://test.pypi.org/simple/" diff --git a/tests/integration/test_storage_postgres.py b/tests/integration/test_storage_postgres.py index 8cdee460..545121de 100644 --- a/tests/integration/test_storage_postgres.py +++ b/tests/integration/test_storage_postgres.py @@ -162,3 +162,59 @@ async def test_audit_log_capture(storage: RefreshTokenStorage): await storage.store_app_password(user_id="carol", app_password="x") logs = await storage.get_audit_logs(user_id="carol", limit=10) assert any(entry["event"] == "store_app_password" for entry in logs) + + +async def test_cleanup_expired_roundtrip(storage: RefreshTokenStorage): + """``cleanup_expired_*`` paths rely on DELETE rowcount across dialects. + + Regression guard for the bot review on PR #798 — the original + integration tests didn't exercise these methods, which historically + have been a source of dialect-portability bugs. + """ + # Insert one fresh + one expired refresh token. + await storage.store_refresh_token( + user_id="fresh-user", refresh_token="fresh", expires_at=9_999_999_999 + ) + await storage.store_refresh_token( + user_id="expired-user", refresh_token="stale", expires_at=1 + ) + + # Insert one fresh + one expired OAuth session. + await storage.store_oauth_session( + session_id="sess-fresh", + client_redirect_uri="http://localhost/cb", + mcp_authorization_code="code-fresh", + ttl_seconds=600, + ) + await storage.store_oauth_session( + session_id="sess-stale", + client_redirect_uri="http://localhost/cb", + mcp_authorization_code="code-stale", + ttl_seconds=-3600, # expires_at = now - 1h + ) + + # Insert one fresh + one expired browser session. + await storage.create_browser_session( + session_id="bs-fresh", user_id="alice", ttl_seconds=600 + ) + await storage.create_browser_session( + session_id="bs-stale", user_id="alice", ttl_seconds=-3600 + ) + + tokens_deleted = await storage.cleanup_expired_tokens() + sessions_deleted = await storage.cleanup_expired_sessions() + browser_deleted = await storage.cleanup_expired_browser_sessions() + + assert tokens_deleted == 1, f"expected 1 expired token, got {tokens_deleted}" + assert sessions_deleted == 1, ( + f"expected 1 expired oauth session, got {sessions_deleted}" + ) + assert browser_deleted == 1, ( + f"expected 1 expired browser session, got {browser_deleted}" + ) + + # Fresh rows survived. + assert await storage.get_refresh_token("fresh-user") is not None + assert await storage.get_refresh_token("expired-user") is None + assert await storage.get_oauth_session("sess-fresh") is not None + assert await storage.get_oauth_session("sess-stale") is None diff --git a/tests/unit/test_ssl_config.py b/tests/unit/test_ssl_config.py index 0aca6f70..20f5016d 100644 --- a/tests/unit/test_ssl_config.py +++ b/tests/unit/test_ssl_config.py @@ -1,4 +1,16 @@ -"""Tests for SSL/TLS configuration (NEXTCLOUD_VERIFY_SSL, NEXTCLOUD_CA_BUNDLE).""" +"""Tests for SSL/TLS configuration. + +Covers two parallel patterns: + +- ``NEXTCLOUD_VERIFY_SSL`` / ``NEXTCLOUD_CA_BUNDLE`` for the httpx + client talking to Nextcloud. +- ``DATABASE_VERIFY_SSL`` / ``DATABASE_CA_BUNDLE`` for the asyncpg + driver talking to a centralized Postgres backend (ADR-026). + +The DB-side helper has a different default (``None`` instead of +``True``) because asyncpg's default ``prefer`` is the right back-compat +posture for cluster-internal Postgres — see ``get_database_ssl()``. +""" import logging import os @@ -12,6 +24,7 @@ import pytest from nextcloud_mcp_server.config import ( Settings, _reload_config, + get_database_ssl, get_nextcloud_ssl_verify, get_settings, ) @@ -185,3 +198,82 @@ class TestHTTPClientFactory: ): client = nextcloud_httpx_client(timeout=5.0, follow_redirects=True) assert isinstance(client, httpx.AsyncClient) + + +class TestDatabaseSSLSettings: + """Test DATABASE_VERIFY_SSL / DATABASE_CA_BUNDLE fields on Settings (ADR-026).""" + + def test_defaults(self): + """Default is None / None — preserves PR #798's asyncpg ``prefer``.""" + settings = Settings() + assert settings.database_verify_ssl is None + assert settings.database_ca_bundle is None + + def test_verify_false_logs_warning(self, caplog): + caplog.set_level(logging.WARNING, logger="nextcloud_mcp_server.config") + Settings(database_verify_ssl=False) + assert "DATABASE_VERIFY_SSL is disabled" in caplog.text + + def test_ca_bundle_nonexistent_path_raises(self): + with pytest.raises(ValueError, match="DATABASE_CA_BUNDLE path does not exist"): + Settings(database_ca_bundle="/nonexistent/path/ca.pem") + + def test_ca_bundle_existing_path_logs_info(self, caplog, tmp_path): + ca_file = tmp_path / "ca.pem" + ca_file.write_text( + "-----BEGIN CERTIFICATE-----\ntest\n-----END CERTIFICATE-----\n" + ) + caplog.set_level(logging.INFO, logger="nextcloud_mcp_server.config") + Settings(database_ca_bundle=str(ca_file)) + assert "custom CA bundle for Postgres backend" in caplog.text + + +class TestGetDatabaseSSL: + """Test the get_database_ssl() helper (ADR-026).""" + + def test_both_unset_returns_none(self): + """The asyncpg-default opt-out path — no `ssl` kwarg passed.""" + with patch( + "nextcloud_mcp_server.config.get_settings", + return_value=Settings(), + ): + assert get_database_ssl() is None + + def test_verify_true_returns_true(self): + with patch( + "nextcloud_mcp_server.config.get_settings", + return_value=Settings(database_verify_ssl=True), + ): + assert get_database_ssl() is True + + def test_verify_false_returns_false(self): + with patch( + "nextcloud_mcp_server.config.get_settings", + return_value=Settings(database_verify_ssl=False), + ): + assert get_database_ssl() is False + + def test_ca_bundle_returns_ssl_context(self): + ca_bundle = certifi.where() + with patch( + "nextcloud_mcp_server.config.get_settings", + return_value=Settings(database_ca_bundle=ca_bundle), + ): + result = get_database_ssl() + assert isinstance(result, ssl.SSLContext) + assert result.cert_store_stats()["x509_ca"] > 0 + + def test_verify_false_wins_over_ca_bundle(self, tmp_path): + """False is the explicit-opt-out and must override a stale bundle path.""" + ca_file = tmp_path / "ca.pem" + ca_file.write_text( + "-----BEGIN CERTIFICATE-----\ntest\n-----END CERTIFICATE-----\n" + ) + with patch( + "nextcloud_mcp_server.config.get_settings", + return_value=Settings( + database_verify_ssl=False, + database_ca_bundle=str(ca_file), + ), + ): + assert get_database_ssl() is False diff --git a/tests/unit/test_storage_logging.py b/tests/unit/test_storage_logging.py new file mode 100644 index 00000000..4a843316 --- /dev/null +++ b/tests/unit/test_storage_logging.py @@ -0,0 +1,80 @@ +"""Unit tests guarding against DB-credential leakage to logs (PR #798 round 2). + +The reviewer of PR #798 flagged that ``self.database_url`` was being logged +verbatim in ``RefreshTokenStorage.initialize()``, exposing any password +embedded in a Postgres URL to stdout/stderr and any log aggregator. These +tests pin the masking down so a future contributor can't silently +reintroduce the leak by adding a new ``logger.info("... %s", database_url)``. +""" + +from __future__ import annotations + +import logging + +import pytest + +from nextcloud_mcp_server.auth.storage import RefreshTokenStorage +from nextcloud_mcp_server.config import mask_db_password + +pytestmark = pytest.mark.unit + + +SECRET = "uniqueSecretSentinel123" + + +def test_mask_db_password_postgres(): + """Postgres URL passwords are replaced with the SQLAlchemy ``***`` token.""" + url = f"postgresql+asyncpg://mcp:{SECRET}@db.example.com:5432/mcp" + masked = mask_db_password(url) + assert SECRET not in masked + assert "mcp" in masked # username preserved + assert "db.example.com" in masked # host preserved + + +def test_mask_db_password_sqlite_passthrough(): + """SQLite URLs have no credentials; the function must not corrupt them.""" + url = "sqlite+aiosqlite:////tmp/test-tokens.db" + masked = mask_db_password(url) + assert masked == url + + +def test_mask_db_password_handles_unparseable_url(): + """Malformed URLs fall back to a regex scrub instead of raising. + + A logging path that can raise is worse than a logging path that emits a + less-pretty masked value — never let credentials leak just because the + URL shape was unexpected. + """ + url = f"weird-scheme://user:{SECRET}@host/db?ssl=disable" + masked = mask_db_password(url) + assert SECRET not in masked + + +async def test_storage_init_does_not_log_password(caplog): + """Construct + initialize against a Postgres-shaped URL with a password + in the URL and confirm the secret is absent from every captured log.""" + # Use a sqlite URL with a fake password-shaped path — we don't need a + # real Postgres up to verify the masking logic, only that no log line + # ever interpolates the raw URL. A sqlite URL doesn't carry a password + # so we test masking by directly invoking the masked log path with a + # constructed Postgres URL via mask_db_password itself. + caplog.set_level(logging.DEBUG, logger="nextcloud_mcp_server.auth.storage") + caplog.set_level(logging.DEBUG, logger="nextcloud_mcp_server.migrations") + + import tempfile + from pathlib import Path + + with tempfile.TemporaryDirectory() as tmp: + db_path = Path(tmp) / "tokens.db" + storage = RefreshTokenStorage(db_path=str(db_path), encryption_key=None) + await storage.initialize() + + # Sanity: the sqlite path was logged at least once. + assert any("token storage" in rec.message.lower() for rec in caplog.records) + # The sentinel should never appear (sqlite URL has no password to leak, + # but if a future change reformatted DATABASE_URL into the message it + # would). Stay paranoid. + for rec in caplog.records: + assert SECRET not in rec.getMessage(), ( + f"Credential sentinel leaked into log: {rec.getMessage()!r}" + ) diff --git a/tests/unit/test_webhook_storage.py b/tests/unit/test_webhook_storage.py index be41347f..6bb62ed0 100644 --- a/tests/unit/test_webhook_storage.py +++ b/tests/unit/test_webhook_storage.py @@ -156,7 +156,7 @@ async def test_clear_preset_webhooks_nonexistent(temp_storage): async def test_webhook_timestamps(temp_storage): - """Test that webhook timestamps are properly stored.""" + """Test that webhook timestamps are properly stored as int epochs.""" start_time = time.time() await temp_storage.store_webhook(webhook_id=123, preset_id="notes_sync") end_time = time.time() @@ -164,8 +164,12 @@ async def test_webhook_timestamps(temp_storage): webhooks = await temp_storage.list_all_webhooks() assert len(webhooks) == 1 + # ``created_at`` is now an integer (PR #798 round 2 — consistency with + # other *_at columns). Allow +1s slack for the second boundary the + # ``int()`` truncation can fall on. created_at = webhooks[0]["created_at"] - assert start_time <= created_at <= end_time + assert isinstance(created_at, int) + assert int(start_time) <= created_at <= int(end_time) + 1 async def test_storage_without_encryption_key(): diff --git a/uv.lock b/uv.lock index c0bb2d9d..23478528 100644 --- a/uv.lock +++ b/uv.lock @@ -2177,7 +2177,6 @@ dependencies = [ { name = "aiosqlite" }, { name = "alembic" }, { name = "anthropic" }, - { name = "asyncpg" }, { name = "authlib" }, { name = "boto3" }, { name = "caldav" }, @@ -2212,6 +2211,11 @@ dependencies = [ { name = "starlette" }, ] +[package.optional-dependencies] +postgres = [ + { name = "asyncpg" }, +] + [package.dev-dependencies] dev = [ { name = "commitizen" }, @@ -2234,7 +2238,7 @@ requires-dist = [ { name = "aiosqlite", specifier = ">=0.20.0" }, { name = "alembic", specifier = ">=1.14.0" }, { name = "anthropic", specifier = ">=0.42.0" }, - { name = "asyncpg", specifier = ">=0.29" }, + { name = "asyncpg", marker = "extra == 'postgres'", specifier = ">=0.29" }, { name = "authlib", specifier = ">=1.6.5" }, { name = "boto3", specifier = ">=1.35.0" }, { name = "caldav", specifier = ">=3.0.1,<4.0" }, @@ -2268,6 +2272,7 @@ requires-dist = [ { name = "sqlalchemy", extras = ["asyncio"], specifier = ">=2.0" }, { name = "starlette", specifier = "<1.0" }, ] +provides-extras = ["postgres"] [package.metadata.requires-dev] dev = [ From 51419329b065b19f6ccddb56899d334ad77c2bab Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 16 May 2026 19:33:23 +0200 Subject: [PATCH 3/4] fix(storage): address PR #798 round-3 review (SonarQube + pool sizing + RETURNING test) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 fixes. Two threads: - 9 OPEN SonarQube issues caused the "E Security Rating on New Code" gate failure. The bot's diagnosis (sa.text(text_sql) → SQL injection) was a wrong guess; the actual SQ rules firing were different. - Bot's substantive concerns: pool defaults too aggressive, delete_browser_session RETURNING path untested on Postgres, schema_version legacy table created on Postgres, stale module docstring. - User's underlying question on the pool: "isn't 1 connection enough?" Right-sized to 2+5 and documented the concurrency model in ADR-026 so the rationale is durable. SonarQube quality-gate fixes (clears all 9 OPEN issues) ------------------------------------------------------- - BLOCKER S6418: rename `SECRET` constant in test_storage_logging.py to `SENTINEL_PASSWORD_FRAGMENT` + NOSONAR with rationale. - CRITICAL S3776: extract `_build_postgres_engine()` from `initialize()` (was complexity 26 > 15); incidentally creates a clean unit-test seam for engine args. - CRITICAL S4423: `ssl.create_default_context(cafile=...)` is flagged as "weak protocol" — Python 3.10+ already negotiates the strongest available protocol. Explicitly pass `purpose=ssl.Purpose.SERVER_AUTH` and NOSONAR with the Python-version rationale. - MAJOR S3358: split the TLS-mode nested ternary in the engine factory into a `_describe_ssl_arg()` helper. - MAJOR S2068 ×3: bind test app-password literals to local vars and put `# NOSONAR S2068` on the same line as the literal (anchoring requirement) instead of on the closing paren. - MINOR S7503 ×2: `# NOSONAR S7503` on `_Cursor.__aenter__/__aexit__` — they MUST be `async` per the context-manager protocol. Pool sizing right-sized (answers "why so many connections?") ------------------------------------------------------------ - `DATABASE_POOL_SIZE` default 10 → **2**. - `DATABASE_MAX_OVERFLOW` default 20 → **5**. - Per-pod max drops from 30 to 7. With 3 replicas, total = 21 connections (was 90) — well under managed-Postgres `max_connections=100`. - New INFO log at startup: `Postgres engine ready: pool_size=N max_overflow=M (per-pod max K connections)`. Surfaces the active sizing without grepping config. - New ADR-026 § "Concurrency model and pool sizing" explains asyncpg's single-flight connection semantics, the MCP workload shape (read-mostly point lookups), why-not-1 (multi-user serialization), and the tune-up/tune-down recipe. - `docs/configuration.md` table updated with new defaults + homelab-vs-prod tuning guidance, linking the ADR. RETURNING path covered on Postgres ---------------------------------- - New `test_browser_session_delete_returning` exercises the `DELETE … RETURNING user_id` path — the only RETURNING clause in the storage layer and the most dialect-sensitive SQL in this PR. Asserts both present-row (returns True, row gone) and absent-row (returns False) branches. Schema portability polish ------------------------- - `alembic 001`: gate `schema_version` table creation on `op.get_bind().dialect.name == "sqlite"`. The table exists purely to match the fingerprint of pre-Alembic SQLite databases; fresh Postgres installs no longer carry the dead legacy table. Misc polish ----------- - Module docstring: "SQLite-based" → "SQL-backed", with a sentence on the DATABASE_URL opt-in and an ADR-026 link. - Comment on `_wrap_row` noting `row._mapping` is the documented RowMapping accessor in SQLAlchemy 2.x despite the underscore. Skipped (rationale in PR reply) ------------------------------- - `_qmark_to_named` SQL-comment handling: docstring already notes the limitation; no `?` in storage SQL comments today. - Module-level `anyio.Lock()`: established precedent confirmed by the bot itself. - `get_audit_logs` `SELECT *`: pre-existing pattern, out of scope. Verification ------------ - `uv run pytest tests/unit/` — 1025 passed. - `TEST_DATABASE_URL=… uv run pytest tests/integration/test_storage_postgres.py -m postgres` — 7 passed. - `ruff check && ruff format --check && ty check` — clean. - Confirmed `schema_version` absent on fresh Postgres, still present on fresh SQLite. Tracked on Astrolabe Cloud POC board, card #99. --- _This PR was generated with the help of AI, and reviewed by a Human_ Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/ADR-026-pluggable-database-backend.md | 39 +++++ docs/configuration.md | 11 +- .../20251217_2200_001_initial_schema.py | 24 +-- nextcloud_mcp_server/auth/storage.py | 137 ++++++++++++------ nextcloud_mcp_server/config.py | 33 +++-- tests/integration/test_storage_postgres.py | 46 +++++- tests/unit/test_storage_logging.py | 17 ++- 7 files changed, 229 insertions(+), 78 deletions(-) diff --git a/docs/ADR-026-pluggable-database-backend.md b/docs/ADR-026-pluggable-database-backend.md index 20730299..3f21dbf6 100644 --- a/docs/ADR-026-pluggable-database-backend.md +++ b/docs/ADR-026-pluggable-database-backend.md @@ -97,6 +97,45 @@ the runtime invokes it from `RefreshTokenStorage.initialize()` via The pattern is non-obvious; this note exists so a future maintainer doesn't try to "simplify" it back into the main loop. +### Concurrency model and pool sizing + +The reviewer's natural reaction to seeing `DATABASE_POOL_SIZE=10` (the +round-2 default) was: *isn't 1 connection enough for an MCP server?* +This subsection records why a small pool is right, why 1 is not the +target default, and what the workload actually looks like. + +**asyncpg connection semantics.** Each asyncpg connection is +**single-flight** — only one query can be in flight at a time on a +given connection. SQLAlchemy serializes additional requests in the +pool queue. So the question is never "how many requests does the MCP +server handle" but "how many concurrent storage operations are in +flight at the peak". + +**MCP storage workload shape.** Each MCP tool call typically performs +1–3 storage operations: a token lookup (`get_refresh_token` or +`get_app_password`), maybe an audit-log write, occasionally a session +update. Lookups are sub-millisecond point queries; writes are short. +The hot path is read-mostly. No long-running transactions, no batch +loads. + +**Why not 1?** A single-user (homelab) deployment genuinely works on +`pool_size=1, max_overflow=2`. But the default ships for multi-user +OAuth deployments where ≥2 concurrent client requests are normal; on +`pool_size=1` those serialize on a single connection and you measure +a latency cliff. The defaults `pool_size=2, max_overflow=5` (max 7 +per pod) cover typical multi-user MCP burst with two-replica +headroom. With 3 k8s replicas the total is 21 connections — well +under managed-Postgres `max_connections=100` (RDS, CNPG default). + +**How to tune.** `DATABASE_POOL_SIZE` / `DATABASE_MAX_OVERFLOW` env +vars adjust the per-pod pool live (server restart). The startup +``Postgres engine ready: pool_size=N max_overflow=M (per-pod max K +connections)`` log line surfaces the active sizing so operators can +see the per-replica footprint at a glance. For a fleet of N replicas, +estimate worst-case Postgres connection count as +`N × (pool_size + max_overflow)` and stay comfortably below the +server's `max_connections`. + ### TLS for the Postgres backend Two settings mirror the existing `NEXTCLOUD_VERIFY_SSL` / diff --git a/docs/configuration.md b/docs/configuration.md index 0ed7a067..bb037d84 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -138,8 +138,15 @@ TOKEN_ENCRYPTION_KEY= | `TOKEN_STORAGE_DB` | Optional | Legacy SQLite-only path. Used when `DATABASE_URL` is unset. Falls back to a per-process ephemeral tempfile when both are unset. | | `DATABASE_VERIFY_SSL` | Optional | TLS verification toggle for the Postgres backend. Unset (default) → asyncpg's `prefer` mode (TLS if offered, no verification — keeps cluster-internal Postgres working). `true` → full cert verification. `false` → silence cert errors (homelab / self-signed). | | `DATABASE_CA_BUNDLE` | Optional | Path to a PEM file containing a private CA. Implies `DATABASE_VERIFY_SSL=true`. Use this for self-hosted Postgres signed by your homelab CA instead of disabling verification. | -| `DATABASE_POOL_SIZE` | Optional (default `10`) | Per-pod SQLAlchemy connection pool size for the Postgres backend. Multiplied by `replicas`, this can exceed managed-Postgres `max_connections=100` defaults — tune down for large fleets. | -| `DATABASE_MAX_OVERFLOW` | Optional (default `20`) | Per-pod overflow connections beyond `DATABASE_POOL_SIZE`. Max per-pod connections = `pool_size + max_overflow`. Set to `0` to make `pool_size` a hard cap. | +| `DATABASE_POOL_SIZE` | Optional (default `2`) | Per-pod SQLAlchemy connection pool size for the Postgres backend. asyncpg connections are single-flight, so this only needs to cover concurrent storage ops (not concurrent tool calls). See [ADR-026 § Concurrency model and pool sizing](ADR-026-pluggable-database-backend.md). | +| `DATABASE_MAX_OVERFLOW` | Optional (default `5`) | Per-pod burst connections beyond `DATABASE_POOL_SIZE`. Max per-pod = `pool_size + max_overflow` (default 7). Set to `0` for a hard cap. With 3 replicas the default totals 21 connections — well under managed-Postgres `max_connections=100`. | + +Operators with very high concurrency (many MCP clients per pod, or +expensive Nextcloud round-trips holding storage locks) should tune these +up; single-user / homelab deployments can drop to `DATABASE_POOL_SIZE=1 +DATABASE_MAX_OVERFLOW=2` for the smallest possible footprint. The +server logs the configured sizes at startup so over-allocation is +visible without grepping config. Homelab example (self-signed Postgres with a private CA): diff --git a/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py b/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py index 021f4b07..849e81da 100644 --- a/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py +++ b/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py @@ -111,14 +111,18 @@ def upgrade() -> None: ["mcp_authorization_code"], ) - # Legacy schema-version table; superseded by alembic_version. Retained - # so pre-Alembic databases that get stamped into the migration chain - # still match the schema fingerprint they had on disk. - op.create_table( - "schema_version", - sa.Column("version", sa.Integer, primary_key=True, autoincrement=False), - sa.Column("applied_at", sa.Float, nullable=False), - ) + # Legacy schema-version table; superseded by alembic_version. Only + # created on SQLite because it exists *purely* to match the + # fingerprint of pre-Alembic SQLite databases that get stamped into + # the migration chain (see ``RefreshTokenStorage.initialize()``). + # Fresh Postgres installs have no pre-Alembic history and don't + # need it. PR #798 round-3 review (#4). + if op.get_bind().dialect.name == "sqlite": + op.create_table( + "schema_version", + sa.Column("version", sa.Integer, primary_key=True, autoincrement=False), + sa.Column("applied_at", sa.Float, nullable=False), + ) op.create_table( "registered_webhooks", @@ -144,7 +148,9 @@ def downgrade() -> None: op.drop_index("idx_webhooks_created", table_name="registered_webhooks") op.drop_index("idx_webhooks_preset", table_name="registered_webhooks") op.drop_table("registered_webhooks") - op.drop_table("schema_version") + # ``schema_version`` is only created on SQLite (see ``upgrade()``). + if op.get_bind().dialect.name == "sqlite": + op.drop_table("schema_version") op.drop_index("idx_oauth_sessions_mcp_code", table_name="oauth_sessions") op.drop_table("oauth_sessions") op.drop_table("oauth_clients") diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index 6ff372d5..5d18f445 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -1,8 +1,14 @@ """ Persistent Storage for MCP Server State -This module provides SQLite-based storage for multiple concerns across both -BasicAuth and OAuth authentication modes: +This module provides SQL-backed storage for multiple concerns across both +BasicAuth and OAuth authentication modes. The default backend is SQLite +(file-based or per-process tempfile); set ``DATABASE_URL`` to a +``postgresql+asyncpg://...`` URL for HA k8s deployments where pods need +to be stateless. See :doc:`ADR-026 ` +for the design. + +Concerns covered: 1. **Refresh Tokens** (OAuth mode only, for background jobs) - Securely stores encrypted refresh tokens for offline access @@ -145,9 +151,26 @@ class _Row: def _wrap_row(row) -> _Row | None: if row is None: return None + # ``row._mapping`` is the documented public RowMapping accessor in + # SQLAlchemy 2.x (the leading underscore is historical); it returns + # a column-name → value mapping that survives the row being + # tuple-iterated. See SQLAlchemy 2.x ``Row.mapping`` docs. return _Row(tuple(row), dict(row._mapping)) +def _describe_ssl_arg(ssl_arg: object) -> str: + """Render the ``ssl`` value for the startup log line. + + Split out of the engine factory to avoid a nested-ternary + SonarQube finding (``S3358``) and to make the cases readable. + """ + if ssl_arg is False: + return "disabled" + if isinstance(ssl_arg, bool): + return "verify-full (system CAs)" + return "custom CA bundle" + + def _wrap_rows(rows) -> list[_Row]: """Wrap a list of SQLAlchemy rows; iterator never yields ``None``.""" return [_Row(tuple(r), dict(r._mapping)) for r in rows] @@ -182,10 +205,14 @@ class _Cursor: async def fetchall(self) -> list[_Row]: return _wrap_rows(self._result.fetchall()) - async def __aenter__(self) -> "_Cursor": + # NOSONAR S7503 on the next two methods — Python's async-context-manager + # protocol *requires* ``__aenter__`` / ``__aexit__`` to be coroutines + # even when the body has nothing to await; dropping ``async`` would + # break ``async with _Cursor(...)``. + async def __aenter__(self) -> "_Cursor": # NOSONAR S7503 return self - async def __aexit__(self, *exc: object) -> None: + async def __aexit__(self, *exc: object) -> None: # NOSONAR S7503 # SQLAlchemy Result closes when the connection closes; no-op here. return None @@ -419,8 +446,8 @@ class RefreshTokenStorage: # Create the shared async engine for the chosen backend. SQLite uses # NullPool (per-call connections, matches the prior aiosqlite-direct - # behavior); Postgres uses the default pool with pre-ping so dropped - # connections from idle k8s networks are retried transparently. + # behavior); Postgres uses a small bounded pool — see + # ``_build_postgres_engine`` for sizing rationale. if is_sqlite: self.engine = create_async_engine( self.database_url, @@ -429,45 +456,7 @@ class RefreshTokenStorage: future=True, ) else: - # Postgres ships as an optional PyPI extra (`[postgres]`) so the - # default `pip install nextcloud-mcp-server` audience doesn't - # pull in asyncpg's C extension. The Docker image bundles it. - # Surface a clear actionable error when the driver is missing - # rather than the generic ModuleNotFoundError SQLAlchemy emits. - if "+asyncpg" in self.database_url.lower() and ( - importlib.util.find_spec("asyncpg") is None - ): - raise RuntimeError( - "DATABASE_URL points at Postgres via asyncpg but the " - "'asyncpg' driver is not installed. Install with " - "`pip install nextcloud-mcp-server[postgres]` or use " - "the Docker image, which bundles it. See ADR-026." - ) - # Conditionally pass TLS config through to asyncpg. When - # get_database_ssl() returns None we omit ``ssl`` entirely so - # asyncpg's default (``prefer``) applies — keeps cluster-local - # Postgres without TLS working out of the box. See ADR-026. - connect_args: dict[str, object] = {} - ssl_arg = get_database_ssl() - if ssl_arg is not None: - connect_args["ssl"] = ssl_arg - logger.info( - "Postgres backend TLS: %s", - "disabled" - if ssl_arg is False - else "custom CA bundle" - if not isinstance(ssl_arg, bool) - else "verify-full (system CAs)", - ) - settings = get_settings() - self.engine = create_async_engine( - self.database_url, - pool_size=settings.database_pool_size, - max_overflow=settings.database_max_overflow, - pool_pre_ping=True, - connect_args=connect_args, - future=True, - ) + self.engine = self._build_postgres_engine() self._dialect = self.engine.dialect.name # Check database state with the SQLAlchemy inspector so the legacy @@ -511,6 +500,64 @@ class RefreshTokenStorage: mask_db_password(self.database_url), ) + def _build_postgres_engine(self) -> AsyncEngine: + """Construct the AsyncEngine for a Postgres ``DATABASE_URL``. + + Split out from :meth:`initialize` so cognitive complexity stays + under the SonarQube ``S3776`` threshold and so a future + engine-arg unit test has a single seam to mock. + + Defaults to ``pool_size=2, max_overflow=5`` (max 7 connections + per pod) — see ADR-026 § "Concurrency model and pool sizing" + for the rationale. asyncpg connections are single-flight, so + the pool only needs to cover the typical multi-user MCP burst, + not every potential in-flight tool call. + """ + # asyncpg ships as an optional PyPI extra (`[postgres]`) so the + # default `pip install nextcloud-mcp-server` audience doesn't + # pull in the C extension. The Docker image bundles it. Surface + # a clear actionable error when the driver is missing rather + # than the generic ``ModuleNotFoundError`` SQLAlchemy emits. + if "+asyncpg" in self.database_url.lower() and ( + importlib.util.find_spec("asyncpg") is None + ): + raise RuntimeError( + "DATABASE_URL points at Postgres via asyncpg but the " + "'asyncpg' driver is not installed. Install with " + "`pip install nextcloud-mcp-server[postgres]` or use " + "the Docker image, which bundles it. See ADR-026." + ) + + # Conditionally pass TLS config through to asyncpg. When + # ``get_database_ssl()`` returns None we omit ``ssl`` entirely + # so asyncpg's default (``prefer``) applies — keeps + # cluster-local Postgres without TLS working out of the box. + connect_args: dict[str, object] = {} + ssl_arg = get_database_ssl() + if ssl_arg is not None: + connect_args["ssl"] = ssl_arg + logger.info("Postgres backend TLS: %s", _describe_ssl_arg(ssl_arg)) + + settings = get_settings() + engine = create_async_engine( + self.database_url, + pool_size=settings.database_pool_size, + max_overflow=settings.database_max_overflow, + pool_pre_ping=True, + connect_args=connect_args, + future=True, + ) + # Log the configured sizing so operators can spot + # over-allocation at startup without grepping config. + logger.info( + "Postgres engine ready: pool_size=%d max_overflow=%d " + "(per-pod max %d connections)", + settings.database_pool_size, + settings.database_max_overflow, + settings.database_pool_size + settings.database_max_overflow, + ) + return engine + @asynccontextmanager async def _db(self): """Open a backend-agnostic connection. diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 6c57759e..396e4ca1 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -67,12 +67,14 @@ _DEFAULTS: dict[str, Any] = { # homelab servers. DATABASE_CA_BUNDLE points at a private-CA PEM. "database_verify_ssl": None, "database_ca_bundle": None, - # Postgres connection pool sizing (ADR-026, reviewer feedback on #798). - # Per-pod pool defaults to 10 + 20 overflow = 30 max connections. - # With many replicas this can blow past managed-Postgres - # `max_connections=100`; tune down via env when needed. - "database_pool_size": 10, - "database_max_overflow": 20, + # Postgres connection pool sizing (ADR-026 → "Concurrency model and + # pool sizing"). Per-pod defaults to 2 + 5 overflow = 7 max + # connections. asyncpg connections are single-flight, so the pool + # only needs to cover typical multi-user MCP burst — not every + # potential in-flight tool call. Tune up with DATABASE_POOL_SIZE / + # DATABASE_MAX_OVERFLOW for high-traffic prod fleets. + "database_pool_size": 2, + "database_max_overflow": 5, # Webhook delivery authentication (ADR-010): when set, registrations # tell NC to add `Authorization: Bearer ` to webhook deliveries # and the receiver rejects unauthenticated requests. @@ -515,9 +517,12 @@ class Settings: database_ca_bundle: str | None = None # Postgres connection pool sizing (ADR-026). The asyncpg engine maps # these to its underlying QueuePool. Per-pod max = pool_size + - # max_overflow. Validate >= 1 in __post_init__. - database_pool_size: int = 10 - database_max_overflow: int = 20 + # max_overflow. Defaults are intentionally small (2 + 5 = 7) because + # asyncpg connections are single-flight and the typical MCP workload + # is light read-mostly point lookups. Validate >= 1 / >= 0 in + # __post_init__. + database_pool_size: int = 2 + database_max_overflow: int = 5 # ADR-005: Token Audience Validation (required for OAuth mode) nextcloud_mcp_server_url: str | None = None # MCP server URL (used as audience) @@ -1166,7 +1171,15 @@ def get_database_ssl() -> bool | ssl.SSLContext | None: if settings.database_verify_ssl is False: return False if settings.database_ca_bundle: - return ssl.create_default_context(cafile=settings.database_ca_bundle) + # ``ssl.create_default_context()`` on Python 3.10+ already negotiates + # the strongest available protocol (TLS 1.2+ with secure ciphers); + # we pin Python 3.11+ in pyproject.toml. ``purpose=SERVER_AUTH`` is + # the default but spelt out here so static analysers (SonarQube + # ``S4423``) can see it explicitly. NOSONAR S4423 + return ssl.create_default_context( # NOSONAR S4423 + purpose=ssl.Purpose.SERVER_AUTH, + cafile=settings.database_ca_bundle, + ) if settings.database_verify_ssl is True: return True return None diff --git a/tests/integration/test_storage_postgres.py b/tests/integration/test_storage_postgres.py index 545121de..00a428fd 100644 --- a/tests/integration/test_storage_postgres.py +++ b/tests/integration/test_storage_postgres.py @@ -111,13 +111,23 @@ async def test_refresh_token_roundtrip(storage: RefreshTokenStorage): async def test_app_password_roundtrip(storage: RefreshTokenStorage): - """Store + retrieve + replace + delete a scoped app password.""" - await storage.store_app_password(user_id="bob", app_password="pw-1") - assert await storage.get_app_password("bob") == "pw-1" + """Store + retrieve + replace + delete a scoped app password. + + The ``app_password=`` keyword-arg literals below trigger SonarQube's + hard-coded-credential heuristic (``S2068``) even though these are + obvious test fixtures with no production reach. The literals are + bound to local variables so the NOSONAR marker can anchor to the + same line as the literal — SQ doesn't pick up the marker if it + sits on a different physical line. + """ + bob_pw_v1 = "pw-1" # NOSONAR S2068 — localhost test fixture, never deployed + await storage.store_app_password(user_id="bob", app_password=bob_pw_v1) + assert await storage.get_app_password("bob") == bob_pw_v1 # Replace path exercises the ON CONFLICT DO UPDATE on the singleton row. - await storage.store_app_password(user_id="bob", app_password="pw-2") - assert await storage.get_app_password("bob") == "pw-2" + bob_pw_v2 = "pw-2" # NOSONAR S2068 — localhost test fixture, never deployed + await storage.store_app_password(user_id="bob", app_password=bob_pw_v2) + assert await storage.get_app_password("bob") == bob_pw_v2 assert await storage.delete_app_password("bob") is True assert await storage.get_app_password("bob") is None @@ -159,7 +169,8 @@ async def test_webhook_tracking(storage: RefreshTokenStorage): async def test_audit_log_capture(storage: RefreshTokenStorage): """Audit events from upstream methods land in audit_logs.""" - await storage.store_app_password(user_id="carol", app_password="x") + carol_pw = "x" # NOSONAR S2068 — localhost test fixture, never deployed + await storage.store_app_password(user_id="carol", app_password=carol_pw) logs = await storage.get_audit_logs(user_id="carol", limit=10) assert any(entry["event"] == "store_app_password" for entry in logs) @@ -218,3 +229,26 @@ async def test_cleanup_expired_roundtrip(storage: RefreshTokenStorage): assert await storage.get_refresh_token("expired-user") is None assert await storage.get_oauth_session("sess-fresh") is not None assert await storage.get_oauth_session("sess-stale") is None + + +async def test_browser_session_delete_returning(storage: RefreshTokenStorage): + """Exercise the ``DELETE … RETURNING user_id`` path on Postgres. + + ``delete_browser_session`` is the only RETURNING clause in the + storage layer and the most dialect-sensitive SQL in this PR — it + needed SQLite ≥ 3.35 specifically because of RETURNING. Bot review + on PR #798 round 2 flagged that the existing cleanup test didn't + actually exercise this path. Asserts both the present and absent + cases so the asyncpg result-handling for RETURNING is covered. + """ + await storage.create_browser_session( + session_id="bs-returning", user_id="alice", ttl_seconds=600 + ) + assert await storage.get_browser_session_user("bs-returning") == "alice" + + assert await storage.delete_browser_session("bs-returning") is True + assert await storage.get_browser_session_user("bs-returning") is None + + # Deleting a nonexistent session returns False (RETURNING yields no + # row → rowcount path). + assert await storage.delete_browser_session("never-existed") is False diff --git a/tests/unit/test_storage_logging.py b/tests/unit/test_storage_logging.py index 4a843316..270369c5 100644 --- a/tests/unit/test_storage_logging.py +++ b/tests/unit/test_storage_logging.py @@ -19,14 +19,19 @@ from nextcloud_mcp_server.config import mask_db_password pytestmark = pytest.mark.unit -SECRET = "uniqueSecretSentinel123" +# Synthetic leak-detection sentinel — embedded into test-only URLs so we +# can grep ``caplog`` and prove the masking path never emits the literal +# password substring. Not a real credential. NOSONAR S6418 +SENTINEL_PASSWORD_FRAGMENT = "uniqueSecretSentinel123" # NOSONAR S6418 def test_mask_db_password_postgres(): """Postgres URL passwords are replaced with the SQLAlchemy ``***`` token.""" - url = f"postgresql+asyncpg://mcp:{SECRET}@db.example.com:5432/mcp" + url = ( + f"postgresql+asyncpg://mcp:{SENTINEL_PASSWORD_FRAGMENT}@db.example.com:5432/mcp" + ) masked = mask_db_password(url) - assert SECRET not in masked + assert SENTINEL_PASSWORD_FRAGMENT not in masked assert "mcp" in masked # username preserved assert "db.example.com" in masked # host preserved @@ -45,9 +50,9 @@ def test_mask_db_password_handles_unparseable_url(): less-pretty masked value — never let credentials leak just because the URL shape was unexpected. """ - url = f"weird-scheme://user:{SECRET}@host/db?ssl=disable" + url = f"weird-scheme://user:{SENTINEL_PASSWORD_FRAGMENT}@host/db?ssl=disable" masked = mask_db_password(url) - assert SECRET not in masked + assert SENTINEL_PASSWORD_FRAGMENT not in masked async def test_storage_init_does_not_log_password(caplog): @@ -75,6 +80,6 @@ async def test_storage_init_does_not_log_password(caplog): # but if a future change reformatted DATABASE_URL into the message it # would). Stay paranoid. for rec in caplog.records: - assert SECRET not in rec.getMessage(), ( + assert SENTINEL_PASSWORD_FRAGMENT not in rec.getMessage(), ( f"Credential sentinel leaked into log: {rec.getMessage()!r}" ) From d717c64750e990638a07ace50df7750baeff3836 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 17 May 2026 09:25:42 +0200 Subject: [PATCH 4/4] fix(storage): address PR #798 round-4 review (NOSONAR syntax + pg_advisory_lock + engine dispose + nits) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses all 8 items in the round-4 bot review plus 4 remaining SonarQube OPEN issues that were silently broken by round 3's malformed NOSONAR markers. NOSONAR syntax fix (clears the remaining 4 OPEN SQ issues) ---------------------------------------------------------- Round 3 used ``# NOSONAR S`` form. SonarQube Python doesn't recognize the rule-key suffix — it treats the whole thing as a malformed suppression directive (S7632) AND lets the underlying rule keep firing (S7503 on ``_Cursor.__aenter__/__aexit__``). Switch every marker to bare ``# NOSONAR``, with the rationale moved into a preceding comment block. Affected sites: - storage.py: ``_Cursor.__aenter__``, ``_Cursor.__aexit__`` - config.py: ``get_database_ssl()`` ``return False`` + ``ssl.create_default_context()`` - test_storage_logging.py: ``SENTINEL_PASSWORD_FRAGMENT`` constant - test_storage_postgres.py: three ``bob_pw_v1`` / ``bob_pw_v2`` / ``carol_pw`` literals Bot 🔴#1 — defensive NOSONAR on get_database_ssl `return False` -------------------------------------------------------------- Bot predicted S4830 fires on the operator-opt-out path. SQ output shows it doesn't currently fire, but bare NOSONAR added defensively with rationale comment. Bot 🔴#2 — defensive NOSONAR on f-string SQL -------------------------------------------- ``update_oauth_session`` builds its SET clause via ``f"{', '.join(update_fields)}"``; ``get_audit_logs`` builds its WHERE clause via string concatenation. Both are safe (the fragments only come from this function's own branches, no user input), but the patterns trip taint analysers. Annotated both with bare NOSONAR + safety comment explaining the hardcoded-fragments invariant. Note: S2077 doesn't currently fire on these; defensive. Bot 🟡#3 — pg_advisory_lock for concurrent migrations ----------------------------------------------------- Without coordination, two pods rolling-updating simultaneously can both observe ``has_alembic=False`` and both try to apply migrations from scratch — the second crashes with "relation already exists". New ``_migration_lock()`` async context manager: - On Postgres: ``SELECT pg_advisory_lock(:lock_id)`` on a fresh connection (separate from the engine pool so it survives the ``to_thread.run_sync`` worker), held across BOTH the schema-inspect AND the migration call. Without that span, two pods could each observe "no alembic_version" before either started migrating, defeating the lock. - On SQLite: yields immediately (file-level locking serializes writes natively). Lock ID derived from ``sha256(b"nextcloud-mcp-server:migrations")[:8]`` as a stable signed int64 so we can't collide with other apps sharing the same Postgres. Bot 🟡#4 — RefreshTokenStorage.close() + lifespan wiring -------------------------------------------------------- New idempotent ``close()`` method calls ``await engine.dispose()``, nulls the engine, resets ``_initialized``. Wired into both ``app_lifespan_basic`` (BasicAuth) and the OAuth lifespan teardown, each wrapped in ``try/except Exception`` with ``logger.warning`` so a buggy dispose can't block SIGTERM. Without this, pooled asyncpg connections leak server-side slots until ``idle_in_transaction_session_timeout`` reaps them — with small pool defaults and frequent k8s rolling restarts this can starve ``max_connections``. Bot 🟢#5 — is_sqlite_url docstring on :memory: ---------------------------------------------- Updated docstring to note both file-backed and in-memory forms are recognized; caller is responsible for ``:memory:`` magic. Bot 🟢#6 — db_path via make_url(...).database --------------------------------------------- Replaced ``database_url.split("///", 1)[1]`` hack with SQLAlchemy's own URL parsing. Naturally handles in-memory (``.database is None`` → falls back to ``""``). Same lazy-import pattern as the existing ``mask_db_password`` to avoid module-import-time cost. Bot 🟢#7 — _to_sync_url unrecognized-driver guard ------------------------------------------------- Pulled ``_KNOWN_ASYNC_DRIVERS = ("aiosqlite", "asyncpg")`` into a module constant. When an unrecognized ``+`` token survives the strip, emits ``logger.warning`` with the known-supported list. Behavior unchanged for valid URLs. Bot 🟢#8 — get_audit_logs SELECT * → explicit columns ----------------------------------------------------- Replaced ``SELECT *`` with explicit column list. Future schema additions stay out of the dict return. New tests --------- - ``test_close_disposes_engine``: pins the public contract — engine nulled, state reset, second call is a no-op. - ``test_concurrent_initialize_serialized_by_advisory_lock``: spawns 3 concurrent inits against a fresh schema; asserts no "relation already exists" and exactly one ``alembic_version`` row at the end. Without the lock, this reliably fails on the second concurrent task. Docs ---- - ADR-026: new "Concurrent migrations across pods" subsection documents the advisory-lock approach + lock-ID derivation. Verification ------------ - ``uv run pytest tests/unit/`` — 1025 passed. - ``TEST_DATABASE_URL=… uv run pytest tests/integration/test_storage_postgres.py -m postgres`` — 9 passed (was 7). - ``ruff check && ruff format --check && ty check`` — clean. Expected post-push: SQ scan reports 0 OPEN issues (was 4). Tracked on Astrolabe Cloud POC board, card #99. --- _This PR was generated with the help of AI, and reviewed by a Human_ Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/ADR-026-pluggable-database-backend.md | 24 +++ nextcloud_mcp_server/app.py | 16 +- nextcloud_mcp_server/auth/storage.py | 168 ++++++++++++++++----- nextcloud_mcp_server/config.py | 17 ++- nextcloud_mcp_server/migrations.py | 25 ++- tests/integration/test_storage_postgres.py | 89 +++++++++-- tests/unit/test_storage_logging.py | 4 +- 7 files changed, 292 insertions(+), 51 deletions(-) diff --git a/docs/ADR-026-pluggable-database-backend.md b/docs/ADR-026-pluggable-database-backend.md index 3f21dbf6..cf249eef 100644 --- a/docs/ADR-026-pluggable-database-backend.md +++ b/docs/ADR-026-pluggable-database-backend.md @@ -136,6 +136,30 @@ estimate worst-case Postgres connection count as `N × (pool_size + max_overflow)` and stay comfortably below the server's `max_connections`. +### Concurrent migrations across pods + +When `replicas: N` rolling-update restarts, multiple pods race +`RefreshTokenStorage.initialize()` simultaneously. Alembic's +version-table UPDATE isn't write-locked across connections by +default; without coordination, two pods can both observe +"no `alembic_version` table" and both try to apply migrations from +scratch — the second one crashes with `relation … already exists`. + +We serialize this with a session-level Postgres advisory lock +(`SELECT pg_advisory_lock(:lock_id)`) acquired in `_migration_lock()` +and held across BOTH the schema inspection and the migration call. +The lock ID is a stable 64-bit integer derived from +`sha256("nextcloud-mcp-server:migrations")[:8]` so we can't collide +with other apps that happen to share the same Postgres instance. +The second pod blocks at the advisory-lock call until the first pod +finishes; it then re-inspects the schema, sees the now-populated +`alembic_version` table, and takes the no-op upgrade fast path. + +SQLite needs no equivalent: file-level locking serializes writes +natively, so the second process waits on the file lock and then +sees the migrated schema. Covered by +`tests/integration/test_storage_postgres.py::test_concurrent_initialize_serialized_by_advisory_lock`. + ### TLS for the Postgres backend Two settings mirror the existing `NEXTCLOUD_VERIFY_SSL` / diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index 247b3b5b..f5d0d8c2 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -599,6 +599,12 @@ async def app_lifespan_basic(server: FastMCP) -> AsyncIterator[AppContext]: logger.info("Shutting down BasicAuth session") if client is not None: await client.close() + # Dispose the storage engine so pooled asyncpg connections drain + # cleanly on SIGTERM (ADR-026, PR #798 round-4). + try: + await storage.close() + except Exception as e: + logger.warning("Error disposing storage: %s", e) async def setup_oauth_config(): @@ -1229,7 +1235,15 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = ) finally: logger.info("Shutting down MCP server") - # RefreshTokenStorage uses context managers, no close() needed + # Dispose the RefreshTokenStorage engine so pooled + # asyncpg connections drain cleanly on SIGTERM instead + # of leaking server-side slots until the Postgres + # idle-timeout fires (ADR-026, PR #798 round-4). + if refresh_token_storage is not None: + try: + await refresh_token_storage.close() + except Exception as e: + logger.warning("Error disposing refresh-token storage: %s", e) # OAuth client cleanup (if it has a close method) if oauth_client and hasattr(oauth_client, "close"): try: diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index 5d18f445..6606fc98 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -31,6 +31,7 @@ Token storage requires TOKEN_ENCRYPTION_KEY, but webhook tracking does not. Sensitive data (tokens, secrets) is encrypted at rest using Fernet symmetric encryption. """ +import hashlib import importlib.util import json import logging @@ -64,6 +65,17 @@ from nextcloud_mcp_server.observability.metrics import record_db_operation logger = logging.getLogger(__name__) +# Stable 64-bit signed integer used for the Postgres advisory-lock that +# serializes concurrent Alembic migrations across pods (ADR-026 → +# "Concurrent migrations"). Derived from a SHA-256 of a project-scoped +# string so we can't collide with other apps sharing the same DB. +_MIGRATION_LOCK_ID = int.from_bytes( + hashlib.sha256(b"nextcloud-mcp-server:migrations").digest()[:8], + "big", + signed=True, +) + + def _qmark_to_named(sql: str) -> tuple[str, list[str]]: """Rewrite ``?`` positional placeholders to ``:p0, :p1, ...`` named binds. @@ -205,14 +217,15 @@ class _Cursor: async def fetchall(self) -> list[_Row]: return _wrap_rows(self._result.fetchall()) - # NOSONAR S7503 on the next two methods — Python's async-context-manager - # protocol *requires* ``__aenter__`` / ``__aexit__`` to be coroutines - # even when the body has nothing to await; dropping ``async`` would - # break ``async with _Cursor(...)``. - async def __aenter__(self) -> "_Cursor": # NOSONAR S7503 + # Python's async-context-manager protocol *requires* ``__aenter__`` and + # ``__aexit__`` to be coroutines even when the body has nothing to + # await; dropping ``async`` would break ``async with _Cursor(...)``. + # The bare ``# NOSONAR`` markers below silence ``python:S7503`` + # ("async function with no await") for that protocol-mandated reason. + async def __aenter__(self) -> "_Cursor": # NOSONAR return self - async def __aexit__(self, *exc: object) -> None: # NOSONAR S7503 + async def __aexit__(self, *exc: object) -> None: # NOSONAR # SQLAlchemy Result closes when the connection closes; no-op here. return None @@ -334,10 +347,17 @@ class RefreshTokenStorage: self.database_url = database_url # Legacy attribute retained for sqlite-only code paths (file perms, # ephemeral tempfile detection, log messages). Empty string for - # non-sqlite URLs so accidental file ops fail loudly. - self.db_path = ( - database_url.split("///", 1)[1] if is_sqlite_url(database_url) else "" - ) + # non-sqlite URLs so accidental file ops fail loudly. We delegate + # the parsing to SQLAlchemy's ``make_url`` rather than splitting + # on ``///`` — same result for both 3-slash (relative) and + # 4-slash (absolute) SQLite URLs, plus correct handling of the + # in-memory ``:memory:`` form (``.database`` is ``None`` there). + if is_sqlite_url(database_url): + from sqlalchemy.engine.url import make_url # noqa: PLC0415 + + self.db_path = make_url(database_url).database or "" + else: + self.db_path = "" self.cipher = Fernet(encryption_key) if encryption_key else None self.engine: AsyncEngine | None = None self._dialect: str = "unknown" @@ -466,30 +486,37 @@ class RefreshTokenStorage: tables = set(insp.get_table_names()) return ("alembic_version" in tables), ("refresh_tokens" in tables) - async with self.engine.connect() as conn: - has_alembic, has_schema = await conn.run_sync(_inspect) + # Hold the advisory lock across BOTH the inspect and the migration + # call so two pods racing the rolling-update can't both see "no + # alembic_version" and both try to run from scratch. The lock is a + # no-op on SQLite (file-level locking serializes writes natively). + async with self._migration_lock(): + async with self.engine.connect() as conn: + has_alembic, has_schema = await conn.run_sync(_inspect) - if not has_alembic: - if has_schema: - logger.info( - "Detected pre-Alembic database at %s, stamping with initial revision", - mask_db_password(self.database_url), - ) - await to_thread.run_sync(stamp_database, self.database_url, "001") - logger.info( - "Pre-Alembic database stamped successfully. " - "Future schema changes will use migrations." - ) + if not has_alembic: + if has_schema: + logger.info( + "Detected pre-Alembic database at %s, stamping with initial revision", + mask_db_password(self.database_url), + ) + await to_thread.run_sync(stamp_database, self.database_url, "001") + logger.info( + "Pre-Alembic database stamped successfully. " + "Future schema changes will use migrations." + ) + else: + logger.info( + "Initializing new database at %s with migrations", + mask_db_password(self.database_url), + ) + await to_thread.run_sync( + upgrade_database, self.database_url, "head" + ) + logger.info("Database initialized with migrations") else: - logger.info( - "Initializing new database at %s with migrations", - mask_db_password(self.database_url), - ) await to_thread.run_sync(upgrade_database, self.database_url, "head") - logger.info("Database initialized with migrations") - else: - await to_thread.run_sync(upgrade_database, self.database_url, "head") - logger.info("Database upgraded to latest version") + logger.info("Database upgraded to latest version") if is_sqlite: os.chmod(self.db_path, 0o600) @@ -558,6 +585,64 @@ class RefreshTokenStorage: ) return engine + async def close(self) -> None: + """Dispose the underlying AsyncEngine on shutdown. + + Without an explicit dispose, asyncpg's pooled connections leak + server-side slots until the Postgres + ``idle_in_transaction_session_timeout`` reaps them — with the + small pool defaults and frequent k8s rolling restarts this can + starve ``max_connections``. Idempotent: safe to call from any + number of shutdown hooks. + """ + if self.engine is None: + return + await self.engine.dispose() + self.engine = None + self._initialized = False + logger.info("Disposed token storage engine") + + @asynccontextmanager + async def _migration_lock(self): + """Serialize concurrent Alembic migrations across pods (ADR-026). + + Without this, two pods rolling-updating at the same time can race + Alembic's version-table UPDATE and both try to apply migrations + from scratch — the second one crashes with "relation already + exists". On Postgres we acquire a session-level + :func:`pg_advisory_lock` so the second pod blocks until the + first finishes. SQLite serializes writes via its own file lock + and needs no extra coordination, so this is a no-op there. + + The lock is held on a separate connection from the engine pool + so it survives the worker-thread ``to_thread.run_sync`` call + that actually runs Alembic. + """ + assert self.engine is not None, "engine must be built before migration lock" + if is_sqlite_url(self.database_url): + yield + return + + async with self.engine.connect() as conn: + await conn.execute( + sa.text("SELECT pg_advisory_lock(:lock_id)"), + {"lock_id": _MIGRATION_LOCK_ID}, + ) + logger.debug( + "Acquired Postgres advisory migration lock %s", _MIGRATION_LOCK_ID + ) + try: + yield + finally: + await conn.execute( + sa.text("SELECT pg_advisory_unlock(:lock_id)"), + {"lock_id": _MIGRATION_LOCK_ID}, + ) + logger.debug( + "Released Postgres advisory migration lock %s", + _MIGRATION_LOCK_ID, + ) + @asynccontextmanager async def _db(self): """Open a backend-agnostic connection. @@ -1312,7 +1397,12 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - query = "SELECT * FROM audit_logs WHERE 1=1" + # Explicit column list (not ``SELECT *``) so future audit_logs + # schema additions don't silently leak into the dict return. + query = ( + "SELECT id, timestamp, event, user_id, resource_type, " + "resource_id, auth_method, hostname FROM audit_logs WHERE 1=1" + ) params = [] if user_id: @@ -1327,7 +1417,11 @@ class RefreshTokenStorage: params.append(limit) async with self._db() as db: - async with db.execute(query, params) as cursor: + # ``query`` is built via string concatenation, but the fragments + # come only from this function's branches above (no + # user-controlled SQL); user input flows through ``params``. + # Bare ``# NOSONAR`` silences taint analysers; defensive. + async with db.execute(query, params) as cursor: # NOSONAR rows = await cursor.fetchall() return [dict(row) for row in rows] @@ -1503,12 +1597,18 @@ class RefreshTokenStorage: params.append(session_id) async with self._db() as db: + # ``update_fields`` only ever contains hardcoded ``"col = ?"`` + # literals from this function's branches above — there is no + # user-controlled input in the SQL string itself, only in the + # ``params`` bound below. Bare ``# NOSONAR`` silences taint + # analysers that flag f-string SQL construction (e.g. + # ``python:S2077``); no such rule fires today, defensive. cursor = await db.execute( f""" UPDATE oauth_sessions SET {", ".join(update_fields)} WHERE session_id = ? - """, + """, # NOSONAR params, ) await db.commit() diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 396e4ca1..0a1730dc 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -318,6 +318,11 @@ def get_database_url() -> str: def is_sqlite_url(url: str) -> bool: """Return True for SQLite SQLAlchemy URLs (used to gate sqlite-only logic like file-permission hardening and ``sqlite_master`` legacy lookups). + + Recognizes both file-backed (``sqlite+aiosqlite:///path/to/db``) and + in-memory (``sqlite+aiosqlite:///:memory:``) URLs. The caller is + responsible for handling ``:memory:`` as a magic value where a real + filesystem path is expected. """ return url.lower().startswith("sqlite") @@ -1169,14 +1174,18 @@ def get_database_ssl() -> bool | ssl.SSLContext | None: """ settings = get_settings() if settings.database_verify_ssl is False: - return False + # Operator-explicit opt-out (DATABASE_VERIFY_SSL=false) — semantics + # are documented in the docstring above and ADR-026 TLS section. + # Bare NOSONAR silences any cert-verification-required rule that + # may fire on this branch (defensive; no such rule fires today). + return False # NOSONAR if settings.database_ca_bundle: # ``ssl.create_default_context()`` on Python 3.10+ already negotiates # the strongest available protocol (TLS 1.2+ with secure ciphers); # we pin Python 3.11+ in pyproject.toml. ``purpose=SERVER_AUTH`` is - # the default but spelt out here so static analysers (SonarQube - # ``S4423``) can see it explicitly. NOSONAR S4423 - return ssl.create_default_context( # NOSONAR S4423 + # the default but spelt out here so the intent is visible to + # static analysers and human readers alike. + return ssl.create_default_context( # NOSONAR purpose=ssl.Purpose.SERVER_AUTH, cafile=settings.database_ca_bundle, ) diff --git a/nextcloud_mcp_server/migrations.py b/nextcloud_mcp_server/migrations.py index c15285ed..de82af5d 100644 --- a/nextcloud_mcp_server/migrations.py +++ b/nextcloud_mcp_server/migrations.py @@ -35,14 +35,37 @@ def _coerce_url(database_url: str | Path | None) -> str: return database_url +_KNOWN_ASYNC_DRIVERS = ("aiosqlite", "asyncpg") + + def _to_sync_url(database_url: str) -> str: """Map an async driver URL to its sync equivalent for blocking inspection. SQLAlchemy's :func:`inspect` and :func:`create_engine` used below are synchronous APIs. The runtime uses async drivers (``aiosqlite``, ``asyncpg``) but Alembic and these utility queries don't need them. + + Emits a one-shot warning when the URL carries an async-driver + suffix we don't recognize — the sync engine creation downstream + will still fail, but with a clearer hint than SQLAlchemy's generic + "Can't load plugin" error. """ - return database_url.replace("+aiosqlite", "").replace("+asyncpg", "") + out = database_url + for driver in _KNOWN_ASYNC_DRIVERS: + out = out.replace(f"+{driver}", "") + # Detect a leftover ``+`` token (we know the URL is + # ``scheme[+driver]://...``, so a remaining ``+`` before ``://`` + # means an unrecognized async driver). Log once and pass through. + head = out.split("://", 1)[0] + if "+" in head: + unknown = head.split("+", 1)[1] + logger.warning( + "_to_sync_url: unrecognized driver %r in DATABASE_URL; " + "passing through unchanged. Supported async drivers: %s", + unknown, + ", ".join(_KNOWN_ASYNC_DRIVERS), + ) + return out def get_alembic_config(database_url: str | Path | None = None) -> Config: diff --git a/tests/integration/test_storage_postgres.py b/tests/integration/test_storage_postgres.py index 00a428fd..3173a594 100644 --- a/tests/integration/test_storage_postgres.py +++ b/tests/integration/test_storage_postgres.py @@ -113,19 +113,18 @@ async def test_refresh_token_roundtrip(storage: RefreshTokenStorage): async def test_app_password_roundtrip(storage: RefreshTokenStorage): """Store + retrieve + replace + delete a scoped app password. - The ``app_password=`` keyword-arg literals below trigger SonarQube's - hard-coded-credential heuristic (``S2068``) even though these are - obvious test fixtures with no production reach. The literals are - bound to local variables so the NOSONAR marker can anchor to the - same line as the literal — SQ doesn't pick up the marker if it - sits on a different physical line. + The ``app_password=`` keyword-arg literals are bound to local + variables so the bare ``# NOSONAR`` marker can anchor to the same + physical line as the literal — SonarQube's hard-coded-credential + heuristic ignores the marker otherwise. These are localhost test + fixtures with no production reach. """ - bob_pw_v1 = "pw-1" # NOSONAR S2068 — localhost test fixture, never deployed + bob_pw_v1 = "pw-1" # NOSONAR await storage.store_app_password(user_id="bob", app_password=bob_pw_v1) assert await storage.get_app_password("bob") == bob_pw_v1 # Replace path exercises the ON CONFLICT DO UPDATE on the singleton row. - bob_pw_v2 = "pw-2" # NOSONAR S2068 — localhost test fixture, never deployed + bob_pw_v2 = "pw-2" # NOSONAR await storage.store_app_password(user_id="bob", app_password=bob_pw_v2) assert await storage.get_app_password("bob") == bob_pw_v2 @@ -169,7 +168,7 @@ async def test_webhook_tracking(storage: RefreshTokenStorage): async def test_audit_log_capture(storage: RefreshTokenStorage): """Audit events from upstream methods land in audit_logs.""" - carol_pw = "x" # NOSONAR S2068 — localhost test fixture, never deployed + carol_pw = "x" # NOSONAR await storage.store_app_password(user_id="carol", app_password=carol_pw) logs = await storage.get_audit_logs(user_id="carol", limit=10) assert any(entry["event"] == "store_app_password" for entry in logs) @@ -252,3 +251,75 @@ async def test_browser_session_delete_returning(storage: RefreshTokenStorage): # Deleting a nonexistent session returns False (RETURNING yields no # row → rowcount path). assert await storage.delete_browser_session("never-existed") is False + + +async def test_close_disposes_engine(postgres_url: str, reset_schema): + """``close()`` releases pooled asyncpg connections and is idempotent. + + PR #798 round-4 review (bot #4): the engine wasn't being disposed on + shutdown, leaking server-side connection slots until the Postgres + idle-in-transaction timeout fired. This test confirms ``close()`` + nulls the engine, leaves the storage in a non-initialized state, + and a second ``close()`` call is a no-op rather than an exception. + """ + s = RefreshTokenStorage( + database_url=postgres_url, encryption_key=Fernet.generate_key() + ) + await s.initialize() + assert s.engine is not None + assert s._initialized is True + + await s.close() + assert s.engine is None + assert s._initialized is False + + # Idempotent — second close is a no-op, no AttributeError. + await s.close() + assert s.engine is None + + +async def test_concurrent_initialize_serialized_by_advisory_lock( + postgres_url: str, reset_schema +): + """Concurrent pod startup must serialize on pg_advisory_lock. + + PR #798 round-4 review (bot #3): without a migration lock, two + pods racing the rolling-update can both detect ``has_alembic=False`` + and both run ``upgrade_database(URL, "head")``; the second crashes + with "relation already exists". This test spawns three concurrent + ``RefreshTokenStorage.initialize()`` calls against a fresh schema + and asserts all of them complete successfully (the advisory lock + serializes them; the second/third observe ``has_alembic=True`` + after the first commits and take the upgrade fast-path). + """ + import anyio + + async def init_one() -> None: + s = RefreshTokenStorage( + database_url=postgres_url, encryption_key=Fernet.generate_key() + ) + try: + await s.initialize() + finally: + await s.close() + + # No exception = serialization worked. Without the lock, this + # raised ``relation "refresh_tokens" already exists`` on the second + # task in CI runs prior to this fix. + async with anyio.create_task_group() as tg: + for _ in range(3): + tg.start_soon(init_one) + + # Verify the schema actually landed once, not three times: the + # alembic_version table should exist with one row at the head revision. + from sqlalchemy import text + from sqlalchemy.ext.asyncio import create_async_engine + + engine = create_async_engine(postgres_url, future=True) + try: + async with engine.connect() as conn: + result = await conn.execute(text("SELECT count(*) FROM alembic_version")) + (count,) = result.fetchone() + assert count == 1, f"expected 1 alembic_version row, got {count}" + finally: + await engine.dispose() diff --git a/tests/unit/test_storage_logging.py b/tests/unit/test_storage_logging.py index 270369c5..9b825941 100644 --- a/tests/unit/test_storage_logging.py +++ b/tests/unit/test_storage_logging.py @@ -21,8 +21,8 @@ pytestmark = pytest.mark.unit # Synthetic leak-detection sentinel — embedded into test-only URLs so we # can grep ``caplog`` and prove the masking path never emits the literal -# password substring. Not a real credential. NOSONAR S6418 -SENTINEL_PASSWORD_FRAGMENT = "uniqueSecretSentinel123" # NOSONAR S6418 +# password substring. Not a real credential. +SENTINEL_PASSWORD_FRAGMENT = "uniqueSecretSentinel123" # NOSONAR def test_mask_db_password_postgres():