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.

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

Show accessible tables only in New Query view for PostgreSQL #3599

Merged
merged 3 commits into from
Mar 27, 2019

Conversation

dairyo
Copy link
Contributor

@dairyo dairyo commented Mar 18, 2019

What type of PR is this? (check all applicable)

  • Bug Fix

Description

Background

Left pane of New Query view 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_tables does not consider privileges of a user for tables.

The Fix

Get tables for which current_user of PostgreSQL have privileges.

Related Tickets & Documents

--

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

--

@ghost ghost added the in progress label Mar 18, 2019
@ranbena
Copy link
Contributor

ranbena commented Mar 18, 2019

@arikfr is omitting inaccessible tables the desired user experience?
An alternative would be to show these in disabled mode with a tooltip explanation, but I'm not knowledgable enough to know if this has any value.

Also, should this type of filtering be replicated across all query runners?

@arikfr
Copy link
Member

arikfr commented Mar 18, 2019

@ranbena

is omitting inaccessible tables the desired user experience?

Yes.

Also, should this type of filtering be replicated across all query runners?

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.

Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

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:

  1. When the user doesn't have access to all tables in a schema.
  2. 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?

@dairyo
Copy link
Contributor Author

dairyo commented Mar 18, 2019

@arikfr

Thank you for checking my PR.
I agree with you. I will fix this PR with option 3.

@dairyo
Copy link
Contributor Author

dairyo commented Mar 18, 2019

@arikfr

I fixed this PR according to your advice.
Could you check it if you have time?

table_name,
column_name
FROM information_schema.columns
WHERE table_schema NOT IN ('pg_catalog', 'information_schema')
Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akariv

Thank you for the advice. I have fixed again.

@dairyo dairyo force-pushed the restrict-shown-table branch from b968f32 to 30899b4 Compare March 26, 2019 03:47
@dairyo dairyo force-pushed the restrict-shown-table branch from 30899b4 to d766d2d Compare March 26, 2019 03:51
@arikfr arikfr merged commit 872d0ca into getredash:master Mar 27, 2019
@arikfr
Copy link
Member

arikfr commented Mar 27, 2019

Thanks!

@dairyo dairyo deleted the restrict-shown-table branch March 28, 2019 02:51
@dairyo
Copy link
Contributor Author

dairyo commented Mar 28, 2019

@arikfr

Thank you for accepting my PR :)

harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
…sh#3599)

* Show accessible tables only.

* Get table information from information_schema.columns.

* Union old query.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants