-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
Custom state object #73
Comments
I think something along these lines would be very useful. I'm using the Auth0 strategy and was puzzled there didn't seem to be a simple way to provide additional state to the authorization URL. I looked through some code and realised it isn't made possible by the OAuth2 strategy itself. I'm not sure if this is a part of the OAuth 2 spec, but Auth0's authorization URL supports it at least. That is, we can append a For example... // app/routes/signin.callback.ts
export const loader = ({ request }: LoaderFunctionArgs) => {
const params = new URLSearchParams(request.url)
return authenticator.authenticate("auth0", request, {
successRedirect: params.get("returnTo") || "/dashboard"
})
} Unless I'm missing something, this could be solved by allowing // app/root.tsx
export const loader = ({ request }: LoaderFunctionArgs) => {
const user = await authenticator.isAuthenticated(request)
if (user) {
return null
} else {
return authenticator.authenticate("auth0", request, {
authorizationParams: {
returnTo: request.url,
}
})
}
} @sergiodxa If this sounds reasonable I'm happy to open a PR. Although, as @aaronadamsCA suggests, having the supplied state be written to and retrieved from the configured session cookie storage automagically would be far superior and more secure |
@lukerollans, I have observed the same Auth0 behaviour you describe. I've also noticed this library already passes through any search params from the request URL to Auth0. So if you wanted to try your approach, I think you already could: const url = new URL(request.url);
url.searchParams.append("returnTo", returnTo);
authenticator.authenticate("auth0", new Request(url, { headers: request.headers })); However AFAIK the only spec-compliant method uses the |
I think it will help to show just how complicated it is to meet the following requirements:
Here's the
|
I'd second this. We're looking to maintain state from our headless shopify registration page so we can capture extra fields, namely first and last name as well as marketing consent. Would be great to see an API like this: // app/routes/login.tsx
import { authenticator } from "~/auth.server";
export async function action({ request }: ActionArgs) {
const url = new URL(request.url);
const returnTo = url.searchParams.get("returnTo") || "/";
const formData = Object.fromEntries(await request.formData());
return authenticator.authenticate('shopify', request, {
failureRedirect: "/login",
// Stored in session storage (coupled with oauth state param)
state: {
returnTo: safeRedirect(new URL(request.url).searchParams.get("returnTo")),
firstName: formData.firstName,
lastName: formData.lastName,
emailMarketingConsent: formData.emailMarketingConsent
}
});
} // app/auth.server.tsx
// ...
authenticator.use(
new OAuth2Strategy({
// ...
}, ({
accessToken,
refreshToken,
extraParams,
profile,
context,
request,
state // <--
}) => {
console.log(state); // { returnTo: '/foo', firstName: 'bar', lastName: 'baz', marketingConsent: false }
})
); |
The That's the only safe way to do it, regardless if other libraries allows it. |
yep, this is understood - I think the point of this (now closed?) issue is to have this functionality available as a part of the library without extra work by the developer. in my initial comment I expressed interest in opening a PR, but I was wary of committing the time and effort if a PR would not be welcome. given you've closed the issue without providing any alternatives, I'm guessing a PR would not be welcome? |
May anyone provide any example on how to do this ? |
@sergiodxa, it is a real bummer to make a request only to have it closed months later for unrelated reasons. Could you please re-read my original request? As it says,
I did my best to explain why this is so difficult to do well right now. Your linked tutorial, for one, results in the very frustrating experience where a user who opens/reopens multiple tabs will see all but one redirect to an incorrect The Passport.js solution to which I linked is secure, since it does exactly what you describe: storing state in the session. If this library offered the same capability as Passport.js:
|
We've had this library implemented for some time, and we continue to encounter challenges with the current recommendation to use session storage to store auth state (like
returnTo
) ourselves:authenticate()
callsgetSession()
, there is a race condition with our owngetSession()
calls that cause new sessions to be lost.returnTo
values to overwrite each other.#73 (comment) includes some messy workarounds for these problems.
Between the above and the additional work to gracefully recover from #67, our auth routes have become quite the mess. 😕
My request is to add support to
authenticate
for an optionalstate
object stored in session storage, plus an optionalsuccessCallback
to get it back.As prior art,
passport-oauth2
has offered basically the same thing since v1.6.0:https://medium.com/passportjs/application-state-in-oauth-2-0-1d94379164e
I think this feature would make this strategy much more robust and easier to use.
I realize feature request #1 was declined at the time, but I'm not asking to change the value of
state
; I just want to store additional auth-related state alongside it in session storage. This would also line up nicely with a potential future fix to #67.The text was updated successfully, but these errors were encountered: