Skip to content

Commit ad4e53f

Browse files
craig[bot]andyyang890
craig[bot]
and
andyyang890
committed
Merge #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 #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]>
2 parents 4ad7b7b + f43b3c1 commit ad4e53f

File tree

8 files changed

+89
-39
lines changed

8 files changed

+89
-39
lines changed

‎pkg/sql/alter_role.go

Lines changed: 11 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ import (
3232
"github.com/cockroachdb/cockroach/pkg/sql/sessioninit"
3333
"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
3434
"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
35-
"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
3635
"github.com/cockroachdb/cockroach/pkg/sql/types"
3736
"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
3837
)
@@ -182,7 +181,7 @@ func (n *alterRoleNode) startExec(params runParams) error {
182181
// CockroachDB does not support the superuser role option right now, but we
183182
// make it so any member of the ADMIN role can only be edited by another ADMIN
184183
// (done after checking for existence of the role).
185-
if err := params.p.CheckRoleOption(params.ctx, roleoption.CREATEROLE); err != nil {
184+
if err := params.p.CheckGlobalPrivilegeOrRoleOption(params.ctx, privilege.CREATEROLE); err != nil {
186185
return err
187186
}
188187
// Check that the requested combination of password options is
@@ -278,7 +277,7 @@ func (*alterRoleNode) Values() tree.Datums { return tree.Datums{} }
278277
func (*alterRoleNode) Close(context.Context) {}
279278

280279
// AlterRoleSet represents a `ALTER ROLE ... SET` statement.
281-
// Privileges: CREATEROLE privilege; or admin-only if `ALTER ROLE ALL`.
280+
// Privileges: CREATEROLE, MODIFYCLUSTERSETTING, MODIFYSQLCLUSTERSETTING privilege; or admin-only if `ALTER ROLE ALL`.
282281
func (p *planner) AlterRoleSet(ctx context.Context, n *tree.AlterRoleSet) (planNode, error) {
283282
// Note that for Postgres, only superuser can ALTER another superuser.
284283
// 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
292291
return nil, err
293292
}
294293
} else {
295-
hasModify := false
296-
hasSqlModify := false
297-
hasCreateRole := false
298-
// TODO(109258): Refactor this to use HasGlobalPrivilegeOrRoleOption.
299-
// Check system privileges.
300-
if ok, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYCLUSTERSETTING, p.User()); err != nil {
294+
canAlterRoleSet, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.CREATEROLE)
295+
if err != nil {
301296
return nil, err
302-
} else if ok {
303-
hasModify = true
304-
hasSqlModify = true
305-
}
306-
if !hasModify {
307-
if ok, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYSQLCLUSTERSETTING, p.User()); err != nil {
308-
return nil, err
309-
} else if ok {
310-
hasSqlModify = true
311-
}
312297
}
313-
// Check role options.
314-
if !hasSqlModify {
315-
if ok, err := p.HasRoleOption(ctx, roleoption.MODIFYCLUSTERSETTING); err != nil {
298+
if !canAlterRoleSet {
299+
canAlterRoleSet, err = p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYCLUSTERSETTING)
300+
if err != nil {
316301
return nil, err
317-
} else if ok {
318-
hasModify = true
319-
hasSqlModify = true
320302
}
321303
}
322-
if !hasModify && !hasSqlModify {
323-
if ok, err := p.HasRoleOption(ctx, roleoption.CREATEROLE); err != nil {
304+
if !canAlterRoleSet {
305+
canAlterRoleSet, err = p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.MODIFYSQLCLUSTERSETTING)
306+
if err != nil {
324307
return nil, err
325-
} else if ok {
326-
hasCreateRole = true
327308
}
328309
}
329-
if !hasModify && !hasSqlModify && !hasCreateRole {
310+
if !canAlterRoleSet {
330311
return nil, pgerror.Newf(pgcode.InsufficientPrivilege, "ALTER ROLE ... SET requires %s, %s or %s", privilege.MODIFYCLUSTERSETTING, privilege.MODIFYSQLCLUSTERSETTING, roleoption.CREATEROLE)
331312
}
332313
}

‎pkg/sql/create_role.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
2424
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
2525
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
26+
"github.com/cockroachdb/cockroach/pkg/sql/privilege"
2627
"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
2728
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
2829
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
@@ -62,7 +63,7 @@ func (p *planner) CreateRoleNode(
6263
opName string,
6364
kvOptions tree.KVOptions,
6465
) (*CreateRoleNode, error) {
65-
if err := p.CheckRoleOption(ctx, roleoption.CREATEROLE); err != nil {
66+
if err := p.CheckGlobalPrivilegeOrRoleOption(ctx, privilege.CREATEROLE); err != nil {
6667
return nil, err
6768
}
6869

‎pkg/sql/drop_role.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import (
2323
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
2424
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
2525
"github.com/cockroachdb/cockroach/pkg/sql/privilege"
26-
"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
2726
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
2827
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
2928
"github.com/cockroachdb/cockroach/pkg/sql/sessioninit"
@@ -53,7 +52,7 @@ func (p *planner) DropRole(ctx context.Context, n *tree.DropRole) (planNode, err
5352
func (p *planner) DropRoleNode(
5453
ctx context.Context, roleSpecs tree.RoleSpecList, ifExists bool, isRole bool, opName string,
5554
) (*DropRoleNode, error) {
56-
if err := p.CheckRoleOption(ctx, roleoption.CREATEROLE); err != nil {
55+
if err := p.CheckGlobalPrivilegeOrRoleOption(ctx, privilege.CREATEROLE); err != nil {
5756
return nil, err
5857
}
5958

‎pkg/sql/grant_role.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"github.com/cockroachdb/cockroach/pkg/sql/decodeusername"
2020
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
2121
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
22+
"github.com/cockroachdb/cockroach/pkg/sql/privilege"
2223
"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
2324
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
2425
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
@@ -54,10 +55,11 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR
5455
ctx, span := tracing.ChildSpan(ctx, n.StatementTag())
5556
defer span.Finish()
5657

57-
hasCreateRolePriv, err := p.HasRoleOption(ctx, roleoption.CREATEROLE)
58+
hasCreateRolePriv, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.CREATEROLE)
5859
if err != nil {
5960
return nil, err
6061
}
62+
6163
// Check permissions on each role.
6264
allRoles, err := p.MemberOfWithAdminOption(ctx, p.User())
6365
if err != nil {

‎pkg/sql/logictest/testdata/logic_test/role

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1664,3 +1664,64 @@ RESET CLUSTER SETTING server.user_login.password_encryption;
16641664

16651665
statement ok
16661666
RESET CLUSTER SETTING server.user_login.password_hashes.default_cost.scram_sha_256
1667+
1668+
subtest end
1669+
1670+
subtest create_role_priv
1671+
1672+
user root
1673+
1674+
# Revoke the CREATEROLE role option granted in an earlier subtest.
1675+
statement ok
1676+
ALTER ROLE testuser NOCREATEROLE
1677+
1678+
user testuser
1679+
1680+
statement error user testuser does not have CREATEROLE privilege
1681+
CREATE ROLE u_create_role_priv
1682+
1683+
user root
1684+
1685+
statement ok
1686+
GRANT SYSTEM CREATEROLE TO testuser
1687+
1688+
user testuser
1689+
1690+
statement ok
1691+
CREATE ROLE u_create_role_priv
1692+
1693+
user root
1694+
1695+
statement ok
1696+
REVOKE SYSTEM CREATEROLE FROM testuser
1697+
1698+
user testuser
1699+
1700+
statement error user testuser does not have CREATEROLE privilege
1701+
DROP ROLE u_create_role_priv
1702+
1703+
user root
1704+
1705+
statement ok
1706+
CREATE ROLE parent_with_createrole
1707+
1708+
statement ok
1709+
GRANT parent_with_createrole TO testuser
1710+
1711+
statement ok
1712+
GRANT SYSTEM CREATEROLE TO parent_with_createrole
1713+
1714+
user testuser
1715+
1716+
statement ok
1717+
DROP ROLE u_create_role_priv
1718+
1719+
user root
1720+
1721+
statement ok
1722+
REVOKE SYSTEM CREATEROLE FROM parent_with_createrole
1723+
1724+
statement ok
1725+
DROP ROLE parent_with_createrole
1726+
1727+
subtest end

‎pkg/sql/privilege/kind_string.go

Lines changed: 4 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/sql/privilege/privilege.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,8 @@ const (
7272
REPLICATION Kind = 29
7373
MANAGETENANT Kind = 30
7474
VIEWSYSTEMTABLE Kind = 31
75-
largestKind = VIEWSYSTEMTABLE
75+
CREATEROLE Kind = 32
76+
largestKind = CREATEROLE
7677
)
7778

7879
var isDeprecatedKind = map[Kind]bool{
@@ -160,7 +161,7 @@ var (
160161
GlobalPrivileges = List{
161162
ALL, BACKUP, RESTORE, MODIFYCLUSTERSETTING, EXTERNALCONNECTION, VIEWACTIVITY, VIEWACTIVITYREDACTED,
162163
VIEWCLUSTERSETTING, CANCELQUERY, NOSQLLOGIN, VIEWCLUSTERMETADATA, VIEWDEBUG, EXTERNALIOIMPLICITACCESS, VIEWJOB,
163-
MODIFYSQLCLUSTERSETTING, REPLICATION, MANAGETENANT, VIEWSYSTEMTABLE,
164+
MODIFYSQLCLUSTERSETTING, REPLICATION, MANAGETENANT, VIEWSYSTEMTABLE, CREATEROLE,
164165
}
165166
VirtualTablePrivileges = List{ALL, SELECT}
166167
ExternalConnectionPrivileges = List{ALL, USAGE, DROP}
@@ -208,6 +209,7 @@ var ByName = map[string]Kind{
208209
"REPLICATION": REPLICATION,
209210
"MANAGETENANT": MANAGETENANT,
210211
"VIEWSYSTEMTABLE": VIEWSYSTEMTABLE,
212+
"CREATEROLE": CREATEROLE,
211213
}
212214

213215
// List is a list of privileges.

‎pkg/sql/revoke_role.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import (
1717
"github.com/cockroachdb/cockroach/pkg/sql/decodeusername"
1818
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
1919
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
20-
"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
20+
"github.com/cockroachdb/cockroach/pkg/sql/privilege"
2121
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
2222
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
2323
"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
@@ -44,10 +44,11 @@ func (p *planner) RevokeRoleNode(ctx context.Context, n *tree.RevokeRole) (*Revo
4444
ctx, span := tracing.ChildSpan(ctx, n.StatementTag())
4545
defer span.Finish()
4646

47-
hasCreateRolePriv, err := p.HasRoleOption(ctx, roleoption.CREATEROLE)
47+
hasCreateRolePriv, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.CREATEROLE)
4848
if err != nil {
4949
return nil, err
5050
}
51+
5152
// check permissions on each role.
5253
allRoles, err := p.MemberOfWithAdminOption(ctx, p.User())
5354
if err != nil {

0 commit comments

Comments
 (0)