diff --git a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go index 52ec69440b45..317039fc3b4b 100644 --- a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go +++ b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go @@ -1893,6 +1893,13 @@ func TestTenantLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestTenantLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestTenantLogic_stats( t *testing.T, ) { diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 3d9db50fb401..8607bc79c4ec 100644 --- a/pkg/sql/crdb_internal.go +++ b/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("")) + } else { + if stats.Stats.SensitiveInfo.LastErr != "" { + errString = alloc.NewDString(tree.DString(stats.Stats.SensitiveInfo.LastErr)) + } } errCode := tree.DNull diff --git a/pkg/sql/logictest/testdata/logic_test/statement_statistics_errors b/pkg/sql/logictest/testdata/logic_test/statement_statistics_errors index d2329bb75d74..e1f1d123d4ee 100644 --- a/pkg/sql/logictest/testdata/logic_test/statement_statistics_errors +++ b/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 diff --git a/pkg/sql/logictest/testdata/logic_test/statement_statistics_errors_redacted b/pkg/sql/logictest/testdata/logic_test/statement_statistics_errors_redacted new file mode 100644 index 000000000000..efaca48b2189 --- /dev/null +++ b/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 + +# 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 +3D000 + +# 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 +3D000 +42704 + +# 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 +3D000 +42704 diff --git a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go index 2702962c07e6..cde8143f9c53 100644 --- a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go @@ -1871,6 +1871,13 @@ func TestLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestLogic_storing( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go index 9b5b4d4b10a4..66146e0e8635 100644 --- a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go @@ -1871,6 +1871,13 @@ func TestLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestLogic_stats( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/fakedist/generated_test.go b/pkg/sql/logictest/tests/fakedist/generated_test.go index aec1ef0da99f..b3dc43ab2240 100644 --- a/pkg/sql/logictest/tests/fakedist/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist/generated_test.go @@ -1885,6 +1885,13 @@ func TestLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestLogic_stats( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go b/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go index 0451e951c829..9ffbaefaddf8 100644 --- a/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go +++ b/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go @@ -1857,6 +1857,13 @@ func TestLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestLogic_stats( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go b/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go index 88d9c0b23138..135432046814 100644 --- a/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go +++ b/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go @@ -1836,6 +1836,13 @@ func TestLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestLogic_stats( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-vec-off/generated_test.go b/pkg/sql/logictest/tests/local-vec-off/generated_test.go index 50661f12d7a1..929ad67329ef 100644 --- a/pkg/sql/logictest/tests/local-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/local-vec-off/generated_test.go @@ -1885,6 +1885,13 @@ func TestLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestLogic_stats( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local/generated_test.go b/pkg/sql/logictest/tests/local/generated_test.go index 685c05074e50..03458c4f9042 100644 --- a/pkg/sql/logictest/tests/local/generated_test.go +++ b/pkg/sql/logictest/tests/local/generated_test.go @@ -2081,6 +2081,13 @@ func TestLogic_statement_statistics_errors( runLogicTest(t, "statement_statistics_errors") } +func TestLogic_statement_statistics_errors_redacted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "statement_statistics_errors_redacted") +} + func TestLogic_stats( t *testing.T, ) {