Evaluate RLS joins with the owner identity#2832
Open
egormanga wants to merge 2 commits intoclockworklabs:masterfrom
Open
Evaluate RLS joins with the owner identity#2832egormanga wants to merge 2 commits intoclockworklabs:masterfrom
egormanga wants to merge 2 commits intoclockworklabs:masterfrom
Conversation
crates/expr/src/rls.rs
Outdated
Comment on lines
+272
to
+273
| // Use the owner identity to evaluate the RLS query joins | ||
| &AuthCtx::for_current(auth.owner), |
Collaborator
There was a problem hiding this comment.
@egormanga this means the expression will be evaluated as though it were from the owner identity, not the caller identity. And the owner bypasses RLS rules, meaning their access is not restricted. So this would result in non-owner identities gaining access to data they shouldn't.
Contributor
Author
There was a problem hiding this comment.
I don't think so. According to my testing, this only applies to the RLS rule evaluation, not to the original user query.
I might be wrong, but if I wouldn't get both positive test results for my case and negative for restricted data access (e.g. the access table in my example), I wouldn't've submitted this PR :)
This comment was marked as resolved.
This comment was marked as resolved.
Additionally, this seems to allow for some extra query optimizations (see tests).
2df7580 to
d1ea89b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Changes
Fix for a bigger issue brought up in #2830 (comment).
Additionally, this seems to allow for some extra query optimizations (see tests).
API and ABI breaking changes
None.
Expected complexity level and risk
2:
While the fix seems pretty straightforward, it somehow affected query optimization logic so it might go a bit deeper than that.
Testing
test_addAccessSELECT * FROM test;Result:
[]→[0xIDENTITY]