diff --git a/docs/generated/sql/bnf/drop_owned_by_stmt.bnf b/docs/generated/sql/bnf/drop_owned_by_stmt.bnf index 5207ec50dbe4..9ace203cbdcc 100644 --- a/docs/generated/sql/bnf/drop_owned_by_stmt.bnf +++ b/docs/generated/sql/bnf/drop_owned_by_stmt.bnf @@ -1,2 +1,2 @@ drop_owned_by_stmt ::= - 'DROP' 'OWNED' 'BY' role_spec_list opt_drop_behavior + 'DROP' 'OWNED' 'BY' name_list opt_drop_behavior diff --git a/docs/generated/sql/bnf/reassign_owned_by_stmt.bnf b/docs/generated/sql/bnf/reassign_owned_by_stmt.bnf index ead3d8cfdecc..9eb4edc91f3d 100644 --- a/docs/generated/sql/bnf/reassign_owned_by_stmt.bnf +++ b/docs/generated/sql/bnf/reassign_owned_by_stmt.bnf @@ -1,2 +1,2 @@ reassign_owned_by_stmt ::= - 'REASSIGN' 'OWNED' 'BY' role_spec_list 'TO' role_spec + 'REASSIGN' 'OWNED' 'BY' name_list 'TO' role_spec diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf index 0b381ba17862..16c6579f9403 100644 --- a/docs/generated/sql/bnf/stmt_block.bnf +++ b/docs/generated/sql/bnf/stmt_block.bnf @@ -95,10 +95,10 @@ savepoint_stmt ::= 'SAVEPOINT' name reassign_owned_by_stmt ::= - 'REASSIGN' 'OWNED' 'BY' role_spec_list 'TO' role_spec + 'REASSIGN' 'OWNED' 'BY' name_list 'TO' role_spec drop_owned_by_stmt ::= - 'DROP' 'OWNED' 'BY' role_spec_list opt_drop_behavior + 'DROP' 'OWNED' 'BY' name_list opt_drop_behavior release_stmt ::= 'RELEASE' savepoint_name @@ -326,9 +326,6 @@ prep_type_clause ::= '(' type_list ')' | -role_spec_list ::= - ( role_spec ) ( ( ',' role_spec ) )* - role_spec ::= username_or_sconst | 'CURRENT_USER' diff --git a/pkg/sql/drop_owned_by.go b/pkg/sql/drop_owned_by.go index 54b5734d3598..bc5977501114 100644 --- a/pkg/sql/drop_owned_by.go +++ b/pkg/sql/drop_owned_by.go @@ -22,7 +22,7 @@ import ( // dropOwnedByNode represents a DROP OWNED BY statement. type dropOwnedByNode struct { // TODO(angelaw): Uncomment when implementing - commenting out due to linting error. - // n *tree.DropOwnedBy + //n *tree.DropOwnedBy } func (p *planner) DropOwnedBy(ctx context.Context) (planNode, error) { diff --git a/pkg/sql/grant_revoke.go b/pkg/sql/grant_revoke.go index 481eac905ea3..4ac04e397705 100644 --- a/pkg/sql/grant_revoke.go +++ b/pkg/sql/grant_revoke.go @@ -47,15 +47,10 @@ func (p *planner) Grant(ctx context.Context, n *tree.Grant) (planNode, error) { return nil, err } - grantees := make([]security.SQLUsername, len(n.Grantees)) - for i, grantee := range n.Grantees { - normalizedGrantee, err := security.MakeSQLUsernameFromUserInput(string(grantee), security.UsernameValidation) - if err != nil { - return nil, err - } - grantees[i] = normalizedGrantee + grantees, err := n.Grantees.ToSQLUsernames() + if err != nil { + return nil, err } - return &changePrivilegesNode{ isGrant: true, targets: n.Targets, diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y index 29f30af0659e..0b38139d36f2 100644 --- a/pkg/sql/parser/sql.y +++ b/pkg/sql/parser/sql.y @@ -1321,7 +1321,6 @@ func (u *sqlSymUnion) setVar() *tree.SetVar { // TODO(solon): The type for role_spec needs to be updated to fix // https://github.com/cockroachdb/cockroach/issues/54696 %type role_spec -%type <[]security.SQLUsername> role_spec_list %type zone_value %type string_or_placeholder %type string_or_placeholder_list @@ -2466,16 +2465,6 @@ opt_add_val_placement: $$.val = (*tree.AlterTypeAddValuePlacement)(nil) } -role_spec_list: - role_spec - { - $$.val = []security.SQLUsername{$1.user()} - } -| role_spec_list ',' role_spec - { - $$.val = append($1.users(), $3.user()) - } - role_spec: username_or_sconst { $$.val = $1.user() } | CURRENT_USER @@ -8900,10 +8889,10 @@ multiple_set_clause: // TO { | CURRENT_USER | SESSION_USER} // %SeeAlso: DROP OWNED BY reassign_owned_by_stmt: - REASSIGN OWNED BY role_spec_list TO role_spec + REASSIGN OWNED BY name_list TO role_spec { $$.val = &tree.ReassignOwnedBy{ - OldRoles: $4.users(), + OldRoles: $4.nameList(), NewRole: $6.user(), } } @@ -8915,10 +8904,10 @@ reassign_owned_by_stmt: // [RESTRICT | CASCADE] // %SeeAlso: REASSIGN OWNED BY drop_owned_by_stmt: - DROP OWNED BY role_spec_list opt_drop_behavior + DROP OWNED BY name_list opt_drop_behavior { $$.val = &tree.DropOwnedBy{ - Roles: $4.users(), + Roles: $4.nameList(), DropBehavior: $5.dropBehavior(), } } diff --git a/pkg/sql/parser/testdata/reassign b/pkg/sql/parser/testdata/reassign index 69c4e3fd4784..0ce588fd30f5 100644 --- a/pkg/sql/parser/testdata/reassign +++ b/pkg/sql/parser/testdata/reassign @@ -16,22 +16,13 @@ REASSIGN OWNED BY foo, bar TO third -- literals removed REASSIGN OWNED BY _, _ TO _ -- identifiers removed parse -REASSIGN OWNED BY CURRENT_USER TO foo +REASSIGN OWNED BY fOoOOOoo TO bar ---- -REASSIGN OWNED BY "current_user" TO foo -- normalized! -REASSIGN OWNED BY "current_user" TO foo -- fully parenthesized -REASSIGN OWNED BY "current_user" TO foo -- literals removed +REASSIGN OWNED BY fooooooo TO bar -- normalized! +REASSIGN OWNED BY fooooooo TO bar -- fully parenthesized +REASSIGN OWNED BY fooooooo TO bar -- literals removed REASSIGN OWNED BY _ TO _ -- identifiers removed -parse -REASSIGN OWNED BY SESSION_USER TO foo ----- -REASSIGN OWNED BY "session_user" TO foo -- normalized! -REASSIGN OWNED BY "session_user" TO foo -- fully parenthesized -REASSIGN OWNED BY "session_user" TO foo -- literals removed -REASSIGN OWNED BY _ TO _ -- identifiers removed - - parse DROP OWNED BY foo ---- @@ -63,19 +54,3 @@ DROP OWNED BY foo RESTRICT DROP OWNED BY foo RESTRICT -- fully parenthesized DROP OWNED BY foo RESTRICT -- literals removed DROP OWNED BY _ RESTRICT -- identifiers removed - -parse -DROP OWNED BY CURRENT_USER ----- -DROP OWNED BY "current_user" -- normalized! -DROP OWNED BY "current_user" -- fully parenthesized -DROP OWNED BY "current_user" -- literals removed -DROP OWNED BY _ -- identifiers removed - -parse -DROP OWNED BY SESSION_USER ----- -DROP OWNED BY "session_user" -- normalized! -DROP OWNED BY "session_user" -- fully parenthesized -DROP OWNED BY "session_user" -- literals removed -DROP OWNED BY _ -- identifiers removed diff --git a/pkg/sql/reassign_owned_by.go b/pkg/sql/reassign_owned_by.go index e758571576f5..1e69f186bfb2 100644 --- a/pkg/sql/reassign_owned_by.go +++ b/pkg/sql/reassign_owned_by.go @@ -13,6 +13,7 @@ package sql import ( "context" + "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" @@ -28,7 +29,8 @@ import ( // ReassignOwnedByNode represents a REASSIGN OWNED BY TO statement. type reassignOwnedByNode struct { - n *tree.ReassignOwnedBy + n *tree.ReassignOwnedBy + normalizedOldRoles []security.SQLUsername } func (p *planner) ReassignOwnedBy(ctx context.Context, n *tree.ReassignOwnedBy) (planNode, error) { @@ -40,9 +42,13 @@ func (p *planner) ReassignOwnedBy(ctx context.Context, n *tree.ReassignOwnedBy) return nil, err } + normalizedOldRole, err := n.OldRoles.ToSQLUsernames() + if err != nil { + return nil, err + } // Check all roles in old roles exist. Checks in authorization.go will confirm that current user // is a member of old roles and new roles and has CREATE privilege. - for _, oldRole := range n.OldRoles { + for _, oldRole := range normalizedOldRole { roleExists, err := RoleExists(ctx, p.ExecCfg(), p.Txn(), oldRole) if err != nil { return nil, err @@ -51,7 +57,7 @@ func (p *planner) ReassignOwnedBy(ctx context.Context, n *tree.ReassignOwnedBy) return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %q does not exist", oldRole) } } - return &reassignOwnedByNode{n: n}, nil + return &reassignOwnedByNode{n: n, normalizedOldRoles: normalizedOldRole}, nil } func (n *reassignOwnedByNode) startExec(params runParams) error { @@ -74,7 +80,7 @@ func (n *reassignOwnedByNode) startExec(params runParams) error { currentDbDesc.ImmutableCopy().(catalog.DatabaseDescriptor), nil /* fallback */) // Iterate through each object, check for ownership by an old role. - for _, oldRole := range n.n.OldRoles { + for _, oldRole := range n.normalizedOldRoles { // There should only be one database (current). for _, dbID := range lCtx.dbIDs { if IsOwner(lCtx.dbDescs[dbID], oldRole) { diff --git a/pkg/sql/sem/tree/drop_owned_by.go b/pkg/sql/sem/tree/drop_owned_by.go index 8a5e99ed1d77..4fdc379918d2 100644 --- a/pkg/sql/sem/tree/drop_owned_by.go +++ b/pkg/sql/sem/tree/drop_owned_by.go @@ -10,11 +10,9 @@ package tree -import "github.com/cockroachdb/cockroach/pkg/security" - // DropOwnedBy represents a DROP OWNED BY command. type DropOwnedBy struct { - Roles []security.SQLUsername + Roles NameList DropBehavior DropBehavior } @@ -27,7 +25,7 @@ func (node *DropOwnedBy) Format(ctx *FmtCtx) { if i > 0 { ctx.WriteString(", ") } - ctx.FormatUsername(node.Roles[i]) + ctx.FormatUsernameN(node.Roles[i]) } if node.DropBehavior != DropDefault { ctx.WriteString(" ") diff --git a/pkg/sql/sem/tree/format.go b/pkg/sql/sem/tree/format.go index db447c8e7b12..fe48a96ceb1b 100644 --- a/pkg/sql/sem/tree/format.go +++ b/pkg/sql/sem/tree/format.go @@ -385,6 +385,11 @@ func (ctx *FmtCtx) FormatUsername(s security.SQLUsername) { ctx.FormatName(s.Normalized()) } +//FormatUsernameN formats a username that is type Name +func (ctx *FmtCtx) FormatUsernameN(s Name) { + ctx.FormatName(s.Normalize()) +} + // FormatNode recurses into a node for pretty-printing. // Flag-driven special cases can hook into this. func (ctx *FmtCtx) FormatNode(n NodeFormatter) { diff --git a/pkg/sql/sem/tree/reassign_owned_by.go b/pkg/sql/sem/tree/reassign_owned_by.go index 2626e04fc0a5..39225ce2d42e 100644 --- a/pkg/sql/sem/tree/reassign_owned_by.go +++ b/pkg/sql/sem/tree/reassign_owned_by.go @@ -14,9 +14,7 @@ import "github.com/cockroachdb/cockroach/pkg/security" // ReassignOwnedBy represents a REASSIGN OWNED BY TO statement. type ReassignOwnedBy struct { - // TODO(solon): Adjust this, see - // https://github.com/cockroachdb/cockroach/issues/54696 - OldRoles []security.SQLUsername + OldRoles NameList NewRole security.SQLUsername } @@ -29,7 +27,7 @@ func (node *ReassignOwnedBy) Format(ctx *FmtCtx) { if i > 0 { ctx.WriteString(", ") } - ctx.FormatUsername(node.OldRoles[i]) + ctx.FormatUsernameN(node.OldRoles[i]) } ctx.WriteString(" TO ") ctx.FormatUsername(node.NewRole)