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 ReCaptcha coverage for Contact GraphQL (SwiftOtter's SOP-348) #335

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

lbajsarowicz
Copy link

Description (*)

According to the official documentation, Magento 2.4.7 comes without ReCaptcha coverage for Contact form GraphQL endpoint.

image

This Pull Requests fills this incomprehensible gap.

Fixed Issues (if relevant)

  1. magento/magento2#<issue_number>: Issue title

Manual testing scenarios (*)

GraphQL call to verify:

mutation {
  contactUs(
    input: {
      email: "[email protected]",
      name: "Test User",
      telephone: "1234567890",
      comment: "This is a test comment."
    }
  ) {
    status
  }
}

Questions or comments

Contribution checklist (*)

  • Author has signed the Adobe CLA
  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@lbajsarowicz lbajsarowicz force-pushed the SwiftOtter--SOP-348-contact-recaptcha-graphql branch from c7faa0e to b37730b Compare May 14, 2024 14:30
@lbajsarowicz
Copy link
Author

@magento run all tests

@lbajsarowicz lbajsarowicz changed the title Add missing ReCaptcha coverage for Contact GraphQL (SwiftOtter's SOP-348) Add ReCaptcha coverage for Contact GraphQL (SwiftOtter's SOP-348) May 14, 2024
@lbajsarowicz
Copy link
Author

@ihor-sviziev Can you take a look?

@lbajsarowicz
Copy link
Author

@ihor-sviziev @sivaschenko ?

@hostep
Copy link

hostep commented Jul 29, 2024

Tagging @nathanjosiah, maybe he'll be able to move this PR forward

public function getConfigFor(EndpointInterface $endpoint): ?ValidationConfigInterface
{
if ($endpoint->getServiceMethod() === 'resolve'
&& $endpoint->getServiceClass() === ContactUs::class) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see in other places we have a || here. are you sure it should be &&?
Example: https://github.com/magento/security-package/blob/develop/ReCaptchaCustomer/Model/WebapiConfigProvider.php#L65-L69

Copy link
Author

Choose a reason for hiding this comment

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

@ihor-sviziev After the research, I found out that \Magento\ReCaptchaSendFriend\Model\WebapiConfigProvider::getConfigFor is the most in-line with Contact form, that's why it's && not ||

<type name="Magento\ReCaptchaWebapiApi\Model\CompositeWebapiValidationConfigProvider">
<arguments>
<argument name="providers" xsi:type="array">
<item name="contactUs" xsi:type="object">Magento\ReCaptchaContact\Model\WebapiConfigProvider</item>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not "Contact", as a module name?

Suggested change
<item name="contactUs" xsi:type="object">Magento\ReCaptchaContact\Model\WebapiConfigProvider</item>
<item name="contact" xsi:type="object">Magento\ReCaptchaContact\Model\WebapiConfigProvider</item>

Copy link
Author

Choose a reason for hiding this comment

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

Introduced change in the codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants