Ensure first table matched rule in access control #12204
Conversation
guyco33
changed the title
guyco33
force-pushed
the
add_masking_tests
branch
3 times, most recently
from
57f1f7f to
212bf47
Compare
guyco33
changed the title
|
@ksobolew Can you please take a look at this fix ? |
There was a problem hiding this comment.
Sorry, my bad. Streams of Optionals are tricky, and I was not really sure what the expected semantics are (initially I wanted to return all matching rules). The fix looks good, but as @Praveen2112 says, this should also be applied to connector-level FileBasedAccessControl.
|
|
||
| assertEquals( | ||
| accessControl.getRowFilters( | ||
| userGroup1Group3, |
There was a problem hiding this comment.
This is interesting edge case. The user belongs to group1 and group3, but the table only has a filter for group3. I'm not quite sure what the correct semantics should be here.
For instance, the first entry may be about allowing users in group1 (e.g., employees in some department) to access a table, but the second entry might be about limiting what users in group3 can see (e.g., employees in some specific geography). In that case, it should use all filters that are present.
Let me think more about it.
There was a problem hiding this comment.
I think that in all cases we should pick the first matching rule. This way we can control the way we want to limit access.
For example, in cases that we want to implement a more restrictive approach, we will place the table rules with filters and masks before the rules without them.
There was a problem hiding this comment.
I personally would lean towards returning all matching rules. But I'm not very familiar with file-based AC and its use cases so I'm not sure what's actually expected.
I think that in all cases we should pick the first matching rule. This way we can control the way we want to limit access.
But doesn't it mean that if there is a second matching rule, it will never actually be returned and is entirely redundant?
There was a problem hiding this comment.
Safe logic for returning all matching rules must take into account the rules without filters and maskings.
Rules order is important, so possible solution might be retrieving all matched filters / maskings rules until a rule without filter / masks is matched.
Anyway it's a subject for different PR
although it doesn't have masked columns or table filters
|
Merged !! Thanks for fixing this. |
although it doesn't have masked columns or table filters
Description
Fix regression for PR #11654
Related issues, pull requests, and links
Fixes #12203
Documentation
(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.
Release notes
( ) No release notes entries required.
( ) Release notes entries required with the following suggested text: