fix(05): revise Phase 5 plans based on checker feedback — B1-B4, W1-W4
B1: Mark RESEARCH.md Open Questions as (RESOLVED) with decision text for all 3
B2: Backends now stateless — raise CloudConnectionError(reason=) only; API layer
in cloud.py owns token refresh + DB update via _call_cloud_op helper
B3: Add Task 3 to Plan 05 — cloud connection + object cleanup on account deletion (SEC-09)
B4: Add frontend_url setting to Plan 01 Task 1; Plan 05 uses settings.frontend_url
for OAuth callback redirects
W1: ROADMAP.md Phase 5 now correctly labels Plans 03+04 as Wave 3 (not Wave 2)
W2: Plan 06 invalid_grant test now asserts both 503 HTTP response AND DB REQUIRES_REAUTH
W3: Plan 06 Task 2 split into unit tests (4, cloud_utils.py) and integration tests (11, HTTP)
W4: Plan 07 adds Vitest tests for cloudConnections store (4 tests) and SettingsCloudTab
mount test (2 tests) per CLAUDE.md testing protocol
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -161,58 +161,115 @@ print('documents.py parses without error: OK')
|
||||
</task>
|
||||
|
||||
<task type="auto" tdd="true">
|
||||
<name>Task 2: Promote all 15 xfail stubs to real passing tests</name>
|
||||
<name>Task 2: Promote unit test stubs to real tests (cloud_utils.py coverage)</name>
|
||||
<files>backend/tests/test_cloud.py</files>
|
||||
<read_first>
|
||||
- backend/tests/test_cloud.py — current 15 xfail stubs
|
||||
- backend/tests/test_cloud.py — current xfail stubs
|
||||
- backend/storage/cloud_utils.py — validate_cloud_url, encrypt_credentials, decrypt_credentials
|
||||
- backend/storage/__init__.py — get_storage_backend_for_document
|
||||
- backend/storage/minio_backend.py — MinIOBackend class
|
||||
</read_first>
|
||||
<behavior>
|
||||
- 4 unit tests promoted; they test cloud_utils.py and the factory — no DB, no HTTP client, no network (W3 split: unit tests only)
|
||||
- test_credential_round_trip: pure unit test; calls encrypt_credentials + decrypt_credentials; asserts round-trip equals original; asserts ciphertext != plaintext string
|
||||
- test_ssrf_validation: @pytest.mark.parametrize over [("http://localhost/dav",True),("http://127.0.0.1/dav",True),("http://169.254.169.254/dav",True),("http://10.0.0.1/dav",True),("http://192.168.1.1/dav",True),("https://nextcloud.example.com/dav",False)]; asserts ValueError raised for private IPs; no exception for valid public URL
|
||||
- test_ssrf_link_local: calls validate_cloud_url("http://169.254.169.254/metadata"); asserts ValueError
|
||||
- test_factory_returns_correct_backend: constructs a mock Document(storage_backend="minio") and mock User; patches get_storage_backend() to return a MagicMock of MinIOBackend; calls get_storage_backend_for_document with a mock AsyncSession; asserts result is the expected backend type
|
||||
</behavior>
|
||||
<action>
|
||||
Promote the 4 unit-test stubs in test_cloud.py. These tests have no DB/HTTP dependencies:
|
||||
|
||||
1. test_credential_round_trip — no fixtures needed:
|
||||
from storage.cloud_utils import encrypt_credentials, decrypt_credentials
|
||||
master_key = b"test-master-key-32bytes-padded!!"
|
||||
user_id = "550e8400-e29b-41d4-a716-446655440000"
|
||||
creds = {"access_token": "ya29.xxx", "refresh_token": "1//xxx"}
|
||||
enc = encrypt_credentials(master_key, user_id, creds)
|
||||
assert isinstance(enc, str) and "access_token" not in enc
|
||||
dec = decrypt_credentials(master_key, user_id, enc)
|
||||
assert dec == creds
|
||||
|
||||
2. test_ssrf_validation — @pytest.mark.parametrize:
|
||||
All private/loopback/link-local URLs raise ValueError; valid public URL passes.
|
||||
Remove the xfail decorator; add parametrize decorator from behavior spec.
|
||||
|
||||
3. test_ssrf_link_local — simple unit test:
|
||||
from storage.cloud_utils import validate_cloud_url
|
||||
with pytest.raises(ValueError): validate_cloud_url("http://169.254.169.254/metadata")
|
||||
|
||||
4. test_factory_returns_correct_backend — mock-based unit test:
|
||||
from unittest.mock import MagicMock, AsyncMock, patch
|
||||
from storage import get_storage_backend_for_document
|
||||
Mock a Document with storage_backend="minio", a User, and an AsyncSession.
|
||||
Patch get_storage_backend() to return a MinIOBackend mock.
|
||||
Run asyncio.run(get_storage_backend_for_document(mock_doc, mock_user, mock_session)).
|
||||
Assert result is the patched MinIOBackend.
|
||||
|
||||
Remove @pytest.mark.xfail(strict=False) from all 4 stubs once implemented.
|
||||
Leave the other 11 stubs with xfail decorators (they are promoted in Task 3).
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd /Users/nik/Documents/Progamming/document_scanner/backend && python -m pytest tests/test_cloud.py::test_credential_round_trip tests/test_cloud.py::test_ssrf_validation tests/test_cloud.py::test_ssrf_link_local tests/test_cloud.py::test_factory_returns_correct_backend -v 2>&1 | tail -10</automated>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- test_credential_round_trip, test_ssrf_validation, test_ssrf_link_local, test_factory_returns_correct_backend all PASSED
|
||||
- test_ssrf_validation is parametrized (multiple params visible in output)
|
||||
- No xfail decorators on these 4 tests
|
||||
- Other 11 tests still xfail (not broken by this task)
|
||||
- `pytest tests/test_cloud.py -v` exits 0
|
||||
</acceptance_criteria>
|
||||
<done>4 unit tests promoted to PASSED; cloud_utils.py coverage established; 11 integration stubs still xfailed</done>
|
||||
</task>
|
||||
|
||||
<task type="auto" tdd="true">
|
||||
<name>Task 3: Promote integration test stubs to real passing tests (HTTP endpoint coverage)</name>
|
||||
<files>backend/tests/test_cloud.py</files>
|
||||
<read_first>
|
||||
- backend/tests/test_cloud.py — current xfail stubs (11 remaining after Task 2)
|
||||
- backend/tests/conftest.py — all fixtures including cloud_connection_factory, mock_google_drive_creds, async_client, db_session
|
||||
- backend/api/cloud.py — endpoint paths and request/response shapes
|
||||
- backend/api/admin.py — CloudConnectionOut fields
|
||||
- backend/storage/cloud_utils.py — validate_cloud_url, encrypt_credentials, decrypt_credentials
|
||||
- .planning/phases/05-cloud-storage-backends/05-VALIDATION.md — test map with requirement → test correspondence
|
||||
- backend/db/models.py — CloudConnection, User, Document fields
|
||||
- .planning/phases/05-cloud-storage-backends/05-VALIDATION.md — test map with requirement → test correspondence
|
||||
</read_first>
|
||||
<behavior>
|
||||
- All 15 tests pass (no xfailed, no failed) after implementation
|
||||
- test_credential_round_trip: pure unit test; calls encrypt_credentials + decrypt_credentials; asserts round-trip equals original; asserts ciphertext != plaintext
|
||||
- 11 integration tests promoted; all use async_client, db_session, and/or monkeypatch (W3 split: integration tests only)
|
||||
- test_credentials_enc_not_exposed: creates CloudConnection via cloud_connection_factory; calls GET /api/cloud/connections with valid auth; asserts "credentials_enc" not in response JSON at any level
|
||||
- test_cloud_upload_no_presigned: creates CloudConnection; mocks cloud backend put_object; calls POST /api/documents/upload with target_backend="google_drive"; asserts no "upload_url" in response
|
||||
- test_connection_status_display: creates ACTIVE CloudConnection; calls GET /api/cloud/connections; asserts response item has status == "ACTIVE"
|
||||
- test_invalid_grant_sets_requires_reauth: creates CloudConnection; monkey-patches get_storage_backend_for_document to raise CloudConnectionError; calls GET /api/documents/{id}/content; asserts 503 response; then separately tests that the DB connection has status == "REQUIRES_REAUTH" after the transition is triggered through the backend
|
||||
- test_invalid_grant_sets_requires_reauth: creates CloudConnection with status="ACTIVE"; monkey-patches the cloud backend operation to raise CloudConnectionError(reason="invalid_grant"); calls GET /api/documents/{id}/content; asserts 503 response; then re-queries the CloudConnection from DB and asserts connection.status == "REQUIRES_REAUTH" — both HTTP response AND DB state verified (W2 fix)
|
||||
- test_disconnect_deletes_credentials: creates CloudConnection; calls DELETE /api/cloud/connections/{id}; asserts 204; queries DB to confirm row deleted
|
||||
- test_factory_returns_correct_backend: calls get_storage_backend_for_document with mock Document(storage_backend="minio"); asserts isinstance result MinIOBackend
|
||||
- test_ssrf_validation: parametrized over RFC-1918, loopback, link-local, valid URL inputs; asserts ValueError raised for private IPs; no exception for valid public URL
|
||||
- test_ssrf_link_local: calls validate_cloud_url("http://169.254.169.254/metadata"); asserts ValueError
|
||||
- test_admin_cannot_see_credentials: creates admin user + CloudConnection; calls GET /api/cloud/connections with admin auth; asserts 403 response
|
||||
- test_cross_user_idor: creates two users + CloudConnections; calls DELETE /api/cloud/connections/{user2_connection_id} with user1 auth; asserts 404
|
||||
- test_connect_google_drive: calls GET /api/cloud/oauth/initiate/google_drive with valid auth; asserts 302 redirect containing "accounts.google.com" in location header; asserts Redis key "oauth_state:" exists
|
||||
- test_connect_google_drive: calls GET /api/cloud/oauth/initiate/google_drive with valid auth; asserts 302 redirect containing "accounts.google.com" in location header
|
||||
- test_oauth_callback_valid_state: pre-seeds Redis with oauth_state key; mocks google_auth_oauthlib.flow.Flow.fetch_token; calls GET /api/cloud/oauth/callback/google_drive?code=test&state={seed_state}; asserts 302 redirect to /settings?cloud_connected=google_drive
|
||||
- test_oauth_callback_invalid_state: calls GET /api/cloud/oauth/callback/google_drive?code=x&state=invalid; asserts 400
|
||||
- test_webdav_connect_validates: mocks WebDAVBackend health_check to return False; calls POST /api/cloud/connections/webdav with localhost URL; asserts 422 (SSRF blocked before health check)
|
||||
|
||||
For tests requiring auth: use helper to create User rows and generate access tokens (pattern from test_auth_api.py or test_documents.py).
|
||||
For tests requiring Redis: use monkeypatch to mock app.state.redis.setex, get, delete.
|
||||
For tests requiring cloud SDKs: monkeypatch/MagicMock the SDK calls — no real network calls in tests.
|
||||
- test_webdav_connect_validates: calls POST /api/cloud/connections/webdav with localhost URL; asserts 422 (SSRF blocked — validate_cloud_url raises ValueError before health check)
|
||||
</behavior>
|
||||
<action>
|
||||
Rewrite backend/tests/test_cloud.py, replacing each pytest.xfail("not implemented yet") stub body with a real test implementation.
|
||||
Promote the 11 remaining xfail stubs in test_cloud.py to real integration tests.
|
||||
|
||||
Keep: all 15 test function names, all @pytest.mark.asyncio decorators, pytestmark = pytest.mark.asyncio.
|
||||
Remove: @pytest.mark.xfail(strict=False) decorators from all stubs once each is implemented.
|
||||
Add: proper fixture parameters to each test function (async_client, db_session, monkeypatch, etc.).
|
||||
Keep: all 11 test function names, all @pytest.mark.asyncio decorators.
|
||||
Remove: @pytest.mark.xfail(strict=False) from all 11 stubs.
|
||||
Add: proper fixture parameters (async_client, db_session, monkeypatch).
|
||||
|
||||
Auth helper (add as a local conftest helper or module-level fixture):
|
||||
async def _create_user_and_token(session, role="user") — creates User row, generates JWT access token
|
||||
(Mirror pattern from existing test_auth_api.py or test_documents.py)
|
||||
Auth helper (add as module-level async def or import from conftest):
|
||||
async def _create_user_and_token(session, role="user") — creates User row, generates JWT access token.
|
||||
Mirror pattern from existing test_auth_api.py or test_documents.py.
|
||||
|
||||
For test_credential_round_trip: no fixtures needed (pure unit test).
|
||||
For test_ssrf_validation: parametrize with @pytest.mark.parametrize.
|
||||
For tests needing cloud API: use async_client fixture.
|
||||
For tests needing Redis: monkeypatch app.state.redis.
|
||||
For Redis tests (test_connect_google_drive, test_oauth_callback_valid_state, test_oauth_callback_invalid_state):
|
||||
monkeypatch app.state.redis.setex, app.state.redis.get, app.state.redis.delete.
|
||||
test_oauth_callback_valid_state: pre-seed via monkeypatch return values; mock Flow.fetch_token.
|
||||
|
||||
Important: tests must pass under SQLite in-memory (non-INTEGRATION mode). Cloud SDK calls must be mocked (no real network calls). OAuth state tests mock Redis.
|
||||
For test_invalid_grant_sets_requires_reauth (W2 requirement):
|
||||
Create CloudConnection; monkeypatch get_storage_backend_for_document to raise
|
||||
CloudConnectionError(reason="invalid_grant"); call GET /api/documents/{id}/content;
|
||||
assert 503; then session.refresh(connection); assert connection.status == "REQUIRES_REAUTH".
|
||||
Note: the DB write of REQUIRES_REAUTH must actually be committed by _call_cloud_op —
|
||||
test verifies the real DB state, not just the HTTP response.
|
||||
|
||||
When implementing test_invalid_grant_sets_requires_reauth: focus on the 503 response assertion (the backend routing returning 503 when CloudConnectionError is raised). The REQUIRES_REAUTH DB update happens inside the cloud backend during the operation — for unit testing, verify the 503 response is returned and trust the integration test to verify the DB state.
|
||||
For SDK mocking: monkeypatch or patch the SDK calls at the module import level.
|
||||
All tests must pass under SQLite in-memory (non-INTEGRATION mode).
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd /Users/nik/Documents/Progamming/document_scanner/backend && python -m pytest tests/test_cloud.py -v 2>&1</automated>
|
||||
@@ -220,8 +277,7 @@ print('documents.py parses without error: OK')
|
||||
<acceptance_criteria>
|
||||
- `pytest tests/test_cloud.py -v` exits 0
|
||||
- Output shows all 15 tests PASSED (no xfailed, no FAILED, no ERROR)
|
||||
- test_credential_round_trip: no xfail decorator; passes with round-trip assertion
|
||||
- test_ssrf_validation: parametrized; all params pass
|
||||
- test_invalid_grant_sets_requires_reauth: 503 HTTP response AND DB connection.status == "REQUIRES_REAUTH" (W2 + W3 combined)
|
||||
- test_credentials_enc_not_exposed: "credentials_enc" not present anywhere in response JSON
|
||||
- test_admin_cannot_see_credentials: 403 for admin role
|
||||
- test_cross_user_idor: 404 for cross-user connection access
|
||||
@@ -258,6 +314,8 @@ cd /Users/nik/Documents/Progamming/document_scanner/backend && python -m pytest
|
||||
<success_criteria>
|
||||
- POST /api/documents/upload: target_backend routing works for cloud backends; MinIO flow unchanged
|
||||
- GET /api/documents/{id}/content: uses get_storage_backend_for_document; CloudConnectionError → 503
|
||||
- test_cloud.py Task 2 (unit): test_credential_round_trip, test_ssrf_validation, test_ssrf_link_local, test_factory_returns_correct_backend all PASSED
|
||||
- test_cloud.py Task 3 (integration): all 11 integration stubs PASSED including REQUIRES_REAUTH DB assertion
|
||||
- test_cloud.py: all 15 tests PASSED; no xfailed
|
||||
- pytest -v (full suite): exits 0, 0 failures
|
||||
</success_criteria>
|
||||
|
||||
Reference in New Issue
Block a user