Merge #68830

68830: sql: fix sql input to security.SQLUsername issue r=RichardJCai a=e-mbrown Resolves: #67818 The statements `drop owned by` and `reassigned owned by` were case-sensitive because they relied on `role_spec_list` which wasn't correctly generating `security.SQLusername`. With this change the two statement use `NameLists` that are then converted into `security.SQLusernames`. `role_spec_list` has been removed from the parser because it is no longer being used. Release note: None Co-authored-by: e-mbrown <[email protected]>
cockroachdb · Aug 18, 2021 · 61fdb08 · 61fdb08
2 parents b16c86d + c3f09f1
commit 61fdb08
Show file tree

Hide file tree

Showing 11 changed files with 35 additions and 72 deletions.
diff --git 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
@@ -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
@@ -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
@@ -22,7 +22,7 @@ import (
 // dropOwnedByNode represents a DROP OWNED BY <role(s)> 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
@@ -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
@@ -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 <security.SQLUsername> role_spec
-%type <[]security.SQLUsername> role_spec_list
 %type <tree.Expr> zone_value
 %type <tree.Expr> string_or_placeholder
 %type <tree.Expr> 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 {<name> | 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
@@ -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
@@ -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 <role(s)> TO <role> 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
@@ -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
@@ -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
@@ -14,9 +14,7 @@ import "github.com/cockroachdb/cockroach/pkg/security"
 
 // ReassignOwnedBy represents a REASSIGN OWNED BY <name> TO <name> 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)