fix(analyzer): Check CREATE_VIEW_WITH_SELECT_COLUMNS permission for definer rights MVs

[tdcmeehan]

Description

Fix privilege escalation vulnerability in materialized views with SECURITY DEFINER mode.

Motivation and Context

Materialized views with SECURITY DEFINER were not checking CREATE_VIEW_WITH_SELECT_COLUMNS (SELECT WITH GRANT OPTION) for the definer. This allowed privilege escalation: a user with only SELECT on a table could create a DEFINER MV and effectively grant access to other users, bypassing the GRANT OPTION requirement.

Regular views already enforce this check via ViewAccessControl. This change applies the same security model to MVs.

Impact

When experimental.legacy-materialized-views = false, SECURITY DEFINER MVs now require the owner to have CREATE_VIEW_WITH_SELECT_COLUMNS (SELECT WITH GRANT OPTION) on base tables for non-owner queries. Owner querying their own MV still uses regular access control (SELECT only), matching regular view behavior. No impact on SECURITY INVOKER MVs, and no impact when experimental.legacy-materialized-views = true (default).

Test Plan

Unit tests have been added.

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.
• If adding new dependencies, verified they have an OpenSSF Scorecard score of 5.0 or higher (or obtained explicit TSC approval for lower scores).

Release Notes

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

== RELEASE NOTES ==

General Changes
* Fix Materilized Views with ``DEFINER`` rights to require ``CREATE_VIEW_WITH_SELECT_COLUMNS`` on base tables.

[sourcery-ai]

Reviewer's Guide

Aligns SECURITY DEFINER materialized views with regular views by using ViewAccessControl for non-owner queries, enforcing CREATE_VIEW_WITH_SELECT_COLUMNS on base tables, and adds/updates tests to cover privilege escalation, SHOW CREATE, session user access, and SECURITY INVOKER column-level behavior.

Sequence diagram for SECURITY_DEFINER materialized view access control

sequenceDiagram
    actor User
    participant Coordinator
    participant StatementAnalyzer
    participant AccessControl
    participant ViewAccessControl

    User->>Coordinator: Query SECURITY_DEFINER materialized_view
    Coordinator->>StatementAnalyzer: Analyze query

    alt security_mode_definer
        StatementAnalyzer->>StatementAnalyzer: Resolve owner and queryIdentity
        alt session_user_is_owner
            StatementAnalyzer->>AccessControl: checkSelectOnBaseTables
            AccessControl-->>StatementAnalyzer: SELECT allowed
        else session_user_is_not_owner
            StatementAnalyzer->>ViewAccessControl: checkSelectOnBaseTables
            ViewAccessControl->>AccessControl: checkCreateViewWithSelectColumnsOnBaseTables
            AccessControl-->>ViewAccessControl: CREATE_VIEW_WITH_SELECT_COLUMNS allowed/denied
            ViewAccessControl-->>StatementAnalyzer: Allowed if definer has privilege
        end
    else security_mode_invoker
        StatementAnalyzer->>AccessControl: checkSelectOnBaseTables
        AccessControl-->>StatementAnalyzer: SELECT allowed
    end

    StatementAnalyzer-->>Coordinator: Analysis result (authorized or failure)
    Coordinator-->>User: Results or access denied

Loading

Class diagram for updated materialized view access control

classDiagram
    class AccessControl {
        <<interface>>
        +checkCanSelectFromColumns(identity, tableName, columns)
        +checkCanCreateViewWithSelectColumns(identity, tableName, columns)
    }

    class ViewAccessControl {
        -AccessControl delegate
        +ViewAccessControl(delegate)
        +checkCanSelectFromColumns(identity, tableName, columns)
        +checkCanCreateViewWithSelectColumns(identity, tableName, columns)
    }

    class StatementAnalyzer {
        -AccessControl accessControl
        -AccessControl queryAccessControl
        -Identity queryIdentity
        -Scope processMaterializedView(session, statement, securityMode, owner)
    }

    ViewAccessControl ..|> AccessControl
    StatementAnalyzer --> AccessControl
    StatementAnalyzer --> ViewAccessControl

Loading

File-Level Changes

Change	Details	Files
Use ViewAccessControl for SECURITY DEFINER materialized view analysis when the querying user is not the owner, enforcing CREATE_VIEW_WITH_SELECT_COLUMNS on base tables.	In StatementAnalyzer.processMaterializedView, derive a queryIdentity based on the MV owner for SECURITY DEFINER mode. Conditionally wrap the accessControl in ViewAccessControl when the session user differs from the MV owner. Preserve use of regular accessControl (not ViewAccessControl) when the session user is the MV owner, so owner queries still rely on SELECT privileges only.	presto-main-base/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java
Update existing SECURITY DEFINER MV tests to assert CREATE_VIEW_WITH_SELECT_COLUMNS-based behavior and owner-vs-non-owner semantics.	Change tests to deny CREATE_VIEW_WITH_SELECT_COLUMNS instead of SELECT_COLUMN when validating restricted definer permissions. Adjust expectations so MV owners retain the ability to query their own SECURITY DEFINER MV while non-owners fail when the owner lacks CREATE_VIEW_WITH_SELECT_COLUMNS. Update test names and comments to describe validation of view permissions instead of plain select permissions, and to expect specific error messages about view owner lacking CREATE_VIEW_WITH_SELECT_COLUMNS.	presto-memory/src/test/java/com/facebook/presto/plugin/memory/TestMaterializedViewAccessControl.java
Add regression tests covering SHOW CREATE, privilege escalation prevention, session user requirements, and SECURITY INVOKER column-level access for materialized views.	Add testShowCreateMaterializedViewAccessDenied to verify SHOW CREATE MATERIALIZED VIEW respects SHOW_CREATE_TABLE privileges. Add testDefinerMvPreventsPrivilegeEscalation to ensure a user with only SELECT cannot use a SECURITY DEFINER MV to delegate access when lacking CREATE_VIEW_WITH_SELECT_COLUMNS. Add testSessionUserDoesNotNeedSelectOnBaseTableForDefinerMv to confirm non-owner session users can query SECURITY DEFINER MVs without base table SELECT or CREATE_VIEW_WITH_SELECT_COLUMNS. Add testColumnLevelAccessControlWithSecurityInvoker to validate that SECURITY INVOKER MVs enforce column-level SELECT_COLUMN access on the base table for the invoking user.	presto-memory/src/test/java/com/facebook/presto/plugin/memory/TestMaterializedViewAccessControl.java

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• The logic for when to wrap accessControl in ViewAccessControl for SECURITY DEFINER materialized views is now embedded directly in processMaterializedView; consider extracting a shared helper with the regular view path so view/MV behavior stays aligned in one place.
• Several of the new tests around SECURITY DEFINER MVs repeat the same setup/teardown and privilege-denial patterns; you could reduce duplication and make future changes easier by factoring these into small helper methods (e.g., for creating/refreshing a DEFINER MV and configuring access control).

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The logic for when to wrap `accessControl` in `ViewAccessControl` for SECURITY DEFINER materialized views is now embedded directly in `processMaterializedView`; consider extracting a shared helper with the regular view path so view/MV behavior stays aligned in one place.
- Several of the new tests around SECURITY DEFINER MVs repeat the same setup/teardown and privilege-denial patterns; you could reduce duplication and make future changes easier by factoring these into small helper methods (e.g., for creating/refreshing a DEFINER MV and configuring access control).

## Individual Comments

### Comment 1
<location> `presto-memory/src/test/java/com/facebook/presto/plugin/memory/TestMaterializedViewAccessControl.java:618-619` </location>
<code_context>

-            // Verify that restricted_user cannot access the base table directly
-            assertQueryFails(restrictedSession, "SELECT COUNT(*) FROM bypass_test_base", ".*Access Denied.*");
+            // restricted_user (owner) can still query their own MV (uses regular access control, only checks SELECT)
+            assertQuery(restrictedSession, "SELECT COUNT(*) FROM mv_bypass_test", "SELECT 2");

-            // And that restricted_user cannot access the materialized view either
</code_context>

<issue_to_address>
**suggestion (testing):** Consider also asserting base-table access for the owner to prove only delegation (CREATE_VIEW_WITH_SELECT_COLUMNS) is restricted, not SELECT itself.

Right now the test only checks that the owner can still query the MV and the admin cannot. To clearly show that only delegation (`CREATE_VIEW_WITH_SELECT_COLUMNS` via DEFINER MVs) is restricted—and not the owner’s `SELECT`—please also assert that `restricted_user` can still read directly from `bypass_test_base` (for example, `SELECT COUNT(*) FROM bypass_test_base`).

```suggestion
            // restricted_user (owner) can still query their own MV (uses regular access control, only checks SELECT)
            assertQuery(restrictedSession, "SELECT COUNT(*) FROM mv_bypass_test", "SELECT 2");

            // And restricted_user (owner) can still read directly from the base table; only delegation is restricted
            assertQuery(restrictedSession, "SELECT COUNT(*) FROM bypass_test_base", "SELECT 2");
```
</issue_to_address>

### Comment 2
<location> `presto-memory/src/test/java/com/facebook/presto/plugin/memory/TestMaterializedViewAccessControl.java:695-705` </location>
<code_context>
+                    "SHOW CREATE MATERIALIZED VIEW mv_show_create_test",
+                    ".*Cannot show create table.*mv_show_create_test.*");
+
+            assertUpdate(adminSession, "DROP MATERIALIZED VIEW mv_show_create_test");
+            assertUpdate(adminSession, "DROP TABLE show_create_base");
+        }
+        finally {
</code_context>

<issue_to_address>
**suggestion (testing):** Clean up test objects in the finally block to avoid leaking tables/views when an earlier assertion fails.

In `testShowCreateMaterializedViewAccessDenied`, the MV and base table are dropped inside the `try` block, so a failing assertion above them can leave these objects behind and affect later tests. Move the DROP statements into the `finally` block (with existence checks if needed) to ensure cleanup even when the test fails.

```suggestion
            // Restricted user should be denied
            assertQueryFails(restrictedSession,
                    "SHOW CREATE MATERIALIZED VIEW mv_show_create_test",
                    ".*Cannot show create table.*mv_show_create_test.*");
        }
        finally {
            assertUpdate(adminSession, "DROP MATERIALIZED VIEW IF EXISTS mv_show_create_test");
            assertUpdate(adminSession, "DROP TABLE IF EXISTS show_create_base");
            getQueryRunner().getAccessControl().reset();
        }
```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (testing): Consider also asserting base-table access for the owner to prove only delegation (CREATE_VIEW_WITH_SELECT_COLUMNS) is restricted, not SELECT itself.

Right now the test only checks that the owner can still query the MV and the admin cannot. To clearly show that only delegation (CREATE_VIEW_WITH_SELECT_COLUMNS via DEFINER MVs) is restricted—and not the owner’s SELECT—please also assert that restricted_user can still read directly from bypass_test_base (for example, SELECT COUNT(*) FROM bypass_test_base).

Suggested change

	// restricted_user (owner) can still query their own MV (uses regular access control, only checks SELECT)
	assertQuery(restrictedSession, "SELECT COUNT(*) FROM mv_bypass_test", "SELECT 2");
	// restricted_user (owner) can still query their own MV (uses regular access control, only checks SELECT)
	assertQuery(restrictedSession, "SELECT COUNT(*) FROM mv_bypass_test", "SELECT 2");
	// And restricted_user (owner) can still read directly from the base table; only delegation is restricted
	assertQuery(restrictedSession, "SELECT COUNT(*) FROM bypass_test_base", "SELECT 2");

[sourcery-ai]

suggestion (testing): Clean up test objects in the finally block to avoid leaking tables/views when an earlier assertion fails.

In testShowCreateMaterializedViewAccessDenied, the MV and base table are dropped inside the try block, so a failing assertion above them can leave these objects behind and affect later tests. Move the DROP statements into the finally block (with existence checks if needed) to ensure cleanup even when the test fails.

Suggested change

	// Restricted user should be denied
	assertQueryFails(restrictedSession,
	"SHOW CREATE MATERIALIZED VIEW mv_show_create_test",
	".*Cannot show create table.*mv_show_create_test.*");
	assertUpdate(adminSession, "DROP MATERIALIZED VIEW mv_show_create_test");
	assertUpdate(adminSession, "DROP TABLE show_create_base");
	}
	finally {
	getQueryRunner().getAccessControl().reset();
	}
	// Restricted user should be denied
	assertQueryFails(restrictedSession,
	"SHOW CREATE MATERIALIZED VIEW mv_show_create_test",
	".*Cannot show create table.*mv_show_create_test.*");
	}
	finally {
	assertUpdate(adminSession, "DROP MATERIALIZED VIEW IF EXISTS mv_show_create_test");
	assertUpdate(adminSession, "DROP TABLE IF EXISTS show_create_base");
	getQueryRunner().getAccessControl().reset();
	}

[hantangwangd]

Thanks @tdcmeehan, lgtm!

[steveburnett]

Reviewing late (post-merge), but LGTM! (docs)

Pull branch, local doc build, no problems found.