Add invoker security mode for views by electrum · Pull Request #12082 · prestodb/presto

electrum · 2018-12-15T05:44:52Z

No description provided.

electrum · 2018-12-15T05:45:35Z

sopel39 · 2018-12-17T11:03:31Z

Is this semantics defined by SQL standard or this is Presto extension?

findepi · 2018-12-17T22:45:13Z

presto-main/src/main/java/com/facebook/presto/metadata/ViewDefinition.java

            @JsonProperty("columns") List<ViewColumn> columns,
-            @JsonProperty("owner") Optional<String> owner)
+            @JsonProperty("owner") Optional<String> owner,
+            @JsonProperty("runAsInvoker") boolean runAsInvoker)


Do i understand correctly that existing view definitions will be parsed correctly because Jackson defaults missing boolean properties to false?

Am i right that ViewDefinition is what gets persisted in a Metastore?
if so, we should have tests like:

json literal → deserialization → check parsed correctly

to ensure backwards compat with view definitions stored by older Presto vesions

Yes, existing views will parse correctly. Good point -- I will add a test for existing views.

findepi · 2018-12-17T22:47:06Z

presto-main/src/main/java/com/facebook/presto/metadata/ViewDefinition.java

        this.columns = ImmutableList.copyOf(requireNonNull(columns, "columns is null"));
        this.owner = requireNonNull(owner, "owner is null");
+        this.runAsInvoker = runAsInvoker;
+        checkArgument(!runAsInvoker || !owner.isPresent(), "owner cannot be present with runAsInvoker");


Why owner was optional until now?
other than that, i would expect checkArgument(runAsInvoker == !owner.isPresent() here

It was optional because legacy views from before owner was added will not have them. I will also add a test for that.

That's what i suspected. Having a test would have a bonus advantage that it would self-document

findepi · 2018-12-17T22:50:50Z

presto-parser/src/main/java/com/facebook/presto/sql/parser/AstBuilder.java

    {
+        Optional<CreateView.Security> security = Optional.empty();
+        if (context.DEFINER() != null) {
+            security = Optional.of(CreateView.Security.DEFINER);


In case of an { INNER? | OUTER } case, AstBuilder applies "defaulting", i.e. determines that the Join is INNER when not explicitly specified.
Please explain why you don't follow the same path for CREATE VIEW. This would simplify the code, with the obvious drawback that SqlFormatter wouldn't be able to reconstruct the original text 1-1 (not sure if this is important though). Any other reasons?

@martint has the opinion that in general, the AST should be a faithful representation of the SQL syntax and not have knowledge of semantics. Where to draw the line is a bit arbitrary. For example, we agree that "noise words" like the optional AS clause for table aliases can be omitted. I'll let him comment further.

rschlussel

Can you add more description to the commit message (like what's in the docs).

Also, is the syntax defined by the SQL standard? I think it's a bit clunky because it doesn't make sense when you read it aloud as a sentence. So if it's not required to be exactly this way, I'd prefer something more similar what we have for table properties WITH SECURITY =<security_type>.

rschlussel · 2018-12-17T22:41:07Z

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java

            if (view != null) {
                ViewDefinition definition = deserializeView(view.getViewData());
-                if (view.getOwner().isPresent()) {
+                if (view.getOwner().isPresent() && !definition.isRunAsInvoker()) {


how could a view have an owner if runAsInvoker is also set?

This is ConnectorViewDefinition which comes from the connector, not the serialized view. Connectors will set this unconditionally (they have no knowledge of the view security model). For example, in Hive, this is always set to the table owner returned by the metastore.

Ah, I think I understand now. Let me now if this is correct.
Viewdata doesn't contain information about the owner (for legacy views only?), so it's passed only with the connector view information. Therefore we need some other field to say whether we want to get the view owner from the connector or not, and that's why we you added the "runAsInvoker" boolean.

Yes, exactly

rschlussel · 2018-12-17T22:41:42Z

presto-main/src/main/java/com/facebook/presto/metadata/ViewDefinition.java

            @JsonProperty("columns") List<ViewColumn> columns,
-            @JsonProperty("owner") Optional<String> owner)
+            @JsonProperty("owner") Optional<String> owner,
+            @JsonProperty("runAsInvoker") boolean runAsInvoker)


What's the difference between when owner is empty and runAsInvoker is false and when owner is empty and runAsInvoker is true?

Legacy views from before owner was added will have neither of these fields. I had originally wanted to simply use "owner not present" to indicate invoker security, but that doesn't work due to legacy views.

But what's the difference in behavior? Won't both of them use regular access control?

I believe you answered your own question above.

electrum · 2018-12-17T23:35:11Z

This is a Presto extension to the SQL standard. The grammar is based on the standard for SQL routines which support both modes.

electrum · 2018-12-17T23:55:55Z

Here's an example of the standard syntax for CREATE PROCEDURE that this syntax is based on:

CREATE PROCEDURE account_count()
SQL SECURITY INVOKER
BEGIN
  SELECT count(*) FROM mysql.user;
END;

Note that the word SQL is used to differentiate from EXTERNAL, which are two different security models. I don't think we need for views, since they always use the SQL security model.

findepi · 2018-12-18T13:22:36Z

FWIW, Oracle has equivalent functionality using BEQUEATH keyword (https://docs.oracle.com/database/121/DBSEG/dr_ir.htm#DBSEG668):

CREATE VIEW view_name [ BEQUEATH { DEFINER | CURRENT_USER } ] AS ...

Teradata, SQL Server and PostgreSQL seem not to have the functionality.

electrum · 2018-12-18T21:59:57Z

@findepi Nice find, I missed that. Should we use that syntax instead, both for familiarity/compatibility, and given that it is now the likely choice if the behavior is ever standardized?

findepi · 2018-12-19T14:55:55Z

@electrum i don't know. I am OK whatever you and @martint agree on.

BTW my second thought is with "invoker" term. It's suitable for procedures. Should we say you invoke a view?

martint · 2018-12-19T17:11:55Z

I was having similar thoughts as @findepi. Views are not “invoked” or “called”, so the term “invoker” doesn’t seem appropriate.

There’s also the more neutral “authid” syntax in some databases (I don’tremember if it was oracle, db2 or sql server that had it)

electrum · 2018-12-20T02:05:35Z

Oracle uses AUTHID DEFINER or AUTHID CURRENT_USER (mentioned on the page linked above) when creating procedures. It is interesting that they chose a different syntax for this feature added recently in Oracle 12c (2013) when the other syntax has been around since Oracle 8 (1997).

electrum · 2018-12-20T02:18:01Z

SQL Server has a WITH options for procedures: https://docs.microsoft.com/en-us/sql/t-sql/statements/create-procedure-transact-sql?view=sql-server-2017

It looks like the syntax would be one of

CREATE PROCEDURE abc WITH EXECUTE AS OWNER ...
CREATE PROCEDURE abc WITH EXECUTE AS USER ...

There is appeal to having a WITH clause (as @rschlussel mentioned) and I also prefer OWNER to DEFINER since that directly matches the owner concept in SQL. However, in practice, this syntax seems rather verbose (even without the WITH), and the repeated AS clause isn't great:

CREATE VIEW abc WITH EXECUTE AS OWNER AS ...
CREATE VIEW abc WITH EXECUTE AS CURRENT_USER AS ...

electrum · 2018-12-20T02:19:41Z

This is how the AUTHID would look using WITH:

CREATE VIEW abc WITH AUTHID OWNER AS ...
CREATE VIEW abc WITH AUTHID CURRENT_USER AS ...

But when thinking about AUTHID as a word, I actually don't know what it's supposed to mean.

kokosing · 2018-12-20T08:14:42Z

What about:

CREATE VIEW abc WITH EXECUTE BY OWNER AS ...
CREATE VIEW abc WITH EXECUTE BY CURRENT_USER AS ...

?

That way AS is not repeated.

From the other hand, maybe repeating AS is a better idea, because user does not need to think which preposition they have to use.

electrum · 2019-01-03T07:22:43Z

Coming back to this with fresh eyes, I think we should copy the Oracle syntax for creating views, both for user familiarity and because it is the likely choice to be standardized (if this feature ever is).

Add invoker security mode for views

a2c3a1f

facebook-github-bot added the CLA Signed label Dec 15, 2018

electrum requested a review from dain December 15, 2018 05:45

electrum added the syntax-needs-review label Dec 15, 2018

findepi reviewed Dec 17, 2018

View reviewed changes

rschlussel reviewed Dec 17, 2018

View reviewed changes

electrum closed this Jan 23, 2019

electrum deleted the view-security branch January 23, 2019 05:38

electrum mentioned this pull request Jan 23, 2019

Add current user security mode for views trinodb/trino#30

Merged

Conversation

electrum commented Dec 15, 2018

Uh oh!

electrum commented Dec 15, 2018

Uh oh!

sopel39 commented Dec 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rschlussel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

electrum commented Dec 17, 2018

Uh oh!

electrum commented Dec 17, 2018

Uh oh!

findepi commented Dec 18, 2018

Uh oh!

electrum commented Dec 18, 2018

Uh oh!

findepi commented Dec 19, 2018

Uh oh!

martint commented Dec 19, 2018

Uh oh!

electrum commented Dec 20, 2018

Uh oh!

electrum commented Dec 20, 2018

Uh oh!

electrum commented Dec 20, 2018

Uh oh!

kokosing commented Dec 20, 2018

Uh oh!

electrum commented Jan 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants