From b8b4ab98c16b8168b8bd0a1c3298622e9f82ce23 Mon Sep 17 00:00:00 2001 From: Konstantin Wohlwend Date: Thu, 5 Dec 2024 19:54:59 -0800 Subject: [PATCH] Neon OAuth "Create new project" flow --- .../oauth/callback/[provider_id]/route.tsx | 2 +- .../app/api/v1/auth/oauth/oauth-helpers.tsx | 6 +- .../neon/internal/confirm/route.tsx | 2 +- .../neon/oauth/idp/[[...route]]/idp.ts | 55 +++- .../neon/oauth/idp/[[...route]]/route.tsx | 6 +- apps/backend/src/middleware.tsx | 4 +- .../src/route-handlers/redirect-handler.tsx | 5 +- .../src/route-handlers/smart-response.tsx | 7 +- .../route-handlers/smart-route-handler.tsx | 3 + .../new-project/page-client.tsx | 11 +- .../neon/confirm/neon-confirm-card.tsx | 21 +- apps/e2e/tests/backend/backend-helpers.ts | 24 +- .../integrations/{ => neon}/api-keys.test.ts | 47 ++- .../{ => neon}/oauth-providers.test.ts | 4 +- .../api/v1/integrations/neon/oauth.test.ts | 268 ++++++++++++++++++ .../api/v1/internal/api-keys.test.ts | 45 ++- apps/e2e/tests/helpers.ts | 22 +- apps/e2e/tests/snapshot-serializer.ts | 76 +++-- packages/stack-shared/src/utils/bytes.tsx | 10 + 19 files changed, 537 insertions(+), 81 deletions(-) rename apps/e2e/tests/backend/endpoints/api/v1/integrations/{ => neon}/api-keys.test.ts (83%) rename apps/e2e/tests/backend/endpoints/api/v1/integrations/{ => neon}/oauth-providers.test.ts (98%) create mode 100644 apps/e2e/tests/backend/endpoints/api/v1/integrations/neon/oauth.test.ts diff --git a/apps/backend/src/app/api/v1/auth/oauth/callback/[provider_id]/route.tsx b/apps/backend/src/app/api/v1/auth/oauth/callback/[provider_id]/route.tsx index cfc1ef910..7b4f8a84f 100644 --- a/apps/backend/src/app/api/v1/auth/oauth/callback/[provider_id]/route.tsx +++ b/apps/backend/src/app/api/v1/auth/oauth/callback/[provider_id]/route.tsx @@ -40,7 +40,7 @@ const handler = createSmartRouteHandler({ body: yupMixed().optional(), }), response: yupObject({ - statusCode: yupNumber().oneOf([302]).defined(), + statusCode: yupNumber().oneOf([307]).defined(), bodyType: yupString().oneOf(["json"]).defined(), body: yupMixed().defined(), headers: yupMixed().defined(), diff --git a/apps/backend/src/app/api/v1/auth/oauth/oauth-helpers.tsx b/apps/backend/src/app/api/v1/auth/oauth/oauth-helpers.tsx index ffcc65652..3c5a48fa9 100644 --- a/apps/backend/src/app/api/v1/auth/oauth/oauth-helpers.tsx +++ b/apps/backend/src/app/api/v1/auth/oauth/oauth-helpers.tsx @@ -9,7 +9,11 @@ export function oauthResponseToSmartResponse(oauthResponse: OAuthResponse): Smar throw new StackAssertionError(`OAuth server error: ${JSON.stringify(oauthResponse.body)}`, { oauthResponse }); } else if (oauthResponse.status >= 200 && oauthResponse.status < 500) { return { - statusCode: oauthResponse.status, + statusCode: { + // our API never returns 301 or 302 by convention, so transform them to 307 or 308 + 301: 308, + 302: 307, + }[oauthResponse.status] ?? oauthResponse.status, bodyType: "json", body: oauthResponse.body, headers: Object.fromEntries(Object.entries(oauthResponse.headers || {}).map(([k, v]) => ([k, [v]]))), diff --git a/apps/backend/src/app/api/v1/integrations/neon/internal/confirm/route.tsx b/apps/backend/src/app/api/v1/integrations/neon/internal/confirm/route.tsx index b99568842..d9edcb0dd 100644 --- a/apps/backend/src/app/api/v1/integrations/neon/internal/confirm/route.tsx +++ b/apps/backend/src/app/api/v1/integrations/neon/internal/confirm/route.tsx @@ -32,7 +32,7 @@ export const POST = createSmartRouteHandler({ const set = await prismaClient.apiKeySet.create({ data: { projectId: req.body.project_id, - description: "API key for Neon x Stack Auth integration", + description: "Auto-generated for Neon", expiresAt: new Date(Date.now() + 1000 * 60 * 60 * 24 * 365 * 100), superSecretAdminKey: `sak_${generateSecureRandomString()}`, }, diff --git a/apps/backend/src/app/api/v1/integrations/neon/oauth/idp/[[...route]]/idp.ts b/apps/backend/src/app/api/v1/integrations/neon/oauth/idp/[[...route]]/idp.ts index 0cdf2bab6..67f317df4 100644 --- a/apps/backend/src/app/api/v1/integrations/neon/oauth/idp/[[...route]]/idp.ts +++ b/apps/backend/src/app/api/v1/integrations/neon/oauth/idp/[[...route]]/idp.ts @@ -1,9 +1,11 @@ import { prismaClient } from '@/prisma-client'; import { Prisma } from '@prisma/client'; +import { decodeBase64OrBase64Url } from '@stackframe/stack-shared/dist/utils/bytes'; import { getEnvVariable } from '@stackframe/stack-shared/dist/utils/env'; import { StackAssertionError, captureError, throwErr } from '@stackframe/stack-shared/dist/utils/errors'; import { sha512 } from '@stackframe/stack-shared/dist/utils/hashes'; import { getPerAudienceSecret, getPrivateJwk, getPublicJwkSet } from '@stackframe/stack-shared/dist/utils/jwt'; +import { deindent } from '@stackframe/stack-shared/dist/utils/strings'; import { generateUuid } from '@stackframe/stack-shared/dist/utils/uuids'; import Provider, { Adapter, AdapterConstructor, AdapterPayload } from 'oidc-provider'; @@ -45,17 +47,20 @@ function createAdapter(options: { constructor(model: string) { this.model = model; + if (!model) { + throw new StackAssertionError(deindent` + model must be non-empty. + + oidc-provider should never call the constructor with an empty string. However, it relies on 'constructor.name' in some locations, causing it to fail when class name minification is enabled. Make sure that server-side class names are not minified, for example by disabling serverMinification in next.config.mjs. + `); + } } async upsert(id: string, payload: AdapterPayload, expiresInSeconds: number): Promise { - try { - if (expiresInSeconds < 0) throw new StackAssertionError(`expiresInSeconds of ${this.model}:${id} must be non-negative, got ${expiresInSeconds}`, { expiresInSeconds, model: this.model, id, payload }); - if (expiresInSeconds > 60 * 60 * 24 * 365 * 100) throw new StackAssertionError(`expiresInSeconds of ${this.model}:${id} must be less than 100 years, got ${expiresInSeconds}`, { expiresInSeconds, model: this.model, id, payload }); - if (!Number.isFinite(expiresInSeconds)) throw new StackAssertionError(`expiresInSeconds of ${this.model}:${id} must be a finite number, got ${expiresInSeconds}`, { expiresInSeconds, model: this.model, id, payload }); - } catch (err) { - captureError('idp-adapter-upsert-assertion-error', err); - expiresInSeconds = 60 * 60 * 60 * 24; - } + // if one of these assertions is triggered, make sure you're not minifying class names (see the constructor) + if (expiresInSeconds < 0) throw new StackAssertionError(`expiresInSeconds of ${this.model}:${id} must be non-negative, got ${expiresInSeconds}`, { expiresInSeconds, model: this.model, id, payload }); + if (expiresInSeconds > 60 * 60 * 24 * 365 * 100) throw new StackAssertionError(`expiresInSeconds of ${this.model}:${id} must be less than 100 years, got ${expiresInSeconds}`, { expiresInSeconds, model: this.model, id, payload }); + if (!Number.isFinite(expiresInSeconds)) throw new StackAssertionError(`expiresInSeconds of ${this.model}:${id} must be a finite number, got ${expiresInSeconds}`, { expiresInSeconds, model: this.model, id, payload }); await niceUpdate(this.model, id, () => ({ payload, expiresAt: new Date(Date.now() + expiresInSeconds * 1000) })); } @@ -236,6 +241,16 @@ export async function createOidcProvider(options: { id: string, baseUrl: string }); } + // Log all errors + middleware(async (ctx, next) => { + try { + return await next(); + } catch (e) { + console.warn("IdP threw an error. This most likely indicates a misconfigured client, not a server error.", e, { path: ctx.path, ctx }); + throw e; + } + }); + // .well-known/jwks.json middleware(async (ctx, next) => { if (ctx.path === '/.well-known/jwks.json') { @@ -339,9 +354,33 @@ export async function createOidcProvider(options: { id: string, baseUrl: string } } } else if (ctx.method === 'GET' && /^\/interaction\/[^/]+$/.test(ctx.path)) { + const details = await oidc.interactionDetails(ctx.req, ctx.res); + + const state = details.params.state || ""; + if (typeof state !== 'string') { + throwErr(`state is not a string`); + } + let neonProjectDisplayName: string | undefined; + try { + const base64Decoded = new TextDecoder().decode(decodeBase64OrBase64Url(state)); + const json = JSON.parse(base64Decoded); + neonProjectDisplayName = json?.details?.neon_project_name; + if (typeof neonProjectDisplayName !== 'string') { + throwErr(`neon_project_name is not a string`, { type: typeof neonProjectDisplayName, neonProjectDisplayName }); + } + } catch (e) { + // this probably shouldn't happen, because it means Neon messed up the configuration + // (or maybe someone is playing with the API, but in that case it's not a bad idea to notify us either) + // either way, let's capture an error and continue without the display name + captureError('idp-oidc-provider-interaction-state-decode-error', e); + } + const uid = ctx.path.split('/')[2]; const interactionUrl = new URL(`/integrations/neon/confirm`, getEnvVariable("NEXT_PUBLIC_STACK_DASHBOARD_URL")); interactionUrl.searchParams.set("interaction_uid", uid); + if (neonProjectDisplayName) { + interactionUrl.searchParams.set("neon_project_display_name", neonProjectDisplayName); + } return ctx.redirect(interactionUrl.toString()); } await next(); diff --git a/apps/backend/src/app/api/v1/integrations/neon/oauth/idp/[[...route]]/route.tsx b/apps/backend/src/app/api/v1/integrations/neon/oauth/idp/[[...route]]/route.tsx index 5ec67e5c9..2ea3a7792 100644 --- a/apps/backend/src/app/api/v1/integrations/neon/oauth/idp/[[...route]]/route.tsx +++ b/apps/backend/src/app/api/v1/integrations/neon/oauth/idp/[[...route]]/route.tsx @@ -47,7 +47,11 @@ const handler = handleApiRequest(async (req: NextRequest) => { return new NextResponse(body, { headers: Object.entries(serverResponse.getHeaders()).filter(([k, v]) => v) as any, - status: serverResponse.statusCode, + status: { + // our API never returns 301 or 302 by convention, so transform them to 307 or 308 + 301: 308, + 302: 307, + }[serverResponse.statusCode] ?? serverResponse.statusCode, statusText: serverResponse.statusMessage, }); }); diff --git a/apps/backend/src/middleware.tsx b/apps/backend/src/middleware.tsx index ce18cd445..0c1d970e4 100644 --- a/apps/backend/src/middleware.tsx +++ b/apps/backend/src/middleware.tsx @@ -3,8 +3,8 @@ import { StackAssertionError } from '@stackframe/stack-shared/dist/utils/errors' import { wait } from '@stackframe/stack-shared/dist/utils/promises'; import './polyfills'; -import { NextResponse } from 'next/server'; import type { NextRequest } from 'next/server'; +import { NextResponse } from 'next/server'; const corsAllowedRequestHeaders = [ // General @@ -43,7 +43,7 @@ export async function middleware(request: NextRequest) { const delay = +getEnvVariable('STACK_ARTIFICIAL_DEVELOPMENT_DELAY_MS', '0'); if (delay) { if (getNodeEnvironment().includes('production')) { - throw new StackAssertionError('STACK_ARTIFICIAL_DEVELOPMENT_DELAY_MS is only allowed in development'); + throw new StackAssertionError('STACK_ARTIFICIAL_DEVELOPMENT_DELAY_MS environment variable is only allowed in development'); } if (!request.headers.get('x-stack-disable-artificial-development-delay')) { await wait(delay); diff --git a/apps/backend/src/route-handlers/redirect-handler.tsx b/apps/backend/src/route-handlers/redirect-handler.tsx index 90e5e0027..266029e6e 100644 --- a/apps/backend/src/route-handlers/redirect-handler.tsx +++ b/apps/backend/src/route-handlers/redirect-handler.tsx @@ -1,11 +1,10 @@ import "../polyfills"; +import { yupArray, yupNumber, yupObject, yupString } from "@stackframe/stack-shared/dist/schema-fields"; import { NextRequest } from "next/server"; -import * as yup from "yup"; import { createSmartRouteHandler } from "./smart-route-handler"; -import { yupObject, yupString, yupNumber, yupBoolean, yupArray, yupMixed } from "@stackframe/stack-shared/dist/schema-fields"; -export function redirectHandler(redirectPath: string, statusCode: 301 | 302 | 303 | 307 | 308 = 307): (req: NextRequest, options: any) => Promise { +export function redirectHandler(redirectPath: string, statusCode: 303 | 307 | 308 = 307): (req: NextRequest, options: any) => Promise { return createSmartRouteHandler({ request: yupObject({ url: yupString().defined(), diff --git a/apps/backend/src/route-handlers/smart-response.tsx b/apps/backend/src/route-handlers/smart-response.tsx index 15391a73d..26d6568b9 100644 --- a/apps/backend/src/route-handlers/smart-response.tsx +++ b/apps/backend/src/route-handlers/smart-response.tsx @@ -1,11 +1,10 @@ import "../polyfills"; +import { StackAssertionError } from "@stackframe/stack-shared/dist/utils/errors"; +import { Json } from "@stackframe/stack-shared/dist/utils/json"; +import { deepPlainEquals } from "@stackframe/stack-shared/dist/utils/objects"; import { NextRequest } from "next/server"; import * as yup from "yup"; -import { Json } from "@stackframe/stack-shared/dist/utils/json"; -import { StackAssertionError } from "@stackframe/stack-shared/dist/utils/errors"; -import { deepPlainEquals } from "@stackframe/stack-shared/dist/utils/objects"; -import { yupObject } from "@stackframe/stack-shared/dist/schema-fields"; export type SmartResponse = { statusCode: number, diff --git a/apps/backend/src/route-handlers/smart-route-handler.tsx b/apps/backend/src/route-handlers/smart-route-handler.tsx index 55a634601..9f5dc66a7 100644 --- a/apps/backend/src/route-handlers/smart-route-handler.tsx +++ b/apps/backend/src/route-handlers/smart-route-handler.tsx @@ -82,6 +82,9 @@ export function handleApiRequest(handler: (req: NextRequest, options: any, reque const timeStart = performance.now(); const res = await handler(req, options, requestId); const time = (performance.now() - timeStart); + if ([301, 302].includes(res.status)) { + throw new StackAssertionError("HTTP status codes 301 and 302 should not be returned by our APIs because the behavior for non-GET methods is inconsistent across implementations. Use 303 (to rewrite method to GET) or 307/308 (to preserve the original method and data) instead.", { status: res.status, url: req.nextUrl, req, res }); + } console.log(`[ RES] [${requestId}] ${req.method} ${censoredUrl} (in ${time.toFixed(0)}ms)`); return res; } catch (e) { diff --git a/apps/dashboard/src/app/(main)/(protected)/(outside-dashboard)/new-project/page-client.tsx b/apps/dashboard/src/app/(main)/(protected)/(outside-dashboard)/new-project/page-client.tsx index d80ddf19c..7f5816818 100644 --- a/apps/dashboard/src/app/(main)/(protected)/(outside-dashboard)/new-project/page-client.tsx +++ b/apps/dashboard/src/app/(main)/(protected)/(outside-dashboard)/new-project/page-client.tsx @@ -6,6 +6,7 @@ import { AuthPage, useUser } from "@stackframe/stack"; import { allProviders } from "@stackframe/stack-shared/dist/utils/oauth"; import { runAsynchronouslyWithAlert, wait } from "@stackframe/stack-shared/dist/utils/promises"; import { BrowserFrame, Button, Form, Separator, Typography } from "@stackframe/stack-ui"; +import { useSearchParams } from "next/navigation"; import { useState } from "react"; import { useForm } from "react-hook-form"; import * as yup from "yup"; @@ -33,6 +34,7 @@ export default function PageClient () { mode: "onChange", }); const router = useRouter(); + const searchParams = useSearchParams(); const mockProject = { id: "id", @@ -63,7 +65,14 @@ export default function PageClient () { } as const)).filter(({ enabled }) => enabled), } }); - router.push('/projects/' + newProject.id); + const redirectToNeonConfirmWith = searchParams.get("redirect_to_neon_confirm_with"); + if (redirectToNeonConfirmWith) { + const confirmSearchParams = new URLSearchParams(redirectToNeonConfirmWith); + confirmSearchParams.set("default_selected_project_id", newProject.id); + router.push('/integrations/neon/confirm?' + confirmSearchParams.toString()); + } else { + router.push('/projects/' + encodeURIComponent(newProject.id)); + } await wait(2000); } finally { setLoading(false); diff --git a/apps/dashboard/src/app/(main)/integrations/neon/confirm/neon-confirm-card.tsx b/apps/dashboard/src/app/(main)/integrations/neon/confirm/neon-confirm-card.tsx index 9c23857c0..81be42c0a 100644 --- a/apps/dashboard/src/app/(main)/integrations/neon/confirm/neon-confirm-card.tsx +++ b/apps/dashboard/src/app/(main)/integrations/neon/confirm/neon-confirm-card.tsx @@ -11,12 +11,12 @@ import NeonLogo from "../../../../../../public/neon.png"; export default function NeonConfirmCard(props: { onContinue: (options: { projectId: string }) => Promise<{ error: string } | undefined> }) { const user = useUser({ or: "redirect", projectIdMustMatch: "internal" }); const projects = user.useOwnedProjects(); - - const [selectedProject, setSelectedProject] = useState(null); - const router = useRouter(); const searchParams = useSearchParams(); + const [selectedProject, setSelectedProject] = useState(projects.find((project) => project.id === searchParams.get("default_selected_project_id")) ?? null); + + return ( @@ -62,12 +62,20 @@ export default function NeonConfirmCard(props: { onContinue: (options: { project Which projects would you like to connect? - } value={searchParams.get("neon_project_name") || "Neon project connected!"} /> + } value={searchParams.get("neon_project_display_name") || "Neon project connected!"} />
- { + if (p === "create-new") { + const createSearchParams = new URLSearchParams(); + createSearchParams.set("redirect_to_neon_confirm_with", searchParams.toString()); + window.location.href = '/new-project?' + createSearchParams.toString(); + } else { + setSelectedProject(projects.find((project) => project.id === p) ?? null); + } + }}> {selectedProject && ( @@ -84,6 +92,9 @@ export default function NeonConfirmCard(props: { onContinue: (options: { project {project.displayName} {(project.id)} ))} + + Create new Stack project... + diff --git a/apps/e2e/tests/backend/backend-helpers.ts b/apps/e2e/tests/backend/backend-helpers.ts index 4a841f287..1eb684cad 100644 --- a/apps/e2e/tests/backend/backend-helpers.ts +++ b/apps/e2e/tests/backend/backend-helpers.ts @@ -144,7 +144,7 @@ export namespace Auth { if (response.status !== 200) { throw new StackAssertionError("Expected session to be valid, but was actually invalid.", { response }); } - expect(response).toEqual({ + expect(response).toMatchObject({ status: 200, headers: expect.objectContaining({}), body: expect.objectContaining({}), @@ -535,7 +535,7 @@ export namespace Auth { const redirectResponse1 = await niceFetch(authLocation, { redirect: "manual", }); - expect(redirectResponse1).toEqual({ + expect(redirectResponse1).toMatchObject({ status: 303, headers: expect.any(Headers), body: expect.any(String), @@ -555,7 +555,7 @@ export namespace Auth { cookie: signInInteractionCookies, }, }); - expect(response1).toEqual({ + expect(response1).toMatchObject({ status: 303, headers: expect.any(Headers), body: expect.any(ArrayBuffer), @@ -566,7 +566,7 @@ export namespace Auth { cookie: updateCookiesFromResponse(signInInteractionCookies, response1), }, }); - expect(redirectResponse2).toEqual({ + expect(redirectResponse2).toMatchObject({ status: 303, headers: expect.any(Headers), body: expect.any(String), @@ -584,7 +584,7 @@ export namespace Auth { cookie: authorizeInteractionCookies, }, }); - expect(response2).toEqual({ + expect(response2).toMatchObject({ status: 303, headers: expect.any(Headers), body: expect.any(ArrayBuffer), @@ -595,7 +595,7 @@ export namespace Auth { cookie: updateCookiesFromResponse(authorizeInteractionCookies, response2), }, }); - expect(redirectResponse3).toEqual({ + expect(redirectResponse3).toMatchObject({ status: 303, headers: expect.any(Headers), body: expect.any(String), @@ -618,8 +618,8 @@ export namespace Auth { cookie, }, }); - expect(response).toEqual({ - status: 302, + expect(response).toMatchObject({ + status: 307, headers: expect.any(Headers), body: {}, }); @@ -820,6 +820,14 @@ export namespace ApiKey { backendContext.set({ projectKeys: res.projectKeys }); return res; } + + export async function listAll() { + const response = await niceBackendFetch("/api/v1/internal/api-keys", { + accessType: "admin", + }); + expect(response.status).toBe(200); + return response.body; + } } export namespace Project { diff --git a/apps/e2e/tests/backend/endpoints/api/v1/integrations/api-keys.test.ts b/apps/e2e/tests/backend/endpoints/api/v1/integrations/neon/api-keys.test.ts similarity index 83% rename from apps/e2e/tests/backend/endpoints/api/v1/integrations/api-keys.test.ts rename to apps/e2e/tests/backend/endpoints/api/v1/integrations/neon/api-keys.test.ts index a216b64e6..cb0b4568c 100644 --- a/apps/e2e/tests/backend/endpoints/api/v1/integrations/api-keys.test.ts +++ b/apps/e2e/tests/backend/endpoints/api/v1/integrations/neon/api-keys.test.ts @@ -1,6 +1,6 @@ import { describe } from "vitest"; -import { it } from "../../../../../helpers"; -import { ApiKey, Auth, Project, backendContext, niceBackendFetch } from "../../../../backend-helpers"; +import { it } from "../../../../../../helpers"; +import { ApiKey, Auth, Project, backendContext, niceBackendFetch } from "../../../../../backend-helpers"; describe("without project access", () => { @@ -27,6 +27,29 @@ describe("without project access", () => { }); }); +describe("with server access", () => { + it("should not have access to api keys", async ({ expect }) => { + const response = await niceBackendFetch("/api/v1/integrations/neon/api-keys", { accessType: "server" }); + expect(response).toMatchInlineSnapshot(` + NiceResponse { + "status": 401, + "body": { + "code": "INSUFFICIENT_ACCESS_TYPE", + "details": { + "actual_access_type": "server", + "allowed_access_types": ["admin"], + }, + "error": "The x-stack-access-type header must be 'admin', but was 'server'.", + }, + "headers": Headers { + "x-stack-known-error": "INSUFFICIENT_ACCESS_TYPE", +