Skip to content

Commit

Permalink
🤖 backported "Redshfit: sync tables with partial select permission" (#…
Browse files Browse the repository at this point in the history 
…40504)

* Redshfit: sync tables with partial select permission (#40421)

* uncomment since 40058 is backported

---------

Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>
  • Loading branch information
@metabase-bot @qnkhuat
metabase-bot[bot] and qnkhuat committed Mar 25, 2024
1 parent 4411108 commit bbba0dc
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 81 deletions.
51 changes: 27 additions & 24 deletions modules/drivers/redshift/src/metabase/driver/redshift.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -433,30 +433,33 @@
;; KNOWN LIMITATION: this won't return privileges for external tables, calling has_table_privilege on an external table
;; result in an operation not supported error
(->> (jdbc/query
conn-spec
(str/join
"\n"
["with table_privileges as ("
" select"
" NULL as role,"
" t.schemaname as schema,"
" t.objectname as table,"
" pg_catalog.has_table_privilege(current_user, '\"' || t.schemaname || '\"' || '.' || '\"' || t.objectname || '\"', 'UPDATE') as update,"
" pg_catalog.has_table_privilege(current_user, '\"' || t.schemaname || '\"' || '.' || '\"' || t.objectname || '\"', 'SELECT') as select,"
" pg_catalog.has_table_privilege(current_user, '\"' || t.schemaname || '\"' || '.' || '\"' || t.objectname || '\"', 'INSERT') as insert,"
" pg_catalog.has_table_privilege(current_user, '\"' || t.schemaname || '\"' || '.' || '\"' || t.objectname || '\"', 'DELETE') as delete"
" from ("
" select schemaname, tablename as objectname from pg_catalog.pg_tables"
" union"
" select schemaname, viewname as objectname from pg_views"
" ) t"
" where t.schemaname !~ '^pg_'"
" and t.schemaname <> 'information_schema'"
" and pg_catalog.has_schema_privilege(current_user, t.schemaname, 'USAGE')"
")"
"select t.*"
"from table_privileges t"]))
(filter #(or (:select %) (:update %) (:delete %) (:update %)))))
conn-spec
(str/join
"\n"
["with table_privileges as ("
" select"
" NULL as role,"
" t.schemaname as schema,"
" t.objectname as table,"
;; if `has_table_privilege` is true `has_any_column_privilege` is false and vice versa, so we have to check both.
" pg_catalog.has_table_privilege(current_user, '\"' || t.schemaname || '\".\"' || t.objectname || '\"', 'SELECT')"
" OR pg_catalog.has_any_column_privilege(current_user, '\"' || t.schemaname || '\"' || '.' || '\"' || t.objectname || '\"', 'SELECT') as select,"
" pg_catalog.has_table_privilege(current_user, '\"' || t.schemaname || '\"' || '.' || '\"' || t.objectname || '\"', 'UPDATE')"
" OR pg_catalog.has_any_column_privilege(current_user, '\"' || t.schemaname || '\"' || '.' || '\"' || t.objectname || '\"', 'UPDATE') as update,"
" pg_catalog.has_table_privilege(current_user, '\"' || t.schemaname || '\"' || '.' || '\"' || t.objectname || '\"', 'INSERT') as insert,"
" pg_catalog.has_table_privilege(current_user, '\"' || t.schemaname || '\"' || '.' || '\"' || t.objectname || '\"', 'DELETE') as delete"
" from ("
" select schemaname, tablename as objectname from pg_catalog.pg_tables"
" union"
" select schemaname, viewname as objectname from pg_views"
" ) t"
" where t.schemaname !~ '^pg_'"
" and t.schemaname <> 'information_schema'"
" and pg_catalog.has_schema_privilege(current_user, t.schemaname, 'USAGE')"
")"
"select t.*"
"from table_privileges t"]))
(filter #(or (:select %) (:update %) (:delete %) (:update %)))))


;;; ----------------------------------------------- Connection Impersonation ------------------------------------------
Expand Down
123 changes: 66 additions & 57 deletions modules/drivers/redshift/test/metabase/driver/redshift_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,78 +403,87 @@
(mt/test-driver :redshift
(testing "`table-privileges` should return the correct data for current_user and role privileges"
(mt/with-temp [Database database {:engine :redshift :details (tx/dbdef->connection-details :redshift nil nil)}]
(let [schema-name (redshift.test/unique-session-schema)
username (str (sql.tu.unique-prefix/unique-prefix) "privilege_rows_test_role")
db-name (:name database)
table-name (tx/db-qualified-table-name db-name "table")
qual-tbl-name (format "\"%s\".\"%s\"" schema-name table-name)
view-nm (tx/db-qualified-table-name db-name "view")
qual-view-name (format "\"%s\".\"%s\"" schema-name view-nm)
mview-name (tx/db-qualified-table-name db-name "mview")
qual-mview-name (format "\"%s\".\"%s\"" schema-name mview-name)
conn-spec (sql-jdbc.conn/db->pooled-connection-spec database)
get-privileges (fn []
(sql-jdbc.conn/with-connection-spec-for-testing-connection
[spec [:redshift (assoc (:details database) :user username)]]
(with-redefs [sql-jdbc.conn/db->pooled-connection-spec (fn [_] spec)]
(set (sql-jdbc.sync/current-user-table-privileges driver/*driver* spec)))))]
(let [schema-name (redshift.test/unique-session-schema)
username (str (sql.tu.unique-prefix/unique-prefix) "privilege_rows_test_role")
db-name (:name database)
table-name (tx/db-qualified-table-name db-name "table")
qual-tbl-name (format "\"%s\".\"%s\"" schema-name table-name)
table-partial-select-name (tx/db-qualified-table-name db-name "tbl_sel")
qual-tbl-partial-select-name (format "\"%s\".\"%s\"" schema-name table-partial-select-name)
table-partial-update-name (tx/db-qualified-table-name db-name "tbl_upd")
qual-tbl-partial-update-name (format "\"%s\".\"%s\"" schema-name table-partial-update-name)
view-nm (tx/db-qualified-table-name db-name "view")
qual-view-name (format "\"%s\".\"%s\"" schema-name view-nm)
mview-name (tx/db-qualified-table-name db-name "mview")
qual-mview-name (format "\"%s\".\"%s\"" schema-name mview-name)
conn-spec (sql-jdbc.conn/db->pooled-connection-spec database)
get-privileges (fn []
(sql-jdbc.conn/with-connection-spec-for-testing-connection
[spec [:redshift (assoc (:details database) :user username)]]
(with-redefs [sql-jdbc.conn/db->pooled-connection-spec (fn [_] spec)]
(set (sql-jdbc.sync/current-user-table-privileges driver/*driver* spec)))))]
(try
(execute! (format
(str
"CREATE TABLE %1$s (id INTEGER);\n"
"CREATE VIEW %2$s AS SELECT * from %1$s;\n"
"CREATE MATERIALIZED VIEW %3$s AS SELECT * from %1$s;\n"
"CREATE USER \"%4$s\" WITH PASSWORD '%5$s';\n"
"GRANT SELECT ON %1$s TO \"%4$s\";\n"
"GRANT UPDATE ON %1$s TO \"%4$s\";\n"
"GRANT SELECT ON %2$s TO \"%4$s\";\n"
"GRANT SELECT ON %3$s TO \"%4$s\";")
"CREATE TABLE %4$s (id INTEGER);\n"
"CREATE TABLE %5$s (id INTEGER);\n"
"CREATE USER \"%6$s\" WITH PASSWORD '%7$s';\n"
"GRANT SELECT ON %1$s TO \"%6$s\";\n"
"GRANT UPDATE ON %1$s TO \"%6$s\";\n"
"GRANT SELECT ON %2$s TO \"%6$s\";\n"
"GRANT SELECT ON %3$s TO \"%6$s\";\n"
"GRANT SELECT (id) ON %4$s TO \"%6$s\";\n"
"GRANT UPDATE (id) ON %5$s TO \"%6$s\";")
qual-tbl-name
qual-view-name
qual-mview-name
qual-tbl-partial-select-name
qual-tbl-partial-update-name
username
(get-in database [:details :password])))
(testing "check that without USAGE privileges on the schema, nothing is returned"
(is (= #{}
(get-privileges))))
(testing "with USAGE privileges, SELECT and UPDATE privileges are returned"
(jdbc/execute! conn-spec (format "GRANT USAGE ON SCHEMA \"%s\" TO \"%s\";" schema-name username))
(is (= #{{:role nil
:schema schema-name
:table table-name
:update true
:select true
:insert false
:delete false}
{:role nil
:schema schema-name
:table view-nm
:update false
:select true
:insert false
:delete false}
{:role nil
:schema schema-name
:table mview-name
:select true
:update false
:insert false
:delete false}}
(is (= (into #{} (map #(merge {:schema schema-name
:role nil
:select false
:update false
:insert false
:delete false}
%)
[{:table table-name
:update true
:select true}
{:table table-partial-select-name
:select true}
{:table table-partial-update-name
:update true}
{:table view-nm
:select true}
{:table mview-name
:select true}]))
(get-privileges))))
(finally
;; comment out since it's causing flake
;; can uncomment after we tackle #40058
#_(execute! (format
(str
"DROP TABLE IF EXISTS %2$s CASCADE;\n"
"DROP VIEW IF EXISTS %3$s CASCADE;\n"
"DROP MATERIALIZED VIEW IF EXISTS %4$s CASCADE;\n"
"REVOKE ALL PRIVILEGES ON ALL TABLES IN SCHEMA \"%1$s\" FROM \"%5$s\";\n"
"REVOKE ALL PRIVILEGES ON SCHEMA \"%1$s\" FROM \"%5$s\";\n"
"REVOKE USAGE ON SCHEMA \"%1$s\" FROM \"%5$s\";\n"
"DROP USER IF EXISTS \"%5$s\";")
schema-name
qual-tbl-name
qual-view-name
qual-mview-name
username)))))))))
(execute! (format
(str
"DROP TABLE IF EXISTS %2$s CASCADE;\n"
"DROP VIEW IF EXISTS %3$s CASCADE;\n"
"DROP MATERIALIZED VIEW IF EXISTS %4$s CASCADE;\n"
"DROP TABLE IF EXISTS %5$s CASCADE;\n"
"DROP TABLE IF EXISTS %6$s CASCADE;\n"
"REVOKE ALL PRIVILEGES ON ALL TABLES IN SCHEMA \"%1$s\" FROM \"%7$s\";\n"
"REVOKE ALL PRIVILEGES ON SCHEMA \"%1$s\" FROM \"%7$s\";\n"
"REVOKE USAGE ON SCHEMA \"%1$s\" FROM \"%7$s\";\n"
"DROP USER IF EXISTS \"%7$s\";")
schema-name
qual-tbl-name
qual-view-name
qual-mview-name
qual-tbl-partial-select-name
qual-tbl-partial-update-name
username)))))))))

0 comments on commit bbba0dc

Please sign in to comment.