Add generic SET AUTHORIZATION #21794
Would that help with this issue?
Would that help with this issue?
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.
I did that some time ago but got no response
https://trinodb.slack.com/archives/CGB0QHWSW/p1712241258245569
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua
I assume you are still working on this @djsstarburst
I assume you are still working on this @djsstarburst
Yes, @mosabua.
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!
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.
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.
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.
@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.
@djsstarburst see #10593, there are some (other) options there
@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.
| 49 | 50 | if (method.getDeclaringClass() == Object.class) { | |
| 50 | 51 | continue; | |
| 51 | 52 | } | |
| 53 | if (Arrays.stream(method.getAnnotations()).anyMatch(annotation -> annotation instanceof Deprecated deprecated && deprecated.forRemoval())) { |
dain48 days ago@electrum what do you think about this change?
djsstarburst37 days agoFTR, I removed this line, and will add any dependency methods that are found to be missing.
@electrum can you please take a look? It looks this PR waits on your feedback only (see #21794 (review)).
The comment for me was addressed. This can proceed.
| 945 | 943 | ; | |
| 946 | 944 | ||
| 945 | ownedEntityKind | ||
| 946 | : TABLE | SCHEMA | VIEW | MATERIALIZED VIEW | identifier |
kokosing27 days agoMATERIALIZED 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.
kokosing27 days agoWhat 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.
djsstarburst22 days agoThe 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.
ksobolew22 days agoWe already support arbitrary entity kinds in GRANT, REVOKE and DENY, so it perfectly makes sense to have it here as well.
kokosing27 days agoMATERIALIZED VIEW is missing here, maybe we should simply remove it from the grammar?
djsstarburst22 days agoNo, it's not missing. MATERIALIZED VIEW is treated as a VIEW. See line 512 of AstBuilder.
kokosing22 days agoPlease 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?
djsstarburst22 days agoOh! You are absolutely right - - using the MATERIALIZED keyword in the SET AUTHORIZATION grammar is a bug.
Fixed in the grammar and in AstBuilder.visitSetAuthorization.
| 235 | 217 | public void columnNotNullConstraintDropped(Session session, CatalogSchemaTableName table, String column) {} | |
| 236 | 218 | ||
| 219 | @Override | ||
| 220 | public void setEntityOwner(Session session, String ownedKind, List<String> name, TrinoPrincipal principal) |
kokosing27 days agoCan 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.
djsstarburst22 days ago👎 1As 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.
kokosing27 days agoMaterialized view is also missing here.
djsstarburst22 days agoNo, it's not missing. MATERIALIZED VIEW is treated as a VIEW. See line 512 of AstBuilder.
| 180 | 180 | public final class MetadataManager | |
| 181 | 181 | implements Metadata | |
| 182 | 182 | { | |
| 183 | private static final Set<String> ENTITY_KINDS_WITH_CATALOG = ImmutableSet.of("SCHEMA", "TABLE", "VIEW"); |
kokosing27 days agoAre there any entities that are not catalog related?
djsstarburst22 days agoClients 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.
| 1443 | 1398 | } | |
| 1444 | 1399 | ||
| 1400 | @Override | ||
| 1401 | public void checkCanSetEntityAuthorization(SecurityContext securityContext, String ownedKind, List<String> name, TrinoPrincipal principal) |
kokosing27 days agoCan we use io.trino.metadata.QualifiedObjectName instead of List<String>?
djsstarburst22 days agoNo, we can't use QualifiedObjectName because it is based on the assumption that all ownable entity kinds are contained in a catalog and schema.
ksobolew22 days agoSee 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.
djsstarburst22 days ago (edited 22 days ago)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.
Add generic SET AUTHORIZATION
@djsstarburst Thank you! Merged.
Thank you! Merged.
Thank YOU @kokosing for your review!
Login to write a write a comment.
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: