sql: fix VIEWACTIVITY privilege for ListSessions

Fixes cockroachdb#104354.
Partially addresses cockroachdb#106588.

Previously, when a non-admin user was given the `VIEWACTIVITY`
privilege, they were able to see other users' sessions from the SQL
shell but not from the UI.

This commit fixes the ListSessions endpoint to check for the
`VIEWACTIVITY` privilege in addition to the `VIEWACTIVITY` role when
returning a response for the ListSessions endpoint.

Release note (bug fix): users with the `VIEWACTIVITY` privilege should
be able to see other users' sessions from both the CLI and the DB
Console.

Author: gtr

pkg/server/application_api/BUILD.bazel

@@ -85,5 +85,6 @@ go_test(
         "@com_github_kr_pretty//:pretty",
         "@com_github_stretchr_testify//assert",
         "@com_github_stretchr_testify//require",
+        "@org_golang_x_sync//errgroup",
     ],
 )

pkg/server/application_api/sessions_test.go

@@ -34,6 +34,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/errors"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/sync/errgroup"
 )
 
 func TestListSessionsSecurity(t *testing.T) {
@@ -104,6 +105,128 @@ func TestListSessionsSecurity(t *testing.T) {
 	}
 }
 
+// TestListSessionsPrivileges tests that the VIEWACTIVITY and VIEWACTIVITYREDACTED privileges
+// are respected when listing sessions, particularly for other users' sessions.
+func TestListSessionsPrivileges(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	// Skip under stress race as the sleep query might finish before the stress race can finish.
+	skip.UnderStressRace(t, "list sessions privileges")
+
+	ts, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{})
+	defer ts.Stopper().Stop(context.Background())
+	endpoint := "sessions"
+	appName := "test_sessions_privileges"
+	user := apiconstants.TestingUserNameNoAdmin().Normalized()
+	sleepQuery := "SELECT pg_sleep(3000)"
+	sleepQueryRedacted := "SELECT pg_sleep(_)"
+
+	serverSQL := sqlutils.MakeSQLRunner(sqlDB)
+	serverSQL.Exec(t, fmt.Sprintf(`SET application_name = "%s"`, appName))
+	queryCtx, cancel := context.WithCancel(context.Background())
+
+	// Run a sleep query as root in another goroutine to make sure that root's session has one
+	// active query while we list sessions. This sleep query will be cancelled at the end of the
+	// test.
+	var g errgroup.Group
+	g.Go(func() error {
+		_, err := serverSQL.DB.ExecContext(queryCtx, sleepQuery)
+		if strings.Contains(err.Error(), "canceled") && strings.Contains(queryCtx.Err().Error(), "canceled") {
+			// Both errors contain the "canceled" substring, this means the query was
+			// canceled as expected.
+			return nil
+		}
+		t.Errorf("unexpected error: %v", err)
+		return err
+	})
+
+	// We test all combinations of VIEWACTIVITY and VIEWACTIVITYREDACTED. We could also start
+	// by granting all privileges and then revoking them one by one, but we want to keep the
+	// tests as isolated as possible. If a non-admin user has neither privilege, they should
+	// not see root's session. If a non-admin user has VIEWACTIVITY, they should see root's
+	// session with the full query. If a non-admin user has VIEWACTIVITYREDACTED, they should
+	// see root's session with the redacted query. If a non-admin user has both privileges,
+	// VIEWACTIVITYREDACTED should take precedence.
+	testCases := []struct {
+		grantViewActivity         bool
+		grantViewActivityRedacted bool
+		expectedQuery             string
+	}{
+		{false, false, ""},
+		{false, true, sleepQueryRedacted},
+		{true, false, sleepQuery},
+		{true, true, sleepQueryRedacted},
+	}
+
+	// Filters sessions by appName.
+	filterSessions := func(sessions []serverpb.Session) []serverpb.Session {
+		var filteredSessions []serverpb.Session
+		for _, s := range sessions {
+			if s.ApplicationName == appName {
+				filteredSessions = append(filteredSessions, s)
+			}
+		}
+		return filteredSessions
+	}
+
+	for _, tc := range testCases {
+		if tc.grantViewActivity {
+			serverSQL.Exec(t, fmt.Sprintf(`GRANT SYSTEM VIEWACTIVITY TO %s`, user))
+		}
+		if tc.grantViewActivityRedacted {
+			serverSQL.Exec(t, fmt.Sprintf(`GRANT SYSTEM VIEWACTIVITYREDACTED TO %s`, user))
+		}
+
+		var response serverpb.ListSessionsResponse
+		err := srvtestutils.GetStatusJSONProtoWithAdminOption(ts, endpoint, &response, false)
+
+		if err != nil {
+			t.Errorf("unexpected failure listing sessions from %s; error: %v; response errors: %v",
+				endpoint, err, response.Errors)
+		}
+
+		filteredSessions := filterSessions(response.Sessions)
+		numberOfSessions := len(filteredSessions)
+
+		// A non-admin user with no privileges should not see any other users' sessions.
+		if !tc.grantViewActivity && !tc.grantViewActivityRedacted {
+			if numberOfSessions != 0 {
+				t.Errorf("expected 0 sessions, but got %d", numberOfSessions)
+			}
+			continue
+		}
+
+		// A non-admin user with at least one of the privileges should see other users' sessions.
+		if numberOfSessions != 1 {
+			t.Errorf("expected 1 session, but got %d", numberOfSessions)
+		} else {
+			session := filteredSessions[0]
+			numberOfActiveQueries := len(session.ActiveQueries)
+			if numberOfActiveQueries != 1 {
+				t.Errorf("expected 1 active query, but got %d", numberOfActiveQueries)
+			} else {
+				activeQuery := session.ActiveQueries[0].Sql
+				if activeQuery != tc.expectedQuery {
+					t.Errorf("expected active query to be %s, but got %s", tc.expectedQuery, activeQuery)
+				}
+			}
+		}
+
+		// Only revoke the privilege if we granted it in this test case.
+		if tc.grantViewActivity {
+			serverSQL.Exec(t, fmt.Sprintf(`REVOKE SYSTEM VIEWACTIVITY FROM %s`, user))
+		}
+		if tc.grantViewActivityRedacted {
+			serverSQL.Exec(t, fmt.Sprintf(`REVOKE SYSTEM VIEWACTIVITYREDACTED FROM %s`, user))
+		}
+	}
+
+	// Cancel the query so that the test can finish.
+	cancel()
+	_ = g.Wait()
+}
+
 func TestStatusCancelSessionGatewayMetadataPropagation(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)

pkg/server/privchecker/api.go

@@ -77,6 +77,9 @@ type SQLPrivilegeChecker interface {
 
 	// HasGlobalPrivilege is a convenience wrapper
 	HasGlobalPrivilege(ctx context.Context, user username.SQLUsername, privilege privilege.Kind) (bool, error)
+
+	// HasPrivilegeOrRoleOption is a convenience wrapper
+	HasPrivilegeOrRoleOption(ctx context.Context, user username.SQLUsername, priv privilege.Kind) (bool, error)
 }
 
 // NewChecker constructs a new CheckerForRPCHandlers.

pkg/server/privchecker/privchecker.go

@@ -67,14 +67,11 @@ func (c *adminPrivilegeChecker) RequireViewActivityPermission(ctx context.Contex
 	if isAdmin {
 		return nil
 	}
-	if hasView, err := c.HasGlobalPrivilege(ctx, userName, privilege.VIEWACTIVITY); err != nil {
+	hasView, err := c.HasPrivilegeOrRoleOption(ctx, userName, privilege.VIEWACTIVITY)
+	if err != nil {
 		return srverrors.ServerError(ctx, err)
-	} else if hasView {
-		return nil
 	}
-	if hasView, err := c.HasRoleOption(ctx, userName, roleoption.VIEWACTIVITY); err != nil {
-		return srverrors.ServerError(ctx, err)
-	} else if hasView {
+	if hasView {
 		return nil
 	}
 	return grpcstatus.Errorf(
@@ -93,24 +90,18 @@ func (c *adminPrivilegeChecker) RequireViewActivityOrViewActivityRedactedPermiss
 	if isAdmin {
 		return nil
 	}
-	if hasView, err := c.HasGlobalPrivilege(ctx, userName, privilege.VIEWACTIVITY); err != nil {
+	hasView, err := c.HasPrivilegeOrRoleOption(ctx, userName, privilege.VIEWACTIVITY)
+	if err != nil {
 		return srverrors.ServerError(ctx, err)
-	} else if hasView {
-		return nil
 	}
-	if hasViewRedacted, err := c.HasGlobalPrivilege(ctx, userName, privilege.VIEWACTIVITYREDACTED); err != nil {
-		return srverrors.ServerError(ctx, err)
-	} else if hasViewRedacted {
+	if hasView {
 		return nil
 	}
-	if hasView, err := c.HasRoleOption(ctx, userName, roleoption.VIEWACTIVITY); err != nil {
+	hasViewRedacted, err := c.HasPrivilegeOrRoleOption(ctx, userName, privilege.VIEWACTIVITYREDACTED)
+	if err != nil {
 		return srverrors.ServerError(ctx, err)
-	} else if hasView {
-		return nil
 	}
-	if hasViewRedacted, err := c.HasRoleOption(ctx, userName, roleoption.VIEWACTIVITYREDACTED); err != nil {
-		return srverrors.ServerError(ctx, err)
-	} else if hasViewRedacted {
+	if hasViewRedacted {
 		return nil
 	}
 	return grpcstatus.Errorf(
@@ -129,24 +120,18 @@ func (c *adminPrivilegeChecker) RequireViewClusterSettingOrModifyClusterSettingP
 	if isAdmin {
 		return nil
 	}
-	if hasView, err := c.HasGlobalPrivilege(ctx, userName, privilege.VIEWCLUSTERSETTING); err != nil {
+	hasView, err := c.HasPrivilegeOrRoleOption(ctx, userName, privilege.VIEWCLUSTERSETTING)
+	if err != nil {
 		return srverrors.ServerError(ctx, err)
-	} else if hasView {
-		return nil
 	}
-	if hasModify, err := c.HasGlobalPrivilege(ctx, userName, privilege.MODIFYCLUSTERSETTING); err != nil {
-		return srverrors.ServerError(ctx, err)
-	} else if hasModify {
+	if hasView {
 		return nil
 	}
-	if hasView, err := c.HasRoleOption(ctx, userName, roleoption.VIEWCLUSTERSETTING); err != nil {
+	hasModify, err := c.HasPrivilegeOrRoleOption(ctx, userName, privilege.MODIFYCLUSTERSETTING)
+	if err != nil {
 		return srverrors.ServerError(ctx, err)
-	} else if hasView {
-		return nil
 	}
-	if hasModify, err := c.HasRoleOption(ctx, userName, roleoption.MODIFYCLUSTERSETTING); err != nil {
-		return srverrors.ServerError(ctx, err)
-	} else if hasModify {
+	if hasModify {
 		return nil
 	}
 	return grpcstatus.Errorf(
@@ -307,6 +292,28 @@ func (c *adminPrivilegeChecker) HasRoleOption(
 	return bool(dbDatum), nil
 }
 
+// HasPrivilegeOrRoleOption is a helper function which calls both HasGlobalPrivilege and HasRoleOption.
+func (c *adminPrivilegeChecker) HasPrivilegeOrRoleOption(
+	ctx context.Context, username username.SQLUsername, privilege privilege.Kind,
+) (bool, error) {
+	if privilegeName, err := c.HasGlobalPrivilege(ctx, username, privilege); err != nil {
+		return false, err
+	} else if privilegeName {
+		return true, nil
+	}
+	privName := privilege.String()
+	roleOption, ok := roleoption.ByName[privName]
+	if !ok {
+		return false, nil
+	}
+	if hasRoleOption, err := c.HasRoleOption(ctx, username, roleOption); err != nil {
+		return false, err
+	} else if hasRoleOption {
+		return true, nil
+	}
+	return false, nil
+}
+
 // HasGlobalPrivilege is a helper function which calls
 // CheckPrivilege and returns a true/false based on the returned
 // result.

pkg/server/status.go

@@ -158,12 +158,12 @@ func (b *baseStatusServer) getLocalSessions(
 		return nil, srverrors.ServerError(ctx, err)
 	}
 
-	hasViewActivityRedacted, err := b.privilegeChecker.HasRoleOption(ctx, sessionUser, roleoption.VIEWACTIVITYREDACTED)
+	hasViewActivityRedacted, err := b.privilegeChecker.HasPrivilegeOrRoleOption(ctx, sessionUser, privilege.VIEWACTIVITYREDACTED)
 	if err != nil {
 		return nil, srverrors.ServerError(ctx, err)
 	}
 
-	hasViewActivity, err := b.privilegeChecker.HasRoleOption(ctx, sessionUser, roleoption.VIEWACTIVITY)
+	hasViewActivity, err := b.privilegeChecker.HasPrivilegeOrRoleOption(ctx, sessionUser, privilege.VIEWACTIVITY)
 	if err != nil {
 		return nil, srverrors.ServerError(ctx, err)
 	}
@@ -204,6 +204,11 @@ func (b *baseStatusServer) getLocalSessions(
 		}
 	}
 
+	// At this point, we have decided if we are going to show all sessions so we
+	// can set the username to the session user if it is undefined.
+	if reqUsername.Undefined() {
+		reqUsername = sessionUser
+	}
 	reqUserNameNormalized := reqUsername.Normalized()
 
 	userSessions := make([]serverpb.Session, 0, len(sessions)+len(closedSessions))