Add readOnly access level to file-based system access control #1153

bill-warshaw · 2019-07-21T19:15:43Z

add a read-only field to the file-based system access control mechanism
optional, defaults to false if not specified
first rule which has read-only field explicitly set will be honored, from top to bottom

https://prestosql.io/docs/current/security/built-in-system-access-control.html

findepi

@bill-warshaw this is nice. Some early feedback.

findepi · 2019-07-21T19:27:24Z

presto-main/src/main/java/io/prestosql/security/CatalogAccessControlRule.java

    private final Optional<Pattern> userRegex;
    private final Optional<Pattern> catalogRegex;

    @JsonCreator
    public CatalogAccessControlRule(
            @JsonProperty("allow") boolean allow,
+            @JsonProperty("readOnly") boolean readOnly,


Can we think of any other access modes in the future? If yes, this would be an enum.

Then in JSON this could look like:

"access": "READ_ONLY",

cc @electrum

I didn't come up with any future extensions so in JSON this may be represented as "readOnly": true (as implemented), but maybe @electrum has some other thoughts.

presto-main/src/main/java/io/prestosql/security/CatalogAccessControlRule.java

findepi

@bill-warshaw thanks for updating the code and sorry for the review delay.
A couple of editorial comments.

presto-main/src/main/java/io/prestosql/security/CatalogAccessControlRule.java

presto-main/src/main/java/io/prestosql/security/FileBasedSystemAccessControl.java

findepi · 2019-07-26T12:00:24Z

presto-main/src/main/java/io/prestosql/security/CatalogAccessControlRule.java

    private final Optional<Pattern> userRegex;
    private final Optional<Pattern> catalogRegex;

    @JsonCreator
    public CatalogAccessControlRule(
            @JsonProperty("allow") boolean allow,
+            @JsonProperty("readOnly") boolean readOnly,


I didn't come up with any future extensions so in JSON this may be represented as "readOnly": true (as implemented), but maybe @electrum has some other thoughts.

bill-warshaw · 2019-08-05T21:36:32Z

@findepi comments addressed, and this passes tests for me locally. Thanks for the review!

Should I also update the docs as well? Or is there a more formal process for doc updates?

electrum · 2019-08-06T07:14:38Z

Yes, please update the docs as part of the same commit (in the presto-docs module).

bill-warshaw · 2019-08-06T17:41:12Z

initial docs added, also renamed it to read-only to be consistent with the existing read-only system-level access control mechanism. Open to feedback on the docs

dain · 2019-08-26T18:45:35Z

@findepi @electrum @martint: for this PR we are introducing a "read-only" concept to the file based access control. This is in addition to the existing allow-all/deny-all we currently have. Should we instead go down the path of specifying actual GRANT actions like SELECT, INSERT, DELETE?

dain

This change looks good to me. I'd like to hear from others on the approach before merging.

dain · 2019-08-26T18:50:53Z

...plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/CatalogAccessControlRule.java

            @JsonProperty("user") Optional<Pattern> userRegex,
            @JsonProperty("catalog") Optional<Pattern> catalogRegex)
    {
        this.allow = allow;
+        this.readOnly = readOnly;


Should we enforce that readOnly can only be set when allow is set?

Make sense. Or we could turn allow into a tri-state flag "allow" | "deny" | "read-only" with true | false as legacy options.

I like the thought process, but these names look strange:

"allow": "allow" "allow": "deny"

Maybe "all" | "read-only" | "none"

Or keep true/false and add read-only as the third option. (having multiple values for the same state is easy with an enum)

If we want different names than booleans, we could do something like

enum AccessMode { ALL("all", "true"), READ_ONLY("read-only"), NONE("none", "false"), ... }

That would be great! And, as I understand, you need something like

@JsonCreator public static AccessMode fromString(String value) { ... }

right?

findepi · 2019-08-26T21:00:09Z

for this PR we are introducing a "read-only" concept to the file based access control. This is in addition to the existing allow-all/deny-all we currently have. Should we instead go down the path of specifying actual GRANT actions like SELECT, INSERT, DELETE?

Since this can be managed by hand, I prefer a simplified model (no access | full access | read-only).

presto-docs/src/main/sphinx/security/built-in-system-access-control.rst

electrum · 2019-08-27T04:20:08Z

...plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/CatalogAccessControlRule.java

            @JsonProperty("user") Optional<Pattern> userRegex,
            @JsonProperty("catalog") Optional<Pattern> catalogRegex)
    {
        this.allow = allow;
+        this.readOnly = readOnly;


I like the thought process, but these names look strange:

"allow": "allow" "allow": "deny"

Maybe "all" | "read-only" | "none"

Or keep true/false and add read-only as the third option. (having multiple values for the same state is easy with an enum)

electrum · 2019-08-27T04:22:53Z

...plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/CatalogAccessControlRule.java

            @JsonProperty("user") Optional<Pattern> userRegex,
            @JsonProperty("catalog") Optional<Pattern> catalogRegex)
    {
        this.allow = allow;
+        this.readOnly = readOnly;


If we want different names than booleans, we could do something like

enum AccessMode { ALL("all", "true"), READ_ONLY("read-only"), NONE("none", "false"), ... }

dain · 2019-08-27T19:43:24Z

@bill-warshaw What do you think about @electrum's suggestion to change allow to "all" | "read-only" | "none"? If you do, do you have time to make that change? If not, I can make the change.

bill-warshaw · 2019-08-27T19:52:41Z

@dain I would be open to it, but I'm not entirely sure how to do it cleanly. We'd end up with a JSON field that could be of either type boolean or string, which wouldn't map nicely to a POJO, since we have to support the following values:

true
false
"all"
"read-only"
"none"

Is there an easy way to model this kind of union type with Jackson annotations?

dain · 2019-08-27T20:17:49Z

Edit: fixed it to support real booleans :)

@bill-warshaw This should do it: https://gist.github.com/dain/720382a08898e4c54f80ff80bdd8b9d3

bill-warshaw · 2019-08-28T01:20:10Z

@electrum @dain comments addressed, I used @dain 's code snippet and it definitely feels cleaner.

findepi

(not full review; i didn't review tests,docs)

presto-main/src/test/resources/catalog.json

presto-main/src/test/java/io/prestosql/security/TestFileBasedSystemAccessControl.java

...plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/CatalogAccessControlRule.java

...in-toolkit/src/main/java/io/prestosql/plugin/base/security/FileBasedSystemAccessControl.java

findepi · 2019-08-28T06:33:48Z

@bill-warshaw FYI TestFileBasedSystemAccessControl failed on Travis

[ERROR] testCatalogOperations(io.prestosql.plugin.base.security.TestFileBasedSystemAccessControl)  Time elapsed: 0.027 s  <<< FAILURE!
java.lang.IllegalArgumentException: Invalid JSON file '/home/travis/build/prestosql/presto/presto-plugin-toolkit/target/test-classes/catalog.json' for 'class io.prestosql.plugin.base.security.FileBasedSystemAccessControlRules'
	at io.prestosql.plugin.base.JsonUtils.parseJson(JsonUtils.java:49)
	at io.prestosql.plugin.base.security.FileBasedSystemAccessControl$Factory.create(FileBasedSystemAccessControl.java:134)
	at io.prestosql.plugin.base.security.FileBasedSystemAccessControl$Factory.create(FileBasedSystemAccessControl.java:122)
	at io.prestosql.plugin.base.security.TestFileBasedSystemAccessControl.newFileBasedSystemAccessControl(TestFileBasedSystemAccessControl.java:168)
	at io.prestosql.plugin.base.security.TestFileBasedSystemAccessControl.newFileBasedSystemAccessControl(TestFileBasedSystemAccessControl.java:163)
	at io.prestosql.plugin.base.security.TestFileBasedSystemAccessControl.testCatalogOperations(TestFileBasedSystemAccessControl.java:102)

(+2 other fails in the class)

bill-warshaw · 2019-08-28T15:16:33Z

@findepi comments addressed, open question on whether or not allow being documented as required but not actually required in code is behavior we want to maintain.

Tests passed locally for me with the most recent changes. I thought they worked locally the last time I pushed but I've had some weirdness with my local build so I might have been running against stale code.

findepi

LGTM (not full review; i didn't review tests,docs)

@dain ?

dain · 2019-08-30T23:52:22Z

There is a test failure that looks related:

[ERROR]   TestFileBasedSystemAccessControl.testCanSetUserOperations:95->newFileBasedSystemAccessControl:163->newFileBasedSystemAccessControl:168 Â» IllegalArgument
[ERROR]   TestFileBasedSystemAccessControl.testCatalogOperations:102->newFileBasedSystemAccessControl:163->newFileBasedSystemAccessControl:168 Â» IllegalArgument
[ERROR]   TestFileBasedSystemAccessControl.testRefreshing:132 Â» IllegalArgument Invalid ...

Here is a stack:

[ERROR] testRefreshing(io.prestosql.plugin.base.security.TestFileBasedSystemAccessControl)  Time elapsed: 0.044 s  <<< FAILURE!
java.lang.IllegalArgumentException: Invalid JSON file '/tmp/a0f727ae-10b9-439d-bf1e-af4d3de397ac.txt' for 'class io.prestosql.plugin.base.security.FileBasedSystemAccessControlRules'
	at io.prestosql.plugin.base.JsonUtils.parseJson(JsonUtils.java:49)
	at io.prestosql.plugin.base.security.FileBasedSystemAccessControl$Factory.create(FileBasedSystemAccessControl.java:136)
	at io.prestosql.plugin.base.security.FileBasedSystemAccessControl$Factory.lambda$create$0(FileBasedSystemAccessControl.java:119)
	at com.google.common.base.Suppliers$ExpiringMemoizingSupplier.get(Suppliers.java:241)
	at io.prestosql.plugin.base.security.ForwardingSystemAccessControl$1.delegate(ForwardingSystemAccessControl.java:44)
	at io.prestosql.plugin.base.security.ForwardingSystemAccessControl.checkCanCreateView(ForwardingSystemAccessControl.java:192)
	at io.prestosql.plugin.base.security.TestFileBasedSystemAccessControl.testRefreshing(TestFileBasedSystemAccessControl.java:132)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:645)
	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused by: com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot construct instance of `io.prestosql.plugin.base.security.CatalogAccessControlRule`, problem: accessMode is null
 at [Source: (byte[])"{
  "catalogs": [
    {
      "user": "admin",
      "catalog": ".*",
      "allow": true
    },
    {
      "catalog": "secret",
      "allow": false
    },
    {
      "user": ".*",
      "catalog": "open-to-all",
      "allow": true
    },
    {
      "catalog": "all-allowed",
      "allow": true
    },
    {
      "user": "alice",
      "catalog": "alice-catalog",
      "allow": true
    },
    {
      "user": "bob",
      "catalog": "alice-catalog",
      "allow": false
    },
    {
      ""[truncated 147 bytes]; line: 33, column: 5] (through reference chain: io.prestosql.plugin.base.security.FileBasedSystemAccessControlRules["catalogs"]->java.util.ArrayList[6])
	at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:67)
	at com.fasterxml.jackson.databind.DeserializationContext.instantiationException(DeserializationContext.java:1608)
	at com.fasterxml.jackson.databind.deser.std.StdValueInstantiator.wrapAsJsonMappingException(StdValueInstantiator.java:484)
	at com.fasterxml.jackson.databind.deser.std.StdValueInstantiator.rewrapCtorProblem(StdValueInstantiator.java:503)
	at com.fasterxml.jackson.databind.deser.std.StdValueInstantiator.createFromObjectWith(StdValueInstantiator.java:285)
	at com.fasterxml.jackson.databind.deser.ValueInstantiator.createFromObjectWith(ValueInstantiator.java:229)
	at com.fasterxml.jackson.databind.deser.impl.PropertyBasedCreator.build(PropertyBasedCreator.java:195)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeUsingPropertyBased(BeanDeserializer.java:488)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1287)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:326)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:159)
	at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer.deserialize(CollectionDeserializer.java:286)
	at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer.deserialize(CollectionDeserializer.java:245)
	at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer.deserialize(CollectionDeserializer.java:27)
	at com.fasterxml.jackson.databind.deser.std.ReferenceTypeDeserializer.deserialize(ReferenceTypeDeserializer.java:185)
	at com.fasterxml.jackson.databind.deser.SettableBeanProperty.deserialize(SettableBeanProperty.java:530)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeWithErrorWrapping(BeanDeserializer.java:528)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeUsingPropertyBased(BeanDeserializer.java:417)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1287)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:326)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:159)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4013)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3091)
	at io.prestosql.plugin.base.JsonUtils.parseJson(JsonUtils.java:46)
	... 19 more
Caused by: java.lang.NullPointerException: accessMode is null
	at java.util.Objects.requireNonNull(Objects.java:228)
	at io.prestosql.plugin.base.security.CatalogAccessControlRule.<init>(CatalogAccessControlRule.java:41)
	at sun.reflect.GeneratedConstructorAccessor5.newInstance(Unknown Source)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at com.fasterxml.jackson.databind.introspect.AnnotatedConstructor.call(AnnotatedConstructor.java:124)
	at com.fasterxml.jackson.databind.deser.std.StdValueInstantiator.createFromObjectWith(StdValueInstantiator.java:283)
	... 38 more

bill-warshaw · 2019-09-02T21:57:13Z

@dain I'm working on reproducing this locally, but all of the tests under TestFileBasedSystemAccessControl pass locally for me, both from Maven and from IntelliJ (on OSX).

I could have sworn I saw all TestFileBasedSystemAccessControl pass at least once on Travis recently, but there are multiple recent runs where these tests fail so something's clearly off here.

I'll post again once I figure it out.

bill-warshaw · 2019-09-02T23:12:38Z

@dain figured it out - there are two test classes named TestFileBasedSystemAccessControl which both exercise file-based system access control, one in presto-main, and one in presto-plugin-toolkit. Both share some logic and have similar resource files, but also appear to have diverged before this PR.

To fix these tests I just updated one of the resource files under presto-plugin-toolkit to remove the catalog which did not have the allow value set; should we try to merge these two test classes instead?

findepi · 2019-09-03T10:20:42Z

there are two test classes named TestFileBasedSystemAccessControl which both exercise file-based system access control, one in presto-main, and one in presto-plugin-toolkit. Both share some logic and have similar resource files, but also appear to have diverged before this PR.

@kokosing can you please comment?

should we try to merge these two test classes instead?

even if we decide to merge the classes, I'd rather have it in a separate (follow-up) PR

kokosing · 2019-09-03T10:46:21Z

should we try to merge these two test classes instead?

Please don't. The test file based access control on two different levels. Directly and via access control manager. There is a contract between these two, like access control manager is not going to ask to access control for access to a table if it is known that there is no access to schema to which given table belongs.

findepi · 2019-09-03T19:15:23Z

...plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/CatalogAccessControlRule.java

+
+        @JsonValue
+        @Override
+        public String toString()


I'd prefer not to override toString() (nor name()) and have a dedicated method for this. Maybe getStringValue().

@electrum WDYT?

* adds a `read-only` option to the file-based system access control mechanism * three roles exist for `allow`: `all`, `read-only`, `none` * legacy `allow` values of `true | false` map to `all` and `none` * `allow` can be set to `all | read-only | none`

* create, rename and drop schema operations can now be controlled by the file-based SystemAccessControl mechanism * enabled when allow is set to `all`, disabled when `allow` is set to `none` or `read-only`

findepi · 2019-09-06T09:18:14Z

Merged, thanks!

cla-bot bot added the cla-signed label Jul 21, 2019

findepi reviewed Jul 21, 2019

View reviewed changes

bill-warshaw force-pushed the file_based_read_only_access_control branch 2 times, most recently from a3f4469 to 536c3eb Compare July 21, 2019 22:16

findepi reviewed Jul 26, 2019

View reviewed changes

bill-warshaw force-pushed the file_based_read_only_access_control branch 3 times, most recently from fcf2d39 to cb3d97a Compare August 5, 2019 21:33

bill-warshaw force-pushed the file_based_read_only_access_control branch from cb3d97a to da0f528 Compare August 6, 2019 03:48

bill-warshaw force-pushed the file_based_read_only_access_control branch from da0f528 to 431e5b3 Compare August 6, 2019 15:39

findepi requested a review from electrum August 6, 2019 20:33

bill-warshaw force-pushed the file_based_read_only_access_control branch 2 times, most recently from 4d3136a to 047d4bf Compare August 14, 2019 21:48

dain self-assigned this Aug 16, 2019

dain requested review from dain and removed request for electrum August 16, 2019 23:15

dain approved these changes Aug 26, 2019

View reviewed changes

electrum reviewed Aug 27, 2019

View reviewed changes

bill-warshaw force-pushed the file_based_read_only_access_control branch from 047d4bf to 786a748 Compare August 28, 2019 01:18

bill-warshaw force-pushed the file_based_read_only_access_control branch from 786a748 to 74b7432 Compare August 28, 2019 01:20

findepi reviewed Aug 28, 2019

View reviewed changes

bill-warshaw force-pushed the file_based_read_only_access_control branch from 74b7432 to 905f770 Compare August 28, 2019 15:14

findepi reviewed Aug 28, 2019

View reviewed changes

bill-warshaw force-pushed the file_based_read_only_access_control branch from 905f770 to 775cf45 Compare August 29, 2019 13:48

bill-warshaw force-pushed the file_based_read_only_access_control branch from 775cf45 to 27ec530 Compare September 2, 2019 23:10

findepi approved these changes Sep 3, 2019

View reviewed changes

bill-warshaw force-pushed the file_based_read_only_access_control branch from 27ec530 to ab6001c Compare September 6, 2019 00:05

Add schema checks to file-based SystemAccessControl

2f02151

* create, rename and drop schema operations can now be controlled by the file-based SystemAccessControl mechanism * enabled when allow is set to `all`, disabled when `allow` is set to `none` or `read-only`

bill-warshaw force-pushed the file_based_read_only_access_control branch from ab6001c to 2f02151 Compare September 6, 2019 00:06

findepi merged commit 78974ab into trinodb:master Sep 6, 2019

findepi mentioned this pull request Sep 6, 2019

Release notes for 319 #1378

Closed

6 tasks

bill-warshaw deleted the file_based_read_only_access_control branch September 6, 2019 14:26

martint added this to the 319 milestone Sep 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add readOnly access level to file-based system access control #1153

Add readOnly access level to file-based system access control #1153

bill-warshaw commented Jul 21, 2019 •

edited

findepi left a comment

findepi Jul 21, 2019

findepi Jul 21, 2019

findepi Jul 26, 2019

findepi left a comment

findepi Jul 26, 2019

bill-warshaw commented Aug 5, 2019

electrum commented Aug 6, 2019

bill-warshaw commented Aug 6, 2019

dain commented Aug 26, 2019

dain left a comment

dain Aug 26, 2019

findepi Aug 26, 2019

electrum Aug 27, 2019

electrum Aug 27, 2019

findepi Aug 27, 2019

findepi commented Aug 26, 2019

electrum Aug 27, 2019

electrum Aug 27, 2019

dain commented Aug 27, 2019

bill-warshaw commented Aug 27, 2019

dain commented Aug 27, 2019 •

edited

bill-warshaw commented Aug 28, 2019 •

edited

findepi left a comment

findepi commented Aug 28, 2019

bill-warshaw commented Aug 28, 2019

findepi left a comment

dain commented Aug 30, 2019

bill-warshaw commented Sep 2, 2019 •

edited

bill-warshaw commented Sep 2, 2019

findepi commented Sep 3, 2019

kokosing commented Sep 3, 2019

findepi Sep 3, 2019

findepi commented Sep 6, 2019

Add readOnly access level to file-based system access control #1153

Add readOnly access level to file-based system access control #1153

Conversation

bill-warshaw commented Jul 21, 2019 • edited

findepi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bill-warshaw commented Aug 5, 2019

electrum commented Aug 6, 2019

bill-warshaw commented Aug 6, 2019

dain commented Aug 26, 2019

dain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dain commented Aug 27, 2019

bill-warshaw commented Aug 27, 2019

dain commented Aug 27, 2019 • edited

bill-warshaw commented Aug 28, 2019 • edited

findepi left a comment

Choose a reason for hiding this comment

findepi commented Aug 28, 2019

bill-warshaw commented Aug 28, 2019

findepi left a comment

Choose a reason for hiding this comment

dain commented Aug 30, 2019

bill-warshaw commented Sep 2, 2019 • edited

bill-warshaw commented Sep 2, 2019

findepi commented Sep 3, 2019

kokosing commented Sep 3, 2019

Choose a reason for hiding this comment

findepi commented Sep 6, 2019

bill-warshaw commented Jul 21, 2019 •

edited

dain commented Aug 27, 2019 •

edited

bill-warshaw commented Aug 28, 2019 •

edited

bill-warshaw commented Sep 2, 2019 •

edited