feat: tenant isolation Phase 3 — audit_logs, tree_shares, remaining RLS
P3-A: Add account_id to audit_logs model + migration (backfill via user_id → users.account_id). log_audit() gains optional account_id param with fallback SELECT to avoid churn across 40 call sites. P3-B: Add account_id to tree_shares model + migration (backfill via created_by → users.account_id). TreeShare constructor updated in trees.py. P3-C: Enable RLS on 6 remaining tables: step_ratings, step_usage_log, target_lists, session_shares, audit_logs, tree_shares. P3-D: Drop team_id from target_lists — endpoint, schema, and model now use account_id as the sole isolation key. P3-E: Append Phase 3 RLS isolation tests for all 6 tables. test_target_lists.py: fix cross-account test to use Account model (not Team) and set account_id on new User. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
chihlasm
@@ -708,3 +708,249 @@ async def test_step_library_account_a_can_see_account_b_public_steps(admin_conn,
|
||||
await admin_conn.execute(
|
||||
f"DELETE FROM step_library WHERE id = '{public_step_id}'"
|
||||
)
|
||||
|
||||
|
||||
# ===========================================================================
|
||||
# Phase 3 RLS isolation tests
|
||||
# Tables: step_ratings, step_usage_log, target_lists,
|
||||
# session_shares, audit_logs, tree_shares
|
||||
# ===========================================================================
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Helpers shared by Phase 3 fixtures
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
async def _get_user_b_id(admin_conn) -> str:
|
||||
row = await admin_conn.fetchrow(
|
||||
"SELECT id FROM users WHERE email = 'rls-user-b@example.com'"
|
||||
)
|
||||
return str(row["id"])
|
||||
|
||||
|
||||
async def _get_tree_b_id(admin_conn) -> str:
|
||||
row = await admin_conn.fetchrow(
|
||||
f"SELECT id FROM trees WHERE account_id = '{ACCOUNT_B_ID}' LIMIT 1"
|
||||
)
|
||||
return str(row["id"])
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# step_ratings
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_step_ratings_account_a_cannot_see_account_b(admin_conn, conn_a):
|
||||
"""Account A must not see step ratings belonging to Account B."""
|
||||
user_b_id = await _get_user_b_id(admin_conn)
|
||||
|
||||
# Need a step_library row as FK target
|
||||
step_id = str(uuid.uuid4())
|
||||
await admin_conn.execute(f"""
|
||||
INSERT INTO step_library (
|
||||
id, account_id, title, step_type, content,
|
||||
visibility, is_active, created_at, updated_at
|
||||
) VALUES (
|
||||
'{step_id}', '{ACCOUNT_B_ID}', 'Phase3 RLS Step', 'action',
|
||||
'{{}}'::jsonb, 'private', TRUE, NOW(), NOW()
|
||||
)
|
||||
""")
|
||||
|
||||
rating_id = str(uuid.uuid4())
|
||||
await admin_conn.execute(f"""
|
||||
INSERT INTO step_ratings (
|
||||
id, step_id, user_id, account_id, is_verified_use, is_visible,
|
||||
created_at, updated_at
|
||||
) VALUES (
|
||||
'{rating_id}', '{step_id}', '{user_b_id}', '{ACCOUNT_B_ID}',
|
||||
FALSE, TRUE, NOW(), NOW()
|
||||
)
|
||||
""")
|
||||
try:
|
||||
rows = await conn_a.fetch(
|
||||
f"SELECT id FROM step_ratings WHERE account_id = '{ACCOUNT_B_ID}'"
|
||||
)
|
||||
assert len(rows) == 0, "Account A should not see Account B step_ratings"
|
||||
finally:
|
||||
await admin_conn.execute(f"DELETE FROM step_ratings WHERE id = '{rating_id}'")
|
||||
await admin_conn.execute(f"DELETE FROM step_library WHERE id = '{step_id}'")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# step_usage_log
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_step_usage_log_account_a_cannot_see_account_b(admin_conn, conn_a):
|
||||
"""Account A must not see step usage logs belonging to Account B."""
|
||||
user_b_id = await _get_user_b_id(admin_conn)
|
||||
tree_b_id = await _get_tree_b_id(admin_conn)
|
||||
|
||||
step_id = str(uuid.uuid4())
|
||||
await admin_conn.execute(f"""
|
||||
INSERT INTO step_library (
|
||||
id, account_id, title, step_type, content,
|
||||
visibility, is_active, created_at, updated_at
|
||||
) VALUES (
|
||||
'{step_id}', '{ACCOUNT_B_ID}', 'Phase3 Usage Step', 'action',
|
||||
'{{}}'::jsonb, 'private', TRUE, NOW(), NOW()
|
||||
)
|
||||
""")
|
||||
|
||||
# Need a sessions row as FK for usage log
|
||||
session_id = str(uuid.uuid4())
|
||||
await admin_conn.execute(f"""
|
||||
INSERT INTO sessions (
|
||||
id, tree_id, user_id, account_id, tree_snapshot,
|
||||
path_taken, decisions, custom_steps, started_at
|
||||
) VALUES (
|
||||
'{session_id}', '{tree_b_id}', '{user_b_id}', '{ACCOUNT_B_ID}',
|
||||
'[]'::jsonb, '[]'::jsonb, '[]'::jsonb, '[]'::jsonb, NOW()
|
||||
)
|
||||
""")
|
||||
|
||||
log_id = str(uuid.uuid4())
|
||||
await admin_conn.execute(f"""
|
||||
INSERT INTO step_usage_log (
|
||||
id, step_id, user_id, account_id, session_id, used_at
|
||||
) VALUES (
|
||||
'{log_id}', '{step_id}', '{user_b_id}', '{ACCOUNT_B_ID}',
|
||||
'{session_id}', NOW()
|
||||
)
|
||||
""")
|
||||
try:
|
||||
rows = await conn_a.fetch(
|
||||
f"SELECT id FROM step_usage_log WHERE account_id = '{ACCOUNT_B_ID}'"
|
||||
)
|
||||
assert len(rows) == 0, "Account A should not see Account B step_usage_log"
|
||||
finally:
|
||||
await admin_conn.execute(f"DELETE FROM step_usage_log WHERE id = '{log_id}'")
|
||||
await admin_conn.execute(f"DELETE FROM sessions WHERE id = '{session_id}'")
|
||||
await admin_conn.execute(f"DELETE FROM step_library WHERE id = '{step_id}'")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# target_lists
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_target_lists_account_a_cannot_see_account_b(admin_conn, conn_a):
|
||||
"""Account A must not see target lists belonging to Account B."""
|
||||
user_b_id = await _get_user_b_id(admin_conn)
|
||||
|
||||
tl_id = str(uuid.uuid4())
|
||||
await admin_conn.execute(f"""
|
||||
INSERT INTO target_lists (
|
||||
id, account_id, created_by, name, targets, created_at, updated_at
|
||||
) VALUES (
|
||||
'{tl_id}', '{ACCOUNT_B_ID}', '{user_b_id}',
|
||||
'Phase3 RLS Target List', '[]'::jsonb, NOW(), NOW()
|
||||
)
|
||||
""")
|
||||
try:
|
||||
rows = await conn_a.fetch(
|
||||
f"SELECT id FROM target_lists WHERE account_id = '{ACCOUNT_B_ID}'"
|
||||
)
|
||||
assert len(rows) == 0, "Account A should not see Account B target_lists"
|
||||
finally:
|
||||
await admin_conn.execute(f"DELETE FROM target_lists WHERE id = '{tl_id}'")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# session_shares
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_session_shares_account_a_cannot_see_account_b(admin_conn, conn_a):
|
||||
"""Account A must not see session shares belonging to Account B."""
|
||||
user_b_id = await _get_user_b_id(admin_conn)
|
||||
tree_b_id = await _get_tree_b_id(admin_conn)
|
||||
|
||||
# Need a sessions row as FK
|
||||
session_id = str(uuid.uuid4())
|
||||
await admin_conn.execute(f"""
|
||||
INSERT INTO sessions (
|
||||
id, tree_id, user_id, account_id, tree_snapshot,
|
||||
path_taken, decisions, custom_steps, started_at
|
||||
) VALUES (
|
||||
'{session_id}', '{tree_b_id}', '{user_b_id}', '{ACCOUNT_B_ID}',
|
||||
'[]'::jsonb, '[]'::jsonb, '[]'::jsonb, '[]'::jsonb, NOW()
|
||||
)
|
||||
""")
|
||||
|
||||
share_id = str(uuid.uuid4())
|
||||
share_token = f"phase3-rls-test-{share_id[:8]}"
|
||||
await admin_conn.execute(f"""
|
||||
INSERT INTO session_shares (
|
||||
id, session_id, account_id, share_token, visibility,
|
||||
created_by, view_count, is_active, created_at, updated_at
|
||||
) VALUES (
|
||||
'{share_id}', '{session_id}', '{ACCOUNT_B_ID}',
|
||||
'{share_token}', 'account', '{user_b_id}',
|
||||
0, TRUE, NOW(), NOW()
|
||||
)
|
||||
""")
|
||||
try:
|
||||
rows = await conn_a.fetch(
|
||||
f"SELECT id FROM session_shares WHERE account_id = '{ACCOUNT_B_ID}'"
|
||||
)
|
||||
assert len(rows) == 0, "Account A should not see Account B session_shares"
|
||||
finally:
|
||||
await admin_conn.execute(f"DELETE FROM session_shares WHERE id = '{share_id}'")
|
||||
await admin_conn.execute(f"DELETE FROM sessions WHERE id = '{session_id}'")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# audit_logs
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_audit_logs_account_a_cannot_see_account_b(admin_conn, conn_a):
|
||||
"""Account A must not see audit logs belonging to Account B."""
|
||||
user_b_id = await _get_user_b_id(admin_conn)
|
||||
|
||||
log_id = str(uuid.uuid4())
|
||||
await admin_conn.execute(f"""
|
||||
INSERT INTO audit_logs (
|
||||
id, user_id, account_id, action, resource_type, created_at
|
||||
) VALUES (
|
||||
'{log_id}', '{user_b_id}', '{ACCOUNT_B_ID}',
|
||||
'test.action', 'test_resource', NOW()
|
||||
)
|
||||
""")
|
||||
try:
|
||||
rows = await conn_a.fetch(
|
||||
f"SELECT id FROM audit_logs WHERE account_id = '{ACCOUNT_B_ID}'"
|
||||
)
|
||||
assert len(rows) == 0, "Account A should not see Account B audit_logs"
|
||||
finally:
|
||||
await admin_conn.execute(f"DELETE FROM audit_logs WHERE id = '{log_id}'")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# tree_shares
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_tree_shares_account_a_cannot_see_account_b(admin_conn, conn_a):
|
||||
"""Account A must not see tree shares belonging to Account B."""
|
||||
user_b_id = await _get_user_b_id(admin_conn)
|
||||
tree_b_id = await _get_tree_b_id(admin_conn)
|
||||
|
||||
share_id = str(uuid.uuid4())
|
||||
share_token = f"phase3-tree-rls-{share_id[:8]}"
|
||||
await admin_conn.execute(f"""
|
||||
INSERT INTO tree_shares (
|
||||
id, tree_id, account_id, share_token, created_by,
|
||||
allow_forking, created_at
|
||||
) VALUES (
|
||||
'{share_id}', '{tree_b_id}', '{ACCOUNT_B_ID}',
|
||||
'{share_token}', '{user_b_id}', TRUE, NOW()
|
||||
)
|
||||
""")
|
||||
try:
|
||||
rows = await conn_a.fetch(
|
||||
f"SELECT id FROM tree_shares WHERE account_id = '{ACCOUNT_B_ID}'"
|
||||
)
|
||||
assert len(rows) == 0, "Account A should not see Account B tree_shares"
|
||||
finally:
|
||||
await admin_conn.execute(f"DELETE FROM tree_shares WHERE id = '{share_id}'")
|
||||
|
||||
@@ -3,37 +3,10 @@ import pytest
|
||||
from httpx import AsyncClient
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
from app.models.team import Team
|
||||
from app.models.user import User
|
||||
from sqlalchemy import select
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
async def auth_headers(client: AsyncClient, test_db: AsyncSession, test_user: dict):
|
||||
"""Override auth_headers to ensure the test user has a team_id assigned."""
|
||||
# Fetch the user from DB and assign a team
|
||||
result = await test_db.execute(select(User).where(User.email == test_user["email"]))
|
||||
user = result.scalar_one()
|
||||
|
||||
# Create a team and assign the user to it
|
||||
team = Team(name="Test Team")
|
||||
test_db.add(team)
|
||||
await test_db.flush()
|
||||
|
||||
user.team_id = team.id
|
||||
await test_db.commit()
|
||||
|
||||
# Re-login to get a fresh token
|
||||
login_data = {
|
||||
"email": test_user["email"],
|
||||
"password": test_user["password"],
|
||||
}
|
||||
resp = await client.post("/api/v1/auth/login/json", json=login_data)
|
||||
assert resp.status_code == 200
|
||||
token_data = resp.json()
|
||||
return {"Authorization": f"Bearer {token_data['access_token']}"}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_target_list(client: AsyncClient, auth_headers: dict):
|
||||
resp = await client.post(
|
||||
@@ -107,25 +80,28 @@ async def test_delete_target_list(client: AsyncClient, auth_headers: dict):
|
||||
assert get.status_code == 404
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cannot_access_other_teams_list(client: AsyncClient, auth_headers: dict, test_db):
|
||||
"""User from team B cannot access team A's list."""
|
||||
async def test_cannot_access_other_accounts_list(client: AsyncClient, auth_headers: dict, test_db):
|
||||
"""User from account B cannot access account A's target list."""
|
||||
import uuid
|
||||
from app.models.team import Team
|
||||
from app.models.account import Account
|
||||
from app.models.user import User
|
||||
from app.core.security import get_password_hash
|
||||
|
||||
# Create team A list using existing auth_headers
|
||||
# Create account A list using existing auth_headers
|
||||
create = await client.post(
|
||||
"/api/v1/target-lists/",
|
||||
json={"name": "Team A List", "targets": [{"label": "SRV-A"}]},
|
||||
json={"name": "Account A List", "targets": [{"label": "SRV-A"}]},
|
||||
headers=auth_headers,
|
||||
)
|
||||
assert create.status_code == 201
|
||||
list_id = create.json()["id"]
|
||||
|
||||
# Create a separate team B with its own user
|
||||
team_b = Team(name=f"Team B {uuid.uuid4()}")
|
||||
test_db.add(team_b)
|
||||
# Create a separate account B with its own user
|
||||
account_b = Account(
|
||||
name=f"Account B {uuid.uuid4()}",
|
||||
display_code=f"AB{str(uuid.uuid4())[:6].upper()}",
|
||||
)
|
||||
test_db.add(account_b)
|
||||
await test_db.flush()
|
||||
|
||||
user_b = User(
|
||||
@@ -133,11 +109,13 @@ async def test_cannot_access_other_teams_list(client: AsyncClient, auth_headers:
|
||||
password_hash=get_password_hash("password123"),
|
||||
name="User B",
|
||||
is_active=True,
|
||||
team_id=team_b.id,
|
||||
account_id=account_b.id,
|
||||
account_role="engineer",
|
||||
role="engineer",
|
||||
)
|
||||
test_db.add(user_b)
|
||||
await test_db.flush()
|
||||
await test_db.commit()
|
||||
|
||||
# Get auth token for user B
|
||||
login = await client.post(
|
||||
@@ -148,6 +126,6 @@ async def test_cannot_access_other_teams_list(client: AsyncClient, auth_headers:
|
||||
token_b = login.json()["access_token"]
|
||||
headers_b = {"Authorization": f"Bearer {token_b}"}
|
||||
|
||||
# Team B cannot access Team A's list
|
||||
# Account B cannot access Account A's list
|
||||
resp = await client.get(f"/api/v1/target-lists/{list_id}", headers=headers_b)
|
||||
assert resp.status_code == 404
|
||||
|
||||
Reference in New Issue
Block a user