Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
dwahler merged 6 commits intomainfromdavid/reset-password-cli
May 12, 2022
Merged
Show file tree
Hide file tree
Changes fromall 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
90 changes: 90 additions & 0 deletionscli/resetpassword.go
View file
Open in desktop
Original file line numberDiff line numberDiff 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)
}
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 in9d94f4f

_,err=cliui.Prompt(cmd, cliui.PromptOptions{
Text:"Confirm "+cliui.Styles.Field.Render("password")+":",
Secret:true,
Validate:func(sstring)error {
ifs!=password {
returnxerrors.Errorf("Passwords do not match")
}
returnnil
},
})
iferr!=nil {
returnxerrors.Errorf("confirm password prompt: %w",err)
}

Copy link
ContributorAuthor

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 UNIXpasswd.

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
ContributorAuthor

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 ofcoder login slightly in that if the passwords don't match, we just exit instead of repeating the second prompt. This matches the behavior of thepasswd 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.

mafredri reacted with thumbs up emojimafredri reacted with hooray emoji
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
}
106 changes: 106 additions & 0 deletionscli/resetpassword_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff 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"
)

funcTestResetPassword(t*testing.T) {
t.Parallel()

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

constemail="some@one.com"
constusername="example"
constoldPassword="password"
constnewPassword="password2"

// start postgres and coder server processes

connectionURL,closeFunc,err:=postgres.Open()
require.NoError(t,err)
defercloseFunc()
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 thinkcancel is descriptive enough and addingFunc 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
ContributorAuthor

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 fromserver_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 isclose 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 thinkpgClose orcloseConn could be a good fit (i.e. adds a bit more meaning whilst avoiding the naming conflict).

coadler reacted with thumbs up emoji
serverDone:=make(chanstruct{})
serverCmd,cfg:=clitest.New(t,"server","--address",":0","--postgres-url",connectionURL)
gofunc() {
deferclose(serverDone)
err=serverCmd.ExecuteContext(ctx)
require.ErrorIs(t,err,context.Canceled)
}()
varclient*codersdk.Client
require.Eventually(t,func()bool {
rawURL,err:=cfg.URL().Read()
iferr!=nil {
returnfalse
}
accessURL,err:=url.Parse(rawURL)
require.NoError(t,err)
client=codersdk.New(accessURL)
returntrue
},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(chanstruct{})
pty:=ptytest.New(t)
resetCmd.SetIn(pty.Input())
resetCmd.SetOut(pty.Output())
gofunc() {
deferclose(cmdDone)
err=resetCmd.Execute()
require.NoError(t,err)
}()

matches:= []struct {
outputstring
inputstring
}{
{"Enter new",newPassword},
{"Confirm",newPassword},
}
for_,match:=rangematches {
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
}
1 change: 1 addition & 0 deletionscli/root.go
View file
Open in desktop
Original file line numberDiff line numberDiff 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 deletionscoderd/database/migrate.go
View file
Open in desktop
Original file line numberDiff line numberDiff 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
returnsourceDriver,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 themigrateSetup 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
ContributorAuthor

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 theEnsureClean function instead, but that seems difficult because its behavior implicitly depends on the globalmigrations 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 deletionscoderd/database/migrate_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand Up@@ -75,3 +78,54 @@ func testSQLDB(t testing.TB) *sql.DB {

returndb
}

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

typeteststruct {
currentVersionuint
existingVersions []uint
expectedResultstring
}

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"},
}

fori,tc:=rangetests {
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:=rangetc.existingVersions {
stub.Migrations.Append(&source.Migration{
Version:version,
Identifier:"",
Direction:source.Up,
Raw:"",
})
}

err:=database.CheckLatestVersion(driver,tc.currentVersion)
varerrMessagestring
iferr!=nil {
errMessage=err.Error()
}
require.Equal(t,tc.expectedResult,errMessage)
})
}
}
[8]ページ先頭
©2009-2025 Movatter.jp