Add blocking trigger type #4395

colerogers · 2022-04-03T15:49:58Z

Adds deployment code for blocking trigger types

firebase deploy --only functions

Requirements:

allows at most one blocking event type in a deployment
reduces the trigger options of all triggers to the OR of each trigger
does not overwrite identity platform options on function updates
does not overwrite identity platform URIs on delete if endpoint.uri !== resource uri
sets the invoker to allUsers on create & update

…cking

src/deploy/functions/backend.ts

src/deploy/functions/release/fabricator.ts

src/deploy/functions/backend.ts

src/deploy/functions/prepare.ts

taeold · 2022-04-04T04:56:14Z

src/deploy/functions/services/index.ts

+  ensureTriggerIsValid: noop,
+  copyResourceOptionsToEndpoint: noop,


rant alert - i don't know how to improve this code, but something smells here.

All these noops are making me nervious whether this is a good abstraction.

It's starting to feel like we are stuffing any "do something if this kind of endpoint is given" into this code, and I'm not sure if this is making things clearer for me to understand.

I do like that we have one place to look, but things are starting to look too "abstract" e.g. copyResourceOptionsToEndpoint didn't really mean anything to me until I read the implementation for Auth blocking function, and I can't think of a reason why this would be relevant to say pubsub function in the future.

After moving the resource options code into it's own function, I was able to remove this Service function property. We're only left with ensureTriggerIsValid which seems like an okay function to have for every service.

src/deploy/functions/prepare.ts

src/gcp/identityPlatform.ts

…cking

…ri for every httpsTrigger

inlined · 2022-04-05T22:00:47Z

src/deploy/functions/backend.ts

+  accessToken?: boolean;
+  idToken?: boolean;
+  refreshToken?: boolean;


I have no suggestion on how to fix this, but I'm worried that this is not scalable. These should be blocking functions, not auth functions. Maybe we should have a map of features or something?

@inlined How about we go with a record structure for options and for auth blocking triggers we'd assume they'd use these three options. So the trigger will now be

export interface BlockingTrigger { eventType: | typeof events.v1.AUTH_BLOCKING_EVENTS[number] | typeof events.v2.AUTH_BLOCKING_EVENTS[number]; options: Record<string, any>; }

What do you think? The biggest thing is we'd lose out on is type completion in ts

src/deploy/functions/backend.ts

inlined · 2022-04-05T23:15:09Z

src/deploy/functions/prepare.ts

@@ -271,3 +272,29 @@ function maybeCopyTriggerRegion(wantE: backend.Endpoint, haveE: backend.Endpoint
  }
  wantE.eventTrigger.region = haveE.eventTrigger.region;
 }
+
+/** Figures out the blocking endpoint options by taking the OR of every trigger option and reassigning that value back to the endpoint. */


Why is this the correct behavior? If I have a beforeUserCreated function I must have the same security options as a beforeUserLoggedIn function?

It's part of the trigger proposal - http://go/cf3-auth-blocking-triggers

Since these are resource level options, they must be the same. Instead of erroring if a user sets the beforeCreate & beforeSignIn options to different values, we just take the OR of them. This will need to be made very clear in our docs

src/deploy/functions/release/fabricator.ts

inlined · 2022-04-05T23:19:14Z

src/deploy/functions/release/fabricator.ts

+      // Blocking functions should always be public
+      await this.executor
+        .run(async () => {
+          await gcf.setInvokerCreate(endpoint.project, backend.functionName(endpoint), ["public"]);


Here and elsewhere, it's public because it's an auth blocking function, not because it's a blocking function. Please make the code forwards compatible to other blocking trigger types.

Added a check

if (backend.isBlockingTriggered(endpoint) && AUTH_BLOCKING_EVENTS.includes(endpoint.blockingTrigger.eventType)) { ... }

src/deploy/functions/validate.ts

inlined · 2022-04-05T23:32:15Z

src/functions/events/v2.ts

+export const BEFORE_CREATE_EVENT = "providers/cloud.auth/eventTypes/user.beforeCreate";
+
+export const BEFORE_SIGN_IN_EVENT = "providers/cloud.auth/eventTypes/user.beforeSignIn";
+
+export const AUTH_BLOCKING_EVENTS = [BEFORE_CREATE_EVENT, BEFORE_SIGN_IN_EVENT] as const;


IDK if I really love having this defined as a separate event type. I saw some places where only one type is handled and this also gets in the way of GCIP offering a new v2 api (not that they're super inclined to anyway).

In reality, events.v1 and events.v2 isn't about GCF versions as much as it is "eventflow" or "eventarc". It's OK that these event types will be handled by a v2 function and it's possible that we'll be forced to add support for all of events.v1 in v2 functions.

Would you recommend just dropping v1 events entirely?

inlined · 2022-04-05T23:41:35Z

src/gcp/identityPlatform.ts

+  project: string,
+  blockingConfig: BlockingFunctionsConfig
+): Promise<BlockingFunctionsConfig> {
+  const config =


Nit: Would this be easier if you just used proto.fieldMasks to create your field mask instead of setting it to "blockingFunctions"? Then you wouldn't need to merge both beforeCreate and beforeSignIn before setting it.

So funny thing about their API... if you try and use a field mask that has the inner property that changed, like blockingFunctions.triggers.beforeCreate.functionUri, it will not actually pickup the changes. That's why I went and set it to blockingFunctions

inlined · 2022-04-05T23:45:29Z

src/deploy/functions/services/authBlocking.ts

+    };
+  }
+
+  if (!update) {


Why don't we modify on updates? Is that not allowed?

I'm preserving out-of-band resource changes. That seemed to fit with our current model, let me know if you disagree

src/test/deploy/functions/services/authBlocking.spec.ts

…cking

…ts/v2

inlined

Looking really good! I'd personally make the trigger queue more narrowly scoped (e.g. inside auth) so we can parallelize as much as possible and it's not clear to me why we don't update options when the trigger is updated. Otherwise LGTM.

inlined · 2022-04-11T20:37:54Z

src/deploy/functions/release/fabricator.ts

+    this.triggerQueue = this.triggerQueue.then(async () => {
+      await this.executor
+        .run(() => services.serviceForEndpoint(endpoint).registerTrigger(endpoint, update))
+        .catch(rethrowAs(endpoint, "register blocking trigger"));
+    });


Why don't we make the triggerQueue be a feature of the Auth service? That way we run auth trigger updates in serial but can run other trigger updates in parallel.

I was trying to avoid defining a class for the Auth Blocking service, but parallelizing makes a lot of sense here, I'll make the change

inlined · 2022-04-11T20:39:21Z

src/deploy/functions/runtimes/discovery/v1alpha1.ts

@@ -201,18 +201,9 @@ function parseEndpoints(
      requireKeys(prefix + ".blockingTrigger", ep.blockingTrigger, "eventType");
      assertKeyTypes(prefix + ".blockingTrigger", ep.blockingTrigger, {
        eventType: "string",
-        accessToken: "boolean",


Depending how thorough you want to be, you can detect the eventType is an auth blocking event type and then do the assertKeyTypes for the inner options object

I ended up taking out the explicit eventType per your last review. The reason why I didn't compare each option is because other blockingTriggers will have a different set of options

inlined · 2022-04-11T20:44:30Z

src/deploy/functions/services/auth.ts

@@ -83,9 +82,9 @@ export async function registerTrigger(

  if (!update) {


I still don't think I understand why we only set these fields on function create instead of update.

I was trying to preserve out-of-band changes, but I guess if we're re-registering the trigger on updates then we should probably re-register the options too

taeold

Few nitpicks, but overall looks good.

taeold · 2022-04-12T17:46:47Z

src/deploy/functions/backend.ts

@@ -3,6 +3,7 @@ import * as gcf from "../../gcp/cloudfunctions";
 import * as gcfV2 from "../../gcp/cloudfunctionsv2";
 import * as utils from "../../utils";
 import * as runtimes from "./runtimes";
+import * as events from "../../functions/events";


nit* remove events per linter

Removed in #4443

taeold · 2022-04-12T17:49:57Z

src/deploy/functions/prepare.ts

+
+/** Figures out the blocking endpoint options by taking the OR of every trigger option and reassigning that value back to the endpoint. */
+export function inferBlockingDetails(want: backend.Backend): void {
+  const authBlockingEndpoints = backend


nit* technically blockingEndpoints 😛

Actually here I do a check in filter to see if the eventType is an Auth Blocking Trigger event

taeold · 2022-04-12T17:54:15Z

src/deploy/functions/release/fabricator.ts

+    endpoint: backend.Endpoint & backend.BlockingTriggered,
+    update: boolean
+  ): Promise<void> {
+    this.triggerQueue = this.triggerQueue.then(async () => {


q: where do we await on this triggerQueue? I was expecting something to wait until the triggerQueue is cleared in the fabricator.

I think you're checking out an old piece of code, but we're just adding the next task in the queue to the then function. When the current task is fulfilled, the next task will be executed.

taeold · 2022-04-12T17:55:19Z

src/deploy/functions/runtimes/node/parseTriggers.ts

@@ -68,6 +68,10 @@ export interface TriggerAnnotation {
    };
    invoker?: string[];
  };
+  blockingTrigger?: {
+    eventType: string;
+    options?: Record<string, any>;


nit* I've been preferring unknown over any to have the caller assert the type as needed. Is doing so here going to cause a whole bunch of work?

Fixed in #4443

taeold · 2022-04-12T18:47:47Z

src/deploy/functions/services/index.ts

  ensureTriggerRegion: (ep: backend.Endpoint & backend.EventTriggered) => Promise<void>;
+  validateTrigger: (
+    ep: backend.Endpoint & backend.BlockingTriggered,


nit* This seems unnecessarily strict - in theory, we expect other services to define validateTrigger no?

Fixed in #4443

taeold · 2022-04-12T18:56:23Z

src/deploy/functions/services/auth.ts

+   * @param ep the blocking endpoint
+   */
+  registerTrigger(ep: backend.Endpoint & backend.BlockingTriggered): Promise<void> {
+    this.triggerQueue = this.triggerQueue.then(() => this.registerTriggerLocked(ep));


So.. this is ensuring that we don't have concurrent update auth policy. We can have at most 2 requests, but the second one would short-circuit b/c policy would not have been changed?

I think we could have the same effect by just keeping a variable that says "haveUpdated" or something to make sure we the update IAM policy function just once? But that's a small optimization to save 1 unneeded IAM policy call, so I think this impl is fine.

I pushed Cole to this implementation. The problem is that we update functions concurrently and it's possible that we update the beforeCreate function at the same time as beforeSignIn. Since the process of updating involves reading a whole config, changing a field, and saving a whole config, it's possible that two racing updates will read the same original config, change one field, and only one write will win.

By using a promise chain we guarantee that the racing updates will happen sequentially. Think of it as running inside a mutex.

taeold · 2022-04-12T19:00:26Z

src/test/deploy/functions/validate.spec.ts

@@ -198,6 +199,101 @@ describe("validate", () => {
        /they have fewer than 2GB memory/
      );
    });
+
+    it("Disallows multiple beforeCreate blocking", () => {


nit - prefer lowercase disallow (same comment for tests below).

fixed in #4443

taeold · 2022-04-12T19:01:26Z

src/test/deploy/functions/services/auth.spec.ts

+  beforeEach(() => {
+    getConfig = sinon
+      .stub(identityPlatform, "getBlockingFunctionsConfig")
+      .throws("Unexpected call to getBlockingFunctionsConfig");


nit* for async functions, you'd use reject. I guess this works since await is forgiving 🤷🏼

fixed in #4443

taeold · 2022-04-12T19:04:27Z

src/test/deploy/functions/services/auth.spec.ts

+
+      await authBlockingService.registerTrigger(ep);
+
+      expect(blockingConfig).to.deep.equal(newBlockingConfig);


huh I'm having a brain freeze, but how does this pass? blockingConfig and newBlockingConfig clearly looks different to me.

Agreed. I think this is happening because we're mutating return values of blockingConfig? Which seems wrong here... Maybe setConfig should resolve w/o any return value (unless we depend on it) and we shoudl be verifying that setConfig was called with newBlockingConfig.

fixed in #4443, we no longer check for a modification to the object

inlined · 2022-04-13T00:37:02Z

src/deploy/functions/services/auth.ts

    newBlockingConfig.forwardInboundCredentials = {
      idToken: endpoint.blockingTrigger.options?.idToken || false,
      accessToken: endpoint.blockingTrigger.options?.accessToken || false,
      refreshToken: endpoint.blockingTrigger.options?.refreshToken || false,


I like your intention to preserve OOB changes. I think what I would do is say something like

newBlockingConfig.forwardInboundCredentials = { ...oldBlockingConfig.forwardInboundCredentials, ...endpoint.blockingTrigger.options }

This would keep options that were set out of band unless it was specified in the customer's code. The || false you have should generally be unnecessary because the API server will automatically fill zero values for any missing field.

fixed in #4443

inlined · 2022-04-13T00:39:08Z

src/deploy/functions/services/auth.ts

+   * @param ep the blocking endpoint
+   */
+  registerTrigger(ep: backend.Endpoint & backend.BlockingTriggered): Promise<void> {
+    this.triggerQueue = this.triggerQueue.then(() => this.registerTriggerLocked(ep));


I pushed Cole to this implementation. The problem is that we update functions concurrently and it's possible that we update the beforeCreate function at the same time as beforeSignIn. Since the process of updating involves reading a whole config, changing a field, and saving a whole config, it's possible that two racing updates will read the same original config, change one field, and only one write will win.

By using a promise chain we guarantee that the racing updates will happen sequentially. Think of it as running inside a mutex.

inlined · 2022-04-13T00:40:07Z

src/deploy/functions/services/auth.ts

+  name: Name;
+  api: string;
+  triggerQueue: Promise<void>;
+  unregisterQueue: Promise<void>;


nit: unused var

fixed in #4443

inlined · 2022-04-13T00:41:29Z

src/deploy/functions/services/index.ts

  "google.cloud.pubsub.topic.v1.messagePublished": PubSubService,
  "google.cloud.storage.object.v1.finalized": StorageService,
  "google.cloud.storage.object.v1.archived": StorageService,
  "google.cloud.storage.object.v1.deleted": StorageService,
  "google.cloud.storage.object.v1.metadataUpdated": StorageService,
  "google.firebase.firebasealerts.alerts.v1.published": FirebaseAlertsService,
-  "providers/cloud.auth/eventTypes/user.beforeCreate": AuthBlockingService,
-  "providers/cloud.auth/eventTypes/user.beforeSignIn": AuthBlockingService,
+  "providers/cloud.auth/eventTypes/user.beforeCreate": authBlockingService,
+  "providers/cloud.auth/eventTypes/user.beforeSignIn": authBlockingService,


nit: the capitalization here confuses me. Are these Classes or variables?

inlined · 2022-04-13T00:44:45Z

src/test/deploy/functions/services/auth.spec.ts

+
+      await authBlockingService.registerTrigger(ep);
+
+      expect(blockingConfig).to.deep.equal(newBlockingConfig);


Agreed. I think this is happening because we're mutating return values of blockingConfig? Which seems wrong here... Maybe setConfig should resolve w/o any return value (unless we depend on it) and we shoudl be verifying that setConfig was called with newBlockingConfig.

colerogers added 13 commits January 31, 2022 13:28

tinkering with implementations

25f3d62

Merge remote-tracking branch 'origin/master' into colerogers.auth-blo…

6fe07de

…cking

adding in cache for discovery

f616332

adding in skeleton for service object and identity platform api calls

8566842

merging master

6a4b7b9

fix conflicts

43b2316

adding in all the service code

eb016f7

formatter

9ce55af

rename

801cb5f

Merge remote-tracking branch 'origin/master' into colerogers.auth-blo…

0b22d42

…cking

removing the lookup before endpointToFunction & adding v2 stuff

94c4db4

add tests & changelog

b1cbf95

adding tsdoc comments

bee1b93

colerogers requested review from taeold and inlined and removed request for taeold April 3, 2022 20:50

colerogers marked this pull request as ready for review April 3, 2022 20:51

taeold requested changes Apr 4, 2022

View reviewed changes

colerogers added 10 commits April 4, 2022 15:43

addressing pr comments

ed416c8

fixing conflicts

a298aad

removing the empty object from gcp/identityPlatform

62999db

Merge remote-tracking branch 'origin/master' into colerogers.auth-blo…

e35f6c0

…cking

added typing & fixing endpointFromFunction to set securityLevel and u…

fc47475

…ri for every httpsTrigger

removing changelog entries

43516bc

cleaning up & set default endpoint options to false

851c3a3

adding in inferBlockingDetails

b674320

checking if config changed before calling set blocking config

bd9a0b9

formatter

7f4fce2

inlined requested changes Apr 5, 2022

View reviewed changes

colerogers added 10 commits April 5, 2022 20:16

cleaning up

7b69417

Merge remote-tracking branch 'origin/master' into colerogers.auth-blo…

320d222

…cking

adding event type if in fab & changing api to match prod

fbd7e3e

adding serviceForEndpoint in fab setTrigger

4fff5c0

converting to options record & yanking auth blocking events from even…

153bffb

…ts/v2

removing old comments

c671094

fixing delete blocking function bug

3c95a35

merging master

9faf7a8

adding a promise queue for trigger registration & deletion

77de23e

merge master

e758d2b

inlined approved these changes Apr 11, 2022

View reviewed changes

colerogers added 3 commits April 12, 2022 11:14

making auth blocking service a class & moving trigger queue into it

bb56d25

remove comment

a4bfd25

merging master

a2f66cd

colerogers requested a review from taeold April 12, 2022 18:39

colerogers enabled auto-merge (squash) April 12, 2022 18:43

taeold approved these changes Apr 12, 2022

View reviewed changes

colerogers merged commit 0ea9629 into master Apr 12, 2022

inlined reviewed Apr 13, 2022

View reviewed changes

colerogers mentioned this pull request Apr 13, 2022

Fix comments from Auth Blocking Triggers PR #4443

Merged

		ensureTriggerIsValid: noop,
		copyResourceOptionsToEndpoint: noop,

		@@ -83,9 +82,9 @@ export async function registerTrigger(

		if (!update) {


		await authBlockingService.registerTrigger(ep);

		expect(blockingConfig).to.deep.equal(newBlockingConfig);

Add blocking trigger type #4395

Add blocking trigger type #4395

Conversation

colerogers commented Apr 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colerogers Apr 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colerogers Apr 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inlined left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taeold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colerogers Apr 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colerogers commented Apr 3, 2022 •

edited

colerogers Apr 7, 2022 •

edited

colerogers Apr 6, 2022 •

edited

colerogers Apr 13, 2022 •

edited