time2fa icon indicating copy to clipboard operation
time2fa copied to clipboard

Consideration for ValidationError behavior in validate method

Open renatocron opened this issue 2 years ago • 0 comments

Hello,

First, thank you for this typescript/no external dep implementation of totp, it was about time to replace speakeasy.

I was writing the tests for my application and got a bit confused about a ValidationError: Invalid passcode exception being thrown. Initially, I thought something was broken in my NestJS application, because the string was actually the same one I used in my custom exception!

While reviewing the validate method in the TOTP (time based) implementation:

public validate(options: TotpValidateOptions, config?: TotpConfig): boolean {
  const validatedConfig = generateConfig(config);

  const passcode = options?.passcode.replace(/\s/g, "") || "";
  if (passcode.length !== validatedConfig.digits) {
    throw new ValidationError("Invalid passcode");
  }

  const codes = this.generatePasscodes(options, validatedConfig);

  if (codes.includes(passcode)) {
    return true;
  }

  return false;
}

I noticed that when the length of the passcode is not equal to validatedConfig.digits, the code throws a ValidationError stating "Invalid passcode".

Since the validation seems to be logically part of the overall functionality and not related to the configuration issues of the library itself, I was wondering if it's more appropriate for this situation to return false instead of throwing a ValidationError.

By adjusting this, users of the library might have a smoother experience, specifically when the throw error interrupts application flow. Instead, they could manage the false return value in a way that suits their specific context.

Or maybe we should update the docs to add the try/catch in the usage section.

Does this adjustment make sense from your perspective, or are there specific reasons that a ValidationError needs to be thrown in this method?

Thank you, Renato

renatocron avatar Jul 09 '23 23:07 renatocron