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

Add ability to specify OAuth endpoints #1136

Merged
merged 2 commits into from
Dec 4, 2024
Merged

Conversation

scotttrinh
Copy link
Collaborator

If your database is deployed to a private network and is not available at a public URL, this adds the ability to drive the whole flow from your application server, delegating to the auth server from your application server.


When testing this manually, I set up a new route handler at /auth/oauth that shadows the one built into our createAuthRouteHandlers behavior like this:

export async function GET(request: NextRequest): Promise<NextResponse> {
  const callbackUrl = new URL("auth/server/oauth/callback", authConfig.baseUrl);
  const authorizeUrl = new URL(
    "auth/server/oauth/authorize",
    authConfig.baseUrl,
  );
  request.nextUrl.searchParams.set("callback_url", callbackUrl.toString());
  request.nextUrl.searchParams.set("authorize_url", authorizeUrl.toString());

  return await auth.oAuth.handleOAuth(request);
}

Which set the authorize and callback URLs explicitly to routes in my application, and then those route handlers simply called through to these new methods like:

//  app/auth/server/oauth/authorize.ts
export async function GET(request: NextRequest): Promise<NextResponse> {
  return await auth.oAuth.handleAuthorize(request);
}

// app/auth/server/oauth/callback.ts
export async function GET(request: NextRequest): Promise<NextResponse> {
  return await auth.oAuth.handleCallback(request);
}

Of course, instead of just calling this methods, one could do any kind of check here on the request if your intention is to add some behavior here (analytics? logic? check email against an invite list?)

@scotttrinh scotttrinh requested a review from jaclarke November 28, 2024 11:38
redirectToOnSignup: redirectToOnSignup.toString(),
...(callbackUrl ? { callbackUrl } : {}),
});
console.log(`Redirecting to ${location}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log(`Redirecting to ${location}`);

redirectToOnSignup.searchParams.set("isSignUp", "true");

const authorizeUrl =
req.nextUrl.searchParams.get("authorize_url") ??
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this (and callback_url) be args to this handleOAuth function? I'm not sure they are meant to be something the user should be able to control? and in the example usage thay're appended to the req right before it's passed to this function anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API I'm going for here is that these "handler" functions should always take a NextRequest and return a Promise<NextResponse> in a middleware like fashion. An alternative way to do this is to be more explicit about what parameters we explicitly depend on within the function itself, but I'm worried that it adds boilerplate of getting the data we need from the request. I can sketch that out though and see how that feels in actual use though.

Copy link
Member

@jaclarke jaclarke Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, maybe strip out those two values from the search params here: https://github.com/edgedb/edgedb-js/pull/1136/files#diff-660934b707fc817f8bd5cef3341ffb7f24ffc16f9e200843c762e384e2820c41L173 then? I haven't fully thought out the whole flow and there probably isn't an attack you could do using these params, but just to be safe maybe 😄.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, maybe strip out those two values from the search params here: #1136 (files) then?

Maybe I'm not understanding the suggestion but these parameters will be present in the URL that the browser uses as a link, just like provider, and it needs to be passed all the way to the auth extension. It's not something an end user needs to worry about, but the application developer will absolutely need to set this either in the link button's URL, or manually in their own backend like I did in my example.

I haven't fully thought out the whole flow and there probably isn't an attack you could do using these params, but just to be safe

The URLs are checked against the allowed_redirect_urls config, so if there is an attack vector there, the attack also exists in the existing redirect_to search parameters.

If your database is deployed to a private network and is not available
at a public URL, this adds the ability to drive the whole flow from your
application server, delegating to the auth server from your application
server.
@scotttrinh scotttrinh force-pushed the support-oauth-callback-url branch from 6da4603 to abe79e9 Compare December 2, 2024 17:12
@scotttrinh scotttrinh requested a review from jaclarke December 3, 2024 01:35
@scotttrinh scotttrinh merged commit 8a28c30 into master Dec 4, 2024
10 checks passed
@scotttrinh scotttrinh deleted the support-oauth-callback-url branch December 4, 2024 16:04
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

Successfully merging this pull request may close these issues.

2 participants