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

supporting recaptcha verdict for auth blocking functions #1458

Merged
merged 6 commits into from
Nov 2, 2023

Conversation

Xiaoshouzi-gh
Copy link
Contributor

@Xiaoshouzi-gh Xiaoshouzi-gh commented Aug 23, 2023

Description

Adding recaptcha support for auth blocking function beforeCreate and beforeSignIn

Code sample

V2

exports.checkrecaptcha = beforeUserSignedIn(async (event) => {
  if (event.additionalUserInfo.recaptchaScore > 0.6) {
    return {
      recaptchaPassed: true
    }
  } else {
    return {
      recaptchaPassed: false
    }
  }
});

V1

exports.recaptchaV1 = auth.user().beforeSignIn((userRecord, context) => {
  if (context.additionalUserInfo.recaptchaScore > 0.5) {
    return {
      recaptchaPassed: true,
    };
  } else {
    return {
      recaptchaPassed: false,
    };
  }
});

@Xiaoshouzi-gh Xiaoshouzi-gh marked this pull request as ready for review August 24, 2023 17:21
@Xiaoshouzi-gh
Copy link
Contributor Author

cc @prameshj @eromney

Copy link
Contributor

@blidd-google blidd-google left a comment

Choose a reason for hiding this comment

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

Just a few minor things, but looks great and thanks for your work on this! Also, please add an entry to the changelog documenting the changes. Will leave this in a commented state until we get the go-ahead from the API council.

src/common/providers/identity.ts Outdated Show resolved Hide resolved
};
}

/** Helper to generate payload to GCIP from client request.
Copy link
Contributor

Choose a reason for hiding this comment

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

add newline after /**

*/
export function generateRequestPayload(
authResponse?: BeforeCreateResponse | BeforeSignInResponse
): any {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know exactly the shape of the data GCIP expects? If we do then we could statically type the return value here instead of using any, otherwise not really a huge deal.

Choose a reason for hiding this comment

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

+1, let's avoid 'any' if possible. I guess BeforeSignInResponse is the type to use? (Or if we consolidate all fields into BlockingFunctionsResponse we could use that, in future)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BeforeSignInResponse is the interface for blocking function returns for developers (e.g. developers can return displayName, emailVeried etc field when they implemented the blocking function). The requestPayload(responsePayload) here is (will be) a new different interface that follows GCIP backend proto. They are very similar but nested a bit differently. This is also the reason why the naming is kind of confusing, it is generated from the blocking function response as a request to GCIP backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new interface for this.

src/common/providers/identity.ts Outdated Show resolved Hide resolved
src/common/providers/identity.ts Outdated Show resolved Hide resolved
@@ -338,8 +339,13 @@ export interface AuthBlockingEvent extends AuthEventContext {
data: AuthUserRecord;
}

/** The base handler response type for beforeCreate and beforeSignIn blocking events*/
export interface BlockingFunctionResponse {
recaptchaPassed?: boolean;

Choose a reason for hiding this comment

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

Since all fields are optional in BeforeCreateResponse and BeforeSignInResponse, should we put them all here? I understand this will be a much larger refactor, so this change is very optional.

Choose a reason for hiding this comment

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

Actually, given this is a public interface, I think we should keep it as you have it here. Maybe we can have an internal/hidden interface which consolidates all the fields, mainly as return type for generatePayloadRequest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason why separating this out is this new interface will be used in the beforeEmailSent trigger. And in the beforeEmailSent response, recaptchaPassed is the only field we support.

Choose a reason for hiding this comment

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

makes sense.. Since the fields are all optional, technically, we could use the same Response for beforeEmailSent trigger. But what you have is cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updates from API proposal - we are going to rename recaptchaPassed to recaptchaActionOverride and add this property to both BeforeCreateResponse and BeforeSignInResponse same as Pavithra suggested here.

*/
export function generateRequestPayload(
authResponse?: BeforeCreateResponse | BeforeSignInResponse
): any {

Choose a reason for hiding this comment

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

+1, let's avoid 'any' if possible. I guess BeforeSignInResponse is the type to use? (Or if we consolidate all fields into BlockingFunctionsResponse we could use that, in future)

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

@prameshj prameshj left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes!

@@ -762,4 +770,52 @@ describe("identity", () => {
);
});
});

describe("generateRequestPayload", () => {

Choose a reason for hiding this comment

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

"generateResponsePayload"

@@ -338,8 +339,13 @@ export interface AuthBlockingEvent extends AuthEventContext {
data: AuthUserRecord;
}

/** The base handler response type for beforeCreate and beforeSignIn blocking events*/
export interface BlockingFunctionResponse {
recaptchaPassed?: boolean;

Choose a reason for hiding this comment

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

makes sense.. Since the fields are all optional, technically, we could use the same Response for beforeEmailSent trigger. But what you have is cleaner.

[key: string]: any;
}

/** @internal */

Choose a reason for hiding this comment

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

nit: add a comment on why this is used.

"Internal definition to include all the fields that can be sent as a response from the blocking function to the backend. This is added mainly to have a type definition for 'generateResponsePayload'".

Copy link
Contributor

@blidd-google blidd-google left a comment

Choose a reason for hiding this comment

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

LGTM besides one very minor nit

@@ -673,7 +678,8 @@ export function generateResponsePayload(
return {};
}

const { recaptchaPassed, ...formattedAuthResponse } = authResponse;
const { recaptchaActionOverride: recaptchaActionOverride, ...formattedAuthResponse } =
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I missing something with the destructuring assignment for recaptchaActionOverride, or can we just do const { recaptchaActionOverride, ...formatedAuthResponse } = authResponse; instead?

/**
* The reCACPTCHA action options.
*/
export type RecatpchaActionOptions = "ALLOW" | "BLOCK";

Choose a reason for hiding this comment

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

nit: "Recaptcha" typo. Also in the comment.

@Xiaoshouzi-gh Xiaoshouzi-gh merged commit b897b0d into master Nov 2, 2023
13 checks passed
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.

3 participants