From 893b8a500899d3aa1d69c110e93590cd02461103 Mon Sep 17 00:00:00 2001 From: chihlasm Date: Sat, 11 Apr 2026 05:17:25 +0000 Subject: [PATCH] fix: tree_shares.account_id must come from tree owner, not the actor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - trees.py: change account_id=current_user.account_id → account_id=tree.account_id so super-admin cross-account shares land in the tree's tenant where RLS will see them. - migration a05e1a1bea7c: fix backfill to join tree_shares → trees instead of tree_shares → users(created_by). Same logic: historical shares belong to the tree's tenant. - test_tree_sharing.py: add test_share_account_id_matches_tree_not_actor to assert share.account_id == tree.account_id after POST /share; also add missing account_id to all direct TreeShare(...) constructors in existing tests. - test_phase1_migrations.py: remove team_id= from TargetList constructor (column dropped in Phase 3). Co-Authored-By: Claude Sonnet 4.6 --- ...a05e1a1bea7c_add_account_id_tree_shares.py | 18 ++++++---- backend/app/api/endpoints/trees.py | 2 +- backend/tests/test_phase1_migrations.py | 1 - backend/tests/test_tree_sharing.py | 35 +++++++++++++++++++ 4 files changed, 48 insertions(+), 8 deletions(-) diff --git a/backend/alembic/versions/a05e1a1bea7c_add_account_id_tree_shares.py b/backend/alembic/versions/a05e1a1bea7c_add_account_id_tree_shares.py index 8d69aec3..f8216ace 100644 --- a/backend/alembic/versions/a05e1a1bea7c_add_account_id_tree_shares.py +++ b/backend/alembic/versions/a05e1a1bea7c_add_account_id_tree_shares.py @@ -1,4 +1,8 @@ -"""Add account_id to tree_shares and backfill via created_by user. +"""Add account_id to tree_shares and backfill via tree owner's account. + +The share belongs to the tree's tenant, not the actor who created it. +A super admin in account A can share a tree owned by account B; that share +must land in account B so account B's RLS filter sees it. Revision ID: a05e1a1bea7c Revises: 2a9056eddd90 @@ -21,13 +25,15 @@ def upgrade() -> None: ['account_id'], ['id'], ondelete='CASCADE', ) - # Backfill: derive from the creating user's account + # Backfill: derive from the tree's account, not the creator's account. + # A share lives in the same tenant as its tree so that the tree owner's + # RLS context covers their own shares regardless of who created them. op.execute(""" UPDATE tree_shares ts - SET account_id = u.account_id - FROM users u - WHERE ts.created_by = u.id - AND u.account_id IS NOT NULL + SET account_id = t.account_id + FROM trees t + WHERE ts.tree_id = t.id + AND t.account_id IS NOT NULL AND ts.account_id IS NULL """) diff --git a/backend/app/api/endpoints/trees.py b/backend/app/api/endpoints/trees.py index fe5246c7..6f7c0853 100644 --- a/backend/app/api/endpoints/trees.py +++ b/backend/app/api/endpoints/trees.py @@ -1048,7 +1048,7 @@ async def create_tree_share( # Create share tree_share = TreeShare( tree_id=tree.id, - account_id=current_user.account_id, + account_id=tree.account_id, # share belongs to the tree's tenant, not the actor share_token=share_token, created_by=current_user.id, allow_forking=share_data.allow_forking, diff --git a/backend/tests/test_phase1_migrations.py b/backend/tests/test_phase1_migrations.py index eefeba17..59e54820 100644 --- a/backend/tests/test_phase1_migrations.py +++ b/backend/tests/test_phase1_migrations.py @@ -464,7 +464,6 @@ async def test_target_list_account_id_from_team_admin(test_db: AsyncSession): await test_db.flush() target_list = TargetList( - team_id=team.id, account_id=account.id, created_by=user.id, name="Server Targets", diff --git a/backend/tests/test_tree_sharing.py b/backend/tests/test_tree_sharing.py index ea05c50f..a9adee54 100644 --- a/backend/tests/test_tree_sharing.py +++ b/backend/tests/test_tree_sharing.py @@ -117,6 +117,7 @@ class TestTreeSharing: for i in range(3): share = TreeShare( tree_id=sample_tree.id, + account_id=sample_tree.account_id, share_token=f"token_{i}_" + "x" * 56, created_by=sample_tree.author_id, allow_forking=i % 2 == 0 @@ -162,6 +163,7 @@ class TestTreeSharing: # Create a share share = TreeShare( tree_id=sample_tree.id, + account_id=sample_tree.account_id, share_token="public_test_token" + "x" * 47, created_by=UUID(test_user["user_data"]["id"]), allow_forking=True @@ -192,6 +194,7 @@ class TestTreeSharing: # Create expired share share = TreeShare( tree_id=sample_tree.id, + account_id=sample_tree.account_id, share_token="expired_token" + "x" * 50, created_by=UUID(test_user["user_data"]["id"]), allow_forking=True, @@ -209,6 +212,7 @@ class TestTreeSharing: from uuid import UUID share = TreeShare( tree_id=sample_tree.id, + account_id=sample_tree.account_id, share_token="inactive_tree_token" + "x" * 44, created_by=UUID(test_user["user_data"]["id"]), allow_forking=True @@ -248,6 +252,37 @@ class TestTreeSharing: tokens.add(token) assert len(tokens) == 5 + async def test_share_account_id_matches_tree_not_actor( + self, client: AsyncClient, sample_tree, auth_headers, test_db + ): + """Share account_id must equal tree.account_id, not the actor's account_id. + + A super admin in a different account can share any tree. The resulting + TreeShare row must live in the tree-owner's account so that the tree + owner's RLS context covers it. If account_id were derived from the + actor instead, the share would vanish from the tree owner's view once + RLS is enabled. + """ + from uuid import UUID + from sqlalchemy import select + + response = await client.post( + f"/api/v1/trees/{sample_tree.id}/share", + json={"allow_forking": True}, + headers=auth_headers, + ) + assert response.status_code == 201 + share_token = response.json()["share_token"] + + result = await test_db.execute( + select(TreeShare).where(TreeShare.share_token == share_token) + ) + share = result.scalar_one() + assert share.account_id == sample_tree.account_id, ( + "TreeShare.account_id must equal tree.account_id, not the actor's account. " + "Shares must live in the tree owner's tenant for RLS to cover them." + ) + @pytest.mark.asyncio async def test_migration_defaults_visibility_to_team(test_db):