Asub 8192/captcha2 login #1978

edwardcho1231 · 2024-03-04T21:46:48Z

If you have not filled out the checklist below, the pr is not ready for review.

Description

Create a new component to handle reCaptcha V2 for signup, signin, magicLink, and checkout
Add reCaptcha V2 component to login block

Jira Ticket

https://arcpublishing.atlassian.net/browse/ASUB-8192

Acceptance Criteria

Update BotChallengeProtection component in identity-block
BotChallengeProtection will accept a challengeIn attribute. This attribute could be one of:
'signup' || ‘signin’ || ‘magiclink’ || ‘checkout’
IF !!`${challengeIn}Recaptcha && recaptchaScore between 0 - 1 &&!!recaptchaSiteKey
THEN render reCaptchaV3. reCaptchaV3 should be initialized by passing the recaptchaSiteKey
Every time you grab a response token from reCAPTCHA component, store the response in localStorage.
The BotChallengeProtection should be part of the Login block, and the reCaptcha token should be sent during the call to Identity.login();
User should be successfully Logged In when reCaptcha v3 is enabled for signin.
Write the integration tests required to check login is working as expected when signinRechaptcha (reCaptchaV3) is enabled/disabled.

Test Steps

Checkout this branch git checkout {branch name}
Run fusion repo with linked blocks npx fusion start -f -l @wpmedia/identity-block
Enable reCaptcha for login by visiting Subscription admin page of the org and site you want to test in
Create a page with login block or visit a site with login block
You should see the recaptcha box in the login block

Effect Of Changes

Before

Login block does not contain recaptcha box even when it's enabled for the org/site

After

reCaptcha box should be visible if enabled and required to pass before logging in.

Dependencies or Side Effects

Examples of dependencies or side effects are:

Manifest pr for new block:
Feature pack pr for local development:
How to deploy to news: https://arcpublishing.atlassian.net/wiki/spaces/TI/pages/3138682964/News+Theme+2.0+Migration+Development+Deployment+Notes
Additional settings that will be required in the blocks.json
Changes to the custom fields which will require users to reconfigure features
Dependency on another PR that needs to be merged first

Author Checklist

The author of the PR should fill out the following sections to ensure this PR is ready for review.

Confirmed all the test steps a reviewer will follow above are working.
Confirmed there are no linter errors. Please run npm run lint to check for errors. Often, npm run lint:fix will fix those errors and warnings.
Ran this code locally and checked that there are not any unintended side effects. For example, that a CSS selector is scoped only to a particular block.
Confirmed this PR has reasonable code coverage. You can run npm run test:coverage to see your progress.
- Confirmed this PR has unit test files
- Ran npm run test, made sure all tests are passing
- If the amount of work to write unit tests for this change are excessive,
  please explain why (so that we can fix it whenever it gets refactored).
Confirmed relevant documentation has been updated/added.

Reviewer Checklist

The reviewer of the PR should copy-paste this template into the review comments on review.

Linting code actions have passed.
Ran the code locally based on the test instructions.
- I don’t think this is needed to be tested locally. For example, a padding style change (storybook?) or a logic change (write a test).
I am a member of the engine theme team so that I can approve and merge this. If you're not on the team, you won't have access to approve and merge this pr.
Looked to see that the new or changed code has code coverage, specifically. We want the global code coverage to keep on going up with targeted testing.

blocks/identity-block/features/login/default.jsx

LauraPinilla · 2024-03-05T23:55:06Z

blocks/identity-block/components/bot-challenge-protection/index.jsx

+	const { Identity, isInitialized } = useIdentity();
+  const { Sales } = useSales();
+	const [siteKey, setSiteKey] = useState();
+	console.log('siteKey', siteKey)


remove consoleLog

Looks like some attributes are not being used: error, setError, isAdmin, phrases, ...

vgalatro · 2024-03-06T15:42:28Z

Is this PR a WIP, or is it ready for review? There's no test instructions, description, or linked ticket. The large amount of changes to our localization files is also concerning, was all of the code up to date before the generate:intl script was run?

edwardcho1231 · 2024-03-06T15:53:14Z

@vgalatro I just put this up for Laura to take a look for now. Once she approves, I was going to ask for a review from the Themes team. I figured that we would do the same thing as how we did it for the 2.0 migration, where the subs team would review functionality, and the themes team would look at things specific to themes.

LauraPinilla · 2024-03-06T17:15:01Z

blocks/identity-block/utils/useSales.js

@@ -0,0 +1,34 @@
+import { useState } from "react";


useIdentity was defined inside arc-themes-component project. I think it's worth defining this hook there too.

WPMedia/arc-themes-components#242

@edwardcho1231 Now It can be removed

LauraPinilla · 2024-03-07T15:26:04Z

blocks/identity-block/components/bot-challenge-protection/index.jsx

+
+	useEffect(() => {
+		const checkCaptcha = async () => {
+			const config = await Identity.getConfig();


@edwardcho1231 I think you can move L23-L25 into if on L27, thus you call config from identity or sales, depending on challengeIn

@LauraPinilla Don't we have to call Identity.config even when the challengeIn === 'checkout' since we need the recaptchaSiteKey?

good point @edwardcho1231 I missed that. Yep for checkout we need to call both

vgalatro · 2024-03-11T14:59:56Z

This will need to go into 2.3.0 once the branch is created, as we're launching 2.2.1 soon with security hotfixes.

LauraPinilla · 2024-03-12T21:54:36Z

blocks/identity-block/intl.json

@@ -1273,5 +1273,20 @@
 		"vi": "Tên người dùng",
 		"zh-CN": "用户名",
 		"zh-TW": "使用者名稱"
+	},
+	"identity-block.bot-protection-error": {


Hi @edwardcho1231 , As part of the designs we have an additional reCaptcha state when it expires.

LauraPinilla · 2024-03-12T22:03:06Z

blocks/identity-block/components/bot-challenge-protection/index.jsx

+
+	const onChange = (value) => {
+		setCaptchaToken(value);
+    localStorage.setItem('captchaToken', captchaToken);


@edwardcho1231 do you mind to use a different cookie name. One less common, something like ArcXP_captchaToken or something like this. We want to avoid another app could overwrite this value

LauraPinilla · 2024-03-12T22:45:09Z

blocks/identity-block/components/bot-challenge-protection/index.jsx

+		<section className={`${className}__bot-protection-section`} data-testid="bot-challege-protection-container">
+			{!!siteKey && <ReCAPTCHA
+				sitekey={siteKey}
+				onChange={onChange}


Checking the reCaptcha documentation if you include onExpired, you will be able to see the error "Verification expired, Check the checkbox again" after two minutes the challenge was completed, once the token is expired

<ReCAPTCHA sitekey={siteKey} onChange={onChange} onExpired />

LauraPinilla

@vgalatro this ticket LGTM now. In case you want to start to take a look.
Thanks @edwardcho1231 for all these changes.

vgalatro · 2024-03-13T17:45:37Z

Only thing I see is the package.json and package-lock.json have some conflicts with the new base branch. The versions of those files in arc-themes-release-version-2.3.0 would be the correct ones.

vgalatro · 2024-03-19T21:10:04Z

The changes look good now, but there's one last issue. There are failing tests that will prevent the block from being published. Please check them with npm run test:changed-feature-branch. Also do npm run lint:changed-feature-branch to make sure there's no linting errors, as they will also prevent the block publish.

…' into ASUB-8192/captcha2_login

vgalatro · 2024-03-22T16:57:29Z

@edwardcho1231 , the unit test check is still not passing due to low coverage:

Jest: "global" coverage threshold for branches (66%) not met: 62.5%

It looks like the new /bot-challenge-protection/index.jsx file has the lowest coverage, so improving that should get it over the global minimum.

…a2_login

vgalatro

Changes look good, and the test/lint action is passing, approved!

vgalatro · 2024-03-25T13:28:43Z

@edwardcho1231 , I'll go ahead and merge this in since I'm not sure you'll be able to.

edwardcho1231 requested a review from a team as a code owner March 4, 2024 21:46

github-advanced-security bot found potential problems Mar 4, 2024

View reviewed changes

blocks/identity-block/features/login/default.jsx Fixed Show fixed Hide fixed

blocks/identity-block/features/login/default.jsx Fixed Show fixed Hide fixed

LauraPinilla reviewed Mar 5, 2024

View reviewed changes

LauraPinilla reviewed Mar 6, 2024

View reviewed changes

LauraPinilla reviewed Mar 7, 2024

View reviewed changes

edwardcho1231 changed the base branch from arc-themes-release-version-2.2.0 to arc-themes-release-version-2.2.1 March 8, 2024 19:43

edwardcho1231 requested a review from LauraPinilla March 11, 2024 15:01

LauraPinilla reviewed Mar 12, 2024

View reviewed changes

edwardcho1231 requested a review from LauraPinilla March 13, 2024 15:02

LauraPinilla previously approved these changes Mar 13, 2024

View reviewed changes

edwardcho1231 dismissed LauraPinilla’s stale review via b438244 March 13, 2024 16:55

edwardcho1231 requested a review from LauraPinilla March 13, 2024 17:05

vgalatro changed the base branch from arc-themes-release-version-2.2.1 to arc-themes-release-version-2.3.0 March 13, 2024 17:41

LauraPinilla added the ready for review The PR author has completed the PR template and is ready for a review label Mar 13, 2024

edwardcho1231 added 10 commits March 13, 2024 14:42

add new compoent for bot protection

041690a

update styles

7507276

update login-link

e0ea359

address feedback

2d87df7

address feedback

3262d03

fix intl

0b0e73e

use useSales from component repo

3cca67c

remove unused vars

8be0f19

add test and clean up

6809a3b

address feedback

b3d73fe

vgalatro added needs changes The reviewer has requested changes from the PR author and removed ready for review The PR author has completed the PR template and is ready for a review labels Mar 19, 2024

address feedback

6c414d7

edwardcho1231 added ready for review The PR author has completed the PR template and is ready for a review and removed needs changes The reviewer has requested changes from the PR author labels Mar 19, 2024

vgalatro added needs changes The reviewer has requested changes from the PR author and removed ready for review The PR author has completed the PR template and is ready for a review labels Mar 20, 2024

edwardcho1231 added 3 commits March 20, 2024 15:06

clean up

360d528

update tests

8e4f082

Merge remote-tracking branch 'origin/arc-themes-release-version-2.3.0…

4847c64

…' into ASUB-8192/captcha2_login

edwardcho1231 requested a review from vgalatro March 20, 2024 20:01

edwardcho1231 added ready for review The PR author has completed the PR template and is ready for a review and removed needs changes The reviewer has requested changes from the PR author labels Mar 20, 2024

vgalatro added needs changes The reviewer has requested changes from the PR author and removed ready for review The PR author has completed the PR template and is ready for a review labels Mar 22, 2024

edwardcho1231 added 4 commits March 22, 2024 14:24

update tests

57eb86f

fix lint errors

009228d

fix lint warnings

fdc4830

Merge branch 'arc-themes-release-version-2.3.0' into ASUB-8192/captch…

a84de20

…a2_login

edwardcho1231 added ready for review The PR author has completed the PR template and is ready for a review and removed needs changes The reviewer has requested changes from the PR author labels Mar 22, 2024

vgalatro approved these changes Mar 25, 2024

View reviewed changes

vgalatro added ready to merge It's time! Merge this PR. Woo! and removed ready for review The PR author has completed the PR template and is ready for a review labels Mar 25, 2024

vgalatro merged commit ff21f40 into arc-themes-release-version-2.3.0 Mar 25, 2024
3 of 5 checks passed

vgalatro deleted the ASUB-8192/captcha2_login branch March 25, 2024 13:28

LauraPinilla mentioned this pull request Apr 5, 2024

ASUB-8193 ASUB-8192 Google reCaptcha v2 & v3 WPMedia/arc-themes-components#251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asub 8192/captcha2 login #1978

Asub 8192/captcha2 login #1978

edwardcho1231 commented Mar 4, 2024 •

edited

Loading

LauraPinilla Mar 5, 2024

LauraPinilla Mar 6, 2024

vgalatro commented Mar 6, 2024

edwardcho1231 commented Mar 6, 2024

LauraPinilla Mar 6, 2024

edwardcho1231 Mar 6, 2024

LauraPinilla Mar 11, 2024

LauraPinilla Mar 7, 2024 •

edited

Loading

edwardcho1231 Mar 8, 2024

LauraPinilla Mar 11, 2024

vgalatro commented Mar 11, 2024

LauraPinilla Mar 12, 2024 •

edited

Loading

LauraPinilla Mar 12, 2024 •

edited

Loading

LauraPinilla Mar 12, 2024 •

edited

Loading

LauraPinilla left a comment

vgalatro commented Mar 13, 2024

vgalatro commented Mar 19, 2024

vgalatro commented Mar 22, 2024

vgalatro left a comment

vgalatro commented Mar 25, 2024

Asub 8192/captcha2 login #1978

Asub 8192/captcha2 login #1978

Conversation

edwardcho1231 commented Mar 4, 2024 • edited Loading

Description

Jira Ticket

Acceptance Criteria

Test Steps

Effect Of Changes

Before

After

Dependencies or Side Effects

Author Checklist

Reviewer Checklist

LauraPinilla Mar 5, 2024

Choose a reason for hiding this comment

LauraPinilla Mar 6, 2024

Choose a reason for hiding this comment

vgalatro commented Mar 6, 2024

edwardcho1231 commented Mar 6, 2024

LauraPinilla Mar 6, 2024

Choose a reason for hiding this comment

edwardcho1231 Mar 6, 2024

Choose a reason for hiding this comment

LauraPinilla Mar 11, 2024

Choose a reason for hiding this comment

LauraPinilla Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

edwardcho1231 Mar 8, 2024

Choose a reason for hiding this comment

LauraPinilla Mar 11, 2024

Choose a reason for hiding this comment

vgalatro commented Mar 11, 2024

LauraPinilla Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

LauraPinilla Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

LauraPinilla Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

LauraPinilla left a comment

Choose a reason for hiding this comment

vgalatro commented Mar 13, 2024

vgalatro commented Mar 19, 2024

vgalatro commented Mar 22, 2024

vgalatro left a comment

Choose a reason for hiding this comment

vgalatro commented Mar 25, 2024

edwardcho1231 commented Mar 4, 2024 •

edited

Loading

LauraPinilla Mar 7, 2024 •

edited

Loading

LauraPinilla Mar 12, 2024 •

edited

Loading

LauraPinilla Mar 12, 2024 •

edited

Loading

LauraPinilla Mar 12, 2024 •

edited

Loading