Skip to content

Conversation

jhonatanhulse
Copy link
Contributor

Description

Add support to password strength validation.

Checklist

  • the pull request title describes what this PR does (not a vague title like Update index.md)
  • the pull request targets the default branch of the repository (develop)
  • the code follows the established code style of the repository
    • npm run prettier:check passes
    • npm run lint:check passes
  • tests are added for the changes I made (if any source code was modified)
  • documentation added or updated
  • I have run the project locally and verified that there are no errors

Fixes

fixes #1024

@rpCal
Copy link

rpCal commented Jun 30, 2021

@jhullse I just check code, its awesome! I just notice

validate: (value, args): boolean => isStrongPassword(value, args.constraints[0]) 

arg can be undefined and it can throw error... maybe you can update line to?

validate: (value, args): boolean => isStrongPassword(value, args?.constraints?.[0] ?? {})

@rpCal
Copy link

rpCal commented Jun 30, 2021

@jhullse
if you can please update also interface IsStrongPasswordOptions to contains optional params from validator.js isStrongPassword LINK

export interface IsStrongPasswordOptions {
  ...(prev interface code)

  returnScore?: boolean;
  pointsPerUnique?: number;
  pointsPerRepeat?: number;
  pointsForContainingLower?: number;
  pointsForContainingUpper?: number;
  pointsForContainingNumber?: number;
  pointsForContainingSymbol?: number;
}

in the definition of isStrongPassword If returnScore is true, then the function returns an integer score for the password rather than a boolean. so return type should be boolean or number

export function isStrongPassword(
  value: unknown,
  options?: IsStrongPasswordOptions
): boolean | number {
  ...
}

and decorator probably will look like this

export function IsStrongPassword(
  options?: IsStrongPasswordOptions,
  validationOptions?: ValidationOptions
): PropertyDecorator {
  return ValidateBy(
    {
      name: IS_STRONG_PASSWORD,
      constraints: [options],
      validator: {
        validate: (value, args): boolean => {
          const validationResult = isStrongPassword(
            value,
            args?.constraints?.[0] ?? {}
          );
          if (typeof validationResult == "boolean") {
            return validationResult;
          } else {
            return validationResult > 0;
          }
        },
        defaultMessage: buildMessage((eachPrefix) => {
          return eachPrefix + "$property is not strong enough";
        }, validationOptions),
      },
    },
    validationOptions
  );
}

@jhonatanhulse
Copy link
Contributor Author

jhonatanhulse commented Jul 1, 2021

Thanks for the feedback @rpCal. I'd like to clarify some things before I change the code.


About the first suggestion:

arg can be undefined and it can throw error... maybe you can update line to?

When I was implementing the decorator I realized that args could be undefined looking at

validate(value: any, validationArguments?: ValidationArguments): Promise<boolean> | boolean;

But at the same time I looked at several decorators in https://github.com/typestack/class-validator/tree/develop/src/decorator and they were all implemented assuming that args will be there and defined, I implemented this way to keep consistency. So I believe somehow the library is enforcing args to be available (I didn't find how exactly) and if that's the case maybe the ValidatorConstraintInterface needs to be changed. I'm OK with changing the args there but could you please give me an example of @IsStrongPassword's usage where args will be undefined?


About the second suggestion:

in the definition of isStrongPassword If returnScore is true, then the function returns an integer score for the password rather than a boolean. so return type should be boolean or number

When I was implementing the decorator the scoring part didn't seem to fit because if you use @IsStrongPassword you will never get back the score so why have an option in the decorator that says returnScore if the score won't be returned? Of course the user could use isStrongPassword function instead but we would be polluting IsStrongPasswordOptions interface, so maybe two different interfaces would make sense, one for the decorator (without scoring) and another for the function (with scoring). With two different interfaces we can avoid

          if (typeof validationResult == "boolean") {
            return validationResult;
          } else {
            return validationResult > 0;
          }

which would cause the decorator to always return true even with weak passwords (assuming that returnScore was set to true in the decorator). What do you think?

@dakotacookenmaster
Copy link

I am eagerly awaiting this! Let's get this added ASAP - I know other developers who would be very interested in having this built in to class-validator.

@Jogiter
Copy link

Jogiter commented Feb 14, 2022

nice work @jhullse. after a few months, this is still not merged. i have checked this pr:

remove changes in package.json will fix the conflicts.

i really nead this feature. so to ask would u update it. great thanks to u.

@jhonatanhulse
Copy link
Contributor Author

jhonatanhulse commented Feb 15, 2022

@Jogiter done. The problem was that Dependabot updated some dependencies. The original PR didn't change any dependencies so I basically rebased my fork and now things look good.

Jogiter
Jogiter previously approved these changes May 24, 2022
Copy link

@Jogiter Jogiter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@NoNameProvided NoNameProvided left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are still interested in getting this merged, then please rebase to the latest develop.

@NoNameProvided
Copy link
Member

Did you close by accident?

@jhonatanhulse jhonatanhulse reopened this Nov 21, 2022
Copy link
Member

@NoNameProvided NoNameProvided left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the rebase! Tests and build are currently failing.

@jhonatanhulse
Copy link
Contributor Author

jhonatanhulse commented Nov 21, 2022

@NoNameProvided The version ofvalidator.js was updated and I didn't notice, that is why the test failed. I believe it should be working now. I believe it needs approval in order to run the workflow again

@jhonatanhulse jhonatanhulse requested review from Jogiter, NoNameProvided and braaar and removed request for NoNameProvided, Jogiter and braaar November 21, 2022 19:22
@jhonatanhulse jhonatanhulse requested review from NoNameProvided and removed request for Jogiter November 21, 2022 19:22
@nosachamos
Copy link

Hey guys, really looking forward to this one - anyone able to merge it? Seems like it's all green :)

@jhonatanhulse
Copy link
Contributor Author

@NoNameProvided I believe there is a change request from you that is already fixed (tests were failing before but not anymore) but it is still blocking the PR. When you have time, could you take a look at it? I'm not sure if @braaar or @Jogiter can help with this

@NoNameProvided NoNameProvided merged commit c3130af into typestack:develop Dec 2, 2022
@github-actions
Copy link

github-actions bot commented Jan 2, 2023

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

feature: add IsStrongPassword decorator
7 participants