-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Add invoker security mode for views #12082
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| import java.util.Optional; | ||
|
|
||
| import static com.google.common.base.MoreObjects.toStringHelper; | ||
| import static com.google.common.base.Preconditions.checkArgument; | ||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| public final class ViewDefinition | ||
|
|
@@ -31,20 +32,24 @@ public final class ViewDefinition | |
| private final Optional<String> schema; | ||
| private final List<ViewColumn> columns; | ||
| private final Optional<String> owner; | ||
| private final boolean runAsInvoker; | ||
|
|
||
| @JsonCreator | ||
| public ViewDefinition( | ||
| @JsonProperty("originalSql") String originalSql, | ||
| @JsonProperty("catalog") Optional<String> catalog, | ||
| @JsonProperty("schema") Optional<String> schema, | ||
| @JsonProperty("columns") List<ViewColumn> columns, | ||
| @JsonProperty("owner") Optional<String> owner) | ||
| @JsonProperty("owner") Optional<String> owner, | ||
| @JsonProperty("runAsInvoker") boolean runAsInvoker) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the difference between when owner is empty and runAsInvoker is false and when owner is empty and runAsInvoker is true?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But what's the difference in behavior? Won't both of them use regular access control?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe you answered your own question above.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do i understand correctly that existing view definitions will be parsed correctly because Jackson defaults missing boolean properties to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am i right that to ensure backwards compat with view definitions stored by older Presto vesions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, existing views will parse correctly. Good point -- I will add a test for existing views. |
||
| { | ||
| this.originalSql = requireNonNull(originalSql, "originalSql is null"); | ||
| this.catalog = requireNonNull(catalog, "catalog is null"); | ||
| this.schema = requireNonNull(schema, "schema is null"); | ||
| 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"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was optional because legacy views from before owner was added will not have them. I will also add a test for that.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what i suspected. Having a test would have a bonus advantage that it would self-document |
||
| } | ||
|
|
||
| @JsonProperty | ||
|
|
@@ -77,6 +82,12 @@ public Optional<String> getOwner() | |
| return owner; | ||
| } | ||
|
|
||
| @JsonProperty | ||
| public boolean isRunAsInvoker() | ||
| { | ||
| return runAsInvoker; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() | ||
| { | ||
|
|
@@ -86,13 +97,14 @@ public String toString() | |
| .add("schema", schema.orElse(null)) | ||
| .add("columns", columns) | ||
| .add("owner", owner.orElse(null)) | ||
| .add("runAsInvoker", runAsInvoker) | ||
| .omitNullValues() | ||
| .toString(); | ||
| } | ||
|
|
||
| public ViewDefinition withOwner(String owner) | ||
| { | ||
| return new ViewDefinition(originalSql, catalog, schema, columns, Optional.of(owner)); | ||
| return new ViewDefinition(originalSql, catalog, schema, columns, Optional.of(owner), runAsInvoker); | ||
| } | ||
|
|
||
| public static final class ViewColumn | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -369,11 +369,20 @@ public Node visitDropColumn(SqlBaseParser.DropColumnContext context) | |
| @Override | ||
| public Node visitCreateView(SqlBaseParser.CreateViewContext context) | ||
| { | ||
| Optional<CreateView.Security> security = Optional.empty(); | ||
| if (context.DEFINER() != null) { | ||
| security = Optional.of(CreateView.Security.DEFINER); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case of an
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 |
||
| } | ||
| else if (context.INVOKER() != null) { | ||
| security = Optional.of(CreateView.Security.INVOKER); | ||
| } | ||
|
|
||
| return new CreateView( | ||
| getLocation(context), | ||
| getQualifiedName(context.qualifiedName()), | ||
| (Query) visit(context.query()), | ||
| context.REPLACE() != null); | ||
| context.REPLACE() != null, | ||
| security); | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how could a view have an owner if runAsInvoker is also set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is
ConnectorViewDefinitionwhich 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly