Skip to content

Add checkCanGrantExecuteFunctionPrivilege to ConnectorAccessControl#14191

Merged
kokosing merged 1 commit intotrinodb:masterfrom
huberty89:hubert/add-can-grant-execute-function-to-connector-access-control
Oct 8, 2022
Merged

Add checkCanGrantExecuteFunctionPrivilege to ConnectorAccessControl#14191
kokosing merged 1 commit intotrinodb:masterfrom
huberty89:hubert/add-can-grant-execute-function-to-connector-access-control

Conversation

@huberty89
Copy link
Copy Markdown
Contributor

@huberty89 huberty89 commented Sep 19, 2022

Description

Follow-up to #13944
I will needed this to complete #13713

Release notes

( ) This is not user-visible 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:

# SPI
* Add `ConnectorAccessControl.checkCanGrantExecuteFunctionPrivilege` overload that needs to be implemented
  to allow views that use table functions. ({issue}`13944`)

@huberty89
Copy link
Copy Markdown
Contributor Author

I would like to add a test for that, but not sure where should I start.

Copy link
Copy Markdown
Contributor

@ksobolew ksobolew left a comment

Choose a reason for hiding this comment

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

Makes sense, but I wonder... How much are we going to phase out connector access control in favor of global access control? (I know, this is a bigger question :) )

Copy link
Copy Markdown
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

You can write a test against sql-standard access control, to see it is rejecting table functions by default.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am under impression that this throw is not needed. However, it leaves no question that is a dead code.

Or maybe you can use return switch here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wanted to say the same, but the return value is void so it won't work.

@findepi
Copy link
Copy Markdown
Member

findepi commented Sep 19, 2022

lgtm % let's have a test

@kokosing
Copy link
Copy Markdown
Member

You can use io.trino.tests.product.hive.TestSqlStandardAccessControlChecks for testing

@huberty89 huberty89 force-pushed the hubert/add-can-grant-execute-function-to-connector-access-control branch from 8035179 to 03bc954 Compare September 27, 2022 14:44
@kokosing
Copy link
Copy Markdown
Member

LGTM % test

@huberty89 huberty89 force-pushed the hubert/add-can-grant-execute-function-to-connector-access-control branch from 03bc954 to 9838b12 Compare October 6, 2022 11:24
@huberty89
Copy link
Copy Markdown
Contributor Author

I rebased and add few tests in TestAccessControlManager

@kokosing
Copy link
Copy Markdown
Member

kokosing commented Oct 6, 2022

Maven checks are failiing

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

May be { not needed?

What about testing checkCanExecuteFunction?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

May be { not needed?

execute is overloaded and in-lining cause compilation error

What about testing checkCanExecuteFunction?

This is already checked in other tests

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could you please verify that you have access before setting DenyConnectorAccessControl?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is checked in testAllowExecuteTableFunction test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, I'd prefer to have these tests duplicated, than not to have it here.
It would underline that just before setting DenyConnectorAccessControl it was possible to select from that connector. Actually that's @kokosing's style.

@huberty89 huberty89 force-pushed the hubert/add-can-grant-execute-function-to-connector-access-control branch from 9838b12 to befc8b4 Compare October 6, 2022 15:28
@huberty89
Copy link
Copy Markdown
Contributor Author

@findepi @dain Would you like to take a look? I'm adding new method to SPI so I want to be sure if that's ok

@kokosing kokosing merged commit 4302296 into trinodb:master Oct 8, 2022
@kokosing
Copy link
Copy Markdown
Member

kokosing commented Oct 8, 2022

Merged, thanks!

@github-actions github-actions bot added this to the 400 milestone Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants