-
Notifications
You must be signed in to change notification settings - Fork 924
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
Changes from all commits
b080572
18de0d2
bfa835d
907f457
f6f2310
cb36912
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,90 @@ | ||||||||||||||||||||||||||||
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 <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 = sqlDB.Ping() | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
return xerrors.Errorf("ping postgres: %w", err) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
err = database.EnsureClean(sqlDB) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
return xerrors.Errorf("database needs migration: %w", err) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
coadler marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||
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) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add a confirm step, similar to create first user? Lines 121 to 133 in 9d94f4f
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 👍🏻 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||||||||||||||||
confirmedPassword, err := cliui.Prompt(cmd, cliui.PromptOptions{ | ||||||||||||||||||||||||||||
Text: "Confirm " + cliui.Styles.Field.Render("password") + ":", | ||||||||||||||||||||||||||||
Secret: true, | ||||||||||||||||||||||||||||
Validate: cliui.ValidateNotEmpty, | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
return xerrors.Errorf("confirm password prompt: %w", err) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
if password != confirmedPassword { | ||||||||||||||||||||||||||||
return xerrors.New("Passwords do not match") | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
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 | ||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
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() | ||
} | ||
|
||
const email = "some@one.com" | ||
const username = "example" | ||
const oldPassword = "password" | ||
const newPassword = "password2" | ||
|
||
// start postgres and coder server processes | ||
|
||
connectionURL, closeFunc, err := postgres.Open() | ||
require.NoError(t, err) | ||
defer closeFunc() | ||
ctx, cancelFunc := context.WithCancel(context.Background()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 😄. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One problem is There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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: email, | ||
Username: username, | ||
Password: oldPassword, | ||
OrganizationName: "example", | ||
}) | ||
require.NoError(t, err) | ||
|
||
// reset the password | ||
|
||
resetCmd, cmdCfg := clitest.New(t, "reset-password", "--postgres-url", connectionURL, username) | ||
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 | ||
}{ | ||
{"Enter new", newPassword}, | ||
{"Confirm", newPassword}, | ||
} | ||
for _, match := range matches { | ||
pty.ExpectMatch(match.output) | ||
pty.WriteLine(match.input) | ||
} | ||
<-cmdDone | ||
|
||
// now try logging in | ||
|
||
_, err = client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ | ||
Email: email, | ||
Password: oldPassword, | ||
}) | ||
require.Error(t, err) | ||
|
||
_, err = client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ | ||
Email: email, | ||
Password: newPassword, | ||
}) | ||
require.NoError(t, err) | ||
|
||
cancelFunc() | ||
<-serverDone | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
} | ||
|
@@ -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) | ||
} | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. This would let you swap out the |
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
// 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 | ||
} |
Uh oh!
There was an error while loading. Please reload this page.