Use AccessControl#filterColumns to filter columns from table schema #8752
Conversation
|
This would be misleading to users to return This should be behind feature toggle that is turned off by default. |
core/trino-main/src/main/java/io/trino/testing/TestingAccessControlManager.java
Outdated
Show resolved
Hide resolved
|
I agree it is much less complex. |
I agree, this can be misleading, but it's a fair price. Also, some users consider revealing object existence in case of permission denied a security issue. |
| .map(ColumnSchema::getName) | ||
| .collect(toImmutableSet())); | ||
| List<ColumnSchema> accessibleColumns = tableSchema.getColumns().stream() | ||
| .filter(column -> accessibleColumnNames.contains(column.getName())) |
There was a problem hiding this comment.
This behavior is non standard (non-specwise) AFAIK, so should be an opt-in behavior, guarded with a toggle.
There was a problem hiding this comment.
Where should the toggle knob be located, in your opinion? I imagine it could be in the SystemAccessControl implementation itself if we pass a flag when calling to tell the AccessControl whether this call is for SELECT or for metadata.
Or, should there be another method in the SystemAccessControl interface like
boolean shouldFilterColumns(TableName name) that we check first? This would give the AccessControl implementations the ability to behave differently for different tables.
I suppose the more fine grained control we give to the AccessControl implementations, the closer this becomes to the implementation in #7893 where we expose two methods and let the SystemAccessControl decide entirely when it wants to hide/throw on inaccessible columns.
I think if we really want to maintain a consistent behavior here, we should also take a look at checkCanSelectFromColumns, and maybe enforce that if a column is supposed to be inaccessible then the toggle only controls whether the behavior is to throw or hide.
There was a problem hiding this comment.
I am not sure if toggle belongs to access control. To me it belongs to the engine as it we change the semantic of SQL. Authorization works same way, but its result is interpreted differently.
There was a problem hiding this comment.
Hmmm - is there any value in allowing access control implementations to decide which tables should throw vs which tables should hide? If not, then toggling in the engine makes sense to me.
@findepi what do you think?
There was a problem hiding this comment.
I don't see any value in making this behave differently for a subset of tables.
core/trino-main/src/main/java/io/trino/security/AccessControl.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/sql/query/TestFilterColumns.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/sql/query/TestFilterColumns.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorAccessControl.java
Outdated
Show resolved
Hide resolved
youngchen7
force-pushed
the
trino-7461-alt
branch
from
4d7d0d2
to
d255da0
Compare
|
Fixes #7461 |
|
👋 @youngchen7 - this PR has become inactive. If you're still interested in working on it, please let us know, and we can try to get reviewers to help with that. We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks. |
|
Closing this one out due to inactivity, but please reopen if you would like to pick this back up. |
Alternative approach to #7893. Instead of changing the AccessControl interface, this PR reuses filterColumns to achieve the same behavior. This enforces consistency between what's visible in the metadata and what can be queried. Possible disadvantages and a topic for further discussion is ensuring that we support all existing use cases, as well as whether those use cases are reasonable. For example:
In general, there's a multiplex of possibilities between hiding, throwing, and showing metadata and data. We should confirm which ones are useful and necessary for us to support.