Propagate function lifecycle events to SystemSecurityMetadata #24696

homar · 2025-01-13T15:25:48Z

Description

Propagate function lifecycle events sucha as create function and drop function to SystemSecurityMetadata

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:

## Section
* Add support for functions ownership

ksobolew · 2025-01-16T12:21:42Z

core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java

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.

ksobolew · 2025-01-16T12:29:07Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

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.

Dith3r · 2025-01-16T13:12:04Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

nit. ImmutableSet.of

kokosing · 2025-01-16T14:09:53Z

core/trino-main/src/main/java/io/trino/metadata/DisabledSystemSecurityMetadata.java

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.

ksobolew

Apart from nits LGTM

ksobolew · 2025-01-22T13:05:46Z

core/trino-main/src/main/java/io/trino/metadata/DisabledSystemSecurityMetadata.java

Nit: keep formatting consistent

Suggested change

@Override

public void functionCreated(Session session, CatalogSchemaFunctionName function, TrinoPrincipal principal)

{}

@Override

public void functionCreated(Session session, CatalogSchemaFunctionName function, TrinoPrincipal principal) {}

ksobolew · 2025-01-22T13:08:57Z

core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java

These callbacks should be done as the last thing, specifically after the actual update (here: after metadata.createLanguageFunction is called)

It is called later, here we just prepare a lambda function, it is invoked much later.

I'm sorry, I don't see any lambda...

nvm, my bad i looked at a wrong place, sorry for the confusion

ksobolew · 2025-01-22T13:16:24Z

testing/trino-tests/src/test/java/io/trino/security/TestingSystemSecurityMetadata.java

Suggested change

@Override

public void functionCreated(Session session, CatalogSchemaFunctionName function, TrinoPrincipal principal)

{ }

@Override

public void functionCreated(Session session, CatalogSchemaFunctionName function, TrinoPrincipal principal) {}

ksobolew · 2025-01-23T08:27:04Z

core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java

I mean these two statements should be switched, per convention at least:

Suggested change

if (catalogMetadata.getSecurityManagement() == SYSTEM) {

systemSecurityMetadata.functionCreated(

session,

new CatalogSchemaFunctionName(catalogHandle.getCatalogName().toString(), schemaFunctionName),

new TrinoPrincipal(PrincipalType.USER, session.getUser()));

}

metadata.createLanguageFunction(session.toConnectorSession(catalogHandle), schemaFunctionName, function, replace);

metadata.createLanguageFunction(session.toConnectorSession(catalogHandle), schemaFunctionName, function, replace);

if (catalogMetadata.getSecurityManagement() == SYSTEM) {

systemSecurityMetadata.functionCreated(

session,

new CatalogSchemaFunctionName(catalogHandle.getCatalogName().toString(), schemaFunctionName),

new TrinoPrincipal(PrincipalType.USER, session.getUser()));

}

lukasz-walkiewicz · 2025-01-23T12:55:54Z

core/trino-main/src/main/java/io/trino/metadata/SystemSecurityMetadata.java

Is there an operation which this method should be used by? There's not ALTER FUNCTION .. SET AUTHORIZATION for functions so I don't think there is such operation.

lukasz-walkiewicz · 2025-01-23T12:56:15Z

core/trino-main/src/main/java/io/trino/metadata/SystemSecurityMetadata.java

What's principal for? Isn't session enough to get the identity of the creator of the function?

following convention from other methods

ok i got it, fixed, thanks

lukasz-walkiewicz · 2025-01-27T10:18:47Z

core/trino-main/src/main/java/io/trino/testing/TestingAccessControlManager.java

(identity, function)

lukasz-walkiewicz · 2025-01-27T10:23:04Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

Is this needed if there's also:

assertAccessAllowed( functionOwnerSession, "CREATE FUNCTION memory.default.my_test_function_inner (x integer) RETURNS bigint RETURN x + 42");

?

yes, io.trino.security.TestingSystemSecurityMetadata#functionCreated is empty
This was suggested by @ksobolew to follow the convention

it was during offline conversation when I asked him about #24696 (comment) and #24696 (comment)

lukasz-walkiewicz · 2025-01-27T10:23:40Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

How about a check with the default session?

What would be the point of such a test? Verification if a function can be created is out of scope for this commit

lukasz-walkiewicz · 2025-01-27T10:26:30Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

It's a fairly complicated test case so maybe it's worth adding a few comments.
As I understand here the default session gets access denied because the function owner got access denied on the inner function, correct?

kokosing · 2025-01-27T15:50:15Z

core/trino-main/src/main/java/io/trino/metadata/DisabledSystemSecurityMetadata.java

Please improve a commit message and pull request title and description. It is more about propagating events of function creation than handling onwnership.

core/trino-main/src/main/java/io/trino/metadata/DisabledSystemSecurityMetadata.java

kokosing

Please fix the PR description to not to mention anything about ownerhship.

kokosing · 2025-01-28T10:48:06Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

I don't think it is correct testing of the functionality you have introduced. You need to check if events are propagated. It has not much to do with "ownership". It is a different layer. You need a test similar to io.trino.execution.TestBeginQuery. Then you don't need any changes in TestingAccessControlManager too.

I have discussed this with @lukasz-walkiewicz and now I understand why it was added.

Please use functionCreated and functionDropped in testing access control and extract the case with DENY to inner function as separate test case. This test is getting quite complex as we are reusing entities for different test cases.

I was about to add another test

ksobolew

Looks like now we have more extensive tests for functions than for views :)

ksobolew · 2025-01-29T08:25:41Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

Suggested change

.setIdentity(Identity.forUser(functionOwner)

.build())

.setIdentity(Identity.ofUser(functionOwner))

ksobolew · 2025-01-29T08:28:57Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

Nit: just to be extra-safe, you could add an assertion that with the same denied privilege you are not allowed to call the inner function directly

ksobolew · 2025-01-29T08:29:15Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

Did you mean to deny it here?

ksobolew · 2025-01-29T08:32:59Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

I'm not sure this is what @kokosing had in mind, but this name is not right. It's more about roles than denying.

ksobolew · 2025-01-29T08:33:23Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

Suggested change

.setIdentity(Identity.forUser(functionOwner1)

.build())

.setIdentity(Identity.ofUser(functionOwner1))

ksobolew · 2025-01-29T08:33:33Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

Suggested change

.setIdentity(Identity.forUser(functionOwner2)

.build())

.setIdentity(Identity.ofUser(functionOwner2))

ksobolew · 2025-01-29T08:34:43Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

Isn't this test duplicated now?

ksobolew

Re-approved from my side :)

kokosing · 2025-01-30T13:31:54Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

+    @Test
+    public void testAllowCallFunction()
+    {
+        reset();


nit. it already pre-exisiting, but we could do this as part of @Before method. Maybe an idea for a follow-up?

PTAL #24852

kokosing · 2025-01-30T14:16:38Z

Thank you! Merged.

cla-bot bot added the cla-signed label Jan 13, 2025

homar force-pushed the homar/add_setFunctionOwner branch 4 times, most recently from f3b4bf0 to ceca747 Compare January 16, 2025 09:57

homar changed the title ~~test~~ Add setFunctionOwner to SystemSecurityMetadata Jan 16, 2025

homar marked this pull request as ready for review January 16, 2025 10:03

homar requested review from Dith3r, kokosing and ksobolew January 16, 2025 10:03

ksobolew reviewed Jan 16, 2025

View reviewed changes

Dith3r reviewed Jan 16, 2025

View reviewed changes

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java Outdated

Copy link

Member

Dith3r Jan 16, 2025

Choose a reason for hiding this comment

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

nit. ImmutableSet.of

kokosing reviewed Jan 16, 2025

View reviewed changes

homar force-pushed the homar/add_setFunctionOwner branch 2 times, most recently from d70bdfa to 9566b40 Compare January 21, 2025 12:44

homar requested review from Dith3r, kokosing, ksobolew and lukasz-walkiewicz January 21, 2025 12:45

homar force-pushed the homar/add_setFunctionOwner branch 2 times, most recently from abc7ec8 to 6bd0903 Compare January 21, 2025 14:45

Dith3r approved these changes Jan 21, 2025

View reviewed changes

ksobolew approved these changes Jan 22, 2025

View reviewed changes

homar force-pushed the homar/add_setFunctionOwner branch 2 times, most recently from ffe40a9 to 48488b8 Compare January 22, 2025 21:17

homar requested a review from ksobolew January 22, 2025 23:05

ksobolew reviewed Jan 23, 2025

View reviewed changes

homar force-pushed the homar/add_setFunctionOwner branch 2 times, most recently from bc2bc9f to 23c06c4 Compare January 23, 2025 10:18

lukasz-walkiewicz reviewed Jan 23, 2025

View reviewed changes

homar force-pushed the homar/add_setFunctionOwner branch 2 times, most recently from 8c12f07 to 35808fb Compare January 23, 2025 15:35

lukasz-walkiewicz reviewed Jan 27, 2025

View reviewed changes

homar force-pushed the homar/add_setFunctionOwner branch 2 times, most recently from 920046d to 8832645 Compare January 27, 2025 14:05

kokosing reviewed Jan 27, 2025

View reviewed changes

homar changed the title ~~Add setFunctionOwner to SystemSecurityMetadata~~ Propagate function lifecycle events to SystemSecurityMetadata Jan 27, 2025

homar force-pushed the homar/add_setFunctionOwner branch from 8832645 to b6ded27 Compare January 27, 2025 17:28

lukasz-walkiewicz approved these changes Jan 28, 2025

View reviewed changes

kokosing reviewed Jan 28, 2025

View reviewed changes

homar force-pushed the homar/add_setFunctionOwner branch 3 times, most recently from fe976ee to 1d94fd5 Compare January 29, 2025 07:48

ksobolew reviewed Jan 29, 2025

View reviewed changes

Propagate function lifecycle events to SystemSecurityMetadata

c55ff9f

homar force-pushed the homar/add_setFunctionOwner branch from 1d94fd5 to c55ff9f Compare January 29, 2025 09:59

ksobolew approved these changes Jan 29, 2025

View reviewed changes

homar requested review from kokosing and lukasz-walkiewicz January 29, 2025 12:44

kokosing approved these changes Jan 30, 2025

View reviewed changes

kokosing merged commit 5e3e900 into trinodb:master Jan 30, 2025
94 checks passed

github-actions bot added this to the 470 milestone Jan 30, 2025

mosabua mentioned this pull request Jan 30, 2025

Add Trino 470 release notes #24824

Merged

-    @Override
-    public void functionCreated(Session session, CatalogSchemaFunctionName function, TrinoPrincipal principal)
-    {}
+    @Override
+    public void functionCreated(Session session, CatalogSchemaFunctionName function, TrinoPrincipal principal) {}

-        if (catalogMetadata.getSecurityManagement() == SYSTEM) {
-            systemSecurityMetadata.functionCreated(
-                    session,
-                    new CatalogSchemaFunctionName(catalogHandle.getCatalogName().toString(), schemaFunctionName),
-                    new TrinoPrincipal(PrincipalType.USER, session.getUser()));
-        }
-        metadata.createLanguageFunction(session.toConnectorSession(catalogHandle), schemaFunctionName, function, replace);
+        metadata.createLanguageFunction(session.toConnectorSession(catalogHandle), schemaFunctionName, function, replace);
+        if (catalogMetadata.getSecurityManagement() == SYSTEM) {
+            systemSecurityMetadata.functionCreated(
+                    session,
+                    new CatalogSchemaFunctionName(catalogHandle.getCatalogName().toString(), schemaFunctionName),
+                    new TrinoPrincipal(PrincipalType.USER, session.getUser()));
+        }

	.setIdentity(Identity.forUser(functionOwner)
	.build())
	.setIdentity(Identity.ofUser(functionOwner))

Propagate function lifecycle events to SystemSecurityMetadata #24696

Propagate function lifecycle events to SystemSecurityMetadata #24696

Uh oh!

Conversation

homar commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context and related issues

Release notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksobolew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksobolew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

homar commented Jan 13, 2025 •

edited

Loading