Fix case-sensitivity issue with views and column masks #24055
Conversation
| tableColumnReferences.put(field.getOriginTable().get(), field.getOriginColumnName().get()); | ||
| tableColumnReferences.put(field.getOriginTable().get(), field.getOriginColumnName().get().toLowerCase(ENGLISH)); |
There was a problem hiding this comment.
@martint I sprinkled those toLowerCase in enough places to fix this particular issue, but I'm not entirely sure if these are the right places. I think the convention is to keep the original case in the AST and only convert it as needed in the analyzer, even though it would probably be easier to fix all issues like this by storing converted names in AST. WDYT?
There was a problem hiding this comment.
Identifiers need to be resolved according to standard sql rules. It differs for quoted vs non-quoted identifiers.
Currently, there are some inconsistencies in how they are being handled in various places, but we have a long standing issue to fix them. See #17
There was a problem hiding this comment.
Understood @martint. The question is, though, how far this should extend to interfaces like access control. This is especially relevant to what I call "negative grants" - column masks, row filters and DENY policies. If an access control declares a column mask on column foo, it should apply to foo and Foo but not to "Foo", if I understand correctly. One could argue that this is unexpected and may cause information leaks. To counter that someone could say that if the user uses case-sensitive names then they know what they are doing. But to counter that, some access controls coerce all names to lower case, assuming that they will be matched case-insensitively. So I am not sure what to do next.
I believe this needs to be fixed somehow right now, because at this moment this enables accidental information leaks, like described above, but with regular identifiers.
There was a problem hiding this comment.
I think we can fix this by associating masks to the corresponding Field in StatementAnalyzer and Analysis, instead of doing it by name. Then, in RelationPlanner, the lookup can be done directly by Field instead of relying on the name having the right case.
There was a problem hiding this comment.
@martint I pushed a prototype of a solution which (I think) implements your suggestion. There's no way to do efficient lookups of Fields, though, so in some places we need to do a linear scan of the list of masks in order to find a match.
ksobolew
changed the title
ksobolew
force-pushed
the
kudi/column-mask-case-sensitivity
branch
from
230baaa to
0ede278
Compare
The table column reference was registered incorectly with the original case taken from the view definition. It then failed to match the column schema returned from `SystemAccessControl` and the mask was not applied. Instead of sprinkling `toLowerCase()` here and there, we will associate the original `Field` with the column mask and use `Field#canResove` to do the matching. The problem with this is that there's no way to do efficient lookups by name in a case-insensitive way, so we have to iterate the list of `Field`-`Expression` pairs to find a match. Fixes trinodb#24054.
ksobolew
force-pushed
the
kudi/column-mask-case-sensitivity
branch
from
0ede278 to
32dd720
Compare
Description
The table column reference was registered incorrectly with the original case taken from the view definition. It then failed to match the column schema returned from
SystemAccessControland the mask was not applied.Instead of sprinkling
toLowerCase()here and there, we will associate the originalFieldwith the column mask and useField#canResoveto do the matching. The problem with this is that there's no way to do efficient lookups by name in a case-insensitive way, so we have to iterate the list ofField-Expressionpairs to find a match.Fixes #24054.
Additional context and related issues
I wonder how many more issues like that will we find...
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: