class-validator icon indicating copy to clipboard operation
class-validator copied to clipboard

feat: add IsStrongPassword decorator

Open jhonatanhulse opened this issue 3 years ago • 6 comments

Description

Add support to password strength validation.

Checklist

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

Fixes

fixes #1024

jhonatanhulse avatar Apr 15 '21 19:04 jhonatanhulse

@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 avatar Jun 30 '21 06:06 rpCal

@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
  );
}

rpCal avatar Jun 30 '21 06:06 rpCal

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 https://github.com/typestack/class-validator/blob/ff79d7dc27dd30b1f3f955b9948d1a8e4e8ab125/src/validation/ValidatorConstraintInterface.ts#L9

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?

jhonatanhulse avatar Jul 01 '21 13:07 jhonatanhulse

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.

dakotacookenmaster avatar Jul 01 '21 18:07 dakotacookenmaster

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.

Jogiter avatar Feb 14 '22 09:02 Jogiter

@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.

jhonatanhulse avatar Feb 15 '22 14:02 jhonatanhulse

Did you close by accident?

NoNameProvided avatar Nov 21 '22 15:11 NoNameProvided

@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 avatar Nov 21 '22 15:11 jhonatanhulse

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

nosachamos avatar Nov 28 '22 23:11 nosachamos

@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

jhonatanhulse avatar Nov 30 '22 13:11 jhonatanhulse

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[bot] avatar Jan 02 '23 00:01 github-actions[bot]