diff --git a/pkg/sql/grant_revoke.go b/pkg/sql/grant_revoke.go index be04804a7cf1..45fd9d3b352b 100644 --- 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 index ac1fc3027d64..aa14723458b5 100644 --- 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 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 index a239f99b3f2d..df48ca5327e8 100644 --- 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 new file mode 100644 index 000000000000..c376d5c15de2 --- /dev/null +++ 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 index 998dab33d884..a67f76299f0f 100644 --- 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 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,