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 support for beforeSendEmail trigger #1492

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

blidd-google
Copy link
Contributor

@blidd-google blidd-google commented Nov 28, 2023

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 from blidd.blocking-fn-email.

Copy link
Contributor

@Xiaoshouzi-gh Xiaoshouzi-gh left a 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 */
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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 Show resolved Hide resolved
@@ -391,6 +399,7 @@ interface DecodedPayloadUserRecordEnrolledFactors {
export interface DecodedPayloadUserRecord {
uid: string;
email?: string;
email_type?: string;
Copy link
Contributor

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 Show resolved Hide resolved
? 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) {
Copy link
Contributor

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.

src/v2/providers/identity.ts Show resolved Hide resolved
@@ -41,6 +41,15 @@ const BEFORE_SIGN_IN_TRIGGER = {
},
};

const BEFORE_EMAIL_TRIGGER = {
eventType: "providers/cloud.auth/eventTypes/user.beforeSendEmail",
options: {
Copy link
Contributor

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

spec/v2/providers/identity.spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Xiaoshouzi-gh Xiaoshouzi-gh left a 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 */
Copy link
Contributor

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 Show resolved Hide resolved
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.

2 participants