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

Redshfit: sync tables with partial select permission #40421

Merged
merged 5 commits into from
Mar 22, 2024
Merged

Redshfit: sync tables with partial select permission #40421

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e6e3f85
Redshfit: sync tables with partial select permission
qnkhuat Mar 21, 2024
8e966c5
Merge branch 'master' into ngoc-redshift-sync-table-with-partial-sele…
qnkhuat Mar 21, 2024
75c11b6
Update modules/drivers/redshift/src/metabase/driver/redshift.clj
qnkhuat Mar 21, 2024
6b2633c
Apply suggestions from code review
qnkhuat Mar 21, 2024
bd29fad
newline
qnkhuat Mar 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 27 additions & 24 deletions modules/drivers/redshift/src/metabase/driver/redshift.clj
View file
Open in desktop
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,"
;; interestingly if you have table privilege, has_any_column_privilege is false in redshift. 1) what!
qnkhuat marked this conversation as resolved.
Show resolved Hide resolved
" pg_catalog.has_table_privilege(current_user, '\"' || t.schemaname || '\"' || '.' || '\"' || t.objectname || '\"', 'SELECT')"
qnkhuat marked this conversation as resolved.
Show resolved Hide resolved
" OR pg_catalog.has_any_column_privilege(current_user, '\"' || t.schemaname || '\"' || '.' || '\"' || t.objectname || '\"', 'SELECT') as select,"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

has_any_column_privilege return false if you have full select privileges :), we need to do has_table_privilege OR has_any_column_privilege here.

also has_any_column_privilege only exists on select and update.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is pretty surprising so let's leave this as a comment, like I've suggested here https://github.com/metabase/metabase/pull/40421/files#r1533731501

" 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
119 changes: 65 additions & 54 deletions modules/drivers/redshift/test/metabase/driver/redshift_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,76 +400,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 SELECT (id) ON %4$s TO \"%6$s\";\n"
"GRANT UPDATE (id) ON %5$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\";")
qnkhuat marked this conversation as resolved.
Show resolved Hide resolved
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
(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)))))))))
(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)))))))))
Copy link
Contributor

Choose a reason for hiding this comment

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

we'll need a newline here

Loading