fix: tree_shares.account_id must come from tree owner, not the actor
- 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 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
Revision ID: a05e1a1bea7c
|
||||||
Revises: 2a9056eddd90
|
Revises: 2a9056eddd90
|
||||||
@@ -21,13 +25,15 @@ def upgrade() -> None:
|
|||||||
['account_id'], ['id'], ondelete='CASCADE',
|
['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("""
|
op.execute("""
|
||||||
UPDATE tree_shares ts
|
UPDATE tree_shares ts
|
||||||
SET account_id = u.account_id
|
SET account_id = t.account_id
|
||||||
FROM users u
|
FROM trees t
|
||||||
WHERE ts.created_by = u.id
|
WHERE ts.tree_id = t.id
|
||||||
AND u.account_id IS NOT NULL
|
AND t.account_id IS NOT NULL
|
||||||
AND ts.account_id IS NULL
|
AND ts.account_id IS NULL
|
||||||
""")
|
""")
|
||||||
|
|
||||||
|
|||||||
@@ -1048,7 +1048,7 @@ async def create_tree_share(
|
|||||||
# Create share
|
# Create share
|
||||||
tree_share = TreeShare(
|
tree_share = TreeShare(
|
||||||
tree_id=tree.id,
|
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,
|
share_token=share_token,
|
||||||
created_by=current_user.id,
|
created_by=current_user.id,
|
||||||
allow_forking=share_data.allow_forking,
|
allow_forking=share_data.allow_forking,
|
||||||
|
|||||||
@@ -464,7 +464,6 @@ async def test_target_list_account_id_from_team_admin(test_db: AsyncSession):
|
|||||||
await test_db.flush()
|
await test_db.flush()
|
||||||
|
|
||||||
target_list = TargetList(
|
target_list = TargetList(
|
||||||
team_id=team.id,
|
|
||||||
account_id=account.id,
|
account_id=account.id,
|
||||||
created_by=user.id,
|
created_by=user.id,
|
||||||
name="Server Targets",
|
name="Server Targets",
|
||||||
|
|||||||
@@ -117,6 +117,7 @@ class TestTreeSharing:
|
|||||||
for i in range(3):
|
for i in range(3):
|
||||||
share = TreeShare(
|
share = TreeShare(
|
||||||
tree_id=sample_tree.id,
|
tree_id=sample_tree.id,
|
||||||
|
account_id=sample_tree.account_id,
|
||||||
share_token=f"token_{i}_" + "x" * 56,
|
share_token=f"token_{i}_" + "x" * 56,
|
||||||
created_by=sample_tree.author_id,
|
created_by=sample_tree.author_id,
|
||||||
allow_forking=i % 2 == 0
|
allow_forking=i % 2 == 0
|
||||||
@@ -162,6 +163,7 @@ class TestTreeSharing:
|
|||||||
# Create a share
|
# Create a share
|
||||||
share = TreeShare(
|
share = TreeShare(
|
||||||
tree_id=sample_tree.id,
|
tree_id=sample_tree.id,
|
||||||
|
account_id=sample_tree.account_id,
|
||||||
share_token="public_test_token" + "x" * 47,
|
share_token="public_test_token" + "x" * 47,
|
||||||
created_by=UUID(test_user["user_data"]["id"]),
|
created_by=UUID(test_user["user_data"]["id"]),
|
||||||
allow_forking=True
|
allow_forking=True
|
||||||
@@ -192,6 +194,7 @@ class TestTreeSharing:
|
|||||||
# Create expired share
|
# Create expired share
|
||||||
share = TreeShare(
|
share = TreeShare(
|
||||||
tree_id=sample_tree.id,
|
tree_id=sample_tree.id,
|
||||||
|
account_id=sample_tree.account_id,
|
||||||
share_token="expired_token" + "x" * 50,
|
share_token="expired_token" + "x" * 50,
|
||||||
created_by=UUID(test_user["user_data"]["id"]),
|
created_by=UUID(test_user["user_data"]["id"]),
|
||||||
allow_forking=True,
|
allow_forking=True,
|
||||||
@@ -209,6 +212,7 @@ class TestTreeSharing:
|
|||||||
from uuid import UUID
|
from uuid import UUID
|
||||||
share = TreeShare(
|
share = TreeShare(
|
||||||
tree_id=sample_tree.id,
|
tree_id=sample_tree.id,
|
||||||
|
account_id=sample_tree.account_id,
|
||||||
share_token="inactive_tree_token" + "x" * 44,
|
share_token="inactive_tree_token" + "x" * 44,
|
||||||
created_by=UUID(test_user["user_data"]["id"]),
|
created_by=UUID(test_user["user_data"]["id"]),
|
||||||
allow_forking=True
|
allow_forking=True
|
||||||
@@ -248,6 +252,37 @@ class TestTreeSharing:
|
|||||||
tokens.add(token)
|
tokens.add(token)
|
||||||
assert len(tokens) == 5
|
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
|
@pytest.mark.asyncio
|
||||||
async def test_migration_defaults_visibility_to_team(test_db):
|
async def test_migration_defaults_visibility_to_team(test_db):
|
||||||
|
|||||||
Reference in New Issue
Block a user