Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow schema owner to create, drop and rename schema #1139

Merged
merged 3 commits into from Jul 22, 2019

Conversation

findepi
Copy link
Member

@findepi findepi commented Jul 17, 2019

No description provided.

@cla-bot cla-bot bot added the cla-signed label Jul 17, 2019
Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

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

@kokosing please review?

Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

% comment

@@ -78,19 +78,25 @@ public FileBasedAccessControl(FileBasedAccessControlConfig config)
@Override
public void checkCanCreateSchema(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, String schemaName)
{
denyCreateSchema(schemaName);
if (!isSchemaOwner(identity, schemaName)) {
Copy link
Member

Choose a reason for hiding this comment

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

So the idea is you can create any schema you would become an owner of? IIRC you normally need to be an owner of the catalog to create or rename schemas. I think this is a reasonable hack for the file connector, but it should be documented somewhere.

@martint or @electrum thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable to me. I had originally suggested a separate permission for schema creation, but it doesn't make sense to allow creating a schema for which you would have no ownership.

Copy link
Member

Choose a reason for hiding this comment

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

I think the same logic should apply to table and view creation: rather than checking schema ownership, check whether the user owns the table.

Copy link
Member

Choose a reason for hiding this comment

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

That seems reasonable also. I'd prefer for it to be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

Isn’t this a chicken-and-egg issue? A user can’t be owner of a schema that does not yet exist... what am I missing?

Copy link
Member

Choose a reason for hiding this comment

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

@martint the permissions file allows you to have permissions for anything regardless of its existence or not. Since file based security system is totally generic, there isn't a good way to restrict that even if you wanted to, and instead we rely on the knowledge that the engine only checks for stuff that exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the same logic should apply to table and view creation: rather than checking schema ownership, check whether the user owns the table.

@electrum this sounds reasonable. I am concerned this may reduce protection in some cases though. Let's do it separately.

@findepi findepi merged commit c6c0859 into trinodb:master Jul 22, 2019
@findepi findepi deleted the create-drop-schema branch July 22, 2019 19:06
@findepi findepi added this to the 317 milestone Jul 23, 2019
@findepi findepi mentioned this pull request Jul 23, 2019
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

6 participants