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

Separating AuthorizationError from other errors within the strategy? #24

Closed
bbonamin opened this issue Nov 29, 2022 · 1 comment
Closed

Comments

@bbonamin
Copy link

Hey Sergio!

Given this action:

export async function action({ request }: ActionArgs) {
  try {
    const form = await request.formData();

    validateEmail(form.get("email"));
    validatePasswordAndConfirmation(form.get("password"), form.get("password"));

    await authenticator.authenticate("user-pass", request, {
      throwOnError: true,
      context: { formData: form },
      successRedirect: "/profile",
    });
  } catch (error) {
    if (error instanceof AuthorizationError) {
      return json({ error: errorForToast(error.message) }, { status: 401 });
    }

    // Because redirects work by throwing a Response, it needs to be re-thrown.
    // Any other generic errors must also be thrown too.
    //
    // The "instanceof Response" line is added here for clarity, but "throw error"
    // would cover it already.
    if (error instanceof Response) throw error;
    throw error;
  }
}

If any kind of error is thrown internally, such Prisma throwing that the database is down, (

if (error instanceof Error) {
), it's sent to this.failure, and then it gets wrapped in AuthorizationError by remix-auth here: https://github.com/sergiodxa/remix-auth/blob/d4e4f85745b13b0bbbb08bdd12375a91de48fd84/src/strategy.ts#LL125C35-L125C35

So effectively a prisma Error ends up being an AuthorizationError so it's treated no differently from user errors.
image

How can I determine what's an "AuthenticationError" such as wrong username, or password, which I'm doing by specifically throwing that error from within my Authenticator code [1], so that it's shown to users, versus internal server errors, that must not be displayed and should instead return a 500 or be caught by the CatchBoundary?

In my remix-auth-form strategy I'm specifically throwing AuthorizationError, but the way error handling is done it seems to cast a wider net:

export const authUserWithEmailAndPassword = async ({
  inputEmail,
  inputPassword,
}: loginUserArgs): Promise<User> => {
  const blacklisted = await existsByDomainOrEmail(inputEmail);
  if (blacklisted) {
    throw new AuthorizationError(
      "Sorry, your account has been suspended for breaching our Terms of Service."
    );
  }

  const user = await getUserByEmail(inputEmail);
  if (!user || !user.password) {
    throw new AuthorizationError("Incorrect email or password");
  }

  const passwordMatches = await bcrypt.compare(inputPassword, user.password);

  if (!passwordMatches) {
    await incrementFailedLogins(user);
    throw new AuthorizationError("Incorrect email or password");
  }

  await createLoginRecord({ userId: user.id, email: inputEmail });

  return user;
@bbonamin
Copy link
Author

SOLUTION: I needed to update to >= 1.3.0 to get access to the error.cause, which wraps the original error. Then it was a matter of updating my auth code to thrown a custom error instead, and when catching, check if error.cause is an instance of said error.

//login.tsx
export async function action({ request }: ActionArgs) {
  try {
    const form = await request.formData();

    validateEmail(form.get("email"));
    validatePasswordAndConfirmation(form.get("password"), form.get("password"));

    await authenticator.authenticate("user-pass", request, {
      throwOnError: true,
      context: { formData: form },
      successRedirect: "/profile",
    });
  } catch (error) {
    if (
      error instanceof AuthorizationError &&
      error.cause instanceof UserAuthError
    ) {
      return json(
        { error: errorForToast(getErrorMessage(error)) },
        { status: 401 }
      );
    }

    // Because redirects work by throwing a Response, it needs to be re-thrown.
    // Any other generic errors must also be thrown too.
    //
    // The "instanceof Response" line is added here for clarity, but "throw error"
    // would cover it already.
    if (error instanceof Response) throw error;
    throw error;
  }
}

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