Authenticator 'authenticate' throws error with only successRedirect passed

Describe the bug

When successfully authenticating, authenticator will throw a redirect response if only successRedirect is provided to the config. Not sure why the response is thrown when successful.

Your Example Website or App

localhost

Steps to Reproduce the Bug or Issue

Export this action

export async function action({ request }: ActionArgs) {
  // we call the method with the name of the strategy we want to use and the
  // request object, optionally we pass an object with the URLs we want the user
  // to be redirected to after a success or a failure
  try {
    await authenticator.authenticate("user-pass", request, {
      successRedirect: "/dashboard",
    });
  } catch (err: any) { // NodeResponse is thrown on success....

   // Have to check if its actually the success response redirect
    if (err instanceof Response && err?.status === 302) {
      return err;
    }
    return json({ error: "invalid" }, { status: 401 });
  }
}

Set up FormStrategy

authenticator.use(
  new FormStrategy(async ({ form }) => {
    const body = {
      identifier: form.get("email"),
      password: form.get("password"),
    };

    const response = await fetch("localhost:3001/api/auth/login", {
      method: "POST",
      headers: {
        "Content-Type": "application/json",
      },
      body: JSON.stringify(body),
    });

    if (response.status !== 200) {
      throw new Error("Invalid username or password");
    }

    const json = await response.json();
    return json;
  }),
  // each strategy has a name and can be changed to use another one
  // same strategy multiple times, especially useful for the OAuth2 strategy.
  "user-pass"
);

Test a form login action
Note: in my example and when coming across this, no error was being thrown from the FormStrategy (status was 200 and the fetch succeeded)

Expected behavior

I would expect that, upon successfully authenticating and only a successRedirect being present in the input config, that the redirect response is returned as a response e.g const response = await authenticator.authenticate(...); and then this response can be returned from the action. On any error from the form strategy, this error will be thrown because no failiureRedirect has been supplied

e.g I would expect this code to work:

export async function action({ request }: ActionArgs) {
  // we call the method with the name of the strategy we want to use and the
  // request object, optionally we pass an object with the URLs we want the user
  // to be redirected to after a success or a failure
  try {
   return await authenticator.authenticate("user-pass", request, {
      successRedirect: "/dashboard",
    });
  } catch (err: any) {
    return json({ error: "invalid" }, { status: 401 });
  }
}

Screenshots or Videos

No response

Platform

OS: Mac

Additional context

No response

This is the expected behavior, in Remix a response can be returned or throw, for non-redirect responses there's a difference but in the redirect case Remix will just follow the redirect so return redirect(path) and throw redirect(path) is the same for Remix.

On the app code tho, a throw ensure the rest of the code doesn't continue to happen, which is what we want if you set successRedirect or failureRedirect.

With successRedirect configured, you're saying "if the user is authenticated, redirect here". With failureRedirect configured, you're saying "if the user is not authenticated, redirect here".

To ensure redirects are followed Remix Utils throw them, this way you don't need to do anything special to send the redirect, for example you could build a /login route that only allows non-authenticated requests.

export async function loader({ request }: DataFunctionArgs) {
  await auth.isAuthenticated(request, { successRedirect: "/dashboard" })
  // from this point, the user is not authenticated
}

Or you could create a /dashboard route that only allows authenticated requests

export async function loader({ request }: DataFunctionArgs) {
  let user = await auth.isAuthenticated(request, { failureRedirect: "/dashboard" })
  // from this point, the user is authenticated and the data is in `user`
}

Or you could have a route that checks both, usually set for the OAuth2 callback routes:

export async function loader({ request }: DataFunctionArgs) {
  // the return here is because of TS
  return await auth.isAuthenticated(request, {
    successRedirect: "/dashboard",
    failureRedirect: "/dashboard"
  })
}

If you want to manually do the redirect in case the user is authenticated, don't use successRedirect and instead do the redirect yourself.

export async function loader({ request }: DataFunctionArgs) {
  let user = await auth.isAuthenticated(request);
  if (user) throw redirect("/dashboard") // or return redirect("/dashboard")
  // from this point, the user is not authenticated
}

If you want to handle errors and redirects, you can do a conditions with instanceof.

export async function action({ request }: DataFunctionArgs) {
  try {
   return await auth.authenticate("strategy", request, {
      successRedirect: "/dashboard",
      throwOnError: true,
    });
  } catch (exception: unknown) {
    if (exception instanceof Response) throw exception; // re-throw the redirect
    // handle errors here
    return json({ error: "invalid" }, { status: 401 });
  }
}

I think this is the specific throw statement being discussed. Is that correct?

https://github.com/sergiodxa/remix-auth/blob/05ef79fa0e685c38f826307382200de291919089/src/strategy.ts#L167

I guess I'm a little confused as to why one would not just return the redirect rather than throw it. According to mdn, the throwstatement is intended for a user-defined exception, and this seems to be the opposite of an exception.

Additionally, I think this also means that, if wrapping authenticate in a try/catch, as described in the docs re. error handling, that the function will throw regardless of failure or success. At least that's been my experience and it's been the source of quite a bit of confusion.

Personally, I'd prefer to not use an exception or a catch block as an expected control flow. But maybe I'm the exception, ha ha.

Anyway, imo, it would seem to make more sense to use return rather than throwfor login success.

@andersr in Remix you can throw a redirect to stop executing the loader/action and Remix will follow that redirect, if I returned a redirect response you will be forced to always return the value, so if you wanted to do something like this:

let user = await auth.authenticate("strategy", request, { failureRedirect: "/login" })

Now user would be User | Response and you will need to manually check the type and return the response

if (user instanceof Response) return user;
// here user is a User

So, Remix Auth uses this Remix behavior and throw redirect responses.

@andersr in Remix you can throw a redirect to stop executing the loader/action and Remix will follow that redirect, if I returned a redirect response you will be forced to always return the value, so if you wanted to do something like this:
let user = await auth.authenticate("strategy", request, { failureRedirect: "/login" })
Now user would be User | Response and you will need to manually check the type and return the response
if (user instanceof Response) return user;
// here user is a User
So, Remix Auth uses this Remix behavior and throw redirect responses.

Maybe, but if successRedirect is provided to authenticate you could assume they don't want the user object (as is done by throwing an exception anyway.) The way it would work would be if successRedirect is provided, response is returned, otherwise the user.

=> T extends { successRedirect:string } ? RedirectResponse: User

I get why its written the way it has been now and understand what's happening - still a bit bit of an eyesore either way if your strategies throw any exceptions.

I guess in my case it would be better to handle everything manually without any try/catch and manually return a response if no user is found.

sergiodxa / remix-auth