sql: add VIEWACTIVITY and CANCELQUERY role options #53291
Conversation
|
This change is |
pkg/server/admin.go
Outdated
| if err != nil { | ||
| return false, err | ||
| } | ||
| if len(rows) != 1 || len(cols) != 1 { |
There was a problem hiding this comment.
why do you need the columns here?
pkg/server/admin.go
Outdated
| // Shortcut. | ||
| return true, nil | ||
| } | ||
| rows, cols, err := s.server.sqlServer.internalExecutor.QueryWithCols( |
There was a problem hiding this comment.
Is it possible to share these queries with the planner, maybe helper methods that take in an ie interface?
|
|
||
| if !reqUserHasAdmin { | ||
| // Check if the user has permission to see the query's session. | ||
| var session serverpb.Session |
There was a problem hiding this comment.
this code seems duplicated with the above
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rohany)
pkg/server/admin.go, line 2473 at r2 (raw file):
Previously, rohany (Rohan Yadav) wrote…
Is it possible to share these queries with the planner, maybe helper methods that take in an ie interface?
Can you say more? The planner already has a HasRoleOption method, which is how the crdb_internal.has_role_option built-in is implemented. I used the built-in here since there's no planner available.
pkg/server/admin.go, line 2480 at r2 (raw file):
Previously, rohany (Rohan Yadav) wrote…
why do you need the columns here?
This was cargo-culted from hasAdminRole but it does seem like a good idea to assert that the result shape is as expected. I'm going to switch this to ignore cols and check len(rows[0]) == 1 instead.
pkg/server/status.go, line 1920 at r2 (raw file):
Previously, rohany (Rohan Yadav) wrote…
this code seems duplicated with the above
Might be able to factor something out but this code is matching on query ID rather than session ID above.
solongordon
force-pushed
the
cancelquery
branch
2 times, most recently
from
4a61f0f to
c7cdeb4
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/server/admin.go, line 2473 at r2 (raw file):
Previously, solongordon (Solon) wrote…
Can you say more? The planner already has a
HasRoleOptionmethod, which is how thecrdb_internal.has_role_optionbuilt-in is implemented. I used the built-in here since there's no planner available.
Never mind. I thought the planner's implementation also used an IE to make a query like this, but thats not true
pkg/server/status.go, line 1920 at r2 (raw file):
Previously, solongordon (Solon) wrote…
Might be able to factor something out but this code is matching on query ID rather than session ID above.
Yeah. Perhaps the non-admin check logic could be pulled out, but thats it.
pkg/sql/internal_test.go, line 200 at r3 (raw file):
} func TestQueryHasRoleOptionWithNoTxn(t *testing.T) {
What's the goal of this test?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/internal_test.go, line 200 at r3 (raw file):
Previously, rohany (Rohan Yadav) wrote…
What's the goal of this test?
Just testing the new crdb_internal.has_role_option built-in. Now that I think about it this might be nicer as a logic test, but I was mimicking the existing is_admin unit test.
solongordon
force-pushed
the
cancelquery
branch
2 times, most recently
from
323ba3f to
e946fcf
Compare
Release note (sql change): Introduced a new VIEWACTIVITY role option. This grants non-admin roles the ability to see other users' sessions and queries through the following means: - SHOW SESSIONS - SHOW QUERIES - the WebUI Statements page
Release note (sql change): Introduced a new CANCELQUERY role option. This grants non-admin roles the ability to cancel other users' queries and sessions. Note that non-admins are not allowed to cancel admins' queries or sessions. Release note (bug fix): Admin users are now permitted to cancel other users' queries and sessions. Previously only the root user was allowed to do so. Fixes cockroachdb#52578 Fixes cockroachdb#52579 Fixes cockroachdb#53277
solongordon
force-pushed
the
cancelquery
branch
from
e946fcf to
3fcf5e4
Compare
|
bors r+ |
|
Build succeeded: |
There was a problem hiding this comment.
Reviewable status:
pkg/server/status.go, line 1894 at r5 (raw file):
if !isAdmin && sessionUser != req.Username { // A user can only cancel their own queries. return nil, errRequiresAdmin
Drive-by comment since I'm fixing some merge conflicts with this PR in another PR. Is it correct for this check to stay in? I assume that this will ignore the CANCELQUERY privilege if a non-admin user with this privilege tries to cancel another user's query.
There was a problem hiding this comment.
Reviewable status:
pkg/server/status.go, line 1894 at r5 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Drive-by comment since I'm fixing some merge conflicts with this PR in another PR. Is it correct for this check to stay in? I assume that this will ignore the
CANCELQUERYprivilege if a non-admin user with this privilege tries to cancel another user's query.
I think the check is still correct but the comment is misleading. When a CANCEL QUERY statement is run, this request is invoked with root and the user who issued the query is specified in req.Username. However if this API is invoked from the Admin UI, sessionUser and req.Username should match. I'll have to check if this is actually the case in @jordanlewis's outstanding PR for the Sessions page.
Release note (sql change): Introduced a new VIEWACTIVITY role option.
This grants non-admin roles the ability to see other users' sessions and
queries through the following means:
Release note (sql change): Introduced a new CANCELQUERY role option.
This grants non-admin roles the ability to cancel other users' queries
and sessions. Note that non-admins are not allowed to cancel admins'
queries or sessions.
Release note (bug fix): Admin users are now permitted to cancel other
users' queries and sessions. Previously only the root user was allowed
to do so.
Fixes #52578
Fixes #52579
Fixes #53277