Update StatementAnalyzer and extend AccessControl interface and to filter columns from table schema instead of throwing #7893

youngchen7 · 2021-05-12T01:28:22Z

Initial PR for AccessControl interface changes, initial implementations, and StatementAnalyzer integration.

Will update specific AccessControl implementations (possibly FileBased) to support this feature.

cla-bot · 2021-05-12T01:28:23Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Young Chen.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email email@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2021-05-12T05:07:37Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Young Chen.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email email@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

kokosing

Please add tests.

core/trino-main/src/main/java/io/trino/security/AccessControl.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/SqlStandardAccessControl.java

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorAccessControl.java

core/trino-main/src/main/java/io/trino/security/AccessControl.java

core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControl.java

cla-bot · 2021-05-26T21:09:18Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Young Chen.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email email@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2021-05-26T21:13:05Z

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@trino.io. For more information, see https://github.com/trinodb/cla.

youngchen7 · 2021-05-26T21:18:13Z

Cleaned up the git commit history a bit to remove non github emails - just submitted the CLA as well.

youngchen7 · 2021-05-26T21:24:52Z

Please add tests.

Added some unit tests similar to rowFilter and columnMask. However, one concern I have is the interaction with INSERT/UPDATE/DELETE.

We would typically expect the AccessControl implementation to restrict access to INSERT/UPDATE/DELETE if a user doesn't have full access to the schema. However, it's possible for this to not be implemented so we would probably want a defensive check in the StatementAnalyzer.

Perhaps this would be better implemented via getTableSchemaFilteredColumns or getFilteredColumns similar to how getRowFilter and getColumnMask are done. This would let us easily check access via .isEmpty() and add restrictions in the engine itself.

Or could there be a use case for inserting partial data into a table and having the inaccessible columns set to default? It seems like this would be unexpected/error prone behavior for the most part in my opinion.

What are your thoughts?

cla-bot · 2021-06-02T14:17:03Z

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@trino.io. For more information, see https://github.com/trinodb/cla.

cla-bot · 2021-06-04T20:03:40Z

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@trino.io. For more information, see https://github.com/trinodb/cla.

cla-bot · 2021-06-04T20:15:00Z

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@trino.io. For more information, see https://github.com/trinodb/cla.

martint · 2021-06-11T21:35:53Z

@cla-bot check

cla-bot · 2021-06-11T21:35:55Z

The cla-bot has been summoned, and re-checked this pull request!

martint · 2021-06-11T21:39:53Z

core/trino-main/src/main/java/io/trino/security/AccessControl.java

@@ -222,10 +222,15 @@
    void checkCanShowColumns(SecurityContext context, CatalogSchemaTableName table);

    /**
-     * Filter the list of columns to those visible to the identity.
+     * Filter the list of columns to those visible to the identity when querying metadata.


This needs a better explanation. What's the difference between "when querying metadata" vs "when querying the table"?

filterColumns hides table metadata when running DESCRIBE or SHOW TABLE, as well as when querying the INFORMATION_SCHEMA tables. However, when actually selecting data from the table it doesn't hide the corresponding columns. I'm not really sure how else to describe this so any suggestions are welcome!

I also can't really think of a practical reason why we would want to control these separately. @kokosing mentioned that it would be beneficial to be able to hide column metadata & throw errors on access denied, vs hiding columns completely so that's why I'm leaving it here.

(Slack link: https://trinodb.slack.com/archives/CP1MUNEUX/p1620418412085600?thread_ts=1618261084.236200&cid=CP1MUNEUX)

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

kokosing

Please squash commits.

This looks good to me. I would like to hear from others if they like it. CC: @martint @findepi

core/trino-main/src/test/java/io/trino/sql/query/TestTableSchemaFilter.java

findepi · 2021-06-22T09:19:11Z

This looks good to me. I would like to hear from others if they like it. CC: @martint @findepi

if you ask about my opinion, I can only repeat that for me this shouldn't be in AccessControl at all.
#7461 (comment)
https://trinodb.slack.com/archives/CP1MUNEUX/p1620422082086900?thread_ts=1618261084.236200&cid=CP1MUNEUX

youngchen7 · 2021-06-25T23:33:33Z

Please squash commits.

This looks good to me. I would like to hear from others if they like it. CC: @martint @findepi

Ah does the merge process squash commits? I can rebase and squash all on my branch but I figured having history here would be useful.

kokosing · 2021-06-26T09:05:43Z

Ah does the merge process squash commits? I

We usually do Rebase and merge. Sometimes we do Squash and merge in case where external contributors are not well familiar with git, so we could save communication round trips.

I figured having history here would be useful.

That depends on history kind. We care a lot about the history, so this is why how change is structured between commits and what are the commit messages is also a part of code review. However we prefer not merge anything where change goes back and forth between commits in the same PR. This makes entire git history much cleaner. Notice that writing code is creative process and it is natural to got back and forth when writing the code, however when visiting history such small steps are not helpful.

@youngchen7 Would you like to add the above to https://github.com/trinodb/trino/blob/master/DEVELOPMENT.md (in separate PR)? I see it is not clear to the community members sometimes and it is not the first time I explain this. That way it will be easier for others to contribute.

youngchen7 · 2021-06-28T17:10:16Z

Ah does the merge process squash commits? I

We usually do Rebase and merge. Sometimes we do Squash and merge in case where external contributors are not well familiar with git, so we could save communication round trips.

I figured having history here would be useful.

That depends on history kind. We care a lot about the history, so this is why how change is structured between commits and what are the commit messages is also a part of code review. However we prefer not merge anything where change goes back and forth between commits in the same PR. This makes entire git history much cleaner. Notice that writing code is creative process and it is natural to got back and forth when writing the code, however when visiting history such small steps are not helpful.

@youngchen7 Would you like to add the above to https://github.com/trinodb/trino/blob/master/DEVELOPMENT.md (in separate PR)? I see it is not clear to the community members sometimes and it is not the first time I explain this. That way it will be easier for others to contribute.

Sounds good - I'll open a new PR for the DEVELOPMENT.md change. I'll squash the commits in this PR.

youngchen7 · 2021-06-28T20:24:39Z

@youngchen7 Would you like to add the above to https://github.com/trinodb/trino/blob/master/DEVELOPMENT.md (in separate PR)? I see it is not clear to the community members sometimes and it is not the first time I explain this. That way it will be easier for others to contribute.

#8411

findepi · 2021-07-29T07:35:52Z

core/trino-main/src/main/java/io/trino/security/AccessControl.java

+    /**
+     * Filter the list of columns to those visible to the identity when querying the table.
+     */
+    Set<String> filterTableSchema(SecurityContext context, QualifiedObjectName tableName, Set<String> columns);


how is this method different from filterColumns above?

Why do we need two methods?

What's the occasion where SELECT * would want to give you different set of columns than SHOW COLUMNS FROM ...

kokosing · 2021-11-29T08:57:05Z

I think we should continue with #9991, the simple feature toogle is easier to reason than changing the access control I think.

findepi · 2021-11-29T10:45:14Z

I think we should continue with #9991, the simple feature toogle is easier to reason than changing the access control I think.

@kokosing to me, it's not about simplicity, but what's the right thing to do.
per #7893 (comment) we shouldn't change AccessControl at all.

bitsondatadev · 2022-10-21T22:11:23Z

👋 @youngchen7 - this PR is inactive and doesn't seem to be under development, and it might already be implemented in #7461. If you'd like to continue work on this at any point in the future, feel free to re-open.

youngchen7 force-pushed the trino-7461 branch from cd60a61 to 7edc9b2 Compare May 12, 2021 05:07

kokosing reviewed May 12, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/security/AccessControl.java Outdated Show resolved Hide resolved

plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/SqlStandardAccessControl.java Outdated Show resolved Hide resolved

kokosing reviewed May 12, 2021

View reviewed changes

youngchen7 force-pushed the trino-7461 branch from 7edc9b2 to 7c0e1c7 Compare May 26, 2021 21:09

youngchen7 force-pushed the trino-7461 branch from 7c0e1c7 to 7d191f0 Compare May 26, 2021 21:13

youngchen7 force-pushed the trino-7461 branch from 7d191f0 to c967e22 Compare June 2, 2021 14:17

martint self-requested a review June 2, 2021 14:59

cla-bot bot added the cla-signed label Jun 11, 2021

martint reviewed Jun 11, 2021

View reviewed changes

kokosing reviewed Jun 22, 2021

View reviewed changes

core/trino-main/src/test/java/io/trino/sql/query/TestTableSchemaFilter.java Outdated Show resolved Hide resolved

core/trino-main/src/test/java/io/trino/sql/query/TestTableSchemaFilter.java Show resolved Hide resolved

youngchen7 force-pushed the trino-7461 branch 2 times, most recently from be3a750 to 2e0e909 Compare June 28, 2021 17:15

Extend AccessControl interface to filter columns from table schema

0ae41b2

youngchen7 force-pushed the trino-7461 branch from 2e0e909 to 0ae41b2 Compare June 28, 2021 18:33

martint self-requested a review June 28, 2021 20:36

findepi reviewed Jul 29, 2021

View reviewed changes

findepi force-pushed the master branch from 8538e49 to 1f896ea Compare July 30, 2021 22:13

youngchen7 mentioned this pull request Aug 2, 2021

Use AccessControl#filterColumns to filter columns from table schema #8752

Closed

youngchen7 mentioned this pull request Nov 17, 2021

Extend AccessControl functionality to filter columns from SELECT * instead of throwing #7461

Closed

findepi mentioned this pull request Nov 25, 2021

Use AccessControl functionality to filter columns from SELECT *… #9991

Merged

findepi mentioned this pull request Feb 1, 2022

MongoDB: column allow false return access denied on select * #10891

Closed

bitsondatadev closed this Oct 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update StatementAnalyzer and extend AccessControl interface and to filter columns from table schema instead of throwing #7893

Update StatementAnalyzer and extend AccessControl interface and to filter columns from table schema instead of throwing #7893

youngchen7 commented May 12, 2021 •

edited

Loading

cla-bot bot commented May 12, 2021

cla-bot bot commented May 12, 2021

kokosing left a comment

cla-bot bot commented May 26, 2021

cla-bot bot commented May 26, 2021

youngchen7 commented May 26, 2021

youngchen7 commented May 26, 2021

cla-bot bot commented Jun 2, 2021

cla-bot bot commented Jun 4, 2021

cla-bot bot commented Jun 4, 2021

martint commented Jun 11, 2021

cla-bot bot commented Jun 11, 2021

martint Jun 11, 2021

youngchen7 Jun 17, 2021

kokosing left a comment

findepi commented Jun 22, 2021

youngchen7 commented Jun 25, 2021

kokosing commented Jun 26, 2021

youngchen7 commented Jun 28, 2021

youngchen7 commented Jun 28, 2021

findepi Jul 29, 2021

kokosing commented Nov 29, 2021

findepi commented Nov 29, 2021

bitsondatadev commented Oct 21, 2022

Update StatementAnalyzer and extend AccessControl interface and to filter columns from table schema instead of throwing #7893

Update StatementAnalyzer and extend AccessControl interface and to filter columns from table schema instead of throwing #7893

Conversation

youngchen7 commented May 12, 2021 • edited Loading

cla-bot bot commented May 12, 2021

cla-bot bot commented May 12, 2021

kokosing left a comment

Choose a reason for hiding this comment

cla-bot bot commented May 26, 2021

cla-bot bot commented May 26, 2021

youngchen7 commented May 26, 2021

youngchen7 commented May 26, 2021

cla-bot bot commented Jun 2, 2021

cla-bot bot commented Jun 4, 2021

cla-bot bot commented Jun 4, 2021

martint commented Jun 11, 2021

cla-bot bot commented Jun 11, 2021

martint Jun 11, 2021

Choose a reason for hiding this comment

youngchen7 Jun 17, 2021

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

findepi commented Jun 22, 2021

youngchen7 commented Jun 25, 2021

kokosing commented Jun 26, 2021

youngchen7 commented Jun 28, 2021

youngchen7 commented Jun 28, 2021

findepi Jul 29, 2021

Choose a reason for hiding this comment

kokosing commented Nov 29, 2021

findepi commented Nov 29, 2021

bitsondatadev commented Oct 21, 2022

youngchen7 commented May 12, 2021 •

edited

Loading