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

Cleanup file access control #5039

Merged
merged 20 commits into from
Oct 7, 2020
Merged

Cleanup file access control #5039

merged 20 commits into from
Oct 7, 2020

Conversation

dain
Copy link
Member

@dain dain commented Sep 1, 2020

No description provided.

@dain dain requested a review from electrum September 1, 2020 04:22
@cla-bot cla-bot bot added the cla-signed label Sep 1, 2020
@dain dain force-pushed the cleanup-file-access-control branch from ee6e12c to 2c49db5 Compare September 2, 2020 04:07
@dain dain requested a review from kokosing September 2, 2020 04:07
@dain dain force-pushed the cleanup-file-access-control branch from 2c49db5 to 9347c2e Compare September 2, 2020 18:07
kokosing
kokosing previously approved these changes Sep 2, 2020
View reviewed changes
@dain dain force-pushed the cleanup-file-access-control branch 3 times, most recently from 6bbb705 to 5e2da1a Compare September 3, 2020 00:50
@dain dain force-pushed the cleanup-file-access-control branch 2 times, most recently from ab29061 to 12762aa Compare September 17, 2020 03:55
@dain dain requested a review from kokosing September 17, 2020 04:02
@dain dain force-pushed the cleanup-file-access-control branch from 12762aa to 92cc3e5 Compare September 17, 2020 04:24
@mosabua mosabua added the docs label Sep 17, 2020
mosabua
mosabua requested changes Sep 18, 2020
View reviewed changes
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

A bunch of minor suggestions for docs submitted. I have not looked at the code...

@dain dain force-pushed the cleanup-file-access-control branch from 92cc3e5 to db77636 Compare September 24, 2020 00:50
kokosing
kokosing reviewed Sep 24, 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.

LGTM for first 3 commits. Up to Change catalog check to match other checks. If you could merge them, then it will be easier to continue with that. It is quite hard for me to digest it all at once.

presto-plugin-toolkit/src/test/resources/file-based-system-no-access.json Show resolved Hide resolved
...in-toolkit/src/main/java/io/prestosql/plugin/base/security/FileBasedSystemAccessControl.java Show resolved Hide resolved
...o-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/FileBasedAccessControl.java Show resolved Hide resolved
presto-docs/src/main/sphinx/security/file-system-access-control.rst Show resolved Hide resolved
...ugin-toolkit/src/test/java/io/prestosql/plugin/base/security/TestFileBasedAccessControl.java Show resolved Hide resolved
...oolkit/src/test/java/io/prestosql/plugin/base/security/TestFileBasedSystemAccessControl.java Show resolved Hide resolved
@kokosing kokosing mentioned this pull request Sep 24, 2020
Closed
@dain
Copy link
Member Author

dain commented Sep 29, 2020

@kokosing I prefer to wait until you are done, and then I will merge... the first couple are simple cleanups, and I don't really want to split the PR.

@dain dain requested a review from kokosing September 29, 2020 17:21
@dain dain dismissed kokosing’s stale review September 29, 2020 17:21

Code has been significantly updated since review

kokosing
kokosing approved these changes Sep 30, 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.

Just few comments.

presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/ImpersonationRule.java Outdated Show resolved Hide resolved
...o-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/FileBasedAccessControl.java Show resolved Hide resolved
...in-toolkit/src/main/java/io/prestosql/plugin/base/security/FileBasedSystemAccessControl.java Show resolved Hide resolved
...ugin-toolkit/src/test/java/io/prestosql/plugin/base/security/TestFileBasedAccessControl.java Show resolved Hide resolved
...oolkit/src/test/java/io/prestosql/plugin/base/security/TestFileBasedSystemAccessControl.java Show resolved Hide resolved
@dain dain force-pushed the cleanup-file-access-control branch from db77636 to f9fa302 Compare October 7, 2020 00:22
dain added 20 commits October 6, 2020 17:27
@dain
Simplify FileBasedAccessControl
334fc3d
@dain
Fix warnings in FileBasedSystemAccessControl
6611817
@dain
Change catalog check to match other checks
e10a04b 
Place check catalog method near other checks
@dain
Simplify catalog checks in FileBasedSystemAccessControl
06b1afc
@dain
Simplify query checks in FileBasedSystemAccessControl
9e4dc78
@dain
Clean up exception messages in FileBasedSystemAccessControl
4f846b2
@dain
Change file access control to consistently use snake case
1c65efa 
Old camel case names are still supported
@dain
Fix file access control for table create and rename checks
162f3fe 
Changed file based access control to check for table ownership to create,
rename, or drop tables or views.
@dain
Fix system file create schema check
a5a0325
@dain
Add missing table checks in file system access control
78f61cd
@dain
Filter tables in file access control
4b83065 
Only show tables the user has permissions on
@dain
Add catalog regex to schema and table rules in file system access con…
cf812ba 
…trol
@dain
Allow all catalog access by default in file-based access control
41aa1f9 
If no catalog rules are defined, allow all catalog access
@dain
Simplify table and schema rules in file system access control
a65adc6 
Table and schema default to allow if there are no rules defined, so replace
Optional empty with an allow allow rule
@dain
Change catalog file access default to allow for no rules
4c24f4f 
System access control defaults to allow if no rules are defined for table,
schema, and session property
@dain
Only show catalog if user has schema or table permissions
6039d26
@dain
Only show schema if user is schema owner or has table permissions
a94a921
@dain
Deny grant and revoke for catalog file access control
7cc4a2b 
Catalog file access control is static and not used in combination with other
access control systems, so grant and revoke are denied.
@dain
Add session property rules to system file access control
b8da3dd
@dain
Improve system file based access documentation
a4bc95a
@dain dain force-pushed the cleanup-file-access-control branch from f9fa302 to a4bc95a Compare October 7, 2020 00:27
@dain dain merged commit 9e6e36f into trinodb:master Oct 7, 2020
@dain dain deleted the cleanup-file-access-control branch October 7, 2020 03:48
@dain dain mentioned this pull request Oct 7, 2020
Closed
10 tasks
@mosabua
Copy link
Member

mosabua commented Oct 7, 2020

👍

@martint martint added this to the 344 milestone Oct 10, 2020
@aweisberg aweisberg mentioned this pull request Apr 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mosabua mosabua mosabua requested changes

@kokosing kokosing kokosing approved these changes

@electrum electrum Awaiting requested review from electrum

Assignees
No one assigned
Labels
cla-signed docs
Milestone
344
Development

Successfully merging this pull request may close these issues.
4 participants
@dain @mosabua @kokosing @martint