fix: enforce RLS for table owner with FORCE ROW LEVEL SECURITY#46
fix: enforce RLS for table owner with FORCE ROW LEVEL SECURITY#46
Conversation
PostgreSQL RLS policies don't apply to the table owner by default. Since Django typically connects as the user who created the tables, RLS was being silently bypassed. This fix adds `FORCE ROW LEVEL SECURITY` after `ENABLE ROW LEVEL SECURITY` to ensure policies are enforced regardless of the database user. Also fixes test models and tests to: - Use `current_setting(..., true)` with missing_ok=true in CustomPolicy expressions to handle cases where context isn't set - Properly set RLS context using RLSContext before creating/querying RLS-protected data Fixes: forum issue about RLS not working in admin panel Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @kdpisda, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly strengthens Row Level Security (RLS) enforcement by ensuring that RLS policies apply universally, including to the table owner, which was a critical gap. It also refines how RLS context is managed within policy expressions, making them more resilient to unset variables. The changes are thoroughly validated with an updated test suite that now accurately reflects RLS behavior, providing a more reliable and secure data access control mechanism. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a critical security issue by enforcing Row Level Security (RLS) for table owners using FORCE ROW LEVEL SECURITY. The implementation is solid, and the accompanying test updates are excellent. The tests have been significantly improved to use RLSContext for setting security contexts, making them more realistic and robust. The policy definitions in the tests have also been hardened against missing context variables. Overall, this is a high-quality contribution. I've included a few minor suggestions to further improve code maintainability and test clarity.
| # Models don't have _state, use default connection | ||
| db_alias = 'default' | ||
| db_alias = "default" | ||
| connection = connections[db_alias] |
There was a problem hiding this comment.
This block of code to get the default database connection is duplicated in the disable_rls method. To follow the DRY (Don't Repeat Yourself) principle and improve maintainability, consider extracting it into a private helper class method.
For example:
@classmethod
def _get_db_connection(cls):
"""Get the default database connection."""
from django.db import connections
# Models don't have _state, use default connection
return connections["default"]This new method could then be called from both enable_rls and disable_rls.
| with RLSContext(user_id=self.u1.id): | ||
| assert UserOwnedModel.objects.count() == 1 | ||
| assert UserOwnedModel.objects.first().title == "U1 Data" | ||
|
|
||
| # User 2 should only see their own data | ||
| with RLSContext(user_id=self.u2.id): | ||
| assert UserOwnedModel.objects.count() == 1 | ||
| assert UserOwnedModel.objects.first().title == "U2 Data" |
There was a problem hiding this comment.
The assertions here are making two separate database queries (.count() and .first()) for each user context. A more idiomatic and efficient way to assert that exactly one object matches is to use Model.objects.get(). It will raise an exception if zero or more than one object is found, which is the desired behavior for this test, and it combines the check and fetch into one operation.
This pattern can be applied to other tests in this file as well (e.g., test_multi_tenant_isolation_concept, test_role_hierarchy).
| with RLSContext(user_id=self.u1.id): | |
| assert UserOwnedModel.objects.count() == 1 | |
| assert UserOwnedModel.objects.first().title == "U1 Data" | |
| # User 2 should only see their own data | |
| with RLSContext(user_id=self.u2.id): | |
| assert UserOwnedModel.objects.count() == 1 | |
| assert UserOwnedModel.objects.first().title == "U2 Data" | |
| with RLSContext(user_id=self.u1.id): | |
| obj = UserOwnedModel.objects.get() | |
| assert obj.title == "U1 Data" | |
| # User 2 should only see their own data | |
| with RLSContext(user_id=self.u2.id): | |
| obj = UserOwnedModel.objects.get() | |
| assert obj.title == "U2 Data" |
| try: | ||
| with transaction.atomic(): | ||
| UserOwnedModel( | ||
| id=self.hidden_id, title="Clash", content="x", owner=self.u1 | ||
| ).save(force_insert=True) | ||
| # If we succeed, ID collision didn't happen or we overwrote. | ||
| assert False, "Should have raised IntegrityError" | ||
| except IntegrityError: | ||
| # Confirms that constraints fire regardless of RLS visibility. | ||
| # This IS a side-channel but unavoidable in standard SQL | ||
| # without logic changes (e.g. random UUIDs). | ||
| pass |
There was a problem hiding this comment.
The try...except...pass block with assert False can be simplified by using the self.assertRaises context manager, which is the standard and more declarative way to test for exceptions in unittest.TestCase.
with self.assertRaises(IntegrityError):
# This test confirms that constraints fire regardless of RLS visibility.
# This IS a known side-channel leak in Postgres RLS.
with transaction.atomic():
UserOwnedModel(
id=self.hidden_id, title="Clash", content="x", owner=self.u1
).save(force_insert=True)
Summary
FORCE ROW LEVEL SECURITYafterENABLE ROW LEVEL SECURITYto ensure RLS policies are enforced even for the table ownercurrent_setting(..., true)withmissing_ok=truein CustomPolicy expressionsRLSContextbefore creating/querying RLS-protected dataProblem
PostgreSQL RLS policies don't apply to the table owner by default. Since Django typically connects as the user who created the tables (the owner), RLS was being silently bypassed. This caused the issue reported on the forum where RLS wasn't working in the admin panel.
Solution
Added
schema_editor.force_rls(cls)call inenable_rls()method to executeALTER TABLE ... FORCE ROW LEVEL SECURITY, which ensures policies are enforced regardless of the database user.Test plan
RLSContextto set context before operations on RLS-protected tables🤖 Generated with Claude Code