Refactor isMultiAuthAvailable method to include current authenticator check#9725
Refactor isMultiAuthAvailable method to include current authenticator check#9725AfraHussaindeen wants to merge 1 commit intowso2:masterfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change extracts multi-auth availability logic into a new JSP utility and updates multiple authentication JSPs to include it. Local Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
identity-apps-core/apps/authentication-portal/src/main/webapp/emailOtp.jsp (1)
155-158:⚠️ Potential issue | 🟡 MinorDuplicate closing
</h2>tag.Line 158 has an extra
</h2>that should be removed.<h2> <%= i18n(resourceBundle, customText, "email.otp.heading") %> </h2> - </h2>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@identity-apps-core/apps/authentication-portal/src/main/webapp/emailOtp.jsp` around lines 155 - 158, In emailOtp.jsp the heading block contains a duplicate closing </h2> tag; open the file (emailOtp.jsp) and remove the extra </h2> immediately after the i18n heading expression so only one closing </h2> follows the <h2> that renders i18n(resourceBundle, customText, "email.otp.heading").identity-apps-core/apps/authentication-portal/src/main/webapp/identifierauth.jsp (1)
344-346:⚠️ Potential issue | 🟠 MajorInconsistent usage: single-argument
isMultiAuthAvailablebypasses step-ownership validation.Unlike other identifier-first flows (e.g.,
fido2-identifierfirst.jsp) which callisMultiAuthAvailable(multiOptionURI, request.getParameter("authenticators")), this file uses the single-argument overload that skips authenticator-matching validation. Theauthenticatorsparameter is available in the request stream and should be validated.Update to match the pattern used in other identifier-first flows:
Suggested change
- isMultiAuthAvailable(multiOptionURI)) { + isMultiAuthAvailable(multiOptionURI, request.getParameter("authenticators"))) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@identity-apps-core/apps/authentication-portal/src/main/webapp/identifierauth.jsp` around lines 344 - 346, The code calls AuthenticationEndpointUtil.isMultiAuthAvailable(multiOptionURI) which skips step-ownership/authenticator validation; change the call to the two-argument overload by passing the authenticators parameter from the request (request.getParameter("authenticators"))—i.e., replace isMultiAuthAvailable(multiOptionURI) with isMultiAuthAvailable(multiOptionURI, request.getParameter("authenticators")) after ensuring multiOptionURI is properly encoded (Encode.forJava) and AuthenticationEndpointUtil.isValidMultiOptionURI(multiOptionURI) remains checked.
🧹 Nitpick comments (1)
identity-apps-core/apps/authentication-portal/src/main/webapp/util/authenticator-utils.jsp (1)
38-38: Consider replacing magic number with inline comment.The offset
15is the length of"authenticators=". Adding a brief inline comment would improve clarity.- String authSubstring = multiOptionURI.substring(startIndex + 15); + String authSubstring = multiOptionURI.substring(startIndex + 15); // 15 = "authenticators=".length()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@identity-apps-core/apps/authentication-portal/src/main/webapp/util/authenticator-utils.jsp` at line 38, The substring call uses a magic number 15 which is the length of the literal "authenticators="; update the code around the authSubstring assignment (variable authSubstring and multiOptionURI.substring) to replace the hard-coded 15 with either multiOptionURI.indexOf("authenticators=") + "authenticators=".length() or add a brief inline comment after the 15 explaining it equals the length of "authenticators=" so future readers understand the offset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@identity-apps-core/apps/authentication-portal/src/main/webapp/util/authenticator-utils.jsp`:
- Around line 19-79: authenticator-utils.jsp uses StringUtils, Arrays, and List
inside the isMultiAuthAvailable(String,String) and isMultiAuthAvailable(String)
methods but lacks explicit imports; add JSP page import directives for
org.apache.commons.lang.StringUtils, java.util.Arrays, and java.util.List at the
top of authenticator-utils.jsp so the utility becomes self-contained and
identifierauth.jsp (which doesn't include localize.jsp) will compile.
---
Outside diff comments:
In `@identity-apps-core/apps/authentication-portal/src/main/webapp/emailOtp.jsp`:
- Around line 155-158: In emailOtp.jsp the heading block contains a duplicate
closing </h2> tag; open the file (emailOtp.jsp) and remove the extra </h2>
immediately after the i18n heading expression so only one closing </h2> follows
the <h2> that renders i18n(resourceBundle, customText, "email.otp.heading").
In
`@identity-apps-core/apps/authentication-portal/src/main/webapp/identifierauth.jsp`:
- Around line 344-346: The code calls
AuthenticationEndpointUtil.isMultiAuthAvailable(multiOptionURI) which skips
step-ownership/authenticator validation; change the call to the two-argument
overload by passing the authenticators parameter from the request
(request.getParameter("authenticators"))—i.e., replace
isMultiAuthAvailable(multiOptionURI) with isMultiAuthAvailable(multiOptionURI,
request.getParameter("authenticators")) after ensuring multiOptionURI is
properly encoded (Encode.forJava) and
AuthenticationEndpointUtil.isValidMultiOptionURI(multiOptionURI) remains
checked.
---
Nitpick comments:
In
`@identity-apps-core/apps/authentication-portal/src/main/webapp/util/authenticator-utils.jsp`:
- Line 38: The substring call uses a magic number 15 which is the length of the
literal "authenticators="; update the code around the authSubstring assignment
(variable authSubstring and multiOptionURI.substring) to replace the hard-coded
15 with either multiOptionURI.indexOf("authenticators=") +
"authenticators=".length() or add a brief inline comment after the 15 explaining
it equals the length of "authenticators=" so future readers understand the
offset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 516340fd-407e-4734-aed3-3868195b454d
📒 Files selected for processing (11)
identity-apps-core/apps/authentication-portal/src/main/webapp/emailOtp.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/enableTOTP.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/fido2-auth.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/fido2-identifierfirst.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/identifierauth.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/pushAuth.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/pushDeviceEnrollConsent.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/pushEnroll.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/smsOtp.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/totp.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/util/authenticator-utils.jsp
identity-apps-core/apps/authentication-portal/src/main/webapp/util/authenticator-utils.jsp
Show resolved
Hide resolved
AfraHussaindeen
force-pushed
the
master_multioption-uri
branch
from
12d4c6d to
49a878c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@identity-apps-core/apps/authentication-portal/src/main/webapp/util/authenticator-utils.jsp`:
- Around line 61-65: currentAuthenticator and entries in authList are
servlet-decoded (e.g., "FIDOAuthenticator:LOCAL") but the code splits on
percent-encoded delimiters ("%3A" and "%3B"), so comparisons always fail; update
all splits where "%3A" and "%3B" are used (e.g., the
currentAuthenticator.split("%3A") call and the auth -> auth.split("%3A")
mapping, and any split on "%3B" earlier around line 47) to use the decoded
delimiters ":" and ";" respectively, ensuring you trim/validate parts as needed
before comparing the authenticator names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: adbe4c60-4c63-4ca2-ae8a-cc94be4d4096
📒 Files selected for processing (11)
identity-apps-core/apps/authentication-portal/src/main/webapp/emailOtp.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/enableTOTP.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/fido2-auth.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/fido2-identifierfirst.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/identifierauth.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/pushAuth.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/pushDeviceEnrollConsent.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/pushEnroll.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/smsOtp.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/totp.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/util/authenticator-utils.jsp
🚧 Files skipped from review as they are similar to previous changes (3)
- identity-apps-core/apps/authentication-portal/src/main/webapp/totp.jsp
- identity-apps-core/apps/authentication-portal/src/main/webapp/identifierauth.jsp
- identity-apps-core/apps/authentication-portal/src/main/webapp/fido2-auth.jsp
identity-apps-core/apps/authentication-portal/src/main/webapp/util/authenticator-utils.jsp
Show resolved
Hide resolved
AfraHussaindeen
force-pushed
the
master_multioption-uri
branch
from
49a878c to
f99a73a
Compare
There was a problem hiding this comment.
Pull request overview
Refactors multi-auth availability detection across authentication JSPs to prevent stale multiOptionURI values from previous steps causing incorrect “choose different option” UI rendering.
Changes:
- Extracts
isMultiAuthAvailableinto a shared JSP util and adds “current authenticator belongs to this URI” verification. - Updates multiple authenticator JSPs to include the util and call the new overload with
request.getParameter("authenticators"). - Replaces ad-hoc null checks with
StringUtils.isBlank+ guard clauses and removes duplicated per-page helper implementations.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| identity-apps-core/apps/authentication-portal/src/main/webapp/util/authenticator-utils.jsp | Adds shared isMultiAuthAvailable helper with current-authenticator ownership check. |
| identity-apps-core/apps/authentication-portal/src/main/webapp/totp.jsp | Includes util and uses new overload to avoid stale multiOptionURI UI. |
| identity-apps-core/apps/authentication-portal/src/main/webapp/smsOtp.jsp | Includes util and switches to new overload with current authenticator. |
| identity-apps-core/apps/authentication-portal/src/main/webapp/pushEnroll.jsp | Includes util and switches to new overload with current authenticator. |
| identity-apps-core/apps/authentication-portal/src/main/webapp/pushDeviceEnrollConsent.jsp | Includes util and switches to new overload with current authenticator. |
| identity-apps-core/apps/authentication-portal/src/main/webapp/pushAuth.jsp | Includes util and switches to new overload with current authenticator. |
| identity-apps-core/apps/authentication-portal/src/main/webapp/identifierauth.jsp | Removes local helper and includes util. |
| identity-apps-core/apps/authentication-portal/src/main/webapp/fido2-identifierfirst.jsp | Removes local helper, includes util, and uses new overload. |
| identity-apps-core/apps/authentication-portal/src/main/webapp/fido2-auth.jsp | Removes local helper, includes util, and uses new overload. |
| identity-apps-core/apps/authentication-portal/src/main/webapp/enableTOTP.jsp | Removes local helper, includes util, and uses new overload. |
| identity-apps-core/apps/authentication-portal/src/main/webapp/emailOtp.jsp | Removes local helper, includes util, and uses new overload. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| */ | ||
| private boolean isMultiAuthAvailable(String multiOptionURI, String currentAuthenticator) { | ||
|
|
||
| if (StringUtils.isBlank(multiOptionURI) || "null".equals(multiOptionURI)) { |
There was a problem hiding this comment.
"null"is only filtered when it matches exactly; values like" null "or different casing (e.g.,"NULL"`) will pass this guard and be treated as a valid URI. Consider normalizing before comparison (e.g., trim + case-insensitive compare) so the “null string” protection is consistent with the intent described in the PR.
| if (StringUtils.isBlank(multiOptionURI) || "null".equals(multiOptionURI)) { | |
| if (StringUtils.isBlank(multiOptionURI) | |
| || StringUtils.equalsIgnoreCase("null", StringUtils.trim(multiOptionURI))) { |
| if (StringUtils.isNotBlank(currentAuthenticator)) { | ||
| String currentAuthName = currentAuthenticator.split(":")[0]; | ||
| if (authList.stream() | ||
| .map(auth -> auth.split("%3A")[0]) | ||
| .noneMatch(currentAuthName::equals)) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
This compares an unencoded delimiter in currentAuthenticator (":") against an encoded delimiter in authList ("%3A"). If currentAuthenticator is ever passed in encoded form (e.g., contains "%3A"), currentAuthName won’t be extracted correctly and the method will incorrectly return false. Consider normalizing currentAuthenticator to the same encoding form as authList (or decoding both) before splitting/comparing.
| return false; | ||
| } | ||
|
|
||
| int startIndex = multiOptionURI.indexOf("authenticators="); |
There was a problem hiding this comment.
There are several protocol “magic values” embedded inline ("authenticators=", the 15 offset, "%3B", "%3A", and the backup-code literal). This makes the parsing brittle and harder to update safely. Prefer deriving offsets from the actual key length (e.g., "authenticators=".length()) and centralizing these tokens as named constants (with a short comment on why the list is expected to be percent-encoded).
| return false; | ||
| } | ||
|
|
||
| String authSubstring = multiOptionURI.substring(startIndex + 15); |
There was a problem hiding this comment.
There are several protocol “magic values” embedded inline ("authenticators=", the 15 offset, "%3B", "%3A", and the backup-code literal). This makes the parsing brittle and harder to update safely. Prefer deriving offsets from the actual key length (e.g., "authenticators=".length()) and centralizing these tokens as named constants (with a short comment on why the list is expected to be percent-encoded).
|
|
||
| // Extract the authenticators list. | ||
| String authenticators = (endIndex != -1) ? authSubstring.substring(0, endIndex) : authSubstring; | ||
| List<String> authList = Arrays.asList(authenticators.split("%3B")); |
There was a problem hiding this comment.
There are several protocol “magic values” embedded inline ("authenticators=", the 15 offset, "%3B", "%3A", and the backup-code literal). This makes the parsing brittle and harder to update safely. Prefer deriving offsets from the actual key length (e.g., "authenticators=".length()) and centralizing these tokens as named constants (with a short comment on why the list is expected to be percent-encoded).
| } | ||
|
|
||
| // Backup codes are not considered a separate "option" for the UI toggle. | ||
| if (authList.size() == 2 && authList.contains("backup-code-authenticator%3ALOCAL")) { |
There was a problem hiding this comment.
There are several protocol “magic values” embedded inline ("authenticators=", the 15 offset, "%3B", "%3A", and the backup-code literal). This makes the parsing brittle and harder to update safely. Prefer deriving offsets from the actual key length (e.g., "authenticators=".length()) and centralizing these tokens as named constants (with a short comment on why the list is expected to be percent-encoded).
| ~ under the License. | ||
| --%> | ||
|
|
||
| <%@ page import="org.apache.commons.lang.StringUtils" %> |
There was a problem hiding this comment.
Please double-check that this module uses Commons Lang 2 (org.apache.commons.lang.StringUtils) rather than Commons Lang 3 (org.apache.commons.lang3.StringUtils). If the runtime only includes Lang 3 (common in newer builds), this import will cause JSP compilation failures; switching to the correct package would prevent that.
| <%@ page import="org.apache.commons.lang.StringUtils" %> | |
| <%@ page import="org.apache.commons.lang3.StringUtils" %> |
Purpose
This pull request refactors the logic for determining the availability of multi-authentication options across authentication JSP pages. It addresses a specific state-leakage issue where a multiOptionURI from a previous authentication step could "bleed" into a subsequent single-authenticator step, causing the UI to incorrectly display a "Choose a different option" link.
Key changes include:
The isMultiAuthAvailable method in all relevant JSP files extarcted to util class and refactored to:
Accept a currentAuthenticator parameter to validate that the multiOptionURI belongs to the current authentication step.
Background:
The new check extracts the authenticator list from multiOptionURI and verifies that currentAuth (the current step's authenticator, obtained via request.getParameter("authenticators")) is present in it. If absent, the URI belongs to a prior step and is disregarded.
Use StringUtils.isBlank instead of a plain null check, also guarding against whitespace-only and "null" string values.
Apply a guard-clause pattern with early returns, eliminating the mutable isMultiAuthAvailable flag variable and reducing nesting.
Related Issue
CC : @indeewari