Add SQL syntax for GRANT/REVOKE on schema

[lukasz-walkiewicz]

No description provided.

[lukasz-walkiewicz]

Resolves #4327

[kokosing]

We need some connector independent tests. Maybe you could use MockConnector for that. It would be nice to make sure it works end to end, before we start using this for connectors. What do you think?

Looks good in general. Few comments.

[kokosing]

@martint Can you please double check the syntax?

[lhofhansl]

I do not quite understand the point of this.

We do not have any connector that can support this, and it's not even clear to how to add it existing connectors:

• for Hive we'd diverge from the Hive can do. IMHO that's OK, but we argued earlier that that's not OK.
• for other connector (say Postgres), how would this work? We would likely not want to configure a catalog with a Postgres superuser, but only a "normal" user. So grant/revoke would have to happen in Postgres outside of Presto.

Don't get me wrong. I'd love to add this for Hive and other connectors. It would make our lives easier here, just not clear how to splice this through to the connectors.

[kokosing]

We do not have any connector that can support this

We are thinking about implementing this in Starburst.

it's not even clear to how to add it existing connectors:

I am not sure I followed the previous discussion. I think we can revisit adding this to Hive SQL standard authorization.

for other connector (say Postgres), how would this work?

I didn't think hard about this, but to me it would simply translate to Postgres equivalent statements of GRANT and REVOKE to SCHEMA.

We would likely not want to configure a catalog with a Postgres superuser, but only a "normal" user.

The best would be to use impersonation, credentials passthrough or extra credentials so user bob in Presto becomes bob in Postgres.

Don't get me wrong

I don't. I appreciate when someone is questioning the idea before implementation. It could save hours of work.

[kokosing]

This looks nice. Few comments.

[martint]

Syntax-wise, this is not in the SQL spec, but it's consistent with it:

<grant privilege statement> ::=
GRANT <privileges> TO <grantee> [ { <comma> <grantee> }... ]
      [ WITH HIERARCHY OPTION ]
      [ WITH GRANT OPTION ]
      [ GRANTED BY <grantor> ]

<privileges> ::=
  <object privileges> ON <object name>

<object name> ::= 
      [ TABLE ] <table name>
    | DOMAIN <domain name>
    | COLLATION <collation name>
    | CHARACTER SET <character set name> 
    | TRANSLATION <transliteration name> 
    | TYPE <schema-resolved user-defined 
    | SEQUENCE <sequence generator name> 
    | <specific routine designator>

What I think needs to be discussed are the semantics of this operation, given that it introduces a behavior on hierarchical objects that isn't contemplated by the spec. I.e., does revoking a privilege on a schema imply a revocation of privileges over all tables in the schema? Does performing a SELECT require checking permissions both on the table and the schema? Etc...

For reference, the spec describes SELECT permissions as applying to columns, not tables. Doing a GRANT SELECT ON TABLE t is a way to say "grant SELECT on current and future columns of the table". GRANT SELECT(x) ON TABLE t means "grant SELECT only on column x of table t".

2) SELECT (<column name>) specifies the SELECT privilege on the indicated column and implies one or more column privilege descriptor.
[...]
4) SELECT with neither <privilege column list> nor <privilege method list> specifies the SELECT privilege on all columns of T including any columns subsequently added to T and implies a table privilege descriptor and one or more column privilege descriptors. If T is a table of a structured type TY, then SELECT also specifies the SELECT privilege on all methods of the type TY, including any methods subsequently added to the type TY, and implies one or more table/method privilege descriptors.

It records the privilege in a table privilege descriptor for the purpose of applying the privilege to columns that are added afterwards, but only the column privilege descriptors are consulted when querying. From section "6.7 ":

ii) Otherwise, the current privileges shall include SELECT on the column referenced by CR.

[kokosing]

WITH HIERARCHY OPTION

What does it mean?

does revoking a privilege on a schema imply a revocation of privileges over all tables in the schema

Schema privilege is to me independent to other privileges. So revoking should not affect any other privileges that user currently has.

So

GRANT SELECT on s.t TO bob;
 -- bob can select from s.t
GRANT SELECT SCHEMA s TO bob;
 -- bob can select from s.t
REVOKE SELECT SCHEMA s FROM bob;
 -- bob can select from s.t
REVOKE SELECT on s.t FROM bob;
-- bob can no longer select from s.t

Doing a GRANT SELECT ON TABLE t is a way to say "grant SELECT on current and future columns of the table".

So doing GRANT SELECT ON SCHEMA s is a way to say "grant SELECT on current and future columns of all current and future tables of SCHEMA s"

Does performing a SELECT require checking permissions both on the table and the schema?

No, because the above (GRANT SELECT ON SCHEMA translates to grant SELECT on columns).

What I think needs to be discussed are the semantics of this operation,

One more point. We try to follow data source semantics when writing connectors. So Hive connector should behave like Hive. So should we do the same thing here for access control. For example should allow to have different semantics when hive.security=sql-standard or hive.security=other-authorization-polices-service so we follow the behavior of sql-standard and other-authorization-polices-service in hive?

[kokosing]

This looks decent. Nice! Just few comments otherwise looks good.

@martint Please take a look

[kokosing]

Please also add tests for SHOW GRANTS and information_schema.table_privileges while using GRANT/REVOKE ON SCHEMA

[kokosing]

@martint Would you like to take a look?

[kokosing]

just session?

[kokosing]

session?

[kokosing]

Merged, thanks!