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

Add config for default view security mode #21956

Merged
merged 1 commit into from
Feb 23, 2024
Merged

Add config for default view security mode #21956

merged 1 commit into from
Feb 23, 2024

Conversation

jainxrohit
Copy link
Contributor

@jainxrohit jainxrohit commented Feb 17, 2024 •
edited
Loading

Description

Presto has a default view creation security mode as 'DEFINER'. However, some administrators may prefer to use 'INVOKER' as the default.

To address this, a new configuration flag default-view-security-mode has been added that allows the default view security mode to be changed according to the administrator's preference.

== RELEASE NOTES ==

General Changes
* Add a config `default-view-security-mode` to choose the default security mode for view creation.

@jainxrohit jainxrohit force-pushed the rj_viewmode branch 2 times, most recently from 8b88f10 to eee23b6 Compare February 21, 2024 07:47
@jainxrohit jainxrohit changed the title draft - view mode Add config for default view security mode Feb 21, 2024
@jainxrohit jainxrohit marked this pull request as ready for review February 21, 2024 07:54
@jainxrohit jainxrohit requested a review from a team as a code owner February 21, 2024 07:54
@steveburnett
Copy link
Contributor

Consider adding documentation for this new configuration flag. Would Properties Reference be a good place for it?

@rschlussel
Copy link
Contributor

Consider adding documentation for this new configuration flag. Would Properties Reference be a good place for it?

https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/sql/create-view.rst#security would be a good place also.

rschlussel
rschlussel reviewed Feb 21, 2024
View reviewed changes
Copy link
Contributor

@rschlussel rschlussel left a comment

Choose a reason for hiding this comment

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

Not needed for this PR, but one other aspect of the security mode stuff that would be nice to improve is that when you run SHOW CREATE VIEW it doesn't show you the security mode that it was created with. It makes it hard to validate that any particular view is using the right security mode.

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java Outdated Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 23, 2024 •
edited
Loading

Codenotify: Notifying subscribers in CODENOTIFY files for diff c33c3a3...076c843.

Notify File(s)
@steveburnett presto-docs/src/main/sphinx/sql/create-view.rst

rschlussel
rschlussel previously approved these changes Feb 23, 2024
View reviewed changes
steveburnett
steveburnett requested changes Feb 23, 2024
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Thanks for the documentation!

presto-docs/src/main/sphinx/sql/create-view.rst
@@ -38,6 +38,9 @@ In the ``INVOKER`` security mode, tables referenced in the view are
accessed using the permissions of the query user (the *invoker* of the
view). A view created in this mode is simply a stored query.

The ``default-view-security-mode`` can be used to configure the default
security mode for view creation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding an example that shows how to use default-view-security-mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@steveburnett It can be used as any other session property. Do you have anything specific in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

The example you added works, thanks!

@jainxrohit jainxrohit dismissed stale reviews from rschlussel and NikhilCollooru via b536f3a February 23, 2024 17:36
steveburnett
steveburnett requested changes Feb 23, 2024
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Thanks for the code example! Just one nit about formatting the code example. Here's what it looks like currently in a local build of the doc:
Screenshot 2024-02-23 at 12 44 42 PM

presto-docs/src/main/sphinx/sql/create-view.rst Outdated
@@ -60,6 +63,10 @@ Create a view that replaces an existing view::
SELECT orderkey, orderstatus, totalprice / 4 AS quarter
FROM orders


Set the default view security mode to ``INVOKER``
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Set the default view security mode to ``INVOKER``
Set the default view security mode to ``INVOKER``::

Formatting suggestion to make example a code block.

presto-docs/src/main/sphinx/sql/create-view.rst
@@ -38,6 +38,9 @@ In the ``INVOKER`` security mode, tables referenced in the view are
accessed using the permissions of the query user (the *invoker* of the
view). A view created in this mode is simply a stored query.

The ``default-view-security-mode`` can be used to configure the default
security mode for view creation.

Copy link
Contributor

Choose a reason for hiding this comment

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

The example you added works, thanks!

@jainxrohit
Copy link
Contributor Author

@steveburnett I tested the changes locally, it comes nicely now

image

@jainxrohit
Add config for default view security mode
076c843 
Presto has a default view creation security mode as 'DEFINER'.
However, some administrators may prefer to use 'INVOKER' as the default.

To address this, a new configuration flag `default-view-security-mode`
has been added that allows the default view security mode to be
changed according to the administrator's preference.
steveburnett
steveburnett approved these changes Feb 23, 2024
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull of updated branch, local build of docs, everything looks fine. Thanks!

@jainxrohit jainxrohit merged commit fcfc114 into prestodb:master Feb 23, 2024
57 checks passed
@wanglinsong wanglinsong mentioned this pull request May 1, 2024
Merged
48 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steveburnett steveburnett steveburnett approved these changes

@NikhilCollooru NikhilCollooru NikhilCollooru approved these changes

@rschlussel rschlussel rschlussel approved these changes

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jainxrohit @steveburnett @rschlussel @NikhilCollooru