Authorize table parameters in CTAS and remove deprecated check methods#10939
Conversation
Access controls interfaces allow implementor to inspect new table's properties. This is done for CREATE TABLE`, but was not done for `CREATE TABLE AS`. Instead, a deprecated access control method was called.
Remove `ConnectorAccessControl` and `SystemAccessControl`'s `checkCanCreateTable` and `checkCanCreateMaterializedView` checks that do not take properties that were deprecated some time ago. Remove associated fallback configuration toggle. Among other things, this forces plugin implementors to implement the correct method. This is important, because the old method did not delegate to the new, nor vice versa.
dain
left a comment
dain
left a comment
There was a problem hiding this comment.
Great find!
I suggest we leave the deprecated methods in connector and system access control, and have the new methods pass through to them. This way we don't break someone who did not realize we changed the signature.
| * @deprecated use {@link #checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName, Map properties)} instead | ||
| */ | ||
| @Deprecated | ||
| default void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName) |
There was a problem hiding this comment.
I suggest we leave the deprecated methods in connector and system access control, and have the new methods pass through to them. This way we don't break someone who did not realize we changed the signature.
There was a problem hiding this comment.
I agree this is how this should be implemented in the first place, in 364 when the new methods were added.
But now, it's not worthwhile to fix it, and we want to remove the deprecated methods at some point in time anyway. IMO the time is now.
There was a problem hiding this comment.
Wow, didn't realize it was that long ago. I guess we have what we have
| * @deprecated use {@link #checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName, Map properties)} instead | ||
| */ | ||
| @Deprecated | ||
| default void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName) |
There was a problem hiding this comment.
Wow, didn't realize it was that long ago. I guess we have what we have
2 participants
Access controls interfaces allow implementor to inspect new table's
properties. This is done for
CREATE TABLE, but was not done forCREATE TABLE AS.Instead, a deprecated access control method was called.
Follows #9401