From 4f8daef83caae7cf70b2633cf12e697bfa765ced Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 01:17:49 +0000 Subject: [PATCH 01/27] fix: create workspace_entities/workspace_documents and provision Workspace rows Workspace/Document (workspace_entities/workspace_documents) have been declared in db/models.py since June, but no Alembic migration ever created them explicitly, and no production write path ever inserted a Workspace row. A database that incrementally migrated forward before these models existed never gets the tables (0001's Base.metadata.create_all only reflects today's model metadata, not a historical snapshot), and even where the tables exist, /api/data/documents' Document inserts always violated the workspace_id foreign key since nothing ever created the referenced Workspace row for a real signed session. - Add 0018_workspace_registry.py: idempotent (has_table-guarded) creation of both tables, matching the current model shape and this repo's structured-migration convention. - Add services/workspace_scope.get_or_create_workspace and wire it into both Document-creating endpoints in api/data.py, keyed by the signed session's real workspace claim (workspace-, confirmed against every other call site that derives it) rather than the model's own opaque uuid default. - Fix two unrelated, independently-discovered bugs blocking the documented Alembic path (scripts/migrate_db.py) from ever completing on a genuinely fresh database: schema_backfill_sql() and 0011_email_read_state.py both still targeted the "emails" table, renamed to "email_records" by 0011_email_model_reconciliation long ago. - Add test_workspace_document_migration.py: runs the real scripts/migrate_db.py against a disposable Postgres database (never create_all) and proves /api/data/documents serves cleanly both from an empty database and from one that had already migrated past the point where the registry tables would otherwise be missing. Verified locally against a real PostgreSQL 16 instance: the full Alembic chain now runs 0001->head cleanly from empty, and the full backend test suite passes (1836 passed; the 2 remaining failures are a pre-existing, unrelated is_read NOT NULL smoke-test bug, confirmed present on unmodified develop before this change). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016ErwZSYW3pm585NiM3Q7aN --- .../alembic/versions/0011_email_read_state.py | 36 ++- .../versions/0018_workspace_registry.py | 85 +++++++ backend/api/data.py | 3 + backend/scripts/bootstrap_db.py | 4 - backend/services/workspace_scope.py | 27 +++ backend/tests/test_data_api.py | 29 +++ .../test_workspace_document_migration.py | 218 ++++++++++++++++++ 7 files changed, 387 insertions(+), 15 deletions(-) create mode 100644 backend/alembic/versions/0018_workspace_registry.py create mode 100644 backend/services/workspace_scope.py create mode 100644 backend/tests/test_workspace_document_migration.py diff --git a/backend/alembic/versions/0011_email_read_state.py b/backend/alembic/versions/0011_email_read_state.py index 716590cd1..d1677399e 100644 --- a/backend/alembic/versions/0011_email_read_state.py +++ b/backend/alembic/versions/0011_email_read_state.py @@ -1,4 +1,4 @@ -"""Add is_read to emails (IMAP \\Seen read state). +"""Add is_read to email_records (IMAP \\Seen read state). Existing rows default to read so historical/file imports do not surface as unread. """ @@ -12,18 +12,32 @@ branch_labels = None depends_on = None +_EMAIL_TABLE = "email_records" + def upgrade() -> None: - op.add_column( - "emails", - sa.Column( - "is_read", - sa.Boolean(), - nullable=False, - server_default=sa.text("true"), - ), - ) + connection = op.get_bind() + inspector = sa.inspect(connection) + if not _has_column(inspector, _EMAIL_TABLE, "is_read"): + op.add_column( + _EMAIL_TABLE, + sa.Column( + "is_read", + sa.Boolean(), + nullable=False, + server_default=sa.text("true"), + ), + ) def downgrade() -> None: - op.drop_column("emails", "is_read") + connection = op.get_bind() + inspector = sa.inspect(connection) + if _has_column(inspector, _EMAIL_TABLE, "is_read"): + op.drop_column(_EMAIL_TABLE, "is_read") + + +def _has_column(inspector, table_name: str, column_name: str) -> bool: + return any( + column["name"] == column_name for column in inspector.get_columns(table_name) + ) diff --git a/backend/alembic/versions/0018_workspace_registry.py b/backend/alembic/versions/0018_workspace_registry.py new file mode 100644 index 000000000..cbd23a057 --- /dev/null +++ b/backend/alembic/versions/0018_workspace_registry.py @@ -0,0 +1,85 @@ +"""create workspace registry and workspace document tables + +Revision ID: 0018_workspace_registry +Revises: 0017_merge_newsdom_carddav_heads +Create Date: 2026-09-01 00:00:00.000000 + +``Workspace``/``Document`` (``workspace_entities``/``workspace_documents``) have +been declared in ``db/models.py`` since before this repository's incremental +migration history begins tracking them explicitly. A database that ran +``0001_initial_control_plane``'s ``Base.metadata.create_all`` after these +models existed already has both tables; a database that ran ``0001`` earlier +and has only applied incremental migrations since never got them, so +``/api/data/documents`` fails with an undefined-relation error the first time +it is hit. This revision is idempotent (``has_table`` guarded) so it is a +no-op for a database that already has the tables and a real fix for one that +does not. +""" + +from alembic import op +import sqlalchemy as sa + +revision = "0018_workspace_registry" +down_revision = "0017_merge_newsdom_carddav_heads" + +_ENTITIES_TABLE = "workspace_entities" +_DOCUMENTS_TABLE = "workspace_documents" + + +def upgrade() -> None: + connection = op.get_bind() + inspector = sa.inspect(connection) + + if not inspector.has_table(_ENTITIES_TABLE): + op.create_table( + _ENTITIES_TABLE, + sa.Column("workspace_id", sa.String(), nullable=False), + sa.Column("workspace_name", sa.String(), nullable=False), + sa.Column("workspace_domain", sa.String(), nullable=True), + sa.Column("created_at", sa.DateTime(timezone=True), nullable=False), + sa.PrimaryKeyConstraint("workspace_id"), + ) + + if not inspector.has_table(_DOCUMENTS_TABLE): + op.create_table( + _DOCUMENTS_TABLE, + sa.Column("document_id", sa.String(), nullable=False), + sa.Column("workspace_id", sa.String(), nullable=False), + sa.Column("organization_id", sa.String(), nullable=True), + sa.Column("document_name", sa.String(), nullable=False), + sa.Column("document_type", sa.String(), nullable=False), + sa.Column("document_content", sa.Text(), nullable=True), + sa.Column("document_status", sa.String(), nullable=False), + sa.Column("created_at", sa.DateTime(timezone=True), nullable=False), + sa.ForeignKeyConstraint( + ["workspace_id"], [f"{_ENTITIES_TABLE}.workspace_id"] + ), + sa.PrimaryKeyConstraint("document_id"), + ) + + for index_name, column_names in ( + ("ix_workspace_documents_workspace_id", ["workspace_id"]), + ("ix_workspace_documents_organization_id", ["organization_id"]), + ): + op.create_index( + index_name, + _DOCUMENTS_TABLE, + column_names, + if_not_exists=True, + ) + + +def downgrade() -> None: + connection = op.get_bind() + inspector = sa.inspect(connection) + + if inspector.has_table(_DOCUMENTS_TABLE): + for index_name in ( + "ix_workspace_documents_organization_id", + "ix_workspace_documents_workspace_id", + ): + op.drop_index(index_name, table_name=_DOCUMENTS_TABLE, if_exists=True) + op.drop_table(_DOCUMENTS_TABLE) + + if inspector.has_table(_ENTITIES_TABLE): + op.drop_table(_ENTITIES_TABLE) diff --git a/backend/api/data.py b/backend/api/data.py index dccd85890..639d4194a 100644 --- a/backend/api/data.py +++ b/backend/api/data.py @@ -36,6 +36,7 @@ ) from services.ontology_service import ontology_service from services.webdav_service import webdav_service +from services.workspace_scope import get_or_create_workspace router = APIRouter(prefix="/api/data", tags=["data"]) @@ -3166,6 +3167,7 @@ async def upload_data_document( auth_context: AuthContext = Depends(get_auth_context), db: AsyncSession = Depends(get_db), ) -> DataDocumentActionResponse: + await get_or_create_workspace(db, auth_context.workspace_id) document = Document( workspace_id=auth_context.workspace_id, organization_id=auth_context.organization_id, @@ -3301,6 +3303,7 @@ async def upload_document_for_pdf_dom_recognition( status_code=415, detail="Only application/pdf uploads are supported for DOM recognition.", ) + await get_or_create_workspace(db, auth_context.workspace_id) document = Document( workspace_id=auth_context.workspace_id, organization_id=auth_context.organization_id, diff --git a/backend/scripts/bootstrap_db.py b/backend/scripts/bootstrap_db.py index 1047103e8..3e579a053 100644 --- a/backend/scripts/bootstrap_db.py +++ b/backend/scripts/bootstrap_db.py @@ -186,10 +186,6 @@ def _get_create_indexes_statements() -> list[Executable]: "CREATE INDEX IF NOT EXISTS ix_email_records_owner_date " "ON email_records (user_id, organization_id, date)" ), - text( - "CREATE INDEX IF NOT EXISTS ix_emails_owner_date " - "ON emails (user_id, organization_id, date)" - ), text( "CREATE INDEX IF NOT EXISTS ix_sender_relationships_owner_source " "ON sender_relationships " diff --git a/backend/services/workspace_scope.py b/backend/services/workspace_scope.py new file mode 100644 index 000000000..1c188233e --- /dev/null +++ b/backend/services/workspace_scope.py @@ -0,0 +1,27 @@ +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession + +from db.models import Workspace + + +async def get_or_create_workspace( + session: AsyncSession, + workspace_id: str, +) -> Workspace: + """Return the ``Workspace`` row for ``workspace_id``, creating it first if needed. + + ``workspace_id`` is the signed session's ``workspace`` claim + (``workspace-`` or ``workspace-`` for + personal scope, per ``api/auth.py``/``_derive_workspace_id``), not the + model's own opaque default. Rows created here always use that claim as + the primary key so ``Document.workspace_id``'s foreign key resolves. + """ + result = await session.execute( + select(Workspace).where(Workspace.workspace_id == workspace_id) + ) + workspace = result.scalar_one_or_none() + if workspace is None: + workspace = Workspace(workspace_id=workspace_id, workspace_name=workspace_id) + session.add(workspace) + await session.flush() + return workspace diff --git a/backend/tests/test_data_api.py b/backend/tests/test_data_api.py index cd0b7bf37..547182662 100644 --- a/backend/tests/test_data_api.py +++ b/backend/tests/test_data_api.py @@ -28,6 +28,7 @@ Email, ProjectFolder, WebdavAccount, + Workspace, ) from db.session import get_db from main import app @@ -59,6 +60,7 @@ class MockAsyncSession: def __init__(self, results): self.results = results self.documents: list[Document] = [] + self.workspaces: list[Workspace] = [] self.queries = [] self.execute_calls = 0 @@ -82,6 +84,26 @@ async def execute(self, query): for account in result ] ) + if "from workspace_entities" in rendered_query_lower: + compiled = query.compile() + params = compiled.params + workspace_id = next( + ( + value + for key, value in params.items() + if key.startswith("workspace_id") + ), + None, + ) + workspace = next( + ( + workspace + for workspace in self.workspaces + if workspace.workspace_id == workspace_id + ), + None, + ) + return MockResult(workspace) if "from workspace_documents" in rendered_query_lower: compiled = query.compile() params = compiled.params @@ -121,10 +143,17 @@ def add(self, obj): if not obj.created_at: obj.created_at = _now() self.documents.append(obj) + elif isinstance(obj, Workspace): + if not obj.created_at: + obj.created_at = _now() + self.workspaces.append(obj) async def commit(self): pass + async def flush(self): + pass + async def refresh(self, obj): pass diff --git a/backend/tests/test_workspace_document_migration.py b/backend/tests/test_workspace_document_migration.py new file mode 100644 index 000000000..33a91feb7 --- /dev/null +++ b/backend/tests/test_workspace_document_migration.py @@ -0,0 +1,218 @@ +"""Regression coverage for the missing ``workspace_entities``/``workspace_documents`` +Alembic migration. + +``Workspace``/``Document`` have been declared in ``db/models.py`` since before +this repository's incremental migration history tracked them explicitly (see +``alembic/versions/0018_workspace_registry.py``). No production code path ever +inserted a ``Workspace`` row for a real signed session either, so +``Document.workspace_id``'s foreign key could never be satisfied by a real +``/api/data/documents`` upload. + +These tests exercise the actual documented production path +(``scripts/migrate_db.py`` -> ``alembic upgrade head``, never +``Base.metadata.create_all``) against a real, disposable PostgreSQL database, +then call the real ``/api/data/documents`` endpoints through the real FastAPI +app with only the database session swapped for one bound to that database. +""" + +import os +import subprocess +import sys +import uuid +from pathlib import Path + +import asyncpg +import httpx +import pytest +import pytest_asyncio +from asyncpg.exceptions import ( + InvalidAuthorizationSpecificationError, + InvalidPasswordError, +) +from sqlalchemy import text +from sqlalchemy.engine import make_url +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine + +from api.auth import AuthContext, get_auth_context +from core.config import settings +from db.session import get_db, get_readonly_db +from main import app + +pytestmark = pytest.mark.postgres + +BACKEND_ROOT = Path(__file__).resolve().parents[1] +_PRE_REGISTRY_REVISION = "0017_merge_newsdom_carddav_heads" +_SMOKE_WORKSPACE_ID = "workspace-workspace-migration-smoke-org" + + +def _run_migrations(database_url: str, revision: str = "head") -> None: + env = {**os.environ, "DATABASE_URL": database_url} + result = subprocess.run( + [sys.executable, str(BACKEND_ROOT / "scripts" / "migrate_db.py"), revision], + cwd=BACKEND_ROOT, + env=env, + capture_output=True, + text=True, + timeout=180, + ) + assert result.returncode == 0, ( + f"scripts/migrate_db.py {revision} failed " + f"(exit {result.returncode}):\n" + f"stdout:\n{result.stdout}\nstderr:\n{result.stderr}" + ) + + +@pytest_asyncio.fixture +async def fresh_database_url(): + base_url = make_url(settings.DATABASE_URL) + test_db_name = f"test_workspace_doc_{uuid.uuid4().hex[:16]}" + + async def _admin(sql: str) -> None: + conn = await asyncpg.connect( + host=base_url.host, + port=base_url.port, + user=base_url.username, + password=base_url.password, + database="postgres", + ) + try: + await conn.execute(sql) + finally: + await conn.close() + + try: + await _admin(f'CREATE DATABASE "{test_db_name}"') + except ( + InvalidAuthorizationSpecificationError, + InvalidPasswordError, + OSError, + ConnectionError, + ) as exc: + pytest.skip(f"PostgreSQL smoke database unavailable: {exc}") + + try: + yield base_url.set(database=test_db_name).render_as_string( + hide_password=False + ) + finally: + await _admin(f'DROP DATABASE IF EXISTS "{test_db_name}" WITH (FORCE)') + + +@pytest_asyncio.fixture +async def migrated_client(fresh_database_url): + engine = create_async_engine(fresh_database_url) + sessionmaker = async_sessionmaker(engine, expire_on_commit=False) + + async def override_db(): + async with sessionmaker() as session: + yield session + + async def override_auth_context() -> AuthContext: + return AuthContext( + user_id="workspace_migration_smoke_user", + role="member", + organization_id="workspace-migration-smoke-org", + group_ids=(), + workspace_id=_SMOKE_WORKSPACE_ID, + ) + + app.dependency_overrides[get_db] = override_db + app.dependency_overrides[get_readonly_db] = override_db + app.dependency_overrides[get_auth_context] = override_auth_context + try: + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), base_url="http://testserver" + ) as client: + yield client + finally: + app.dependency_overrides.pop(get_db, None) + app.dependency_overrides.pop(get_readonly_db, None) + app.dependency_overrides.pop(get_auth_context, None) + await engine.dispose() + + +async def _table_exists(database_url: str, name: str) -> bool: + engine = create_async_engine(database_url) + try: + async with engine.connect() as conn: + result = await conn.execute( + text("SELECT to_regclass(:name) IS NOT NULL"), {"name": name} + ) + return bool(result.scalar()) + finally: + await engine.dispose() + + +async def _drop_workspace_registry_tables(database_url: str) -> None: + engine = create_async_engine(database_url) + try: + async with engine.begin() as conn: + await conn.execute(text("DROP TABLE IF EXISTS workspace_documents")) + await conn.execute(text("DROP TABLE IF EXISTS workspace_entities")) + finally: + await engine.dispose() + + +async def _assert_document_upload_serves_cleanly(client: httpx.AsyncClient) -> None: + response = await client.post( + "/api/data/documents", + json={ + "document_name": "roadmap.md", + "document_type": "text/markdown", + "document_content": "# Roadmap", + }, + ) + assert response.status_code == 200, response.text + body = response.json() + assert body["workspace_id"] == _SMOKE_WORKSPACE_ID + assert body["document_id"] + + # A second upload from the same signed workspace must not fail trying to + # re-insert the already-provisioned Workspace row (workspace_id is its + # primary key). + second_response = await client.post( + "/api/data/documents", + json={ + "document_name": "notes.md", + "document_type": "text/markdown", + "document_content": "# Notes", + }, + ) + assert second_response.status_code == 200, second_response.text + + +@pytest.mark.asyncio +async def test_document_upload_serves_after_full_head_migration_from_empty( + fresh_database_url, migrated_client +): + """A brand-new database migrated straight to head must be able to serve + /api/data/documents without a missing-relation or FK-violation error.""" + _run_migrations(fresh_database_url) + await _assert_document_upload_serves_cleanly(migrated_client) + + +@pytest.mark.asyncio +async def test_document_upload_serves_after_upgrading_a_pre_registry_database( + fresh_database_url, migrated_client +): + """Reproduce the exact reported gap: a real, already-incrementally-migrated + production database that was provisioned before ``Workspace``/``Document`` + existed in ``db/models.py`` never gets ``workspace_entities``/ + ``workspace_documents`` created by any migration prior to + ``0018_workspace_registry`` (``0001_initial_control_plane``'s + ``Base.metadata.create_all`` only reflects *today's* model metadata, so it + cannot recreate that historical, pre-model-addition state on its own). + Force that end state directly -- dropping the tables a stopped-at-0017 + database would never have had -- then prove upgrading to head both + recreates them and lets /api/data/documents serve cleanly.""" + _run_migrations(fresh_database_url, revision=_PRE_REGISTRY_REVISION) + + await _drop_workspace_registry_tables(fresh_database_url) + assert await _table_exists(fresh_database_url, "workspace_entities") is False + assert await _table_exists(fresh_database_url, "workspace_documents") is False + + _run_migrations(fresh_database_url) + + assert await _table_exists(fresh_database_url, "workspace_entities") is True + assert await _table_exists(fresh_database_url, "workspace_documents") is True + await _assert_document_upload_serves_cleanly(migrated_client) From 31b400c3537f71fe3534a926fcb05c7a9e3fb5c3 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 11:50:57 +0900 Subject: [PATCH 02/27] fix(data): serialize workspace provisioning --- backend/services/workspace_scope.py | 23 ++++++++++++++--- backend/tests/test_data_api.py | 25 +++++++++++++++++++ .../test_workspace_document_migration.py | 25 +++++++++++++++++++ 3 files changed, 69 insertions(+), 4 deletions(-) diff --git a/backend/services/workspace_scope.py b/backend/services/workspace_scope.py index 1c188233e..5fe076a97 100644 --- a/backend/services/workspace_scope.py +++ b/backend/services/workspace_scope.py @@ -1,4 +1,7 @@ +import datetime + from sqlalchemy import select +from sqlalchemy.dialects.postgresql import insert from sqlalchemy.ext.asyncio import AsyncSession from db.models import Workspace @@ -16,12 +19,24 @@ async def get_or_create_workspace( model's own opaque default. Rows created here always use that claim as the primary key so ``Document.workspace_id``'s foreign key resolves. """ + # The first two requests for a signed workspace may arrive concurrently. + # A SELECT-then-INSERT races on the workspace_id primary key, so let + # PostgreSQL serialize creation and return the row when this transaction + # won the insert. result = await session.execute( - select(Workspace).where(Workspace.workspace_id == workspace_id) + insert(Workspace) + .values( + workspace_id=workspace_id, + workspace_name=workspace_id, + created_at=datetime.datetime.now(datetime.timezone.utc), + ) + .on_conflict_do_nothing(index_elements=[Workspace.workspace_id]) + .returning(Workspace) ) workspace = result.scalar_one_or_none() if workspace is None: - workspace = Workspace(workspace_id=workspace_id, workspace_name=workspace_id) - session.add(workspace) - await session.flush() + result = await session.execute( + select(Workspace).where(Workspace.workspace_id == workspace_id) + ) + workspace = result.scalar_one() return workspace diff --git a/backend/tests/test_data_api.py b/backend/tests/test_data_api.py index 547182662..efdf309a9 100644 --- a/backend/tests/test_data_api.py +++ b/backend/tests/test_data_api.py @@ -68,6 +68,31 @@ async def execute(self, query): self.queries.append(query) rendered_query = str(query) rendered_query_lower = rendered_query.lower() + if "insert into workspace_entities" in rendered_query_lower: + compiled = query.compile() + params = compiled.params + workspace_id = next( + value + for key, value in params.items() + if key.startswith("workspace_id") + ) + workspace = next( + ( + workspace + for workspace in self.workspaces + if workspace.workspace_id == workspace_id + ), + None, + ) + if workspace is None: + workspace = Workspace( + workspace_id=workspace_id, + workspace_name=workspace_id, + created_at=_now(), + ) + self.workspaces.append(workspace) + return MockResult(workspace) + return MockResult(None) if ( "webdav_accounts.source_uid" in rendered_query_lower and "webdav_accounts.account_id" not in rendered_query_lower diff --git a/backend/tests/test_workspace_document_migration.py b/backend/tests/test_workspace_document_migration.py index 33a91feb7..5ad91bdef 100644 --- a/backend/tests/test_workspace_document_migration.py +++ b/backend/tests/test_workspace_document_migration.py @@ -15,6 +15,7 @@ app with only the database session swapped for one bound to that database. """ +import asyncio import os import subprocess import sys @@ -191,6 +192,30 @@ async def test_document_upload_serves_after_full_head_migration_from_empty( await _assert_document_upload_serves_cleanly(migrated_client) +@pytest.mark.asyncio +async def test_concurrent_first_uploads_provision_one_workspace( + fresh_database_url, migrated_client +): + """Concurrent first requests must not race on the workspace primary key.""" + _run_migrations(fresh_database_url) + + responses = await asyncio.gather( + *( + migrated_client.post( + "/api/data/documents", + json={ + "document_name": f"concurrent-{index}.md", + "document_type": "text/markdown", + "document_content": "# Concurrent", + }, + ) + for index in range(16) + ) + ) + + assert [response.status_code for response in responses] == [200] * 16 + + @pytest.mark.asyncio async def test_document_upload_serves_after_upgrading_a_pre_registry_database( fresh_database_url, migrated_client From 37bcd6e38dd26c4aaf9af5837d8f920328bcca16 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 11:53:02 +0900 Subject: [PATCH 03/27] fix(data): bind documents to organization scope --- backend/api/data.py | 6 +++++- backend/tests/test_data_api.py | 31 ++++++++++++++++++++++++++++++- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/backend/api/data.py b/backend/api/data.py index 639d4194a..1da0913db 100644 --- a/backend/api/data.py +++ b/backend/api/data.py @@ -2494,6 +2494,7 @@ async def _get_workspace_document( select(Document).where( Document.document_id == document_id, Document.workspace_id == auth_context.workspace_id, + Document.organization_id == auth_context.organization_id, ) ) document = result.scalar_one_or_none() @@ -3934,7 +3935,10 @@ async def get_data_quality_surface( documents = await _scoped_rows( db, select(Document) - .where(Document.workspace_id == auth_context.workspace_id) + .where( + Document.workspace_id == auth_context.workspace_id, + Document.organization_id == auth_context.organization_id, + ) .order_by(Document.created_at.desc(), Document.document_id.asc()) .limit(8), ) diff --git a/backend/tests/test_data_api.py b/backend/tests/test_data_api.py index efdf309a9..a437638a3 100644 --- a/backend/tests/test_data_api.py +++ b/backend/tests/test_data_api.py @@ -148,11 +148,25 @@ async def execute(self, query): ), None, ) + organization_id = next( + ( + value + for key, value in params.items() + if key.startswith("organization_id") + ), + None, + ) rows = [ document for document in self.documents if (document_id is None or document.document_id == document_id) and (workspace_id is None or document.workspace_id == workspace_id) + and ( + organization_id is None + # Older fixtures predate Document.organization_id; keep + # them usable while enforcing any explicit organization. + or document.organization_id in (None, organization_id) + ) ] if "order by" in rendered_query_lower: return MockResult(rows) @@ -2655,7 +2669,17 @@ def test_data_document_actions_are_workspace_scoped_and_intent_only(mock_db): document_status="uploaded", created_at=_now(), ) - mock_db.documents.extend([document, rival_document]) + other_organization_document = Document( + document_id="doc_other_org", + workspace_id="workspace-org-acme", + organization_id="org-rival", + document_name="other-org.md", + document_type="text/markdown", + document_content="other organization", + document_status="uploaded", + created_at=_now(), + ) + mock_db.documents.extend([document, rival_document, other_organization_document]) token = _signed_session_token(_valid_session_payload()) client, previous_secret, original_overrides = _with_signed_auth(mock_db, token) try: @@ -2667,6 +2691,9 @@ def test_data_document_actions_are_workspace_scoped_and_intent_only(mock_db): "/api/data/documents/doc_owned/hwp-conversion-intent" ) rival_response = client.post("/api/data/documents/doc_rival/reparse") + other_organization_response = client.post( + "/api/data/documents/doc_other_org/reparse" + ) finally: client.close() _restore_overrides(previous_secret, original_overrides) @@ -2692,6 +2719,8 @@ def test_data_document_actions_are_workspace_scoped_and_intent_only(mock_db): assert rival_response.status_code == 404 assert "doc_rival" not in rival_response.text + assert other_organization_response.status_code == 404 + assert "doc_other_org" not in other_organization_response.text def test_data_document_webdav_materialization_executes_source_backed_write( From bfe347e7b7283ca5d10e9056fbf4c718cfdfc5da Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 02:57:37 +0000 Subject: [PATCH 04/27] test: cover the workspace/organization collision in the quality surface CodeRabbit flagged that test_data_quality_surface_includes_workspace_document_assets didn't actually exercise the new Document.organization_id predicate added in 37bcd6e3: doc_owned had no organization_id set, and MockAsyncSession treats a None organization_id as matching any query filter (intentionally, to keep older organization_id-less fixtures usable), so the test passed regardless of whether the real query filtered by organization at all. Give doc_owned its real organization_id and add doc_other_org: same workspace_id, different organization_id. Verified this reproduces the gap (reverting the organization_id predicate in get_data_quality_surface turns this red with object_count 2 instead of 1) and passes with the fix restored. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016ErwZSYW3pm585NiM3Q7aN --- backend/tests/test_data_api.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/backend/tests/test_data_api.py b/backend/tests/test_data_api.py index a437638a3..e4b6ca300 100644 --- a/backend/tests/test_data_api.py +++ b/backend/tests/test_data_api.py @@ -2552,6 +2552,7 @@ def test_data_quality_surface_includes_workspace_document_assets(mock_db): Document( document_id="doc_owned", workspace_id="workspace-org-acme", + organization_id="org-acme", document_name="roadmap.md", document_type="text/markdown", document_content="# Roadmap", @@ -2567,6 +2568,16 @@ def test_data_quality_surface_includes_workspace_document_assets(mock_db): document_status="uploaded", created_at=_now(), ), + Document( + document_id="doc_other_org", + workspace_id="workspace-org-acme", + organization_id="org-rival", + document_name="other-org.md", + document_type="text/markdown", + document_content="other organization", + document_status="uploaded", + created_at=_now(), + ), ] ) token = _signed_session_token(_valid_session_payload()) @@ -2612,6 +2623,7 @@ def test_data_quality_surface_includes_workspace_document_assets(mock_db): } ] assert "doc_rival" not in response.text + assert "doc_other_org" not in response.text def test_data_document_upload_creates_workspace_scoped_document(mock_db): From 317c721e39dec43303e05d4abe8df64676683bc5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 04:26:00 +0000 Subject: [PATCH 05/27] fix: guard 0016 against a genuinely missing workspace_documents table Devin Review correctly identified two real gaps in 0018_workspace_registry: 1. A database that truly never had workspace_entities/workspace_documents (one whose own 0001_initial_control_plane ran before these models existed, and has only applied incremental migrations since) crashes on 0016_document_org_scope with NoSuchTableError, because 0016 calls inspector.get_columns() unconditionally and it sits before 0018 in the chain. My existing regression test only dropped the tables after 0017, which never exercised this because 0001 always recreates them via live create_all for a genuinely fresh test database -- masking the real bug. Reproduced directly (migrate to 0015, drop the tables, continue to head) and confirmed the crash; 0016 is now has_table-guarded like the rest of this repo's idempotent migrations, and the regression test's pre-registry boundary moved from 0017 to 0015 so it actually crosses 0016 with the tables absent. 2. 0018's downgrade unconditionally dropped both tables, including when its own upgrade was a no-op because they already existed -- so a rollback on any database would destroy workspace_documents.document_content (real uploaded content, not rebuildable derived state like most other tables this repo's migrations manage). Made downgrade a documented no-op, matching the same judgment call 0001_initial_control_plane already makes for the same reason. Verified: the reproduction above now completes cleanly end-to-end through /api/data/documents; full backend suite still 1837 passed (same 2 pre-existing unrelated is_read failures), ruff clean. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016ErwZSYW3pm585NiM3Q7aN --- .../versions/0016_document_org_scope.py | 11 ++++++++++ .../versions/0018_workspace_registry.py | 22 ++++++++----------- .../test_workspace_document_migration.py | 20 ++++++++++++----- 3 files changed, 35 insertions(+), 18 deletions(-) diff --git a/backend/alembic/versions/0016_document_org_scope.py b/backend/alembic/versions/0016_document_org_scope.py index 0a5cd0035..9d7165639 100644 --- a/backend/alembic/versions/0016_document_org_scope.py +++ b/backend/alembic/versions/0016_document_org_scope.py @@ -8,6 +8,13 @@ recognition worker can resolve the owning organization's provider without joining through the (organization-less) workspace entity. Nullable and additive so existing rows and personal-scope documents are unaffected. + +A database that has never had ``workspace_documents`` at all (one that ran +``0001_initial_control_plane`` before ``Workspace``/``Document`` existed in +``db/models.py``, and has only applied incremental migrations since) has no +table for this revision to alter. This revision is a no-op for that case; +``0018_workspace_registry`` creates the table later in the chain, already +including this column. """ from alembic import op @@ -24,6 +31,8 @@ def upgrade() -> None: connection = op.get_bind() inspector = sa.inspect(connection) + if not inspector.has_table(_DOCUMENTS_TABLE): + return columns = {column["name"] for column in inspector.get_columns(_DOCUMENTS_TABLE)} if _ORG_COLUMN not in columns: op.add_column( @@ -41,6 +50,8 @@ def upgrade() -> None: def downgrade() -> None: connection = op.get_bind() inspector = sa.inspect(connection) + if not inspector.has_table(_DOCUMENTS_TABLE): + return columns = {column["name"] for column in inspector.get_columns(_DOCUMENTS_TABLE)} op.drop_index(_ORG_INDEX, table_name=_DOCUMENTS_TABLE, if_exists=True) if _ORG_COLUMN in columns: diff --git a/backend/alembic/versions/0018_workspace_registry.py b/backend/alembic/versions/0018_workspace_registry.py index cbd23a057..aaef5f446 100644 --- a/backend/alembic/versions/0018_workspace_registry.py +++ b/backend/alembic/versions/0018_workspace_registry.py @@ -70,16 +70,12 @@ def upgrade() -> None: def downgrade() -> None: - connection = op.get_bind() - inspector = sa.inspect(connection) - - if inspector.has_table(_DOCUMENTS_TABLE): - for index_name in ( - "ix_workspace_documents_organization_id", - "ix_workspace_documents_workspace_id", - ): - op.drop_index(index_name, table_name=_DOCUMENTS_TABLE, if_exists=True) - op.drop_table(_DOCUMENTS_TABLE) - - if inspector.has_table(_ENTITIES_TABLE): - op.drop_table(_ENTITIES_TABLE) + # This revision's upgrade is a no-op whenever the tables already exist + # (e.g. created by 0001's create_all), so a downgrade cannot tell "this + # revision created these tables" apart from "they predate it" -- and + # workspace_documents.document_content holds real uploaded content, not + # rebuildable derived state. Unconditionally dropping it risks destroying + # data this revision never created. As with 0001_initial_control_plane: + # production rollbacks should restore from backup or a later explicit + # down revision rather than dropping customer-owned data. + return None diff --git a/backend/tests/test_workspace_document_migration.py b/backend/tests/test_workspace_document_migration.py index 5ad91bdef..8059a0205 100644 --- a/backend/tests/test_workspace_document_migration.py +++ b/backend/tests/test_workspace_document_migration.py @@ -6,7 +6,9 @@ ``alembic/versions/0018_workspace_registry.py``). No production code path ever inserted a ``Workspace`` row for a real signed session either, so ``Document.workspace_id``'s foreign key could never be satisfied by a real -``/api/data/documents`` upload. +``/api/data/documents`` upload. A database missing these tables also used to +crash on ``0016_document_org_scope`` (``NoSuchTableError``) before ever +reaching ``0018_workspace_registry``; ``0016`` is now ``has_table``-guarded. These tests exercise the actual documented production path (``scripts/migrate_db.py`` -> ``alembic upgrade head``, never @@ -42,7 +44,12 @@ pytestmark = pytest.mark.postgres BACKEND_ROOT = Path(__file__).resolve().parents[1] -_PRE_REGISTRY_REVISION = "0017_merge_newsdom_carddav_heads" +# The revision immediately before 0016_document_org_scope, which -- for a +# database missing workspace_documents -- is the first migration in the +# chain that touches the table at all. Stopping here (rather than at 0017, +# after 0016 has already run) is what actually exercises the real historical +# gap: 0016 must not crash before 0018_workspace_registry ever gets to run. +_PRE_REGISTRY_REVISION = "0015_merge_newsdom_email_heads" _SMOKE_WORKSPACE_ID = "workspace-workspace-migration-smoke-org" @@ -227,9 +234,12 @@ async def test_document_upload_serves_after_upgrading_a_pre_registry_database( ``0018_workspace_registry`` (``0001_initial_control_plane``'s ``Base.metadata.create_all`` only reflects *today's* model metadata, so it cannot recreate that historical, pre-model-addition state on its own). - Force that end state directly -- dropping the tables a stopped-at-0017 - database would never have had -- then prove upgrading to head both - recreates them and lets /api/data/documents serve cleanly.""" + Force that end state directly -- dropping the tables a stopped-at-0015 + database would never have had -- then migrate straight to head in one + call, crossing 0016_document_org_scope (which used to crash with + NoSuchTableError on a database in exactly this state) before + 0018_workspace_registry ever runs. Prove that both tables end up correct + and /api/data/documents serves cleanly.""" _run_migrations(fresh_database_url, revision=_PRE_REGISTRY_REVISION) await _drop_workspace_registry_tables(fresh_database_url) From e05f1b34d985a5d79c594b8b170f1c1f548c93ae Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 13:41:46 +0900 Subject: [PATCH 06/27] test(data): reproduce legacy document organization scope regression --- .../test_legacy_document_scope_postgres.py | 175 ++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 backend/tests/test_legacy_document_scope_postgres.py diff --git a/backend/tests/test_legacy_document_scope_postgres.py b/backend/tests/test_legacy_document_scope_postgres.py new file mode 100644 index 000000000..011170d99 --- /dev/null +++ b/backend/tests/test_legacy_document_scope_postgres.py @@ -0,0 +1,175 @@ +"""PostgreSQL regression for legacy workspace documents with no organization id. + +Revision 0016 intentionally left pre-existing ``workspace_documents.organization_id`` +values NULL. Organization-scoped sessions still need to reach those rows when the +signed workspace claim is the canonical ``workspace-`` value, +without making the same NULL row visible to a different organization that presents +the same workspace string. +""" + +import os +import subprocess +import sys +import uuid +from pathlib import Path + +import asyncpg +import httpx +import pytest +from asyncpg.exceptions import InvalidAuthorizationSpecificationError, InvalidPasswordError +from sqlalchemy import text +from sqlalchemy.engine import make_url +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine + +from api.auth import AuthContext, get_auth_context +from core.config import settings +from db.session import get_db, get_readonly_db +from main import app + +pytestmark = pytest.mark.postgres + +_BACKEND_ROOT = Path(__file__).resolve().parents[1] +_ORGANIZATION_ID = "legacy-document-org" +_WORKSPACE_ID = f"workspace-{_ORGANIZATION_ID}" +_DOCUMENT_ID = "document_legacy_org_scope" + + +def _run_migrations(database_url: str) -> None: + result = subprocess.run( + [sys.executable, str(_BACKEND_ROOT / "scripts" / "migrate_db.py"), "head"], + cwd=_BACKEND_ROOT, + env={**os.environ, "DATABASE_URL": database_url}, + capture_output=True, + text=True, + timeout=180, + ) + assert result.returncode == 0, result.stderr + + +@pytest.mark.asyncio +async def test_legacy_null_org_document_is_visible_only_to_matching_signed_org() -> None: + base_url = make_url(settings.DATABASE_URL) + database_name = f"test_legacy_doc_scope_{uuid.uuid4().hex[:12]}" + + async def admin(sql: str) -> None: + connection = await asyncpg.connect( + host=base_url.host, + port=base_url.port, + user=base_url.username, + password=base_url.password, + database="postgres", + ) + try: + await connection.execute(sql) + finally: + await connection.close() + + try: + await admin(f'CREATE DATABASE "{database_name}"') + except ( + InvalidAuthorizationSpecificationError, + InvalidPasswordError, + OSError, + ConnectionError, + ) as exc: + pytest.skip(f"PostgreSQL smoke database unavailable: {exc}") + + database_url = base_url.set(database=database_name).render_as_string( + hide_password=False + ) + engine = create_async_engine(database_url) + session_factory = async_sessionmaker(engine, expire_on_commit=False) + + async def override_db(): + async with session_factory() as session: + yield session + + async def matching_auth() -> AuthContext: + return AuthContext( + user_id="legacy-document-user", + role="member", + organization_id=_ORGANIZATION_ID, + group_ids=(), + workspace_id=_WORKSPACE_ID, + ) + + async def other_org_same_workspace_auth() -> AuthContext: + return AuthContext( + user_id="other-user", + role="member", + organization_id="other-organization", + group_ids=(), + workspace_id=_WORKSPACE_ID, + ) + + try: + _run_migrations(database_url) + async with engine.begin() as connection: + await connection.execute( + text( + """ + INSERT INTO workspace_entities + (workspace_id, workspace_name, workspace_domain, created_at) + VALUES (:workspace_id, :workspace_name, NULL, now()) + ON CONFLICT (workspace_id) DO NOTHING + """ + ), + {"workspace_id": _WORKSPACE_ID, "workspace_name": _WORKSPACE_ID}, + ) + await connection.execute( + text( + """ + INSERT INTO workspace_documents + (document_id, workspace_id, organization_id, document_name, + document_type, document_content, document_status, created_at) + VALUES + (:document_id, :workspace_id, NULL, :document_name, + 'text/markdown', '# Legacy', 'uploaded', now()) + """ + ), + { + "document_id": _DOCUMENT_ID, + "workspace_id": _WORKSPACE_ID, + "document_name": "legacy.md", + }, + ) + + app.dependency_overrides[get_db] = override_db + app.dependency_overrides[get_readonly_db] = override_db + app.dependency_overrides[get_auth_context] = matching_auth + + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), base_url="http://testserver" + ) as client: + reparse_response = await client.post( + f"/api/data/documents/{_DOCUMENT_ID}/reparse" + ) + assert reparse_response.status_code == 200, reparse_response.text + + quality_response = await client.get("/api/data/quality-surface") + assert quality_response.status_code == 200, quality_response.text + assert _DOCUMENT_ID in { + asset["asset_key"] for asset in quality_response.json()["repository_assets"] + } + + app.dependency_overrides[get_auth_context] = other_org_same_workspace_auth + denied_response = await client.post( + f"/api/data/documents/{_DOCUMENT_ID}/reparse" + ) + assert denied_response.status_code == 404 + + other_quality_response = await client.get("/api/data/quality-surface") + assert other_quality_response.status_code == 200, other_quality_response.text + assert _DOCUMENT_ID not in { + asset["asset_key"] + for asset in other_quality_response.json()["repository_assets"] + } + finally: + app.dependency_overrides.pop(get_db, None) + app.dependency_overrides.pop(get_readonly_db, None) + app.dependency_overrides.pop(get_auth_context, None) + await engine.dispose() + try: + await admin(f'DROP DATABASE IF EXISTS "{database_name}" WITH (FORCE)') + except (OSError, ConnectionError): + pass From 5054a8e73b70ef542bcee65f5a232f320b152771 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 04:46:05 +0000 Subject: [PATCH 07/27] fix: trust legacy NULL-organization documents only for the owning org test_legacy_document_scope_postgres.py (e05f1b34) reproduced Devin Review's Finding 4 against real PostgreSQL: 0016_document_org_scope left existing workspace_documents.organization_id unbackfilled (NULL), and the strict Document.organization_id == auth_context.organization_id predicate added in 37bcd6e3 made such a row invisible even to the organization that actually owns its workspace. Add _document_organization_filter: an exact organization_id match, OR a NULL organization_id, but only when the requesting session's own workspace_id/organization_id pairing is the canonical workspace- (or workspace-) derivation -- not an internally inconsistent claim. This is what keeps the cross-tenant boundary 37bcd6e3 added intact: a session whose workspace_id doesn't match what its own organization_id would derive gets the strict, no-NULL-fallback check, so a forged or malformed claim pairing still can't read a same-workspace document under a different organization. Wired into both _get_workspace_document and get_data_quality_surface's Document query. Verified: test_legacy_document_scope_postgres.py now passes (was failing on this branch's previous commit). Full backend suite: 1838 passed (same 2 pre-existing unrelated is_read failures noted earlier in this PR), ruff clean. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016ErwZSYW3pm585NiM3Q7aN --- backend/api/data.py | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/backend/api/data.py b/backend/api/data.py index 1da0913db..db4681083 100644 --- a/backend/api/data.py +++ b/backend/api/data.py @@ -2485,6 +2485,36 @@ async def _count_scalar(db: AsyncSession, statement) -> int: return int(result.scalar_one() or 0) +def _auth_context_owns_its_workspace(auth_context: AuthContext) -> bool: + """Whether ``auth_context.workspace_id`` is the canonical + ``workspace-``/``workspace-`` derivation for + this session, rather than an internally inconsistent claim pairing.""" + expected_workspace_id = ( + f"workspace-{auth_context.organization_id}" + if auth_context.organization_id + else f"workspace-{auth_context.user_id}" + ) + return auth_context.workspace_id == expected_workspace_id + + +def _document_organization_filter(auth_context: AuthContext) -> ColumnElement: + """Scope a ``Document`` query to this session's organization. + + Revision 0016_document_org_scope added ``organization_id`` without + backfilling existing rows, so a legacy document can have it unset. Such a + row is trusted to belong to whichever single organization the document's + (already-filtered) ``workspace_id`` canonically maps to -- but only when + this session's own workspace/organization pairing is that canonical + derivation, not an internally inconsistent claim. + """ + organization_filter = Document.organization_id == auth_context.organization_id + if _auth_context_owns_its_workspace(auth_context): + organization_filter = or_( + organization_filter, Document.organization_id.is_(None) + ) + return organization_filter + + async def _get_workspace_document( db: AsyncSession, auth_context: AuthContext, @@ -2494,7 +2524,7 @@ async def _get_workspace_document( select(Document).where( Document.document_id == document_id, Document.workspace_id == auth_context.workspace_id, - Document.organization_id == auth_context.organization_id, + _document_organization_filter(auth_context), ) ) document = result.scalar_one_or_none() @@ -3937,7 +3967,7 @@ async def get_data_quality_surface( select(Document) .where( Document.workspace_id == auth_context.workspace_id, - Document.organization_id == auth_context.organization_id, + _document_organization_filter(auth_context), ) .order_by(Document.created_at.desc(), Document.document_id.asc()) .limit(8), From 6fe1de1d5739cf3d482fe4e3268c4b2907d5a6ce Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 04:58:19 +0000 Subject: [PATCH 08/27] fix: reconcile emails/email_records fix with PR #1502 PR #1502 (opened in parallel, ~5 minutes before this one) independently diagnosed the same underlying gap from the same root cause -- running the real backend suite against actual PostgreSQL for the first time -- and found something this PR didn't: app-ci.yml's backend job has never had a Postgres services: container, so every @pytest.mark.postgres test (including all the new ones in this PR) has always silently skipped in real CI. It ships the authoritative fix for that, plus its own version of the 0011_email_read_state /bootstrap_db.py fix, with a pinned contract test. Landing two different versions of the same files from two open PRs would conflict. Adopt #1502's exact pattern for the overlapping files instead of this PR's earlier approach: - 0011_email_read_state.py: has_table("emails")-guarded no-op, keeping the original "emails" target, rather than retargeting to "email_records". (0011_email_model_reconciliation's own docstring clarifies no migration ever renamed "emails" to "email_records" for a real managed database -- email_records was the actual table name since inception; "emails" was only ever a stale copy-pasted string. Both approaches are safe in practice, so there's no reason to diverge from the already-tested pattern.) - bootstrap_db.py / 0001_initial_control_plane.py: schema_backfill_sql()'s callers now go through execute_schema_backfill(), which skips the legacy ix_emails_owner_date statement via identity-matching a LEGACY_EMAILS_INDEX sentinel rather than this PR's simpler unconditional deletion. - test_alembic_migrations.py: contract test now asserts execute_schema_backfill, plus #1502's own test_email_read_state_legacy_table_guard_is_reversible pinning the reconciled 0011 file's shape. - test_bootstrap_db.py / test_data_api.py: the 4 raw SQL `INSERT INTO email_records` smoke-seeding call sites now set is_read explicitly (Python-side ORM default only, no DB server default, so real Postgres rejects the omission) -- the exact bug flagged as a follow-up earlier in this PR's own investigation. Re-verified end-to-end: fresh-database migration to head, and the true historical-database reproduction (migrate to 0015, drop the workspace registry tables, continue to head crossing both 0011_email_read_state and 0016_document_org_scope) both still complete cleanly. Full backend suite: 1841 passed, 0 failed, 3 skipped (up from 1838/2 failed -- the last 2 pre-existing failures are now fixed too), ruff clean. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016ErwZSYW3pm585NiM3Q7aN --- .../versions/0001_initial_control_plane.py | 5 +-- .../alembic/versions/0011_email_read_state.py | 44 ++++++++----------- backend/scripts/bootstrap_db.py | 23 +++++++++- backend/tests/test_alembic_migrations.py | 12 ++++- backend/tests/test_bootstrap_db.py | 9 ++-- backend/tests/test_data_api.py | 12 ++--- 6 files changed, 63 insertions(+), 42 deletions(-) diff --git a/backend/alembic/versions/0001_initial_control_plane.py b/backend/alembic/versions/0001_initial_control_plane.py index cc14ce39b..31b187f55 100644 --- a/backend/alembic/versions/0001_initial_control_plane.py +++ b/backend/alembic/versions/0001_initial_control_plane.py @@ -9,7 +9,7 @@ from sqlalchemy import text from db.models import Base -from scripts.bootstrap_db import schema_backfill_sql +from scripts.bootstrap_db import execute_schema_backfill revision = "0001_initial_control_plane" down_revision = None @@ -19,8 +19,7 @@ def upgrade() -> None: connection = op.get_bind() connection.execute(text("CREATE EXTENSION IF NOT EXISTS vector")) Base.metadata.create_all(connection) - for statement in schema_backfill_sql(): - connection.execute(statement) + execute_schema_backfill(connection) def downgrade() -> None: diff --git a/backend/alembic/versions/0011_email_read_state.py b/backend/alembic/versions/0011_email_read_state.py index d1677399e..46344deba 100644 --- a/backend/alembic/versions/0011_email_read_state.py +++ b/backend/alembic/versions/0011_email_read_state.py @@ -1,4 +1,4 @@ -"""Add is_read to email_records (IMAP \\Seen read state). +"""Add is_read to emails (IMAP \\Seen read state). Existing rows default to read so historical/file imports do not surface as unread. """ @@ -12,32 +12,26 @@ branch_labels = None depends_on = None -_EMAIL_TABLE = "email_records" - def upgrade() -> None: - connection = op.get_bind() - inspector = sa.inspect(connection) - if not _has_column(inspector, _EMAIL_TABLE, "is_read"): - op.add_column( - _EMAIL_TABLE, - sa.Column( - "is_read", - sa.Boolean(), - nullable=False, - server_default=sa.text("true"), - ), - ) + # A fresh install's 0001 migration creates only the current ORM tables + # (email_records, which already carries is_read) via + # Base.metadata.create_all(); the legacy "emails" table this migration + # targets exists only on databases provisioned before that rename. + if not sa.inspect(op.get_bind()).has_table("emails"): + return + op.add_column( + "emails", + sa.Column( + "is_read", + sa.Boolean(), + nullable=False, + server_default=sa.text("true"), + ), + ) def downgrade() -> None: - connection = op.get_bind() - inspector = sa.inspect(connection) - if _has_column(inspector, _EMAIL_TABLE, "is_read"): - op.drop_column(_EMAIL_TABLE, "is_read") - - -def _has_column(inspector, table_name: str, column_name: str) -> bool: - return any( - column["name"] == column_name for column in inspector.get_columns(table_name) - ) + if not sa.inspect(op.get_bind()).has_table("emails"): + return + op.drop_column("emails", "is_read") diff --git a/backend/scripts/bootstrap_db.py b/backend/scripts/bootstrap_db.py index 3e579a053..ebce33779 100644 --- a/backend/scripts/bootstrap_db.py +++ b/backend/scripts/bootstrap_db.py @@ -2,13 +2,20 @@ import os from collections.abc import Sequence -from sqlalchemy import Executable, text +from sqlalchemy import Executable, inspect, text from sqlalchemy.engine import Connection from db.models import Base from db.session import engine INVALID_EMAIL_BACKFILL_OWNER_IDS = {None, "", "default"} +# Only present on databases that predate the email_records rename; a fresh +# install has no "emails" table for CREATE INDEX to target. Identity-matched +# in execute_schema_backfill() below so it can be skipped instead of failing. +LEGACY_EMAILS_INDEX = text( + "CREATE INDEX IF NOT EXISTS ix_emails_owner_date " + "ON emails (user_id, organization_id, date)" +) def _static_bootstrap_sql(statement: str) -> Executable: @@ -186,6 +193,7 @@ def _get_create_indexes_statements() -> list[Executable]: "CREATE INDEX IF NOT EXISTS ix_email_records_owner_date " "ON email_records (user_id, organization_id, date)" ), + LEGACY_EMAILS_INDEX, text( "CREATE INDEX IF NOT EXISTS ix_sender_relationships_owner_source " "ON sender_relationships " @@ -528,11 +536,22 @@ def _execute_statements(conn: Connection, statements: Sequence[Executable]) -> N conn.execute(statement) +def execute_schema_backfill( + conn: Connection, statements: Sequence[Executable] | None = None +) -> None: + statements = schema_backfill_sql() if statements is None else statements + legacy_emails_exists = inspect(conn).has_table("emails") + for statement in statements: + if statement is LEGACY_EMAILS_INDEX and not legacy_emails_exists: + continue + conn.execute(statement) + + async def bootstrap_db() -> None: async with engine.begin() as conn: await conn.execute(text("CREATE EXTENSION IF NOT EXISTS vector")) await conn.run_sync(Base.metadata.create_all) - await conn.run_sync(_execute_statements, schema_backfill_sql()) + await conn.run_sync(execute_schema_backfill) if __name__ == "__main__": diff --git a/backend/tests/test_alembic_migrations.py b/backend/tests/test_alembic_migrations.py index f8f3ffeae..f1e12450a 100644 --- a/backend/tests/test_alembic_migrations.py +++ b/backend/tests/test_alembic_migrations.py @@ -32,7 +32,17 @@ def test_initial_alembic_revision_records_current_schema_path(): assert "down_revision = None" in revision_text assert "CREATE EXTENSION IF NOT EXISTS vector" in revision_text assert "Base.metadata.create_all" in revision_text - assert "schema_backfill_sql" in revision_text + assert "execute_schema_backfill" in revision_text + + +def test_email_read_state_legacy_table_guard_is_reversible(): + revision_path = BACKEND_ROOT / "alembic" / "versions" / "0011_email_read_state.py" + revision_text = revision_path.read_text() + + assert revision_text.count('has_table("emails")') == 2 + assert revision_text.count("return") == 2 + assert 'op.add_column(\n "emails"' in revision_text + assert 'op.drop_column("emails", "is_read")' in revision_text def test_provider_writeback_retry_queue_has_incremental_revision(): diff --git a/backend/tests/test_bootstrap_db.py b/backend/tests/test_bootstrap_db.py index 5af0540f0..95a4e72d9 100644 --- a/backend/tests/test_bootstrap_db.py +++ b/backend/tests/test_bootstrap_db.py @@ -6,7 +6,7 @@ from core.config import settings from db.models import Base -from scripts.bootstrap_db import schema_backfill_sql +from scripts.bootstrap_db import execute_schema_backfill, schema_backfill_sql from db.models import ( AgentRunRecord, CalendarWritebackSource, @@ -30,8 +30,7 @@ def _get_schema_statements(monkeypatch): def _execute_schema_backfill(sync_conn): - for statement in schema_backfill_sql(): - sync_conn.execute(statement) + execute_schema_backfill(sync_conn) def test_schema_backfill_adds_email_columns(monkeypatch): @@ -770,11 +769,11 @@ async def test_connector_signal_events_real_postgres_bootstrap_smoke(): text(""" INSERT INTO email_records ( user_id, organization_id, message_id, sender, recipients, - subject, "date", body + subject, "date", body, is_read ) VALUES ( :user_id, :organization_id, :message_id, :sender, - :recipients, :subject, now(), :body + :recipients, :subject, now(), :body, true ) RETURNING id """), diff --git a/backend/tests/test_data_api.py b/backend/tests/test_data_api.py index e4b6ca300..39df79883 100644 --- a/backend/tests/test_data_api.py +++ b/backend/tests/test_data_api.py @@ -3046,11 +3046,11 @@ async def _seed_smoke_test_data(conn, ids: dict): """ INSERT INTO email_records ( user_id, organization_id, message_id, thread_id, - fingerprint, sender, recipients, subject, "date", body + fingerprint, sender, recipients, subject, "date", body, is_read ) VALUES ( :user_id, :organization_id, :message_id, :thread_id, - :fingerprint, :sender, :recipients, :subject, now(), :body + :fingerprint, :sender, :recipients, :subject, now(), :body, true ) RETURNING id """ @@ -3072,11 +3072,11 @@ async def _seed_smoke_test_data(conn, ids: dict): """ INSERT INTO email_records ( user_id, organization_id, message_id, sender, recipients, - subject, "date", body + subject, "date", body, is_read ) VALUES ( :user_id, :organization_id, :message_id, :sender, - :recipients, :subject, now(), :body + :recipients, :subject, now(), :body, true ) RETURNING id """ @@ -3096,11 +3096,11 @@ async def _seed_smoke_test_data(conn, ids: dict): """ INSERT INTO email_records ( user_id, organization_id, message_id, thread_id, - fingerprint, sender, recipients, subject, "date", body + fingerprint, sender, recipients, subject, "date", body, is_read ) VALUES ( :user_id, :organization_id, :message_id, :thread_id, - :fingerprint, :sender, :recipients, :subject, now(), :body + :fingerprint, :sender, :recipients, :subject, now(), :body, true ) RETURNING id """ From 053c066ce91e53b053a43b75feb4b8478e6bf2cc Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 05:12:17 +0000 Subject: [PATCH 09/27] fix: guard 0011_email_read_state on column, not just table, existence Devin Review found two real gaps in the has_table("emails")-only pattern adopted from PR #1502 in the previous commit: 1. A genuinely historical database -- one whose own 0001 ran before is_read was added to the Email model, so it has email_records without is_read -- silently never gets the column: has_table("emails") is False (per 0011_email_model_reconciliation's docstring, no managed database ever really had a table literally named "emails"), so upgrade() returned without touching email_records at all. Reproduced directly: migrated to 0009, dropped email_records.is_read to simulate that historical state, continued to head with the has_table("emails")-only version -- it completed with no error, but is_read was permanently missing. Confirmed the same reproduction now correctly adds is_read to email_records. 2. Not idempotent: a legacy "emails" table that already has is_read (e.g. from a partial/earlier application) made upgrade() crash with a duplicate-column error, since it only checked has_table before calling op.add_column. Reproduced directly (manually created an "emails" table with is_read already present, migrated to head) and confirmed it no longer crashes. Now checks both "email_records" (the table that actually matters) and "emails" (defensive, in case a real one somehow exists), guarded by column existence via the same _has_column helper this repo's other migrations already use, so upgrade/downgrade are safely idempotent either way. This diverges from PR #1502's exact pinned file shape (its test_email_read_state_legacy_table_guard_is_reversible asserted the has_table-only version byte-for-byte), so updated this PR's own contract test to check for the corrected shape instead of matching that exact text. Worth flagging on #1502 too, since the same gaps apply to its own version of this file if it hasn't already been fixed there. Full backend suite: 1841 passed, 0 failed, 3 skipped, ruff clean. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016ErwZSYW3pm585NiM3Q7aN --- .../alembic/versions/0011_email_read_state.py | 56 ++++++++++++------- backend/tests/test_alembic_migrations.py | 20 +++++-- 2 files changed, 52 insertions(+), 24 deletions(-) diff --git a/backend/alembic/versions/0011_email_read_state.py b/backend/alembic/versions/0011_email_read_state.py index 46344deba..b80104641 100644 --- a/backend/alembic/versions/0011_email_read_state.py +++ b/backend/alembic/versions/0011_email_read_state.py @@ -1,6 +1,14 @@ -"""Add is_read to emails (IMAP \\Seen read state). +"""Add is_read to email_records (IMAP \\Seen read state). Existing rows default to read so historical/file imports do not surface as unread. + +Checks both "email_records" (the real, current table -- a database whose own +0001_initial_control_plane ran before ``is_read`` was added to the ``Email`` +model has this table without the column, and needs it added) and "emails" +(a legacy name that, per 0011_email_model_reconciliation's docstring, no +migration in this repo's history ever actually created for a real managed +database, but is checked defensively in case one somehow exists). Guarded by +column existence, not just table existence, so it is safely idempotent. """ from alembic import op @@ -12,26 +20,36 @@ branch_labels = None depends_on = None +_CANDIDATE_TABLES = ("email_records", "emails") + def upgrade() -> None: - # A fresh install's 0001 migration creates only the current ORM tables - # (email_records, which already carries is_read) via - # Base.metadata.create_all(); the legacy "emails" table this migration - # targets exists only on databases provisioned before that rename. - if not sa.inspect(op.get_bind()).has_table("emails"): - return - op.add_column( - "emails", - sa.Column( - "is_read", - sa.Boolean(), - nullable=False, - server_default=sa.text("true"), - ), - ) + inspector = sa.inspect(op.get_bind()) + for table_name in _CANDIDATE_TABLES: + if inspector.has_table(table_name) and not _has_column( + inspector, table_name, "is_read" + ): + op.add_column( + table_name, + sa.Column( + "is_read", + sa.Boolean(), + nullable=False, + server_default=sa.text("true"), + ), + ) def downgrade() -> None: - if not sa.inspect(op.get_bind()).has_table("emails"): - return - op.drop_column("emails", "is_read") + inspector = sa.inspect(op.get_bind()) + for table_name in _CANDIDATE_TABLES: + if inspector.has_table(table_name) and _has_column( + inspector, table_name, "is_read" + ): + op.drop_column(table_name, "is_read") + + +def _has_column(inspector, table_name: str, column_name: str) -> bool: + return any( + column["name"] == column_name for column in inspector.get_columns(table_name) + ) diff --git a/backend/tests/test_alembic_migrations.py b/backend/tests/test_alembic_migrations.py index f1e12450a..255f25efc 100644 --- a/backend/tests/test_alembic_migrations.py +++ b/backend/tests/test_alembic_migrations.py @@ -35,14 +35,24 @@ def test_initial_alembic_revision_records_current_schema_path(): assert "execute_schema_backfill" in revision_text -def test_email_read_state_legacy_table_guard_is_reversible(): +def test_email_read_state_guards_both_legacy_and_current_table_names(): + """0011_email_read_state must add is_read to a genuinely historical + email_records table missing it (a database whose own 0001 ran before + is_read was added to the Email model), not just a legacy "emails" table + that, per 0011_email_model_reconciliation's docstring, no migration in + this repo's history ever actually created for a real managed database. + Both checks must guard on column existence, not just table existence, so + upgrade/downgrade stay idempotent against a table that already has (or + lacks) the column either way.""" revision_path = BACKEND_ROOT / "alembic" / "versions" / "0011_email_read_state.py" revision_text = revision_path.read_text() - assert revision_text.count('has_table("emails")') == 2 - assert revision_text.count("return") == 2 - assert 'op.add_column(\n "emails"' in revision_text - assert 'op.drop_column("emails", "is_read")' in revision_text + assert '"email_records"' in revision_text + assert '"emails"' in revision_text + assert "has_table" in revision_text + assert "_has_column" in revision_text + assert "op.add_column(" in revision_text + assert "op.drop_column(" in revision_text def test_provider_writeback_retry_queue_has_incremental_revision(): From d3020ca780f669dad69e3c5ca397178adf481f22 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 05:17:52 +0000 Subject: [PATCH 10/27] fix: make 0011_email_read_state's downgrade non-destructive Devin Review found the same ownership-ambiguity problem in this migration's downgrade that 0018_workspace_registry's downgrade already had (fixed earlier in this PR): a fresh database's email_records.is_read comes from 0001's live Base.metadata.create_all, not from 0011_email_read_state, so there is no way for downgrade() to tell "this revision added the column" apart from "the baseline already had it" -- and is_read holds real per-message read/unread state, not rebuildable derived data. Reproduced directly against real PostgreSQL: migrated a fresh database to head, then ran alembic downgrade to 0009 -- the previous op.drop_column version silently deleted email_records.is_read and its data. Made downgrade a documented no-op instead, matching the same judgment call already applied to 0001_initial_control_plane and 0018_workspace_registry. Also added backend/tests/test_email_read_state_migration_postgres.py: permanent real-Postgres coverage for all three scenarios this migration must handle (historical email_records missing is_read gets it added; idempotent against a legacy "emails" table that already has it; downgrade does not destroy a fresh database's read state), addressing Devin's separate note that the existing contract test's string-matching assertions can't detect a destructive downgrade or prove idempotence. Confirmed the downgrade test fails red against the reverted (destructive) version before restoring the fix. Full backend suite: 1844 passed, 0 failed, 3 skipped, ruff clean. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016ErwZSYW3pm585NiM3Q7aN --- .../alembic/versions/0011_email_read_state.py | 16 +- backend/tests/test_alembic_migrations.py | 13 +- ...est_email_read_state_migration_postgres.py | 181 ++++++++++++++++++ 3 files changed, 200 insertions(+), 10 deletions(-) create mode 100644 backend/tests/test_email_read_state_migration_postgres.py diff --git a/backend/alembic/versions/0011_email_read_state.py b/backend/alembic/versions/0011_email_read_state.py index b80104641..04fa068ce 100644 --- a/backend/alembic/versions/0011_email_read_state.py +++ b/backend/alembic/versions/0011_email_read_state.py @@ -41,12 +41,16 @@ def upgrade() -> None: def downgrade() -> None: - inspector = sa.inspect(op.get_bind()) - for table_name in _CANDIDATE_TABLES: - if inspector.has_table(table_name) and _has_column( - inspector, table_name, "is_read" - ): - op.drop_column(table_name, "is_read") + # Same ownership-ambiguity problem as 0018_workspace_registry's downgrade: + # a fresh database gets email_records.is_read from 0001's live + # Base.metadata.create_all, not from this revision, so there is no way to + # tell "this revision added the column" apart from "the baseline already + # had it" -- and is_read holds real per-message read/unread state, not + # rebuildable derived data. As with 0001_initial_control_plane and + # 0018_workspace_registry: production rollbacks should restore from + # backup or a later explicit down revision rather than dropping + # customer-owned data. + return None def _has_column(inspector, table_name: str, column_name: str) -> bool: diff --git a/backend/tests/test_alembic_migrations.py b/backend/tests/test_alembic_migrations.py index 255f25efc..0045cab10 100644 --- a/backend/tests/test_alembic_migrations.py +++ b/backend/tests/test_alembic_migrations.py @@ -41,9 +41,14 @@ def test_email_read_state_guards_both_legacy_and_current_table_names(): is_read was added to the Email model), not just a legacy "emails" table that, per 0011_email_model_reconciliation's docstring, no migration in this repo's history ever actually created for a real managed database. - Both checks must guard on column existence, not just table existence, so - upgrade/downgrade stay idempotent against a table that already has (or - lacks) the column either way.""" + The upgrade check must guard on column existence, not just table + existence, so it stays idempotent against a table that already has the + column. downgrade is a no-op: a fresh database's email_records.is_read + comes from 0001's live create_all, not from this revision, so there is + no way to tell "this revision added it" apart from "the baseline already + had it" -- and is_read holds real read/unread state, not rebuildable + derived data (same ownership-ambiguity reasoning as + 0018_workspace_registry's downgrade).""" revision_path = BACKEND_ROOT / "alembic" / "versions" / "0011_email_read_state.py" revision_text = revision_path.read_text() @@ -52,7 +57,7 @@ def test_email_read_state_guards_both_legacy_and_current_table_names(): assert "has_table" in revision_text assert "_has_column" in revision_text assert "op.add_column(" in revision_text - assert "op.drop_column(" in revision_text + assert "op.drop_column(" not in revision_text def test_provider_writeback_retry_queue_has_incremental_revision(): diff --git a/backend/tests/test_email_read_state_migration_postgres.py b/backend/tests/test_email_read_state_migration_postgres.py new file mode 100644 index 000000000..627d6a0dd --- /dev/null +++ b/backend/tests/test_email_read_state_migration_postgres.py @@ -0,0 +1,181 @@ +"""PostgreSQL regression coverage for 0011_email_read_state. + +String-matching the revision file's source (test_alembic_migrations.py) +cannot detect a destructive downgrade or prove the upgrade is actually +idempotent -- both require running the real migration against a real +database in each of the shapes it must handle. +""" + +import os +import subprocess +import sys +import uuid +from pathlib import Path + +import asyncpg +import pytest +from asyncpg.exceptions import InvalidAuthorizationSpecificationError, InvalidPasswordError +from sqlalchemy import text +from sqlalchemy.engine import make_url +from sqlalchemy.ext.asyncio import create_async_engine + +from core.config import settings + +pytestmark = pytest.mark.postgres + +_BACKEND_ROOT = Path(__file__).resolve().parents[1] +_PRE_READ_STATE_REVISION = "0009_project_graph_projection" + + +def _run_migrations(database_url: str, revision: str = "head") -> None: + result = subprocess.run( + [sys.executable, str(_BACKEND_ROOT / "scripts" / "migrate_db.py"), revision], + cwd=_BACKEND_ROOT, + env={**os.environ, "DATABASE_URL": database_url}, + capture_output=True, + text=True, + timeout=180, + ) + assert result.returncode == 0, ( + f"scripts/migrate_db.py {revision} failed " + f"(exit {result.returncode}):\nstdout:\n{result.stdout}\nstderr:\n{result.stderr}" + ) + + +def _run_downgrade(database_url: str, revision: str) -> None: + # scripts/migrate_db.py only wraps alembic's upgrade command; reuse its + # alembic_config() but call command.downgrade() directly for this test. + script = ( + "from scripts.migrate_db import alembic_config\n" + "from alembic import command\n" + f"command.downgrade(alembic_config(), {revision!r})\n" + ) + result = subprocess.run( + [sys.executable, "-c", script], + cwd=_BACKEND_ROOT, + env={**os.environ, "DATABASE_URL": database_url}, + capture_output=True, + text=True, + timeout=180, + ) + assert result.returncode == 0, ( + f"alembic downgrade {revision} failed " + f"(exit {result.returncode}):\nstdout:\n{result.stdout}\nstderr:\n{result.stderr}" + ) + + +@pytest.fixture +def fresh_database_url(): + base_url = make_url(settings.DATABASE_URL) + database_name = f"test_email_read_state_{uuid.uuid4().hex[:12]}" + + async def admin(sql: str) -> None: + connection = await asyncpg.connect( + host=base_url.host, + port=base_url.port, + user=base_url.username, + password=base_url.password, + database="postgres", + ) + try: + await connection.execute(sql) + finally: + await connection.close() + + import asyncio + + try: + asyncio.run(admin(f'CREATE DATABASE "{database_name}"')) + except ( + InvalidAuthorizationSpecificationError, + InvalidPasswordError, + OSError, + ConnectionError, + ) as exc: + pytest.skip(f"PostgreSQL smoke database unavailable: {exc}") + + try: + yield base_url.set(database=database_name).render_as_string(hide_password=False) + finally: + asyncio.run(admin(f'DROP DATABASE IF EXISTS "{database_name}" WITH (FORCE)')) + + +async def _column_exists(database_url: str, table_name: str, column_name: str) -> bool: + engine = create_async_engine(database_url) + try: + async with engine.connect() as connection: + result = await connection.execute( + text( + "SELECT 1 FROM information_schema.columns " + "WHERE table_name = :table_name AND column_name = :column_name" + ), + {"table_name": table_name, "column_name": column_name}, + ) + return result.first() is not None + finally: + await engine.dispose() + + +@pytest.mark.asyncio +async def test_upgrade_adds_is_read_to_a_historical_email_records_table( + fresh_database_url, +): + """A database whose own 0001 ran before is_read existed in the Email + model has email_records without the column; upgrading to head must add + it, not silently leave it missing.""" + _run_migrations(fresh_database_url, revision=_PRE_READ_STATE_REVISION) + + engine = create_async_engine(fresh_database_url) + try: + async with engine.begin() as connection: + await connection.execute( + text("ALTER TABLE email_records DROP COLUMN IF EXISTS is_read") + ) + finally: + await engine.dispose() + + assert await _column_exists(fresh_database_url, "email_records", "is_read") is False + + _run_migrations(fresh_database_url) + + assert await _column_exists(fresh_database_url, "email_records", "is_read") is True + + +@pytest.mark.asyncio +async def test_upgrade_is_idempotent_against_a_legacy_emails_table_with_is_read( + fresh_database_url, +): + """A legacy "emails" table that already has is_read (e.g. from a partial + earlier application) must not make upgrade() crash with a + duplicate-column error.""" + _run_migrations(fresh_database_url, revision=_PRE_READ_STATE_REVISION) + + engine = create_async_engine(fresh_database_url) + try: + async with engine.begin() as connection: + await connection.execute( + text( + "CREATE TABLE emails " + "(id serial primary key, is_read boolean not null default true)" + ) + ) + finally: + await engine.dispose() + + _run_migrations(fresh_database_url) + + +@pytest.mark.asyncio +async def test_downgrade_does_not_destroy_read_state_on_a_fresh_database( + fresh_database_url, +): + """A fresh database's email_records.is_read comes from 0001's live + create_all, not from 0011_email_read_state -- downgrading past 0011 must + not drop it (and, if it did, would destroy real per-message read/unread + state along with it).""" + _run_migrations(fresh_database_url) + assert await _column_exists(fresh_database_url, "email_records", "is_read") is True + + _run_downgrade(fresh_database_url, _PRE_READ_STATE_REVISION) + + assert await _column_exists(fresh_database_url, "email_records", "is_read") is True From da2d7fe168a954141d6d6d080fa148bdb5da4dcb Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 05:27:17 +0000 Subject: [PATCH 11/27] fix: add forward migration repairing already-stamped is_read gap Devin Review's most structurally important finding yet: editing 0011_email_read_state.py cannot repair a database whose alembic_version history already records "0011_email_read_state" as applied -- Alembic never re-runs an already-stamped revision's upgrade(), regardless of how the file content changes afterward. Every fix to that file earlier in this PR only helps a database that hasn't reached 0011 yet. Reproduced directly: migrated a fresh database to head, dropped email_records.is_read to simulate a database that's already past 0011 (via any path -- an earlier broken version of that revision, a partial apply, manual intervention) but never actually got the column, then re-ran `alembic upgrade head` -- nothing happened, since the database was already at head with no revision left to apply. is_read stayed permanently missing. Added 0019_email_read_state_repair.py: a new forward migration appended after the current head (0018_workspace_registry), matching the same pattern already used for the workspace registry gap itself -- idempotently (has_table/has_column guarded) ensures email_records.is_read exists, regardless of what 0011 already did or didn't do for a given database. Downgrade is a no-op for the same ownership-ambiguity reasons as 0011_email_read_state's and 0018_workspace_registry's. Confirmed the same reproduction now correctly repairs the column. Added a permanent regression test for exactly this scenario (stamp through 0018, drop is_read, upgrade to head) and confirmed it fails red without 0019. Full backend suite: 1845 passed, 0 failed, 3 skipped, ruff clean. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016ErwZSYW3pm585NiM3Q7aN --- .../versions/0019_email_read_state_repair.py | 57 +++++++++++++++++++ ...est_email_read_state_migration_postgres.py | 32 +++++++++++ 2 files changed, 89 insertions(+) create mode 100644 backend/alembic/versions/0019_email_read_state_repair.py diff --git a/backend/alembic/versions/0019_email_read_state_repair.py b/backend/alembic/versions/0019_email_read_state_repair.py new file mode 100644 index 000000000..814fd0c25 --- /dev/null +++ b/backend/alembic/versions/0019_email_read_state_repair.py @@ -0,0 +1,57 @@ +"""idempotently ensure email_records has is_read + +Revision ID: 0019_email_read_state_repair +Revises: 0018_workspace_registry +Create Date: 2026-09-01 00:00:00.000000 + +Alembic never re-runs a revision's ``upgrade()`` once that revision id is +recorded as applied for a database -- editing ``0011_email_read_state.py``'s +content cannot repair a database that already has "0011_email_read_state" +in its ``alembic_version`` history but never actually got +``email_records.is_read`` (whatever the reason: an earlier version of that +revision that targeted the wrong table, a partial/interrupted apply, manual +intervention). This revision is the real repair path for such a database: +appended after the current head, so it runs regardless of what 0011 already +did or didn't do. Idempotent (has_table/has_column guarded) so it is a +no-op for every database that already has the column, from any path. +""" + +from alembic import op +import sqlalchemy as sa + +revision = "0019_email_read_state_repair" +down_revision = "0018_workspace_registry" + +_EMAIL_TABLE = "email_records" + + +def upgrade() -> None: + inspector = sa.inspect(op.get_bind()) + if inspector.has_table(_EMAIL_TABLE) and not _has_column( + inspector, _EMAIL_TABLE, "is_read" + ): + op.add_column( + _EMAIL_TABLE, + sa.Column( + "is_read", + sa.Boolean(), + nullable=False, + server_default=sa.text("true"), + ), + ) + + +def downgrade() -> None: + # Same ownership-ambiguity reasoning as 0011_email_read_state and + # 0018_workspace_registry: this revision cannot tell whether it was the + # one that added the column (repairing a stamped-but-incomplete + # database) or the column already existed from another path, and + # is_read holds real read/unread state, not rebuildable derived data. + # No-op; production rollbacks should restore from backup. + return None + + +def _has_column(inspector, table_name: str, column_name: str) -> bool: + return any( + column["name"] == column_name for column in inspector.get_columns(table_name) + ) diff --git a/backend/tests/test_email_read_state_migration_postgres.py b/backend/tests/test_email_read_state_migration_postgres.py index 627d6a0dd..fb1f0415c 100644 --- a/backend/tests/test_email_read_state_migration_postgres.py +++ b/backend/tests/test_email_read_state_migration_postgres.py @@ -179,3 +179,35 @@ async def test_downgrade_does_not_destroy_read_state_on_a_fresh_database( _run_downgrade(fresh_database_url, _PRE_READ_STATE_REVISION) assert await _column_exists(fresh_database_url, "email_records", "is_read") is True + + +@pytest.mark.asyncio +async def test_upgrade_head_repairs_a_database_already_stamped_past_0011( + fresh_database_url, +): + """Alembic never re-runs a revision's upgrade() once that revision id is + recorded as applied -- editing 0011_email_read_state.py cannot repair a + database whose alembic_version history already includes it but is + missing is_read regardless (e.g. an earlier broken version of that + revision, a partial apply, manual intervention). 0019_email_read_state_ + repair is the real fix: it must add the column even though the database + is already stamped through 0018 with 0011 long since applied, so only + 0019 itself -- not a re-run of 0011 -- is what's left to bring it to + head.""" + _run_migrations(fresh_database_url, revision="0018_workspace_registry") + assert await _column_exists(fresh_database_url, "email_records", "is_read") is True + + engine = create_async_engine(fresh_database_url) + try: + async with engine.begin() as connection: + await connection.execute( + text("ALTER TABLE email_records DROP COLUMN IF EXISTS is_read") + ) + finally: + await engine.dispose() + + assert await _column_exists(fresh_database_url, "email_records", "is_read") is False + + _run_migrations(fresh_database_url) + + assert await _column_exists(fresh_database_url, "email_records", "is_read") is True From 2beaf0e92bccb149fcd144386ae2f9071870dec7 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 14:39:06 +0900 Subject: [PATCH 12/27] fix: preserve document scope migration data --- .../versions/0016_document_org_scope.py | 12 ++++------- backend/tests/test_alembic_migrations.py | 8 ++++++++ ...est_email_read_state_migration_postgres.py | 20 ++++++++++++++++--- .../test_legacy_document_scope_postgres.py | 13 +++++++++--- .../test_workspace_document_migration.py | 12 ++++++++--- 5 files changed, 48 insertions(+), 17 deletions(-) diff --git a/backend/alembic/versions/0016_document_org_scope.py b/backend/alembic/versions/0016_document_org_scope.py index 9d7165639..0c823754e 100644 --- a/backend/alembic/versions/0016_document_org_scope.py +++ b/backend/alembic/versions/0016_document_org_scope.py @@ -48,11 +48,7 @@ def upgrade() -> None: def downgrade() -> None: - connection = op.get_bind() - inspector = sa.inspect(connection) - if not inspector.has_table(_DOCUMENTS_TABLE): - return - columns = {column["name"] for column in inspector.get_columns(_DOCUMENTS_TABLE)} - op.drop_index(_ORG_INDEX, table_name=_DOCUMENTS_TABLE, if_exists=True) - if _ORG_COLUMN in columns: - op.drop_column(_DOCUMENTS_TABLE, _ORG_COLUMN) + # 0018 can create this table and column after 0016 was a no-op. Alembic + # cannot distinguish that case from a table altered by this revision, so + # dropping the column here could destroy later organization assignments. + return None diff --git a/backend/tests/test_alembic_migrations.py b/backend/tests/test_alembic_migrations.py index 0045cab10..19b4384b7 100644 --- a/backend/tests/test_alembic_migrations.py +++ b/backend/tests/test_alembic_migrations.py @@ -60,6 +60,14 @@ def test_email_read_state_guards_both_legacy_and_current_table_names(): assert "op.drop_column(" not in revision_text +def test_document_org_scope_downgrade_preserves_later_assignments(): + revision_path = ( + BACKEND_ROOT / "alembic" / "versions" / "0016_document_org_scope.py" + ) + + assert "op.drop_column(" not in revision_path.read_text() + + def test_provider_writeback_retry_queue_has_incremental_revision(): versions_dir = BACKEND_ROOT / "alembic" / "versions" revision_path = versions_dir / "0002_provider_writeback_retry_queue.py" diff --git a/backend/tests/test_email_read_state_migration_postgres.py b/backend/tests/test_email_read_state_migration_postgres.py index fb1f0415c..d6294e69c 100644 --- a/backend/tests/test_email_read_state_migration_postgres.py +++ b/backend/tests/test_email_read_state_migration_postgres.py @@ -6,8 +6,8 @@ database in each of the shapes it must handle. """ -import os import subprocess +import secrets import sys import uuid from pathlib import Path @@ -31,7 +31,10 @@ def _run_migrations(database_url: str, revision: str = "head") -> None: result = subprocess.run( [sys.executable, str(_BACKEND_ROOT / "scripts" / "migrate_db.py"), revision], cwd=_BACKEND_ROOT, - env={**os.environ, "DATABASE_URL": database_url}, + env={ + "DATABASE_URL": database_url, + "AUTH_SESSION_HMAC_SECRET": secrets.token_urlsafe(48), + }, capture_output=True, text=True, timeout=180, @@ -40,6 +43,10 @@ def _run_migrations(database_url: str, revision: str = "head") -> None: f"scripts/migrate_db.py {revision} failed " f"(exit {result.returncode}):\nstdout:\n{result.stdout}\nstderr:\n{result.stderr}" ) + assert not any( + status in result.stdout + result.stderr + for status in ("Timeout", "Fatal", "Warn", "Denied") + ) def _run_downgrade(database_url: str, revision: str) -> None: @@ -53,7 +60,10 @@ def _run_downgrade(database_url: str, revision: str) -> None: result = subprocess.run( [sys.executable, "-c", script], cwd=_BACKEND_ROOT, - env={**os.environ, "DATABASE_URL": database_url}, + env={ + "DATABASE_URL": database_url, + "AUTH_SESSION_HMAC_SECRET": secrets.token_urlsafe(48), + }, capture_output=True, text=True, timeout=180, @@ -62,6 +72,10 @@ def _run_downgrade(database_url: str, revision: str) -> None: f"alembic downgrade {revision} failed " f"(exit {result.returncode}):\nstdout:\n{result.stdout}\nstderr:\n{result.stderr}" ) + assert not any( + status in result.stdout + result.stderr + for status in ("Timeout", "Fatal", "Warn", "Denied") + ) @pytest.fixture diff --git a/backend/tests/test_legacy_document_scope_postgres.py b/backend/tests/test_legacy_document_scope_postgres.py index 011170d99..810dc3562 100644 --- a/backend/tests/test_legacy_document_scope_postgres.py +++ b/backend/tests/test_legacy_document_scope_postgres.py @@ -7,8 +7,8 @@ the same workspace string. """ -import os import subprocess +import secrets import sys import uuid from pathlib import Path @@ -38,12 +38,19 @@ def _run_migrations(database_url: str) -> None: result = subprocess.run( [sys.executable, str(_BACKEND_ROOT / "scripts" / "migrate_db.py"), "head"], cwd=_BACKEND_ROOT, - env={**os.environ, "DATABASE_URL": database_url}, + env={ + "DATABASE_URL": database_url, + "AUTH_SESSION_HMAC_SECRET": secrets.token_urlsafe(48), + }, capture_output=True, text=True, timeout=180, ) - assert result.returncode == 0, result.stderr + output = result.stdout + result.stderr + assert result.returncode == 0, output + assert not any( + status in output for status in ("Timeout", "Fatal", "Warn", "Denied") + ), output @pytest.mark.asyncio diff --git a/backend/tests/test_workspace_document_migration.py b/backend/tests/test_workspace_document_migration.py index 8059a0205..f685675b7 100644 --- a/backend/tests/test_workspace_document_migration.py +++ b/backend/tests/test_workspace_document_migration.py @@ -18,8 +18,8 @@ """ import asyncio -import os import subprocess +import secrets import sys import uuid from pathlib import Path @@ -54,11 +54,13 @@ def _run_migrations(database_url: str, revision: str = "head") -> None: - env = {**os.environ, "DATABASE_URL": database_url} result = subprocess.run( [sys.executable, str(BACKEND_ROOT / "scripts" / "migrate_db.py"), revision], cwd=BACKEND_ROOT, - env=env, + env={ + "DATABASE_URL": database_url, + "AUTH_SESSION_HMAC_SECRET": secrets.token_urlsafe(48), + }, capture_output=True, text=True, timeout=180, @@ -68,6 +70,10 @@ def _run_migrations(database_url: str, revision: str = "head") -> None: f"(exit {result.returncode}):\n" f"stdout:\n{result.stdout}\nstderr:\n{result.stderr}" ) + assert not any( + status in result.stdout + result.stderr + for status in ("Timeout", "Fatal", "Warn", "Denied") + ) @pytest_asyncio.fixture From 9c4e5ed9c5a770ef7a62906f7bd10c508ab1d8ca Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 14:41:13 +0900 Subject: [PATCH 13/27] fix: create legacy index through SQLAlchemy --- backend/scripts/bootstrap_db.py | 37 ++++++++++----------------------- 1 file changed, 11 insertions(+), 26 deletions(-) diff --git a/backend/scripts/bootstrap_db.py b/backend/scripts/bootstrap_db.py index ebce33779..3a1211181 100644 --- a/backend/scripts/bootstrap_db.py +++ b/backend/scripts/bootstrap_db.py @@ -1,23 +1,12 @@ import asyncio import os -from collections.abc import Sequence - -from sqlalchemy import Executable, inspect, text +from sqlalchemy import Executable, Index, MetaData, Table, inspect, text from sqlalchemy.engine import Connection from db.models import Base from db.session import engine INVALID_EMAIL_BACKFILL_OWNER_IDS = {None, "", "default"} -# Only present on databases that predate the email_records rename; a fresh -# install has no "emails" table for CREATE INDEX to target. Identity-matched -# in execute_schema_backfill() below so it can be skipped instead of failing. -LEGACY_EMAILS_INDEX = text( - "CREATE INDEX IF NOT EXISTS ix_emails_owner_date " - "ON emails (user_id, organization_id, date)" -) - - def _static_bootstrap_sql(statement: str) -> Executable: # ponytail: repo-authored static bootstrap SQL only; bind params before runtime input. return text(statement) @@ -193,7 +182,6 @@ def _get_create_indexes_statements() -> list[Executable]: "CREATE INDEX IF NOT EXISTS ix_email_records_owner_date " "ON email_records (user_id, organization_id, date)" ), - LEGACY_EMAILS_INDEX, text( "CREATE INDEX IF NOT EXISTS ix_sender_relationships_owner_source " "ON sender_relationships " @@ -531,20 +519,17 @@ def schema_backfill_sql() -> list[Executable]: return statements -def _execute_statements(conn: Connection, statements: Sequence[Executable]) -> None: - for statement in statements: - conn.execute(statement) - - -def execute_schema_backfill( - conn: Connection, statements: Sequence[Executable] | None = None -) -> None: - statements = schema_backfill_sql() if statements is None else statements - legacy_emails_exists = inspect(conn).has_table("emails") - for statement in statements: - if statement is LEGACY_EMAILS_INDEX and not legacy_emails_exists: - continue +def execute_schema_backfill(conn: Connection) -> None: + for statement in schema_backfill_sql(): conn.execute(statement) + if inspect(conn).has_table("emails"): + emails = Table("emails", MetaData(), autoload_with=conn) + Index( + "ix_emails_owner_date", + emails.c.user_id, + emails.c.organization_id, + emails.c.date, + ).create(conn, checkfirst=True) async def bootstrap_db() -> None: From 9c1851336fa04bcdc77c1c6e531afdb882583af1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 08:53:38 +0000 Subject: [PATCH 14/27] fix: explain intentional best-effort DB teardown swallow github-code-quality flagged the bare except/pass in the disposable test database's cleanup as unexplained; it's deliberate (a transient connectivity error tearing down the scratch database must not mask the test's actual assertions), so document it inline. --- backend/tests/test_legacy_document_scope_postgres.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/backend/tests/test_legacy_document_scope_postgres.py b/backend/tests/test_legacy_document_scope_postgres.py index 810dc3562..952036d9d 100644 --- a/backend/tests/test_legacy_document_scope_postgres.py +++ b/backend/tests/test_legacy_document_scope_postgres.py @@ -179,4 +179,6 @@ async def other_org_same_workspace_auth() -> AuthContext: try: await admin(f'DROP DATABASE IF EXISTS "{database_name}" WITH (FORCE)') except (OSError, ConnectionError): + # Best-effort teardown: a transient connectivity error here must not + # mask the test's actual assertions. pass From 201656a5b004e427fb2f95580990e9047a8bd7e9 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 14 Sep 2026 17:54:46 +0900 Subject: [PATCH 15/27] docs(workspace): keep provisioning contract opaque --- backend/services/workspace_scope.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/backend/services/workspace_scope.py b/backend/services/workspace_scope.py index 5fe076a97..182342163 100644 --- a/backend/services/workspace_scope.py +++ b/backend/services/workspace_scope.py @@ -13,11 +13,12 @@ async def get_or_create_workspace( ) -> Workspace: """Return the ``Workspace`` row for ``workspace_id``, creating it first if needed. - ``workspace_id`` is the signed session's ``workspace`` claim - (``workspace-`` or ``workspace-`` for - personal scope, per ``api/auth.py``/``_derive_workspace_id``), not the - model's own opaque default. Rows created here always use that claim as - the primary key so ``Document.workspace_id``'s foreign key resolves. + ``workspace_id`` is the independently signed, opaque workspace membership + claim supplied by the authenticated boundary. This service does not derive + workspace identity from ``organization_id`` or ``user_id``. Rows created + here use the supplied claim as the primary key so + ``Document.workspace_id``'s foreign key resolves to the authenticated + workspace. """ # The first two requests for a signed workspace may arrive concurrently. # A SELECT-then-INSERT races on the workspace_id primary key, so let From 2ec2134761789d9b084718e26440ff87fbe1ffd6 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 14 Sep 2026 19:44:00 +0900 Subject: [PATCH 16/27] test(data): reproduce opaque workspace legacy document scope --- ...ent_organization_scope_opaque_workspace.py | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 backend/tests/test_document_organization_scope_opaque_workspace.py diff --git a/backend/tests/test_document_organization_scope_opaque_workspace.py b/backend/tests/test_document_organization_scope_opaque_workspace.py new file mode 100644 index 000000000..ef15d104b --- /dev/null +++ b/backend/tests/test_document_organization_scope_opaque_workspace.py @@ -0,0 +1,48 @@ +"""Regression contracts for opaque workspace document authorization.""" + +from api.auth import AuthContext +import api.data as data_api +from db.models import Document +from sqlalchemy import or_, select + + +def _opaque_workspace_auth() -> AuthContext: + """Build a valid signed-session shape whose workspace is not derived from org id.""" + + return AuthContext( + user_id="member-a", + role="member", + organization_id="org-acme", + group_ids=(), + workspace_id="tenant-space-7f3c", + ) + + +def test_opaque_workspace_allows_historical_null_org_inside_exact_workspace() -> None: + """Legacy NULL organization rows remain readable inside the signed workspace.""" + + auth_context = _opaque_workspace_auth() + statement = select(Document.document_id).where( + Document.workspace_id == auth_context.workspace_id, + data_api._document_organization_filter(auth_context), + ) + compiled = statement.compile() + rendered = str(statement.whereclause) + + assert "workspace_documents.workspace_id" in rendered + assert "workspace_documents.organization_id" in rendered + assert "IS NULL" in rendered.upper() + assert auth_context.workspace_id in compiled.params.values() + assert auth_context.organization_id in compiled.params.values() + + +def test_document_organization_filter_rejects_other_non_null_organizations() -> None: + """The compatibility branch is exactly current organization OR historical NULL.""" + + auth_context = _opaque_workspace_auth() + expected = or_( + Document.organization_id == auth_context.organization_id, + Document.organization_id.is_(None), + ) + + assert data_api._document_organization_filter(auth_context).compare(expected) From eb0bfd5be077b40670795c45c3f7f34726f10065 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 14 Sep 2026 19:51:07 +0900 Subject: [PATCH 17/27] test(data): require trusted workspace organization binding --- ...ent_organization_scope_opaque_workspace.py | 54 ++++++++++++------- 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/backend/tests/test_document_organization_scope_opaque_workspace.py b/backend/tests/test_document_organization_scope_opaque_workspace.py index ef15d104b..cb71b8e5e 100644 --- a/backend/tests/test_document_organization_scope_opaque_workspace.py +++ b/backend/tests/test_document_organization_scope_opaque_workspace.py @@ -3,46 +3,64 @@ from api.auth import AuthContext import api.data as data_api from db.models import Document -from sqlalchemy import or_, select +from sqlalchemy import select -def _opaque_workspace_auth() -> AuthContext: - """Build a valid signed-session shape whose workspace is not derived from org id.""" +def _opaque_workspace_auth(*, organization_id: str = "org-acme") -> AuthContext: + """Build a signed-session shape whose opaque workspace is not org-derived.""" return AuthContext( user_id="member-a", role="member", - organization_id="org-acme", + organization_id=organization_id, group_ids=(), workspace_id="tenant-space-7f3c", ) -def test_opaque_workspace_allows_historical_null_org_inside_exact_workspace() -> None: - """Legacy NULL organization rows remain readable inside the signed workspace.""" +def _compiled_document_scope(auth_context: AuthContext) -> tuple[str, dict[str, object]]: + """Compile the document scope so tenant-binding predicates stay observable.""" - auth_context = _opaque_workspace_auth() statement = select(Document.document_id).where( Document.workspace_id == auth_context.workspace_id, data_api._document_organization_filter(auth_context), ) compiled = statement.compile() - rendered = str(statement.whereclause) + return str(statement.whereclause), compiled.params + + +def test_opaque_workspace_legacy_null_requires_trusted_organization_binding() -> None: + """Legacy NULL rows require a server-side workspace-to-org binding.""" + + auth_context = _opaque_workspace_auth() + rendered, params = _compiled_document_scope(auth_context) assert "workspace_documents.workspace_id" in rendered assert "workspace_documents.organization_id" in rendered assert "IS NULL" in rendered.upper() - assert auth_context.workspace_id in compiled.params.values() - assert auth_context.organization_id in compiled.params.values() + assert "workspace_entities" in rendered + assert "workspace_entities.workspace_id" in rendered + assert "workspace_entities.organization_id" in rendered + assert "EXISTS" in rendered.upper() + assert auth_context.workspace_id in params.values() + assert auth_context.organization_id in params.values() -def test_document_organization_filter_rejects_other_non_null_organizations() -> None: - """The compatibility branch is exactly current organization OR historical NULL.""" +def test_same_opaque_workspace_different_organization_needs_distinct_binding() -> None: + """A signed workspace claim alone cannot authorize another organization.""" - auth_context = _opaque_workspace_auth() - expected = or_( - Document.organization_id == auth_context.organization_id, - Document.organization_id.is_(None), - ) + owner = _opaque_workspace_auth(organization_id="org-acme") + other = _opaque_workspace_auth(organization_id="org-other") + + owner_rendered, owner_params = _compiled_document_scope(owner) + other_rendered, other_params = _compiled_document_scope(other) + + for rendered in (owner_rendered, other_rendered): + assert "workspace_entities.organization_id" in rendered + assert "EXISTS" in rendered.upper() + assert "IS NULL" in rendered.upper() - assert data_api._document_organization_filter(auth_context).compare(expected) + assert owner.organization_id in owner_params.values() + assert other.organization_id in other_params.values() + assert owner.workspace_id in owner_params.values() + assert other.workspace_id in other_params.values() From fea4d5c39448671c7a1a519930f4ef6bbfbe9fad Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 14 Sep 2026 19:54:29 +0900 Subject: [PATCH 18/27] test(data): correlate legacy document workspace binding --- ...ent_organization_scope_opaque_workspace.py | 48 ++++++++++++++----- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/backend/tests/test_document_organization_scope_opaque_workspace.py b/backend/tests/test_document_organization_scope_opaque_workspace.py index cb71b8e5e..9a2640d73 100644 --- a/backend/tests/test_document_organization_scope_opaque_workspace.py +++ b/backend/tests/test_document_organization_scope_opaque_workspace.py @@ -2,8 +2,8 @@ from api.auth import AuthContext import api.data as data_api -from db.models import Document -from sqlalchemy import select +from db.models import Document, Workspace +from sqlalchemy import and_, exists, or_, select def _opaque_workspace_auth(*, organization_id: str = "org-acme") -> AuthContext: @@ -18,6 +18,23 @@ def _opaque_workspace_auth(*, organization_id: str = "org-acme") -> AuthContext: ) +def _expected_organization_filter(auth_context: AuthContext): + """Describe the exact trusted binding required for historical NULL rows.""" + + trusted_workspace_binding = exists( + select(1) + .select_from(Workspace) + .where( + Workspace.workspace_id == auth_context.workspace_id, + Workspace.organization_id == auth_context.organization_id, + ) + ) + return or_( + Document.organization_id == auth_context.organization_id, + and_(Document.organization_id.is_(None), trusted_workspace_binding), + ) + + def _compiled_document_scope(auth_context: AuthContext) -> tuple[str, dict[str, object]]: """Compile the document scope so tenant-binding predicates stay observable.""" @@ -29,33 +46,42 @@ def _compiled_document_scope(auth_context: AuthContext) -> tuple[str, dict[str, return str(statement.whereclause), compiled.params -def test_opaque_workspace_legacy_null_requires_trusted_organization_binding() -> None: - """Legacy NULL rows require a server-side workspace-to-org binding.""" +def test_opaque_workspace_legacy_null_requires_correlated_organization_binding() -> None: + """Legacy NULL access requires one workspace row matching both signed claims.""" auth_context = _opaque_workspace_auth() - rendered, params = _compiled_document_scope(auth_context) + assert data_api._document_organization_filter(auth_context).compare( + _expected_organization_filter(auth_context) + ) + + rendered, params = _compiled_document_scope(auth_context) assert "workspace_documents.workspace_id" in rendered - assert "workspace_documents.organization_id" in rendered - assert "IS NULL" in rendered.upper() - assert "workspace_entities" in rendered assert "workspace_entities.workspace_id" in rendered assert "workspace_entities.organization_id" in rendered + assert "IS NULL" in rendered.upper() assert "EXISTS" in rendered.upper() assert auth_context.workspace_id in params.values() assert auth_context.organization_id in params.values() -def test_same_opaque_workspace_different_organization_needs_distinct_binding() -> None: - """A signed workspace claim alone cannot authorize another organization.""" +def test_same_opaque_workspace_different_organization_cannot_share_null_branch() -> None: + """Each organization must correlate against its own registry binding.""" owner = _opaque_workspace_auth(organization_id="org-acme") other = _opaque_workspace_auth(organization_id="org-other") + owner_expected = _expected_organization_filter(owner) + other_expected = _expected_organization_filter(other) + + assert not owner_expected.compare(other_expected) + assert data_api._document_organization_filter(owner).compare(owner_expected) + assert data_api._document_organization_filter(other).compare(other_expected) + owner_rendered, owner_params = _compiled_document_scope(owner) other_rendered, other_params = _compiled_document_scope(other) - for rendered in (owner_rendered, other_rendered): + assert "workspace_entities.workspace_id" in rendered assert "workspace_entities.organization_id" in rendered assert "EXISTS" in rendered.upper() assert "IS NULL" in rendered.upper() From 8e63a20a1641cc5e7c53b196405ac942ff53ea9c Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 14 Sep 2026 20:48:14 +0900 Subject: [PATCH 19/27] test: require auditable workspace organization binding migration --- ...organization_binding_migration_postgres.py | 203 ++++++++++++++++++ 1 file changed, 203 insertions(+) create mode 100644 backend/tests/test_workspace_organization_binding_migration_postgres.py diff --git a/backend/tests/test_workspace_organization_binding_migration_postgres.py b/backend/tests/test_workspace_organization_binding_migration_postgres.py new file mode 100644 index 000000000..ae604703d --- /dev/null +++ b/backend/tests/test_workspace_organization_binding_migration_postgres.py @@ -0,0 +1,203 @@ +"""PostgreSQL acceptance for auditable workspace-organization binding. + +The workspace identifier is an opaque authenticated claim. Historical ownership +must therefore be recovered only from server-side evidence already persisted in +``workspace_documents.organization_id``; identifier shape is not ownership +evidence. Ambiguous and evidence-free workspaces stay unbound so compatibility +access can fail closed instead of guessing a tenant. +""" + +import secrets +import subprocess +import sys +import uuid +from pathlib import Path + +import asyncpg +import pytest +import pytest_asyncio +from sqlalchemy import text +from sqlalchemy.engine import make_url +from sqlalchemy.ext.asyncio import create_async_engine + +from core.config import settings + +pytestmark = pytest.mark.postgres + +_BACKEND_ROOT = Path(__file__).resolve().parents[1] +_PRE_BINDING_REVISION = "0019_email_read_state_repair" + + +def _run_migrations(database_url: str, revision: str = "head") -> None: + result = subprocess.run( + [sys.executable, str(_BACKEND_ROOT / "scripts" / "migrate_db.py"), revision], + cwd=_BACKEND_ROOT, + env={ + "DATABASE_URL": database_url, + "AUTH_SESSION_HMAC_SECRET": secrets.token_urlsafe(48), + }, + capture_output=True, + text=True, + timeout=180, + ) + output = result.stdout + result.stderr + assert result.returncode == 0, output + assert not any( + status in output for status in ("Timeout", "Fatal", "Warn", "Denied") + ), output + + +@pytest_asyncio.fixture +async def fresh_database_url(): + base_url = make_url(settings.DATABASE_URL) + database_name = f"test_workspace_binding_{uuid.uuid4().hex[:12]}" + + async def admin(sql: str) -> None: + connection = await asyncpg.connect( + host=base_url.host, + port=base_url.port, + user=base_url.username, + password=base_url.password, + database="postgres", + ) + try: + await connection.execute(sql) + finally: + await connection.close() + + try: + await admin(f'CREATE DATABASE "{database_name}"') + except Exception as exc: + pytest.fail( + f"PostgreSQL is required for workspace binding acceptance: {exc}", + pytrace=False, + ) + + database_url = base_url.set(database=database_name).render_as_string( + hide_password=False + ) + try: + yield database_url + finally: + try: + await admin(f'DROP DATABASE IF EXISTS "{database_name}" WITH (FORCE)') + except Exception as exc: + pytest.fail( + f"PostgreSQL cleanup failed for workspace binding acceptance: {exc}", + pytrace=False, + ) + + +async def _seed_historical_workspace_evidence(database_url: str) -> None: + engine = create_async_engine(database_url) + try: + async with engine.begin() as connection: + for workspace_id in ( + "opaque-unambiguous-7f3c", + "opaque-ambiguous-8a4d", + "opaque-unbound-9b5e", + ): + await connection.execute( + text( + """ + INSERT INTO workspace_entities + (workspace_id, workspace_name, workspace_domain, created_at) + VALUES (:workspace_id, :workspace_id, NULL, now()) + """ + ), + {"workspace_id": workspace_id}, + ) + + documents = ( + ( + "doc-unambiguous-known", + "opaque-unambiguous-7f3c", + "org-acme", + ), + ( + "doc-unambiguous-legacy", + "opaque-unambiguous-7f3c", + None, + ), + ("doc-ambiguous-a", "opaque-ambiguous-8a4d", "org-red"), + ("doc-ambiguous-b", "opaque-ambiguous-8a4d", "org-blue"), + ("doc-ambiguous-legacy", "opaque-ambiguous-8a4d", None), + ("doc-unbound-legacy", "opaque-unbound-9b5e", None), + ) + for document_id, workspace_id, organization_id in documents: + await connection.execute( + text( + """ + INSERT INTO workspace_documents + (document_id, workspace_id, organization_id, + document_name, document_type, document_content, + document_status, created_at) + VALUES + (:document_id, :workspace_id, :organization_id, + :document_id, 'text/markdown', '# historical', + 'uploaded', now()) + """ + ), + { + "document_id": document_id, + "workspace_id": workspace_id, + "organization_id": organization_id, + }, + ) + finally: + await engine.dispose() + + +async def _read_bindings(database_url: str) -> tuple[dict[str, str | None], dict[str, str | None]]: + engine = create_async_engine(database_url) + try: + async with engine.connect() as connection: + workspace_result = await connection.execute( + text( + "SELECT workspace_id, organization_id " + "FROM workspace_entities ORDER BY workspace_id" + ) + ) + document_result = await connection.execute( + text( + "SELECT document_id, organization_id " + "FROM workspace_documents ORDER BY document_id" + ) + ) + return ( + {row.workspace_id: row.organization_id for row in workspace_result}, + {row.document_id: row.organization_id for row in document_result}, + ) + finally: + await engine.dispose() + + +@pytest.mark.asyncio +async def test_binding_migration_uses_only_unambiguous_persisted_ownership_evidence( + fresh_database_url, +) -> None: + """Bind/backfill one-owner history and leave ambiguous or unknown history closed.""" + + _run_migrations(fresh_database_url, revision=_PRE_BINDING_REVISION) + await _seed_historical_workspace_evidence(fresh_database_url) + + _run_migrations(fresh_database_url) + workspace_bindings, document_bindings = await _read_bindings(fresh_database_url) + + assert workspace_bindings["opaque-unambiguous-7f3c"] == "org-acme" + assert document_bindings["doc-unambiguous-legacy"] == "org-acme" + + assert workspace_bindings["opaque-ambiguous-8a4d"] is None + assert document_bindings["doc-ambiguous-legacy"] is None + + assert workspace_bindings["opaque-unbound-9b5e"] is None + assert document_bindings["doc-unbound-legacy"] is None + + # A repeated managed upgrade is a required deployment path. It must not + # invent new ownership or mutate the fail-closed ambiguous/unbound rows. + _run_migrations(fresh_database_url) + repeated_workspace_bindings, repeated_document_bindings = await _read_bindings( + fresh_database_url + ) + assert repeated_workspace_bindings == workspace_bindings + assert repeated_document_bindings == document_bindings From 3b2e5ba5d091147a6add344b1f3ff44b3f40b3d6 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 14 Sep 2026 20:48:39 +0900 Subject: [PATCH 20/27] fix: bind workspaces from auditable organization evidence --- .../0020_workspace_organization_binding.py | 160 ++++++++++++++++++ 1 file changed, 160 insertions(+) create mode 100644 backend/alembic/versions/0020_workspace_organization_binding.py diff --git a/backend/alembic/versions/0020_workspace_organization_binding.py b/backend/alembic/versions/0020_workspace_organization_binding.py new file mode 100644 index 000000000..fb2eff4c1 --- /dev/null +++ b/backend/alembic/versions/0020_workspace_organization_binding.py @@ -0,0 +1,160 @@ +"""bind workspace registry rows to auditable organization evidence + +Revision ID: 0020_workspace_organization_binding +Revises: 0019_email_read_state_repair +Create Date: 2026-09-14 20:41:00.000000 + +``workspace_id`` is an opaque authenticated claim, not an organization-derived +identifier. This revision therefore binds a workspace only when persisted +``workspace_documents.organization_id`` evidence is non-null and unambiguous: +exactly one distinct organization is already recorded for that workspace. +Ambiguous and evidence-free workspaces remain unbound so authorization can fail +closed instead of guessing an owner from an identifier shape. + +Once a workspace is safely bound, legacy NULL document rows in that same +workspace inherit the binding. Downgrade is intentionally non-destructive: +these bindings are ownership provenance and cannot be distinguished later from +assignments written by normal application traffic. +""" + +from alembic import op +import sqlalchemy as sa + +revision = "0020_workspace_organization_binding" +down_revision = "0019_email_read_state_repair" +branch_labels = None +depends_on = None + +_ENTITIES_TABLE = "workspace_entities" +_DOCUMENTS_TABLE = "workspace_documents" +_ORGANIZATION_COLUMN = "organization_id" +_ORGANIZATION_INDEX = "ix_workspace_entities_organization_id" + + +def upgrade() -> None: + connection = op.get_bind() + inspector = sa.inspect(connection) + if not inspector.has_table(_ENTITIES_TABLE): + raise RuntimeError( + "workspace_entities must exist before workspace organization binding" + ) + if not inspector.has_table(_DOCUMENTS_TABLE): + raise RuntimeError( + "workspace_documents must exist before workspace organization binding" + ) + + entity_columns = { + column["name"] for column in inspector.get_columns(_ENTITIES_TABLE) + } + if _ORGANIZATION_COLUMN not in entity_columns: + op.add_column( + _ENTITIES_TABLE, + sa.Column(_ORGANIZATION_COLUMN, sa.String(), nullable=True), + ) + op.create_index( + _ORGANIZATION_INDEX, + _ENTITIES_TABLE, + [_ORGANIZATION_COLUMN], + if_not_exists=True, + ) + + workspace_entities = sa.table( + _ENTITIES_TABLE, + sa.column("workspace_id", sa.String()), + sa.column(_ORGANIZATION_COLUMN, sa.String()), + ) + workspace_documents = sa.table( + _DOCUMENTS_TABLE, + sa.column("workspace_id", sa.String()), + sa.column(_ORGANIZATION_COLUMN, sa.String()), + ) + + ownership_evidence = ( + sa.select( + workspace_documents.c.workspace_id.label("workspace_id"), + sa.func.min(workspace_documents.c.organization_id).label( + "organization_id" + ), + sa.func.count(sa.distinct(workspace_documents.c.organization_id)).label( + "organization_count" + ), + ) + .where(workspace_documents.c.organization_id.is_not(None)) + .group_by(workspace_documents.c.workspace_id) + .subquery() + ) + unambiguous_evidence = ( + sa.select( + ownership_evidence.c.workspace_id, + ownership_evidence.c.organization_id, + ) + .where(ownership_evidence.c.organization_count == 1) + .subquery() + ) + + inferred_organization = ( + sa.select(unambiguous_evidence.c.organization_id) + .where( + unambiguous_evidence.c.workspace_id + == workspace_entities.c.workspace_id + ) + .correlate(workspace_entities) + .scalar_subquery() + ) + has_unambiguous_evidence = ( + sa.exists( + sa.select(1) + .select_from(unambiguous_evidence) + .where( + unambiguous_evidence.c.workspace_id + == workspace_entities.c.workspace_id + ) + ) + .correlate(workspace_entities) + ) + connection.execute( + sa.update(workspace_entities) + .where( + workspace_entities.c.organization_id.is_(None), + has_unambiguous_evidence, + ) + .values(organization_id=inferred_organization) + ) + + bound_organization = ( + sa.select(workspace_entities.c.organization_id) + .where( + workspace_entities.c.workspace_id == workspace_documents.c.workspace_id, + workspace_entities.c.organization_id.is_not(None), + ) + .correlate(workspace_documents) + .scalar_subquery() + ) + has_binding = ( + sa.exists( + sa.select(1) + .select_from(workspace_entities) + .where( + workspace_entities.c.workspace_id + == workspace_documents.c.workspace_id, + workspace_entities.c.organization_id.is_not(None), + ) + ) + .correlate(workspace_documents) + ) + connection.execute( + sa.update(workspace_documents) + .where( + workspace_documents.c.organization_id.is_(None), + has_binding, + ) + .values(organization_id=bound_organization) + ) + + +def downgrade() -> None: + # Binding/backfill turns ambiguous legacy NULLs into explicit ownership + # provenance. A later downgrade cannot distinguish those values from normal + # application assignments, so removing them or the column could destroy + # security-relevant tenant evidence. + return None From bc23bbca05a3ae3749c9d1f7e66b5b070b18e211 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 14 Sep 2026 20:50:23 +0900 Subject: [PATCH 21/27] fix: add fail-closed workspace organization binding service --- backend/services/workspace_scope.py | 114 +++++++++++++++++++++++++--- 1 file changed, 103 insertions(+), 11 deletions(-) diff --git a/backend/services/workspace_scope.py b/backend/services/workspace_scope.py index 182342163..79d244235 100644 --- a/backend/services/workspace_scope.py +++ b/backend/services/workspace_scope.py @@ -1,11 +1,107 @@ import datetime +from typing import Literal -from sqlalchemy import select +from sqlalchemy import DateTime, String, column, select, table, update from sqlalchemy.dialects.postgresql import insert from sqlalchemy.ext.asyncio import AsyncSession from db.models import Workspace +SessionVerifier = Literal["hmac", "oidc", "override", "server"] +_TRUSTED_BINDING_VERIFIERS = frozenset({"oidc", "override", "server"}) +_WORKSPACE_REGISTRY = table( + "workspace_entities", + column("workspace_id", String()), + column("workspace_name", String()), + column("organization_id", String()), + column("created_at", DateTime(timezone=True)), +) + + +class WorkspaceOrganizationBindingRequired(RuntimeError): + """Raised when a session cannot establish an unbound workspace owner.""" + + +class WorkspaceOrganizationConflict(RuntimeError): + """Raised when a workspace is already bound to another organization.""" + + +async def _workspace_organization_binding( + session: AsyncSession, + workspace_id: str, +) -> str | None: + """Read the server-side organization binding for one opaque workspace.""" + + result = await session.execute( + select(_WORKSPACE_REGISTRY.c.organization_id).where( + _WORKSPACE_REGISTRY.c.workspace_id == workspace_id + ) + ) + row = result.one_or_none() + return None if row is None else row.organization_id + + +async def get_or_create_bound_workspace( + session: AsyncSession, + workspace_id: str, + organization_id: str, + *, + session_verifier: SessionVerifier, +) -> Workspace: + """Return a workspace only after validating its server-side organization binding. + + OIDC and explicit server/test override identities may establish a missing + binding because those paths are the configured membership-authority boundary. + HMAC compatibility sessions may use an already-bound workspace but can never + create or claim a binding. Concurrent bind attempts use insert/CAS semantics; + the losing organization observes the winner and fails closed. + """ + + if not workspace_id or not organization_id: + raise WorkspaceOrganizationBindingRequired( + "workspace and organization claims are required for binding" + ) + + if session_verifier in _TRUSTED_BINDING_VERIFIERS: + await session.execute( + insert(_WORKSPACE_REGISTRY) + .values( + workspace_id=workspace_id, + workspace_name=workspace_id, + organization_id=organization_id, + created_at=datetime.datetime.now(datetime.timezone.utc), + ) + .on_conflict_do_nothing( + index_elements=[_WORKSPACE_REGISTRY.c.workspace_id] + ) + ) + await session.execute( + update(_WORKSPACE_REGISTRY) + .where( + _WORKSPACE_REGISTRY.c.workspace_id == workspace_id, + _WORKSPACE_REGISTRY.c.organization_id.is_(None), + ) + .values(organization_id=organization_id) + ) + + bound_organization_id = await _workspace_organization_binding( + session, + workspace_id, + ) + if bound_organization_id is None: + raise WorkspaceOrganizationBindingRequired( + "workspace has no trusted organization binding" + ) + if bound_organization_id != organization_id: + raise WorkspaceOrganizationConflict( + "workspace is bound to a different organization" + ) + + result = await session.execute( + select(Workspace).where(Workspace.workspace_id == workspace_id) + ) + return result.scalar_one() + async def get_or_create_workspace( session: AsyncSession, @@ -13,17 +109,13 @@ async def get_or_create_workspace( ) -> Workspace: """Return the ``Workspace`` row for ``workspace_id``, creating it first if needed. - ``workspace_id`` is the independently signed, opaque workspace membership - claim supplied by the authenticated boundary. This service does not derive - workspace identity from ``organization_id`` or ``user_id``. Rows created - here use the supplied claim as the primary key so - ``Document.workspace_id``'s foreign key resolves to the authenticated - workspace. + This compatibility path preserves callers that predate server-side + workspace↔organization binding. It establishes only the workspace foreign-key + row and is not authorization evidence. New tenant-sensitive callers must use + :func:`get_or_create_bound_workspace` with an authenticated organization and + verifier. """ - # The first two requests for a signed workspace may arrive concurrently. - # A SELECT-then-INSERT races on the workspace_id primary key, so let - # PostgreSQL serialize creation and return the row when this transaction - # won the insert. + result = await session.execute( insert(Workspace) .values( From 909dacce1366101bc76a19b3cc329725323cc244 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 14 Sep 2026 20:50:50 +0900 Subject: [PATCH 22/27] test: cover fail-closed workspace binding service --- .../test_workspace_scope_binding_postgres.py | 208 ++++++++++++++++++ 1 file changed, 208 insertions(+) create mode 100644 backend/tests/test_workspace_scope_binding_postgres.py diff --git a/backend/tests/test_workspace_scope_binding_postgres.py b/backend/tests/test_workspace_scope_binding_postgres.py new file mode 100644 index 000000000..b56cc1f4d --- /dev/null +++ b/backend/tests/test_workspace_scope_binding_postgres.py @@ -0,0 +1,208 @@ +"""Real PostgreSQL acceptance for server-side workspace organization binding.""" + +import asyncio +import secrets +import subprocess +import sys +import uuid +from pathlib import Path + +import asyncpg +import pytest +import pytest_asyncio +from sqlalchemy import text +from sqlalchemy.engine import make_url +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine + +from core.config import settings +from services.workspace_scope import ( + WorkspaceOrganizationBindingRequired, + WorkspaceOrganizationConflict, + get_or_create_bound_workspace, +) + +pytestmark = pytest.mark.postgres + +_BACKEND_ROOT = Path(__file__).resolve().parents[1] + + +def _run_migrations(database_url: str) -> None: + """Apply the managed migration path with only required bootstrap settings.""" + + result = subprocess.run( + [sys.executable, str(_BACKEND_ROOT / "scripts" / "migrate_db.py"), "head"], + cwd=_BACKEND_ROOT, + env={ + "DATABASE_URL": database_url, + "AUTH_SESSION_HMAC_SECRET": secrets.token_urlsafe(48), + }, + capture_output=True, + text=True, + timeout=180, + ) + output = result.stdout + result.stderr + assert result.returncode == 0, output + assert not any( + status in output for status in ("Timeout", "Fatal", "Warn", "Denied") + ), output + + +@pytest_asyncio.fixture +async def binding_database_url(): + """Create an isolated database; unavailable PostgreSQL is an acceptance failure.""" + + base_url = make_url(settings.DATABASE_URL) + database_name = f"test_workspace_scope_{uuid.uuid4().hex[:12]}" + + async def admin(sql: str) -> None: + connection = await asyncpg.connect( + host=base_url.host, + port=base_url.port, + user=base_url.username, + password=base_url.password, + database="postgres", + ) + try: + await connection.execute(sql) + finally: + await connection.close() + + try: + await admin(f'CREATE DATABASE "{database_name}"') + except Exception as exc: + pytest.fail( + f"PostgreSQL is required for workspace binding acceptance: {exc}", + pytrace=False, + ) + + database_url = base_url.set(database=database_name).render_as_string( + hide_password=False + ) + try: + _run_migrations(database_url) + yield database_url + finally: + try: + await admin(f'DROP DATABASE IF EXISTS "{database_name}" WITH (FORCE)') + except Exception as exc: + pytest.fail( + f"PostgreSQL cleanup failed for workspace binding acceptance: {exc}", + pytrace=False, + ) + + +async def _read_binding(database_url: str, workspace_id: str) -> str | None: + """Read the persisted binding independently of the service session.""" + + engine = create_async_engine(database_url) + try: + async with engine.connect() as connection: + result = await connection.execute( + text( + "SELECT organization_id FROM workspace_entities " + "WHERE workspace_id = :workspace_id" + ), + {"workspace_id": workspace_id}, + ) + return result.scalar_one_or_none() + finally: + await engine.dispose() + + +@pytest.mark.asyncio +async def test_binding_requires_trusted_establishment_and_rejects_mismatch( + binding_database_url, +) -> None: + """HMAC may consume existing evidence but cannot create or change ownership.""" + + engine = create_async_engine(binding_database_url) + session_factory = async_sessionmaker(engine, expire_on_commit=False) + try: + async with session_factory() as session: + with pytest.raises(WorkspaceOrganizationBindingRequired): + async with session.begin(): + await get_or_create_bound_workspace( + session, + "opaque-hmac-unbound-1", + "org-acme", + session_verifier="hmac", + ) + + assert await _read_binding(binding_database_url, "opaque-hmac-unbound-1") is None + + async with session_factory() as session: + async with session.begin(): + created = await get_or_create_bound_workspace( + session, + "opaque-trusted-7f3c", + "org-acme", + session_verifier="override", + ) + assert created.workspace_id == "opaque-trusted-7f3c" + + assert await _read_binding(binding_database_url, "opaque-trusted-7f3c") == "org-acme" + + async with session_factory() as session: + async with session.begin(): + reused = await get_or_create_bound_workspace( + session, + "opaque-trusted-7f3c", + "org-acme", + session_verifier="hmac", + ) + assert reused.workspace_id == "opaque-trusted-7f3c" + + async with session_factory() as session: + with pytest.raises(WorkspaceOrganizationConflict): + async with session.begin(): + await get_or_create_bound_workspace( + session, + "opaque-trusted-7f3c", + "org-other", + session_verifier="oidc", + ) + + assert await _read_binding(binding_database_url, "opaque-trusted-7f3c") == "org-acme" + finally: + await engine.dispose() + + +@pytest.mark.asyncio +async def test_concurrent_trusted_claims_have_one_binding_winner( + binding_database_url, +) -> None: + """Concurrent organizations cannot both claim the same opaque workspace.""" + + engine = create_async_engine(binding_database_url) + session_factory = async_sessionmaker(engine, expire_on_commit=False) + workspace_id = "opaque-race-8a4d" + + async def attempt(organization_id: str): + """Attempt one transactional binding and return either row or conflict.""" + + try: + async with session_factory() as session: + async with session.begin(): + return await get_or_create_bound_workspace( + session, + workspace_id, + organization_id, + session_verifier="oidc", + ) + except WorkspaceOrganizationConflict as exc: + return exc + + try: + results = await asyncio.gather(attempt("org-red"), attempt("org-blue")) + conflicts = [ + result for result in results if isinstance(result, WorkspaceOrganizationConflict) + ] + winners = [result for result in results if not isinstance(result, Exception)] + + assert len(conflicts) == 1 + assert len(winners) == 1 + persisted = await _read_binding(binding_database_url, workspace_id) + assert persisted in {"org-red", "org-blue"} + assert winners[0].workspace_id == workspace_id + finally: + await engine.dispose() From 58655161228d1216933f87f5db255d9dbff5000c Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 15 Sep 2026 00:46:37 +0900 Subject: [PATCH 23/27] test: require personal workspace owner binding --- ...ocument_personal_scope_opaque_workspace.py | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 backend/tests/test_document_personal_scope_opaque_workspace.py diff --git a/backend/tests/test_document_personal_scope_opaque_workspace.py b/backend/tests/test_document_personal_scope_opaque_workspace.py new file mode 100644 index 000000000..2e94c1261 --- /dev/null +++ b/backend/tests/test_document_personal_scope_opaque_workspace.py @@ -0,0 +1,93 @@ +"""Regression contracts for personal-scope opaque workspace authorization.""" + +from api.auth import AuthContext +import api.data as data_api +from db.models import Document, Workspace +from sqlalchemy import and_, exists, select + + +def _personal_workspace_auth(*, user_id: str = "member-a") -> AuthContext: + """Build a personal signed-session shape with an opaque workspace claim.""" + + return AuthContext( + user_id=user_id, + role="member", + organization_id=None, + group_ids=(), + workspace_id="tenant-personal-7f3c", + ) + + +def _expected_personal_scope(auth_context: AuthContext): + """Require one registry row correlating the opaque workspace to its owner.""" + + assert hasattr(Workspace, "owner_user_id"), ( + "workspace_entities must persist owner_user_id before personal opaque " + "workspace claims can authorize organization-null documents" + ) + trusted_workspace_binding = exists( + select(1) + .select_from(Workspace) + .where( + Workspace.workspace_id == auth_context.workspace_id, + Workspace.organization_id.is_(None), + Workspace.owner_user_id == auth_context.user_id, + ) + ) + return and_(Document.organization_id.is_(None), trusted_workspace_binding) + + +def _compiled_document_scope(auth_context: AuthContext) -> tuple[str, dict[str, object]]: + """Compile the personal document scope so ownership correlation is observable.""" + + statement = select(Document.document_id).where( + Document.workspace_id == auth_context.workspace_id, + data_api._document_organization_filter(auth_context), + ) + compiled = statement.compile() + return str(statement.whereclause), compiled.params + + +def test_personal_opaque_workspace_requires_correlated_owner_user_binding() -> None: + """Personal NULL-organization access requires a server-side user binding.""" + + auth_context = _personal_workspace_auth() + expected = _expected_personal_scope(auth_context) + + assert data_api._document_organization_filter(auth_context).compare(expected) + + rendered, params = _compiled_document_scope(auth_context) + assert "workspace_documents.workspace_id" in rendered + assert "workspace_entities.workspace_id" in rendered + assert "workspace_entities.owner_user_id" in rendered + assert "workspace_entities.organization_id" in rendered + assert "IS NULL" in rendered.upper() + assert "EXISTS" in rendered.upper() + assert auth_context.workspace_id in params.values() + assert auth_context.user_id in params.values() + + +def test_same_personal_opaque_workspace_different_users_cannot_share_null_documents() -> None: + """Two signed users cannot share one personal opaque-workspace NULL branch.""" + + owner = _personal_workspace_auth(user_id="member-a") + other = _personal_workspace_auth(user_id="member-b") + owner_expected = _expected_personal_scope(owner) + other_expected = _expected_personal_scope(other) + + assert not owner_expected.compare(other_expected) + assert data_api._document_organization_filter(owner).compare(owner_expected) + assert data_api._document_organization_filter(other).compare(other_expected) + + owner_rendered, owner_params = _compiled_document_scope(owner) + other_rendered, other_params = _compiled_document_scope(other) + for rendered in (owner_rendered, other_rendered): + assert "workspace_entities.owner_user_id" in rendered + assert "workspace_entities.workspace_id" in rendered + assert "EXISTS" in rendered.upper() + assert "IS NULL" in rendered.upper() + + assert owner.user_id in owner_params.values() + assert other.user_id in other_params.values() + assert owner.workspace_id in owner_params.values() + assert other.workspace_id in other_params.values() From c2c1cd41a64d51172b1b2802bcd12ab2412ef5f0 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 15 Sep 2026 00:47:59 +0900 Subject: [PATCH 24/27] fix: add personal workspace owner binding migration --- .../0021_workspace_personal_owner_binding.py | 128 ++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 backend/alembic/versions/0021_workspace_personal_owner_binding.py diff --git a/backend/alembic/versions/0021_workspace_personal_owner_binding.py b/backend/alembic/versions/0021_workspace_personal_owner_binding.py new file mode 100644 index 000000000..e6d978304 --- /dev/null +++ b/backend/alembic/versions/0021_workspace_personal_owner_binding.py @@ -0,0 +1,128 @@ +"""add fail-closed personal owner binding to the workspace registry + +Revision ID: 0021_workspace_personal_owner_binding +Revises: 0020_workspace_organization_binding +Create Date: 2026-09-15 00:51:00.000000 + +Personal workspaces have ``organization_id IS NULL`` and therefore cannot use +organization binding as their membership evidence. ``workspace_id`` is opaque, +so identifier shape such as ``workspace-`` is not ownership evidence. +This revision adds a nullable ``owner_user_id`` slot for trusted runtime +establishment while deliberately leaving historical rows unbound: the current +schema contains no auditable persisted user owner from which to backfill them. + +The registry may be unbound, organization-bound, or personal-user-bound, but it +must never be bound to both an organization and a personal user at once. +Downgrade removes the column only when no personal ownership provenance has +been written; otherwise it fails closed rather than destroying that evidence. +""" + +from alembic import op +import sqlalchemy as sa + +revision = "0021_workspace_personal_owner_binding" +down_revision = "0020_workspace_organization_binding" +branch_labels = None +depends_on = None + +_ENTITIES_TABLE = "workspace_entities" +_OWNER_USER_COLUMN = "owner_user_id" +_OWNER_USER_INDEX = "ix_workspace_entities_owner_user_id" +_SCOPE_OWNER_CHECK = "ck_workspace_entities_single_scope_owner" + + +def upgrade() -> None: + """Add personal-owner evidence without guessing ownership for legacy rows.""" + + connection = op.get_bind() + inspector = sa.inspect(connection) + if not inspector.has_table(_ENTITIES_TABLE): + raise RuntimeError( + "workspace_entities must exist before personal workspace binding" + ) + + entity_columns = { + column["name"] for column in inspector.get_columns(_ENTITIES_TABLE) + } + if "organization_id" not in entity_columns: + raise RuntimeError( + "organization_id binding must exist before personal workspace binding" + ) + if _OWNER_USER_COLUMN not in entity_columns: + op.add_column( + _ENTITIES_TABLE, + sa.Column(_OWNER_USER_COLUMN, sa.String(), nullable=True), + ) + + inspector = sa.inspect(connection) + index_names = { + index["name"] + for index in inspector.get_indexes(_ENTITIES_TABLE) + if index.get("name") + } + if _OWNER_USER_INDEX not in index_names: + op.create_index( + _OWNER_USER_INDEX, + _ENTITIES_TABLE, + [_OWNER_USER_COLUMN], + ) + + check_names = { + constraint["name"] + for constraint in inspector.get_check_constraints(_ENTITIES_TABLE) + if constraint.get("name") + } + if _SCOPE_OWNER_CHECK not in check_names: + op.create_check_constraint( + _SCOPE_OWNER_CHECK, + _ENTITIES_TABLE, + "NOT (organization_id IS NOT NULL AND owner_user_id IS NOT NULL)", + ) + + +def downgrade() -> None: + """Remove the owner column only while it carries no security provenance.""" + + connection = op.get_bind() + inspector = sa.inspect(connection) + if not inspector.has_table(_ENTITIES_TABLE): + return + + entity_columns = { + column["name"] for column in inspector.get_columns(_ENTITIES_TABLE) + } + if _OWNER_USER_COLUMN not in entity_columns: + return + + owner_count = connection.execute( + sa.text( + "SELECT count(*) FROM workspace_entities " + "WHERE owner_user_id IS NOT NULL" + ) + ).scalar_one() + if owner_count: + raise RuntimeError( + "cannot downgrade personal workspace binding while owner provenance exists" + ) + + check_names = { + constraint["name"] + for constraint in inspector.get_check_constraints(_ENTITIES_TABLE) + if constraint.get("name") + } + if _SCOPE_OWNER_CHECK in check_names: + op.drop_constraint( + _SCOPE_OWNER_CHECK, + _ENTITIES_TABLE, + type_="check", + ) + + inspector = sa.inspect(connection) + index_names = { + index["name"] + for index in inspector.get_indexes(_ENTITIES_TABLE) + if index.get("name") + } + if _OWNER_USER_INDEX in index_names: + op.drop_index(_OWNER_USER_INDEX, table_name=_ENTITIES_TABLE) + op.drop_column(_ENTITIES_TABLE, _OWNER_USER_COLUMN) From 854ad91a49f01dafb3ccd589e65b2ec2eb0539d2 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 15 Sep 2026 00:48:28 +0900 Subject: [PATCH 25/27] fix: enforce personal workspace binding semantics --- backend/services/workspace_scope.py | 142 +++++++++++++++++++++------- 1 file changed, 110 insertions(+), 32 deletions(-) diff --git a/backend/services/workspace_scope.py b/backend/services/workspace_scope.py index 79d244235..9a05fdc8c 100644 --- a/backend/services/workspace_scope.py +++ b/backend/services/workspace_scope.py @@ -14,61 +14,87 @@ column("workspace_id", String()), column("workspace_name", String()), column("organization_id", String()), + column("owner_user_id", String()), column("created_at", DateTime(timezone=True)), ) class WorkspaceOrganizationBindingRequired(RuntimeError): - """Raised when a session cannot establish an unbound workspace owner.""" + """Raised when a session lacks trusted workspace ownership evidence.""" class WorkspaceOrganizationConflict(RuntimeError): - """Raised when a workspace is already bound to another organization.""" + """Raised when a workspace is already bound to a different tenant owner.""" -async def _workspace_organization_binding( +async def _workspace_scope_binding( session: AsyncSession, workspace_id: str, -) -> str | None: - """Read the server-side organization binding for one opaque workspace.""" +) -> tuple[str | None, str | None]: + """Read organization and personal-user ownership for one opaque workspace.""" result = await session.execute( - select(_WORKSPACE_REGISTRY.c.organization_id).where( - _WORKSPACE_REGISTRY.c.workspace_id == workspace_id - ) + select( + _WORKSPACE_REGISTRY.c.organization_id, + _WORKSPACE_REGISTRY.c.owner_user_id, + ).where(_WORKSPACE_REGISTRY.c.workspace_id == workspace_id) ) row = result.one_or_none() - return None if row is None else row.organization_id + if row is None: + return None, None + return row.organization_id, row.owner_user_id -async def get_or_create_bound_workspace( +async def _workspace_organization_binding( session: AsyncSession, workspace_id: str, - organization_id: str, +) -> str | None: + """Read the organization binding while preserving the legacy helper contract.""" + + organization_id, _owner_user_id = await _workspace_scope_binding( + session, + workspace_id, + ) + return organization_id + + +async def get_or_create_scoped_workspace( + session: AsyncSession, + workspace_id: str, + organization_id: str | None, *, + owner_user_id: str | None = None, session_verifier: SessionVerifier, ) -> Workspace: - """Return a workspace only after validating its server-side organization binding. - - OIDC and explicit server/test override identities may establish a missing - binding because those paths are the configured membership-authority boundary. - HMAC compatibility sessions may use an already-bound workspace but can never - create or claim a binding. Concurrent bind attempts use insert/CAS semantics; - the losing organization observes the winner and fails closed. + """Return a workspace only after validating server-side tenant ownership. + + Organization scope binds the opaque workspace to ``organization_id`` and + deliberately does not bind it to one member. Personal scope has + ``organization_id is None`` and instead requires ``owner_user_id``. OIDC, + server, and explicit test override identities may establish an entirely + unbound registry row; HMAC compatibility sessions may consume existing + evidence but cannot claim ownership. Concurrent claims use insert/CAS + semantics so only one tenant owner can win. """ - if not workspace_id or not organization_id: + if not workspace_id: + raise WorkspaceOrganizationBindingRequired("workspace claim is required") + if organization_id is None and not owner_user_id: raise WorkspaceOrganizationBindingRequired( - "workspace and organization claims are required for binding" + "personal workspace claims require an authenticated owner user" ) + target_organization_id = organization_id + target_owner_user_id = owner_user_id if organization_id is None else None + if session_verifier in _TRUSTED_BINDING_VERIFIERS: await session.execute( insert(_WORKSPACE_REGISTRY) .values( workspace_id=workspace_id, workspace_name=workspace_id, - organization_id=organization_id, + organization_id=target_organization_id, + owner_user_id=target_owner_user_id, created_at=datetime.datetime.now(datetime.timezone.utc), ) .on_conflict_do_nothing( @@ -80,21 +106,34 @@ async def get_or_create_bound_workspace( .where( _WORKSPACE_REGISTRY.c.workspace_id == workspace_id, _WORKSPACE_REGISTRY.c.organization_id.is_(None), + _WORKSPACE_REGISTRY.c.owner_user_id.is_(None), + ) + .values( + organization_id=target_organization_id, + owner_user_id=target_owner_user_id, ) - .values(organization_id=organization_id) ) - bound_organization_id = await _workspace_organization_binding( + bound_organization_id, bound_owner_user_id = await _workspace_scope_binding( session, workspace_id, ) - if bound_organization_id is None: + if bound_organization_id is None and bound_owner_user_id is None: raise WorkspaceOrganizationBindingRequired( - "workspace has no trusted organization binding" + "workspace has no trusted tenant ownership binding" ) - if bound_organization_id != organization_id: + + if organization_id is None: + if bound_organization_id is not None or bound_owner_user_id != owner_user_id: + raise WorkspaceOrganizationConflict( + "personal workspace is bound to a different tenant owner" + ) + elif ( + bound_organization_id != organization_id + or bound_owner_user_id is not None + ): raise WorkspaceOrganizationConflict( - "workspace is bound to a different organization" + "workspace is bound to a different tenant owner" ) result = await session.execute( @@ -103,17 +142,56 @@ async def get_or_create_bound_workspace( return result.scalar_one() +async def get_or_create_bound_workspace( + session: AsyncSession, + workspace_id: str, + organization_id: str, + *, + session_verifier: SessionVerifier, +) -> Workspace: + """Compatibility wrapper for organization-scoped workspace ownership.""" + + if not organization_id: + raise WorkspaceOrganizationBindingRequired( + "workspace and organization claims are required for binding" + ) + return await get_or_create_scoped_workspace( + session, + workspace_id, + organization_id, + session_verifier=session_verifier, + ) + + +async def get_or_create_personal_workspace( + session: AsyncSession, + workspace_id: str, + owner_user_id: str, + *, + session_verifier: SessionVerifier, +) -> Workspace: + """Compatibility-safe entrypoint for a personal opaque workspace binding.""" + + return await get_or_create_scoped_workspace( + session, + workspace_id, + None, + owner_user_id=owner_user_id, + session_verifier=session_verifier, + ) + + async def get_or_create_workspace( session: AsyncSession, workspace_id: str, ) -> Workspace: """Return the ``Workspace`` row for ``workspace_id``, creating it first if needed. - This compatibility path preserves callers that predate server-side - workspace↔organization binding. It establishes only the workspace foreign-key - row and is not authorization evidence. New tenant-sensitive callers must use - :func:`get_or_create_bound_workspace` with an authenticated organization and - verifier. + This compatibility path preserves callers that predate server-side tenant + binding. It establishes only the workspace foreign-key row and is not + authorization evidence. New tenant-sensitive callers must use + :func:`get_or_create_scoped_workspace` (or one of its scoped wrappers) with + authenticated tenant claims and verifier provenance. """ result = await session.execute( From e204ea75d324809215c57de1fb389376039dc817 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 15 Sep 2026 00:48:48 +0900 Subject: [PATCH 26/27] test: cover personal workspace binding in PostgreSQL --- ...rkspace_personal_scope_binding_postgres.py | 270 ++++++++++++++++++ 1 file changed, 270 insertions(+) create mode 100644 backend/tests/test_workspace_personal_scope_binding_postgres.py diff --git a/backend/tests/test_workspace_personal_scope_binding_postgres.py b/backend/tests/test_workspace_personal_scope_binding_postgres.py new file mode 100644 index 000000000..6103be9df --- /dev/null +++ b/backend/tests/test_workspace_personal_scope_binding_postgres.py @@ -0,0 +1,270 @@ +"""Real PostgreSQL acceptance for personal opaque-workspace ownership binding.""" + +import asyncio +import secrets +import subprocess +import sys +import uuid +from pathlib import Path + +import asyncpg +import pytest +import pytest_asyncio +from sqlalchemy import text +from sqlalchemy.engine import make_url +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine + +from core.config import settings +from services.workspace_scope import ( + WorkspaceOrganizationBindingRequired, + WorkspaceOrganizationConflict, + get_or_create_personal_workspace, +) + +pytestmark = pytest.mark.postgres + +_BACKEND_ROOT = Path(__file__).resolve().parents[1] + + +def _run_migrations(database_url: str) -> None: + """Apply the managed migration path to an isolated PostgreSQL database.""" + + result = subprocess.run( + [sys.executable, str(_BACKEND_ROOT / "scripts" / "migrate_db.py"), "head"], + cwd=_BACKEND_ROOT, + env={ + "DATABASE_URL": database_url, + "AUTH_SESSION_HMAC_SECRET": secrets.token_urlsafe(48), + }, + capture_output=True, + text=True, + timeout=180, + ) + output = result.stdout + result.stderr + assert result.returncode == 0, output + assert not any( + status in output for status in ("Timeout", "Fatal", "Warn", "Denied") + ), output + + +@pytest_asyncio.fixture +async def personal_binding_database_url(): + """Create an isolated database; unavailable PostgreSQL is an acceptance failure.""" + + base_url = make_url(settings.DATABASE_URL) + database_name = f"test_personal_workspace_{uuid.uuid4().hex[:12]}" + + async def admin(sql: str) -> None: + """Execute one database-administration statement outside the test database.""" + + connection = await asyncpg.connect( + host=base_url.host, + port=base_url.port, + user=base_url.username, + password=base_url.password, + database="postgres", + ) + try: + await connection.execute(sql) + finally: + await connection.close() + + try: + await admin(f'CREATE DATABASE "{database_name}"') + except Exception as exc: + pytest.fail( + f"PostgreSQL is required for personal workspace acceptance: {exc}", + pytrace=False, + ) + + database_url = base_url.set(database=database_name).render_as_string( + hide_password=False + ) + try: + _run_migrations(database_url) + yield database_url + finally: + try: + await admin(f'DROP DATABASE IF EXISTS "{database_name}" WITH (FORCE)') + except Exception as exc: + pytest.fail( + f"PostgreSQL cleanup failed for personal workspace acceptance: {exc}", + pytrace=False, + ) + + +async def _read_binding( + database_url: str, + workspace_id: str, +) -> tuple[str | None, str | None] | None: + """Read registry ownership independently of the service transaction.""" + + engine = create_async_engine(database_url) + try: + async with engine.connect() as connection: + result = await connection.execute( + text( + "SELECT organization_id, owner_user_id FROM workspace_entities " + "WHERE workspace_id = :workspace_id" + ), + {"workspace_id": workspace_id}, + ) + row = result.one_or_none() + if row is None: + return None + return row.organization_id, row.owner_user_id + finally: + await engine.dispose() + + +@pytest.mark.asyncio +async def test_personal_binding_requires_trusted_establishment_and_rejects_other_user( + personal_binding_database_url, +) -> None: + """HMAC may consume personal ownership but cannot establish or reassign it.""" + + engine = create_async_engine(personal_binding_database_url) + session_factory = async_sessionmaker(engine, expire_on_commit=False) + try: + async with session_factory() as session: + with pytest.raises(WorkspaceOrganizationBindingRequired): + async with session.begin(): + await get_or_create_personal_workspace( + session, + "opaque-personal-hmac-unbound-1", + "member-a", + session_verifier="hmac", + ) + + assert ( + await _read_binding( + personal_binding_database_url, + "opaque-personal-hmac-unbound-1", + ) + is None + ) + + async with session_factory() as session: + async with session.begin(): + created = await get_or_create_personal_workspace( + session, + "opaque-personal-trusted-7f3c", + "member-a", + session_verifier="override", + ) + assert created.workspace_id == "opaque-personal-trusted-7f3c" + + assert await _read_binding( + personal_binding_database_url, + "opaque-personal-trusted-7f3c", + ) == (None, "member-a") + + async with session_factory() as session: + async with session.begin(): + reused = await get_or_create_personal_workspace( + session, + "opaque-personal-trusted-7f3c", + "member-a", + session_verifier="hmac", + ) + assert reused.workspace_id == "opaque-personal-trusted-7f3c" + + async with session_factory() as session: + with pytest.raises(WorkspaceOrganizationConflict): + async with session.begin(): + await get_or_create_personal_workspace( + session, + "opaque-personal-trusted-7f3c", + "member-b", + session_verifier="oidc", + ) + + assert await _read_binding( + personal_binding_database_url, + "opaque-personal-trusted-7f3c", + ) == (None, "member-a") + finally: + await engine.dispose() + + +@pytest.mark.asyncio +async def test_concurrent_personal_claims_have_one_owner_winner( + personal_binding_database_url, +) -> None: + """Concurrent users cannot both claim the same personal opaque workspace.""" + + engine = create_async_engine(personal_binding_database_url) + session_factory = async_sessionmaker(engine, expire_on_commit=False) + workspace_id = "opaque-personal-race-8a4d" + + async def attempt(owner_user_id: str): + """Attempt one transactional personal binding and return row or conflict.""" + + try: + async with session_factory() as session: + async with session.begin(): + return await get_or_create_personal_workspace( + session, + workspace_id, + owner_user_id, + session_verifier="oidc", + ) + except WorkspaceOrganizationConflict as exc: + return exc + + try: + results = await asyncio.gather(attempt("member-red"), attempt("member-blue")) + conflicts = [ + result + for result in results + if isinstance(result, WorkspaceOrganizationConflict) + ] + winners = [result for result in results if not isinstance(result, Exception)] + + assert len(conflicts) == 1 + assert len(winners) == 1 + persisted = await _read_binding(personal_binding_database_url, workspace_id) + assert persisted in {(None, "member-red"), (None, "member-blue")} + assert winners[0].workspace_id == workspace_id + finally: + await engine.dispose() + + +@pytest.mark.asyncio +async def test_personal_binding_cannot_replace_organization_binding( + personal_binding_database_url, +) -> None: + """Personal ownership cannot cross the registry's organization-bound invariant.""" + + engine = create_async_engine(personal_binding_database_url) + try: + async with engine.begin() as connection: + await connection.execute( + text( + "INSERT INTO workspace_entities " + "(workspace_id, workspace_name, organization_id, owner_user_id, created_at) " + "VALUES (:workspace_id, :workspace_id, :organization_id, NULL, now())" + ), + { + "workspace_id": "opaque-org-owned-9b5e", + "organization_id": "org-acme", + }, + ) + + session_factory = async_sessionmaker(engine, expire_on_commit=False) + async with session_factory() as session: + with pytest.raises(WorkspaceOrganizationConflict): + async with session.begin(): + await get_or_create_personal_workspace( + session, + "opaque-org-owned-9b5e", + "member-a", + session_verifier="server", + ) + + assert await _read_binding( + personal_binding_database_url, + "opaque-org-owned-9b5e", + ) == ("org-acme", None) + finally: + await engine.dispose() From a74db4ea4e16d066502817f028d1710dc8d507be Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 15 Sep 2026 04:48:47 +0900 Subject: [PATCH 27/27] test(auth): require organization-only workspace binding --- ...est_document_organization_scope_opaque_workspace.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/backend/tests/test_document_organization_scope_opaque_workspace.py b/backend/tests/test_document_organization_scope_opaque_workspace.py index 9a2640d73..a8080aa36 100644 --- a/backend/tests/test_document_organization_scope_opaque_workspace.py +++ b/backend/tests/test_document_organization_scope_opaque_workspace.py @@ -21,12 +21,20 @@ def _opaque_workspace_auth(*, organization_id: str = "org-acme") -> AuthContext: def _expected_organization_filter(auth_context: AuthContext): """Describe the exact trusted binding required for historical NULL rows.""" + assert hasattr(Workspace, "organization_id"), ( + "workspace_entities must persist organization_id before opaque workspace " + "claims can authorize organization-null documents" + ) + assert hasattr(Workspace, "owner_user_id"), ( + "organization workspace bindings must prove they are not personal-owner rows" + ) trusted_workspace_binding = exists( select(1) .select_from(Workspace) .where( Workspace.workspace_id == auth_context.workspace_id, Workspace.organization_id == auth_context.organization_id, + Workspace.owner_user_id.is_(None), ) ) return or_( @@ -59,6 +67,7 @@ def test_opaque_workspace_legacy_null_requires_correlated_organization_binding() assert "workspace_documents.workspace_id" in rendered assert "workspace_entities.workspace_id" in rendered assert "workspace_entities.organization_id" in rendered + assert "workspace_entities.owner_user_id" in rendered assert "IS NULL" in rendered.upper() assert "EXISTS" in rendered.upper() assert auth_context.workspace_id in params.values() @@ -83,6 +92,7 @@ def test_same_opaque_workspace_different_organization_cannot_share_null_branch() for rendered in (owner_rendered, other_rendered): assert "workspace_entities.workspace_id" in rendered assert "workspace_entities.organization_id" in rendered + assert "workspace_entities.owner_user_id" in rendered assert "EXISTS" in rendered.upper() assert "IS NULL" in rendered.upper()