Merge #67625 #67821

67625: sql: GRANT/REVOKE treat names case insensitively r=RichardJCai a=rafiss fixes #65556 Release note (bug fix): Previously the GRANT and REVOKE commands would incorrectly handle role names. CockroachDB treats role names as case insensitive, but these commands were incorrectly handling the names. Now, GRANT and REVOKE normalize the names and are case-insensitive. 67821: sql: removed pg_stat exclusion for pg_catalog r=rafiss a=mnovelodou Previously, we excluded pg_stat* tables from difftool This was inadequate because we were unable to add missing empty tables To address this, this patch removes the exclusion Release note: None Co-authored-by: Rafi Shamim <[email protected]> Co-authored-by: MiguelNovelo <[email protected]>
cockroachdb · Jul 22, 2021 · d81b01c · d81b01c
3 parents 375a77c + 78f867f + 156e514
commit d81b01c
Show file tree

Hide file tree

Showing 8 changed files with 2,951 additions and 154 deletions.
diff --git a/pkg/cmd/generate-metadata-tables/rdbms/postgres.go b/pkg/cmd/generate-metadata-tables/rdbms/postgres.go
@@ -44,16 +44,7 @@ var unimplementedEquivalencies = map[oid.Oid]oid.Oid{
 	oid.T_pg_lsn:       oid.T_text,
 }
 
-var emptyStruct = struct{}{}
-
 var postgresExclusions = []*excludePattern{
-	{
-		pattern: regexp.MustCompile(`^pg_stat.+$`),
-		except: map[string]struct{}{
-			"pg_stat_database":           emptyStruct,
-			"pg_stat_database_conflicts": emptyStruct,
-		},
-	},
 	{
 		pattern: regexp.MustCompile(`^_pg_.+$`),
 		except:  make(map[string]struct{}),

diff --git a/pkg/sql/grant_revoke.go b/pkg/sql/grant_revoke.go
@@ -47,13 +47,13 @@ func (p *planner) Grant(ctx context.Context, n *tree.Grant) (planNode, error) {
 		return nil, err
 	}
 
-	// TODO(solon): there are SQL identifiers (tree.Name) in n.Grantees,
-	// but we want SQL usernames. Do we normalize or not? For reference,
-	// REASSIGN / OWNER TO do normalize.
-	// Related: https://github.com/cockroachdb/cockroach/issues/54696
 	grantees := make([]security.SQLUsername, len(n.Grantees))
 	for i, grantee := range n.Grantees {
-		grantees[i] = security.MakeSQLUsernameFromPreNormalizedString(string(grantee))
+		normalizedGrantee, err := security.MakeSQLUsernameFromUserInput(string(grantee), security.UsernameValidation)
+		if err != nil {
+			return nil, err
+		}
+		grantees[i] = normalizedGrantee
 	}
 
 	return &changePrivilegesNode{
@@ -73,7 +73,6 @@ func (p *planner) Grant(ctx context.Context, n *tree.Grant) (planNode, error) {
 // - Target: single database, table, or view.
 // TODO(marc): open questions:
 // - should we have root always allowed and not present in the permissions list?
-// - should we make users case-insensitive?
 // Privileges: GRANT on database/table/view.
 //   Notes: postgres requires the object owner.
 //          mysql requires the "grant option" and the same privileges, and sometimes superuser.
@@ -84,13 +83,13 @@ func (p *planner) Revoke(ctx context.Context, n *tree.Revoke) (planNode, error)
 		return nil, err
 	}
 
-	// TODO(solon): there are SQL identifiers (tree.Name) in n.Grantees,
-	// but we want SQL usernames. Do we normalize or not? For reference,
-	// REASSIGN / OWNER TO do normalize.
-	// Related: https://github.com/cockroachdb/cockroach/issues/54696
 	grantees := make([]security.SQLUsername, len(n.Grantees))
 	for i, grantee := range n.Grantees {
-		grantees[i] = security.MakeSQLUsernameFromPreNormalizedString(string(grantee))
+		normalizedGrantee, err := security.MakeSQLUsernameFromUserInput(string(grantee), security.UsernameValidation)
+		if err != nil {
+			return nil, err
+		}
+		grantees[i] = normalizedGrantee
 	}
 
 	return &changePrivilegesNode{

diff --git a/pkg/sql/grant_role.go b/pkg/sql/grant_role.go
@@ -28,8 +28,8 @@ import (
 // GrantRoleNode creates entries in the system.role_members table.
 // This is called from GRANT <ROLE>
 type GrantRoleNode struct {
-	roles       tree.NameList
-	members     tree.NameList
+	roles       []security.SQLUsername
+	members     []security.SQLUsername
 	adminOption bool
 
 	run grantRoleRun
@@ -59,13 +59,25 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR
 	if err != nil {
 		return nil, err
 	}
-	for i := range n.Roles {
-		// TODO(solon): there are SQL identifiers (tree.Name) in
-		// n.Roles, but we want SQL usernames. Do we normalize or not? For
-		// reference, REASSIGN / OWNER TO do normalize.  Related:
-		// https://github.com/cockroachdb/cockroach/issues/54696
-		r := security.MakeSQLUsernameFromPreNormalizedString(string(n.Roles[i]))
 
+	inputRoles := make([]security.SQLUsername, len(n.Roles))
+	inputMembers := make([]security.SQLUsername, len(n.Members))
+	for i, role := range n.Roles {
+		normalizedRole, err := security.MakeSQLUsernameFromUserInput(string(role), security.UsernameValidation)
+		if err != nil {
+			return nil, err
+		}
+		inputRoles[i] = normalizedRole
+	}
+	for i, member := range n.Members {
+		normalizedMember, err := security.MakeSQLUsernameFromUserInput(string(member), security.UsernameValidation)
+		if err != nil {
+			return nil, err
+		}
+		inputMembers[i] = normalizedMember
+	}
+
+	for _, r := range inputRoles {
 		// If the user is an admin, don't check if the user is allowed to add/drop
 		// roles in the role. However, if the role being modified is the admin role, then
 		// make sure the user is an admin with the admin option.
@@ -75,10 +87,10 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR
 		if isAdmin, ok := allRoles[r]; !ok || !isAdmin {
 			if r.IsAdminRole() {
 				return nil, pgerror.Newf(pgcode.InsufficientPrivilege,
-					"%s is not a role admin for role %s", p.User(), n.Roles[i])
+					"%s is not a role admin for role %s", p.User(), r)
 			}
 			return nil, pgerror.Newf(pgcode.InsufficientPrivilege,
-				"%s is not a superuser or role admin for role %s", p.User(), n.Roles[i])
+				"%s is not a superuser or role admin for role %s", p.User(), r)
 		}
 	}
 
@@ -93,37 +105,24 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR
 	// NOTE: membership manipulation involving the "public" pseudo-role fails with
 	// "role public does not exist". This matches postgres behavior.
 
-	for i := range n.Roles {
-		// TODO(solon): there are SQL identifiers (tree.Name) in
-		// n.Roles, but we want SQL usernames. Do we normalize or not? For
-		// reference, REASSIGN / OWNER TO do normalize.  Related:
-		// https://github.com/cockroachdb/cockroach/issues/54696
-		r := security.MakeSQLUsernameFromPreNormalizedString(string(n.Roles[i]))
-
+	for _, r := range inputRoles {
 		if _, ok := roles[r]; !ok {
 			maybeOption := strings.ToUpper(r.Normalized())
 			for name := range roleoption.ByName {
 				if maybeOption == name {
 					return nil, errors.WithHintf(
 						pgerror.Newf(pgcode.UndefinedObject,
-							"role/user %s does not exist", n.Roles[i]),
+							"role/user %s does not exist", r),
 						"%s is a role option, try using ALTER ROLE to change a role's options.", maybeOption)
 				}
 			}
-			return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", n.Roles[i])
+			return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", r)
 		}
 	}
 
-	for i := range n.Members {
-		// TODO(solon): there are SQL identifiers (tree.Name) in
-		// n.Members but we want SQL usernames. Do we normalize or not? For
-		// reference, REASSIGN / OWNER TO do normalize.  Related:
-		// https://github.com/cockroachdb/cockroach/issues/54696
-		m := security.MakeSQLUsernameFromPreNormalizedString(string(n.Members[i]))
-
+	for _, m := range inputMembers {
 		if _, ok := roles[m]; !ok {
-			return nil, pgerror.Newf(pgcode.UndefinedObject,
-				"role/user %s does not exist", n.Members[i])
+			return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", m)
 		}
 	}
 
@@ -132,13 +131,7 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR
 	// For each grant.Role, we lookup all the roles it is a member of.
 	// After adding a given edge (grant.Member ∈ grant.Role), we add the edge to the list as well.
 	allRoleMemberships := make(map[security.SQLUsername]map[security.SQLUsername]bool)
-	for _, rawR := range n.Roles {
-		// TODO(solon): there are SQL identifiers (tree.Name) in
-		// n.Roles but we want SQL usernames. Do we normalize or not? For
-		// reference, REASSIGN / OWNER TO do normalize.  Related:
-		// https://github.com/cockroachdb/cockroach/issues/54696
-		r := security.MakeSQLUsernameFromPreNormalizedString(string(rawR))
-
+	for _, r := range inputRoles {
 		allRoles, err := p.MemberOfWithAdminOption(ctx, r)
 		if err != nil {
 			return nil, err
@@ -148,24 +141,17 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR
 
 	// Since we perform no queries here, check all role/member pairs for cycles.
 	// Only if there are no errors do we proceed to write them.
-	for _, rawR := range n.Roles {
-		// TODO(solon): there are SQL identifiers (tree.Name) in
-		// n.Roles but we want SQL usernames. Do we normalize or not? For
-		// reference, REASSIGN / OWNER TO do normalize.  Related:
-		// https://github.com/cockroachdb/cockroach/issues/54696
-		r := security.MakeSQLUsernameFromPreNormalizedString(string(rawR))
-		for _, rawM := range n.Members {
-			// TODO(solon): ditto above, names in n.Members.
-			m := security.MakeSQLUsernameFromPreNormalizedString(string(rawM))
+	for _, r := range inputRoles {
+		for _, m := range inputMembers {
 			if r == m {
 				// self-cycle.
-				return nil, pgerror.Newf(pgcode.InvalidGrantOperation, "%s cannot be a member of itself", rawM)
+				return nil, pgerror.Newf(pgcode.InvalidGrantOperation, "%s cannot be a member of itself", m)
 			}
 			// Check if grant.Role ∈ ... ∈ grant.Member
 			if memberOf, ok := allRoleMemberships[r]; ok {
 				if _, ok = memberOf[m]; ok {
 					return nil, pgerror.Newf(pgcode.InvalidGrantOperation,
-						"making %s a member of %s would create a cycle", rawM, rawR)
+						"making %s a member of %s would create a cycle", m, r)
 				}
 			}
 			// Add the new membership. We don't care about the actual bool value.
@@ -177,8 +163,8 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR
 	}
 
 	return &GrantRoleNode{
-		roles:       n.Roles,
-		members:     n.Members,
+		roles:       inputRoles,
+		members:     inputMembers,
 		adminOption: n.AdminOption,
 	}, nil
 }
@@ -205,7 +191,7 @@ func (n *GrantRoleNode) startExec(params runParams) error {
 				params.p.txn,
 				sessiondata.InternalExecutorOverride{User: security.RootUserName()},
 				memberStmt,
-				r, m, n.adminOption,
+				r.Normalized(), m.Normalized(), n.adminOption,
 			)
 			if err != nil {
 				return err

diff --git a/pkg/sql/logictest/testdata/logic_test/role b/pkg/sql/logictest/testdata/logic_test/role
@@ -590,6 +590,28 @@ SELECT * FROM system.role_members
 role   member    isAdmin
 admin  root      true
 
+# Verify that GRANT/REVOKE are not sensitive to the case of role names.
+
+statement ok
+GRANT roLea,rOleB TO tEstUSER WITH ADMIN OPTION
+
+query TTB colnames
+SELECT * FROM system.role_members
+----
+role   member    isAdmin
+admin  root      true
+rolea  testuser  true
+roleb  testuser  true
+
+statement ok
+REVOKE roleA, roleB FROM TestUser
+
+query TTB colnames
+SELECT * FROM system.role_members
+----
+role   member    isAdmin
+admin  root      true
+
 # Test privilege checks.
 
 statement ok

diff --git a/pkg/sql/parser/testdata/grant b/pkg/sql/parser/testdata/grant
@@ -0,0 +1,39 @@
+parse
+GRANT roLea,rOleB TO rOLeC WITH ADMIN OPTION
+----
+GRANT rolea, roleb TO rolec WITH ADMIN OPTION -- normalized!
+GRANT rolea, roleb TO rolec WITH ADMIN OPTION -- fully parenthesized
+GRANT rolea, roleb TO rolec WITH ADMIN OPTION -- literals removed
+GRANT _, _ TO _ WITH ADMIN OPTION -- identifiers removed
+
+parse
+GRANT roLea TO roLeB WITH ADMIN OPTION
+----
+GRANT rolea TO roleb WITH ADMIN OPTION -- normalized!
+GRANT rolea TO roleb WITH ADMIN OPTION -- fully parenthesized
+GRANT rolea TO roleb WITH ADMIN OPTION -- literals removed
+GRANT _ TO _ WITH ADMIN OPTION -- identifiers removed
+
+parse
+GRANT ALL ON TABLE the_table TO rolEC
+----
+GRANT ALL ON TABLE the_table TO rolec -- normalized!
+GRANT ALL ON TABLE (the_table) TO rolec -- fully parenthesized
+GRANT ALL ON TABLE the_table TO rolec -- literals removed
+GRANT ALL ON TABLE _ TO _ -- identifiers removed
+
+parse
+REVOKE roleA, roleB FROM RoleC
+----
+REVOKE rolea, roleb FROM rolec -- normalized!
+REVOKE rolea, roleb FROM rolec -- fully parenthesized
+REVOKE rolea, roleb FROM rolec -- literals removed
+REVOKE _, _ FROM _ -- identifiers removed
+
+parse
+REVOKE roleA FROM roleB
+----
+REVOKE rolea FROM roleb -- normalized!
+REVOKE rolea FROM roleb -- fully parenthesized
+REVOKE rolea FROM roleb -- literals removed
+REVOKE _ FROM _ -- identifiers removed
diff --git a/pkg/sql/revoke_role.go b/pkg/sql/revoke_role.go
@@ -25,8 +25,8 @@ import (
 // RevokeRoleNode removes entries from the system.role_members table.
 // This is called from REVOKE <ROLE>
 type RevokeRoleNode struct {
-	roles       tree.NameList
-	members     tree.NameList
+	roles       []security.SQLUsername
+	members     []security.SQLUsername
 	adminOption bool
 
 	run revokeRoleRun
@@ -56,13 +56,25 @@ func (p *planner) RevokeRoleNode(ctx context.Context, n *tree.RevokeRole) (*Revo
 	if err != nil {
 		return nil, err
 	}
-	for i := range n.Roles {
-		// TODO(solon): there are SQL identifiers (tree.Name) in n.Roles,
-		// but we want SQL usernames. Do we normalize or not? For reference,
-		// REASSIGN / OWNER TO do normalize.
-		// Related: https://github.com/cockroachdb/cockroach/issues/54696
-		r := security.MakeSQLUsernameFromPreNormalizedString(string(n.Roles[i]))
 
+	inputRoles := make([]security.SQLUsername, len(n.Roles))
+	inputMembers := make([]security.SQLUsername, len(n.Members))
+	for i, role := range n.Roles {
+		normalizedRole, err := security.MakeSQLUsernameFromUserInput(string(role), security.UsernameValidation)
+		if err != nil {
+			return nil, err
+		}
+		inputRoles[i] = normalizedRole
+	}
+	for i, member := range n.Members {
+		normalizedMember, err := security.MakeSQLUsernameFromUserInput(string(member), security.UsernameValidation)
+		if err != nil {
+			return nil, err
+		}
+		inputMembers[i] = normalizedMember
+	}
+
+	for _, r := range inputRoles {
 		// If the user is an admin, don't check if the user is allowed to add/drop
 		// roles in the role. However, if the role being modified is the admin role, then
 		// make sure the user is an admin with the admin option.
@@ -87,33 +99,21 @@ func (p *planner) RevokeRoleNode(ctx context.Context, n *tree.RevokeRole) (*Revo
 		return nil, err
 	}
 
-	for i := range n.Roles {
-		// TODO(solon): there are SQL identifiers (tree.Name) in n.Roles,
-		// but we want SQL usernames. Do we normalize or not? For reference,
-		// REASSIGN / OWNER TO do normalize.
-		// Related: https://github.com/cockroachdb/cockroach/issues/54696
-		r := security.MakeSQLUsernameFromPreNormalizedString(string(n.Roles[i]))
-
+	for _, r := range inputRoles {
 		if _, ok := roles[r]; !ok {
-			return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", n.Roles[i])
+			return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", r)
 		}
 	}
 
-	for i := range n.Members {
-		// TODO(solon): there are SQL identifiers (tree.Name) in n.Roles,
-		// but we want SQL usernames. Do we normalize or not? For reference,
-		// REASSIGN / OWNER TO do normalize.
-		// Related: https://github.com/cockroachdb/cockroach/issues/54696
-		m := security.MakeSQLUsernameFromPreNormalizedString(string(n.Members[i]))
-
+	for _, m := range inputMembers {
 		if _, ok := roles[m]; !ok {
-			return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", n.Members[i])
+			return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", m)
 		}
 	}
 
 	return &RevokeRoleNode{
-		roles:       n.Roles,
-		members:     n.Members,
+		roles:       inputRoles,
+		members:     inputMembers,
 		adminOption: n.AdminOption,
 	}, nil
 }
@@ -131,17 +131,8 @@ func (n *RevokeRoleNode) startExec(params runParams) error {
 	}
 
 	var rowsAffected int
-	for i := range n.roles {
-		// TODO(solon): there are SQL identifiers (tree.Name) in
-		// n.Roles, but we want SQL usernames. Do we normalize or not? For
-		// reference, REASSIGN / OWNER TO do normalize.  Related:
-		// https://github.com/cockroachdb/cockroach/issues/54696
-		r := security.MakeSQLUsernameFromPreNormalizedString(string(n.roles[i]))
-
-		for j := range n.members {
-			// TODO(solon): ditto above, names in n.members.
-			m := security.MakeSQLUsernameFromPreNormalizedString(string(n.members[j]))
-
+	for _, r := range n.roles {
+		for _, m := range n.members {
 			if r.IsAdminRole() && m.IsRootUser() {
 				// We use CodeObjectInUseError which is what happens if you tried to delete the current user in pg.
 				return pgerror.Newf(pgcode.ObjectInUse,