-
Notifications
You must be signed in to change notification settings - Fork 802
fix: make headless api-key auth work end-to-end (CLI + server org resolution) #3493
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
f303c11
a507841
a27eaf9
89e159b
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,93 @@ | ||
| import type { auth as betterAuth } from "@superset/auth/server"; | ||
| import { db } from "@superset/db/client"; | ||
| import { members } from "@superset/db/schema"; | ||
| import { and, eq } from "drizzle-orm"; | ||
|
|
||
| const API_KEY_HEADER = "x-api-key"; | ||
| const BEARER_API_KEY_PREFIX = "sk_live_"; | ||
|
|
||
| function extractApiKey(req: Request): string | undefined { | ||
| const headerKey = req.headers.get(API_KEY_HEADER); | ||
| if (headerKey) return headerKey; | ||
|
|
||
| const authorization = req.headers.get("authorization"); | ||
| const match = authorization?.match(/^Bearer\s+(.+)$/i); | ||
| const bearer = match?.[1]; | ||
| if (bearer?.startsWith(BEARER_API_KEY_PREFIX)) return bearer; | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| function parseApiKeyMetadata( | ||
| metadata: unknown, | ||
| ): Record<string, unknown> | null { | ||
| if (!metadata) return null; | ||
|
|
||
| if (typeof metadata === "string") { | ||
| try { | ||
| const parsed = JSON.parse(metadata); | ||
| return parsed && typeof parsed === "object" | ||
| ? (parsed as Record<string, unknown>) | ||
| : null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| return typeof metadata === "object" | ||
| ? (metadata as Record<string, unknown>) | ||
| : null; | ||
| } | ||
|
|
||
| export type ApiKeyResolution = | ||
| | { kind: "not-api-key" } | ||
| | { kind: "no-organization-metadata" } | ||
| | { kind: "ok"; organizationId: string } | ||
| | { kind: "invalid" }; | ||
|
|
||
| /** | ||
| * Resolve the organization an api-key-authed request should operate | ||
| * against. api keys created via `apiKeyRouter.create` store their | ||
| * intended org in `metadata.organizationId`. Without this, the | ||
| * api-key-synthesized session has no `activeOrganizationId` and | ||
| * falls through to the default newest-membership resolver, which | ||
| * silently routes requests to the wrong org when a user belongs to | ||
| * more than one. | ||
| * | ||
| * Kinds: | ||
| * - `not-api-key` — request is not api-key authed; caller should use session as-is | ||
| * - `no-organization-metadata` — legacy key without org metadata; caller should use session as-is | ||
| * - `ok` — key's org is valid and user is still a member; caller should override active org | ||
| * - `invalid` — key verification failed OR user is no longer a member of the key's org; caller should deny the request | ||
| */ | ||
| export async function resolveApiKey( | ||
| req: Request, | ||
| auth: typeof betterAuth, | ||
| userId: string, | ||
| ): Promise<ApiKeyResolution> { | ||
| const apiKey = extractApiKey(req); | ||
| if (!apiKey) return { kind: "not-api-key" }; | ||
|
|
||
| try { | ||
| const result = await auth.api.verifyApiKey({ body: { key: apiKey } }); | ||
| if (!result.valid || !result.key) return { kind: "invalid" }; | ||
|
|
||
| const metadata = parseApiKeyMetadata(result.key.metadata); | ||
| const organizationId = metadata?.organizationId; | ||
| if (typeof organizationId !== "string") { | ||
| return { kind: "no-organization-metadata" }; | ||
| } | ||
|
|
||
| const membership = await db.query.members.findFirst({ | ||
| where: and( | ||
| eq(members.userId, userId), | ||
| eq(members.organizationId, organizationId), | ||
| ), | ||
| }); | ||
| if (!membership) return { kind: "invalid" }; | ||
|
|
||
| return { kind: "ok", organizationId }; | ||
| } catch { | ||
| return { kind: "invalid" }; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -232,8 +232,10 @@ function copyNativePackages(libDir: string, target: Target): void { | |
| * | ||
| * 1. `better-sqlite3`: download the Node-ABI prebuild from GitHub and | ||
| * overwrite `build/Release/better_sqlite3.node`. | ||
| * 2. `node-pty`: delete `build/Release/` so the `bindings` loader falls | ||
| * through to the N-API prebuild in `prebuilds/<target>/pty.node`. | ||
| * 2. `node-pty`: on darwin, fall back to the in-package N-API prebuild | ||
| * by removing `build/`. On linux-x64 there is no upstream prebuild, | ||
| * so rebuild from source via node-gyp; node-pty uses node-addon-api | ||
| * so the resulting binding is ABI-stable across Node versions. | ||
| */ | ||
| async function fixNativeBinariesForNode( | ||
| libDir: string, | ||
|
|
@@ -262,7 +264,22 @@ async function fixNativeBinariesForNode( | |
| join(bsqDest, "better_sqlite3.node"), | ||
| ); | ||
|
|
||
| const nodePtyBuild = join(destModules, "node-pty", "build"); | ||
| const nodePtyDir = join(destModules, "node-pty"); | ||
| const nodePtyBuild = join(nodePtyDir, "build"); | ||
|
|
||
| if (target === "linux-x64") { | ||
| if (existsSync(nodePtyBuild)) { | ||
| rmSync(nodePtyBuild, { recursive: true, force: true }); | ||
| } | ||
| console.log("[build-dist] compiling node-pty from source for linux-x64"); | ||
| await exec("npx", ["--yes", "node-gyp", "rebuild"], nodePtyDir); | ||
|
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.
console.log("[build-dist] compiling node-pty from source for linux-x64 (requires python3 + gcc)");Or check explicitly before calling await exec("python3", ["--version"]).catch(() => {
throw new Error("node-gyp requires python3 — install it and re-run");
});Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/cli/scripts/build-dist.ts
Line: 274-275
Comment:
**`node-gyp rebuild` requires Python 3 and a C++ toolchain**
`node-gyp rebuild` will fail with a cryptic error if `python3` or `gcc`/`g++` are not available on the build machine. This path is new to the script and could surprise first-time linux contributors. Consider adding a preflight check or a clearer failure message:
```ts
console.log("[build-dist] compiling node-pty from source for linux-x64 (requires python3 + gcc)");
```
Or check explicitly before calling `exec`:
```ts
await exec("python3", ["--version"]).catch(() => {
throw new Error("node-gyp requires python3 — install it and re-run");
});
```
How can I resolve this? If you propose a fix, please make it concise. |
||
| const builtBinding = join(nodePtyBuild, "Release", "pty.node"); | ||
| if (!existsSync(builtBinding)) { | ||
| throw new Error(`node-pty build did not produce ${builtBinding}`); | ||
| } | ||
| return; | ||
| } | ||
|
Comment on lines
+278
to
+291
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.
After The darwin path avoids this cleanly by removing const builtBinding = join(nodePtyBuild, "Release", "pty.node");
if (!existsSync(builtBinding)) {
throw new Error(`node-pty build did not produce ${builtBinding}`);
}
// Strip intermediate artifacts — only the .node file is needed at runtime
const objTarget = join(nodePtyBuild, "Release", "obj.target");
if (existsSync(objTarget)) rmSync(objTarget, { recursive: true, force: true });
// Also remove non-Release subdirectories that node-gyp generates
for (const entry of readdirSync(nodePtyBuild)) {
if (entry !== "Release") {
rmSync(join(nodePtyBuild, entry), { recursive: true, force: true });
}
}
return;Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/cli/scripts/build-dist.ts
Line: 276-281
Comment:
**Build artifacts bloat the linux tarball**
After `node-gyp rebuild` succeeds, only `build/Release/pty.node` is needed at runtime. The entire `build/` directory — which includes `obj.target/` (compiled `.o` files), `Makefile`, `config.gypi`, and other intermediate artifacts — stays in the staging tree and is packed into the tarball. Depending on the machine, this can easily add 5–20 MB.
The darwin path avoids this cleanly by removing `build/` entirely (darwin falls back to the in-package prebuilds). The linux path should do the same with a targeted cleanup after the verification:
```ts
const builtBinding = join(nodePtyBuild, "Release", "pty.node");
if (!existsSync(builtBinding)) {
throw new Error(`node-pty build did not produce ${builtBinding}`);
}
// Strip intermediate artifacts — only the .node file is needed at runtime
const objTarget = join(nodePtyBuild, "Release", "obj.target");
if (existsSync(objTarget)) rmSync(objTarget, { recursive: true, force: true });
// Also remove non-Release subdirectories that node-gyp generates
for (const entry of readdirSync(nodePtyBuild)) {
if (entry !== "Release") {
rmSync(join(nodePtyBuild, entry), { recursive: true, force: true });
}
}
return;
```
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| if (existsSync(nodePtyBuild)) { | ||
| console.log( | ||
| "[build-dist] removing node-pty build/ so bindings falls back to prebuilds/", | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.