Skip to content

Add support for Metadata Restriction in Presto#25364

Merged
imjalpreet merged 3 commits into
prestodb:masterfrom
imjalpreet:metadataRestriction
Jul 21, 2025
Merged

Add support for Metadata Restriction in Presto#25364
imjalpreet merged 3 commits into
prestodb:masterfrom
imjalpreet:metadataRestriction

Conversation

@imjalpreet

@imjalpreet imjalpreet commented Jun 18, 2025

Copy link
Copy Markdown
Member

Description

Add authorization support for SHOW CREATE TABLE/VIEW, SHOW COLUMNS and DESCRIBE queries

Motivation and Context

Enhance metadata authorization support in Presto

Part of #20851

Impact

Newly introduced access control callbacks:

  • checkCanShowCreateTable
  • checkCanShowColummsMetadata
  • filterColumns

By default, the newly introduced access control callback methods enforce a deny policy.
To achieve the intended behavior, all custom implementations of SystemAccessControl or ConnectorAccessControl must explicitly implement these new callbacks.

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Security Changes
* Add authorization support for `SHOW CREATE TABLE`, `SHOW CREATE VIEW`, `SHOW COLUMNS`, and `DESCRIBE` queries

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Jun 18, 2025
@imjalpreet imjalpreet self-assigned this Jun 18, 2025
@prestodb-ci

Copy link
Copy Markdown
Contributor

@imjalpreet imported this issue as lakehouse/presto #25364

@imjalpreet imjalpreet marked this pull request as ready for review June 19, 2025 09:27
@imjalpreet imjalpreet requested a review from hantangwangd June 19, 2025 09:27
@prestodb-ci prestodb-ci requested review from a team and ShahimSharafudeen and removed request for a team June 19, 2025 09:27
@imjalpreet imjalpreet force-pushed the metadataRestriction branch from 5de7450 to 172f0c9 Compare June 25, 2025 21:10
@steveburnett

Copy link
Copy Markdown
Contributor

Should the documentation be updated for these, such as https://prestodb.io/docs/current/sql/show-create-table.html? Maybe an example? What do you think?

@agrawalreetika agrawalreetika left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR. Apart from one comment, Overall LGTM.

false,
HiveColumnConverterProvider.DEFAULT_COLUMN_CONVERTER_PROVIDER,
context.getWarningCollector(),
context.getRuntimeStats());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's reduce the redundant code and create a new method for creating MetastoreContext and reuse it across

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Refactored this in a different commit to keep it separate from actual feature commits.

agrawalreetika
agrawalreetika previously approved these changes Jul 15, 2025

@agrawalreetika agrawalreetika left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@imjalpreet

Copy link
Copy Markdown
Member Author

Should the documentation be updated for these, such as https://prestodb.io/docs/current/sql/show-create-table.html? Maybe an example? What do you think?

@steveburnett Sorry, I missed your comment earlier. This PR only adds access control checks for a few specific types of SQL statements. I’ll review whether any existing doc already covers the access control checks that were already implemented. If not, we can see if it is helpful to create a more detailed document outlining the various access control checks that Presto supports for different types of authorization.

@hantangwangd hantangwangd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the work, looks good to me overall! Just a few little things.

Comment thread presto-spi/src/main/java/com/facebook/presto/spi/security/AccessControl.java Outdated
@steveburnett

Copy link
Copy Markdown
Contributor

Should the documentation be updated for these, such as https://prestodb.io/docs/current/sql/show-create-table.html? Maybe an example? What do you think?

@steveburnett Sorry, I missed your comment earlier. This PR only adds access control checks for a few specific types of SQL statements. I’ll review whether any existing doc already covers the access control checks that were already implemented. If not, we can see if it is helpful to create a more detailed document outlining the various access control checks that Presto supports for different types of authorization.

Sounds good! I appreciate your explanation.

If you think we need a more detailed document, please open a issue with a docs label so we don't lose track of the need.

@imjalpreet

Copy link
Copy Markdown
Member Author

If you think we need a more detailed document, please open a issue with a docs label so we don't lose track of the need.

@steveburnett I verified the current docs, we haven't documented SQL statement to required access control mapping for different types of authorization. I will think about what's the best way to document it and create an issue to track that.

@imjalpreet imjalpreet requested a review from hantangwangd July 20, 2025 17:36

@hantangwangd hantangwangd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the fix, looks good to me!

@imjalpreet imjalpreet merged commit acf9156 into prestodb:master Jul 21, 2025
168 of 169 checks passed
@rschlussel

Copy link
Copy Markdown
Contributor

This needs a much clearer "impact" description in the PR and a more descriptive release note to make clear that these new methods need to be implemented by any access control plugins or else all show/describe queries will start to fail with access denied.

I also wonder if this should have defaulted to allow for Connector/System access control to keep backwards compatibility.

@imjalpreet

Copy link
Copy Markdown
Member Author

@rschlussel I have updated the PR description to highlight the impact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:IBM PR from IBM

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants