Skip to content
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

Add new 2nd gen Firestore auth context triggers #1519

Merged
merged 12 commits into from
Apr 4, 2024

Conversation

blidd-google
Copy link
Contributor

@blidd-google blidd-google commented Feb 1, 2024

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.


/**
* Event handler which triggers when a document is updated in Firestore.
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following doc was not added here:

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

Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of things for you, thanks!

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest "also provides"

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

Copy link
Member

@inlined inlined left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@@ -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";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also wouldn't hurt to export.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@colerogers colerogers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm after addressing @inlined's concerns

blidd-google and others added 5 commits April 3, 2024 11:16
This reverts commit 0c51861.
* Ugly typesafety

* Formatter

* consolidate make firestore event fns and simplify typings

---------

Co-authored-by: Brian Li <[email protected]>
// const fn = (
// event: FirestoreEvent<Change<QueryDocumentSnapshot> | undefined, ParamsOf<Document>> & {
// foo: string;
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove dead code

@blidd-google blidd-google merged commit 53f7204 into master Apr 4, 2024
13 checks passed
@inlined inlined deleted the blidd.firestore-v2-auth-context branch April 7, 2024 15:31
@wilpar
Copy link

wilpar commented May 3, 2024

migration guide link in the original post is 404.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants