Add setFunctionOwner to SystemSecurityMetadata #24696
Conversation
homar
force-pushed
the
homar/add_setFunctionOwner
branch
3 times, most recently
from
ca57a41 to
f3b4bf0
Compare
homar
force-pushed
the
homar/add_setFunctionOwner
branch
from
f3b4bf0 to
ceca747
Compare
| metadata.createLanguageFunction(session.toConnectorSession(catalogHandle), name.asSchemaFunctionName(), function, replace); | ||
| SchemaFunctionName schemaFunctionName = name.asSchemaFunctionName(); | ||
| if (catalogMetadata.getSecurityManagement() == SYSTEM) { | ||
| systemSecurityMetadata.setFunctionOwner( | ||
| session, | ||
| new CatalogSchemaFunctionName(catalogHandle.getCatalogName().toString(), schemaFunctionName), | ||
| new TrinoPrincipal(PrincipalType.USER, session.getUser())); | ||
| } | ||
|
|
||
| metadata.createLanguageFunction(session.toConnectorSession(catalogHandle), schemaFunctionName, function, replace); |
There was a problem hiding this comment.
With views it's done differently - there's a tableCreated callback function, which is (presumably) used to update the ownership. The setXOwner methods are called in response to ALTER X SET AUTHORIZATION commands, instead (which we don't have here). Note that the actual function is created by the connector metadata, which is a factor too.
| @@ -552,6 +552,44 @@ public void testJoinBaseTableWithView() | |||
| assertAccessAllowed(viewOwnerSession, "DROP VIEW " + viewName); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testFunctionOwners() | |||
There was a problem hiding this comment.
This test is fine, but it could take some inspiration for the existing tests for views. For example, those tests are also checking things like: the access to the underlying resources (here: my_test_function_inner) is only checked on query/execute, not during creation. Though it could be different for functions.
| String functionOwner = "function_owner"; | ||
| TrinoPrincipal functionOwnerPrincipal = new TrinoPrincipal(USER, functionOwner); | ||
|
|
||
| systemSecurityMetadata.grantRoles(getSession(), Set.of("function_owner_role"), Set.of(functionOwnerPrincipal), false, Optional.empty()); |
| @@ -195,6 +195,10 @@ public Optional<Identity> getFunctionRunAsIdentity(Session session, CatalogSchem | |||
| return Optional.empty(); | |||
| } | |||
|
|
|||
| @Override | |||
| public void setFunctionOwner(Session session, CatalogSchemaFunctionName view, TrinoPrincipal principal) | |||
There was a problem hiding this comment.
I think you need to follow the convention here. Instead of saying what you want to do on event, you should simply emit the event and handle what you want in the implementation.
So you should emit events, when function is created, dropped or renamed. Like for any other object.
Description
Possibility to set ownership was already present for tables and views, so I am adding it to functions
Additional context and related issues
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: