Skip to content

Commit 7e7fb4e

Browse files
committed
fix(webapp): instrument scoped API key authentication
1 parent d9705c2 commit 7e7fb4e

4 files changed

Lines changed: 113 additions & 12 deletions

File tree

apps/webapp/app/services/apiAuth.server.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,10 @@ import {
3030
} from "./organizationAccessToken.server";
3131
import { isPublicJWT, validatePublicJwtKey } from "./realtime/jwtAuth.server";
3232
import { isDefaultDevBranch, sanitizeBranchName } from "@trigger.dev/core/v3/utils/gitBranch";
33-
import { rbac } from "./rbac.server";
34-
import { observeLegacyBearerAuthentication } from "~/services/authTelemetry.server";
33+
import {
34+
authenticateAuthorizeBearerWithTelemetry,
35+
observeLegacyBearerAuthentication,
36+
} from "~/services/authTelemetry.server";
3537

3638
const ClaimsSchema = z.object({
3739
scopes: z.array(z.string()).optional(),
@@ -307,7 +309,7 @@ export async function authenticateApiKeyWithScope(
307309
return { ok: false, status: 401, error: "Invalid or Missing API key" };
308310
}
309311

310-
const result = await rbac.authenticateAuthorizeBearer(
312+
const result = await authenticateAuthorizeBearerWithTelemetry(
311313
request,
312314
{ action, resource },
313315
{ allowJWT }

apps/webapp/app/services/authTelemetry.server.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import type {
55
BearerCredentialKind,
66
BearerLookupPath,
77
HostBearerAuthResult,
8+
RbacResource,
89
} from "@trigger.dev/rbac";
910
import { authFeatureControls } from "~/services/authFeatureControls.server";
1011
import { rbac } from "~/services/rbac.server";
@@ -75,6 +76,25 @@ export async function authenticateBearerWithTelemetry(
7576
}
7677
}
7778

79+
export async function authenticateAuthorizeBearerWithTelemetry(
80+
request: Request,
81+
check: { action: string; resource: RbacResource },
82+
options: { allowJWT: boolean }
83+
) {
84+
// Keep authentication telemetry consistent with apiBuilder: a valid
85+
// credential records a successful authentication even when the subsequent
86+
// resource authorization fails. Authorization correctness is covered by the
87+
// route tests rather than folded into the authentication-health metric.
88+
const result = await authenticateBearerWithTelemetry(request, options);
89+
if (!result.ok) return result;
90+
91+
if (!result.ability.can(check.action, check.resource)) {
92+
return { ok: false as const, status: 403 as const, error: "Unauthorized" };
93+
}
94+
95+
return result;
96+
}
97+
7898
export async function observeLegacyBearerAuthentication<T extends { ok: boolean } | undefined>(
7999
request: Request,
80100
operation: () => Promise<T>

apps/webapp/test/apiAuthScope.test.ts

Lines changed: 65 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,21 @@
11
import { beforeEach, describe, expect, it, vi } from "vitest";
22

33
const rbacMocks = vi.hoisted(() => ({
4-
authenticateAuthorizeBearer: vi.fn<(...args: any[]) => Promise<any>>(),
4+
authenticateBearer: vi.fn<(...args: any[]) => Promise<any>>(),
55
}));
66

7+
const telemetryMocks = vi.hoisted(() => ({
8+
attemptsAdd: vi.fn(),
9+
durationRecord: vi.fn(),
10+
}));
11+
12+
vi.mock("@internal/tracing", () => ({
13+
getMeter: () => ({
14+
createCounter: () => ({ add: telemetryMocks.attemptsAdd }),
15+
createHistogram: () => ({ record: telemetryMocks.durationRecord }),
16+
createObservableGauge: () => ({ addCallback: vi.fn() }),
17+
}),
18+
}));
719
vi.mock("~/services/rbac.server", () => ({ rbac: rbacMocks }));
820
vi.mock("~/db.server", () => ({ prisma: {}, $replica: {} }));
921
vi.mock("~/env.server", () => ({ env: { SESSION_SECRET: "test-session-secret" } }));
@@ -35,7 +47,9 @@ import { authenticateApiKeyWithScope } from "~/services/apiAuth.server";
3547

3648
describe("authenticateApiKeyWithScope", () => {
3749
beforeEach(() => {
38-
rbacMocks.authenticateAuthorizeBearer.mockReset();
50+
rbacMocks.authenticateBearer.mockReset();
51+
telemetryMocks.attemptsAdd.mockReset();
52+
telemetryMocks.durationRecord.mockReset();
3953
});
4054

4155
it("returns 401 without a bearer credential", async () => {
@@ -49,14 +63,14 @@ describe("authenticateApiKeyWithScope", () => {
4963
status: 401,
5064
error: "Invalid or Missing API key",
5165
});
52-
expect(rbacMocks.authenticateAuthorizeBearer).not.toHaveBeenCalled();
66+
expect(rbacMocks.authenticateBearer).not.toHaveBeenCalled();
5367
});
5468

5569
it.each([
5670
{ status: 401 as const, error: "Invalid API key" },
5771
{ status: 403 as const, error: "Unauthorized" },
5872
])("preserves controller $status failures", async (failure) => {
59-
rbacMocks.authenticateAuthorizeBearer.mockResolvedValue({ ok: false, ...failure });
73+
rbacMocks.authenticateBearer.mockResolvedValue({ ok: false, ...failure });
6074
const request = new Request("https://example.com", {
6175
headers: { Authorization: "Bearer tr_test_key" },
6276
});
@@ -72,11 +86,12 @@ describe("authenticateApiKeyWithScope", () => {
7286
it("bridges controller success into the legacy private authentication shape", async () => {
7387
const environment = { id: "env_123" };
7488
const ability = { can: vi.fn(() => true), canSuper: vi.fn(() => true) };
75-
rbacMocks.authenticateAuthorizeBearer.mockResolvedValue({
89+
rbacMocks.authenticateBearer.mockResolvedValue({
7690
ok: true,
7791
environment,
7892
ability,
7993
subject: { type: "apiKey", apiKeyId: "key_123" },
94+
resolution: { credentialKind: "root_api_key", lookupPath: "root_current" },
8095
});
8196
const request = new Request("https://example.com", {
8297
headers: { Authorization: "Bearer tr_test_key", "x-trigger-branch": "feature/test" },
@@ -88,10 +103,22 @@ describe("authenticateApiKeyWithScope", () => {
88103
allowJWT: true,
89104
});
90105

91-
expect(rbacMocks.authenticateAuthorizeBearer).toHaveBeenCalledWith(
92-
request,
93-
{ action: "read", resource: { type: "envvars" } },
94-
{ allowJWT: true }
106+
expect(rbacMocks.authenticateBearer).toHaveBeenCalledWith(request, { allowJWT: true });
107+
expect(ability.can).toHaveBeenCalledWith("read", { type: "envvars" });
108+
expect(telemetryMocks.attemptsAdd).toHaveBeenCalledWith(1, {
109+
resolver: "rbac",
110+
credential_kind: "root_api_key",
111+
result: "success",
112+
lookup_path: "root_current",
113+
});
114+
expect(telemetryMocks.durationRecord).toHaveBeenCalledWith(
115+
expect.any(Number),
116+
expect.objectContaining({
117+
resolver: "rbac",
118+
credential_kind: "root_api_key",
119+
result: "success",
120+
lookup_path: "root_current",
121+
})
95122
);
96123
expect(result).toEqual({
97124
ok: true,
@@ -104,4 +131,33 @@ describe("authenticateApiKeyWithScope", () => {
104131
},
105132
});
106133
});
134+
135+
it("records successful authentication before returning an authorization failure", async () => {
136+
const ability = { can: vi.fn(() => false), canSuper: vi.fn(() => false) };
137+
rbacMocks.authenticateBearer.mockResolvedValue({
138+
ok: true,
139+
environment: { id: "env_123" },
140+
ability,
141+
subject: { type: "apiKey", apiKeyId: "key_123" },
142+
resolution: { credentialKind: "additional_api_key", lookupPath: "additional" },
143+
});
144+
const request = new Request("https://example.com", {
145+
headers: { Authorization: "Bearer tr_prod_sk_0123456789abcdefghijklmn" },
146+
});
147+
148+
await expect(
149+
authenticateApiKeyWithScope(request, {
150+
action: "write",
151+
resource: { type: "deployments" },
152+
})
153+
).resolves.toEqual({ ok: false, status: 403, error: "Unauthorized" });
154+
155+
expect(ability.can).toHaveBeenCalledWith("write", { type: "deployments" });
156+
expect(telemetryMocks.attemptsAdd).toHaveBeenCalledWith(1, {
157+
resolver: "rbac",
158+
credential_kind: "additional_api_key",
159+
result: "success",
160+
lookup_path: "additional",
161+
});
162+
});
107163
});

apps/webapp/test/projectEnvironmentCredentialRoute.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,29 @@ describe("project environment credential response", () => {
8585
});
8686
});
8787

88+
it("does not exchange a grace-window root key for the current root key", async () => {
89+
mocks.authenticateEnvironmentScopedApiRequest.mockResolvedValue({
90+
ok: true,
91+
authentication: {
92+
type: "apiKey",
93+
result: {
94+
ok: true,
95+
apiKey: "tr_prod_rotated_grace_key",
96+
type: "PRIVATE",
97+
environment,
98+
},
99+
},
100+
});
101+
102+
const response = await load();
103+
104+
expect(response.status).toBe(200);
105+
await expect(responseJson(response)).resolves.toMatchObject({
106+
apiKey: "tr_prod_rotated_grace_key",
107+
projectId: "proj_123",
108+
});
109+
});
110+
88111
it("returns the root key to an authorized user token", async () => {
89112
mocks.authenticateEnvironmentScopedApiRequest.mockResolvedValue({
90113
ok: true,

0 commit comments

Comments
 (0)