Skip to content

fix: improve password validation #15376

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

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
b93dc6b
fix(setup): improve password validation flow on first user setup
defelmnq Oct 18, 2024
cf31dde
fix(setup): improve password validation flow on first user setup
defelmnq Oct 18, 2024
309d839
feat(password): WIP
defelmnq Oct 22, 2024
388a58b
feat(password): apply backend logic to all password set fields
defelmnq Oct 22, 2024
f9cce4c
Merge remote-tracking branch 'origin/main' into fix-password-validation
defelmnq Oct 23, 2024
8a5e63f
Merge remote-tracking branch 'origin/main' into fix-password-validation
defelmnq Oct 23, 2024
8f695ab
feat(password): apply backend logic to all password set fields
defelmnq Oct 23, 2024
c103559
feat(password): apply backend logic to all password set fields
defelmnq Oct 23, 2024
dc46019
feat(password): apply backend logic to all password set fields
defelmnq Oct 23, 2024
37072ee
feat(password): apply backend logic to all password set fields
defelmnq Oct 23, 2024
b4b8b06
feat(password): apply backend logic to all password set fields
defelmnq Oct 23, 2024
0efd24f
feat(password): add test for validate password method
defelmnq Oct 23, 2024
a6ee1cc
fix(password): display only if password is set
defelmnq Oct 24, 2024
51b1e51
WIP: Working on testing improvement
defelmnq Oct 24, 2024
e2128e6
site: change logic to generate error instead of helper text on passwo…
defelmnq Oct 24, 2024
f37ef9e
fix storybook
defelmnq Oct 24, 2024
36cadeb
Merge remote-tracking branch 'origin/main' into fix-password-validation
defelmnq Oct 25, 2024
175b4bf
feat(password): add details field to validate password endpoint
defelmnq Oct 28, 2024
f33eac2
feat(password): add details field to validate password endpoint
defelmnq Oct 28, 2024
2e0941b
Extract validation logic to a component
BrunoQuaresma Nov 4, 2024
3869dd5
Merge remote-tracking branch 'origin/main' into fix-password-validation
defelmnq Nov 5, 2024
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
Prev Previous commit
Next Next commit
feat(password): WIP
  • Loading branch information
defelmnq committed Oct 22, 2024
commit 309d8393bba0e882d399be3b407a5c043fce857e
1 change: 1 addition & 0 deletions coderd/coderd.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
32 changes: 32 additions & 0 deletions coderd/userauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,38 @@
}
}

// 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()

Check failure on line 449 in coderd/userauth.go

View workflow job for this annotation

GitHub Actions / lint

unused-receiver: method receiver 'api' is not referenced in method's body, consider removing or renaming it as _ (revive)
)

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
Expand Down
121 changes: 55 additions & 66 deletions coderd/userpassword/userpassword_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,72 +13,61 @@
"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 {

Check failure on line 28 in coderd/userpassword/userpassword_test.go

View workflow job for this annotation

GitHub Actions / lint

Range statement for test TestUserPasswordValidate does not reinitialise the variable tt (paralleltest)
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) {

Check failure on line 41 in coderd/userpassword/userpassword_test.go

View workflow job for this annotation

GitHub Actions / lint

Function TestUserPasswordCompare missing the call to method parallel (paralleltest)
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 {

Check failure on line 56 in coderd/userpassword/userpassword_test.go

View workflow job for this annotation

GitHub Actions / lint

Range statement for test TestUserPasswordCompare does not reinitialise the variable tt (paralleltest)
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)
})
}
}
22 changes: 22 additions & 0 deletions codersdk/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
}
Expand Down Expand Up @@ -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)
Expand Down
7 changes: 7 additions & 0 deletions site/src/api/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1322,6 +1322,13 @@ class ApiMethods {
await this.axios.put(`/api/v2/users/${userId}/password`, updatePassword);
};

validateUserPassword = async (
password: string,
): Promise<boolean> => {
const response = await this.axios.post("/api/v2/users/validate-password", { password });
return response.data.isValid;
};

getRoles = async (): Promise<Array<TypesGen.AssignableRoles>> => {
const response = await this.axios.get<TypesGen.AssignableRoles[]>(
"/api/v2/users/roles",
Expand Down
15 changes: 14 additions & 1 deletion site/src/pages/SetupPage/SetupPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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<boolean | null>(null);

useEffect(() => {
if (!buildInfoQuery.data) {
return;
Expand All @@ -47,12 +52,20 @@ export const SetupPage: FC = () => {
return <Navigate to="/login" state={{ isRedirect: true }} replace />;
}

const validateUserPassword = async (password: string) => {
const isValid = await validateUserPassword(password);
setIsPasswordValid(isValid);
};

const { debounced: debouncedValidateUserPassword } = useDebouncedFunction(validateUserPassword, 500);

return (
<>
<Helmet>
<title>{pageTitle("Set up your account")}</title>
</Helmet>
<SetupPageView
onPasswordChange={validateUserPassword}
isLoading={isSigningIn || createFirstUserMutation.isLoading}
error={createFirstUserMutation.error}
onSubmit={async (firstUser) => {
Expand Down
24 changes: 20 additions & 4 deletions site/src/pages/SetupPage/SetupPageView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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</>,
Expand All @@ -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(),
Expand Down Expand Up @@ -96,6 +94,14 @@ export const SetupPageView: FC<SetupPageViewProps> = ({
error,
isLoading,
}) => {
const [isPasswordValid, setIsPasswordValid] = useState<boolean | null>(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<TypesGen.CreateFirstUserRequest> =
useFormik<TypesGen.CreateFirstUserRequest>({
initialValues: {
Expand All @@ -122,6 +128,14 @@ export const SetupPageView: FC<SetupPageViewProps> = ({
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 (
<SignInLayout>
<header css={{ textAlign: "center", marginBottom: 32 }}>
Expand Down Expand Up @@ -179,6 +193,8 @@ export const SetupPageView: FC<SetupPageViewProps> = ({
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
/>
<label
htmlFor="trial"
Expand Down
Loading