Support role management (v5) #11645

sopel39 · 2018-10-05T15:33:40Z

Supersedes #11475

Leverage newly introduced method for recursive role grants traversal

Identity must hold all the selected roles for all the catalogs. ConnectorIdentity holds only the role selected for some particular catalog.

arhimondr · 2018-10-05T19:47:32Z

presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4

    | CALL qualifiedName '(' (callArgument (',' callArgument)*)? ')'   #call
+    | CREATE ROLE name=identifier
+        (WITH ADMIN grantor)?
+        (IN catalog=identifier)?                                       #createRole


I thought we agreed that the IN extension should not be supported. ROLE manipulations should be allowed only within the session catalog.

I thought we agreed to merge this as it is and fix it later. Would you accept a fix on top of this? Instead of doing rebase one more time?

I missed that part

I thought we agreed to merge this as it is and fix it later. Would you accept a fix on top of this? Instead of doing rebase one more time?

That's what I also think was decided.

@ilfrin @martint you have more context.

We decided to leave that functionality out since it's not clear exactly how it should work and there's an easy workaround.

Sounds good. Would you accept a fix on top of this? Instead of doing rebase one more time?

For SHOW ROLES, issue the query: select role_name as "Role" from catalog.information_schema.roles;

That way roles are enumerated lazily.

This way table privileges are enumerated lazily.

Currently Presto shows that the owner of a table has ALL privileges, even after some privileges are revoked. This commit fixes this issue by listing only privileges actually present in the metastore.

Presto currently lists only privilges of the tables owned by the current user, even after the admin role is set. This commit fixes this and lists all privileges for admins.

When tables of the same name exist across different schemas, Presto lists privileges of the table from all schemas instead of the single schema mentioned in the SHOW GRANTS query. This commit fixes the issue.

sopel39 · 2018-10-08T21:16:54Z

It's green, so maybe merge it?

The roles set during a CLI session were not being set and thus SHOW CURRENT ROLES returned incorrect results.

arhimondr · 2018-11-06T20:19:57Z

@kokosing

Remove 'IN catalog' syntax from role management commands

Thanks for doing this

sopel39 · 2018-11-06T20:21:56Z

@arhimondr is it ok to land with it?

arhimondr · 2018-11-06T20:23:07Z

@sopel39 I think it should be. @martint ?

findepi · 2019-01-25T12:31:54Z

@kokosing said there was some unnoticed problem during rebase, when the source branch of this PR was created.
Let me close this PR as it should not be merged in its current shape.

tooptoop4 · 2019-02-22T20:10:44Z

gentle ping

highker · 2019-02-23T02:31:27Z

I will take a look; @arhimondr do you wanna chime in?

arhimondr · 2019-02-23T03:11:44Z

Yeah, i was going to. I think we should just merge it.

…

On Fri, Feb 22, 2019 at 6:31 PM James Sun ***@***.***> wrote: I will take a look; @arhimondr <https://github.com/arhimondr> do you wanna chime in? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11645 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFUBrELWW7yeERX1SIaKL3r6-xbdyuQOks5vQKgOgaJpZM4XKdNy> .

arhimondr · 2019-02-25T16:52:41Z

@tooptoop4 #12383

Andrii Rosa added 15 commits October 5, 2018 17:08

Introduce CREATE ROLE and DROP ROLE statements

2f86b56

Move PrincipalType to presto-spi

22086ae

Expose Create/Drop/List roles methods in SPI

eb26dbc

Introduce <catalog>.information_schema.roles table

db6f21a

Assign admin role to subset of users in FileHiveMetastore

c72355a

Implement Create/Drop/List roles in Hive connector

a8c461e

Introduce GRANT/REVOKE roles statements

e7e1fcc

Add Grant/Revoke/List roles authorization to the SPI

be4e46f

Introduce APPLICABLE_ROLES view

434412d

Implement Grant/Revoke/ListApplicableRoles in Hive

223b8cb

Refactor GRANT/REVOKE in Hive

0b1c7ff

Leverage newly introduced method for recursive role grants traversal

Introduce access control for GRANT/REVOKE ROLE

b6881d1

Prepare metastore interface to accept ROLE for GRANT/REVOKE

c77cf06

Introduce SET ROLE statement

76476ba

Introduce ConnectorIdentity

d9fe960

Identity must hold all the selected roles for all the catalogs. ConnectorIdentity holds only the role selected for some particular catalog.

sopel39 requested a review from findepi October 5, 2018 15:33

sopel39 mentioned this pull request Oct 5, 2018

Support role management (v4) [WIP] #11475

Closed

facebook-github-bot added the CLA Signed label Oct 5, 2018

arhimondr reviewed Oct 5, 2018

View reviewed changes

sopel39 mentioned this pull request Oct 8, 2018

Rename ConnectorId to CatalogName #11653

Closed

Implement SET ROLE

f5baa92

sopel39 force-pushed the epic/support-role-management/pr5 branch from 75ecb25 to 8ba37e2 Compare October 8, 2018 15:49

sopel39 and others added 8 commits October 8, 2018 22:01

Make HiveQueryRunner default user have admin role

e890a4e

Set admin role before setting system properties in storage format test

8449a2a

Store catalog selected roles in Identity

7d4df68

Introduce ENABLED_ROLES view

f03cfde

Implement SET ROLE in Hive Connector

a69af10

Accept ROLE in GRANT/REVOKE Privileges statements

de3f863

Add SHOW ROLES to the parser

5b54f6b

Rewrite SHOW ROLES as a select query

51494d6

For SHOW ROLES, issue the query: select role_name as "Role" from catalog.information_schema.roles;

kokosing and others added 5 commits October 8, 2018 22:21

Make ThriftMetastoreUtil.list*Roles methods to return Stream

73535bb

That way roles are enumerated lazily.

Make list*TablePrivileges to return Stream

5c6d4f8

This way table privileges are enumerated lazily.

Fix listing privileges of tables owned by an user

d1422e4

Currently Presto shows that the owner of a table has ALL privileges, even after some privileges are revoked. This commit fixes this issue by listing only privileges actually present in the metastore.

Fix listing privileges when admin role is set

e471250

Presto currently lists only privilges of the tables owned by the current user, even after the admin role is set. This commit fixes this and lists all privileges for admins.

Fix rewrite SHOW GRANTS as a SELECT query

4c93a9b

When tables of the same name exist across different schemas, Presto lists privileges of the table from all schemas instead of the single schema mentioned in the SHOW GRANTS query. This commit fixes the issue.

sopel39 force-pushed the epic/support-role-management/pr5 branch from 8ba37e2 to 4c93a9b Compare October 8, 2018 20:25

kokosing approved these changes Oct 8, 2018

View reviewed changes

anusudarsan force-pushed the epic/support-role-management/pr5 branch from 918eb71 to 26f9a22 Compare October 26, 2018 21:46

fixup! Implement SET ROLE

b431446

The roles set during a CLI session were not being set and thus SHOW CURRENT ROLES returned incorrect results.

anusudarsan force-pushed the epic/support-role-management/pr5 branch from 26f9a22 to b431446 Compare October 29, 2018 14:32

Remove 'IN catalog' syntax from role management commands

8a43aae

findepi mentioned this pull request Jan 24, 2019

Support role management trinodb/trino#44

Closed

findepi closed this Jan 25, 2019

sopel39 mentioned this pull request Jan 29, 2019

Roles PR trinodb/trino#90

Merged

findepi deleted the epic/support-role-management/pr5 branch January 29, 2019 12:52

findepi restored the epic/support-role-management/pr5 branch January 29, 2019 12:54

findepi deleted the epic/support-role-management/pr5 branch January 29, 2019 12:58

highker self-assigned this Feb 23, 2019

highker assigned arhimondr and unassigned highker Feb 23, 2019

arhimondr mentioned this pull request Feb 25, 2019

Roles management #12383

Merged

Support role management (v5) #11645

Support role management (v5) #11645

Uh oh!

Conversation

sopel39 commented Oct 5, 2018

Uh oh!

arhimondr Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

kokosing Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

arhimondr Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

sopel39 Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

martint Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

kokosing Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

sopel39 commented Oct 8, 2018

Uh oh!

arhimondr commented Nov 6, 2018

Uh oh!

sopel39 commented Nov 6, 2018

Uh oh!

arhimondr commented Nov 6, 2018

Uh oh!

findepi commented Jan 25, 2019

Uh oh!

tooptoop4 commented Feb 22, 2019

Uh oh!

highker commented Feb 23, 2019

Uh oh!

arhimondr commented Feb 23, 2019 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arhimondr commented Feb 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

arhimondr commented Feb 23, 2019 via email •

edited

Loading