Skip to content

Use less restrictive column access checks when using transform and cardinality functions#18840

Merged
pranjalssh merged 2 commits intoprestodb:masterfrom
pranjalssh:transform-acl
Jan 19, 2023
Merged

Use less restrictive column access checks when using transform and cardinality functions#18840
pranjalssh merged 2 commits intoprestodb:masterfrom
pranjalssh:transform-acl

Conversation

@pranjalssh
Copy link
Contributor

@pranjalssh pranjalssh commented Dec 23, 2022

Commit 1:

When doing subfield access checks in sql like transform(a, col -> col.x).

We previously checked for permission of column a.
After this PR, we will check for permission of column a.x.

To do this, we need to do 2 things:

  1. Parse lambda functions, and map lambda argument col to table column a for access checks
  2. Don't do access check for first argument to transform, a

This PR does both for them.

These cases are hardcoded just for the transform function. Adding other functions to these checks is easier, as demonstrated in commit 2, where we skip the check completely for cardinality function.

== RELEASE NOTES ==

General Changes
* Use less restrictive column access checks when using `transform` and `cardinality` functions

@pranjalssh pranjalssh requested a review from a team as a code owner December 23, 2022 04:56
@pranjalssh pranjalssh changed the title Transform acl Loosen Subfield access checks for transform and cardinality Dec 23, 2022
@pranjalssh pranjalssh changed the title Loosen Subfield access checks for transform and cardinality Use less restrictive column access checks when using transform and cardinality functions Jan 3, 2023
@amitkdutta
Copy link
Contributor

[ RUN ] TaskManagerTest.outOfQueryUserMemory
*** Aborted at 1671785665 (Unix time, try 'date -d @1671785665') ***
*** Signal 11 (SIGSEGV) (0x18) received by PID 3865 (pthread TID 0x7f5d3e020780) (linux TID 3865) (code: address not mapped to object), stack trace: ***
(error retrieving stack trace)

This is fixed in #18842

Copy link
Contributor

@rschlussel rschlussel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add access control tests too (create a test access control with some column restrictions and confirm you don't get an exception if accessing allowed subfields or using cardinality function.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? why not use recursion to go into all of them?

Copy link
Contributor Author

@pranjalssh pranjalssh Jan 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is moved from ExpressionAnalyzer as well. This function is called once for each dereference, when multiple dereferences are nested. So, we have this check that only looks at the dereference chain once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think all the different cases here are a bit confusing. Needs some comments, but I think it would be clearer if written as a visitor instead of a while loop with each case determining whether to continue the loop. It would also be easy to introduce an infinite loop in this logic(continue gets called without the child nod getting updated to something that would break the loop eventually)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to modify this logic in this PR. This is an existing logic moved from ExpressionAnalyzer to here, which is already tested. I can put up a todo to refactor it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it. I guess it's okay since the code already existed. would be good to refactor though as a follow up.

@pranjalssh pranjalssh force-pushed the transform-acl branch 2 times, most recently from c653678 to 19221f3 Compare January 17, 2023 08:07
@pranjalssh pranjalssh requested a review from rschlussel January 17, 2023 08:08
@pranjalssh pranjalssh merged commit e90dd3b into prestodb:master Jan 19, 2023
@wanglinsong wanglinsong mentioned this pull request Feb 25, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants