Add generic SET AUTHORIZATION #21794

djsagain · 2024-05-01T20:38:05Z

This commit adds machinery to set the owner of arbitrary entities, by extending the syntax of
ALTER (SCHEMA | TABLE | VIEW) qualifiedName SET AUTHORIZATION to support arbitrary owningKinds in place of SCHEMA, TABLE or VIEW. Checks that a specific SET AUTHORIZATION is legal is done by AccessControl.checkCanSetEntityAuthorization, also defined by SystemAccessControl. Setting the owner is done by Metadata.setEntityAuthorization and
SystemSecurityMetadata.setEntityAuthorization.

Description

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.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

lozbrown · 2024-05-07T12:40:35Z

Would that help with this issue?

#21450

djsagain · 2024-05-07T14:24:08Z

Would that help with this issue?

#21450

No, I don't think this change will help. Materialized views must have owners when they are created. Perhaps you could post to the Trino #troubleshooting Slack channel about this.

lozbrown · 2024-05-07T18:27:18Z

I did that some time ago but got no response

https://trinodb.slack.com/archives/CGB0QHWSW/p1712241258245569

github-actions · 2024-06-25T17:03:46Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

mosabua · 2024-06-25T21:45:25Z

I assume you are still working on this @djsstarburst

djsagain · 2024-06-26T00:51:43Z

I assume you are still working on this @djsstarburst

Yes, @mosabua.

djsagain · 2024-10-10T16:19:42Z

Hi @martint. We discussed this PR, and you took a quick look at it last spring. I just brought it up-to-date with tip master.

Could you please take another look and see if it can be approved? Thanks!

dain

I would like to take a different approach to this PR. Instead of retaining the existing set*Authorization we remove them entirelly. Then in the SPI we have default implementation of setEntityAuthorization we switch over the entities and call the exisitng methods. Then we mark the existing 3 methods as deprecated for removal. We would do the same thing for the security checks.

The main, notable, difference with the existing code would be that we no longer perform an existance check before doing the authorization assignment. I think that is ok. If we decide we want that later we can add a generic entitiy exists mehtod.

core/trino-grammar/src/main/antlr4/io/trino/grammar/sql/SqlBase.g4

core/trino-main/src/main/java/io/trino/execution/SetAuthorizationTask.java

core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java

djsagain · 2024-11-21T21:44:46Z

I would like to take a different approach to this PR.

@dain - - I've tried to do what you suggested, and I've pushed out the result. But I'm not certain I've gotten the details right. Please take another quick look.

dain

You are getting there. In all modules other than SPI, we should simply remove the methods. In the SPI we leave the methods, but mark them @Deprecated(forRemoval=ture). This means the nothing else in Trino is allowed to call this method. The new method will have a default implementation that calls through to the old deprecated implementations (likely needs the deprecation check suppressed for that method). This way existing implementations should still work, but it forces them to update. Then in a few months we remove the deprecated methods entirelly.

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

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

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

core/trino-main/src/main/java/io/trino/security/AccessControl.java

core/trino-main/src/main/java/io/trino/security/AccessControlManager.java

core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControl.java

core/trino-spi/src/main/java/io/trino/spi/security/AccessDeniedException.java

...-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllSystemAccessControl.java

djsagain · 2025-02-26T23:51:17Z

@homar raised the question of whether there should be a way in SQL to see the owner of an entity. AIUI that would require some new syntax, maybe something like:

SHOW AUTHORIZATION ON ownedEntityKind qualifiedName BY principal

@martint and @dain - - Do you think it's worthwhile to extend this PR to add something like that? If we think it's worth doing, we would have to decide on the visibility rules for owners of entities.

ksobolew · 2025-02-27T09:15:12Z

@djsstarburst see #10593, there are some (other) options there

djsagain · 2025-02-27T17:06:19Z

@djsstarburst see #10593, there are some (other) options there

Good point; maybe exposing ownership as columns in the information_schema makes sense. I don't really have an opinion, but I'll be @martint does.

core/trino-grammar/src/main/antlr4/io/trino/grammar/sql/SqlBase.g4

core/trino-main/src/main/java/io/trino/security/AccessControlManager.java

core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java

core/trino-spi/src/main/java/io/trino/spi/security/AccessDeniedException.java

dain · 2025-02-28T19:14:17Z

core/trino-spi/src/test/java/io/trino/spi/testing/InterfaceTestUtils.java

@electrum what do you think about this change?

FTR, I removed this line, and will add any dependency methods that are found to be missing.

Changes implemented

dain

Other than the build failure, and the outstanding question for @electrum, this looks good to me

kokosing · 2025-03-17T14:25:19Z

@electrum can you please take a look? It looks this PR waits on your feedback only (see #21794 (review)).

electrum · 2025-03-20T14:03:28Z

The comment for me was addressed. This can proceed.

kokosing · 2025-03-21T12:53:33Z

core/trino-grammar/src/main/antlr4/io/trino/grammar/sql/SqlBase.g4

MATERIALIZED VIEW was not supported before. I think it would be nice to add it in separate commit, or at least mention this in the commit message that it is something it is now possible to set owner for materialized view.

EDIT: I see that it is missing in the implementation so let's simply remove it from the grammar.

What is the use case for identifier? Does it mean that user is able to pass anything, syntax will be fine, but we will fail to execute that?

Also I have some hard time to understand why identifier is something for that. Usually identifier is a name of the thing not a type of the thing.

I would prefer to remove this from this PR.

The reason for the identifier is that this is an extension mechanism, and therefore Trino can't know what entity kinds will be used.

This can't be removed from the PR - - the extension mechanism is the entire reason for the PR.

We already support arbitrary entity kinds in GRANT, REVOKE and DENY, so it perfectly makes sense to have it here as well.

kokosing · 2025-03-21T12:54:25Z

core/trino-main/src/main/java/io/trino/execution/SetAuthorizationTask.java

MATERIALIZED VIEW is missing here, maybe we should simply remove it from the grammar?

No, it's not missing. MATERIALIZED VIEW is treated as a VIEW. See line 512 of AstBuilder.

Please correct me if I am wrong. Previously we were also able to set an owner for MATERIALIZED view because it was just had it for the view. Now we do the same but we change the syntax that MATERIALIZED keyword can be added but it is not necessary.

So my question is why are you adding MATERIALIZED keyword? Why not to keep existing behaviour?

Oh! You are absolutely right - - using the MATERIALIZED keyword in the SET AUTHORIZATION grammar is a bug.

Fixed in the grammar and in AstBuilder.visitSetAuthorization.

kokosing · 2025-03-21T12:56:36Z

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

Can we enum for ownedKind. I would prefer the name entityType. Or let's use the name from the grammar OwnedEntityKind.

Notice that using enum will help us to do static code analysis to make sure we cover all the cases by using the switch statement. It will be useful when one would like to add new entries.

As mentioned we cannot use an enum, because this is an extension mechanism, and Trino itself can't know all the entity kinds used by all extension uses.

kokosing · 2025-03-21T12:58:07Z

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

Materialized view is also missing here.

No, it's not missing. MATERIALIZED VIEW is treated as a VIEW. See line 512 of AstBuilder.

kokosing · 2025-03-21T12:59:15Z

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

Are there any entities that are not catalog related?

Clients of the extension mechanism can define entity kinds that not contained in catalogs. For example, the extension mechanism can be used to support ownership of roles, which are not contained in catalogs.

kokosing · 2025-03-21T13:05:05Z

core/trino-main/src/main/java/io/trino/security/AccessControlManager.java

Can we use io.trino.metadata.QualifiedObjectName instead of List<String>?

No, we can't use QualifiedObjectName because it is based on the assumption that all ownable entity kinds are contained in a catalog and schema.

See also io.trino.spi.connector.EntityKindAndName, which is a pair of String entityKind and List<String> name. BTW, perhaps we could use it here as well.

This is a reasonable suggestion, and is consistent with checkCanGrantEntityPrivilege.

I made this change everywhere except denySetEntityAuthorization. I didn't make it there because there are quite a few of calls.

[Later]

I ended up making the change for denySetEntityAuthorization as well, since that is more consistent.

This commit adds machinery to set the owner of arbitrary entities, by extending the syntax of ALTER (SCHEMA | TABLE | VIEW) qualifiedName SET AUTHORIZATION to support arbitrary owningKinds in place of SCHEMA, TABLE or VIEW. Checks that a specific SET AUTHORIZATION is legal is done by AccessControl.checkCanSetEntityAuthorization, also defined by SystemAccessControl. Setting the owner is done by Metadata.setEntityAuthorization and SystemSecurityMetadata.setEntityAuthorization.

kokosing · 2025-03-27T20:29:55Z

@djsstarburst Thank you! Merged.

djsagain · 2025-03-27T21:07:04Z

Thank you! Merged.

Thank YOU @kokosing for your review!

cla-bot bot added the cla-signed label May 1, 2024

djsagain force-pushed the david.stryker/add-generic-set-authorization branch from 19b0949 to bbb067b Compare May 2, 2024 13:38

djsagain force-pushed the david.stryker/add-generic-set-authorization branch from bbb067b to a8153d8 Compare May 7, 2024 15:49

djsagain force-pushed the david.stryker/add-generic-set-authorization branch from a8153d8 to 5b3c06c Compare May 19, 2024 18:57

djsagain requested a review from martint May 20, 2024 12:47

djsagain marked this pull request as ready for review May 20, 2024 12:53

djsagain force-pushed the david.stryker/add-generic-set-authorization branch from 5b3c06c to 67040b4 Compare June 4, 2024 15:20

github-actions bot added the stale label Jun 25, 2024

mosabua added stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. and removed stale labels Jun 25, 2024

djsagain force-pushed the david.stryker/add-generic-set-authorization branch from 67040b4 to 0bb9bea Compare August 4, 2024 14:20

djsagain force-pushed the david.stryker/add-generic-set-authorization branch from 0bb9bea to 5ed9185 Compare October 10, 2024 16:18

dain added the syntax-needs-review label Oct 11, 2024

dain requested changes Nov 19, 2024

View reviewed changes

djsagain force-pushed the david.stryker/add-generic-set-authorization branch 2 times, most recently from 9205917 to a39beac Compare November 21, 2024 21:42

dain previously requested changes Dec 9, 2024

View reviewed changes

djsagain force-pushed the david.stryker/add-generic-set-authorization branch 3 times, most recently from 7374ecc to 35814ce Compare December 18, 2024 18:53

djsagain mentioned this pull request Feb 17, 2025

Add ALTER FUNCTION ... SET AUTHORIZATION #25021

Open

djsagain force-pushed the david.stryker/add-generic-set-authorization branch from 35814ce to 7b96454 Compare February 17, 2025 18:11

dain reviewed Feb 28, 2025

View reviewed changes

djsagain force-pushed the david.stryker/add-generic-set-authorization branch from 7b96454 to 4f454b0 Compare March 2, 2025 15:36

dain reviewed Mar 2, 2025

View reviewed changes

djsagain force-pushed the david.stryker/add-generic-set-authorization branch from 4f454b0 to a5154d6 Compare March 3, 2025 14:25

djsagain force-pushed the david.stryker/add-generic-set-authorization branch 3 times, most recently from 3a0a10c to 47f1383 Compare March 12, 2025 21:32

kokosing reviewed Mar 21, 2025

View reviewed changes

djsagain force-pushed the david.stryker/add-generic-set-authorization branch from 47f1383 to 93ffec9 Compare March 26, 2025 23:38

djsagain force-pushed the david.stryker/add-generic-set-authorization branch from 93ffec9 to d8c5587 Compare March 27, 2025 01:12

kokosing approved these changes Mar 27, 2025

View reviewed changes

kokosing merged commit 1a81b69 into trinodb:master Mar 27, 2025
94 checks passed

github-actions bot added this to the 475 milestone Mar 27, 2025

codyzwief mentioned this pull request Jun 3, 2025

Add support for ALTER MATERIALIZED VIEW .. SET AUTHORIZATION #25910

Merged

Add generic SET AUTHORIZATION #21794

Add generic SET AUTHORIZATION #21794

Uh oh!

Conversation

djsagain commented May 1, 2024

Description

Additional context and related issues

Release notes

Uh oh!

lozbrown commented May 7, 2024

Uh oh!

djsagain commented May 7, 2024

Uh oh!

lozbrown commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 25, 2024

Uh oh!

mosabua commented Jun 25, 2024

Uh oh!

djsagain commented Jun 26, 2024

Uh oh!

djsagain commented Oct 10, 2024

Uh oh!

dain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djsagain commented Nov 21, 2024

Uh oh!

dain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djsagain commented Feb 26, 2025

Uh oh!

ksobolew commented Feb 27, 2025

Uh oh!

djsagain commented Feb 27, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dain left a comment

Choose a reason for hiding this comment

Uh oh!

kokosing commented Mar 17, 2025

Uh oh!

electrum commented Mar 20, 2025

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

lozbrown commented May 7, 2024 •

edited

Loading

djsagain Mar 27, 2025 •

edited

Loading