Skip to content

Commit

Permalink
server: correctly redact sql constants in ListSessions api
Browse files Browse the repository at this point in the history
Previously, there was a bug in the redaction of SQL constants
for `VIEWACTIVITYREDACTED` users in the `ListSessions` api.
Firstly, the range loop used to set the sql query field
was manipulating the copy of the query object provided
by the range. Secondly, the previous logic when redacting
set the `Sql` and `LastActiveQuery` fields to empty strings.
This commit sets those fields to equal `SqlNoConstants` and
`LastActiveQueryNoConstants` respectively for usability
on the client side.

Release note (bug fix): constants in sql query fields are
correctly removed for VIEWACTIVITYREDACTED users
  • Loading branch information
xinhaoz committed May 19, 2022
1 parent fb8e09a commit d8a4bed
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 7 deletions.
11 changes: 6 additions & 5 deletions pkg/server/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,19 +219,20 @@ func (b *baseStatusServer) getLocalSessions(
userSessions := make([]serverpb.Session, 0, len(sessions)+len(closedSessions))
sessions = append(sessions, closedSessions...)

reqUserNameNormalized := reqUsername.Normalized()
for _, session := range sessions {
if reqUsername.Normalized() != session.Username && !showAll {
if reqUserNameNormalized != session.Username && !showAll {
continue
}

if !isAdmin && hasViewActivityRedacted && (sessionUser != reqUsername) {
if !isAdmin && hasViewActivityRedacted && (reqUserNameNormalized != session.Username) {
// Remove queries with constants if user doesn't have correct privileges.
// Note that users can have both VIEWACTIVITYREDACTED and VIEWACTIVITY,
// with the former taking precedence.
for _, query := range session.ActiveQueries {
query.Sql = ""
for idx := range session.ActiveQueries {
session.ActiveQueries[idx].Sql = session.ActiveQueries[idx].SqlNoConstants
}
session.LastActiveQuery = ""
session.LastActiveQuery = session.LastActiveQueryNoConstants
}

userSessions = append(userSessions, session)
Expand Down
4 changes: 2 additions & 2 deletions pkg/server/status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3330,7 +3330,7 @@ func TestStatusAPIListSessions(t *testing.T) {
err = getStatusJSONProtoWithAdminOption(serverProto, "sessions", &resp, false)
require.NoError(t, err)
session := getSessionWithTestAppName(&resp)
require.Empty(t, session.LastActiveQuery)
require.Equal(t, session.LastActiveQuery, session.LastActiveQueryNoConstants)
require.Equal(t, "SELECT _", session.LastActiveQueryNoConstants)

// Grant VIEWACTIVITY, VIEWACTIVITYREDACTED should take precedence.
Expand All @@ -3340,7 +3340,7 @@ func TestStatusAPIListSessions(t *testing.T) {
require.NoError(t, err)
session = getSessionWithTestAppName(&resp)
require.Equal(t, appName, session.ApplicationName)
require.Empty(t, session.LastActiveQuery)
require.Equal(t, session.LastActiveQuery, session.LastActiveQueryNoConstants)
require.Equal(t, "SELECT _, _", session.LastActiveQueryNoConstants)

// Remove VIEWACTIVITYREDCATED. User should now see full query.
Expand Down

0 comments on commit d8a4bed

Please sign in to comment.