server: allow users with VIEWACTIVITY to get console settings

Previously, only users with `ADMIN`, `VIEWCLUSTERSETTING` or
`MODIFYCLUSTERSETTING` could get the settings on the
`_admin/v1/settings`. This API was used by the Console and then
a few places retrieved that information.
Users without those permissions would not be able to see the
values and some functionalities where not working as expected,
for example timezone was showing as UTC even when the cluster
setting `ui.display_timezone` was set to other value.

This commits modifies that endpoint (and that endpoint only)
to return just the cluster settings that are not sensitive and
that are required by the console to have all functionalities
working.

The list of cluster settings:
"cross_cluster_replication.enabled",
"keyvisualizer.enabled",
"keyvisualizer.sample_interval",
"sql.index_recommendation.drop_unused_duration",
"sql.insights.anomaly_detection.latency_threshold",
"sql.insights.high_retry_count.threshold",
"sql.insights.latency_threshold",
"sql.stats.automatic_collection.enabled",
"timeseries.storage.resolution_10s.ttl",
"timeseries.storage.resolution_30m.ttl",
"ui.display_timezone",
"version"

Part Of cockroachdb#108373

Release note (bug fix): Users with VIEWACTIVITY can now view
correct values for timezone.

pkg/server/BUILD.bazel

@@ -367,6 +367,7 @@ go_library(
         "@org_golang_google_grpc//codes",
         "@org_golang_google_grpc//metadata",
         "@org_golang_google_grpc//status",
+        "@org_golang_x_exp//slices",
     ] + select({
         "@io_bazel_rules_go//go/platform:aix": [
             "@org_golang_x_sys//unix",

pkg/server/admin.go

@@ -77,6 +77,7 @@ import (
 	"github.com/cockroachdb/errors"
 	gwruntime "github.com/grpc-ecosystem/grpc-gateway/runtime"
 	gwutil "github.com/grpc-ecosystem/grpc-gateway/utilities"
+	"golang.org/x/exp/slices"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	grpcstatus "google.golang.org/grpc/status"
@@ -1986,17 +1987,15 @@ func (s *adminServer) Settings(
 ) (*serverpb.SettingsResponse, error) {
 	ctx = s.AnnotateCtx(ctx)
 
-	keys := req.Keys
-	if len(keys) == 0 {
-		keys = settings.Keys(settings.ForSystemTenant)
-	}
-
 	_, isAdmin, err := s.privilegeChecker.GetUserAndRole(ctx)
 	if err != nil {
 		return nil, srverrors.ServerError(ctx, err)
 	}
 
 	redactValues := true
+	// Only returns non-sensitive settings that are required
+	// for features on DB Console.
+	consoleSettingsOnly := false
 	if isAdmin {
 		// Root accesses can customize the purpose.
 		// This is used by the UI to see all values (local access)
@@ -2007,7 +2006,32 @@ func (s *adminServer) Settings(
 	} else {
 		// Non-root access cannot see the values in any case.
 		if err := s.privilegeChecker.RequireViewClusterSettingOrModifyClusterSettingPermission(ctx); err != nil {
-			return nil, err
+			if err2 := s.privilegeChecker.RequireViewActivityOrViewActivityRedactedPermission(ctx); err2 != nil {
+				return nil, err
+			}
+			consoleSettingsOnly = true
+			log.Warningf(ctx, "only cluster settings used by DB Console are returned with "+
+				"VIEWACTIVITY or VIEWACTIVITYREDACTED")
+		}
+	}
+
+	var settingsKeys []string
+	if !consoleSettingsOnly {
+		settingsKeys = req.Keys
+		if len(settingsKeys) == 0 {
+			settingsKeys = settings.Keys(settings.ForSystemTenant)
+		}
+	} else {
+
+		if len(req.Keys) == 0 {
+			settingsKeys = settings.ConsoleKeys()
+		} else {
+			settingsKeys = []string{}
+			for _, k := range req.Keys {
+				if slices.Contains(settings.ConsoleKeys(), k) {
+					settingsKeys = append(settingsKeys, k)
+				}
+			}
 		}
 	}
 
@@ -2037,7 +2061,7 @@ func (s *adminServer) Settings(
 	}
 
 	resp := serverpb.SettingsResponse{KeyValues: make(map[string]serverpb.SettingsResponse_Value)}
-	for _, k := range keys {
+	for _, k := range settingsKeys {
 		var v settings.Setting
 		var ok bool
 		if redactValues {

pkg/server/application_api/BUILD.bazel

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

pkg/server/application_api/config_test.go

@@ -12,11 +12,13 @@ package application_api_test
 
 import (
 	"context"
+	"fmt"
 	"net/url"
 	"reflect"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/server/apiconstants"
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/server/srvtestutils"
 	"github.com/cockroachdb/cockroach/pkg/settings"
@@ -29,6 +31,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/safesql"
 	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 	"github.com/cockroachdb/errors"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/exp/slices"
 )
 
 func TestAdminAPISettings(t *testing.T) {
@@ -149,6 +153,91 @@ func TestAdminAPISettings(t *testing.T) {
 			checkSetting(t, k, v)
 		}
 	})
+
+	t.Run("different-permissions", func(t *testing.T) {
+		var resp serverpb.SettingsResponse
+		nonAdminUser := apiconstants.TestingUserNameNoAdmin().Normalized()
+		consoleKeys := settings.ConsoleKeys()
+
+		// Admin should return all cluster settings
+		if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings", &resp, true); err != nil {
+			t.Fatal(err)
+		}
+		require.True(t, len(resp.KeyValues) == len(allKeys))
+
+		// Admin requesting specific cluster setting should return that cluster setting
+		if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings?keys=sql.stats.persisted_rows.max",
+			&resp, true); err != nil {
+			t.Fatal(err)
+		}
+		require.NotNil(t, resp.KeyValues["sql.stats.persisted_rows.max"])
+		require.True(t, len(resp.KeyValues) == 1)
+
+		// Non-admin with no permission should return error message
+		err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings", &resp, false)
+		require.Error(t, err, "this operation requires the VIEWCLUSTERSETTING or MODIFYCLUSTERSETTING system privileges")
+
+		// Non-admin with VIEWCLUSTERSETTING permission should return all cluster settings
+		_, err = conn.Exec(fmt.Sprintf("ALTER USER %s VIEWCLUSTERSETTING", nonAdminUser))
+		require.NoError(t, err)
+		if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings", &resp, false); err != nil {
+			t.Fatal(err)
+		}
+		require.True(t, len(resp.KeyValues) == len(allKeys))
+
+		// Non-admin with VIEWCLUSTERSETTING permission requesting specific cluster setting should return that cluster setting
+		if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings?keys=sql.stats.persisted_rows.max",
+			&resp, false); err != nil {
+			t.Fatal(err)
+		}
+		require.NotNil(t, resp.KeyValues["sql.stats.persisted_rows.max"])
+		require.True(t, len(resp.KeyValues) == 1)
+
+		// Non-admin with VIEWCLUSTERSETTING and VIEWACTIVITY permission should return all cluster settings
+		_, err = conn.Exec(fmt.Sprintf("ALTER USER %s VIEWACTIVITY", nonAdminUser))
+		require.NoError(t, err)
+		if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings", &resp, false); err != nil {
+			t.Fatal(err)
+		}
+		require.True(t, len(resp.KeyValues) == len(allKeys))
+
+		// Non-admin with VIEWCLUSTERSETTING and VIEWACTIVITY permission requesting specific cluster setting
+		// should return that cluster setting
+		if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings?keys=sql.stats.persisted_rows.max",
+			&resp, false); err != nil {
+			t.Fatal(err)
+		}
+		require.NotNil(t, resp.KeyValues["sql.stats.persisted_rows.max"])
+		require.True(t, len(resp.KeyValues) == 1)
+
+		// Non-admin with VIEWACTIVITY and not VIEWCLUSTERSETTING should only see console cluster settings
+		_, err = conn.Exec(fmt.Sprintf("ALTER USER %s NOVIEWCLUSTERSETTING", nonAdminUser))
+		require.NoError(t, err)
+		if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings", &resp, false); err != nil {
+			t.Fatal(err)
+		}
+		require.True(t, len(resp.KeyValues) == len(consoleKeys))
+		for k := range resp.KeyValues {
+			require.True(t, slices.Contains(consoleKeys, k))
+		}
+
+		// Non-admin with VIEWACTIVITY and not VIEWCLUSTERSETTING permission requesting specific cluster setting
+		// from console should return that cluster setting
+		if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings?keys=ui.display_timezone",
+			&resp, false); err != nil {
+			t.Fatal(err)
+		}
+		require.NotNil(t, resp.KeyValues["ui.display_timezone"])
+		require.True(t, len(resp.KeyValues) == 1)
+
+		// Non-admin with VIEWACTIVITY and not VIEWCLUSTERSETTING permission requesting specific cluster setting
+		// that is not from console should not return that cluster setting
+		if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, "settings?keys=sql.stats.persisted_rows.max",
+			&resp, false); err != nil {
+			t.Fatal(err)
+		}
+		require.True(t, len(resp.KeyValues) == 0)
+	})
 }
 
 func TestClusterAPI(t *testing.T) {

pkg/settings/registry.go

@@ -290,6 +290,29 @@ func Keys(forSystemTenant bool) (res []string) {
 	return res
 }
 
+// ConsoleKeys return an array with all cluster settings keys
+// used by the UI Console.
+// This list should only contain settings that have no sensitive
+// information, because it will be returned on `settings` endpoint for
+// users without VIEWCLUSTERSETTING or MODIFYCLUSTERSETTING permission,
+// but that have VIEWACTIVITY or VIEWACTIVITYREDACTED permissions.
+func ConsoleKeys() (res []string) {
+	return []string{
+		"cross_cluster_replication.enabled",
+		"keyvisualizer.enabled",
+		"keyvisualizer.sample_interval",
+		"sql.index_recommendation.drop_unused_duration",
+		"sql.insights.anomaly_detection.latency_threshold",
+		"sql.insights.high_retry_count.threshold",
+		"sql.insights.latency_threshold",
+		"sql.stats.automatic_collection.enabled",
+		"timeseries.storage.resolution_10s.ttl",
+		"timeseries.storage.resolution_30m.ttl",
+		"ui.display_timezone",
+		"version",
+	}
+}
+
 // LookupForLocalAccess returns a NonMaskedSetting by name. Used when a setting
 // is being retrieved for local processing within the cluster and not for
 // reporting; sensitive values are accessible.