sql: require SELECT and UPDATE privileges for SELECT FOR UPDATE #57309
Conversation
Fixes cockroachdb#57282. Before this change, `SELECT ... FOR [KEY] UPDATE/SHARE` statements replaced the SELECT privilege check with an UPDATE privilege check. This was incorrect, as the desired behavior is that the statement requires both SELECT and UPDATE privileges. This commit fixes that bug. Release note (bug fix): SELECT FOR UPDATE now requires both SELECT and UPDATE privileges, instead of just UPDATE privileges.
|
This change is |
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)
pkg/sql/opt/optbuilder/select.go, line 108 at r1 (raw file):
if locking.isSet() { // SELECT ... FOR [KEY] UPDATE/SHARE also requires UPDATE privileges. b.checkPrivilege(depName, ds, privilege.UPDATE)
It'd be nice to be able to pass a bitmask union of the two privileges to resolveDataSource, but it looks like that'd snowball into a much larger change, so probably not worth it.
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/sql/opt/optbuilder/select.go, line 108 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
It'd be nice to be able to pass a bitmask union of the two privileges to
resolveDataSource, but it looks like that'd snowball into a much larger change, so probably not worth it.
Yeah, I was hoping this would be as easy as a pair of s/=/|=/, but it was more involved. This comes up in the mutation paths as well, so we may want to revisit the representation of these privileges at some point to allow for such operations.
|
Build succeeded: |
Fixes #57282.
Before this change,
SELECT ... FOR [KEY] UPDATE/SHAREstatements replacedthe SELECT privilege check with an UPDATE privilege check. This was incorrect,
as the desired behavior is that the statement requires both SELECT and UPDATE
privileges. This commit fixes that bug.
I intend to backport this PR to v20.2 and v20.1.
Release note (bug fix): SELECT FOR UPDATE now requires both SELECT
and UPDATE privileges, instead of just UPDATE privileges.