diff --git a/pkg/cli/cli_test.go b/pkg/cli/cli_test.go index 62cdccbfcd2c..3c92a27b5862 100644 --- a/pkg/cli/cli_test.go +++ b/pkg/cli/cli_test.go @@ -472,7 +472,7 @@ func Example_demo() { // system // (4 rows) // demo --insecure=false -e CREATE USER test WITH PASSWORD 'testpass' - // CREATE ROLE 1 + // CREATE ROLE // demo -e CREATE USER test WITH PASSWORD 'testpass' // ERROR: setting or updating a password is not supported in insecure mode // SQLSTATE: 28P01 diff --git a/pkg/sql/alter_role.go b/pkg/sql/alter_role.go index c4a680786832..426da97fec5a 100644 --- a/pkg/sql/alter_role.go +++ b/pkg/sql/alter_role.go @@ -31,8 +31,6 @@ type alterRoleNode struct { ifExists bool isRole bool roleOptions roleoption.List - - run alterRoleRun } // AlterRole represents a ALTER ROLE statement. @@ -77,12 +75,6 @@ func (p *planner) AlterRoleNode( }, nil } -// alterRoleRun is the run-time state of -// alterRoleNode for local execution. -type alterRoleRun struct { - rowsAffected int -} - func (n *alterRoleNode) startExec(params runParams) error { var opName string if n.isRole { @@ -156,7 +148,7 @@ func (n *alterRoleNode) startExec(params runParams) error { // Updating PASSWORD is a special case since PASSWORD lives in system.users // while the rest of the role options lives in system.role_options. - n.run.rowsAffected, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( + _, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( params.ctx, opName, params.p.txn, @@ -193,7 +185,7 @@ func (n *alterRoleNode) startExec(params runParams) error { } } - rowsAffected, err := params.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx( + _, err := params.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx( params.ctx, opName, params.p.txn, @@ -204,7 +196,6 @@ func (n *alterRoleNode) startExec(params runParams) error { if err != nil { return err } - n.run.rowsAffected += rowsAffected } return nil @@ -213,7 +204,3 @@ func (n *alterRoleNode) startExec(params runParams) error { func (*alterRoleNode) Next(runParams) (bool, error) { return false, nil } func (*alterRoleNode) Values() tree.Datums { return tree.Datums{} } func (*alterRoleNode) Close(context.Context) {} - -func (n *alterRoleNode) FastPathResults() (int, bool) { - return n.run.rowsAffected, true -} diff --git a/pkg/sql/create_role.go b/pkg/sql/create_role.go index 13cbb65e5a2f..2aa32f7e9412 100644 --- a/pkg/sql/create_role.go +++ b/pkg/sql/create_role.go @@ -32,8 +32,6 @@ type CreateRoleNode struct { isRole bool roleOptions roleoption.List userNameInfo - - run createUserRun } var userTableName = tree.NewTableName("system", "users") @@ -155,7 +153,7 @@ func (n *CreateRoleNode) startExec(params runParams) error { } // TODO(richardjcai): move hashedPassword column to system.role_options. - n.run.rowsAffected, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( + rowsAffected, err := params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( params.ctx, opName, params.p.txn, @@ -167,9 +165,9 @@ func (n *CreateRoleNode) startExec(params runParams) error { if err != nil { return err - } else if n.run.rowsAffected != 1 { + } else if rowsAffected != 1 { return errors.AssertionFailedf("%d rows affected by user creation; expected exactly one row affected", - n.run.rowsAffected, + rowsAffected, ) } @@ -197,7 +195,7 @@ func (n *CreateRoleNode) startExec(params runParams) error { } } - rowsAffected, err := params.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx( + _, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx( params.ctx, opName, params.p.txn, @@ -208,16 +206,11 @@ func (n *CreateRoleNode) startExec(params runParams) error { if err != nil { return err } - n.run.rowsAffected += rowsAffected } return nil } -type createUserRun struct { - rowsAffected int -} - // Next implements the planNode interface. func (*CreateRoleNode) Next(runParams) (bool, error) { return false, nil } @@ -227,9 +220,6 @@ func (*CreateRoleNode) Values() tree.Datums { return tree.Datums{} } // Close implements the planNode interface. func (*CreateRoleNode) Close(context.Context) {} -// FastPathResults implements the planNodeFastPath interface. -func (n *CreateRoleNode) FastPathResults() (int, bool) { return n.run.rowsAffected, true } - const usernameHelp = "Usernames are case insensitive, must start with a letter, " + "digit or underscore, may contain letters, digits, dashes, periods, or underscores, and must not exceed 63 characters." diff --git a/pkg/sql/drop_role.go b/pkg/sql/drop_role.go index a18fda89069a..2f7cfc975b13 100644 --- a/pkg/sql/drop_role.go +++ b/pkg/sql/drop_role.go @@ -31,8 +31,6 @@ type DropRoleNode struct { ifExists bool isRole bool names func() ([]string, error) - - run dropRoleRun } // DropRole represents a DROP ROLE statement. @@ -61,12 +59,6 @@ func (p *planner) DropRoleNode( }, nil } -// dropRoleRun contains the run-time state of DropRoleNode during local execution. -type dropRoleRun struct { - // The number of users deleted. - numDeleted int -} - func (n *DropRoleNode) startExec(params runParams) error { var opName string if n.isRole { @@ -160,7 +152,7 @@ func (n *DropRoleNode) startExec(params runParams) error { } // All safe - do the work. - var numUsersDeleted, numRoleMembershipsDeleted, numRoleOptionsDeleted int + var numRoleMembershipsDeleted int for normalizedUsername := range userNames { // Specifically reject special users and roles. Some (root, admin) would fail with // "privileges still exist" first. @@ -173,7 +165,7 @@ func (n *DropRoleNode) startExec(params runParams) error { pgcode.InvalidParameterValue, "cannot drop special user %s", normalizedUsername) } - rowsAffected, err := params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( + numUsersDeleted, err := params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( params.ctx, opName, params.p.txn, @@ -184,13 +176,12 @@ func (n *DropRoleNode) startExec(params runParams) error { return err } - if rowsAffected == 0 && !n.ifExists { + if numUsersDeleted == 0 && !n.ifExists { return errors.Errorf("role/user %s does not exist", normalizedUsername) } - numUsersDeleted += rowsAffected // Drop all role memberships involving the user/role. - rowsAffected, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( + numRoleMembershipsDeleted, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( params.ctx, "drop-role-membership", params.p.txn, @@ -201,7 +192,7 @@ func (n *DropRoleNode) startExec(params runParams) error { return err } - numRoleOptionsDeleted, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( + _, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( params.ctx, opName, params.p.txn, @@ -214,8 +205,6 @@ func (n *DropRoleNode) startExec(params runParams) error { if err != nil { return err } - - numRoleMembershipsDeleted += rowsAffected } if numRoleMembershipsDeleted > 0 { @@ -226,9 +215,6 @@ func (n *DropRoleNode) startExec(params runParams) error { } } - n.run.numDeleted = numUsersDeleted - n.run.numDeleted += numRoleOptionsDeleted - return nil } @@ -240,6 +226,3 @@ func (*DropRoleNode) Values() tree.Datums { return tree.Datums{} } // Close implements the planNode interface. func (*DropRoleNode) Close(context.Context) {} - -// FastPathResults implements the planNodeFastPath interface. -func (n *DropRoleNode) FastPathResults() (int, bool) { return n.run.numDeleted, true } diff --git a/pkg/sql/grant_role.go b/pkg/sql/grant_role.go index 70f6f588e345..7f17574d3cec 100644 --- a/pkg/sql/grant_role.go +++ b/pkg/sql/grant_role.go @@ -12,14 +12,17 @@ package sql import ( "context" + "strings" "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/roleoption" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/util/tracing" + "github.com/cockroachdb/errors" ) // GrantRoleNode creates entries in the system.role_members table. @@ -85,6 +88,13 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR for _, r := range n.Roles { if _, ok := roles[string(r)]; !ok { + for name := range roleoption.ByName { + if uppercase := strings.ToUpper(string(r)); uppercase == name { + return nil, errors.WithHintf( + pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", r), + "%s is a role option, try using ALTER ROLE to change a role's options.", uppercase) + } + } return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", r) } } diff --git a/pkg/sql/plan.go b/pkg/sql/plan.go index bb9d06bd7a4f..2b94ce2b4faf 100644 --- a/pkg/sql/plan.go +++ b/pkg/sql/plan.go @@ -225,9 +225,6 @@ var _ planNode = &virtualTableNode{} var _ planNode = &windowNode{} var _ planNode = &zeroNode{} -var _ planNodeFastPath = &CreateRoleNode{} -var _ planNodeFastPath = &DropRoleNode{} -var _ planNodeFastPath = &alterRoleNode{} var _ planNodeFastPath = &deleteRangeNode{} var _ planNodeFastPath = &rowCountNode{} var _ planNodeFastPath = &serializeNode{} diff --git a/pkg/sql/sem/tree/stmt.go b/pkg/sql/sem/tree/stmt.go index b69a0b2d9b31..56e31df45eeb 100644 --- a/pkg/sql/sem/tree/stmt.go +++ b/pkg/sql/sem/tree/stmt.go @@ -195,7 +195,7 @@ func (*AlterSequence) StatementType() StatementType { return DDL } func (*AlterSequence) StatementTag() string { return "ALTER SEQUENCE" } // StatementType implements the Statement interface. -func (*AlterRole) StatementType() StatementType { return RowsAffected } +func (*AlterRole) StatementType() StatementType { return Ack } // StatementTag returns a short string identifying the type of statement. func (*AlterRole) StatementTag() string { return "ALTER ROLE" } @@ -333,7 +333,7 @@ func (n *CreateTable) StatementTag() string { func (*CreateTable) modifiesSchema() bool { return true } // StatementType implements the Statement interface. -func (*CreateRole) StatementType() StatementType { return RowsAffected } +func (*CreateRole) StatementType() StatementType { return Ack } // StatementTag returns a short string identifying the type of statement. func (*CreateRole) StatementTag() string { return "CREATE ROLE" } @@ -415,7 +415,7 @@ func (*DropSequence) StatementType() StatementType { return DDL } func (*DropSequence) StatementTag() string { return "DROP SEQUENCE" } // StatementType implements the Statement interface. -func (*DropRole) StatementType() StatementType { return RowsAffected } +func (*DropRole) StatementType() StatementType { return Ack } // StatementTag returns a short string identifying the type of statement. func (*DropRole) StatementTag() string { return "DROP ROLE" }