trinodb / trino

Official repository of Trino, the distributed SQL query engine for big data, formerly known as PrestoSQL (https://trino.io)
https://trino.io
Apache License 2.0
10.37k stars 2.98k forks source link

Materialized view should only check permissions of underlying query for refresh #23747

Open dain opened 2 weeks ago

dain commented 2 weeks ago

From a security perspective a materialized view is just a table, and should be treated as such. The only time we should be checking if the definer has permissions to run the view attached to the query is during refresh.

This was found in the discussion: https://github.com/trinodb/trino/discussions/23387

lozbrown commented 2 weeks ago

@dain should refresh materialized view be a separate permission in the API/SPI, refreshing a view of a big table can be a significant load on the platform and we may want to prevent users doing that too frequently?

lozbrown commented 6 days ago

@dain Thinking about this more, whilst I agree for select an MV should be treated as a table, should the end user really need permission to the underlying table just to refresh?

Imagine this scenario

You create an MV over a sensitive_table_a for a report that non_priveliged_userb uses on a regular basis. If the MV aggregates it otherwise does not include the sensitive data by refreshing non_priveliged_userb is not gaining any additional access to data?

By this notion only select on the MV should be necessary for the refresh and only the creator needs access to the underlying data.

However refreshing an MV can be potentially expensive and may have been created (instead of a view) to prevent users from doing the expensive query underneath too frequently, hence a separate refresh permission would be better so that refreshes may only occur when the grace period runs out or someone with the permission refreshes