feat(12-02): decompose api/cloud.py into package with connection-ID browse endpoint
- Add api/cloud/ package: connections.py, browse.py, schemas.py, __init__.py
- Add GET /api/cloud/connections/{connection_id}/items (T-12-01 IDOR, T-12-03 cred-free)
- Add PATCH /api/cloud/connections/{id} for display_name_override rename
- Add display_name_override ORM field to CloudConnection model
- Add CloudResourceAdapter service layer with str/UUID coercion
- Fix UUID type compatibility: test_cloud_items.py now uses UUID(as_uuid=True)
matching conftest — removes String(36) patch that caused type incompatibility
- Add 11 Phase 12 integration tests (IDOR, admin block, credential exclusion,
duplicate providers, rename, malformed UUID)
- Remove deleted api/cloud.py (replaced by api/cloud/ package)
This commit is contained in:
@@ -119,6 +119,7 @@ async def db_session():
|
||||
|
||||
# UUID(as_uuid=True) renders as CHAR(32) in SQLite — already handled by
|
||||
# SQLAlchemy's built-in UUID type mapping — no patch needed.
|
||||
_patched_columns: list = [] # kept for finally-block symmetry
|
||||
|
||||
engine = create_async_engine(
|
||||
"sqlite+aiosqlite:///:memory:",
|
||||
@@ -149,6 +150,9 @@ async def db_session():
|
||||
del SQLiteTypeCompiler.visit_JSONB # type: ignore
|
||||
except AttributeError:
|
||||
pass
|
||||
# Restore UUID column types to leave no side effects for other test files
|
||||
for col, orig_type in _patched_columns:
|
||||
col.type = orig_type
|
||||
|
||||
|
||||
@pytest_asyncio.fixture
|
||||
|
||||
@@ -851,3 +851,233 @@ async def test_oauth_initiate_requires_auth(async_client, db_session):
|
||||
)
|
||||
assert resp.status_code in (401, 403), \
|
||||
f"Expected 401 or 403 for unauthenticated request, got {resp.status_code}"
|
||||
|
||||
|
||||
# ── Phase 12: Connection-ID browse tests ──────────────────────────────────────
|
||||
|
||||
async def _create_cloud_connection(session, user_id, provider: str = "google_drive", name: str = "My Drive"):
|
||||
"""Create a CloudConnection row for test fixtures."""
|
||||
from db.models import CloudConnection
|
||||
from storage.cloud_utils import encrypt_credentials
|
||||
|
||||
master_key = b"test-key-for-testing-32bytes!!"
|
||||
creds_enc = encrypt_credentials(master_key, str(user_id), {"access_token": "tok", "refresh_token": "ref"})
|
||||
conn = CloudConnection(
|
||||
id=_uuid.uuid4(),
|
||||
user_id=user_id,
|
||||
provider=provider,
|
||||
display_name=name,
|
||||
credentials_enc=creds_enc,
|
||||
status="ACTIVE",
|
||||
)
|
||||
session.add(conn)
|
||||
await session.commit()
|
||||
return conn
|
||||
|
||||
|
||||
async def test_browse_connection_rejects_foreign_owner(async_client, db_session):
|
||||
"""GET /api/cloud/connections/{id}/items rejects access by a non-owner (T-12-01).
|
||||
|
||||
User2 cannot browse user1's connection — returns 404 (IDOR protection).
|
||||
"""
|
||||
auth1 = await _create_user_and_token(db_session, role="user")
|
||||
auth2 = await _create_user_and_token(db_session, role="user")
|
||||
|
||||
conn = await _create_cloud_connection(db_session, auth1["user"].id)
|
||||
|
||||
resp = await async_client.get(
|
||||
f"/api/cloud/connections/{conn.id}/items",
|
||||
headers=auth2["headers"],
|
||||
)
|
||||
assert resp.status_code == 404, f"Expected 404 IDOR block, got {resp.status_code}"
|
||||
|
||||
|
||||
async def test_browse_connection_admin_blocked(async_client, db_session):
|
||||
"""GET /api/cloud/connections/{id}/items rejects admin tokens (get_regular_user guard)."""
|
||||
auth_admin = await _create_user_and_token(db_session, role="admin")
|
||||
auth_user = await _create_user_and_token(db_session, role="user")
|
||||
|
||||
conn = await _create_cloud_connection(db_session, auth_user["user"].id)
|
||||
|
||||
resp = await async_client.get(
|
||||
f"/api/cloud/connections/{conn.id}/items",
|
||||
headers=auth_admin["headers"],
|
||||
)
|
||||
# Admin tokens are blocked by get_regular_user — 403 or 404
|
||||
assert resp.status_code in (403, 404), f"Expected 403/404 for admin, got {resp.status_code}"
|
||||
|
||||
|
||||
async def test_browse_connection_response_excludes_credentials(async_client, db_session, monkeypatch):
|
||||
"""Browse response never includes credentials_enc or decrypted credential fields (T-12-03)."""
|
||||
from unittest.mock import AsyncMock, patch
|
||||
from storage.cloud_base import CloudListing, CloudCapability, STATE_SUPPORTED
|
||||
|
||||
auth = await _create_user_and_token(db_session, role="user")
|
||||
conn = await _create_cloud_connection(db_session, auth["user"].id)
|
||||
|
||||
# Mock adapter so no real provider call is made
|
||||
mock_adapter = AsyncMock()
|
||||
mock_adapter.list_folder = AsyncMock(return_value=CloudListing(items=(), complete=True))
|
||||
mock_adapter.get_capabilities = AsyncMock(return_value={
|
||||
action: CloudCapability(action=action, state=STATE_SUPPORTED)
|
||||
for action in ["browse"]
|
||||
})
|
||||
|
||||
from storage.cloud_base import ACTIONS, STATE_UNSUPPORTED, REASON_PROVIDER_UNSUPPORTED
|
||||
caps = {
|
||||
action: CloudCapability(
|
||||
action=action,
|
||||
state=STATE_SUPPORTED if action == "browse" else STATE_UNSUPPORTED,
|
||||
reason=None if action == "browse" else REASON_PROVIDER_UNSUPPORTED,
|
||||
message=None if action == "browse" else "Not available.",
|
||||
)
|
||||
for action in ACTIONS
|
||||
}
|
||||
mock_adapter.get_capabilities = AsyncMock(return_value=caps)
|
||||
|
||||
with patch("api.cloud.browse.build_cloud_resource_adapter", return_value=mock_adapter):
|
||||
resp = await async_client.get(
|
||||
f"/api/cloud/connections/{conn.id}/items",
|
||||
headers=auth["headers"],
|
||||
)
|
||||
|
||||
assert resp.status_code == 200, f"Expected 200, got {resp.status_code}: {resp.text}"
|
||||
data = resp.json()
|
||||
|
||||
# T-12-03: credential fields must not be in response
|
||||
assert "credentials_enc" not in data
|
||||
assert "access_token" not in str(data)
|
||||
assert "refresh_token" not in str(data)
|
||||
assert "password" not in str(data)
|
||||
|
||||
# Response schema validation
|
||||
assert "connection_id" in data
|
||||
assert "items" in data
|
||||
assert "capabilities" in data
|
||||
assert "freshness" in data
|
||||
|
||||
|
||||
async def test_browse_two_google_drive_connections_independently(async_client, db_session):
|
||||
"""Two Google Drive connections for one user are independently browsable (D-05)."""
|
||||
from unittest.mock import AsyncMock, patch
|
||||
from storage.cloud_base import CloudListing, CloudCapability, ACTIONS, STATE_SUPPORTED, STATE_UNSUPPORTED, REASON_PROVIDER_UNSUPPORTED
|
||||
|
||||
auth = await _create_user_and_token(db_session, role="user")
|
||||
conn1 = await _create_cloud_connection(db_session, auth["user"].id, provider="google_drive", name="Drive 1")
|
||||
conn2 = await _create_cloud_connection(db_session, auth["user"].id, provider="google_drive", name="Drive 2")
|
||||
|
||||
caps = {
|
||||
action: CloudCapability(
|
||||
action=action,
|
||||
state=STATE_SUPPORTED if action == "browse" else STATE_UNSUPPORTED,
|
||||
reason=None if action == "browse" else REASON_PROVIDER_UNSUPPORTED,
|
||||
message=None if action == "browse" else "Not available.",
|
||||
)
|
||||
for action in ACTIONS
|
||||
}
|
||||
mock_adapter = AsyncMock()
|
||||
mock_adapter.list_folder = AsyncMock(return_value=CloudListing(items=(), complete=True))
|
||||
mock_adapter.get_capabilities = AsyncMock(return_value=caps)
|
||||
|
||||
with patch("api.cloud.browse.build_cloud_resource_adapter", return_value=mock_adapter):
|
||||
resp1 = await async_client.get(f"/api/cloud/connections/{conn1.id}/items", headers=auth["headers"])
|
||||
resp2 = await async_client.get(f"/api/cloud/connections/{conn2.id}/items", headers=auth["headers"])
|
||||
|
||||
assert resp1.status_code == 200
|
||||
assert resp2.status_code == 200
|
||||
assert resp1.json()["connection_id"] == str(conn1.id)
|
||||
assert resp2.json()["connection_id"] == str(conn2.id)
|
||||
|
||||
|
||||
async def test_rename_connection_display_name(async_client, db_session):
|
||||
"""PATCH /api/cloud/connections/{id} renames display_name only (mass-assignment prevention)."""
|
||||
auth = await _create_user_and_token(db_session, role="user")
|
||||
conn = await _create_cloud_connection(db_session, auth["user"].id, name="Original Name")
|
||||
|
||||
resp = await async_client.patch(
|
||||
f"/api/cloud/connections/{conn.id}",
|
||||
json={"display_name": "My Work Drive"},
|
||||
headers=auth["headers"],
|
||||
)
|
||||
assert resp.status_code == 200, f"Expected 200, got {resp.status_code}: {resp.text}"
|
||||
data = resp.json()
|
||||
assert data["display_name"] == "My Work Drive"
|
||||
|
||||
|
||||
async def test_rename_connection_rejects_foreign_owner(async_client, db_session):
|
||||
"""PATCH /api/cloud/connections/{id} rejects non-owner (T-12-01 IDOR)."""
|
||||
auth1 = await _create_user_and_token(db_session, role="user")
|
||||
auth2 = await _create_user_and_token(db_session, role="user")
|
||||
conn = await _create_cloud_connection(db_session, auth1["user"].id, name="Private Drive")
|
||||
|
||||
resp = await async_client.patch(
|
||||
f"/api/cloud/connections/{conn.id}",
|
||||
json={"display_name": "Hacked"},
|
||||
headers=auth2["headers"],
|
||||
)
|
||||
assert resp.status_code == 404
|
||||
|
||||
|
||||
async def test_rename_connection_rejects_blank_name(async_client, db_session):
|
||||
"""PATCH /api/cloud/connections/{id} rejects blank display_name."""
|
||||
auth = await _create_user_and_token(db_session, role="user")
|
||||
conn = await _create_cloud_connection(db_session, auth["user"].id)
|
||||
|
||||
resp = await async_client.patch(
|
||||
f"/api/cloud/connections/{conn.id}",
|
||||
json={"display_name": " "},
|
||||
headers=auth["headers"],
|
||||
)
|
||||
assert resp.status_code == 422
|
||||
|
||||
|
||||
async def test_rename_connection_rejects_mass_assignment(async_client, db_session):
|
||||
"""PATCH /api/cloud/connections/{id} ignores unknown fields (mass-assignment prevention)."""
|
||||
auth = await _create_user_and_token(db_session, role="user")
|
||||
conn = await _create_cloud_connection(db_session, auth["user"].id)
|
||||
|
||||
resp = await async_client.patch(
|
||||
f"/api/cloud/connections/{conn.id}",
|
||||
json={"display_name": "OK Name", "credentials_enc": "HACKED", "provider": "evil"},
|
||||
headers=auth["headers"],
|
||||
)
|
||||
# Should succeed but only update display_name
|
||||
assert resp.status_code == 200
|
||||
data = resp.json()
|
||||
# credentials_enc must not appear in response
|
||||
assert "credentials_enc" not in data
|
||||
|
||||
# Verify provider was not changed in DB
|
||||
from db.models import CloudConnection as CC
|
||||
from sqlalchemy import select
|
||||
result = await db_session.execute(select(CC).where(CC.id == conn.id))
|
||||
updated = result.scalar_one()
|
||||
assert updated.provider == "google_drive" # unchanged
|
||||
|
||||
|
||||
async def test_browse_connection_malformed_uuid(async_client, db_session):
|
||||
"""GET /api/cloud/connections/{bad-uuid}/items returns 422 for malformed UUID."""
|
||||
auth = await _create_user_and_token(db_session, role="user")
|
||||
resp = await async_client.get(
|
||||
"/api/cloud/connections/not-a-valid-uuid/items",
|
||||
headers=auth["headers"],
|
||||
)
|
||||
assert resp.status_code == 422
|
||||
|
||||
|
||||
async def test_list_connections_returns_all_providers(async_client, db_session):
|
||||
"""GET /api/cloud/connections returns all connections including duplicate providers."""
|
||||
auth = await _create_user_and_token(db_session, role="user")
|
||||
conn1 = await _create_cloud_connection(db_session, auth["user"].id, provider="google_drive", name="Drive A")
|
||||
conn2 = await _create_cloud_connection(db_session, auth["user"].id, provider="google_drive", name="Drive B")
|
||||
|
||||
resp = await async_client.get("/api/cloud/connections", headers=auth["headers"])
|
||||
assert resp.status_code == 200
|
||||
items = resp.json()["items"]
|
||||
assert len(items) >= 2
|
||||
ids = {item["id"] for item in items}
|
||||
assert str(conn1.id) in ids
|
||||
assert str(conn2.id) in ids
|
||||
# credentials_enc must not be in any item
|
||||
for item in items:
|
||||
assert "credentials_enc" not in item
|
||||
|
||||
@@ -25,10 +25,10 @@ from typing import Optional
|
||||
|
||||
import pytest
|
||||
import pytest_asyncio
|
||||
from sqlalchemy import String, Text, event
|
||||
from sqlalchemy import Text, event
|
||||
from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine
|
||||
from sqlalchemy.pool import StaticPool
|
||||
from sqlalchemy.dialects.postgresql import UUID, INET, JSONB
|
||||
from sqlalchemy.dialects.postgresql import UUID
|
||||
|
||||
from db.models import Base, CloudItem, CloudItemTopic, CloudFolderState, CloudConnection, Topic, User, Quota
|
||||
from storage.cloud_base import CloudListing, CloudResource, CloudCapability, STATE_SUPPORTED
|
||||
@@ -48,69 +48,80 @@ from services.cloud_items import (
|
||||
|
||||
@pytest_asyncio.fixture
|
||||
async def db_session():
|
||||
"""In-memory SQLite session with PostgreSQL-type shims."""
|
||||
"""In-memory SQLite session with PostgreSQL-type shims.
|
||||
|
||||
Patches INET/JSONB to Text for SQLite compatibility.
|
||||
UUID(as_uuid=True) is left as-is: SQLAlchemy renders it as CHAR(32) and
|
||||
the bind processor handles uuid.UUID ↔ 32-char hex automatically.
|
||||
"""
|
||||
from sqlalchemy.dialects.sqlite.base import SQLiteTypeCompiler
|
||||
from sqlalchemy.dialects.postgresql import INET, JSONB
|
||||
|
||||
_orig_visit_INET = getattr(SQLiteTypeCompiler, "visit_INET", None)
|
||||
_orig_visit_JSONB = getattr(SQLiteTypeCompiler, "visit_JSONB", None)
|
||||
|
||||
def _visit_inet(self, type_, **kw):
|
||||
return "TEXT"
|
||||
|
||||
def _visit_jsonb(self, type_, **kw):
|
||||
return "TEXT"
|
||||
|
||||
SQLiteTypeCompiler.visit_INET = _visit_inet # type: ignore[attr-defined]
|
||||
SQLiteTypeCompiler.visit_JSONB = _visit_jsonb # type: ignore[attr-defined]
|
||||
|
||||
engine = create_async_engine(
|
||||
"sqlite+aiosqlite:///:memory:",
|
||||
connect_args={"check_same_thread": False},
|
||||
poolclass=StaticPool,
|
||||
)
|
||||
|
||||
# Shim PostgreSQL types to SQLite-compatible equivalents
|
||||
from sqlalchemy import event as sa_event
|
||||
try:
|
||||
async with engine.begin() as conn:
|
||||
await conn.run_sync(Base.metadata.create_all)
|
||||
|
||||
@sa_event.listens_for(engine.sync_engine, "connect")
|
||||
def set_sqlite_pragma(dbapi_connection, connection_record):
|
||||
cursor = dbapi_connection.cursor()
|
||||
cursor.execute("PRAGMA foreign_keys=ON")
|
||||
cursor.close()
|
||||
|
||||
# Patch dialect-specific column types before table creation
|
||||
import sqlalchemy.dialects.postgresql as pg
|
||||
_orig_uuid_init = pg.UUID.__init__
|
||||
|
||||
def _patch_columns(metadata):
|
||||
for table in metadata.tables.values():
|
||||
for col in table.columns:
|
||||
if isinstance(col.type, pg.UUID):
|
||||
col.type = String(36)
|
||||
elif isinstance(col.type, pg.INET):
|
||||
col.type = String(45)
|
||||
elif isinstance(col.type, pg.JSONB):
|
||||
col.type = Text()
|
||||
|
||||
async with engine.begin() as conn:
|
||||
_patch_columns(Base.metadata)
|
||||
await conn.run_sync(Base.metadata.create_all)
|
||||
|
||||
session_factory = async_sessionmaker(engine, expire_on_commit=False)
|
||||
async with session_factory() as session:
|
||||
yield session
|
||||
|
||||
async with engine.begin() as conn:
|
||||
await conn.run_sync(Base.metadata.drop_all)
|
||||
await engine.dispose()
|
||||
session_factory = async_sessionmaker(engine, expire_on_commit=False)
|
||||
async with session_factory() as session:
|
||||
yield session
|
||||
finally:
|
||||
async with engine.begin() as conn:
|
||||
await conn.run_sync(Base.metadata.drop_all)
|
||||
await engine.dispose()
|
||||
if _orig_visit_INET is not None:
|
||||
SQLiteTypeCompiler.visit_INET = _orig_visit_INET # type: ignore
|
||||
else:
|
||||
try:
|
||||
del SQLiteTypeCompiler.visit_INET # type: ignore
|
||||
except AttributeError:
|
||||
pass
|
||||
if _orig_visit_JSONB is not None:
|
||||
SQLiteTypeCompiler.visit_JSONB = _orig_visit_JSONB # type: ignore
|
||||
else:
|
||||
try:
|
||||
del SQLiteTypeCompiler.visit_JSONB # type: ignore
|
||||
except AttributeError:
|
||||
pass
|
||||
|
||||
|
||||
# ── Helpers ───────────────────────────────────────────────────────────────────
|
||||
|
||||
def _user_id() -> str:
|
||||
return str(uuid.uuid4())
|
||||
def _user_id() -> uuid.UUID:
|
||||
return uuid.uuid4()
|
||||
|
||||
|
||||
def _conn_id() -> str:
|
||||
return str(uuid.uuid4())
|
||||
def _conn_id() -> uuid.UUID:
|
||||
return uuid.uuid4()
|
||||
|
||||
|
||||
def _item_id() -> str:
|
||||
return str(uuid.uuid4())
|
||||
def _item_id() -> uuid.UUID:
|
||||
return uuid.uuid4()
|
||||
|
||||
|
||||
async def _make_user(session: AsyncSession, user_id: Optional[str] = None) -> str:
|
||||
async def _make_user(session: AsyncSession, user_id: Optional[uuid.UUID] = None) -> uuid.UUID:
|
||||
uid = user_id or _user_id()
|
||||
u = User(
|
||||
id=uid,
|
||||
handle=f"user_{uid[:8]}",
|
||||
email=f"{uid[:8]}@example.com",
|
||||
handle=f"user_{uid.hex[:8]}",
|
||||
email=f"{uid.hex[:8]}@example.com",
|
||||
password_hash="hash",
|
||||
role="user",
|
||||
)
|
||||
@@ -120,8 +131,8 @@ async def _make_user(session: AsyncSession, user_id: Optional[str] = None) -> st
|
||||
|
||||
|
||||
async def _make_connection(
|
||||
session: AsyncSession, user_id: str, conn_id: Optional[str] = None
|
||||
) -> str:
|
||||
session: AsyncSession, user_id: uuid.UUID, conn_id: Optional[uuid.UUID] = None
|
||||
) -> uuid.UUID:
|
||||
cid = conn_id or _conn_id()
|
||||
conn = CloudConnection(
|
||||
id=cid,
|
||||
@@ -137,8 +148,8 @@ async def _make_connection(
|
||||
|
||||
|
||||
def _cloud_resource(
|
||||
connection_id: str,
|
||||
user_id: str,
|
||||
connection_id: uuid.UUID,
|
||||
user_id: uuid.UUID,
|
||||
provider_item_id: str = "item-001",
|
||||
name: str = "test.pdf",
|
||||
kind: str = "file",
|
||||
@@ -149,8 +160,8 @@ def _cloud_resource(
|
||||
return CloudResource(
|
||||
id=uuid.uuid4(),
|
||||
provider_item_id=provider_item_id,
|
||||
connection_id=uuid.UUID(connection_id),
|
||||
user_id=uuid.UUID(user_id),
|
||||
connection_id=connection_id,
|
||||
user_id=user_id,
|
||||
name=name,
|
||||
kind=kind,
|
||||
parent_ref=parent_ref,
|
||||
@@ -359,7 +370,7 @@ async def test_service_resolve_owned_connection_found(db_session: AsyncSession):
|
||||
cid = await _make_connection(db_session, uid)
|
||||
|
||||
conn = await resolve_owned_connection(db_session, connection_id=cid, user_id=uid)
|
||||
assert str(conn.id) == cid
|
||||
assert conn.id == cid
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
||||
Reference in New Issue
Block a user