Skip to content

feat: Add reset-password command #1380

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 12, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions cli/resetpassword.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package cli

import (
"database/sql"

"github.com/spf13/cobra"
"golang.org/x/xerrors"

"github.com/coder/coder/cli/cliflag"
"github.com/coder/coder/cli/cliui"
"github.com/coder/coder/coderd/database"
"github.com/coder/coder/coderd/userpassword"
)

func resetPassword() *cobra.Command {
var (
postgresURL string
)

root := &cobra.Command{
Use: "reset-password --postgres-url url username",
Short: "Reset a user's password by directly updating the database",
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
username := args[0]

sqlDB, err := sql.Open("postgres", postgresURL)
if err != nil {
return xerrors.Errorf("dial postgres: %w", err)
}
defer sqlDB.Close()

err = database.EnsureClean(sqlDB)
if err != nil {
return xerrors.Errorf("database needs migration: %w", err)
}
db := database.New(sqlDB)

user, err := db.GetUserByEmailOrUsername(cmd.Context(), database.GetUserByEmailOrUsernameParams{
Username: username,
})
if err != nil {
return xerrors.Errorf("retrieving user: %w", err)
}

password, err := cliui.Prompt(cmd, cliui.PromptOptions{
Text: "Enter new " + cliui.Styles.Field.Render("password") + ":",
Secret: true,
Validate: cliui.ValidateNotEmpty,
})
if err != nil {
return xerrors.Errorf("password prompt: %w", err)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a confirm step, similar to create first user?

coder/cli/login.go

Lines 121 to 133 in 9d94f4f

_, err = cliui.Prompt(cmd, cliui.PromptOptions{
Text: "Confirm " + cliui.Styles.Field.Render("password") + ":",
Secret: true,
Validate: func(s string) error {
if s != password {
return xerrors.Errorf("Passwords do not match")
}
return nil
},
})
if err != nil {
return xerrors.Errorf("confirm password prompt: %w", err)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question.

Personally, I think the value of a password confirmation step is in making it harder to accidentally lock yourself out by changing your own password to something with a typo in it. I understand why it's helpful in the "create first user" flow, since you can only do that once, but if you make a typo when resetting a password it's easy to just re-run the command and fix it, since you're assumed to have direct access to the DB.

On the other hand, I can see the argument for just doing the confirmation anyway, for consistency with "create first user" and with UNIX passwd.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree that it's a bit redundant since you can just change it again.

My thought was both for uniformity and that the repetition increases the chance of typing the correct password, which could help when the user hops onto e.g. the webui and tries to login. If they're very confident they typed in the right password on the cli, but can't login on the web, that could lead to a frustrating experience.

I think either approach is fine though so I'll leave it up to you. 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense. I added a confirmation prompt, but I deviated from the behavior of coder login slightly in that if the passwords don't match, we just exit instead of repeating the second prompt. This matches the behavior of the passwd command, and I think it makes more sense because if you make a typo, you probably don't know whether it's the first or second input that's correct.


hashedPassword, err := userpassword.Hash(password)
if err != nil {
return xerrors.Errorf("hash password: %w", err)
}

err = db.UpdateUserHashedPassword(cmd.Context(), database.UpdateUserHashedPasswordParams{
ID: user.ID,
HashedPassword: []byte(hashedPassword),
})
if err != nil {
return xerrors.Errorf("updating password: %w", err)
}

return nil
},
}

cliflag.StringVarP(root.Flags(), &postgresURL, "postgres-url", "", "CODER_PG_CONNECTION_URL", "", "URL of a PostgreSQL database to connect to")

return root
}
100 changes: 100 additions & 0 deletions cli/resetpassword_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
package cli_test

import (
"context"
"net/url"
"runtime"
"testing"
"time"

"github.com/stretchr/testify/require"

"github.com/coder/coder/cli/clitest"
"github.com/coder/coder/coderd/database/postgres"
"github.com/coder/coder/codersdk"
"github.com/coder/coder/pty/ptytest"
)

func TestResetPassword(t *testing.T) {
t.Parallel()

if runtime.GOOS != "linux" || testing.Short() {
// Skip on non-Linux because it spawns a PostgreSQL instance.
t.SkipNow()
}

// start postgres and coder server processes

connectionURL, closeFunc, err := postgres.Open()
require.NoError(t, err)
defer closeFunc()
ctx, cancelFunc := context.WithCancel(context.Background())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit: Variables should be named after what they do or contain, not their types. As such, I think cancel is descriptive enough and adding Func is redundant (similarly with pg open above).

Dave Cheney has a pretty good writeup on this: https://dave.cheney.net/2019/01/29/you-shouldnt-name-your-variables-after-their-types-for-the-same-reason-you-wouldnt-name-your-pets-dog-or-cat

PS. I'm reviewing this with new-hire goggles, fully aware this may be based on existing code, but thought I'd call this out anyway 😄.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, fair point! This particular chunk of code is copy-and-pasted from server_test.go. I should probably go back and see if I can break it out into a cleaner "test helper" abstraction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem is close overrides a builtin function, one of the linters gets mad :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a very good point @coadler, missed that. In that situation I think pgClose or closeConn could be a good fit (i.e. adds a bit more meaning whilst avoiding the naming conflict).

serverDone := make(chan struct{})
serverCmd, cfg := clitest.New(t, "server", "--address", ":0", "--postgres-url", connectionURL)
go func() {
defer close(serverDone)
err = serverCmd.ExecuteContext(ctx)
require.ErrorIs(t, err, context.Canceled)
}()
var client *codersdk.Client
require.Eventually(t, func() bool {
rawURL, err := cfg.URL().Read()
if err != nil {
return false
}
accessURL, err := url.Parse(rawURL)
require.NoError(t, err)
client = codersdk.New(accessURL)
return true
}, 15*time.Second, 25*time.Millisecond)
_, err = client.CreateFirstUser(ctx, codersdk.CreateFirstUserRequest{
Email: "some@one.com",
Username: "example",
Password: "password",
OrganizationName: "example",
})
require.NoError(t, err)

// reset the password

resetCmd, cmdCfg := clitest.New(t, "reset-password", "--postgres-url", connectionURL, "example")
clitest.SetupConfig(t, client, cmdCfg)
cmdDone := make(chan struct{})
pty := ptytest.New(t)
resetCmd.SetIn(pty.Input())
resetCmd.SetOut(pty.Output())
go func() {
defer close(cmdDone)
err = resetCmd.Execute()
require.NoError(t, err)
}()

matches := []struct {
output string
input string
}{
{"password", "password2"},
}
for _, match := range matches {
pty.ExpectMatch(match.output)
pty.WriteLine(match.input)
}
<-cmdDone

// now try logging in

_, err = client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{
Email: "some@one.com",
Password: "password",
})
require.Error(t, err)

_, err = client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{
Email: "some@one.com",
Password: "password2",
})
require.NoError(t, err)

cancelFunc()
<-serverDone
}
1 change: 1 addition & 0 deletions cli/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ func Root() *cobra.Command {
list(),
login(),
publickey(),
resetPassword(),
server(),
show(),
start(),
Expand Down
81 changes: 74 additions & 7 deletions coderd/database/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,38 +4,40 @@ import (
"database/sql"
"embed"
"errors"
"os"

"github.com/golang-migrate/migrate/v4"
"github.com/golang-migrate/migrate/v4/database/postgres"
"github.com/golang-migrate/migrate/v4/source"
"github.com/golang-migrate/migrate/v4/source/iofs"
"golang.org/x/xerrors"
)

//go:embed migrations/*.sql
var migrations embed.FS

func migrateSetup(db *sql.DB) (*migrate.Migrate, error) {
func migrateSetup(db *sql.DB) (source.Driver, *migrate.Migrate, error) {
sourceDriver, err := iofs.New(migrations, "migrations")
if err != nil {
return nil, xerrors.Errorf("create iofs: %w", err)
return nil, nil, xerrors.Errorf("create iofs: %w", err)
}

dbDriver, err := postgres.WithInstance(db, &postgres.Config{})
if err != nil {
return nil, xerrors.Errorf("wrap postgres connection: %w", err)
return nil, nil, xerrors.Errorf("wrap postgres connection: %w", err)
}

m, err := migrate.NewWithInstance("", sourceDriver, "", dbDriver)
if err != nil {
return nil, xerrors.Errorf("new migrate instance: %w", err)
return nil, nil, xerrors.Errorf("new migrate instance: %w", err)
}

return m, nil
return sourceDriver, m, nil
}

// MigrateUp runs SQL migrations to ensure the database schema is up-to-date.
func MigrateUp(db *sql.DB) error {
m, err := migrateSetup(db)
_, m, err := migrateSetup(db)
if err != nil {
return xerrors.Errorf("migrate setup: %w", err)
}
Expand All @@ -55,7 +57,7 @@ func MigrateUp(db *sql.DB) error {

// MigrateDown runs all down SQL migrations.
func MigrateDown(db *sql.DB) error {
m, err := migrateSetup(db)
_, m, err := migrateSetup(db)
if err != nil {
return xerrors.Errorf("migrate setup: %w", err)
}
Expand All @@ -72,3 +74,68 @@ func MigrateDown(db *sql.DB) error {

return nil
}

// EnsureClean checks whether all migrations for the current version have been
// applied, without making any changes to the database. If not, returns a
// non-nil error.
func EnsureClean(db *sql.DB) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it helps, but if you want to test 99% of this function, you could write this as e.g. func EnsureClean(db *sql.DB) error { return ensureClean(db, migrateSetup); }.

This would let you swap out the migrateSetup function in your test, if that helps with the global state (considering your comment here #1380 (comment)). It would still require an internal test package but I think that's fine for this use-case.

sourceDriver, m, err := migrateSetup(db)
if err != nil {
return xerrors.Errorf("migrate setup: %w", err)
}

version, dirty, err := m.Version()
if err != nil {
return xerrors.Errorf("get migration version: %w", err)
}

if dirty {
return xerrors.Errorf("database has not been cleanly migrated")
}

// Verify that the database's migration version is "current" by checking
// that a migration with that version exists, but there is no next version.
err = CheckLatestVersion(sourceDriver, version)
if err != nil {
return xerrors.Errorf("database needs migration: %w", err)
}

return nil
}

// Returns nil if currentVersion corresponds to the latest available migration,
// otherwise an error explaining why not.
func CheckLatestVersion(sourceDriver source.Driver, currentVersion uint) error {
Comment on lines +106 to +108
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only thing I'm significantly unhappy about with this commit. I feel like this ought to be a private function, but that doesn't play nicely with the way we put tests in a separate package.

The alternative would be to test the EnsureClean function instead, but that seems difficult because its behavior implicitly depends on the global migrations data, for consistency with the rest of the package.

// This is ugly, but seems like the only way to do it with the public
// interfaces provided by golang-migrate.

// Check that there is no later version
nextVersion, err := sourceDriver.Next(currentVersion)
if err == nil {
return xerrors.Errorf("current version is %d, but later version %d exists", currentVersion, nextVersion)
}
if !errors.Is(err, os.ErrNotExist) {
return xerrors.Errorf("get next migration after %d: %w", currentVersion, err)
}

// Once we reach this point, we know that either currentVersion doesn't
// exist, or it has no successor (the return value from
// sourceDriver.Next() is the same in either case). So we need to check
// that either it's the first version, or it has a predecessor.

firstVersion, err := sourceDriver.First()
if err != nil {
// the total number of migrations should be non-zero, so this must be
// an actual error, not just a missing file
return xerrors.Errorf("get first migration: %w", err)
}
if firstVersion == currentVersion {
return nil
}

_, err = sourceDriver.Prev(currentVersion)
if err != nil {
return xerrors.Errorf("get previous migration: %w", err)
}
return nil
}
54 changes: 54 additions & 0 deletions coderd/database/migrate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@ package database_test

import (
"database/sql"
"fmt"
"testing"

"github.com/golang-migrate/migrate/v4/source"
"github.com/golang-migrate/migrate/v4/source/stub"
"github.com/stretchr/testify/require"
"go.uber.org/goleak"

Expand Down Expand Up @@ -75,3 +78,54 @@ func testSQLDB(t testing.TB) *sql.DB {

return db
}

// paralleltest linter doesn't correctly handle table-driven tests (https://github.com/kunwardeep/paralleltest/issues/8)
// nolint:paralleltest
func TestCheckLatestVersion(t *testing.T) {
t.Parallel()

type test struct {
currentVersion uint
existingVersions []uint
expectedResult string
}

tests := []test{
// successful cases
{1, []uint{1}, ""},
{3, []uint{1, 2, 3}, ""},
{3, []uint{1, 3}, ""},

// failure cases
{1, []uint{1, 2}, "current version is 1, but later version 2 exists"},
{2, []uint{1, 2, 3}, "current version is 2, but later version 3 exists"},
{4, []uint{1, 2, 3}, "get previous migration: prev for version 4 : file does not exist"},
{4, []uint{1, 2, 3, 5}, "get previous migration: prev for version 4 : file does not exist"},
}

for i, tc := range tests {
i, tc := i, tc
t.Run(fmt.Sprintf("entry %d", i), func(t *testing.T) {
t.Parallel()

driver, _ := stub.WithInstance(nil, &stub.Config{})
stub, ok := driver.(*stub.Stub)
require.True(t, ok)
for _, version := range tc.existingVersions {
stub.Migrations.Append(&source.Migration{
Version: version,
Identifier: "",
Direction: source.Up,
Raw: "",
})
}

err := database.CheckLatestVersion(driver, tc.currentVersion)
var errMessage string
if err != nil {
errMessage = err.Error()
}
require.Equal(t, tc.expectedResult, errMessage)
})
}
}