-
Notifications
You must be signed in to change notification settings - Fork 204
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 support for beforeSendEmail trigger #1492
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, only nit comments.
/** Defines the auth event context for blocking events */ | ||
export interface AuthEventContext extends EventContext { | ||
locale?: string; | ||
ipAddress: string; | ||
userAgent: string; | ||
additionalUserInfo?: AdditionalUserInfo; | ||
credential?: Credential; | ||
emailType?: EmailType; | ||
} | ||
|
||
/** Defines the auth event for 2nd gen blocking events */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a comment say that for beforeEmailSent and beforeSmsSent, this field will be undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this field be defined for beforeEmailSent
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo, this should be undefined for beforeCreate and beforeSignIn events. May be a comment saying this value is only available for beforeEmailSent trigger.
src/common/providers/identity.ts
Outdated
@@ -391,6 +399,7 @@ interface DecodedPayloadUserRecordEnrolledFactors { | |||
export interface DecodedPayloadUserRecord { | |||
uid: string; | |||
email?: string; | |||
email_type?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do not need email_type in the userrecord correct?
src/common/providers/identity.ts
Outdated
? await auth.getAuth(getApp())._verifyAuthBlockingToken(req.body.data.jwt) | ||
: await auth.getAuth(getApp())._verifyAuthBlockingToken(req.body.data.jwt, "run.app"); | ||
const authUserRecord = parseAuthUserRecord(decodedPayload.user_record); | ||
let authUserRecord: AuthUserRecord | undefined; | ||
if (decodedPayload.user_record) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be we should use decodedPayload.event_type value in the if statement. This helps eliminate the backend error.
spec/v2/providers/identity.spec.ts
Outdated
@@ -41,6 +41,15 @@ const BEFORE_SIGN_IN_TRIGGER = { | |||
}, | |||
}; | |||
|
|||
const BEFORE_EMAIL_TRIGGER = { | |||
eventType: "providers/cloud.auth/eventTypes/user.beforeSendEmail", | |||
options: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove the options here. See comments in v2/providers/identity.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nit comments
/** Defines the auth event context for blocking events */ | ||
export interface AuthEventContext extends EventContext { | ||
locale?: string; | ||
ipAddress: string; | ||
userAgent: string; | ||
additionalUserInfo?: AdditionalUserInfo; | ||
credential?: Credential; | ||
emailType?: EmailType; | ||
} | ||
|
||
/** Defines the auth event for 2nd gen blocking events */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo, this should be undefined for beforeCreate and beforeSignIn events. May be a comment saying this value is only available for beforeEmailSent trigger.
GCIP now supports a new event type that triggers before emails are sent using Identity Platform. This PR adds a new trigger to respond to the
beforeSendEmail
event type. Based on changes fromblidd.blocking-fn-email
.