site: change logic to generate error instead of helper text on passwo…

…rd validation
coder · defelmnq · Oct 18, 2024 · Oct 18, 2024 · Oct 22, 2024 · Oct 22, 2024
commit e2128e6a8cdf0dbb7293e25bada172ed01b8d972
diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go
@@ -1880,6 +1880,39 @@ func TestUserForgotPassword(t *testing.T) {
 		requireCanLogin(t, ctx, anotherClient, anotherUser.Email, oldPassword)
 	})
 
+	t.Run("CannotChangePasswordWithWeakPassword", func(t *testing.T) {
+		t.Parallel()
+
+		notifyEnq := &testutil.FakeNotificationsEnqueuer{}
+
+		client := coderdtest.New(t, &coderdtest.Options{
+			NotificationsEnqueuer: notifyEnq,
+		})
+		user := coderdtest.CreateFirstUser(t, client)
+
+		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+		defer cancel()
+
+		anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID)
+
+		oneTimePasscode := requireRequestOneTimePasscode(t, ctx, anotherClient, notifyEnq, anotherUser.Email, anotherUser.ID)
+
+		err := anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{
+			Email:           anotherUser.Email,
+			OneTimePasscode: oneTimePasscode,
+			Password:        "notstrong",
+		})
+		var apiErr *codersdk.Error
+		require.ErrorAs(t, err, &apiErr)
+		require.Equal(t, http.StatusBadRequest, apiErr.StatusCode())
+		require.Contains(t, apiErr.Message, "Invalid password.")
+		require.Equal(t, 1, len(apiErr.Validations))
+		require.Equal(t, "password", apiErr.Validations[0].Field)
+
+		requireCannotLogin(t, ctx, anotherClient, anotherUser.Email, "notstrong")
+		requireCanLogin(t, ctx, anotherClient, anotherUser.Email, oldPassword)
+	})
+
 	t.Run("CannotChangePasswordOfAnotherUser", func(t *testing.T) {
 		t.Parallel()
 

diff --git a/coderd/userpassword/userpassword.go b/coderd/userpassword/userpassword.go
@@ -10,6 +10,7 @@ import (
 	"strconv"
 	"strings"
 
+	passwordvalidator "github.com/wagslane/go-password-validator"
 	"golang.org/x/crypto/pbkdf2"
 	"golang.org/x/exp/slices"
 	"golang.org/x/xerrors"
@@ -137,8 +138,10 @@ func hashWithSaltAndIter(password string, salt []byte, iter int) string {
 // It returns properly formatted errors for detailed form validation on the client.
 func Validate(password string) error {
 	// Ensure passwords are secure enough!
-	if len(password) < 6 {
-		return xerrors.Errorf("password must be at least %d characters", 6)
+	// See: https://github.com/wagslane/go-password-validator#what-entropy-value-should-i-use
+	err := passwordvalidator.Validate(password, 52)
+	if err != nil {
+		return err
 	}
 	if len(password) > 64 {
 		return xerrors.Errorf("password must be no more than %d characters", 64)

diff --git a/coderd/userpassword/userpassword_test.go b/coderd/userpassword/userpassword_test.go
@@ -20,9 +20,10 @@ func TestUserPasswordValidate(t *testing.T) {
 		password string
 		wantErr  bool
 	}{
-		{"Invalid - Too short password", "pass", true},
-		{"Invalid - Too long password", strings.Repeat("a", 65), true},
-		{"Ok", "CorrectPassword", false},
+		{name: "Invalid - Too short password", password: "pass", wantErr: true},
+		{name: "Invalid - Too long password", password: strings.Repeat("a", 65), wantErr: true},
+		{name: "Invalid - easy password", password: "password", wantErr: true},
+		{name: "Ok", password: "PasswordSecured123!", wantErr: false},
 	}
 
 	for _, tt := range tests {
@@ -49,11 +50,46 @@ func TestUserPasswordCompare(t *testing.T) {
 		wantErr            bool
 		wantEqual          bool
 	}{
-		{"Legacy", "$pbkdf2-sha256$65535$z8c1p1C2ru9EImBP1I+ZNA$pNjE3Yk0oG0PmJ0Je+y7ENOVlSkn/b0BEqqdKsq6Y97wQBq0xT+lD5bWJpyIKJqQICuPZcEaGDKrXJn8+SIHRg", "tomato", false, false, true},
-		{"Same", "password", "password", true, false, true},
-		{"Different", "password", "notpassword", true, false, false},
-		{"Invalid", "invalidhash", "password", false, true, false},
-		{"InvalidParts", "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz", "test", false, true, false},
+		{
+			name:               "Legacy",
+			passwordToValidate: "$pbkdf2-sha256$65535$z8c1p1C2ru9EImBP1I+ZNA$pNjE3Yk0oG0PmJ0Je+y7ENOVlSkn/b0BEqqdKsq6Y97wQBq0xT+lD5bWJpyIKJqQICuPZcEaGDKrXJn8+SIHRg",
+			password:           "tomato",
+			shouldHash:         false,
+			wantErr:            false,
+			wantEqual:          true,
+		},
+		{
+			name:               "Same",
+			passwordToValidate: "password",
+			password:           "password",
+			shouldHash:         true,
+			wantErr:            false,
+			wantEqual:          true,
+		},
+		{
+			name:               "Different",
+			passwordToValidate: "password",
+			password:           "notpassword",
+			shouldHash:         true,
+			wantErr:            false,
+			wantEqual:          false,
+		},
+		{
+			name:               "Invalid",
+			passwordToValidate: "invalidhash",
+			password:           "password",
+			shouldHash:         false,
+			wantErr:            true,
+			wantEqual:          false,
+		},
+		{
+			name:               "InvalidParts",
+			passwordToValidate: "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz",
+			password:           "test",
+			shouldHash:         false,
+			wantErr:            true,
+			wantEqual:          false,
+		},
 	}
 
 	for _, tt := range tests {

diff --git a/go.mod b/go.mod
@@ -163,6 +163,7 @@ require (
 	github.com/u-root/u-root v0.14.0
 	github.com/unrolled/secure v1.17.0
 	github.com/valyala/fasthttp v1.56.0
+	github.com/wagslane/go-password-validator v0.3.0
 	go.mozilla.org/pkcs7 v0.9.0
 	go.nhat.io/otelsql v0.14.0
 	go.opentelemetry.io/otel v1.30.0

diff --git a/go.sum b/go.sum
@@ -967,6 +967,8 @@ github.com/vmihailenco/msgpack/v5 v5.4.1 h1:cQriyiUvjTwOHg8QZaPihLWeRAAVoCpE00IU
 github.com/vmihailenco/msgpack/v5 v5.4.1/go.mod h1:GaZTsDaehaPpQVyxrf5mtQlH+pc21PIudVV/E3rRQok=
 github.com/vmihailenco/tagparser/v2 v2.0.0 h1:y09buUbR+b5aycVFQs/g70pqKVZNBmxwAhO7/IwNM9g=
 github.com/vmihailenco/tagparser/v2 v2.0.0/go.mod h1:Wri+At7QHww0WTrCBeu4J6bNtoV6mEfg5OIWRZA9qds=
+github.com/wagslane/go-password-validator v0.3.0 h1:vfxOPzGHkz5S146HDpavl0cw1DSVP061Ry2PX0/ON6I=
+github.com/wagslane/go-password-validator v0.3.0/go.mod h1:TI1XJ6T5fRdRnHqHt14pvy1tNVnrwe7m3/f1f2fDphQ=
 github.com/wlynxg/anet v0.0.3/go.mod h1:eay5PRQr7fIVAMbTbchTnO9gG65Hg/uYGdc7mguHxoA=
 github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM=
 github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg=

diff --git a/site/src/pages/CreateUserPage/CreateUserForm.tsx b/site/src/pages/CreateUserPage/CreateUserForm.tsx
@@ -205,13 +205,16 @@ export const CreateUserForm: FC<
 							helperText:
 								(form.values.login_type !== "password" &&
 									"No password required for this login type") ||
-								(form.values.password !== "" && !passwordIsValid && "password is not strong."),
+								(form.values.password !== "" &&
+									!passwordIsValid &&
+									"password is not strong enough."),
 						})}
 						autoComplete="current-password"
 						fullWidth
 						id="password"
 						data-testid="password-input"
 						disabled={form.values.login_type !== "password"}
+						error={!!(form.values.password !== "" && !passwordIsValid)}
 						label={Language.passwordLabel}
 						type="password"
 					/>

diff --git a/site/src/pages/SetupPage/SetupPageView.tsx b/site/src/pages/SetupPage/SetupPageView.tsx
@@ -183,7 +183,10 @@ export const SetupPageView: FC<SetupPageViewProps> = ({
 						id="password"
 						label={Language.passwordLabel}
 						type="password"
-						helperText={form.values.password !== "" && !passwordIsValid ? "Password is not strong." : ""}
+						error={!!(form.values.password !== "" && !passwordIsValid)}
+						helperText={
+							!passwordIsValid ? "Password is not strong enough." : ""
+						}
 					/>
 					<label
 						htmlFor="trial"

diff --git a/site/src/pages/UserSettingsPage/SecurityPage/SecurityForm.tsx b/site/src/pages/UserSettingsPage/SecurityPage/SecurityForm.tsx
@@ -96,7 +96,12 @@ export const SecurityForm: FC<SecurityFormProps> = ({
 						autoComplete="password"
 						fullWidth
 						label={Language.newPasswordLabel}
-						helperText={form.values.password !== "" && !passwordIsValid ? "Password is not strong." : ""}
+						error={!!(form.values.password !== "" && !passwordIsValid)}
+						helperText={
+							form.values.password !== "" && !passwordIsValid
+								? "Password is not strong enough."
+								: ""
+						}
 						type="password"
 					/>
 					<TextField