-
-
Notifications
You must be signed in to change notification settings - Fork 110
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
Authenticator 'authenticate' throws error with only successRedirect passed #233
Comments
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 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 With 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 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 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 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 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? Line 167 in 05ef79f
I guess I'm a little confused as to why one would not just Additionally, I think this also means that, if wrapping Personally, I'd prefer to not use an exception or a Anyway, imo, it would seem to make more sense to use |
@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 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
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. |
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
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.gconst 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 nofailiureRedirect
has been suppliede.g I would expect this code to work:
Screenshots or Videos
No response
Platform
Additional context
No response
The text was updated successfully, but these errors were encountered: