diff --git a/pkg/sql/alter_role.go b/pkg/sql/alter_role.go index 68265afa9942..9a8445082326 100644 --- 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 index 1f4dfe6d4190..46d8e85288bb 100644 --- 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 index 9052aab021bb..8507c7add2a2 100644 --- 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 index f76d1965f441..a8f66048545c 100644 --- 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 index 96c8971d69d6..b7f061231f79 100644 --- 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 index 993aa094edf2..7257b3710c36 100644 --- a/pkg/sql/privilege/kind_string.go +++ b/pkg/sql/privilege/kind_string.go @@ -39,7 +39,8 @@ func _() { _ = x[REPLICATION-29] _ = x[MANAGETENANT-30] _ = x[VIEWSYSTEMTABLE-31] - _ = x[largestKind-31] + _ = x[CREATEROLE-32] + _ = x[largestKind-32] } func (i Kind) String() string { @@ -106,6 +107,8 @@ func (i Kind) String() string { return "MANAGETENANT" case VIEWSYSTEMTABLE: return "VIEWSYSTEMTABLE" + case CREATEROLE: + return "CREATEROLE" default: return "Kind(" + strconv.FormatInt(int64(i), 10) + ")" } diff --git a/pkg/sql/privilege/privilege.go b/pkg/sql/privilege/privilege.go index 97e7b97b9904..677420cf6ffa 100644 --- 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 index 47ac48c080d2..c618c5300131 100644 --- 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 {