diff --git a/pkg/ccl/backupccl/alter_backup_schedule.go b/pkg/ccl/backupccl/alter_backup_schedule.go index bcd236ea0a6c..ec153e15a084 100644 --- a/pkg/ccl/backupccl/alter_backup_schedule.go +++ b/pkg/ccl/backupccl/alter_backup_schedule.go @@ -24,7 +24,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/parser" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/errors" pbtypes "github.com/gogo/protobuf/types" ) @@ -127,16 +129,16 @@ func doAlterBackupSchedules( s.incJob.ScheduleID()) } - // Check that the user is admin or the owner of the schedules being altered. - isAdmin, err := p.UserHasAdminRole(ctx, p.User()) + // Check that the user has privileges or is the owner of the schedules being altered. + hasPriv, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA, p.User()) if err != nil { return err } isOwnerOfFullJob := s.fullJob == nil || s.fullJob.Owner() == p.User() isOwnerOfIncJob := s.incJob == nil || s.incJob.Owner() == p.User() - if !isAdmin && !(isOwnerOfFullJob && isOwnerOfIncJob) { - return pgerror.New(pgcode.InsufficientPrivilege, "must be admin or owner of the "+ - "schedules being altered.") + if !hasPriv && !(isOwnerOfFullJob && isOwnerOfIncJob) { + return pgerror.Newf(pgcode.InsufficientPrivilege, "must be admin or the owner of the "+ + "schedules being altered, or have %s privilege", privilege.REPAIRCLUSTERMETADATA) } if s, err = processFullBackupRecurrence( diff --git a/pkg/ccl/backupccl/testdata/backup-restore/schedule-privileges b/pkg/ccl/backupccl/testdata/backup-restore/schedule-privileges index fa9ab4fdbc47..28f98eb2f68f 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/schedule-privileges +++ b/pkg/ccl/backupccl/testdata/backup-restore/schedule-privileges @@ -108,7 +108,7 @@ foocluster_admin BACKUP INTO LATEST IN 'external://foo/cluster' WITH OPTIONS (de foocluster_admin BACKUP INTO 'external://foo/cluster' WITH OPTIONS (detached) # nonadmin testuser is not allowed to drop a schedule they do not own. -exec-sql expect-error-regex=(must be admin or owner of the schedule [0-9]+ to DROP it) user=testuser +exec-sql expect-error-regex=(must have REPAIRCLUSTERMETADATA privilege or be owner of the schedule [0-9]+ to DROP it) user=testuser DROP SCHEDULE $fullID ---- regex matches error @@ -141,7 +141,7 @@ let $otherFullID $otherIncID with schedules as (show schedules) select id from schedules where label='foocluster_admin_revoke' order by command->>'backup_type' asc; ---- -exec-sql expect-error-regex=(must be admin or owner of the schedule [0-9]+ to DROP it) user=testuser +exec-sql expect-error-regex=(must have REPAIRCLUSTERMETADATA privilege or be owner of the schedule [0-9]+ to DROP it) user=testuser DROP SCHEDULE $otherFullID ---- regex matches error @@ -180,17 +180,17 @@ DROP SCHEDULE $testuserIncID; ---- # But testuser can't drop, alter, resume or pause the root owned schedules. -exec-sql expect-error-regex=(must be admin or owner of the schedule [0-9]+ to PAUSE it) user=testuser +exec-sql expect-error-regex=(must have REPAIRCLUSTERMETADATA privilege or be owner of the schedule [0-9]+ to PAUSE it) user=testuser PAUSE SCHEDULE $otherFullID ---- regex matches error -exec-sql expect-error-regex=(must be admin or owner of the schedule [0-9]+ to RESUME it) user=testuser +exec-sql expect-error-regex=(must have REPAIRCLUSTERMETADATA privilege or be owner of the schedule [0-9]+ to RESUME it) user=testuser RESUME SCHEDULE $otherFullID ---- regex matches error -exec-sql expect-error-regex=(must be admin or owner of the schedule [0-9]+ to DROP it) user=testuser +exec-sql expect-error-regex=(must have REPAIRCLUSTERMETADATA privilege or be owner of the schedule [0-9]+ to DROP it) user=testuser DROP SCHEDULE $otherFullID; ---- regex matches error diff --git a/pkg/ccl/cloudccl/externalconn/testdata/multi-tenant/show_create_external_connections b/pkg/ccl/cloudccl/externalconn/testdata/multi-tenant/show_create_external_connections index 840901baab39..f2a01adfa423 100644 --- a/pkg/ccl/cloudccl/externalconn/testdata/multi-tenant/show_create_external_connections +++ b/pkg/ccl/cloudccl/externalconn/testdata/multi-tenant/show_create_external_connections @@ -80,12 +80,12 @@ GRANT SYSTEM EXTERNALCONNECTION TO testuser query-sql user=testuser SHOW CREATE ALL EXTERNAL CONNECTIONS ---- -pq: must be admin to run `SHOW CREATE ALL EXTERNAL CONNECTIONS +pq: must have VIEWCLUSTERMETADATA privilege to run `SHOW CREATE ALL EXTERNAL CONNECTIONS` query-sql user=testuser SHOW CREATE EXTERNAL CONNECTION foo ---- -pq: must be admin or owner of the External Connection "foo" +pq: must have VIEWCLUSTERMETADATA privilege or be owner of the External Connection "foo" # Create External Connection where testuser is the owner, they should be able to SHOW this object. exec-sql user=testuser @@ -95,7 +95,7 @@ CREATE EXTERNAL CONNECTION bar AS 'nodelocal://1/foo' query-sql user=testuser SHOW CREATE ALL EXTERNAL CONNECTIONS ---- -pq: must be admin to run `SHOW CREATE ALL EXTERNAL CONNECTIONS +pq: must have VIEWCLUSTERMETADATA privilege to run `SHOW CREATE ALL EXTERNAL CONNECTIONS` # TODO(aditymaru): Synthetic privileges do not have a concept of owners. Once they do, testuser will # be able to run this query successfully since they are the owner of the External Connection object. diff --git a/pkg/ccl/cloudccl/externalconn/testdata/show_create_external_connections b/pkg/ccl/cloudccl/externalconn/testdata/show_create_external_connections index bb19510eab91..a25dd532b232 100644 --- a/pkg/ccl/cloudccl/externalconn/testdata/show_create_external_connections +++ b/pkg/ccl/cloudccl/externalconn/testdata/show_create_external_connections @@ -76,12 +76,12 @@ GRANT SYSTEM EXTERNALCONNECTION TO testuser query-sql user=testuser SHOW CREATE ALL EXTERNAL CONNECTIONS ---- -pq: must be admin to run `SHOW CREATE ALL EXTERNAL CONNECTIONS +pq: must have VIEWCLUSTERMETADATA privilege to run `SHOW CREATE ALL EXTERNAL CONNECTIONS` query-sql user=testuser SHOW CREATE EXTERNAL CONNECTION foo ---- -pq: must be admin or owner of the External Connection "foo" +pq: must have VIEWCLUSTERMETADATA privilege or be owner of the External Connection "foo" # Create External Connection where testuser is the owner, they should be able to SHOW this object. exec-sql user=testuser @@ -91,7 +91,7 @@ CREATE EXTERNAL CONNECTION bar AS 'nodelocal://1/foo' query-sql user=testuser SHOW CREATE ALL EXTERNAL CONNECTIONS ---- -pq: must be admin to run `SHOW CREATE ALL EXTERNAL CONNECTIONS +pq: must have VIEWCLUSTERMETADATA privilege to run `SHOW CREATE ALL EXTERNAL CONNECTIONS` # TODO(aditymaru): Synthetic privileges do not have a concept of owners. Once they do, testuser will # be able to run this query successfully since they are the owner of the External Connection object. diff --git a/pkg/ccl/serverccl/statusccl/tenant_status_test.go b/pkg/ccl/serverccl/statusccl/tenant_status_test.go index e716103b1c11..645bfd42e4ca 100644 --- a/pkg/ccl/serverccl/statusccl/tenant_status_test.go +++ b/pkg/ccl/serverccl/statusccl/tenant_status_test.go @@ -161,6 +161,17 @@ func testTenantSpanStats(ctx context.Context, t *testing.T, helper serverccl.Ten require.Error(t, err) require.Contains(t, err.Error(), "Forbidden") + // VIEWCLUSTERMETADATA should allow the user to see the span stats. + grantStmt := `GRANT SYSTEM VIEWCLUSTERMETADATA TO authentic_user_noadmin;` + helper.TestCluster().TenantConn(0).Exec(t, grantStmt) + + err = client.PostJSONChecked("/_status/span", &req, &resp) + require.NoError(t, err) + require.NotEmpty(t, resp.SpanToStats) + + revokeStmt := `REVOKE SYSTEM VIEWCLUSTERMETADATA FROM authentic_user_noadmin;` + helper.TestCluster().TenantConn(0).Exec(t, revokeStmt) + adminClient := helper.TestCluster().TenantHTTPClient(t, 1, true) adminClient.PostJSON("/_status/span", &req, &resp) require.Greaterf(t, resp.SpanToStats[aSpan.String()].RangeCount, int32(0), "positive range count") @@ -1511,6 +1522,9 @@ func testTenantHotRanges(_ context.Context, t *testing.T, helper serverccl.Tenan client.PostJSON("/_status/v2/hotranges", &req, &resp) require.NotEmpty(t, resp.Ranges) + + revokeStmt := `REVOKE SYSTEM VIEWCLUSTERMETADATA FROM authentic_user_noadmin;` + helper.TestCluster().TenantConn(0).Exec(t, revokeStmt) }) t.Run("test tenant hot ranges respects tenant isolation", func(t *testing.T) { diff --git a/pkg/server/admin.go b/pkg/server/admin.go index 78c8a0702c7b..e36fa47acb81 100644 --- a/pkg/server/admin.go +++ b/pkg/server/admin.go @@ -1582,7 +1582,7 @@ func (s *adminServer) Events( ) (_ *serverpb.EventsResponse, retErr error) { ctx = s.AnnotateCtx(ctx) - userName, err := s.requireAdminUser(ctx) + err := s.requireViewClusterMetadataPermission(ctx) if err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. @@ -1595,6 +1595,10 @@ func (s *adminServer) Events( limit = defaultAPIEventLimit } + userName, err := userFromIncomingRPCContext(ctx) + if err != nil { + return nil, serverError(ctx, err) + } r, err := s.eventsHelper(ctx, req, userName, int(limit), 0, redactEvents) if err != nil { return nil, serverError(ctx, err) @@ -3241,7 +3245,7 @@ func (s *systemAdminServer) EnqueueRange( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.requireAdminUser(ctx); err != nil { + if err := s.requireRepairClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -3389,7 +3393,7 @@ func (s *systemAdminServer) SendKVBatch( ctx = s.AnnotateCtx(ctx) // Note: the root user will bypass SQL auth checks, which is useful in case of // a cluster outage. - user, err := s.requireAdminUser(ctx) + err := s.requireRepairClusterMetadataPermission(ctx) if err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. @@ -3399,6 +3403,11 @@ func (s *systemAdminServer) SendKVBatch( return nil, grpcstatus.Errorf(codes.InvalidArgument, "BatchRequest cannot be nil") } + user, err := userFromIncomingRPCContext(ctx) + if err != nil { + return nil, serverError(ctx, err) + } + // Emit a structured log event for the call. jsonpb := protoutil.JSONPb{} baJSON, err := jsonpb.Marshal(ba) @@ -3447,7 +3456,7 @@ func (s *systemAdminServer) RecoveryCollectReplicaInfo( ) error { ctx := stream.Context() ctx = s.server.AnnotateCtx(ctx) - _, err := s.requireAdminUser(ctx) + err := s.requireViewClusterMetadataPermission(ctx) if err != nil { return err } @@ -3462,7 +3471,7 @@ func (s *systemAdminServer) RecoveryCollectLocalReplicaInfo( ) error { ctx := stream.Context() ctx = s.server.AnnotateCtx(ctx) - _, err := s.requireAdminUser(ctx) + err := s.requireViewClusterMetadataPermission(ctx) if err != nil { return err } @@ -3475,7 +3484,7 @@ func (s *systemAdminServer) RecoveryStagePlan( ctx context.Context, request *serverpb.RecoveryStagePlanRequest, ) (*serverpb.RecoveryStagePlanResponse, error) { ctx = s.server.AnnotateCtx(ctx) - _, err := s.requireAdminUser(ctx) + err := s.requireRepairClusterMetadataPermission(ctx) if err != nil { return nil, err } @@ -3488,7 +3497,7 @@ func (s *systemAdminServer) RecoveryNodeStatus( ctx context.Context, request *serverpb.RecoveryNodeStatusRequest, ) (*serverpb.RecoveryNodeStatusResponse, error) { ctx = s.server.AnnotateCtx(ctx) - _, err := s.requireAdminUser(ctx) + err := s.requireViewClusterMetadataPermission(ctx) if err != nil { return nil, err } @@ -3500,7 +3509,7 @@ func (s *systemAdminServer) RecoveryVerify( ctx context.Context, request *serverpb.RecoveryVerifyRequest, ) (*serverpb.RecoveryVerifyResponse, error) { ctx = s.server.AnnotateCtx(ctx) - _, err := s.requireAdminUser(ctx) + err := s.requireViewClusterMetadataPermission(ctx) if err != nil { return nil, err } @@ -3918,20 +3927,6 @@ type adminPrivilegeChecker struct { makePlanner func(opName string) (interface{}, func()) } -// requireAdminUser's error return is a gRPC error. -func (c *adminPrivilegeChecker) requireAdminUser( - ctx context.Context, -) (userName username.SQLUsername, err error) { - userName, isAdmin, err := c.getUserAndRole(ctx) - if err != nil { - return userName, serverError(ctx, err) - } - if !isAdmin { - return userName, errRequiresAdmin - } - return userName, nil -} - // requireViewActivityPermission's error return is a gRPC error. func (c *adminPrivilegeChecker) requireViewActivityPermission(ctx context.Context) (err error) { userName, isAdmin, err := c.getUserAndRole(ctx) @@ -4071,6 +4066,29 @@ func (c *adminPrivilegeChecker) requireViewClusterMetadataPermission( privilege.VIEWCLUSTERMETADATA) } +// requireRepairClusterMetadataPermission requires the user have admin +// or the REPAIRCLUSTERMETADATA system privilege and returns an error if +// the user does not have it. +func (c *adminPrivilegeChecker) requireRepairClusterMetadataPermission( + ctx context.Context, +) (err error) { + userName, isAdmin, err := c.getUserAndRole(ctx) + if err != nil { + return serverError(ctx, err) + } + if isAdmin { + return nil + } + if hasRepairClusterMetadata, err := c.hasGlobalPrivilege(ctx, userName, privilege.REPAIRCLUSTERMETADATA); err != nil { + return serverError(ctx, err) + } else if hasRepairClusterMetadata { + return nil + } + return grpcstatus.Errorf( + codes.PermissionDenied, "this operation requires the %s system privilege", + privilege.REPAIRCLUSTERMETADATA) +} + // requireViewDebugPermission requires the user have admin or the VIEWDEBUG system privilege // and returns an error if the user does not have it. func (c *adminPrivilegeChecker) requireViewDebugPermission(ctx context.Context) (err error) { diff --git a/pkg/server/index_usage_stats.go b/pkg/server/index_usage_stats.go index d342f6f78a77..addf77d6eef1 100644 --- a/pkg/server/index_usage_stats.go +++ b/pkg/server/index_usage_stats.go @@ -134,7 +134,7 @@ func (s *statusServer) ResetIndexUsageStats( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireRepairClusterMetadataPermission(ctx); err != nil { return nil, err } diff --git a/pkg/server/sql_stats.go b/pkg/server/sql_stats.go index 0fd95d1a820c..ecb7fe59a509 100644 --- a/pkg/server/sql_stats.go +++ b/pkg/server/sql_stats.go @@ -26,7 +26,7 @@ func (s *statusServer) ResetSQLStats( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireRepairClusterMetadataPermission(ctx); err != nil { return nil, err } diff --git a/pkg/server/status.go b/pkg/server/status.go index 88c1070cb534..24f7ed374db5 100644 --- a/pkg/server/status.go +++ b/pkg/server/status.go @@ -708,7 +708,7 @@ func (s *systemStatusServer) Gossip( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -736,7 +736,7 @@ func (s *systemStatusServer) EngineStats( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -875,7 +875,7 @@ func (s *systemStatusServer) CriticalNodes( ctx context.Context, req *serverpb.CriticalNodesRequest, ) (*serverpb.CriticalNodesResponse, error) { ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { return nil, err } conformance, err := s.node.SpanConfigConformance( @@ -1008,7 +1008,7 @@ func (s *statusServer) Certificates( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1129,7 +1129,7 @@ func (s *statusServer) Details( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1170,7 +1170,7 @@ func (s *statusServer) GetFiles( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1226,7 +1226,7 @@ func (s *statusServer) LogFilesList( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1261,7 +1261,7 @@ func (s *statusServer) LogFile( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1359,7 +1359,7 @@ func (s *statusServer) Logs( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1447,7 +1447,7 @@ func (s *statusServer) Stacks( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1480,7 +1480,7 @@ func (s *statusServer) Profile( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1560,8 +1560,8 @@ func (s *statusServer) NodesList( ctx = s.AnnotateCtx(ctx) // The node status contains details about the command line, network - // addresses, env vars etc which are admin-only. - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + // addresses, env vars etc which are privileged information. + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1835,8 +1835,8 @@ func (s *statusServer) Node( ctx = s.AnnotateCtx(ctx) // The node status contains details about the command line, network - // addresses, env vars etc which are admin-only. - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + // addresses, env vars etc which are privileged information.. + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -2255,8 +2255,8 @@ func (t *statusServer) TenantRanges( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = t.AnnotateCtx(ctx) - // The tenant range report contains replica metadata which is admin-only. - if _, err := t.privilegeChecker.requireAdminUser(ctx); err != nil { + // The tenant range report contains replica metadata which is privileged. + if err := t.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { return nil, err } @@ -2268,7 +2268,7 @@ func (s *systemStatusServer) TenantRanges( ) (*serverpb.TenantRangesResponse, error) { forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { return nil, err } @@ -2727,7 +2727,7 @@ func (s *statusServer) KeyVisSamples( ctx context.Context, req *serverpb.KeyVisSamplesRequest, ) (*serverpb.KeyVisSamplesResponse, error) { - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { return nil, err } @@ -3471,7 +3471,7 @@ func (s *statusServer) SpanStats( ctx context.Context, req *roachpb.SpanStatsRequest, ) (*roachpb.SpanStatsResponse, error) { ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -3490,7 +3490,7 @@ func (s *systemStatusServer) SpanStats( ) (*roachpb.SpanStatsResponse, error) { ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -3661,7 +3661,7 @@ func (s *statusServer) JobRegistryStatus( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -3699,7 +3699,7 @@ func (s *statusServer) JobStatus( ) (*serverpb.JobStatusResponse, error) { ctx = s.AnnotateCtx(forwardSQLIdentityThroughRPCCalls(ctx)) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -3731,7 +3731,7 @@ func (s *statusServer) TxnIDResolution( ctx context.Context, req *serverpb.TxnIDResolutionRequest, ) (*serverpb.TxnIDResolutionResponse, error) { ctx = s.AnnotateCtx(forwardSQLIdentityThroughRPCCalls(ctx)) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { return nil, err } diff --git a/pkg/sql/control_schedules.go b/pkg/sql/control_schedules.go index 24240f5aff56..85e3a53902c9 100644 --- a/pkg/sql/control_schedules.go +++ b/pkg/sql/control_schedules.go @@ -20,9 +20,11 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/isql" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/errors" ) @@ -129,14 +131,17 @@ func (n *controlSchedulesNode) startExec(params runParams) error { continue // not an error if schedule does not exist } - isAdmin, err := params.p.UserHasAdminRole(params.ctx, params.p.User()) + // Check that the user has privileges or is the owner of the schedules being altered. + hasPriv, err := params.p.HasPrivilege( + params.ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA, params.p.User(), + ) if err != nil { return err } isOwner := schedule.Owner() == params.p.User() - if !isAdmin && !isOwner { - return pgerror.Newf(pgcode.InsufficientPrivilege, "must be admin or owner of the "+ - "schedule %d to %s it", schedule.ScheduleID(), n.command.String()) + if !hasPriv && !isOwner { + return pgerror.Newf(pgcode.InsufficientPrivilege, "must have %s privilege or be owner of the "+ + "schedule %d to %s it", privilege.REPAIRCLUSTERMETADATA, schedule.ScheduleID(), n.command.String()) } switch n.command { diff --git a/pkg/sql/repair.go b/pkg/sql/repair.go index b37a68859773..7be289db1a0f 100644 --- a/pkg/sql/repair.go +++ b/pkg/sql/repair.go @@ -712,13 +712,9 @@ func checkPlannerStateForRepairFunctions(ctx context.Context, p *planner, method if p.extendedEvalCtx.TxnReadOnly { return readOnlyError(method) } - hasAdmin, err := p.UserHasAdminRole(ctx, p.User()) - if err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA); err != nil { return err } - if !hasAdmin { - return pgerror.Newf(pgcode.InsufficientPrivilege, "admin role required for %s", method) - } return nil } diff --git a/pkg/sql/show_create_external_connection.go b/pkg/sql/show_create_external_connection.go index ec75d1b70a4f..31ee3caf921f 100644 --- a/pkg/sql/show_create_external_connection.go +++ b/pkg/sql/show_create_external_connection.go @@ -18,6 +18,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" @@ -74,12 +75,13 @@ func (p *planner) ShowCreateExternalConnection( ) (planNode, error) { var hasPrivileges bool var err error - if hasPrivileges, err = p.UserHasAdminRole(ctx, p.User()); err != nil { + if hasPrivileges, err = p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA, p.User()); err != nil { return nil, err } - // If the user is not admin, and is running a `SHOW CREATE EXTERNAL CONNECTION foo` - // check if the user is the owner of the object. + // If the user does not have VIEWCLUSTERMETADATA, and is running a `SHOW + // CREATE EXTERNAL CONNECTION foo` check if the user is the owner of the + // object. exprEval := p.ExprEvaluator(externalConnectionOp) if !hasPrivileges && n.ConnectionLabel != nil { name, err := exprEval.String(ctx, n.ConnectionLabel) @@ -94,10 +96,10 @@ func (p *planner) ShowCreateExternalConnection( return nil, err } if !isOwner { - return nil, pgerror.Newf(pgcode.InsufficientPrivilege, "must be admin or owner of the External Connection %q", name) + return nil, pgerror.Newf(pgcode.InsufficientPrivilege, "must have %s privilege or be owner of the External Connection %q", privilege.VIEWCLUSTERMETADATA, name) } } else if !hasPrivileges { - return nil, pgerror.New(pgcode.InsufficientPrivilege, "must be admin to run `SHOW CREATE ALL EXTERNAL CONNECTIONS") + return nil, pgerror.Newf(pgcode.InsufficientPrivilege, "must have %s privilege to run `SHOW CREATE ALL EXTERNAL CONNECTIONS`", privilege.VIEWCLUSTERMETADATA) } sqltelemetry.IncrementShowCounter(sqltelemetry.CreateExternalConnection) diff --git a/pkg/sql/show_create_schedule.go b/pkg/sql/show_create_schedule.go index 55ddc7c4309d..8cf1eb46a6e2 100644 --- a/pkg/sql/show_create_schedule.go +++ b/pkg/sql/show_create_schedule.go @@ -18,11 +18,11 @@ import ( "github.com/cockroachdb/cockroach/pkg/jobs" "github.com/cockroachdb/cockroach/pkg/scheduledjobs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" - "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" - "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/sql/types" ) @@ -85,12 +85,9 @@ func loadSchedules(params runParams, n *tree.ShowCreateSchedules) ([]*jobs.Sched func (p *planner) ShowCreateSchedule( ctx context.Context, n *tree.ShowCreateSchedules, ) (planNode, error) { - // Only admin users can execute SHOW CREATE SCHEDULE - if userIsAdmin, err := p.UserHasAdminRole(ctx, p.User()); err != nil { + // Only privileged users can execute SHOW CREATE SCHEDULE + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil { return nil, err - } else if !userIsAdmin { - return nil, pgerror.Newf(pgcode.InsufficientPrivilege, - "user %s does not have admin role", p.User()) } sqltelemetry.IncrementShowCounter(sqltelemetry.CreateSchedule)