-
Notifications
You must be signed in to change notification settings - Fork 329
feat(db): add PostgreSQL Row-Level Security for multi-tenancy #1957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 10 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="tests/unit/test_rls.py">
<violation number="1" location="tests/unit/test_rls.py:6">
P2: Unit tests in `tests/unit` are expected to be integration-style without mocks. This file relies on AsyncMock/MagicMock/patch, which contradicts the repo’s testing guidelines and reduces confidence in RLS behavior. Consider using real DB/session fixtures or move these tests under a mocks-allowed suite.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa3a4852da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Use SET LOCAL to scope to current transaction | ||
| # This is safe with connection pooling since the settings are cleared on transaction end | ||
| await session.execute( | ||
| text(f"SET LOCAL {RLS_VAR_ORG_ID} = :org_id"), | ||
| {"org_id": org_id_str}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reapply RLS context each transaction or avoid SET LOCAL
The RLS context is set with SET LOCAL, which only persists for the current transaction. get_async_session() calls this once when the session is created, so any later commit()/rollback() in the same session starts a new transaction without the RLS variables. At that point current_setting(..., true) is NULL and your policies will filter out all rows, breaking common flows like “insert → commit → refresh/query” in the same session. Consider using SET with connection checkin/checkout cleanup or reapplying SET LOCAL on every transaction begin.
Useful? React with 👍 / 👎.
| """ | ||
| async with AsyncSession(get_async_engine(), expire_on_commit=False) as session: | ||
| yield session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bypass session doesn’t actually bypass RLS
get_async_session_bypass_rls() yields a session without setting any RLS variables. When RLS is enabled, your policies only bypass if current_setting(...) equals the all‑zero UUID; a missing setting evaluates to NULL and blocks all rows. Any admin or background job using this helper will see empty results instead of unrestricted access. This should set the bypass values (e.g., call set_rls_context(..., None) or a dedicated bypass helper).
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
Code reviewFound 2 issues:
tracecat/tests/unit/test_rls.py Lines 275 to 277 in a6068a7
Multiple test methods contain
The bypass function and its context manager are defined but have zero imports or usages elsewhere. This appears to be dead code that should either be removed or its intended use should be implemented. Generated with Claude Code (https://claude.ai/code)
|
Add database-level multi-tenancy isolation using PostgreSQL native RLS on organization_id and workspace_id columns. Key changes: - Add RLS_ENABLED feature flag for controlled rollout - Add TracecatRLSViolationError exception for access violations - Create tracecat/db/rls.py with context management functions - Modify get_async_session() to automatically set RLS context from ctx_role - Add get_async_session_bypass_rls() for system operations - Create Alembic migration enabling RLS on all tenant-scoped tables - Add RLS audit logging module for security monitoring - Add unit and integration tests for RLS functionality RLS policies: - Workspace-scoped tables (26): filtered by workspace_id - Organization-scoped tables (2): filtered by organization_id - Workspace table: supports both org and workspace-level access Context is set via PostgreSQL session variables using SET LOCAL, which is connection-pool safe (scoped to transaction). Enable with: TRACECAT__FEATURE_FLAGS=rls-enabled
Summary
organization_idandworkspace_idcolumnsTRACECAT__FEATURE_FLAGS=rls-enabledChanges
RLS_ENABLEDfeature flag for controlled rolloutTracecatRLSViolationErrorexception for access violationstracecat/db/rls.pywith RLS context management functionsget_async_session()to automatically set RLS context fromctx_roleget_async_session_bypass_rls()for system operations (migrations, admin tasks)RLS Policies
workspace_idorganization_idTest plan
uv run pytest tests/unit/test_rls.py -vTRACECAT__FEATURE_FLAGS=rls-enabled just cluster up -d --seeduv run pytest tests/unit/test_rls_policies.py -vSELECT * FROM pg_policies WHERE tablename = 'workflow';🤖 Generated with Claude Code
Summary by cubic
Adds PostgreSQL Row-Level Security to enforce tenant isolation at the database level. Sessions set org/workspace context so queries are filtered by policies; rollout is gated by a feature flag.
New Features
Migration
Written for commit b14dc73. Summary will update on new commits.