-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
sql: ensure revalidate_unique_constraint* builtins respect privileges #83959
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @msbutler, @rafiss, and @rytaft)
pkg/sql/backfill.go
line 2608 at r1 (raw file):
ie, txn, username.NodeUserName(),
why node here?
pkg/sql/importer/import_job.go
line 1124 at r1 (raw file):
ie := execCfg.InternalExecutorFactory(ctx, sql.NewFakeSessionData(execCfg.SV())) return ie.WithSyntheticDescriptors([]catalog.Descriptor{desc}, func() error { return sql.RevalidateUniqueConstraintsInTable(ctx, txn, username.NodeUserName(), ie, desc)
I think this should use the job user owner instead of node
.
b8b95cd
to
1cdb741
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTRs!
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @msbutler, and @rafiss)
pkg/sql/backfill.go
line 2608 at r1 (raw file):
Previously, knz (kena) wrote…
why node here?
Fixed.
pkg/sql/importer/import_job.go
line 1124 at r1 (raw file):
Previously, knz (kena) wrote…
I think this should use the job user owner instead of
node
.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good on the bulk side!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @rytaft)
pkg/sql/backfill.go
line 768 at r2 (raw file):
} else if c.IsUniqueWithoutIndex() { if err := validateUniqueWithoutIndexConstraintInTxn( ctx, sc.ieFactory(ctx, evalCtx.SessionData()), desc, txn, username.NodeUserName(), c.GetName(),
maybe evalCtx.SessionData().User()
here?
1cdb741
to
3390373
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @rafiss)
pkg/sql/backfill.go
line 768 at r2 (raw file):
Previously, knz (kena) wrote…
maybe
evalCtx.SessionData().User()
here?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 4 files at r1, 2 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @rafiss)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rafiss)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
GitHub status checks took too long to complete, so bors is giving up. You can adjust bors configuration to have it wait longer if you like. |
This commit updates the builtins crdb_internal.revalidate_unique_constraints_in_all_tables, crdb_internal.revalidate_unique_constraints_in_table, and crdb_internal.revalidate_unique_constraint to ensure that the correct user is passed to the internal executor when running the validation query. This ensures that privileges will be respected. Release note (bug fix): Fixed the following builtins so that users can only run them if they have SELECT privileges on the relevant tables: crdb_internal.revalidate_unique_constraints_in_all_tables, crdb_internal.revalidate_unique_constraints_in_table, and crdb_internal.revalidate_unique_constraint.
3390373
to
d2715d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors r+
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rafiss)
Build failed (retrying...): |
Build succeeded: |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from d2715d2 to blathers/backport-release-21.2-83959: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 21.2.x failed. See errors above. error creating merge commit from d2715d2 to blathers/backport-release-22.1-83959: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 22.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
This commit updates the builtins
crdb_internal.revalidate_unique_constraints_in_all_tables
,crdb_internal.revalidate_unique_constraints_in_table
,and
crdb_internal.revalidate_unique_constraint
to ensure that the correctuser is passed to the internal executor when running the validation query.
This ensures that privileges will be respected.
Release note (bug fix): Fixed the following builtins so that users can only
run them if they have
SELECT
privileges on the relevant tables:crdb_internal.revalidate_unique_constraints_in_all_tables
,crdb_internal.revalidate_unique_constraints_in_table
,and
crdb_internal.revalidate_unique_constraint
.