sql: refactor global privilege and legacy role option checking

This patch abstracts the logic for checking whether a user has a
global (system) privilege or the corresponding legacy role option
into a new function (`HasGlobalPrivilegeOrRoleOption`).

It also adds a wrapper function that returns an insufficient
privileges error when the user has neither the privilege nor
the legacy role option (`CheckGlobalPrivilegeOrRoleOption`).

Release note: None

pkg/sql/alter_role.go

@@ -295,6 +295,7 @@ func (p *planner) AlterRoleSet(ctx context.Context, n *tree.AlterRoleSet) (planN
 		hasModify := false
 		hasSqlModify := false
 		hasCreateRole := false
+		// TODO(109258): Refactor this to use HasGlobalPrivilegeOrRoleOption.
 		// Check system privileges.
 		if ok, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYCLUSTERSETTING, p.User()); err != nil {
 			return nil, err

pkg/sql/alter_table.go

@@ -36,7 +36,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
-	"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/semenumpb"
@@ -47,7 +46,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/stats"
 	"github.com/cockroachdb/cockroach/pkg/sql/storageparam"
 	"github.com/cockroachdb/cockroach/pkg/sql/storageparam/tablestorageparam"
-	"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
 	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
@@ -895,19 +893,13 @@ func (p *planner) setAuditMode(
 	}
 	if !hasAdmin {
 		// Check for system privilege first, otherwise fall back to role options.
-		hasModify, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYCLUSTERSETTING, p.User())
+		hasModify, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYCLUSTERSETTING)
 		if err != nil {
 			return false, err
 		}
 		if !hasModify {
-			hasModify, err = p.HasRoleOption(ctx, roleoption.MODIFYCLUSTERSETTING)
-			if err != nil {
-				return false, err
-			}
-			if !hasModify {
-				return false, pgerror.Newf(pgcode.InsufficientPrivilege,
-					"only users with admin or %s system privilege are allowed to change audit settings on a table ", privilege.MODIFYCLUSTERSETTING.String())
-			}
+			return false, pgerror.Newf(pgcode.InsufficientPrivilege,
+				"only users with admin or %s system privilege are allowed to change audit settings on a table ", privilege.MODIFYCLUSTERSETTING.String())
 		}
 	}
 

pkg/sql/authorization.go

@@ -124,6 +124,14 @@ type AuthorizationAccessor interface {
 	// the role options table. Example: CREATEROLE instead of NOCREATEROLE.
 	// NOLOGIN instead of LOGIN.
 	HasRoleOption(ctx context.Context, roleOption roleoption.Option) (bool, error)
+
+	// HasGlobalPrivilegeOrRoleOption returns a bool representing whether the current user
+	// has a global privilege or the corresponding legacy role option.
+	HasGlobalPrivilegeOrRoleOption(ctx context.Context, privilege privilege.Kind) (bool, error)
+
+	// CheckGlobalPrivilegeOrRoleOption checks if the current user has a global privilege
+	// or the corresponding legacy role option, and returns an error if the user does not.
+	CheckGlobalPrivilegeOrRoleOption(ctx context.Context, privilege privilege.Kind) error
 }
 
 var _ AuthorizationAccessor = &planner{}
@@ -804,6 +812,59 @@ func (p *planner) CheckRoleOption(ctx context.Context, roleOption roleoption.Opt
 	return nil
 }
 
+func privilegeToLegacyRoleOption(priv privilege.Kind) roleoption.Option {
+	switch priv {
+	case privilege.CANCELQUERY:
+		return roleoption.CANCELQUERY
+	case privilege.CHANGEFEED:
+		return roleoption.CONTROLCHANGEFEED
+	case privilege.MODIFYCLUSTERSETTING:
+		return roleoption.MODIFYCLUSTERSETTING
+	case privilege.REPLICATION:
+		return roleoption.REPLICATION
+	case privilege.VIEWACTIVITY:
+		return roleoption.VIEWACTIVITY
+	case privilege.VIEWACTIVITYREDACTED:
+		return roleoption.VIEWACTIVITYREDACTED
+	case privilege.VIEWCLUSTERSETTING:
+		return roleoption.VIEWCLUSTERSETTING
+	default:
+		return roleoption.UNKNOWN
+	}
+}
+
+// HasGlobalPrivilegeOrRoleOption implements the AuthorizationAccessor interface.
+func (p *planner) HasGlobalPrivilegeOrRoleOption(
+	ctx context.Context, privilege privilege.Kind,
+) (bool, error) {
+	ok, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege, p.User())
+	if err != nil {
+		return false, err
+	}
+	if ok {
+		return true, nil
+	}
+	if roleOption := privilegeToLegacyRoleOption(privilege); roleOption != roleoption.UNKNOWN {
+		return p.HasRoleOption(ctx, roleOption)
+	}
+	return false, nil
+}
+
+// CheckGlobalPrivilegeOrRoleOption implements the AuthorizationAccessor interface.
+func (p *planner) CheckGlobalPrivilegeOrRoleOption(
+	ctx context.Context, privilege privilege.Kind,
+) error {
+	ok, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege)
+	if err != nil {
+		return err
+	}
+	if !ok {
+		return pgerror.Newf(pgcode.InsufficientPrivilege,
+			"user %s does not have %s privilege", p.User(), privilege)
+	}
+	return nil
+}
+
 // ConnAuditingClusterSettingName is the name of the cluster setting
 // for the cluster setting that enables pgwire-level connection audit
 // logs.
@@ -997,31 +1058,11 @@ func (p *planner) HasViewActivityOrViewActivityRedactedRole(ctx context.Context)
 }
 
 func (p *planner) HasViewActivityRedacted(ctx context.Context) (bool, error) {
-	if hasViewRedacted, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWACTIVITYREDACTED, p.User()); err != nil {
-		return false, err
-	} else if hasViewRedacted {
-		return true, nil
-	}
-	if hasViewRedacted, err := p.HasRoleOption(ctx, roleoption.VIEWACTIVITYREDACTED); err != nil {
-		return false, err
-	} else if hasViewRedacted {
-		return true, nil
-	}
-	return false, nil
+	return p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.VIEWACTIVITYREDACTED)
 }
 
 func (p *planner) HasViewActivity(ctx context.Context) (bool, error) {
-	if hasView, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWACTIVITY, p.User()); err != nil {
-		return false, err
-	} else if hasView {
-		return true, nil
-	}
-	if hasView, err := p.HasRoleOption(ctx, roleoption.VIEWACTIVITY); err != nil {
-		return false, err
-	} else if hasView {
-		return true, nil
-	}
-	return false, nil
+	return p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.VIEWACTIVITY)
 }
 
 func insufficientPrivilegeError(

pkg/sql/crdb_internal.go

@@ -86,7 +86,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/sslocal"
-	"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/sql/vtable"
 	"github.com/cockroachdb/cockroach/pkg/util/admission/admissionpb"
@@ -2342,28 +2341,18 @@ CREATE TABLE crdb_internal.cluster_settings (
   key           STRING NOT NULL
 )`,
 	populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error {
-		hasSqlModify, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYSQLCLUSTERSETTING, p.User())
+		hasModify, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYCLUSTERSETTING)
 		if err != nil {
 			return err
 		}
-		hasModify, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYCLUSTERSETTING, p.User())
+		hasSqlModify, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYSQLCLUSTERSETTING)
 		if err != nil {
 			return err
 		}
-		hasView, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERSETTING, p.User())
+		hasView, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.VIEWCLUSTERSETTING)
 		if err != nil {
 			return err
 		}
-		if !hasModify {
-			if hasModify, err = p.HasRoleOption(ctx, roleoption.MODIFYCLUSTERSETTING); err != nil {
-				return err
-			}
-		}
-		if !hasView {
-			if hasView, err = p.HasRoleOption(ctx, roleoption.VIEWCLUSTERSETTING); err != nil {
-				return err
-			}
-		}
 		if !hasModify && !hasSqlModify && !hasView {
 			return pgerror.Newf(pgcode.InsufficientPrivilege,
 				"only users with %s, %s or %s system privileges are allowed to read "+

pkg/sql/roleoption/role_option.go

@@ -38,7 +38,7 @@ type RoleOption struct {
 // using a global privilege instead. Global privileges are inherited from role
 // to user, but role options are not.
 const (
-	_ Option = iota
+	UNKNOWN Option = iota
 	CREATEROLE
 	NOCREATEROLE
 	PASSWORD

pkg/sql/set_cluster_setting.go

@@ -37,14 +37,12 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
-	"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessioninit"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
-	"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -69,47 +67,17 @@ type setClusterSettingNode struct {
 func checkPrivilegesForSetting(
 	ctx context.Context, p *planner, name settings.SettingName, action string,
 ) error {
-	// First check system privileges.
-	hasModify := false
-	hasSqlModify := false
-	hasView := false
-	if ok, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYCLUSTERSETTING, p.User()); err != nil {
+	hasModify, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYCLUSTERSETTING)
+	if err != nil {
 		return err
-	} else if ok {
-		hasModify = true
-		hasSqlModify = true
-		hasView = true
 	}
-	if !hasSqlModify {
-		if ok, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYSQLCLUSTERSETTING, p.User()); err != nil {
-			return err
-		} else if ok {
-			hasSqlModify = true
-		}
-	}
-	if !hasView {
-		if ok, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERSETTING, p.User()); err != nil {
-			return err
-		} else if ok {
-			hasView = true
-		}
-	}
-
-	// Fallback to role option if the user doesn't have the privilege.
-	if !hasModify {
-		ok, err := p.HasRoleOption(ctx, roleoption.MODIFYCLUSTERSETTING)
-		if err != nil {
-			return err
-		}
-		hasModify = hasModify || ok
-		hasView = hasView || ok
+	hasSqlModify, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYSQLCLUSTERSETTING)
+	if err != nil {
+		return err
 	}
-	if !hasView {
-		ok, err := p.HasRoleOption(ctx, roleoption.VIEWCLUSTERSETTING)
-		if err != nil {
-			return err
-		}
-		hasView = hasView || ok
+	hasView, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.VIEWCLUSTERSETTING)
+	if err != nil {
+		return err
 	}
 
 	isSqlSetting := strings.HasPrefix(string(name), "sql.defaults")
@@ -118,12 +86,12 @@ func checkPrivilegesForSetting(
 	if hasModify {
 		return nil
 	}
-	// If the user has sql modify they can do either action as long as its a sql.defaults
+	// If the user has sql modify, they can do either action as long as it's a sql.defaults
 	// setting.
 	if hasSqlModify && isSqlSetting {
 		return nil
 	}
-	// From this point, the user does not have modify or has sql modify but it is not a
+	// From this point, the user does not have modify or sql modify but it is not a
 	// sql.defaults setting so we can expect an error if the user wants to edit.
 	if action == "set" {
 		if !isSqlSetting {