release-23.1: server: replace admin checks with VIEWCLUSTERMETADATA and REPAIRCLUSTERMETADATA

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(

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

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.

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.

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) {

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) {

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
 	}
 

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
 	}