diff --git a/pkg/server/status_test.go b/pkg/server/status_test.go index 1ca5700da405..464daa6eec6c 100644 --- a/pkg/server/status_test.go +++ b/pkg/server/status_test.go @@ -48,6 +48,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catconstants" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/execinfrapb" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sqlstats" "github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats" "github.com/cockroachdb/cockroach/pkg/sql/tests" @@ -2907,3 +2908,30 @@ func TestSQLStatsAPIInMixedVersionState(t *testing.T) { "SELECT count(*) FROM system.transaction_statistics", [][]string{{"0"}}) }) } + +func TestUnprivilegedUserResetIndexUsageStats(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ctx := context.Background() + + s, conn, _ := serverutils.StartServer(t, base.TestServerArgs{}) + defer s.Stopper().Stop(ctx) + + sqlConn := sqlutils.MakeSQLRunner(conn) + sqlConn.Exec(t, "CREATE USER nonAdminUser") + + ie := s.InternalExecutor().(*sql.InternalExecutor) + + _, err := ie.ExecEx( + ctx, + "test-reset-index-usage-stats-as-non-admin-user", + nil, /* txn */ + sessiondata.InternalExecutorOverride{ + User: security.MakeSQLUsernameFromPreNormalizedString("nonAdminUser"), + }, + "SELECT crdb_internal.reset_index_usage_stats()", + ) + + require.Contains(t, err.Error(), "requires admin privilege") +} diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 0d3784192d8f..737164db6ddc 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -6127,12 +6127,20 @@ table's zone configuration this will return NULL.`, ), "crdb_internal.reset_index_usage_stats": makeBuiltin( tree.FunctionProperties{ - Category: categorySystemInfo, + Category: categorySystemInfo, + DistsqlBlocklist: true, // applicable only on the gateway }, tree.Overload{ Types: tree.ArgTypes{}, ReturnType: tree.FixedReturnType(types.Bool), Fn: func(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) { + isAdmin, err := evalCtx.SessionAccessor.HasAdminRole(evalCtx.Ctx()) + if err != nil { + return nil, err + } + if !isAdmin { + return nil, errors.New("crdb_internal.reset_index_usage_stats() requires admin privilege") + } if evalCtx.IndexUsageStatsController == nil { return nil, errors.AssertionFailedf("index usage stats controller not set") } @@ -6148,12 +6156,20 @@ table's zone configuration this will return NULL.`, ), "crdb_internal.reset_sql_stats": makeBuiltin( tree.FunctionProperties{ - Category: categorySystemInfo, + Category: categorySystemInfo, + DistsqlBlocklist: true, // applicable only on the gateway }, tree.Overload{ Types: tree.ArgTypes{}, ReturnType: tree.FixedReturnType(types.Bool), Fn: func(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) { + isAdmin, err := evalCtx.SessionAccessor.HasAdminRole(evalCtx.Ctx()) + if err != nil { + return nil, err + } + if !isAdmin { + return nil, errors.New("crdb_internal.reset_sql_stats() requires admin privilege") + } if evalCtx.SQLStatsController == nil { return nil, errors.AssertionFailedf("sql stats controller not set") } diff --git a/pkg/sql/sem/builtins/builtins_test.go b/pkg/sql/sem/builtins/builtins_test.go index a7e845f34dbb..744358359eca 100644 --- a/pkg/sql/sem/builtins/builtins_test.go +++ b/pkg/sql/sem/builtins/builtins_test.go @@ -12,18 +12,14 @@ package builtins import ( "bytes" - "context" "fmt" "math/rand" "testing" "time" - "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" - "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" - "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/duration" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/timeutil" @@ -370,35 +366,6 @@ func TestExtractTimeSpanFromTimeTZ(t *testing.T) { } } -// TestResetIndexUsageStatsOnRemoteSQLNode asserts that the built-in for -// resetting index usage statistics works when it's being set up on a remote -// node via DistSQL. -func TestResetIndexUsageStatsOnRemoteSQLNode(t *testing.T) { - defer leaktest.AfterTest(t)() - ctx := context.Background() - - testCluster := serverutils.StartNewTestCluster(t, 3 /* numNodes */, base.TestClusterArgs{ - ServerArgs: base.TestServerArgs{}, - }) - defer testCluster.Stopper().Stop(ctx) - testConn := testCluster.ServerConn(2 /* idx */) - sqlDB := sqlutils.MakeSQLRunner(testConn) - - query := ` -CREATE TABLE t (k INT PRIMARY KEY); -INSERT INTO t SELECT generate_series(1, 30); - -ALTER TABLE t SPLIT AT VALUES (10), (20); -ALTER TABLE t EXPERIMENTAL_RELOCATE LEASE SELECT 1, 1; -ALTER TABLE t EXPERIMENTAL_RELOCATE LEASE SELECT 2, 15; -ALTER TABLE t EXPERIMENTAL_RELOCATE LEASE SELECT 3, 25; - -SELECT count(*) FROM t WHERE crdb_internal.reset_index_usage_stats(); -` - - sqlDB.Exec(t, query) -} - func TestExtractTimeSpanFromInterval(t *testing.T) { defer leaktest.AfterTest(t)() diff --git a/pkg/sql/sqlstats/sslocal/BUILD.bazel b/pkg/sql/sqlstats/sslocal/BUILD.bazel index 25e8aafe58b4..b50b20f00757 100644 --- a/pkg/sql/sqlstats/sslocal/BUILD.bazel +++ b/pkg/sql/sqlstats/sslocal/BUILD.bazel @@ -51,6 +51,7 @@ go_test( "//pkg/settings", "//pkg/settings/cluster", "//pkg/sql", + "//pkg/sql/sessiondata", "//pkg/sql/sessionphase", "//pkg/sql/sqlstats", "//pkg/sql/sqlstats/persistedsqlstats", diff --git a/pkg/sql/sqlstats/sslocal/sql_stats_test.go b/pkg/sql/sqlstats/sslocal/sql_stats_test.go index a1b3cd18679a..f89aa614e3c8 100644 --- a/pkg/sql/sqlstats/sslocal/sql_stats_test.go +++ b/pkg/sql/sqlstats/sslocal/sql_stats_test.go @@ -17,10 +17,12 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sessionphase" "github.com/cockroachdb/cockroach/pkg/sql/sqlstats" "github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats" @@ -640,3 +642,30 @@ WHERE `, [][]string{{"0"}}) } + +func TestUnprivilegedUserReset(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ctx := context.Background() + + s, conn, _ := serverutils.StartServer(t, base.TestServerArgs{}) + defer s.Stopper().Stop(ctx) + + sqlConn := sqlutils.MakeSQLRunner(conn) + sqlConn.Exec(t, "CREATE USER nonAdminUser") + + ie := s.InternalExecutor().(*sql.InternalExecutor) + + _, err := ie.ExecEx( + ctx, + "test-reset-sql-stats-as-non-admin-user", + nil, /* txn */ + sessiondata.InternalExecutorOverride{ + User: security.MakeSQLUsernameFromPreNormalizedString("nonAdminUser"), + }, + "SELECT crdb_internal.reset_sql_stats()", + ) + + require.Contains(t, err.Error(), "requires admin privilege") +}