Skip to content

Comments

Adding schema access rules to FileBasedSystemAccessControl#3766

Merged
kokosing merged 1 commit intotrinodb:masterfrom
haldes:schemaSystemAccess
Jun 1, 2020
Merged

Adding schema access rules to FileBasedSystemAccessControl#3766
kokosing merged 1 commit intotrinodb:masterfrom
haldes:schemaSystemAccess

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

kokosing commented May 18, 2020

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

kokosing commented May 18, 2020

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

@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 reviewed May 21, 2020
View reviewed changes
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
Copy link
Member

@kokosing kokosing May 21, 2020

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

@haldes haldes May 21, 2020

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
Copy link
Member

@kokosing kokosing May 21, 2020

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
Copy link
Member

@kokosing kokosing May 21, 2020

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

@haldes haldes May 21, 2020

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
Copy link
Member

@kokosing kokosing May 21, 2020

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

@haldes haldes May 21, 2020

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.

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

kokosing commented May 26, 2020

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.

2 participants

@haldes @kokosing