Merge #107076

107076: sql: add system privileges to error messages in statement stats tables r=gtr a=gtr

Part of #87785.

This commit implements the `VIEWACTIVITY` and `VIEWACTIVITYREDACTED` system
privileges to the `crdb_internal.node_statement_statistics` table to
provide fine-grained permissions to view the error message for failed
statements.

Release note (sql change): the `crdb_interanal.node_statement_statistics`
table redact the error message if the user has `VIEWACTIVITYREDACTED`,
and do not redact the error message if the user has `VIEWACTIVITY`. If the
user has both, `VIEWACTIVITYREDACTED` takes precedence and the last error
is redacted.

Co-authored-by: gtr <[email protected]>

pkg/sql/crdb_internal.go

@@ -1516,12 +1516,24 @@ CREATE TABLE crdb_internal.node_statement_statistics (
   latency_seconds_p99 FLOAT
 )`,
 	populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error {
-		hasViewActivityOrViewActivityRedacted, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
-		if err != nil {
+		shouldRedactError := false
+		// Check if the user is admin.
+		if isAdmin, err := p.HasAdminRole(ctx); err != nil {
 			return err
-		}
-		if !hasViewActivityOrViewActivityRedacted {
-			return noViewActivityOrViewActivityRedactedRoleError(p.User())
+		} else if !isAdmin {
+			// If the user is not admin, check the individual VIEWACTIVITY and VIEWACTIVITYREDACTED
+			// privileges.
+			if hasViewActivityRedacted, err := p.HasViewActivityRedacted(ctx); err != nil {
+				return err
+			} else if hasViewActivityRedacted {
+				shouldRedactError = 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())
+			}
 		}
 
 		var alloc tree.DatumAlloc
@@ -1530,8 +1542,12 @@ CREATE TABLE crdb_internal.node_statement_statistics (
 
 		statementVisitor := func(_ context.Context, stats *appstatspb.CollectedStatementStatistics) error {
 			errString := tree.DNull
-			if stats.Stats.SensitiveInfo.LastErr != "" {
-				errString = alloc.NewDString(tree.DString(stats.Stats.SensitiveInfo.LastErr))
+			if shouldRedactError {
+				errString = alloc.NewDString(tree.DString("<redacted>"))
+			} else {
+				if stats.Stats.SensitiveInfo.LastErr != "" {
+					errString = alloc.NewDString(tree.DString(stats.Stats.SensitiveInfo.LastErr))
+				}
 			}
 
 			errCode := tree.DNull

pkg/sql/logictest/testdata/logic_test/statement_statistics_errors

@@ -6,14 +6,16 @@
 # by CRDB ("$ internal-migration-manager-find-jobs" for example) since they can be flaky. We are
 # also ordering by "last_error_code" to maintain consistency.
 
+user root
+
 # Test 1: division by zero. Error code "22012" should be added.
 statement error division by zero
 SELECT 2/0;
 
-query T
-SELECT last_error_code FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC;
+query TT
+SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC;
 ----
-22012
+22012  division by zero
 
 # Test 2: database does not exist. Error code "3D000" should be added.
 query TTTTTT colnames,rowsort
@@ -28,34 +30,34 @@ test           root   NULL            NULL              {}       NULL
 statement error pq: database "posgres" does not exist
 use posgres
 
-query T
-SELECT last_error_code FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC;
+query TT
+SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC;
 ----
-22012
-3D000
+22012  division by zero
+3D000  database "posgres" does not exist
 
 # Test 3: Nonexistant user. Error code "42704" should be added.
 statement error pq: role/user "who" does not exist
 ALTER DEFAULT PRIVILEGES GRANT SELECT ON TABLES to who
 
-query T
-SELECT last_error_code FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC;
+query TT
+SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC;
 ----
-22012
-3D000
-42704
+22012  division by zero
+3D000  database "posgres" does not exist
+42704  role/user "who" does not exist 
 
 # Test 4: Insufficient privilege. Error code "42501" should be added.
 statement error schema cannot be modified: "crdb_internal"
 CREATE TABLE crdb_internal.example (abc INT)
 
-query T
-SELECT last_error_code FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC;
+query TT
+SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC;
 ----
-22012
-3D000
-42501
-42704
+22012  division by zero
+3D000  database "posgres" does not exist
+42501  schema cannot be modified: "crdb_internal"
+42704  role/user "who" does not exist
 
 # Test 5: Foreign key violation. Error code "23503" should be added.
 statement ok
@@ -67,11 +69,11 @@ INSERT INTO dst(x) VALUES ('example')
 statement error foreign key
 UPDATE dst SET x = 'xyz'
 
-query T
-SELECT last_error_code FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC;
+query TT
+SELECT last_error_code, last_error   FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC;
 ----
-22012
-23503
-3D000
-42501
-42704
+22012  division by zero
+23503  update on table "dst" violates foreign key constraint "dst_x_fkey"
+3D000  database "posgres" does not exist
+42501  schema cannot be modified: "crdb_internal"
+42704  role/user "who" does not exist

pkg/sql/logictest/testdata/logic_test/statement_statistics_errors_redacted

@@ -0,0 +1,96 @@
+# Check that error messages and error codes are being surfaced correctly
+#
+# For these tests, a statement containing an intentional error is made and the
+# crdb_internal.node_statement_statistics virtual table is queried to validate that the error code
+# is recorded. When querying the node statistics table, we are ignoring statements made internally
+# by CRDB ("$ internal-migration-manager-find-jobs" for example) since they can be flaky. We are
+# also ordering by "last_error_code" to maintain consistency. Here we are also testing that the
+# error message is redacted for the VIEWACTIVITYREDACTED privilege.
+
+
+# Grant testuser the VIEWACTIVITYREDACTED system privilege.
+statement ok
+GRANT SYSTEM VIEWACTIVITYREDACTED TO testuser;
+
+query TTB
+SHOW SYSTEM GRANTS
+----
+testuser  VIEWACTIVITYREDACTED  false
+
+
+# Switch to testuser
+user testuser
+
+# Test 1: division by zero. Error code "22012" should be added.
+statement error division by zero
+SELECT 2/0;
+
+query TT
+SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC;
+----
+22012  <redacted>
+
+# Test 2: database does not exist. Error code "3D000" should be added.
+query TTTTTT colnames,rowsort
+SHOW DATABASES
+----
+database_name  owner  primary_region  secondary_region  regions  survival_goal
+defaultdb      root   NULL            NULL              {}       NULL
+postgres       root   NULL            NULL              {}       NULL
+test           root   NULL            NULL              {}       NULL
+
+statement error pq: database "posgres" does not exist
+use posgres
+
+query TT
+SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC;
+----
+22012  <redacted>
+3D000  <redacted>
+
+# Test 3: Nonexistant user. Error code "42704" should be added.
+statement error pq: role/user "who" does not exist
+ALTER DEFAULT PRIVILEGES GRANT SELECT ON TABLES to who
+
+query TT
+SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC;
+----
+22012  <redacted>
+3D000  <redacted>
+42704  <redacted>
+
+# Test 4: Give testuser the VIEWACTIVITY system privilege and remove the VIEWACTIVITYREDACTED privilege.
+user root
+
+statement ok
+GRANT SYSTEM VIEWACTIVITY TO testuser
+
+statement ok
+REVOKE SYSTEM VIEWACTIVITYREDACTED FROM testuser
+
+user testuser
+
+# Now the error message should not be redacted.
+
+query TT
+SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC;
+----
+22012  division by zero
+3D000  database "posgres" does not exist
+42704  role/user "who" does not exist
+
+# Test 5: Give tesuser the VIEWACTIVITYREDACTED privilege again. This time, the error message should be redacted,
+# as VIEWACTIVITYREDACTED takes precedence over VIEWACTIVITY.
+user root
+
+statement ok
+GRANT SYSTEM VIEWACTIVITYREDACTED TO testuser
+
+user testuser
+
+query TT
+SELECT last_error_code, last_error FROM crdb_internal.node_statement_statistics WHERE last_error_code!='NULL' AND application_name NOT LIKE '$ %' ORDER BY last_error_code ASC;
+----
+22012  <redacted>
+3D000  <redacted>
+42704  <redacted>