From 3cd24e8377c219b1dd05ef612e0366bd5c3c3dad Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Fri, 1 Mar 2024 13:53:43 -0500 Subject: [PATCH] sql: allow user to see their own queries without needing a privilege Release note (bug fix): The SHOW QUERIES and SHOW STATEMENTS commands were incorrectly requiring that the user has the VIEWACTIVITY or VIEWACTIVITYREDACTED privilege. However, a user always should be able to view their own queries, even without this privilege. This is fixed now. --- pkg/sql/crdb_internal.go | 32 +++++++++----- pkg/sql/show_test.go | 95 ++++++++++++++++++++++++---------------- 2 files changed, 80 insertions(+), 47 deletions(-) diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index d13f0cd01cc7..31db03c41dfe 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -2674,10 +2674,13 @@ func populateQueriesTable( addRow func(...tree.Datum) error, response *serverpb.ListSessionsResponse, ) error { - shouldRedactQuery := false + shouldRedactOtherUserQuery := false + canViewOtherUser := false // Check if the user is admin. if isAdmin, err := p.HasAdminRole(ctx); err != nil { return err + } else if isAdmin { + canViewOtherUser = true } else if !isAdmin { // If the user is not admin, check the individual VIEWACTIVITY and VIEWACTIVITYREDACTED // privileges. @@ -2686,16 +2689,24 @@ func populateQueriesTable( } else if hasViewActivityRedacted { // If the user has VIEWACTIVITYREDACTED, redact the query as it takes precedence // over VIEWACTIVITY. - shouldRedactQuery = true - } else if hasViewActivity, err := p.HasViewActivity(ctx); err != nil { - return err - } else if !hasViewActivity { - // If the user is not admin and does not have VIEWACTIVITY or VIEWACTIVITYREDACTED, - // return insufficient privileges error. - return noViewActivityOrViewActivityRedactedRoleError(p.User()) + shouldRedactOtherUserQuery = true + canViewOtherUser = true + } else if !hasViewActivityRedacted { + if hasViewActivity, err := p.HasViewActivity(ctx); err != nil { + return err + } else if hasViewActivity { + canViewOtherUser = true + } } } for _, session := range response.Sessions { + normalizedUser, err := username.MakeSQLUsernameFromUserInput(session.Username, username.PurposeValidation) + if err != nil { + return err + } + if !canViewOtherUser && normalizedUser != p.User() { + continue + } sessionID := getSessionID(session) for _, query := range session.ActiveQueries { isDistributedDatum := tree.DNull @@ -2739,8 +2750,9 @@ func populateQueriesTable( // Interpolate placeholders into the SQL statement. sql := formatActiveQuery(query) - // If the user does not have the correct privileges, show the query without literals or constants. - if shouldRedactQuery { + // If the user does not have the correct privileges, show the query + // without literals or constants. + if shouldRedactOtherUserQuery && session.Username != p.SessionData().User().Normalized() { sql = query.SqlNoConstants } if err := addRow( diff --git a/pkg/sql/show_test.go b/pkg/sql/show_test.go index edfab798bc74..214d8e61fc0a 100644 --- a/pkg/sql/show_test.go +++ b/pkg/sql/show_test.go @@ -1078,16 +1078,18 @@ func TestShowRedactedActiveStatements(t *testing.T) { _ = sqlDBroot.Exec(t, `GRANT SYSTEM VIEWACTIVITYREDACTED TO bothperms`) type user struct { - username string - canViewTable bool // Can the user view the `cluster_queries` table? - isQueryRedacted bool // Is the user's query redacted? - sqlRunner *sqlutils.SQLRunner + username string + canViewOtherRows bool // Can the user view other users' rows in the table? + isQueryRedacted bool // Are the other userss queries redacted? + sqlRunner *sqlutils.SQLRunner } - // A user with no permissions should not see the table. A user with only - // VIEWACTIVITY should be able to see the whole query. A user with only - // VIEWACTIVITYREDACTED should see a redacted query. A user with both should - // see the redacted query, as VIEWACTIVITYREDACTED takes precedence. + // A user with no permissions should only be able to see their own rows + // in the table. + // A user with only VIEWACTIVITY should be able to see the whole query. + // A user with only VIEWACTIVITYREDACTED should see a redacted query. + // A user with both should see the redacted query, as VIEWACTIVITYREDACTED + // takes precedence. users := []user{ {"onlyviewactivityredacted", true, true, nil}, {"onlyviewactivity", true, false, nil}, @@ -1126,10 +1128,10 @@ func TestShowRedactedActiveStatements(t *testing.T) { // Wait for the start signal. <-startSignal - selectQuery := `SELECT query FROM [SHOW CLUSTER QUERIES] WHERE query LIKE 'SELECT pg_sleep%'` + selectRootQuery := `SELECT query FROM [SHOW CLUSTER QUERIES] WHERE query LIKE 'SELECT pg_sleep%' AND user_name = 'root'` testutils.SucceedsSoon(t, func() error { - rows := sqlDBroot.Query(t, selectQuery) + rows := sqlDBroot.Query(t, selectRootQuery) defer rows.Close() count := 0 for rows.Next() { @@ -1150,41 +1152,60 @@ func TestShowRedactedActiveStatements(t *testing.T) { for _, u := range users { t.Run(u.username, func(t *testing.T) { - // Make sure that if the user can't view the table, they get an error. - if !u.canViewTable { - u.sqlRunner.ExpectErr(t, "does not have VIEWACTIVITY or VIEWACTIVITYREDACTED privilege", selectQuery) - } else { - rows := u.sqlRunner.Query(t, selectQuery) - defer rows.Close() - if err := rows.Err(); err != nil { + rootRows := u.sqlRunner.Query(t, selectRootQuery) + defer func() { require.NoError(t, rootRows.Close()) }() + + count := 0 + for rootRows.Next() { + count++ + + var query string + if err := rootRows.Scan(&query); err != nil { t.Fatal(err) } - count := 0 - for rows.Next() { - count++ - var query string - if err := rows.Scan(&query); err != nil { - t.Fatal(err) + t.Log(query) + // Make sure that if the user is supposed to see a redacted query, they do. + if u.isQueryRedacted { + if !strings.HasPrefix(query, "SELECT pg_sleep(_)") { + t.Fatalf("Expected `SELECT pg_sleep(_)`, got %s", query) } - - t.Log(query) - // Make sure that if the user is supposed to see a redacted query, they do. - if u.isQueryRedacted { - if !strings.HasPrefix(query, "SELECT pg_sleep(_)") { - t.Fatalf("Expected `SELECT pg_sleep(_)`, got %s", query) - } - // Make sure that if the user is supposed to see the full query, they do. - } else { - if !strings.HasPrefix(query, "SELECT pg_sleep(30)") { - t.Fatalf("Expected `SELECT pg_sleep(30)`, got %s", query) - } + // Make sure that if the user is supposed to see the full query, they do. + } else { + if !strings.HasPrefix(query, "SELECT pg_sleep(30)") { + t.Fatalf("Expected `SELECT pg_sleep(30)`, got %s", query) } } - if count != 1 { - t.Fatalf("expected 1 row, got %d", count) + } + if u.canViewOtherRows { + require.Equalf(t, 1, count, "expected 1 row, got %d", count) + } else { + require.Equalf(t, 0, count, "expected 0 rows, got %d", count) + } + + selectOwnQuery := fmt.Sprintf( + `SELECT query FROM [SHOW CLUSTER QUERIES] WHERE query LIKE 'SELECT query FROM%%' AND user_name = '%s'`, + u.username, + ) + ownRows := u.sqlRunner.Query(t, selectOwnQuery) + defer func() { require.NoError(t, ownRows.Close()) }() + + count = 0 + for ownRows.Next() { + count++ + + var query string + if err := ownRows.Scan(&query); err != nil { + t.Fatal(err) + } + + t.Log(query) + // Any user can always see their own unredacted queries. + if !strings.Contains(query, fmt.Sprintf("user_name = '%s'", u.username)) { + t.Fatalf("Expected unredacted query, got %s", query) } } + require.Equalf(t, 1, count, "expected 1 row, got %d", count) }) }