Change password (#237)

* Implement ChangePassword

* Refactor: extract authenticate method

* Update tests to verify oplog entries

* More refactoring

* Remove extraneous clones

* More refactoring

internal/auth/password/argon2.go

@@ -136,6 +136,7 @@ func newArgon2Credential(accountId string, password string, conf *Argon2Configur
 			PrivateId:         id,
 			PasswordAccountId: accountId,
 			PasswordConfId:    conf.PrivateId,
+			PasswordMethodId:  conf.PasswordMethodId,
 		},
 	}
 

internal/auth/password/errors.go

@@ -14,4 +14,8 @@ var (
 	// ErrInvalidConfiguration results from attempting to perform an
 	// operation that sets a password configuration with invalid settings.
 	ErrInvalidConfiguration = errors.New("invalid configuration")
+
+	// ErrPasswordsEqual is returned from ChangePassword when the old and
+	// new passwords are equal.
+	ErrPasswordsEqual = errors.New("old and new password are equal")
 )

internal/auth/password/query.go

@@ -11,6 +11,7 @@ select acct.name,                        -- Account.Name
        acct.create_time,                 -- Account.CreateTime
        acct.update_time,                 -- Account.UpdateTime
        cred.private_id as credential_id, -- Account.CredentialId
+       cred.private_id,                  -- Argon2Credential.PrivateId
        cred.password_conf_id,            -- Argon2Credential.PasswordConfId
        cred.salt,                        -- Argon2Credential.CtSalt/Salt
        cred.derived_key,                 -- Argon2Credential.DerivedKey

internal/auth/password/repository_password.go

@@ -10,6 +10,13 @@ import (
 	"golang.org/x/crypto/argon2"
 )
 
+type authAccount struct {
+	*Account
+	*Argon2Credential
+	*Argon2Configuration
+	IsCurrentConf bool
+}
+
 // Authenticate authenticates userName and password match for userName in
 // authMethodId. The account for the userName is returned if authentication
 // is successful. Returns nil if authentication fails.
@@ -32,14 +39,121 @@ func (r *Repository) Authenticate(ctx context.Context, authMethodId string, user
 		return nil, fmt.Errorf("password authenticate: no password: %w", db.ErrInvalidParameter)
 	}
 
-	type AuthAccount struct {
-		*Account
-		*Argon2Credential
-		*Argon2Configuration
-		IsCurrentConf bool
+	acct, err := r.authenticate(ctx, authMethodId, userName, password)
+	if err != nil {
+		return nil, fmt.Errorf("password authenticate: %w", err)
 	}
+	if acct == nil {
+		return nil, nil
+	}
+
+	if !acct.IsCurrentConf {
+		cc, err := r.currentConfig(ctx, authMethodId)
+		if err != nil {
+			return acct.Account, fmt.Errorf("password authenticate: retrieve current password configuration: %w", err)
+		}
+		cred, err := newArgon2Credential(acct.PublicId, password, cc.argon2())
+		if err != nil {
+			return acct.Account, fmt.Errorf("password authenticate: update credential: %w", err)
+		}
 
-	var accts []AuthAccount
+		// do not change the Credential Id
+		cred.PrivateId = acct.CredentialId
+		if err := cred.encrypt(ctx, r.wrapper); err != nil {
+			return acct.Account, fmt.Errorf("password authenticate: update credential: encrypt: %w", err)
+		}
+
+		fields := []string{"CtSalt", "DerivedKey", "PasswordConfId"}
+		metadata := cred.oplog(oplog.OpType_OP_TYPE_UPDATE)
+
+		_, err = r.writer.DoTx(ctx, db.StdRetryCnt, db.ExpBackoff{},
+			func(_ db.Reader, w db.Writer) error {
+				rowsUpdated, err := w.Update(ctx, cred, fields, nil, db.WithOplog(r.wrapper, metadata))
+				if err == nil && rowsUpdated > 1 {
+					return db.ErrMultipleRecords
+				}
+				return err
+			},
+		)
+		if err != nil {
+			return acct.Account, fmt.Errorf("password authenticate: update credential: %w", err)
+		}
+	}
+	return acct.Account, nil
+}
+
+// ChangePassword updates the password for userName in authMethodId to new
+// if old equals the stored password. The account for the userName is
+// returned with a new CredentialId if password is successfully changed.
+//
+// Returns nil if old does not match the stored password for userName.
+// Returns ErrPasswordsEqual if old and new are equal.
+func (r *Repository) ChangePassword(ctx context.Context, authMethodId string, userName string, old, new string) (*Account, error) {
+	if authMethodId == "" {
+		return nil, fmt.Errorf("change password: no authMethodId: %w", db.ErrInvalidParameter)
+	}
+	if userName == "" {
+		return nil, fmt.Errorf("change password: no userName: %w", db.ErrInvalidParameter)
+	}
+	if old == "" {
+		return nil, fmt.Errorf("change password: no old password: %w", db.ErrInvalidParameter)
+	}
+	if new == "" {
+		return nil, fmt.Errorf("change password: no new password: %w", db.ErrInvalidParameter)
+	}
+	if old == new {
+		return nil, fmt.Errorf("change password: %w", ErrPasswordsEqual)
+	}
+
+	acct, err := r.authenticate(ctx, authMethodId, userName, old)
+	if err != nil {
+		return nil, fmt.Errorf("change password: %w", err)
+	}
+	if acct == nil {
+		return nil, nil
+	}
+
+	cc, err := r.currentConfig(ctx, authMethodId)
+	if err != nil {
+		return nil, fmt.Errorf("change password: retrieve current password configuration: %w", err)
+	}
+	if cc.MinPasswordLength > len(new) {
+		return nil, fmt.Errorf("change password: %w", ErrTooShort)
+	}
+	newCred, err := newArgon2Credential(acct.PublicId, new, cc.argon2())
+	if err != nil {
+		return nil, fmt.Errorf("change password: %w", err)
+	}
+
+	if err := newCred.encrypt(ctx, r.wrapper); err != nil {
+		return nil, fmt.Errorf("change password: encrypt: %w", err)
+	}
+
+	oldCred := acct.Argon2Credential
+
+	_, err = r.writer.DoTx(ctx, db.StdRetryCnt, db.ExpBackoff{},
+		func(_ db.Reader, w db.Writer) error {
+			rowsDeleted, err := w.Delete(ctx, oldCred, db.WithOplog(r.wrapper, oldCred.oplog(oplog.OpType_OP_TYPE_DELETE)))
+			if err == nil && rowsDeleted > 1 {
+				return db.ErrMultipleRecords
+			}
+			if err != nil {
+				return err
+			}
+			return w.Create(ctx, newCred, db.WithOplog(r.wrapper, newCred.oplog(oplog.OpType_OP_TYPE_CREATE)))
+		},
+	)
+	if err != nil {
+		return nil, fmt.Errorf("change password: %w", err)
+	}
+
+	// change the Credential Id
+	acct.Account.CredentialId = newCred.PrivateId
+	return acct.Account, nil
+}
+
+func (r *Repository) authenticate(ctx context.Context, authMethodId string, userName string, password string) (*authAccount, error) {
+	var accts []authAccount
 
 	tx, err := r.reader.DB()
 	if err != nil {
@@ -51,77 +165,32 @@ func (r *Repository) Authenticate(ctx context.Context, authMethodId string, user
 	}
 	defer rows.Close()
 	for rows.Next() {
-		var aa AuthAccount
+		var aa authAccount
 		if err := r.reader.ScanRows(rows, &aa); err != nil {
 			return nil, err
 		}
 		accts = append(accts, aa)
 	}
 
-	var acct AuthAccount
+	var acct authAccount
 	switch {
 	case len(accts) == 0:
 		return nil, nil
 	case len(accts) > 1:
 		// this should never happen
-		return nil, fmt.Errorf("authenticate: multiple accounts returned for user name")
+		return nil, fmt.Errorf("multiple accounts returned for user name")
 	default:
 		acct = accts[0]
 	}
 
 	if err := acct.decrypt(ctx, r.wrapper); err != nil {
-		return nil, fmt.Errorf("authenticate: credential: cannot decrypt value: %w", err)
+		return nil, fmt.Errorf("cannot decrypt credential: %w", err)
 	}
 
 	inputKey := argon2.IDKey([]byte(password), acct.Salt, acct.Iterations, acct.Memory, uint8(acct.Threads), acct.KeyLength)
 	if subtle.ConstantTimeCompare(inputKey, acct.DerivedKey) == 0 {
+		// authentication failed, password does not match
 		return nil, nil
 	}
-
-	if !acct.IsCurrentConf {
-		cc, err := r.currentConfig(ctx, authMethodId)
-		if err != nil {
-			return acct.Account, fmt.Errorf("authenticate: retrieve current password configuration: %w", err)
-		}
-		cred, err := newArgon2Credential(acct.PublicId, password, cc.argon2())
-		if err != nil {
-			return acct.Account, fmt.Errorf("authenticate: rehash current password: %w", err)
-		}
-		cred.PrivateId = acct.CredentialId
-		cred.PasswordMethodId = cc.PasswordMethodId
-
-		var newCred *Argon2Credential
-		_, err = r.writer.DoTx(ctx, db.StdRetryCnt, db.ExpBackoff{},
-			func(_ db.Reader, w db.Writer) error {
-				newCred = cred.clone()
-				if err := newCred.encrypt(ctx, r.wrapper); err != nil {
-					return err
-				}
-				rowsUpdated, err := w.Update(
-					ctx,
-					newCred,
-					[]string{"CtSalt", "DerivedKey", "PasswordConfId"},
-					nil,
-					db.WithOplog(r.wrapper, cred.oplog(oplog.OpType_OP_TYPE_UPDATE)),
-				)
-				if err == nil && rowsUpdated > 1 {
-					return db.ErrMultipleRecords
-				}
-				return err
-			},
-		)
-		if err != nil {
-			return acct.Account, fmt.Errorf("authenticate: update rehashed password: %w", err)
-		}
-	}
-	return acct.Account, nil
-}
-
-// ChangePassword updates the password for userName in authMethodId to new
-// if old equals the stored password. The account for the userName is
-// returned with a new CredentialId if password is successfully changed.
-//
-// Returns nil if old does not match the stored password for userName.
-func (r *Repository) ChangePassword(ctx context.Context, authMethodId string, userName string, old, new string) (*Account, error) {
-	panic("not implemented")
+	return &acct, nil
 }

internal/auth/password/repository_password_test.go

@@ -4,10 +4,12 @@ import (
 	"context"
 	"errors"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/watchtower/internal/auth/password/store"
 	"github.com/hashicorp/watchtower/internal/db"
 	"github.com/hashicorp/watchtower/internal/iam"
+	"github.com/hashicorp/watchtower/internal/oplog"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -114,6 +116,9 @@ func TestRepository_Authenticate(t *testing.T) {
 			assert.NotEmpty(authAcct.CredentialId, "CredentialId")
 			assert.Equal(tt.args.authMethodId, authAcct.AuthMethodId, "authMethodId")
 			assert.Equal(tt.args.userName, authAcct.UserName, "UserName")
+			err = db.TestVerifyOplog(t, rw, authAcct.CredentialId, db.WithOperation(oplog.OpType_OP_TYPE_UPDATE), db.WithCreateNotBefore(10*time.Second))
+			assert.Error(err)
+			assert.True(errors.Is(db.ErrRecordNotFound, err))
 		})
 	}
 }
@@ -230,4 +235,179 @@ func TestRepository_AuthenticateRehash(t *testing.T) {
 	assert.NotEqual(origCred.PasswordConfId, auth2Cred.PasswordConfId, "the configuration Id should be different")
 	assert.NotEqual(origCred.Salt, auth2Cred.Salt, "a new salt value should be generated")
 	assert.NotEqual(origCred.DerivedKey, auth2Cred.DerivedKey, "the derived key should be different")
+
+	assert.NoError(db.TestVerifyOplog(t, rw, auth2Cred.PrivateId, db.WithOperation(oplog.OpType_OP_TYPE_UPDATE), db.WithCreateNotBefore(10*time.Second)))
+}
+
+func TestRepository_ChangePassword(t *testing.T) {
+	conn, _ := db.TestSetup(t, "postgres")
+	rw := db.New(conn)
+	wrapper := db.TestWrapper(t)
+
+	repo, err := NewRepository(rw, rw, wrapper)
+	require.NoError(t, err)
+	require.NotNil(t, repo)
+
+	inAcct := &Account{
+		Account: &store.Account{
+			UserName: "kazmierczak",
+		},
+	}
+	passwd := "12345678"
+
+	type args struct {
+		authMethodId string
+		userName     string
+		old, new     string
+	}
+
+	var tests = []struct {
+		name            string
+		args            args
+		useAuthMethodId bool
+		wantAccount     bool
+		wantIsErr       error
+	}{
+		{
+			name: "invalid-no-authMethodId",
+			args: args{
+				authMethodId: "",
+				userName:     inAcct.UserName,
+				old:          passwd,
+				new:          "12345678-changed",
+			},
+			useAuthMethodId: true,
+			wantIsErr:       db.ErrInvalidParameter,
+		},
+		{
+			name: "invalid-no-userName",
+			args: args{
+				userName: "",
+				old:      passwd,
+				new:      "12345678-changed",
+			},
+			wantIsErr: db.ErrInvalidParameter,
+		},
+		{
+			name: "invalid-no-old-password",
+			args: args{
+				userName: inAcct.UserName,
+				old:      "",
+				new:      "12345678-changed",
+			},
+			wantIsErr: db.ErrInvalidParameter,
+		},
+		{
+			name: "invalid-no-new-password",
+			args: args{
+				userName: inAcct.UserName,
+				old:      passwd,
+				new:      "",
+			},
+			wantIsErr: db.ErrInvalidParameter,
+		},
+		{
+			name: "invalid-same-passwords",
+			args: args{
+				userName: inAcct.UserName,
+				old:      passwd,
+				new:      passwd,
+			},
+			wantIsErr: ErrPasswordsEqual,
+		},
+		{
+			name: "auth-failure-unknown-authMethodId",
+			args: args{
+				authMethodId: "not-an-authMethod-Id",
+				userName:     inAcct.UserName,
+				old:          passwd,
+				new:          "12345678-changed",
+			},
+			useAuthMethodId: true,
+			wantAccount:     false,
+			wantIsErr:       nil,
+		},
+		{
+			name: "auth-failure-wrong-old-password",
+			args: args{
+				userName: inAcct.UserName,
+				old:      "wrong-password",
+				new:      "12345678-changed",
+			},
+			wantAccount: false,
+			wantIsErr:   nil,
+		},
+		{
+			name: "password-to-short",
+			args: args{
+				userName: inAcct.UserName,
+				old:      passwd,
+				new:      "1",
+			},
+			wantIsErr: ErrTooShort,
+		},
+		{
+			name: "valid",
+			args: args{
+				userName: inAcct.UserName,
+				old:      passwd,
+				new:      "12345678-changed",
+			},
+			wantAccount: true,
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			assert, require := assert.New(t), require.New(t)
+
+			o, _ := iam.TestScopes(t, conn)
+			authMethod := TestAuthMethods(t, conn, o.GetPublicId(), 1)[0]
+			inAcct.AuthMethodId = authMethod.PublicId
+			outAcct, err := repo.CreateAccount(context.Background(), inAcct, WithPassword(passwd))
+			require.NoError(err)
+			require.NotNil(outAcct)
+
+			if !tt.useAuthMethodId {
+				tt.args.authMethodId = authMethod.PublicId
+			}
+			// Calls to Authenticate should always succeed in these tests
+			authFn := func(pwd string, name string) *Account {
+				acct, err := repo.Authenticate(context.Background(), inAcct.AuthMethodId, inAcct.UserName, pwd)
+				require.NotNilf(acct, "%s: Authenticate should return an account", name)
+				require.NoErrorf(err, "%s: Authenticate should succeed", name)
+				return acct
+			}
+			// authenticate with original password
+			authAcct1 := authFn(passwd, "original account")
+
+			chgAuthAcct, err := repo.ChangePassword(context.Background(), tt.args.authMethodId, tt.args.userName, tt.args.old, tt.args.new)
+			if tt.wantIsErr != nil {
+				assert.Error(err)
+				assert.Truef(errors.Is(err, tt.wantIsErr), "want err: %q got: %q", tt.wantIsErr, err)
+				assert.Nil(chgAuthAcct, "returned account")
+				authAcct2 := authFn(passwd, "error changing password: using old password")
+				assert.Equal(authAcct1.CredentialId, authAcct2.CredentialId, "CredentialId should not change")
+				return
+			}
+			assert.NoError(err)
+			if !tt.wantAccount {
+				assert.Nil(chgAuthAcct)
+				authAcct2 := authFn(passwd, "no account from changing password: using old password")
+				assert.Equal(authAcct1.CredentialId, authAcct2.CredentialId, "CredentialId should not change")
+				return
+			}
+			require.NotNil(chgAuthAcct, "returned account")
+			assert.NotEmpty(chgAuthAcct.CredentialId, "CredentialId")
+			assert.NotEqual(authAcct1.CredentialId, chgAuthAcct.CredentialId, "CredentialId should change")
+			assert.Equal(authAcct1.AuthMethodId, chgAuthAcct.AuthMethodId, "authMethodId")
+			assert.Equal(authAcct1.UserName, chgAuthAcct.UserName, "UserName")
+
+			authAcct2 := authFn(tt.args.new, "successful change password: using new password")
+			assert.Equal(chgAuthAcct.CredentialId, authAcct2.CredentialId, "CredentialId should not change")
+
+			assert.NoError(db.TestVerifyOplog(t, rw, authAcct1.CredentialId, db.WithOperation(oplog.OpType_OP_TYPE_DELETE), db.WithCreateNotBefore(10*time.Second)))
+			assert.NoError(db.TestVerifyOplog(t, rw, authAcct2.CredentialId, db.WithOperation(oplog.OpType_OP_TYPE_CREATE), db.WithCreateNotBefore(10*time.Second)))
+		})
+	}
 }

internal/db/migrations/postgres.gen.go

@@ -1836,6 +1836,20 @@ begin;
   end;
   $$ language plpgsql;
 
+  -- delete_auth_password_credential_subtype() is an after delete trigger
+  -- function for subtypes of auth_password_credential
+  create or replace function
+    delete_auth_password_credential_subtype()
+    returns trigger
+  as $$
+  begin
+    delete
+      from auth_password_credential
+     where private_id = old.private_id;
+    return null; -- result is ignored since this is an after trigger
+  end;
+  $$ language plpgsql;
+
   --
   -- triggers for time columns
   --
@@ -1993,6 +2007,11 @@ begin;
   after update on auth_password_argon2_cred
     for each row execute procedure update_auth_password_credential_subtype();
 
+  create trigger
+    delete_auth_password_credential_subtype
+  after delete on auth_password_argon2_cred
+    for each row execute procedure delete_auth_password_credential_subtype();
+
   --
   -- triggers for time columns
   --

internal/db/migrations/postgres/12_auth_password.up.sql

@@ -204,6 +204,20 @@ begin;
   end;
   $$ language plpgsql;
 
+  -- delete_auth_password_credential_subtype() is an after delete trigger
+  -- function for subtypes of auth_password_credential
+  create or replace function
+    delete_auth_password_credential_subtype()
+    returns trigger
+  as $$
+  begin
+    delete
+      from auth_password_credential
+     where private_id = old.private_id;
+    return null; -- result is ignored since this is an after trigger
+  end;
+  $$ language plpgsql;
+
   --
   -- triggers for time columns
   --

internal/db/migrations/postgres/13_auth_password_argon.up.sql

@@ -88,6 +88,11 @@ begin;
   after update on auth_password_argon2_cred
     for each row execute procedure update_auth_password_credential_subtype();
 
+  create trigger
+    delete_auth_password_credential_subtype
+  after delete on auth_password_argon2_cred
+    for each row execute procedure delete_auth_password_credential_subtype();
+
   --
   -- triggers for time columns
   --