-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Branch utilized view columns ACL #24589
base: master
Are you sure you want to change the base?
Branch utilized view columns ACL #24589
Conversation
Analyzer analyzer = createAnalyzer(session, metadata, WarningCollector.NOOP); | ||
Statement statement = SQL_PARSER.createStatement(query); | ||
Analysis analysis = analyzer.analyze(statement); | ||
assertEquals(analysis.getAccessControlReferences().getTableColumnAndSubfieldReferencesForAccessControl().values().toString(), "[{tpch.s1.v6=[a, c], tpch.s1.v7=[y]}, {tpch.s1.t1=[]}, {tpch.s1.t13=[]}]"); |
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.
This test case passes, so it shows that ACL is not being run on columns v6.b and v7.x
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.
oh i think this is the problem. The tables are getting empty lists, but our permissions service checks ALL columns when it gets an empty list of columns. The table access controls should have the columns that are used from the table (via the view).
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.
Ok, I will see if adding an equals method to AccessControlContext will change the results from empty list.
So the correct behavior would be something like:
"[{tpch.s1.v6=[a, c], tpch.s1.v7=[y]}, {tpch.s1.t1=[a, c]}, {tpch.s1.t13=[y]}]"
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.
yeah, that's correct
Optional.empty(), | ||
true)); |
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.
Used runAsInvoker as true
Description
Not able to reproduce the bug in this tasks using automated testing: T214103298
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
If release note is NOT required, use: