Skip to content
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

Consideration for ValidationError behavior in validate method #72

Open
renatocron opened this issue Jul 9, 2023 · 0 comments
Open

Consideration for ValidationError behavior in validate method #72

renatocron opened this issue Jul 9, 2023 · 0 comments

Comments

@renatocron
Copy link

renatocron commented Jul 9, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant