-
Notifications
You must be signed in to change notification settings - Fork 282
Introduce Google reCaptcha alternatives for IAM products #8199
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
base: feature-recaptcha-alternatives
Are you sure you want to change the base?
Introduce Google reCaptcha alternatives for IAM products #8199
Conversation
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. |
if (scriptAttributesList != null) { | ||
for (Map<String, String> scriptAttributes : scriptAttributesList) { |
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.
if (scriptAttributesList != null) { | |
for (Map<String, String> scriptAttributes : scriptAttributesList) { | |
if (scriptAttributesList != null) { | |
for (Map<String, String> scriptAttributes : scriptAttributesList) { |
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.
Pull Request Overview
This PR refactors the existing Google reCaptcha integration in various authentication and recovery JSP pages to a dynamic implementation using CaptchaFEUtils. Key changes include:
- Replacing hardcoded reCaptcha API URLs, keys, and response parameters with dynamic values from CaptchaFEUtils.
- Consistent rendering of captcha-related script and widget HTML using data from CaptchaFEUtils.
- Updating header and form submission logic across multiple JSP files to support the new captcha integration.
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
self-registration-without-verification.jsp | Replaces reCaptcha API/script inclusion and widget attributes with dynamic CaptchaFEUtils values. |
self-registration-with-verification.jsp | Updates captcha headers and widget rendering using CaptchaFEUtils in place of reCaptchaUtil calls. |
self-registration-username-request.jsp | Switches captcha header addition from reCaptcha to CaptchaFEUtils. |
password-recovery.jsp and related recovery pages | Dynamically injects captcha script attributes and response identifiers using CaptchaFEUtils. |
resend-confirmation-captcha.jsp, login.jsp, identifierauth.jsp, basicauth.jsp | Refactors captcha widget rendering, form submission, and reset functionality to use CaptchaFEUtils. |
Comments suppressed due to low confidence (1)
identity-apps-core/apps/authentication-portal/src/main/webapp/identifierauth.jsp:348
- [nitpick] Ensure that all instances where the captcha site key is rendered (for example, in the identifierauth.jsp file) have been thoroughly tested to guarantee that the dynamic CaptchaFEUtils values are correctly injected and that the resulting HTML complies with the expected widget behavior.
data-sitekey="<%=Encode.forHtmlContent(captchaKey)%>"
grecaptcha.reset(); | ||
} | ||
// Reset the captcha to allow another submission. | ||
CaptchaFEUtils.getCaptchaFunctions().get("reset"); |
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.
It appears that the captcha reset function is being retrieved but not invoked. Please ensure that the reset function is correctly called (for example, by appending '()' if it’s a callable function) to properly reset the captcha UI on subsequent submissions.
CaptchaFEUtils.getCaptchaFunctions().get("reset"); | |
CaptchaFEUtils.getCaptchaFunctions().get("reset")(); |
Copilot uses AI. Check for mistakes.
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.
CaptchaFEUtils.getCaptchaFunctions() returns a Map<String, String>, so get("reset") returns the function name as a string, not a callable. We’re just retrieving the function name here to be used in the frontend, not invoking it. Calling get("reset")() would throw an error since the value is a string, not a function.
@@ -32,6 +32,7 @@ | |||
<%@ page import="static org.wso2.carbon.identity.application.authentication.endpoint.util.Constants.ENABLE_AUTHENTICATION_WITH_REST_API" %> | |||
<%@ page import="static org.wso2.carbon.identity.application.authentication.endpoint.util.Constants.ERROR_WHILE_BUILDING_THE_ACCOUNT_RECOVERY_ENDPOINT_URL" %> | |||
<%@ page import="org.wso2.carbon.identity.captcha.util.CaptchaUtil" %> | |||
<%@ page import="org.wso2.carbon.identity.captcha.provider_mgt.util.CaptchaFEUtils" %> |
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.
Shall we rename this util to CaptchaUIUtils
?
data-sitekey="<%=Encode.forHtmlAttribute(reCaptchaKey)%>" | ||
<div | ||
<% | ||
Map<String, String> captchaAttributes = CaptchaFEUtils.getWidgetAttributes(); |
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.
Let's move the reusable logics to a utils or helpers.
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.
In this case, moving the logic to a helper or utility isn't straightforward because the code is tightly coupled with dynamic HTML rendering inside the JSP. Unlike JavaScript or modern templating engines, JSP doesn’t support returning reusable fragments easily — helpers would have to return raw HTML strings or write directly to the output stream, which is messy and not cleanly maintainable. While we could create custom JSP tags or tag files for better separation, that introduces additional complexity and multiple files just to handle small, context-specific logic blocks. Given that, keeping the current logic inline avoids overengineering and remains easier to follow for now in my point of view.
|
||
console.log("reCaptcha response: " + reCaptchaResponse); |
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.
Let's remove any unnecessary logs
…release identity-apps-myaccount-2.20.1
…for next development iteration
…lease branch release-action-3909
Add new developer checklist to the PR template
…tion Fix guest user deletion
[Release] [GitHub Action] Update package versions
…release identity-apps-console-2.62.1
…for next development iteration
…lease branch release-action-3914
[Release] [GitHub Action] Update package versions
…release identity-apps-console-2.65.5
…for next development iteration
…release identity-apps-myaccount-2.20.3
…for next development iteration
…release identity-apps-core-2.26.2
…for next development iteration
…lease branch release-action-3958
Enhancements to the Flows Feature
[Release] [GitHub Action] Update package versions
…release identity-apps-console-2.65.6
…for next development iteration
…lease branch release-action-3960
Fix minor content issues in the 'Invite User to Set Password' flow
[Release] [GitHub Action] Update package versions
…release identity-apps-console-2.65.7
…for next development iteration
…lease branch release-action-3962
Support configuring Recovery Portal URL via branding settings and apply in Forgot Password link
…to feature-CAPTCHA-alternatives
Purpose
Related Issues
Merge After
Related PRs