Repository navigation
Reject GRANT/REVOKE ON ALL in temporary schemas - #40468
Conversation
0ecb3cc to
ade5043
Compare
|
Part of me feels like rather than return an explicit mz_temp namespace, we should just filter it out wherever we get schemas and we throw an error like "mz_temp does not exist" similar to Postgres. This way we don't have to always keep temporary schemas in mind if we were to create a new RBAC command. This would lead to less descriptive errors however. |
ggevay
left a comment
ggevay
left a comment
There was a problem hiding this comment.
LGTM! One question inline
| .iter() | ||
| .any(|schema| schema.schema_spec().is_temporary()) | ||
| { | ||
| sql_bail!( |
There was a problem hiding this comment.
Is it intended that this also rejects the statement when the session already has temporary tables? On main, CREATE TEMP TABLE t; GRANT SELECT ON ALL TABLES IN SCHEMA mz_temp TO PUBLIC grants on t, so this changes behavior beyond fixing the panic. Rejecting looks fine to me, but could you mention it in the description and make the code comment state that rule? It currently gives only the panic as the reason. A test with a temporary table would then pin the rule: no case in this file creates one, so all three new cases take the no-temporary-schema path.
There was a problem hiding this comment.
Ah it was unintentional! I decided to go the route of accepting it if the temporary schema exists, similar to postgres. This also means changing the error message to be the same as postgres as per #40468 (comment)
…cking - Throws a SQL error for `GRANT/REVOKE ON ALL ... IN SCHEMA mz_temp` statements - Adds a SLT case next to the other error checking cases surrounding RBAC and temporary schemas
`GRANT/REVOKE ... ON ALL ... IN SCHEMA mz_temp` applies to the connection's own temporary items again, as it did before the temporary schema became lazily created. A connection that has not created a temporary item yet gets `unknown schema 'mz_temp'`, which matches PostgreSQL for `pg_temp`, instead of a coordinator panic. The new test creates temporary tables on two connections and checks that the grant and revoke only touch the issuing connection's table.
ade5043 to
ade9ab6
Compare
For RBAC handling around the temporary schema
mz_temp, for the most part we throw a SQL error by detecting if it's temporary. However we missed the case for theGRANT/REVOKE ON ALLcommand.Motivation
Closes: SQL-364
Description
GRANT/REVOKE ON ALL ... IN SCHEMA mz_tempstatements