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

Redshfit: sync tables with partial select permission #40421

Merged
merged 5 commits into from
Mar 22, 2024

Conversation

qnkhuat
Copy link
Contributor

@qnkhuat qnkhuat commented Mar 21, 2024

Similiar to #40034 but for redshift

@qnkhuat qnkhuat requested a review from camsaul as a code owner March 21, 2024 04:24
@metabase-bot metabase-bot bot added the .Team/BackendComponents also known as BEC label Mar 21, 2024
" t.objectname as table,"
;; interestingly if you have table privilege, has_any_column_privilege is false in redshift. 1) what!
" 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,"
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

Copy link

replay-io bot commented Mar 21, 2024

Status Complete ↗︎
Commit bd29fad
Results
⚠️ 2 Flaky
2375 Passed

@qnkhuat qnkhuat requested a review from a team March 21, 2024 09:50
@calherries calherries added the backport Automatically create PR on current release branch on merge label Mar 21, 2024
Copy link
Contributor

@calherries calherries left a comment

Choose a reason for hiding this comment

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

Looks good, just left minor suggestions

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

modules/drivers/redshift/src/metabase/driver/redshift.clj Outdated Show resolved Hide resolved
modules/drivers/redshift/src/metabase/driver/redshift.clj Outdated Show resolved Hide resolved
qnkhuat and others added 3 commits March 21, 2024 22:35
Co-authored-by: Cal Herries <39073188+calherries@users.noreply.github.com>
Co-authored-by: Cal Herries <39073188+calherries@users.noreply.github.com>
@qnkhuat qnkhuat enabled auto-merge (squash) March 22, 2024 02:51
@qnkhuat qnkhuat merged commit c772c0a into master Mar 22, 2024
107 checks passed
@qnkhuat qnkhuat deleted the ngoc-redshift-sync-table-with-partial-select-perms branch March 22, 2024 12:20
Copy link

@qnkhuat Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

@qnkhuat qnkhuat added this to the 0.49.2 milestone Mar 24, 2024
qnkhuat added a commit that referenced this pull request Mar 25, 2024
…40504)

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

* uncomment since 40058 is backported

---------

Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Automatically create PR on current release branch on merge .Team/BackendComponents also known as BEC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants