Show accessible tables only in New Query view for PostgreSQL#3599
Conversation
ghost
added
the
|
@arikfr is omitting inaccessible tables the desired user experience? Also, should this type of filtering be replicated across all query runners? |
Yes.
In theory query runners should be implemented in this way already. But there is no single recipe that will work for all of them anyway. |
arikfr
left a comment
arikfr
left a comment
There was a problem hiding this comment.
Thanks, @shinsuke-nara! This was introduced in #2549 when the support for partitioned tables was added.
While your fix makes sure we load tables from schemas the user has access to, it still might show tables/columns the user doesn't have access to:
- When the user doesn't have access to all tables in a schema.
- When the user doesn't have access to all columns in a table.
Some possible solutions:
We could fix the above by using information_schema.column_privileges and making sure the schema/table/columns are referenced there, but I'm concerned this might break support for the special kinds (materialized views, foreign tables and partitioned tables).
Use the has_column_privilege function, but it has the same concern as above (does it apply to the special kinds?). Also it was introduced in Postgres 8.4 only.
Use information_schema.columns, which already applies ACL. For the special kinds it does not include, we can augment with information from the current query while ignoring ACL.
I think option 3 is the safest way forward which for sure improves the current situation without requiring much research.
What do you think?
|
Thank you for checking my PR. |
|
I fixed this PR according to your advice. |
| table_name, | ||
| column_name | ||
| FROM information_schema.columns | ||
| WHERE table_schema NOT IN ('pg_catalog', 'information_schema') |
There was a problem hiding this comment.
Thank you for the update, but we still need the old query for the "special types", so it should be something like:
query = """
SELECT s.nspname as table_schema,
c.relname as table_name,
a.attname as column_name
FROM pg_class c
JOIN pg_namespace s
ON c.relnamespace = s.oid
AND s.nspname NOT IN ('pg_catalog', 'information_schema')
JOIN pg_attribute a
ON a.attrelid = c.oid
AND a.attnum > 0
AND NOT a.attisdropped
WHERE c.relkind IN ('m', 'f', 'p')
UNION
SELECT table_schema,
table_name,
column_name
FROM information_schema.columns
WHERE table_schema NOT IN ('pg_catalog', 'information_schema')
"""
Thanks :)There was a problem hiding this comment.
Thank you for the advice. I have fixed again.
b968f32 to
30899b4
Compare
30899b4 to
d766d2d
Compare
|
Thanks! |
|
Thank you for accepting my PR :) |
…sh#3599) * Show accessible tables only. * Get table information from information_schema.columns. * Union old query.
A UNION was added in getredash#3599 which is incorrect: this results in duplicate column names (because of a NULL data type) for partitioned tables and foreign tables.
A UNION was added in getredash#3599 which is incorrect: this results in duplicate column names (because of a NULL data type) for partitioned tables and foreign tables.
A UNION was added in getredash#3599 which is incorrect: this results in duplicate column names (because of a NULL data type) for partitioned tables and foreign tables.
A UNION was added in getredash#3599 which is incorrect: this results in duplicate column names (because of a NULL data type) for partitioned tables and foreign tables.
3 participants
What type of PR is this? (check all applicable)
Description
Background
Left pane of
New Queryview shows table list. The list includes tables which user can not access.Tables which user can not access should be hidden.
The bug
Function
redash.query_runner.pg.PostgreSQL._get_tablesdoes not consider privileges of a user for tables.The Fix
Get tables for which
current_userof PostgreSQL have privileges.Related Tickets & Documents
--
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
--