-
Notifications
You must be signed in to change notification settings - Fork 962
feat(auth): migrate from mcp() to oauthProvider() for OAuth 2.1 #1324
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| import { oauthProviderAuthServerMetadata } from "@better-auth/oauth-provider"; | ||
| import { auth } from "@superset/auth/server"; | ||
|
|
||
| export const GET = oauthProviderAuthServerMetadata(auth); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { oauthProviderAuthServerMetadata } from "@better-auth/oauth-provider"; | ||
| import { auth } from "@superset/auth/server"; | ||
| import { oAuthDiscoveryMetadata } from "better-auth/plugins"; | ||
|
|
||
| export const GET = oAuthDiscoveryMetadata(auth); | ||
| export const GET = oauthProviderAuthServerMetadata(auth); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| import { protectedResourceHandler } from "mcp-handler"; | ||
| import { env } from "@/env"; | ||
|
|
||
| export const GET = protectedResourceHandler({ | ||
| authServerUrls: [env.NEXT_PUBLIC_API_URL], | ||
| resourceUrl: env.NEXT_PUBLIC_API_URL, | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,7 @@ | ||
| import { auth } from "@superset/auth/server"; | ||
| import { oAuthProtectedResourceMetadata } from "better-auth/plugins"; | ||
| import { protectedResourceHandler } from "mcp-handler"; | ||
| import { env } from "@/env"; | ||
|
|
||
| export const GET = oAuthProtectedResourceMetadata(auth); | ||
| export const GET = protectedResourceHandler({ | ||
| authServerUrls: [env.NEXT_PUBLIC_API_URL], | ||
| resourceUrl: env.NEXT_PUBLIC_API_URL, | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| import { oauthProviderOpenIdConfigMetadata } from "@better-auth/oauth-provider"; | ||
| import { auth } from "@superset/auth/server"; | ||
|
|
||
| export const GET = oauthProviderOpenIdConfigMetadata(auth); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| import { oauthProviderOpenIdConfigMetadata } from "@better-auth/oauth-provider"; | ||
| import { auth } from "@superset/auth/server"; | ||
|
|
||
| export const GET = oauthProviderOpenIdConfigMetadata(auth); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "use client"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { authClient } from "@superset/auth/client"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Button } from "@superset/ui/button"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Select, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -24,7 +25,6 @@ interface Organization { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| interface ConsentFormProps { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| consentCode: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clientId: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clientName?: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| scopes: string[]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -56,7 +56,6 @@ const SCOPE_DESCRIPTIONS: Record< | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function ConsentForm({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| consentCode, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clientId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clientName, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| scopes, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -74,63 +73,41 @@ export function ConsentForm({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const selectedOrg = organizations.find((o) => o.id === selectedOrgId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const handleConsent = async (accept: boolean) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (accept && !selectedOrgId) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setError("Please select an organization"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setIsLoading(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setError(null); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Add organization scope to verification value before consent | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // (Better Auth's consent endpoint doesn't accept scope in body) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (accept && selectedOrgId) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const addScopeResponse = await fetch( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| `${process.env.NEXT_PUBLIC_API_URL}/api/auth/oauth/add-org-scope`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| method: "POST", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| headers: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "Content-Type": "application/json", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| credentials: "include", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| body: JSON.stringify({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| consent_code: consentCode, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| organizationId: selectedOrgId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!addScopeResponse.ok) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const data = await addScopeResponse.json(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error(data.error || "Failed to set organization scope"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (accept) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { error: setActiveError } = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await authClient.organization.setActive({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| organizationId: selectedOrgId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (setActiveError) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setActiveError.message ?? "Failed to set organization", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const response = await fetch( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| `${process.env.NEXT_PUBLIC_API_URL}/api/auth/oauth2/consent`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| method: "POST", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| headers: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "Content-Type": "application/json", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| credentials: "include", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| body: JSON.stringify({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| accept, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| consent_code: consentCode, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const data = await response.json(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { data, error: consentError } = await authClient.oauth2.consent({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| accept, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| scope: accept ? scopes.join(" ") : undefined, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!response.ok) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| data.error_description || data.message || "Failed to process consent", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (consentError) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error(consentError.message ?? "Failed to process consent"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const redirectUrl = data.redirectURI || data.redirectTo; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (redirectUrl) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| window.location.href = redirectUrl; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (data?.uri) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| window.location.href = data.uri; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+97
to
108
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing If Suggested fix if (data?.uri) {
window.location.href = data.uri;
+ } else {
+ setError("No redirect URI received from server");
+ setIsLoading(false);
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.error("Consent error:", err); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.error("[oauth/consent] Error:", err); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setError(err instanceof Error ? err.message : "An error occurred"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setIsLoading(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,11 +9,7 @@ import { api } from "@/trpc/server"; | |
| import { ConsentForm } from "./components/ConsentForm"; | ||
|
|
||
| interface ConsentPageProps { | ||
| searchParams: Promise<{ | ||
| consent_code?: string; | ||
| client_id?: string; | ||
| scope?: string; | ||
| }>; | ||
| searchParams: Promise<Record<string, string>>; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: cat -n apps/web/src/app/oauth/consent/page.tsx | head -100Repository: superset-sh/superset Length of output: 2984 🌐 Web query:
💡 Result: In Next.js 15 (App Router), the export default async function Page({
params,
searchParams,
}: {
params: Promise<{ slug: string }>
searchParams: Promise<{ [key: string]: string | string[] | undefined }>
}) {
const { slug } = await params
const { q } = await searchParams
}
``` ([nextjs.org](https://nextjs.org/docs/app/api-reference/file-conventions/page))
If you want a **generated, route-aware type** (strict `params` keys, typed from the route literal), use the **global** helper:
```ts
export default async function Page(props: PageProps<'/blog/[slug]'>) {
const { slug } = await props.params
const query = await props.searchParams
}
``` ([nextjs.org](https://nextjs.org/docs/app/api-reference/file-conventions/page))
Docs note: in Next.js 15, `params` (Page/Layout) and `searchParams` (Page) are Promises. ([nextjs.org](https://nextjs.org/docs/messages/next-prerender-sync-params))
Citations:
- 1: https://nextjs.org/docs/app/api-reference/file-conventions/page
- 2: https://nextjs.org/docs/app/api-reference/file-conventions/page
- 3: https://nextjs.org/docs/messages/next-prerender-sync-params
---
</details>
**`searchParams` type is narrower than Next.js 15 actually provides.**
Next.js 15 App Router pages receive `searchParams` typed as `Promise<Record<string, string | string[] | undefined>>`. Using `Promise<Record<string, string>>` silently discards the possibility of array values (e.g., `?scope=a&scope=b`) and `undefined`. This can lead to runtime errors if a query param is repeated or absent — `params.client_id` is typed as `string` but could be `string[]` or `undefined`. Line 58's `scope?.split(" ")` would throw if `scope` were an array instead of a string.
<details>
<summary>Suggested fix</summary>
```diff
interface ConsentPageProps {
- searchParams: Promise<Record<string, string>>;
+ searchParams: Promise<Record<string, string | string[] | undefined>>;
}Then when extracting values, coerce to string: const clientId = typeof params.client_id === "string" ? params.client_id : undefined;
const scope = typeof params.scope === "string" ? params.scope : undefined;🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| export default async function ConsentPage({ searchParams }: ConsentPageProps) { | ||
|
|
@@ -23,13 +19,14 @@ export default async function ConsentPage({ searchParams }: ConsentPageProps) { | |
|
|
||
| if (!session) { | ||
| const params = await searchParams; | ||
| const returnUrl = `/oauth/consent?${new URLSearchParams(params as Record<string, string>).toString()}`; | ||
| const returnUrl = `/oauth/consent?${new URLSearchParams(params).toString()}`; | ||
| redirect(`/sign-in?redirect=${encodeURIComponent(returnUrl)}`); | ||
| } | ||
|
|
||
| const { consent_code, client_id, scope } = await searchParams; | ||
| const params = await searchParams; | ||
| const { client_id, scope } = params; | ||
|
|
||
| if (!consent_code || !client_id) { | ||
| if (!client_id) { | ||
| return ( | ||
| <div className="relative flex min-h-screen flex-col"> | ||
| <header className="container mx-auto px-6 py-6"> | ||
|
|
@@ -64,7 +61,7 @@ export default async function ConsentPage({ searchParams }: ConsentPageProps) { | |
| const trpc = await api(); | ||
| const userOrganizations = await trpc.user.myOrganizations.query(); | ||
|
|
||
| const oauthApp = await db.query.oauthApplications.findFirst({ | ||
| const oauthApp = await db.query.oauthClients.findFirst({ | ||
| where: (table, { eq }) => eq(table.clientId, client_id), | ||
| columns: { name: true }, | ||
| }); | ||
|
|
@@ -90,7 +87,6 @@ export default async function ConsentPage({ searchParams }: ConsentPageProps) { | |
| </header> | ||
| <main className="flex flex-1 items-center justify-center"> | ||
| <ConsentForm | ||
| consentCode={consent_code} | ||
| clientId={client_id} | ||
| clientName={oauthApp?.name ?? undefined} | ||
| scopes={scopes} | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
authClient.organization.setActiveis called before consent — verify this doesn't persist the org switch if the user later abandons or the consent call fails.If
setActivesucceeds but the subsequentconsentcall throws, the user's active organization has already been changed server-side. On retry or navigating away, they may find themselves in a different org than expected. Consider callingsetActiveonly after consent succeeds, or rolling back on failure.🤖 Prompt for AI Agents