Merge cockroachdb#109258

109258: sql: add new CREATEROLE system privilege r=rafiss a=andyyang890 This patch adds a new `CREATEROLE` system privilege, which is analogous to the existing `CREATEROLE` role option, but can also be inherited by role membership. Informs cockroachdb#103237 Release note (sql change): There is now a `CREATEROLE` system privilege, which is analogous to the existing `CREATEROLE` role option, but can also be inherited by role membership. Co-authored-by: Andy Yang <[email protected]>
AlexTalks · Sep 6, 2023 · ad4e53f · ad4e53f
2 parents 4ad7b7b + f43b3c1
commit ad4e53f
Show file tree

Hide file tree

Showing 8 changed files with 89 additions and 39 deletions.
diff --git a/pkg/sql/alter_role.go b/pkg/sql/alter_role.go
@@ -32,7 +32,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sessioninit"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
 	"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/log/eventpb"
 )
@@ -182,7 +181,7 @@ func (n *alterRoleNode) startExec(params runParams) error {
 		// CockroachDB does not support the superuser role option right now, but we
 		// make it so any member of the ADMIN role can only be edited by another ADMIN
 		// (done after checking for existence of the role).
-		if err := params.p.CheckRoleOption(params.ctx, roleoption.CREATEROLE); err != nil {
+		if err := params.p.CheckGlobalPrivilegeOrRoleOption(params.ctx, privilege.CREATEROLE); err != nil {
 			return err
 		}
 		// Check that the requested combination of password options is
@@ -278,7 +277,7 @@ func (*alterRoleNode) Values() tree.Datums          { return tree.Datums{} }
 func (*alterRoleNode) Close(context.Context)        {}
 
 // AlterRoleSet represents a `ALTER ROLE ... SET` statement.
-// Privileges: CREATEROLE privilege; or admin-only if `ALTER ROLE ALL`.
+// Privileges: CREATEROLE, MODIFYCLUSTERSETTING, MODIFYSQLCLUSTERSETTING privilege; or admin-only if `ALTER ROLE ALL`.
 func (p *planner) AlterRoleSet(ctx context.Context, n *tree.AlterRoleSet) (planNode, error) {
 	// Note that for Postgres, only superuser can ALTER another superuser.
 	// CockroachDB does not support the superuser role option right now.
@@ -292,41 +291,23 @@ func (p *planner) AlterRoleSet(ctx context.Context, n *tree.AlterRoleSet) (planN
 			return nil, err
 		}
 	} else {
-		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 {
+		canAlterRoleSet, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.CREATEROLE)
+		if err != nil {
 			return nil, err
-		} else if ok {
-			hasModify = true
-			hasSqlModify = true
-		}
-		if !hasModify {
-			if ok, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYSQLCLUSTERSETTING, p.User()); err != nil {
-				return nil, err
-			} else if ok {
-				hasSqlModify = true
-			}
 		}
-		// Check role options.
-		if !hasSqlModify {
-			if ok, err := p.HasRoleOption(ctx, roleoption.MODIFYCLUSTERSETTING); err != nil {
+		if !canAlterRoleSet {
+			canAlterRoleSet, err = p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYCLUSTERSETTING)
+			if err != nil {
 				return nil, err
-			} else if ok {
-				hasModify = true
-				hasSqlModify = true
 			}
 		}
-		if !hasModify && !hasSqlModify {
-			if ok, err := p.HasRoleOption(ctx, roleoption.CREATEROLE); err != nil {
+		if !canAlterRoleSet {
+			canAlterRoleSet, err = p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYSQLCLUSTERSETTING)
+			if err != nil {
 				return nil, err
-			} else if ok {
-				hasCreateRole = true
 			}
 		}
-		if !hasModify && !hasSqlModify && !hasCreateRole {
+		if !canAlterRoleSet {
 			return nil, pgerror.Newf(pgcode.InsufficientPrivilege, "ALTER ROLE ... SET requires %s, %s or %s", privilege.MODIFYCLUSTERSETTING, privilege.MODIFYSQLCLUSTERSETTING, roleoption.CREATEROLE)
 		}
 	}

diff --git a/pkg/sql/create_role.go b/pkg/sql/create_role.go
@@ -23,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"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/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
@@ -62,7 +63,7 @@ func (p *planner) CreateRoleNode(
 	opName string,
 	kvOptions tree.KVOptions,
 ) (*CreateRoleNode, error) {
-	if err := p.CheckRoleOption(ctx, roleoption.CREATEROLE); err != nil {
+	if err := p.CheckGlobalPrivilegeOrRoleOption(ctx, privilege.CREATEROLE); err != nil {
 		return nil, err
 	}
 

diff --git a/pkg/sql/drop_role.go b/pkg/sql/drop_role.go
@@ -23,7 +23,6 @@ import (
 	"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/roleoption"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessioninit"
@@ -53,7 +52,7 @@ func (p *planner) DropRole(ctx context.Context, n *tree.DropRole) (planNode, err
 func (p *planner) DropRoleNode(
 	ctx context.Context, roleSpecs tree.RoleSpecList, ifExists bool, isRole bool, opName string,
 ) (*DropRoleNode, error) {
-	if err := p.CheckRoleOption(ctx, roleoption.CREATEROLE); err != nil {
+	if err := p.CheckGlobalPrivilegeOrRoleOption(ctx, privilege.CREATEROLE); err != nil {
 		return nil, err
 	}
 

diff --git a/pkg/sql/grant_role.go b/pkg/sql/grant_role.go
@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/decodeusername"
 	"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/roleoption"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
@@ -54,10 +55,11 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR
 	ctx, span := tracing.ChildSpan(ctx, n.StatementTag())
 	defer span.Finish()
 
-	hasCreateRolePriv, err := p.HasRoleOption(ctx, roleoption.CREATEROLE)
+	hasCreateRolePriv, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.CREATEROLE)
 	if err != nil {
 		return nil, err
 	}
+
 	// Check permissions on each role.
 	allRoles, err := p.MemberOfWithAdminOption(ctx, p.User())
 	if err != nil {

diff --git a/pkg/sql/logictest/testdata/logic_test/role b/pkg/sql/logictest/testdata/logic_test/role
@@ -1664,3 +1664,64 @@ RESET CLUSTER SETTING server.user_login.password_encryption;
 
 statement ok
 RESET CLUSTER SETTING server.user_login.password_hashes.default_cost.scram_sha_256
+
+subtest end
+
+subtest create_role_priv
+
+user root
+
+# Revoke the CREATEROLE role option granted in an earlier subtest.
+statement ok
+ALTER ROLE testuser NOCREATEROLE
+
+user testuser
+
+statement error user testuser does not have CREATEROLE privilege
+CREATE ROLE u_create_role_priv
+
+user root
+
+statement ok
+GRANT SYSTEM CREATEROLE TO testuser
+
+user testuser
+
+statement ok
+CREATE ROLE u_create_role_priv
+
+user root
+
+statement ok
+REVOKE SYSTEM CREATEROLE FROM testuser
+
+user testuser
+
+statement error user testuser does not have CREATEROLE privilege
+DROP ROLE u_create_role_priv
+
+user root
+
+statement ok
+CREATE ROLE parent_with_createrole
+
+statement ok
+GRANT parent_with_createrole TO testuser
+
+statement ok
+GRANT SYSTEM CREATEROLE TO parent_with_createrole
+
+user testuser
+
+statement ok
+DROP ROLE u_create_role_priv
+
+user root
+
+statement ok
+REVOKE SYSTEM CREATEROLE FROM parent_with_createrole
+
+statement ok
+DROP ROLE parent_with_createrole
+
+subtest end
diff --git a/pkg/sql/privilege/kind_string.go b/pkg/sql/privilege/kind_string.go
diff --git a/pkg/sql/privilege/privilege.go b/pkg/sql/privilege/privilege.go
@@ -72,7 +72,8 @@ const (
 	REPLICATION              Kind = 29
 	MANAGETENANT             Kind = 30
 	VIEWSYSTEMTABLE          Kind = 31
-	largestKind                   = VIEWSYSTEMTABLE
+	CREATEROLE               Kind = 32
+	largestKind                   = CREATEROLE
 )
 
 var isDeprecatedKind = map[Kind]bool{
@@ -160,7 +161,7 @@ var (
 	GlobalPrivileges   = List{
 		ALL, BACKUP, RESTORE, MODIFYCLUSTERSETTING, EXTERNALCONNECTION, VIEWACTIVITY, VIEWACTIVITYREDACTED,
 		VIEWCLUSTERSETTING, CANCELQUERY, NOSQLLOGIN, VIEWCLUSTERMETADATA, VIEWDEBUG, EXTERNALIOIMPLICITACCESS, VIEWJOB,
-		MODIFYSQLCLUSTERSETTING, REPLICATION, MANAGETENANT, VIEWSYSTEMTABLE,
+		MODIFYSQLCLUSTERSETTING, REPLICATION, MANAGETENANT, VIEWSYSTEMTABLE, CREATEROLE,
 	}
 	VirtualTablePrivileges       = List{ALL, SELECT}
 	ExternalConnectionPrivileges = List{ALL, USAGE, DROP}
@@ -208,6 +209,7 @@ var ByName = map[string]Kind{
 	"REPLICATION":              REPLICATION,
 	"MANAGETENANT":             MANAGETENANT,
 	"VIEWSYSTEMTABLE":          VIEWSYSTEMTABLE,
+	"CREATEROLE":               CREATEROLE,
 }
 
 // List is a list of privileges.

diff --git a/pkg/sql/revoke_role.go b/pkg/sql/revoke_role.go
@@ -17,7 +17,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/decodeusername"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
-	"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
+	"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/sqlerrors"
@@ -44,10 +44,11 @@ func (p *planner) RevokeRoleNode(ctx context.Context, n *tree.RevokeRole) (*Revo
 	ctx, span := tracing.ChildSpan(ctx, n.StatementTag())
 	defer span.Finish()
 
-	hasCreateRolePriv, err := p.HasRoleOption(ctx, roleoption.CREATEROLE)
+	hasCreateRolePriv, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.CREATEROLE)
 	if err != nil {
 		return nil, err
 	}
+
 	// check permissions on each role.
 	allRoles, err := p.MemberOfWithAdminOption(ctx, p.User())
 	if err != nil {