From 7984b94c1452d1464086af49ee37f3861b1f0da7 Mon Sep 17 00:00:00 2001 From: maryliag Date: Wed, 9 Aug 2023 17:44:43 -0400 Subject: [PATCH] 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 #108373 Release note (bug fix): Users with VIEWACTIVITY can now view correct values for timezone. --- pkg/server/BUILD.bazel | 1 + pkg/server/admin.go | 43 ++++++++++++++++++++++++++++++++-------- pkg/settings/registry.go | 23 +++++++++++++++++++++ 3 files changed, 59 insertions(+), 8 deletions(-) diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel index d221cd25746a..9c6ce5b677a2 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -347,6 +347,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", diff --git a/pkg/server/admin.go b/pkg/server/admin.go index fe09f260a56d..186b9f61b5b8 100644 --- a/pkg/server/admin.go +++ b/pkg/server/admin.go @@ -80,6 +80,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" @@ -2022,17 +2023,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.getUserAndRole(ctx) if err != nil { return nil, 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) @@ -2041,9 +2040,37 @@ func (s *adminServer) Settings( redactValues = false } } else { - // Non-root access cannot see the values in any case. + // Non-root access cannot see the values. + // Exception: users with VIEWACTIVITY and VIEWACTIVITYREDACTED can see cluster + // settings used by the UI Console. if err := s.adminPrivilegeChecker.requireViewClusterSettingOrModifyClusterSettingPermission(ctx); err != nil { - return nil, err + if err2 := s.adminPrivilegeChecker.requireViewActivityOrViewActivityRedactedPermission(ctx); err2 != nil { + // The check for VIEWACTIVITY or VIEWATIVITYREDACTED is a special case so cluster settings from + // the console can be returned, but if the user doesn't have them (i.e. err2 != nil), we don't want + // to share this error message, so only return `err`. + return nil, err + } + consoleSettingsOnly = true + } + } + + 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) + } + } } } @@ -2073,7 +2100,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 { diff --git a/pkg/settings/registry.go b/pkg/settings/registry.go index c74b2cff3c3a..9a638d9646f9 100644 --- a/pkg/settings/registry.go +++ b/pkg/settings/registry.go @@ -282,6 +282,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.