feature/CF3-Auth-Event-Triggers#1897
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces Firebase Authentication v2 triggers (onUserCreated and onUserDeleted) for Google Cloud Identity Platform, along with comprehensive unit tests. The feedback identifies three key issues: first, project and tenantId in AuthEvent should be marked as optional since they can be undefined at runtime; second, the raw event parameter should be shallow-copied instead of mutated directly to prevent side effects; and third, the truthiness check for opts.tenantId should be replaced with a strict undefined check to correctly handle empty string values.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for Firebase Authentication event triggers (onUserCreated and onUserDeleted) in Cloud Functions v2, including the necessary types, helper functions, and comprehensive unit tests. The feedback recommends optimizing the event trigger implementation by wrapping the handler at definition time to avoid runtime closure overhead, enhancing robustness by supporting both camelCase and lowercase tenant IDs, making the project ID regex extraction more flexible, and adding a definition-time validation check for the handler function.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for Firebase Authentication event triggers (onUserCreated and onUserDeleted) in Cloud Functions v2. The feedback suggests correctly parsing the raw CloudEvent data payload into a proper UserRecord instance using userRecordConstructor, updating the unit tests to verify this parsing, and cleaning up a duplicate argument in the copyIfPresent call.
wandamora
left a comment
There was a problem hiding this comment.
Thanks for making the changes!
inlined
left a comment
There was a problem hiding this comment.
Since you're creating new files you're going to need to edit package.json to map the import path to the actual file for both JS and .d.ts files. You also need to create a dummy file in /v2 (not /src/v2) due to a longstanding bug in eslint that follows actual paths instead of declared import paths. IDR if it's been fixed; last I checked and filed the bug report, the package maintainer tried to extort me to fix it 🫠
|
|
||
| it("should handle IS_NOT_TENANT option", () => { | ||
| const func = identity.onUserCreated({ tenantId: identity.IS_NOT_TENANT }, () => null); | ||
| expect(func.__endpoint.eventTrigger?.eventFilters?.tenantid).to.equal(""); |
There was a problem hiding this comment.
Just to be clear, does this mean there's a semantic difference between any of null, undefined, and ""?
Is it that unspecified listens to all tenants and "" listens to no tenant?
There was a problem hiding this comment.
Thats right. Unspecified (undefined) matches the default Eventarc behavior which listens across all tenants (as well as default project users). Setting it to IS_NOT_TENANT sets the manifest filter to "", which instructs Eventarc to match only users created in the default project (no tenant). Providing a specific string scopes the trigger to that specific tenant ID.
|
|
||
| const mockEvent = { | ||
| specversion: "1.0" as const, | ||
| source: "//identitytoolkit.googleapis.com/something-else", |
There was a problem hiding this comment.
This is peculiar. What are the cases for this. I clearly don't understand something about GCIP.
There was a problem hiding this comment.
This test case is primarily here for defensive programming. While standard production events will include /projects/YOUR_PROJECT_ID, local emulator events, custom tests or future Eventarc routing changes might omit the /projects/ segment in source. This test ensures our regex parsing doesn't throw a runtime error and gracefully leaves event.project as undefined if raw.source has an unexpected shape. Might not be entirely necessary to keep this since we don't really expect event arc sources to change.
| let called = false; | ||
| const func = identity.onUserCreated((event) => { | ||
| called = true; | ||
| expect(event.tenantId).to.be.undefined; |
There was a problem hiding this comment.
Is it to be present but undefined or not present?
There was a problem hiding this comment.
Setting it explicitly to undefined matches the optional property definition (tenantId?: string) in the interface, while ensuring a consistent object shape for consumers who might be destructuring the payload
| }); | ||
|
|
||
| it("should handle Expression for tenantId", () => { | ||
| const param = { |
There was a problem hiding this comment.
You can also just call defineString and delete it with the private API later
There was a problem hiding this comment.
Good point. I've updated the test to use defineString('MY_TENANT') and clean up afterward with clearParams()
| */ | ||
| export interface AuthEvent<T> extends CloudEvent<T> { | ||
| /** The project identifier*/ | ||
| project?: string; |
There was a problem hiding this comment.
nit: spaces between fields with comments.
There was a problem hiding this comment.
Done! Added blank lines between fields in AuthEvent
| eventTrigger: { | ||
| eventType, | ||
| eventFilters: {}, | ||
| retry: false, // Default retry policy |
There was a problem hiding this comment.
I'm curious: if it's the default, why does it need to be specified? Do we need to fix something elsewhere?
There was a problem hiding this comment.
While false is the default behavior on the GCP side, all other v2 event provider endpoints in the SDK explicitly initialize retry: opts.retry ?? false to ensure a fully populated manifest definition before applying overrides. Leaving it explicit here keeps this consistent with the rest of the SDK
| retry: false, // Default retry policy | ||
| // Firebase Auth event triggers are global by nature, and Eventarc requires the trigger | ||
| // region to be set to "global" even if the Cloud Function is deployed regionally. | ||
| region: "global", |
There was a problem hiding this comment.
I wonder if the CLI should also set this? That way we're defensive against other SDKs in other languages getting it wrong? We now have automatic region setting (I think that was added in my absence?) so that should be compatible with the coding style there.
There was a problem hiding this comment.
Okay noted. I've implemented a dedicated autheventarc service definition in firebase-tools. The CLI now defensively intercepts v2 Auth Eventarc triggers during deploy validation and automatically ensures endpoint.eventTrigger.region = 'global', matching the pattern used for Test Lab and Firebase Alerts. I'll send in another PR for that.
| }; | ||
| if (opts.tenantId !== undefined) { | ||
| if (opts.tenantId === IS_NOT_TENANT) { | ||
| endpoint.eventTrigger.eventFilters["tenantid"] = ""; |
There was a problem hiding this comment.
I thought IS_NOT_TENANT already is "", making this a safe assignment in all cases?
There was a problem hiding this comment.
IS_NOT_TENANT is actually assigned to RESET_VALUE (which is the special { __type: 'reset' }), rather than ''. Because of this, we have to check for IS_NOT_TENANT explicitly so we can map it to '' in the manifest filters, preventing the manifest from serializing the sentinel object.
| return func; | ||
| } | ||
|
|
||
| const USER_CREATED_EVENT = "google.firebase.auth.user.v2.created"; |
There was a problem hiding this comment.
Nit: Space before block comments
There was a problem hiding this comment.
Done! Added blank lines before the block comments.
| @@ -1 +1,2 @@ | |||
| - fix(v1): Call onInit for schedule.onRun functions (#1801) | |||
| fix(v1): Call onInit for schedule.onRun functions (#1801) | |||
| Added support for Firebase Authentication event triggers in Cloud Functions v2 (`onUserCreated` and `onUserDeleted`). (#1897) | |||
There was a problem hiding this comment.
Leave it out of changelogs while we're testing.
There was a problem hiding this comment.
Noted. Removed it from the change logs
This pull request introduces support for Firebase Authentication event triggers in Cloud Functions v2 by adding the onUserCreated and onUserDeleted functions. These new triggers leverage Eventarc to listen for user lifecycle events at both the project and tenant levels, unblocking a key migration path for developers moving from V1 to V2. To ensure a maintainable implementation, a shared helper function called makeAuthTrigger was created to handle the common logic for manifest generation and event execution wrapping, effectively reducing code duplication and keeping the implementation DRY. The PR also ensures correct deployment behavior by setting the trigger region to global in the generated manifest, aligning with Eventarc's requirements for these global events and preventing location mismatch errors during deployment. Comprehensive unit tests have been added to cover standard behavior as well as edge cases involving missing or parameterized tenant IDs, ensuring robust operation in diverse environments.