timescale / timescaledb

An open-source time-series SQL database optimized for fast ingest and complex queries. Packaged as a PostgreSQL extension.
https://www.timescale.com/
Other
16.83k stars 852 forks source link

Fix REASSIGN OWNED BY for background jobs #6987

Open mkindahl opened 1 month ago

mkindahl commented 1 month ago

Using REASSIGN OWNED BY for background jobs do not work because it does not change the owner of the job. This commit fixes this by capturing the utility command and makes the necessary changes to the bgw_job table.

It also factors out background jobs DDL tests into a separate file.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review.

Project coverage is 81.79%. Comparing base (59f50f2) to head (d9ad469). Report is 243 commits behind head on main.

Files Patch % Lines
src/process_utility.c 88.46% 1 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6987 +/- ## ========================================== + Coverage 80.06% 81.79% +1.72% ========================================== Files 190 202 +12 Lines 37181 37422 +241 Branches 9450 9763 +313 ========================================== + Hits 29770 30608 +838 + Misses 2997 2891 -106 + Partials 4414 3923 -491 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

svenklemm commented 1 day ago

Please add a test for reassigning to postgres.

mkindahl commented 1 day ago

Please add a test for reassigning to postgres.

I assume you mean a user with SUPERUSER privileges: we do not have the "postgres" user in the test suite.

mkindahl commented 1 day ago

Please add a test for reassigning to postgres.

I updated it with a test that assigning the job to a different owner does not work unless you've got superuser privileges.

Don't think this is how we want it to work since an administrator should be able to reassign jobs without having superuser privileges.

svenklemm commented 21 hours ago

Please add a test for reassigning to postgres.

I updated it with a test that assigning the job to a different owner does not work unless you've got superuser privileges.

Don't think this is how we want it to work since an administrator should be able to reassign jobs without having superuser privileges.

I wanted to see that it doesnt work, anything else would be a security vulnerability.

mkindahl commented 14 hours ago

Please add a test for reassigning to postgres.

I updated it with a test that assigning the job to a different owner does not work unless you've got superuser privileges. Don't think this is how we want it to work since an administrator should be able to reassign jobs without having superuser privileges.

I wanted to see that it doesnt work, anything else would be a security vulnerability.

Yes, obviously, but in this form it is also a usability issue.

I am not sure how to have some sort of administrative user that does not have "superuser" privileges but still can run a "reassigned owned by" in a limited fashion. Not even sure it is possible to define a "safe" rule for this. Something we need to think about.