Add new 2nd gen Firestore auth context triggers #1519

blidd-google · 2024-02-01T18:31:28Z

Auth context support has only been available for RTDB 1st gen functions — until now! The Firestore and Eventarc teams have recently added support for Firestore Cloud Events to include event actor information, adding four new event types. These changes enable us to offer four new 2nd gen Firestore trigger types whose events will additionally contain the auth context along with the existing event data:

onDocumentWrittenWithAuthContext
onDocumentUpdatedWithAuthContext
onDocumentCreatedWithAuthContext
onDocumentDeletedWithAuthContext

The API is very similar to the existing 2nd gen Firestore triggers API; however, users should take special care to avoid event loss when migrating from the original 2nd gen Firestore trigger to the auth-context trigger. See the migration guide for more details about best practices. (TODO: link migration guide when available)

exaby73 · 2024-03-17T16:28:53Z

src/v2/providers/firestore.ts

+
+/**
+ * Event handler which triggers when a document is updated in Firestore.
+ *


The following doc was not added here:

This trigger will also provide the authentication context of the principal who triggered the event.

egilmorez

Couple of things for you, thanks!

egilmorez · 2024-03-21T20:25:23Z

src/v2/providers/firestore.ts

@@ -153,6 +177,50 @@ export function onDocumentWritten<Document extends string>(
  return onChangedOperation(writtenEventType, documentOrOpts, handler);
 }

+/**
+ * Event handler which triggers when a document is created, updated, or deleted in Firestore.


This and most other instances of "which" in these comments is better as "that" (more restrictive, rather than additive).

https://prowritingaid.com/art/438/-Which--or--That-%3A-Know-When-to-Use-Each.aspx

egilmorez · 2024-03-21T20:25:50Z

src/v2/providers/firestore.ts

@@ -153,6 +177,50 @@ export function onDocumentWritten<Document extends string>(
  return onChangedOperation(writtenEventType, documentOrOpts, handler);
 }

+/**
+ * Event handler which triggers when a document is created, updated, or deleted in Firestore.
+ * This trigger will also provide the authentication context of the principal who triggered the event.


Suggest "also provides"

Avoid "will" wherever you can. Please check this globally in this PR, thanks!

inlined

Where's the withInit calls? Is this based on an old CL?

inlined · 2024-04-01T17:00:49Z

src/v2/providers/firestore.ts

@@ -95,6 +115,10 @@ export interface FirestoreEvent<T, Params = Record<string, string>> extends Clou
  namespace: string;
  /** The document path */
  document: string;
+  /** The type of principal that triggered the event */
+  authType?: AuthType;


These will always be populated in the new event type, right? If so, we should create a separate event type where the fields are omitted in the previous event handlers and guaranteed to exist in the new ones.

@exaby73 actually made the same suggestion when implementing the triggers in the Python SDK, but my reasoning for using the same event type is that we had discussed plans for aliasing onDocumentX to onDocumentXWithAuthContext in the next major release of the SDK, which may pollute the namespace a bit and cause confusion; a user writing a onDocumentX function will need to use the FirestoreEventWithAuthContext interface as opposed to just a single FirestoreEvent interface. The additional authId field is also optional and not guaranteed to exist in the delivered event.

But I also see the argument for having a separate event type. WDYT? Happy to make the changes and coordinate with @exaby73 to restore his old changes if we think the correct approach.

I'm pretty strongly in favor of fields being non-optional when they will always be provided and omitted when they will never be provided (in TypeScript at least. I'm not the expert for Python)

inlined · 2024-04-01T17:01:54Z

src/v2/providers/firestore.ts

@@ -83,6 +101,8 @@ export type DocumentSnapshot = firestore.DocumentSnapshot;
 /** A Firestore QueryDocumentSnapshot */
 export type QueryDocumentSnapshot = firestore.QueryDocumentSnapshot;

+type AuthType = "service_account" | "api_key" | "system" | "unauthenticated" | "unknown";


Can we document when "unknown" will happen and if there are any assumptions the dev can make (e.g. that it's from an admin context)?

Also wouldn't hurt to export.

Yep the CL is a few months old, quite a few things have changed in the meantime!

spec/v2/providers/firestore.spec.ts

colerogers

lgtm after addressing @inlined's concerns

This reverts commit 0c51861.

This reverts commit aa8df0f.

* Ugly typesafety * Formatter * consolidate make firestore event fns and simplify typings --------- Co-authored-by: Brian Li <blidd@google.com>

inlined · 2024-04-03T22:08:10Z

src/v2/providers/firestore.ts

+  // const fn = (
+  //   event: FirestoreEvent<Change<QueryDocumentSnapshot> | undefined, ParamsOf<Document>> & {
+  //     foo: string;
+  //   }


Remove dead code

wilpar · 2024-05-03T19:42:00Z

migration guide link in the original post is 404.

blidd-google self-assigned this Feb 1, 2024

blidd-google requested review from colerogers and inlined February 1, 2024 18:33

blidd-google mentioned this pull request Feb 1, 2024

Add support for deploying 2nd gen Firestore auth context triggers firebase/firebase-tools#6734

Merged

blidd-google requested a review from egilmorez March 7, 2024 19:46

exaby73 reviewed Mar 17, 2024

View reviewed changes

egilmorez reviewed Mar 21, 2024

View reviewed changes

inlined reviewed Apr 1, 2024

View reviewed changes

blidd-google added 6 commits April 1, 2024 13:45

rebasing

bfc36df

add changelog

b660601

update docstrings

1184a58

update docstrings (again)

5204b0d

export authtype and add comment

b28d3d8

post rebase

0c51861

blidd-google force-pushed the blidd.firestore-v2-auth-context branch from 393c5e1 to 0c51861 Compare April 1, 2024 18:12

inlined reviewed Apr 1, 2024

View reviewed changes

spec/v2/providers/firestore.spec.ts Show resolved Hide resolved

colerogers reviewed Apr 2, 2024

View reviewed changes

blidd-google and others added 5 commits April 3, 2024 11:16

separate out event type with auth context

aa8df0f

Revert "post rebase"

c5bda98

This reverts commit 0c51861.

post rebase

92853d5

Revert "separate out event type with auth context"

8c83dd8

This reverts commit aa8df0f.

Ugly type safety (#1548)

3902b80

* Ugly typesafety * Formatter * consolidate make firestore event fns and simplify typings --------- Co-authored-by: Brian Li <blidd@google.com>

inlined approved these changes Apr 3, 2024

View reviewed changes

add oninit tests

7836601

blidd-google merged commit 53f7204 into master Apr 4, 2024
13 checks passed

inlined deleted the blidd.firestore-v2-auth-context branch April 7, 2024 15:31

Bullfrog1234 mentioned this pull request Apr 23, 2024

withAuthContext firestore functions cause error in emulator firebase/firebase-tools#7037

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new 2nd gen Firestore auth context triggers #1519

Add new 2nd gen Firestore auth context triggers #1519

blidd-google commented Feb 1, 2024

exaby73 Mar 17, 2024

egilmorez left a comment

egilmorez Mar 21, 2024

egilmorez Mar 21, 2024

inlined left a comment

inlined Apr 1, 2024

blidd-google Apr 1, 2024

inlined Apr 1, 2024

inlined Apr 1, 2024

inlined Apr 1, 2024

blidd-google Apr 1, 2024

colerogers left a comment

inlined Apr 3, 2024

wilpar commented May 3, 2024

Add new 2nd gen Firestore auth context triggers #1519

Add new 2nd gen Firestore auth context triggers #1519

Conversation

blidd-google commented Feb 1, 2024

Choose a reason for hiding this comment

egilmorez left a comment

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

colerogers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilpar commented May 3, 2024