Chris CoutinhoandClaude Opus 4.7
f2b7bf132f
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) <noreply@anthropic.com>
212 lines
7.6 KiB
Python
212 lines
7.6 KiB
Python
"""
|
|
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
|
|
import time
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
from nextcloud_mcp_server.auth.storage import RefreshTokenStorage
|
|
|
|
pytestmark = pytest.mark.unit
|
|
|
|
|
|
@pytest.fixture
|
|
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()
|
|
try:
|
|
yield storage
|
|
finally:
|
|
await storage_backend["reset"]()
|
|
|
|
|
|
async def test_store_webhook(temp_storage):
|
|
"""Test storing a webhook."""
|
|
await temp_storage.store_webhook(webhook_id=123, preset_id="notes_sync")
|
|
|
|
webhooks = await temp_storage.list_all_webhooks()
|
|
assert len(webhooks) == 1
|
|
assert webhooks[0]["webhook_id"] == 123
|
|
assert webhooks[0]["preset_id"] == "notes_sync"
|
|
assert "created_at" in webhooks[0]
|
|
|
|
|
|
async def test_store_webhook_duplicate(temp_storage):
|
|
"""Test storing duplicate webhook replaces existing."""
|
|
await temp_storage.store_webhook(webhook_id=123, preset_id="notes_sync")
|
|
await temp_storage.store_webhook(webhook_id=123, preset_id="calendar_sync")
|
|
|
|
webhooks = await temp_storage.list_all_webhooks()
|
|
# Should only have one entry due to UNIQUE constraint
|
|
assert len(webhooks) == 1
|
|
assert webhooks[0]["preset_id"] == "calendar_sync"
|
|
|
|
|
|
async def test_get_webhooks_by_preset(temp_storage):
|
|
"""Test retrieving webhooks by preset."""
|
|
await temp_storage.store_webhook(webhook_id=123, preset_id="notes_sync")
|
|
await temp_storage.store_webhook(webhook_id=456, preset_id="notes_sync")
|
|
await temp_storage.store_webhook(webhook_id=789, preset_id="calendar_sync")
|
|
|
|
notes_webhooks = await temp_storage.get_webhooks_by_preset("notes_sync")
|
|
assert len(notes_webhooks) == 2
|
|
assert 123 in notes_webhooks
|
|
assert 456 in notes_webhooks
|
|
|
|
calendar_webhooks = await temp_storage.get_webhooks_by_preset("calendar_sync")
|
|
assert len(calendar_webhooks) == 1
|
|
assert 789 in calendar_webhooks
|
|
|
|
|
|
async def test_get_webhooks_by_preset_empty(temp_storage):
|
|
"""Test retrieving webhooks for non-existent preset."""
|
|
webhooks = await temp_storage.get_webhooks_by_preset("nonexistent")
|
|
assert len(webhooks) == 0
|
|
|
|
|
|
async def test_delete_webhook(temp_storage):
|
|
"""Test deleting a webhook."""
|
|
await temp_storage.store_webhook(webhook_id=123, preset_id="notes_sync")
|
|
await temp_storage.store_webhook(webhook_id=456, preset_id="notes_sync")
|
|
|
|
deleted = await temp_storage.delete_webhook(webhook_id=123)
|
|
assert deleted is True
|
|
|
|
webhooks = await temp_storage.get_webhooks_by_preset("notes_sync")
|
|
assert len(webhooks) == 1
|
|
assert 456 in webhooks
|
|
|
|
|
|
async def test_delete_webhook_nonexistent(temp_storage):
|
|
"""Test deleting non-existent webhook."""
|
|
deleted = await temp_storage.delete_webhook(webhook_id=999)
|
|
assert deleted is False
|
|
|
|
|
|
async def test_list_all_webhooks(temp_storage):
|
|
"""Test listing all webhooks."""
|
|
await temp_storage.store_webhook(webhook_id=123, preset_id="notes_sync")
|
|
await temp_storage.store_webhook(webhook_id=456, preset_id="calendar_sync")
|
|
await temp_storage.store_webhook(webhook_id=789, preset_id="notes_sync")
|
|
|
|
webhooks = await temp_storage.list_all_webhooks()
|
|
assert len(webhooks) == 3
|
|
|
|
# Verify all expected fields present
|
|
for webhook in webhooks:
|
|
assert "webhook_id" in webhook
|
|
assert "preset_id" in webhook
|
|
assert "created_at" in webhook
|
|
|
|
# Verify webhook IDs
|
|
webhook_ids = [w["webhook_id"] for w in webhooks]
|
|
assert 123 in webhook_ids
|
|
assert 456 in webhook_ids
|
|
assert 789 in webhook_ids
|
|
|
|
|
|
async def test_list_all_webhooks_empty(temp_storage):
|
|
"""Test listing webhooks when none exist."""
|
|
webhooks = await temp_storage.list_all_webhooks()
|
|
assert len(webhooks) == 0
|
|
|
|
|
|
async def test_clear_preset_webhooks(temp_storage):
|
|
"""Test clearing all webhooks for a preset."""
|
|
await temp_storage.store_webhook(webhook_id=123, preset_id="notes_sync")
|
|
await temp_storage.store_webhook(webhook_id=456, preset_id="notes_sync")
|
|
await temp_storage.store_webhook(webhook_id=789, preset_id="calendar_sync")
|
|
|
|
deleted_count = await temp_storage.clear_preset_webhooks("notes_sync")
|
|
assert deleted_count == 2
|
|
|
|
# Verify notes_sync webhooks are gone
|
|
notes_webhooks = await temp_storage.get_webhooks_by_preset("notes_sync")
|
|
assert len(notes_webhooks) == 0
|
|
|
|
# Verify calendar_sync webhook still exists
|
|
calendar_webhooks = await temp_storage.get_webhooks_by_preset("calendar_sync")
|
|
assert len(calendar_webhooks) == 1
|
|
assert 789 in calendar_webhooks
|
|
|
|
|
|
async def test_clear_preset_webhooks_nonexistent(temp_storage):
|
|
"""Test clearing webhooks for non-existent preset."""
|
|
deleted_count = await temp_storage.clear_preset_webhooks("nonexistent")
|
|
assert deleted_count == 0
|
|
|
|
|
|
async def test_webhook_timestamps(temp_storage):
|
|
"""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()
|
|
|
|
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 isinstance(created_at, int)
|
|
assert int(start_time) <= created_at <= int(end_time) + 1
|
|
|
|
|
|
async def test_storage_without_encryption_key():
|
|
"""Test that storage can be initialized without encryption key."""
|
|
with tempfile.TemporaryDirectory() as tmpdir:
|
|
db_path = Path(tmpdir) / "test_no_encryption.db"
|
|
storage = RefreshTokenStorage(db_path=str(db_path), encryption_key=None)
|
|
await storage.initialize()
|
|
|
|
# Webhook operations should work without encryption key
|
|
await storage.store_webhook(webhook_id=123, preset_id="notes_sync")
|
|
webhooks = await storage.get_webhooks_by_preset("notes_sync")
|
|
assert len(webhooks) == 1
|
|
assert 123 in webhooks
|
|
|
|
|
|
async def test_multiple_presets_independence(temp_storage):
|
|
"""Test that different presets maintain independent webhook lists."""
|
|
presets = ["notes_sync", "calendar_sync", "deck_sync", "files_sync"]
|
|
|
|
# Store webhooks for each preset
|
|
for i, preset in enumerate(presets):
|
|
webhook_id = 100 + i
|
|
await temp_storage.store_webhook(webhook_id=webhook_id, preset_id=preset)
|
|
|
|
# Verify each preset has exactly one webhook
|
|
for i, preset in enumerate(presets):
|
|
webhooks = await temp_storage.get_webhooks_by_preset(preset)
|
|
assert len(webhooks) == 1
|
|
assert (100 + i) in webhooks
|
|
|
|
# Clear one preset
|
|
deleted = await temp_storage.clear_preset_webhooks("notes_sync")
|
|
assert deleted == 1
|
|
|
|
# Verify other presets unchanged
|
|
for preset in ["calendar_sync", "deck_sync", "files_sync"]:
|
|
webhooks = await temp_storage.get_webhooks_by_preset(preset)
|
|
assert len(webhooks) == 1
|