Fix database user auto-provisioning #51945
Open
+568
−246
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GavinFrazar
added
aws
GavinFrazar
commented
Feb 7, 2025
| @@ -8,11 +8,62 @@ BEGIN | |||
| IF EXISTS (SELECT usename FROM pg_stat_activity WHERE usename = username AND datname = current_database()) THEN | |||
| RAISE NOTICE 'User has active connections to current database'; | |||
| ELSE | |||
| -- Loop through table permissions | |||
| FOR row_data IN (SELECT DISTINCT table_schema, table_name FROM information_schema.table_privileges WHERE grantee = username) | |||
| CREATE TEMPORARY TABLE cur_perms ON COMMIT DROP AS ( | |||
There was a problem hiding this comment.
by default postgres temp tables are dropped when your session ends instead of dropping them on commit/rollback, so I added ON COMMIT DROP to this and the other procedure to make them retryable, otherwise you can get an error that the table already exists if you retry calling the proc.
GavinFrazar
force-pushed
the
gavinfrazar/fix-db-auto-user-bugs
branch
2 times, most recently
from
9789f5f to
925e216
Compare
greedy52
reviewed
Feb 7, 2025
|
🙏 thanks for all the effort you've put into investigating this!!! |
* Use a custom query to find user db privileges on tables to avoid the "grantor" filter condition in the information_schema.tables_privileges view. This fixes the cases where the grantor for a privilege is set to the table owner rather than the user (teleport) who issued the grant. Most notably, this happens when a superuser grants privileges on a table they do not own to a user. * Grant USAGE on schemas that contain tables where we intend to grant table privileges. This is necessary to use the table privileges we grant. * Wrap all remaining plpgsql procedure creation/calls with retries. * Add a db permissions e2e test for RDS * Expand e2e tests to test with and without a superuser db admin * Significantly speed up the RDS e2e tests by wrapping EventuallyWithT in a helper func that tries the condition func immediately rather than waiting for the first tick duration.
GavinFrazar
force-pushed
the
gavinfrazar/fix-db-auto-user-bugs
branch
from
925e216 to
59671e1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelog: Fixed Postgres database access control privileges auto-provisioning to grant USAGE on schemas as needed for table privileges and fixed an issue that prevented user privileges from being revoked at the end of their session in some cases.
Fixes #51681
Fixes #51851
publicschema #51851This PR includes the following:
I tested this quite a bit manually on RDS postgres and self-hosted, but still expanded the e2e tests just to be sure and to avoid regressions
I'm also hopeful that the additional retries I added will fix the Redshift flakiness.
Reviewers: sorry for the large diff in the e2e tests, but I noticed they were wasting quite a lot time after each subtest just waiting for 10 seconds before trying the post-connection test fn.