From d02b9f8718129e1639e3f92db50abf42b174f669 Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Mon, 10 Feb 2025 14:14:50 -0800 Subject: [PATCH 1/6] Improve auth policy configuration for callable functions. --- spec/v2/providers/https.spec.ts | 135 +++++++++++++++++++++++++------- src/common/providers/https.ts | 10 ++- src/v2/providers/https.ts | 75 ++++++++++++++---- 3 files changed, 172 insertions(+), 48 deletions(-) diff --git a/spec/v2/providers/https.spec.ts b/spec/v2/providers/https.spec.ts index 3685e22b1..c8a54df08 100644 --- a/spec/v2/providers/https.spec.ts +++ b/spec/v2/providers/https.spec.ts @@ -30,12 +30,11 @@ import { expectedResponseHeaders, MockRequest } from "../../fixtures/mockrequest import { runHandler } from "../../helper"; import { FULL_ENDPOINT, MINIMAL_V2_ENDPOINT, FULL_OPTIONS, FULL_TRIGGER } from "./fixtures"; import { onInit } from "../../../src/v2/core"; -import { Handler } from "express"; import { genkit } from "genkit"; function request(args: { data?: any; - auth?: Record; + auth?: Record; headers?: Record; method?: MockRequest["method"]; }): any { @@ -526,34 +525,92 @@ describe("onCall", () => { expect(anonResp.status).to.equal(403); }); - it("should check hasClaim", async () => { - const anyValue = https.onCall( - { - authPolicy: https.hasClaim("meaning"), - }, - () => "HHGTTG" - ); - const specificValue = https.onCall( - { - authPolicy: https.hasClaim("meaning", "42"), - }, - () => "HHGTG" - ); - - const cases: Array<{ fn: Handler; auth?: Record; status: number }> = [ - { fn: anyValue, auth: { meaning: "42" }, status: 200 }, - { fn: anyValue, auth: { meaning: "43" }, status: 200 }, - { fn: anyValue, auth: { order: "66" }, status: 403 }, - { fn: anyValue, status: 403 }, - { fn: specificValue, auth: { meaning: "42" }, status: 200 }, - { fn: specificValue, auth: { meaning: "43" }, status: 403 }, - { fn: specificValue, auth: { order: "66" }, status: 403 }, - { fn: specificValue, status: 403 }, - ]; - for (const test of cases) { - const resp = await runHandler(test.fn, request({ auth: test.auth })); - expect(resp.status).to.equal(test.status); - } + describe("hasClaim", () => { + it("should check single claim with specific value", async () => { + const func = https.onCall( + { + authPolicy: https.hasClaim("meaning", "42"), + }, + () => true + ); + const validResp = await runHandler(func, request({ auth: { meaning: "42" } })); + expect(validResp.status).to.equal(200); + + const noClaimResp = await runHandler(func, request({ auth: {} })); + expect(noClaimResp.status).to.equal(403); + }); + + it("should check single claim with default value (true)", async () => { + const func = https.onCall( + { + authPolicy: https.hasClaim("admin"), + }, + () => true + ); + const validResp = await runHandler(func, request({ auth: { admin: true } })); + expect(validResp.status).to.equal(200); + + const wrongTypeResp = await runHandler(func, request({ auth: { admin: "true" } })); + expect(wrongTypeResp.status).to.equal(403); + + const falseResp = await runHandler(func, request({ auth: { admin: false } })); + expect(falseResp.status).to.equal(403); + + const noClaimResp = await runHandler(func, request({ auth: {} })); + expect(noClaimResp.status).to.equal(403); + }); + + it("should check multiple claims with default value (true)", async () => { + const func = https.onCall( + { + authPolicy: https.hasClaim(["pro", "eap"]), + }, + () => true + ); + const validResp = await runHandler(func, request({ auth: { pro: true, eap: true } })); + expect(validResp.status).to.equal(200); + + const missingResp = await runHandler(func, request({ auth: { pro: true } })); + expect(missingResp.status).to.equal(403); + + const wrongTypeResp = await runHandler( + func, + request({ auth: { pro: "true", eap: "true" } }) + ); + expect(wrongTypeResp.status).to.equal(403); + + const noClaimResp = await runHandler(func, request({ auth: {} })); + expect(noClaimResp.status).to.equal(403); + }); + + it("should check multiple claims with specific values", async () => { + const func = https.onCall( + { + authPolicy: https.hasClaim({ + meaning: 42, + animal: "dolphin", + }), + }, + () => true + ); + const validResp = await runHandler( + func, + request({ auth: { meaning: 42, animal: "dolphin" } }) + ); + expect(validResp.status).to.equal(200); + + const wrongTypeResp = await runHandler( + func, + request({ auth: { meaning: "42", animal: "dolphin" } }) + ); + expect(wrongTypeResp.status).to.equal(403); + + const missingResp = await runHandler(func, request({ auth: { meaing: 42 } })); + expect(missingResp.status).to.equal(403); + + const noClaimResp = await runHandler(func, request({ auth: {} })); + expect(noClaimResp.status).to.equal(403); + }); }); it("can be any callback", async () => { @@ -569,6 +626,24 @@ describe("onCall", () => { const accessDenied = await runHandler(divTwo, request({ data: 1 })); expect(accessDenied.status).to.equal(403); }); + + it("should check emailVerified", async () => { + const func = https.onCall( + { + authPolicy: https.emailVerified(), + }, + () => 42 + ); + + const verifiedResp = await runHandler(func, request({ auth: { email_verified: true } })); + expect(verifiedResp.status).to.equal(200); + + const unverifiedResp = await runHandler(func, request({ auth: { email_verified: false } })); + expect(unverifiedResp.status).to.equal(403); + + const noAuthResp = await runHandler(func, request({ auth: {} })); + expect(noAuthResp.status).to.equal(403); + }); }); }); diff --git a/src/common/providers/https.ts b/src/common/providers/https.ts index 6ca37f69a..7faa4ff78 100644 --- a/src/common/providers/https.ts +++ b/src/common/providers/https.ts @@ -846,9 +846,13 @@ function wrapOnCallHandler( const data: Req = decode(req.body.data); if (options.authPolicy) { - const authorized = await options.authPolicy(context.auth ?? null, data); - if (!authorized) { - throw new HttpsError("permission-denied", "Permission Denied"); + try { + const authorized = await options.authPolicy(context.auth ?? null, data); + if (!authorized) { + throw new HttpsError("permission-denied", "Permission Denied"); + } + } catch (e: unknown) { + throw new HttpsError("permission-denied", (e as any).message || "Permission Denied"); } } let result: Res; diff --git a/src/v2/providers/https.ts b/src/v2/providers/https.ts index 831dabacd..745b4fc74 100644 --- a/src/v2/providers/https.ts +++ b/src/v2/providers/https.ts @@ -165,6 +165,12 @@ export interface HttpsOptions extends Omit = (auth: AuthData | null, data: T) => boolean | Promise; + /** * Options that can be set on a callable HTTPS function. */ @@ -212,34 +218,73 @@ export interface CallableOptions extends HttpsOptions { /** * Callback for whether a request is authorized. * - * Designed to allow reusable auth policies to be passed as an options object. Two built-in reusable policies exist: - * isSignedIn and hasClaim. + * Designed to allow reusable auth policies to be passed as an options object. */ - authPolicy?: (auth: AuthData | null, data: T) => boolean | Promise; + authPolicy?: AuthPolicy; } /** * An auth policy that requires a user to be signed in. */ -export const isSignedIn = - () => - (auth: AuthData | null): boolean => - !!auth; +export function isSignedIn(): AuthPolicy { + return (auth: AuthData | null): boolean => { + if (!auth) { + throw new Error("Must be signed in"); + } + return true; + }; +} +export function hasClaim(claim: string, value?: string): AuthPolicy; +export function hasClaim(claims: string[]): AuthPolicy; +export function hasClaim(claims: Record): AuthPolicy; /** - * An auth policy that requires a user to be both signed in and have a specific claim (optionally with a specific value) + * An auth policy that requires a user to be both signed in and have a specific claims (optionally with a specific value) */ -export const hasClaim = - (claim: string, value?: string) => - (auth: AuthData | null): boolean => { +export function hasClaim( + claimOrClaims: string | string[] | Record, + value?: string +): AuthPolicy { + let claimsToCheck: Record = {}; + + if (typeof claimOrClaims === "string") { + claimsToCheck[claimOrClaims] = value ?? true; + } else if (Array.isArray(claimOrClaims)) { + for (const claim of claimOrClaims) { + claimsToCheck[claim] = true; + } + } else { + claimsToCheck = claimOrClaims; + } + + return (auth: AuthData | null): boolean => { if (!auth) { - return false; + throw new Error("Must be signed in"); } - if (!(claim in auth.token)) { - return false; + for (const claim of Object.keys(claimsToCheck)) { + if (!(claim in auth.token)) { + throw new Error(`Missing claim '${claim}'`); + } + if (auth.token[claim] !== claimsToCheck[claim]) { + throw new Error(`Missing claim '${claim}' with value '${value}'`); + } + } + return true; + }; +} + +/** + * An auth policy that requires a user to be both signed in and have email verified + */ +export function emailVerified(): AuthPolicy { + return (auth: AuthData | null): boolean | Promise => { + if (!auth) { + throw new Error("Must be signed in"); } - return !value || auth.token[claim] === value; + console.log(auth) + return hasClaim("email_verified")(auth, undefined); }; +} /** * Handles HTTPS requests. From 33428c895cd6c6ae50259c111f1e0dcef360d17a Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Mon, 10 Feb 2025 15:11:21 -0800 Subject: [PATCH 2/6] Fix linter. --- spec/helper.ts | 4 +--- src/v2/providers/https.ts | 1 - 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/spec/helper.ts b/spec/helper.ts index bc1760f0e..9df32156b 100644 --- a/spec/helper.ts +++ b/spec/helper.ts @@ -78,9 +78,7 @@ export function runHandler( const toSend = typeof sendBody === "object" ? JSON.stringify(sendBody) : sendBody; const body = - typeof this.sentBody === "undefined" - ? toSend - : this.sentBody + String(toSend || ""); + typeof this.sentBody === "undefined" ? toSend : this.sentBody + String(toSend || ""); this.end(body); } diff --git a/src/v2/providers/https.ts b/src/v2/providers/https.ts index 745b4fc74..35f1a91cd 100644 --- a/src/v2/providers/https.ts +++ b/src/v2/providers/https.ts @@ -281,7 +281,6 @@ export function emailVerified(): AuthPolicy { if (!auth) { throw new Error("Must be signed in"); } - console.log(auth) return hasClaim("email_verified")(auth, undefined); }; } From 38a632f334725fd0081c50f53f856641c8808164 Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Mon, 10 Feb 2025 15:13:10 -0800 Subject: [PATCH 3/6] Add one more test case. --- spec/v2/providers/https.spec.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/v2/providers/https.spec.ts b/spec/v2/providers/https.spec.ts index c8a54df08..e26bd03e6 100644 --- a/spec/v2/providers/https.spec.ts +++ b/spec/v2/providers/https.spec.ts @@ -536,6 +536,9 @@ describe("onCall", () => { const validResp = await runHandler(func, request({ auth: { meaning: "42" } })); expect(validResp.status).to.equal(200); + const wrongValResp = await runHandler(func, request({ auth: { meaning: "43" } })); + expect(wrongValResp.status).to.equal(403); + const noClaimResp = await runHandler(func, request({ auth: {} })); expect(noClaimResp.status).to.equal(403); }); @@ -567,6 +570,7 @@ describe("onCall", () => { }, () => true ); + const validResp = await runHandler(func, request({ auth: { pro: true, eap: true } })); expect(validResp.status).to.equal(200); From bd8db7d12ea4022b238c9392130346c3b7867e57 Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Mon, 10 Feb 2025 16:07:17 -0800 Subject: [PATCH 4/6] Change impl to check for truthy values. --- spec/v2/providers/https.spec.ts | 9 ++++++--- src/v2/providers/https.ts | 13 ++++++++++--- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/spec/v2/providers/https.spec.ts b/spec/v2/providers/https.spec.ts index e26bd03e6..31f169510 100644 --- a/spec/v2/providers/https.spec.ts +++ b/spec/v2/providers/https.spec.ts @@ -543,7 +543,7 @@ describe("onCall", () => { expect(noClaimResp.status).to.equal(403); }); - it("should check single claim with default value (true)", async () => { + it("should check single claim with default value (truthy)", async () => { const func = https.onCall( { authPolicy: https.hasClaim("admin"), @@ -553,12 +553,15 @@ describe("onCall", () => { const validResp = await runHandler(func, request({ auth: { admin: true } })); expect(validResp.status).to.equal(200); - const wrongTypeResp = await runHandler(func, request({ auth: { admin: "true" } })); - expect(wrongTypeResp.status).to.equal(403); + const truthyResp = await runHandler(func, request({ auth: { admin: "true" } })); + expect(truthyResp.status).to.equal(200); const falseResp = await runHandler(func, request({ auth: { admin: false } })); expect(falseResp.status).to.equal(403); + const falseStrResp = await runHandler(func, request({ auth: { admin: "false" } })); + expect(falseStrResp.status).to.equal(403); + const noClaimResp = await runHandler(func, request({ auth: {} })); expect(noClaimResp.status).to.equal(403); }); diff --git a/src/v2/providers/https.ts b/src/v2/providers/https.ts index 35f1a91cd..1e2ec0996 100644 --- a/src/v2/providers/https.ts +++ b/src/v2/providers/https.ts @@ -248,7 +248,7 @@ export function hasClaim( let claimsToCheck: Record = {}; if (typeof claimOrClaims === "string") { - claimsToCheck[claimOrClaims] = value ?? true; + claimsToCheck[claimOrClaims] = value; } else if (Array.isArray(claimOrClaims)) { for (const claim of claimOrClaims) { claimsToCheck[claim] = true; @@ -265,8 +265,15 @@ export function hasClaim( if (!(claim in auth.token)) { throw new Error(`Missing claim '${claim}'`); } - if (auth.token[claim] !== claimsToCheck[claim]) { - throw new Error(`Missing claim '${claim}' with value '${value}'`); + const expectedValue = claimsToCheck[claim]; + const actualValue = auth.token[claim]; + + if (expectedValue === undefined) { + if (!actualValue || actualValue === "false") { + return false; + } + } else if (actualValue !== expectedValue) { + return false; } } return true; From e4dabe74592be440813d3869ada5172306c6927a Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Tue, 11 Feb 2025 10:46:39 -0800 Subject: [PATCH 5/6] Accept truthy values for multiple claims. --- spec/v2/providers/https.spec.ts | 12 ++++++------ src/v2/providers/https.ts | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/spec/v2/providers/https.spec.ts b/spec/v2/providers/https.spec.ts index 31f169510..4d051371c 100644 --- a/spec/v2/providers/https.spec.ts +++ b/spec/v2/providers/https.spec.ts @@ -577,14 +577,14 @@ describe("onCall", () => { const validResp = await runHandler(func, request({ auth: { pro: true, eap: true } })); expect(validResp.status).to.equal(200); - const missingResp = await runHandler(func, request({ auth: { pro: true } })); - expect(missingResp.status).to.equal(403); - - const wrongTypeResp = await runHandler( + const truthyResp = await runHandler( func, - request({ auth: { pro: "true", eap: "true" } }) + request({ auth: { pro: "true", eap: "abc" } }) ); - expect(wrongTypeResp.status).to.equal(403); + expect(truthyResp.status).to.equal(200); + + const missingResp = await runHandler(func, request({ auth: { pro: true } })); + expect(missingResp.status).to.equal(403); const noClaimResp = await runHandler(func, request({ auth: {} })); expect(noClaimResp.status).to.equal(403); diff --git a/src/v2/providers/https.ts b/src/v2/providers/https.ts index 1e2ec0996..748c587cc 100644 --- a/src/v2/providers/https.ts +++ b/src/v2/providers/https.ts @@ -61,11 +61,11 @@ export interface HttpsOptions extends Omit - | Expression - | ResetValue; + | SupportedRegion + | string + | Array + | Expression + | ResetValue; /** If true, allows CORS on requests to this function. * If this is a `string` or `RegExp`, allows requests from domains that match the provided value. @@ -251,7 +251,7 @@ export function hasClaim( claimsToCheck[claimOrClaims] = value; } else if (Array.isArray(claimOrClaims)) { for (const claim of claimOrClaims) { - claimsToCheck[claim] = true; + claimsToCheck[claim] = undefined; } } else { claimsToCheck = claimOrClaims; From fde5df3a165eb4cbc2d2b1364c34fa48b9cff49a Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Tue, 11 Feb 2025 10:49:11 -0800 Subject: [PATCH 6/6] Make linter happy. --- spec/v2/providers/https.spec.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/spec/v2/providers/https.spec.ts b/spec/v2/providers/https.spec.ts index 4d051371c..5062883ca 100644 --- a/spec/v2/providers/https.spec.ts +++ b/spec/v2/providers/https.spec.ts @@ -577,10 +577,7 @@ describe("onCall", () => { const validResp = await runHandler(func, request({ auth: { pro: true, eap: true } })); expect(validResp.status).to.equal(200); - const truthyResp = await runHandler( - func, - request({ auth: { pro: "true", eap: "abc" } }) - ); + const truthyResp = await runHandler(func, request({ auth: { pro: "true", eap: "abc" } })); expect(truthyResp.status).to.equal(200); const missingResp = await runHandler(func, request({ auth: { pro: true } }));