From b93dc6baec0fdd73d91757f8e6d18f25e9de620f Mon Sep 17 00:00:00 2001 From: defelmnq Date: Fri, 18 Oct 2024 02:26:01 +0000 Subject: [PATCH 01/17] fix(setup): improve password validation flow on first user setup --- coderd/userpassword/userpassword.go | 7 ++---- coderd/userpassword/userpassword_test.go | 27 ++++++++++++++++++++++ site/src/pages/SetupPage/SetupPageView.tsx | 7 +++++- 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/coderd/userpassword/userpassword.go b/coderd/userpassword/userpassword.go index fa16a2c89edf4..4da05ece4daf1 100644 --- a/coderd/userpassword/userpassword.go +++ b/coderd/userpassword/userpassword.go @@ -10,7 +10,6 @@ import ( "strconv" "strings" - passwordvalidator "github.com/wagslane/go-password-validator" "golang.org/x/crypto/pbkdf2" "golang.org/x/exp/slices" "golang.org/x/xerrors" @@ -138,10 +137,8 @@ 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! - // 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) < 6 { + return xerrors.Errorf("password must be at least %d characters", 6) } 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 index 1617748d5ada1..f2457def67a19 100644 --- a/coderd/userpassword/userpassword_test.go +++ b/coderd/userpassword/userpassword_test.go @@ -5,6 +5,7 @@ package userpassword_test import ( + "strings" "testing" "github.com/stretchr/testify/require" @@ -14,6 +15,32 @@ import ( func TestUserPassword(t *testing.T) { t.Parallel() + + t.Run("Invalid - Too short password", func(t *testing.T) { + t.Parallel() + err := userpassword.Validate("pass") + require.Error(t, err) + }) + + t.Run("Invalid - Too long password", func(t *testing.T) { + t.Parallel() + + var sb strings.Builder + for i := 0; i < 65; i++ { + sb.WriteString("a") + } + + err := userpassword.Validate(sb.String()) + require.Error(t, err) + }) + + t.Run("Ok", func(t *testing.T) { + t.Parallel() + + err := userpassword.Validate("CorrectPassword") + require.NoError(t, err) + }) + t.Run("Legacy", func(t *testing.T) { t.Parallel() // Ensures legacy v1 passwords function for v2. diff --git a/site/src/pages/SetupPage/SetupPageView.tsx b/site/src/pages/SetupPage/SetupPageView.tsx index a4b0536ae0b85..39d14fac61d5b 100644 --- a/site/src/pages/SetupPage/SetupPageView.tsx +++ b/site/src/pages/SetupPage/SetupPageView.tsx @@ -30,6 +30,8 @@ export const Language = { usernameLabel: "Username", emailInvalid: "Please enter a valid email address.", emailRequired: "Please enter an email address.", + passwordTooShort: "Password should be at least 6 characters.", + passwordTooLong: "Password should be no more than 64 characters.", passwordRequired: "Please enter a password.", create: "Create account", welcomeMessage: <>Welcome to Coder, @@ -54,7 +56,10 @@ const validationSchema = Yup.object({ .trim() .email(Language.emailInvalid) .required(Language.emailRequired), - password: Yup.string().required(Language.passwordRequired), + password: Yup.string() + .min(6, Language.passwordTooShort) + .max(64, Language.passwordTooLong) + .required(Language.passwordRequired), username: nameValidator(Language.usernameLabel), trial: Yup.bool(), trial_info: Yup.object().when("trial", { From cf31dde1793dc282a64e05cbe717edf2cf86fb31 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Fri, 18 Oct 2024 02:28:41 +0000 Subject: [PATCH 02/17] fix(setup): improve password validation flow on first user setup --- site/src/pages/SetupPage/SetupPageView.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/site/src/pages/SetupPage/SetupPageView.tsx b/site/src/pages/SetupPage/SetupPageView.tsx index 39d14fac61d5b..732b64d821fde 100644 --- a/site/src/pages/SetupPage/SetupPageView.tsx +++ b/site/src/pages/SetupPage/SetupPageView.tsx @@ -57,9 +57,9 @@ const validationSchema = Yup.object({ .email(Language.emailInvalid) .required(Language.emailRequired), password: Yup.string() - .min(6, Language.passwordTooShort) - .max(64, Language.passwordTooLong) - .required(Language.passwordRequired), + .min(6, Language.passwordTooShort) + .max(64, Language.passwordTooLong) + .required(Language.passwordRequired), username: nameValidator(Language.usernameLabel), trial: Yup.bool(), trial_info: Yup.object().when("trial", { From 309d8393bba0e882d399be3b407a5c043fce857e Mon Sep 17 00:00:00 2001 From: defelmnq Date: Tue, 22 Oct 2024 16:17:29 +0200 Subject: [PATCH 03/17] feat(password): WIP --- coderd/coderd.go | 1 + coderd/userauth.go | 32 ++++++ coderd/userpassword/userpassword_test.go | 121 ++++++++++----------- codersdk/users.go | 22 ++++ site/src/api/api.ts | 7 ++ site/src/pages/SetupPage/SetupPage.tsx | 15 ++- site/src/pages/SetupPage/SetupPageView.tsx | 24 +++- 7 files changed, 151 insertions(+), 71 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index cb0884808ef27..f59d3ece444bd 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -991,6 +991,7 @@ func New(options *Options) *API { r.Use(httpmw.RateLimit(options.LoginRateLimit, time.Minute)) r.Post("/login", api.postLogin) r.Post("/otp/request", api.postRequestOneTimePasscode) + r.Post("/validate-password", api.validateUserPassword) r.Post("/otp/change-password", api.postChangePasswordWithOneTimePasscode) r.Route("/oauth2", func(r chi.Router) { r.Route("/github", func(r chi.Router) { diff --git a/coderd/userauth.go b/coderd/userauth.go index 0ff3dfa8f97cc..91eb6f2e838e2 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -435,6 +435,38 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r } } +// ValidateUserPassword validates the complexity of a user password and that it is secured enough. +// +// @Summary Validate the complexity of a user password +// @ID validate-user-password +// @Accept json +// @Tags Authorization +// @Param request body codersdk.ValidateUserPasswordRequest true "Validate user password request" +// @Success 200 {object} codersdk.ValidateUserPasswordResponse +// @Router /users/validate-password [post] +func (api *API) validateUserPassword(rw http.ResponseWriter, r *http.Request) { + var ( + ctx = r.Context() + ) + + var req codersdk.ValidateUserPasswordRequest + if !httpapi.Read(ctx, rw, r, &req) { + return + } + + err := userpassword.Validate(req.Password) + if err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.ValidateUserPasswordResponse{ + Valid: false, + }) + return + } + + httpapi.Write(ctx, rw, http.StatusOK, codersdk.ValidateUserPasswordResponse{ + Valid: true, + }) +} + // Authenticates the user with an email and password. // // @Summary Log in user diff --git a/coderd/userpassword/userpassword_test.go b/coderd/userpassword/userpassword_test.go index f2457def67a19..4acc3d1edc81f 100644 --- a/coderd/userpassword/userpassword_test.go +++ b/coderd/userpassword/userpassword_test.go @@ -13,72 +13,61 @@ import ( "github.com/coder/coder/v2/coderd/userpassword" ) -func TestUserPassword(t *testing.T) { +func TestUserPasswordValidate(t *testing.T) { t.Parallel() + tests := []struct { + name string + password string + wantErr bool + }{ + {"Invalid - Too short password", "pass", true}, + {"Invalid - Too long password", strings.Repeat("a", 65), true}, + {"Ok", "CorrectPassword", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := userpassword.Validate(tt.password) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} - t.Run("Invalid - Too short password", func(t *testing.T) { - t.Parallel() - err := userpassword.Validate("pass") - require.Error(t, err) - }) - - t.Run("Invalid - Too long password", func(t *testing.T) { - t.Parallel() - - var sb strings.Builder - for i := 0; i < 65; i++ { - sb.WriteString("a") - } - - err := userpassword.Validate(sb.String()) - require.Error(t, err) - }) - - t.Run("Ok", func(t *testing.T) { - t.Parallel() - - err := userpassword.Validate("CorrectPassword") - require.NoError(t, err) - }) - - t.Run("Legacy", func(t *testing.T) { - t.Parallel() - // Ensures legacy v1 passwords function for v2. - // This has is manually generated using a print statement from v1 code. - equal, err := userpassword.Compare("$pbkdf2-sha256$65535$z8c1p1C2ru9EImBP1I+ZNA$pNjE3Yk0oG0PmJ0Je+y7ENOVlSkn/b0BEqqdKsq6Y97wQBq0xT+lD5bWJpyIKJqQICuPZcEaGDKrXJn8+SIHRg", "tomato") - require.NoError(t, err) - require.True(t, equal) - }) - - t.Run("Same", func(t *testing.T) { - t.Parallel() - hash, err := userpassword.Hash("password") - require.NoError(t, err) - equal, err := userpassword.Compare(hash, "password") - require.NoError(t, err) - require.True(t, equal) - }) - - t.Run("Different", func(t *testing.T) { - t.Parallel() - hash, err := userpassword.Hash("password") - require.NoError(t, err) - equal, err := userpassword.Compare(hash, "notpassword") - require.NoError(t, err) - require.False(t, equal) - }) - - t.Run("Invalid", func(t *testing.T) { - t.Parallel() - equal, err := userpassword.Compare("invalidhash", "password") - require.False(t, equal) - require.Error(t, err) - }) - - t.Run("InvalidParts", func(t *testing.T) { - t.Parallel() - equal, err := userpassword.Compare("abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz", "test") - require.False(t, equal) - require.Error(t, err) - }) +func TestUserPasswordCompare(t *testing.T) { + tests := []struct { + name string + hash string + password string + wantErr bool + wantEqual bool + }{ + {"Legacy", "$pbkdf2-sha256$65535$z8c1p1C2ru9EImBP1I+ZNA$pNjE3Yk0oG0PmJ0Je+y7ENOVlSkn/b0BEqqdKsq6Y97wQBq0xT+lD5bWJpyIKJqQICuPZcEaGDKrXJn8+SIHRg", "tomato", false, true}, + {"Same", "", "password", false, true}, + {"Different", "", "password", false, false}, + {"Invalid", "invalidhash", "password", true, false}, + {"InvalidParts", "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz", "test", true, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if tt.hash == "" { + hash, err := userpassword.Hash(tt.password) + require.NoError(t, err) + tt.hash = hash + } + equal, err := userpassword.Compare(tt.hash, tt.password) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + require.Equal(t, tt.wantEqual, equal) + }) + } } diff --git a/codersdk/users.go b/codersdk/users.go index f57b8010f9229..7a7231471d74f 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -176,6 +176,14 @@ type UpdateUserProfileRequest struct { Name string `json:"name" validate:"user_real_name"` } +type ValidateUserPasswordRequest struct { + Password string `json:"password" validate:"required"` +} + +type ValidateUserPasswordResponse struct { + Valid bool `json:"valid"` +} + type UpdateUserAppearanceSettingsRequest struct { ThemePreference string `json:"theme_preference" validate:"required"` } @@ -405,6 +413,20 @@ func (c *Client) UpdateUserProfile(ctx context.Context, user string, req UpdateU return resp, json.NewDecoder(res.Body).Decode(&resp) } +// ValidateUserPassword validates the complexity of a user password and that it is secured enough. +func (c *Client) ValidateUserPassword(ctx context.Context, req ValidateUserPasswordRequest) (ValidateUserPasswordResponse, error) { + res, err := c.Request(ctx, http.MethodPost, "/api/v2/users/validate-password", req) + if err != nil { + return ValidateUserPasswordResponse{}, err + } + defer res.Body.Close() + if res.StatusCode != http.StatusOK { + return ValidateUserPasswordResponse{}, ReadBodyAsError(res) + } + var resp ValidateUserPasswordResponse + return resp, json.NewDecoder(res.Body).Decode(&resp) +} + // UpdateUserStatus sets the user status to the given status func (c *Client) UpdateUserStatus(ctx context.Context, user string, status UserStatus) (User, error) { path := fmt.Sprintf("/api/v2/users/%s/status/", user) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 103a3c50e7900..50d95795c9aae 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -1322,6 +1322,13 @@ class ApiMethods { await this.axios.put(`/api/v2/users/${userId}/password`, updatePassword); }; + validateUserPassword = async ( + password: string, + ): Promise => { + const response = await this.axios.post("/api/v2/users/validate-password", { password }); + return response.data.isValid; + }; + getRoles = async (): Promise> => { const response = await this.axios.get( "/api/v2/users/roles", diff --git a/site/src/pages/SetupPage/SetupPage.tsx b/site/src/pages/SetupPage/SetupPage.tsx index ed07534919481..6e861ce3a15fd 100644 --- a/site/src/pages/SetupPage/SetupPage.tsx +++ b/site/src/pages/SetupPage/SetupPage.tsx @@ -3,13 +3,15 @@ import { createFirstUser } from "api/queries/users"; import { Loader } from "components/Loader/Loader"; import { useAuthContext } from "contexts/auth/AuthProvider"; import { useEmbeddedMetadata } from "hooks/useEmbeddedMetadata"; -import { type FC, useEffect } from "react"; +import { type FC, useEffect, useState } from "react"; import { Helmet } from "react-helmet-async"; import { useMutation, useQuery } from "react-query"; import { Navigate, useNavigate } from "react-router-dom"; import { pageTitle } from "utils/page"; import { sendDeploymentEvent } from "utils/telemetry"; import { SetupPageView } from "./SetupPageView"; +import { useDebouncedFunction } from "hooks/debounce"; + export const SetupPage: FC = () => { const { @@ -24,6 +26,9 @@ export const SetupPage: FC = () => { const { metadata } = useEmbeddedMetadata(); const buildInfoQuery = useQuery(buildInfo(metadata["build-info"])); const navigate = useNavigate(); + + const [isPasswordValid, setIsPasswordValid] = useState(null); + useEffect(() => { if (!buildInfoQuery.data) { return; @@ -47,12 +52,20 @@ export const SetupPage: FC = () => { return ; } + const validateUserPassword = async (password: string) => { + const isValid = await validateUserPassword(password); + setIsPasswordValid(isValid); + }; + + const { debounced: debouncedValidateUserPassword } = useDebouncedFunction(validateUserPassword, 500); + return ( <> {pageTitle("Set up your account")} { diff --git a/site/src/pages/SetupPage/SetupPageView.tsx b/site/src/pages/SetupPage/SetupPageView.tsx index 732b64d821fde..89cd8d40f4079 100644 --- a/site/src/pages/SetupPage/SetupPageView.tsx +++ b/site/src/pages/SetupPage/SetupPageView.tsx @@ -22,6 +22,8 @@ import { } from "utils/formUtils"; import * as Yup from "yup"; import { countries } from "./countries"; +import { useEffect, useState } from "react"; +import { debounce } from "lodash"; export const Language = { emailLabel: "Email", @@ -30,8 +32,6 @@ export const Language = { usernameLabel: "Username", emailInvalid: "Please enter a valid email address.", emailRequired: "Please enter an email address.", - passwordTooShort: "Password should be at least 6 characters.", - passwordTooLong: "Password should be no more than 64 characters.", passwordRequired: "Please enter a password.", create: "Create account", welcomeMessage: <>Welcome to Coder, @@ -57,8 +57,6 @@ const validationSchema = Yup.object({ .email(Language.emailInvalid) .required(Language.emailRequired), password: Yup.string() - .min(6, Language.passwordTooShort) - .max(64, Language.passwordTooLong) .required(Language.passwordRequired), username: nameValidator(Language.usernameLabel), trial: Yup.bool(), @@ -96,6 +94,14 @@ export const SetupPageView: FC = ({ error, isLoading, }) => { + const [isPasswordValid, setIsPasswordValid] = useState(null); + + // Debounce function to validate password + const validatePassword = debounce(async (password: string) => { + const isValid = await validateUserPassword(password); + setIsPasswordValid(isValid); // Update state based on response + }, 500); // Adjust debounce time as needed + const form: FormikContextType = useFormik({ initialValues: { @@ -122,6 +128,14 @@ export const SetupPageView: FC = ({ error, ); + useEffect(() => { + if (form.values.password) { + validatePassword(form.values.password); // Call the debounce function + } else { + setIsPasswordValid(null); // Reset validation state if password is empty + } + }, [form.values.password]); // Run effect when password changes + return (
@@ -179,6 +193,8 @@ export const SetupPageView: FC = ({ id="password" label={Language.passwordLabel} type="password" + error={isPasswordValid === false} // Show error if password is invalid + helperText={isPasswordValid === false ? "Password is not strong enough." : ""} // Provide feedback />