Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adding schema access rules to FileBasedSystemAccessControl #3766

Merged
merged 1 commit into from Jun 1, 2020
Merged

Adding schema access rules to FileBasedSystemAccessControl #3766

merged 1 commit into from Jun 1, 2020

Conversation

haldes
Copy link
Contributor

@haldes haldes commented May 17, 2020

Currently FileBasedSystemAccessControl control has catalogRules, queryAccessRules, impersonationRules and principalUserMatchRules. The proposal here is to add schema access rules to it. This will give more granular access controls over the datasets.

At high level idea is to implement/enhance the below methods of FileBasedSystemAccessControl

  • checkCanDropSchema - Allow if user has access to catalog and is SchemaOwner
  • checkCanRenameSchema - Allow if user has access to catalog and is SchemaOwner
  • checkCanSetSchemaAuthorization - Allow if user has access to catalog and is SchemaOwner
  • checkCanShowCreateSchema - Allow if user has access to catalog and is SchemaOwner

Original Issue : #3733

@cla-bot
Copy link

cla-bot bot commented May 17, 2020

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

@haldes
Copy link
Contributor Author

haldes commented May 17, 2020

@findepi @kokosing
Have already sent request mail for Cla.

@haldes haldes mentioned this pull request May 17, 2020
Closed
@cla-bot
Copy link

cla-bot bot commented May 17, 2020

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

@findepi findepi requested a review from kokosing May 17, 2020 22:26
@kokosing
Copy link
Member

I think it was a deliberate choice to have table/schemas access control checked at catalog level with io.prestosql.plugin.base.security.FileBasedAccessControl and check catalog access control with io.prestosql.plugin.base.security.FileBasedSystemAccessControl.

@haldes Why above mentioned solution does not work for you?

@haldes
Copy link
Contributor Author

haldes commented May 18, 2020

Hi @kokosing,

As I understand io.prestosql.plugin.base.security.FileBasedAccessControl is only available for hive connectors. We don't have similar access controls (schema, table) for other connectors.

Please find below the slack discussion on the same
https://prestosql.slack.com/archives/CFLB9AMBN/p1588921035123100?thread_ts=1507847605.000156&cid=CFLB9AMBN

@kokosing
Copy link
Member

Thanks.

Are you going to cover access to tables and views as well?

@haldes
Copy link
Contributor Author

haldes commented May 18, 2020

Hi @kokosing - Sure. Please let me know if this PR looks good.
@findepi suggested to start with schemas so started with this PR.

kokosing
kokosing reviewed May 19, 2020
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far so good. Some initial comments

...in-toolkit/src/main/java/io/prestosql/plugin/base/security/FileBasedSystemAccessControl.java Outdated Show resolved Hide resolved
presto-plugin-toolkit/src/test/resources/file-based-system-access-schema.json Show resolved Hide resolved
...oolkit/src/test/java/io/prestosql/plugin/base/security/TestFileBasedSystemAccessControl.java Outdated Show resolved Hide resolved
...oolkit/src/test/java/io/prestosql/plugin/base/security/TestFileBasedSystemAccessControl.java Outdated Show resolved Hide resolved
@haldes
Copy link
Contributor Author

haldes commented May 20, 2020

@kokosing Thanks - Will work on it and get back.

@cla-bot cla-bot bot added the cla-signed label May 21, 2020
kokosing
kokosing approved these changes May 21, 2020
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

% few minor comments

Thanks!

...oolkit/src/test/java/io/prestosql/plugin/base/security/TestFileBasedSystemAccessControl.java Outdated
private static final SystemSecurityContext BOB = new SystemSecurityContext(bob, queryId);
private static final SystemSecurityContext CHARLIE = new SystemSecurityContext(charlie, queryId);

private static final String DROP_SCHEMA_ERR_MESSAGE = "Access Denied: Cannot drop schema";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DROP_SCHEMA_ERR_MESSAGE -> DROP_SCHEMA_ACCESS_DENIED_MESSAGE

Same for all below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is changed.

...oolkit/src/test/java/io/prestosql/plugin/base/security/TestFileBasedSystemAccessControl.java Outdated
private static final String RENAME_SCHEMA_ERR_MESSAGE = "Access Denied: Cannot rename schema from";
private static final String AUTH_SCHEMA_ERR_MESSAGE = "Access Denied: Cannot set authorization for schema";
private static final String SHOW_CREATE_SCHEMA_ERR_MESSAGE = "Access Denied: Cannot show create schema for";
private static final String INVALID_JSON_ERR_MESSAGE = "Invalid JSON file";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

INVALID_JSON_ERR_MESSAGE -> INVALID_JSON

...oolkit/src/test/java/io/prestosql/plugin/base/security/TestFileBasedSystemAccessControl.java Outdated
@@ -268,4 +354,18 @@ private String getResourcePath(String resourceName)
{
return this.getClass().getClassLoader().getResource(resourceName).getPath();
}

private static void assertDenied(ThrowingCallable callable, String expectedErrMessage)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertDenied -> assertAccessDenied
expectedErrMessage -> expectedMessage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is changed.

...oolkit/src/test/java/io/prestosql/plugin/base/security/TestFileBasedSystemAccessControl.java Outdated
{
assertThatThrownBy(callable)
.isInstanceOf(AccessDeniedException.class)
.hasMessageStartingWith(expectedErrMessage);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check that message equals or matches the pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kokosing - I have changed to hasMessageMatching() and using pattern to match. Let me know if that's ok.

...oolkit/src/test/java/io/prestosql/plugin/base/security/TestFileBasedSystemAccessControl.java Outdated Show resolved Hide resolved
kokosing
kokosing approved these changes May 25, 2020
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash all commits and rebase.

@haldes
Add schema access rules to FileBasedSystemAccessControl
e541399 
Add schema access rules to FileBasedSystemAccessControl

Add schema access rules to FileBasedSystemAccessControl

Add schema access rules to FileBasedSystemAccessControl

Add schema access rules to FileBasedSystemAccessControl

Add schema access rules to FileBasedSystemAccessControl
@haldes
Copy link
Contributor Author

haldes commented May 26, 2020

@kokosing - I have squashed and did rebase.

@kokosing
Copy link
Member

Automation hit: #2435

@kokosing kokosing mentioned this pull request May 28, 2020
Closed
@kokosing kokosing merged commit 35e8734 into trinodb:master Jun 1, 2020
@kokosing
Copy link
Member

kokosing commented Jun 1, 2020

Merged, thanks!

@kokosing kokosing mentioned this pull request Jun 1, 2020
Closed
9 tasks
@haldes haldes mentioned this pull request Jun 7, 2020
Open
@ebyhr ebyhr mentioned this pull request Aug 2, 2020
Closed
@aweisberg aweisberg mentioned this pull request Apr 9, 2021
Merged
agrawalreetika added a commit to agrawalreetika/prestodb that referenced this pull request Apr 15, 2021
@agrawalreetika @haldes
Adding schema access rules to file based system access control
94af7fc 
Cherry pick of trinodb/trino#3766

Co-authored-by: haldes <sumit.halder@gmail.com>
rschlussel pushed a commit to prestodb/presto that referenced this pull request Apr 28, 2021
@agrawalreetika @haldes @rschlussel
Adding schema access rules to file based system access control
ea8e5f4 
Cherry pick of trinodb/trino#3766

Co-authored-by: haldes <sumit.halder@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kokosing kokosing kokosing approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@haldes @kokosing