fix: improve password validation flow #15132

defelmnq · 2024-10-18T02:21:17Z

Refers to #14984

Currently, password validation is done backend side and is not explicit enough so it can be painful to create first users.
We'd like to make this validation easier - but also duplicate it frontend side to make it smoother.

Flows involved :

First user set password
New user set password
Change password

github-actions · 2024-10-18T02:21:36Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

johnstcn

I'm in two minds about this change.

On the one hand, I agree that the entropy-based approach is obtuse and annoying, and that it is in some way the user's responsibility to have a minimum password security value. Also, if security is your top priority, password authentication is maybe not the best approach.

On the other hand, allowing folks to just use password or abc123 feels wrong to me, and this seems like the kind of thing a security audit would find deeply wanting.

I considered the below rules:

Mandate use of at least one character from each of the following classes: [a-z] [A-Z] [0-9]
Mandate minimum length 9

However this would immediately discount passwords consisting entirely of non-English υηι¢σ∂є ¢нαяα¢тєяѕ into account.

Here's an idea: given that the entropy calculation check is relatively cheap, would it be a terrible idea to expose an endpoint to validate this? The frontend could then consume this and show the entropy calculation transparently.

EDIT: alternative proposition: adjust the rule-based approach to

At least one non-alphanumeric non-whitespace character (e.g. [^a-zA-Z0-9\s]+)
Minimum length of 9

coderd/userpassword/userpassword_test.go

BrunoQuaresma · 2024-10-22T15:37:47Z

I think the way you did it is fine, but I’d suggest trying to use Yup features to simplify it since we’re already using it. Reposting from Slack:

Ok, cool. The good thing is, you can already do something like that using Yup, the validation library we use. You can check out an example on this StackOverflow question. You can probably place it in the formUtils.ts file.

Sorry for the late message 🙏

defelmnq · 2024-10-23T12:59:17Z

I think the way you did it is fine, but I’d suggest trying to use Yup features to simplify it since we’re already using it. Reposting from Slack:

Ok, cool. The good thing is, you can already do something like that using Yup, the validation library we use. You can check out an example on this StackOverflow question. You can probably place it in the formUtils.ts file.

Sorry for the late message 🙏

Hey @BrunoQuaresma thanks for the review and validation on this piece of code. A bit more context can be found here but globally the idea is to keep frontend validation as small as possible - (probably just for the obvious things) and then have a debounced backend one for the more detailed validation - so we ensure consistent validation everywhere.

I'll put the same kind of changes in all other password fields - so the PR will be fully reviewable.

defelmnq · 2024-10-23T13:37:45Z

coderd/userpassword/userpassword.go

-	err := passwordvalidator.Validate(password, 52)
-	if err != nil {
-		return err
+	if len(password) < 6 {


For now this logic stays pretty simple - here's the place where we'll be able to apply all the validation logic we want

As far as I'm concerned here, we are squishing together three distinct separate changes:

Expose a password complexity validation endpoint on the backend

Modify the password validation logic on the FE to query the BE

Modify the password validation logic on the BE

Of these three options, 1) and 2) are relatively innocuous. 3) is the one I'm mainly concerned about.

I propose keeping the existing password validation logic (via entropy) in this PR and opening a separate PR to modify the password validation logic if required.

My proposal for a roughly equivalent non-entropy-based password validation logic is:

At least one non-alphanumeric non-whitespace character ([^a-zA-Z0-9\s])

Minimum length of 9 runes

EDIT: we should however be extremely careful if we change the validation, as users could have existing passwords that are valid per entropy but invalid per the above rule.

coderd/users_test.go

defelmnq · 2024-10-23T18:25:03Z

PR is for me ready for a first review - both frontend and backend side.

We'd just need to :

Define what we define as a strong enough password security wise cc @johnstcn (?) and change the rules based on that.
Potentially improve the wording frontend side.

defelmnq · 2024-10-24T15:36:42Z

Based on conversation @johnstcn - I changed a bit the PR.

We pushed back the validation of the password as a prerequisite. If we then want it to just be a hint, it will be done in another PR.

Also we continue to use entropy-based validation as before.

Edit : @BrunoQuaresma I am not sure yet how validation should be done for storybook ?

coderd/coderd.go

coderd/userauth.go

coderd/userpassword/userpassword_test.go

coderd/users_test.go

codersdk/users.go

BrunoQuaresma · 2024-10-25T13:13:07Z

I am not sure yet how validation should be done for storybook ?

Here are the steps that I would take:

Create an "invalid password" story for each component that uses it
Add an interaction test
Mock the API call to return the API error
Expect to see if the message is correctly displayed

You can see an example of a "test like this"here. Please, let me know if you this is helpful enough or if you have any additional questions 🙏 .

defelmnq · 2024-10-28T15:55:49Z

Just sharing some screenshot of what it currently looks like for visibility.

defelmnq · 2024-10-28T18:06:14Z

@johnstcn @BrunoQuaresma

I added some frontend tests, not sure how to do for all of them but tried to add some.
Overall, I also added the details logic described by Cian - I've not added any logic to hide the user's password yet, because the library itself describe the error as being safe to display for user, but I can add it if you think it's better.

johnstcn

Backed changes look good to me! @BrunoQuaresma could you take a look at the FE changes?

johnstcn · 2024-10-31T09:27:31Z

site/src/pages/UserSettingsPage/SecurityPage/SecurityPage.test.tsx

-test("update password with invalid password", async () => {
-	jest.spyOn(API, "updateUserPassword").mockRejectedValueOnce(
-		mockApiError({
-			message: "Invalid password.",
-			validations: [{ detail: "Invalid password.", field: "password" }],
-		}),
-	);
-
-	const { user } = await renderPage();
-	fillAndSubmitSecurityForm();
-
-	const errorMessage = await screen.findAllByText("Invalid password.");
-	expect(errorMessage).toBeDefined();
-	expect(errorMessage).toHaveLength(2);
-	expect(API.updateUserPassword).toBeCalledTimes(1);
-	expect(API.updateUserPassword).toBeCalledWith(user.id, newSecurityFormValues);
-});
-


I think we instead modify this test to spy on the validate user password endpoint.

Kira-Pilot · 2024-11-04T15:18:01Z

site/src/api/api.ts

@@ -1322,6 +1322,15 @@ class ApiMethods {
 		await this.axios.put(`/api/v2/users/${userId}/password`, updatePassword);
 	};

+	validateUserPassword = async (
+		password: string,


Should we be using the ValidateUserPasswordRequest interface here?

// typesGenerated.ts // From codersdk/users.go export interface ValidateUserPasswordRequest { readonly password: string; }

Kira-Pilot · 2024-11-04T15:31:24Z

site/src/pages/CreateUserPage/CreateUserForm.tsx

@@ -63,6 +63,8 @@ export const authMethodLanguage = {
 export interface CreateUserFormProps {
 	onSubmit: (user: TypesGen.CreateUserRequestWithOrgs) => void;
 	onCancel: () => void;
+	onPasswordChange: (password: string) => void;


I think we can also use ValidateUserPasswordRequest here

Kira-Pilot · 2024-11-04T15:35:21Z

site/src/api/queries/users.ts

@@ -9,6 +9,7 @@ import type {
 	UpdateUserProfileRequest,
 	User,
 	UsersRequest,
+	ValidateUserPasswordRequest,


Where are we using this?

Kira-Pilot · 2024-11-04T16:07:29Z

site/src/pages/CreateUserPage/CreateUserForm.tsx

@@ -104,6 +114,10 @@ export const CreateUserForm: FC<
 		error,
 	);

+	useEffect(() => {
+		onPasswordChange?.(form.values.password);
+	}, [form.values.password, onPasswordChange]); // Run effect when password changes


I think we want to avoid useEffect as much as possible - it causes unexpected rerenders, which, decoupled with your debouncing function, are going to be difficult to troubleshoot. Instead, I suggest you add an onChange attribute to the TextField component used for the password input. Something like:

onChange={(e) => { form.handleChange(e); handlePasswordChange(e.target.value); }}

If you search in the file you can see us using this pattern for other inputs.

Kira-Pilot · 2024-11-04T16:12:45Z

site/src/pages/CreateUserPage/CreateUserPage.tsx

@@ -17,6 +18,25 @@ export const CreateUserPage: FC = () => {
 	const queryClient = useQueryClient();
 	const createUserMutation = useMutation(createUser(queryClient));
 	const authMethodsQuery = useQuery(authMethods());
+	const validatePasswordMutation = useMutation(validatePassword());
+
+	const [passwordValidator, setPasswordValidator] = useState({


Is there a reason all this logic must live in the parent component instead of directly inside CreateUserForm? I could be missing something and @BrunoQuaresma might be able to weigh in.

But if this logic can live in the form component itself, I think it should, for simplicity.

Scanning down through the PR, I see that we're duplicating this logic across several views. To avoid doing this, I think we should create a hook called useValidatePassword or something similar, and then call that hook across these components.

I suggest pairing with @BrunoQuaresma or @aslilac if you need help here!

Kira-Pilot

Thanks for taking a look at this user improvement and nice job committing some full stack code!

I left some comments on your FE changes. Might be easiest to hop in Discord with one of our UI engineers. LMK if you have any questions!

aslilac · 2024-11-04T18:17:58Z

coderd/userauth.go

+// @Param request body codersdk.ValidateUserPasswordRequest true "Validate user password request"
+// @Success 200 {object} codersdk.ValidateUserPasswordResponse
+// @Router /users/validate-password [post]
+func (*API) validateUserPassword(rw http.ResponseWriter, r *http.Request) {


nit about referring to all of this stuff as "user password" validation: the password is not yet attached to a user, so it's not a user password, it's just a password, so I'd expect it to be called validatePassword and so on.

BrunoQuaresma · 2024-11-04T21:20:57Z

@defelmnq what do you think about the changes? I would also appreciate a QA from you just to ensure I didn't miss any use cases.

BrunoQuaresma

I’m self-approving this for now to avoid blocking the PR any longer. If needed, I can take any follow-up actions later on. @defelmnq it is up to you when to merge this PR 🙏

FE requests applied

EdwardAngert · 2024-11-05T17:37:54Z

#15382 (merged to main) fixes the errors in the weekly-docs check that's currently failing

Refers to #14984 Currently, password validation is done backend side and is not explicit enough so it can be painful to create first users. We'd like to make this validation easier - but also duplicate it frontend side to make it smoother. Flows involved : - First user set password - New user set password - Change password --------- Co-authored-by: BrunoQuaresma <bruno_nonato_quaresma@hotmail.com>

defelmnq requested a review from BrunoQuaresma October 18, 2024 02:21

defelmnq self-assigned this Oct 18, 2024

fix(setup): improve password validation flow on first user setup

b93dc6b

defelmnq force-pushed the fix-password-validation branch from 07708bb to b93dc6b Compare October 18, 2024 02:26

defelmnq changed the title ~~fix(pages): improve password validation flow on first user setup~~ fix: improve password validation flow on first user setup Oct 18, 2024

fix(setup): improve password validation flow on first user setup

cf31dde

mtojek requested a review from johnstcn October 18, 2024 07:50

johnstcn reviewed Oct 18, 2024

View reviewed changes

coderd/userpassword/userpassword_test.go Outdated Show resolved Hide resolved

feat(password): WIP

309d839

defelmnq commented Oct 23, 2024

View reviewed changes

defelmnq force-pushed the fix-password-validation branch from 43124c0 to 207d5cc Compare October 23, 2024 15:36

feat(password): apply backend logic to all password set fields

388a58b

defelmnq force-pushed the fix-password-validation branch from 207d5cc to 388a58b Compare October 23, 2024 15:42

defelmnq added 5 commits October 23, 2024 15:43

Merge remote-tracking branch 'origin/main' into fix-password-validation

f9cce4c

Merge remote-tracking branch 'origin/main' into fix-password-validation

8a5e63f

feat(password): apply backend logic to all password set fields

8f695ab

feat(password): apply backend logic to all password set fields

c103559

feat(password): apply backend logic to all password set fields

dc46019

defelmnq changed the title ~~fix: improve password validation flow on first user setup~~ fix: improve password validation flow Oct 23, 2024

defelmnq added 3 commits October 23, 2024 17:32

feat(password): apply backend logic to all password set fields

37072ee

feat(password): apply backend logic to all password set fields

b4b8b06

feat(password): add test for validate password method

0efd24f

defelmnq commented Oct 23, 2024

View reviewed changes

coderd/users_test.go Outdated Show resolved Hide resolved

defelmnq requested a review from johnstcn October 23, 2024 18:23

defelmnq marked this pull request as ready for review October 23, 2024 18:24

fix(password): display only if password is set

a6ee1cc

johnstcn reviewed Oct 25, 2024

View reviewed changes

coderd/coderd.go Show resolved Hide resolved

coderd/userauth.go Show resolved Hide resolved

coderd/userpassword/userpassword_test.go Show resolved Hide resolved

coderd/users_test.go Outdated Show resolved Hide resolved

codersdk/users.go Outdated Show resolved Hide resolved

defelmnq added 2 commits October 25, 2024 13:50

Merge remote-tracking branch 'origin/main' into fix-password-validation

36cadeb

feat(password): add details field to validate password endpoint

175b4bf

defelmnq requested a review from johnstcn October 28, 2024 17:44

feat(password): add details field to validate password endpoint

f33eac2

defelmnq force-pushed the fix-password-validation branch from 277cb57 to f33eac2 Compare October 28, 2024 17:45

johnstcn approved these changes Oct 31, 2024

View reviewed changes

Kira-Pilot reviewed Nov 4, 2024

View reviewed changes

Kira-Pilot previously requested changes Nov 4, 2024

View reviewed changes

aslilac reviewed Nov 4, 2024

View reviewed changes

Extract validation logic to a component

2e0941b

BrunoQuaresma requested review from aslilac and Kira-Pilot November 4, 2024 21:19

Merge remote-tracking branch 'origin/main' into fix-password-validation

3869dd5

BrunoQuaresma approved these changes Nov 5, 2024

View reviewed changes

defelmnq mentioned this pull request Nov 5, 2024

fix: improve password validation #15376

Closed

defelmnq merged commit 4fe2c5f into main Nov 5, 2024
55 of 60 checks passed

defelmnq deleted the fix-password-validation branch November 5, 2024 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: improve password validation flow #15132

fix: improve password validation flow #15132

defelmnq commented Oct 18, 2024 •

edited

Loading

github-actions bot commented Oct 18, 2024 •

edited

Loading

johnstcn left a comment •

edited

Loading

BrunoQuaresma commented Oct 22, 2024

defelmnq commented Oct 23, 2024

defelmnq Oct 23, 2024

johnstcn Oct 24, 2024 •

edited

Loading

defelmnq commented Oct 23, 2024

defelmnq commented Oct 24, 2024 •

edited

Loading

BrunoQuaresma commented Oct 25, 2024

defelmnq commented Oct 28, 2024

defelmnq commented Oct 28, 2024

johnstcn left a comment

johnstcn Oct 31, 2024

Kira-Pilot Nov 4, 2024 •

edited

Loading

Kira-Pilot Nov 4, 2024

Kira-Pilot Nov 4, 2024

Kira-Pilot Nov 4, 2024

Kira-Pilot Nov 4, 2024

Kira-Pilot Nov 4, 2024

Kira-Pilot left a comment

aslilac Nov 4, 2024 •

edited

Loading

BrunoQuaresma commented Nov 4, 2024

BrunoQuaresma left a comment

EdwardAngert commented Nov 5, 2024 •

edited

Loading

fix: improve password validation flow #15132

fix: improve password validation flow #15132

Conversation

defelmnq commented Oct 18, 2024 • edited Loading

github-actions bot commented Oct 18, 2024 • edited Loading

johnstcn left a comment • edited Loading

Choose a reason for hiding this comment

BrunoQuaresma commented Oct 22, 2024

defelmnq commented Oct 23, 2024

defelmnq Oct 23, 2024

Choose a reason for hiding this comment

johnstcn Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

defelmnq commented Oct 23, 2024

defelmnq commented Oct 24, 2024 • edited Loading

BrunoQuaresma commented Oct 25, 2024

defelmnq commented Oct 28, 2024

defelmnq commented Oct 28, 2024

johnstcn left a comment

Choose a reason for hiding this comment

johnstcn Oct 31, 2024

Choose a reason for hiding this comment

Kira-Pilot Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Kira-Pilot Nov 4, 2024

Choose a reason for hiding this comment

Kira-Pilot Nov 4, 2024

Choose a reason for hiding this comment

Kira-Pilot Nov 4, 2024

Choose a reason for hiding this comment

Kira-Pilot Nov 4, 2024

Choose a reason for hiding this comment

Kira-Pilot Nov 4, 2024

Choose a reason for hiding this comment

Kira-Pilot left a comment

Choose a reason for hiding this comment

aslilac Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

BrunoQuaresma commented Nov 4, 2024

BrunoQuaresma left a comment

Choose a reason for hiding this comment

EdwardAngert commented Nov 5, 2024 • edited Loading

defelmnq commented Oct 18, 2024 •

edited

Loading

github-actions bot commented Oct 18, 2024 •

edited

Loading

johnstcn left a comment •

edited

Loading

johnstcn Oct 24, 2024 •

edited

Loading

defelmnq commented Oct 24, 2024 •

edited

Loading

Kira-Pilot Nov 4, 2024 •

edited

Loading

aslilac Nov 4, 2024 •

edited

Loading

EdwardAngert commented Nov 5, 2024 •

edited

Loading