Introduce debug feature for FIdP#10277
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an end-to-end Identity Provider connection testing feature: debug endpoints and API client, a "Test Connection" action in the connection edit page, a Connection Test results page with caching/polling and three result tabs, styled UI components, JSP popup pages, and routing/config updates. ChangesIdentity Provider Connection Testing
Suggested reviewers:
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (6)
identity-apps-core/apps/authentication-portal/src/main/webapp/debugSuccess.jsp (1)
21-22: 💤 Low valueUnused imports
EncodeandStringEscapeUtils.Neither symbol is referenced anywhere in the JSP. Drop them, or actually use
Encode.forHtml(...)/Encode.forJavaScript(...)if there is text from request attributes that should be encoded (as needed indebugError.jsp).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@identity-apps-core/apps/authentication-portal/src/main/webapp/debugSuccess.jsp` around lines 21 - 22, Remove the unused imports Encode and StringEscapeUtils from the JSP or replace their removal by applying proper encoding where needed; specifically either delete the lines importing org.owasp.encoder.Encode and org.apache.commons.text.StringEscapeUtils, or locate uses of request attributes/output in debugSuccess.jsp and wrap them with Encode.forHtml(...) or Encode.forJavaScript(...) (or StringEscapeUtils equivalents) to properly escape user-controlled content before rendering.features/admin.connections.v1/pages/connection-edit.tsx (1)
775-785: ⚡ Quick winLocalize the button label and confirm the icon choice.
The action button copy
Test Connectionand the<Icon name="flask"/>glyph are hardcoded:
- Wrap the label in
t(...)against anauthenticationProvider:namespace key consistent with the surrounding tabs/buttons in this page.- For new UI introduced in this PR, the guideline is to use icons from
@oxygen-ui/react-icons;semantic-ui-react'sIcon name="flask"is being added fresh here. Either reuse an existing Oxygen icon (e.g., a "play/test" semantic) or keepsemantic-ui-reactconsistent with the rest of the page's chrome and note the decision.As per coding guidelines: "Use i18next via
@wso2is/i18nwith namespace-based keys in format 'namespace:path.to.key'" and "Import icons from@oxygen-ui/react-icons".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/admin.connections.v1/pages/connection-edit.tsx` around lines 775 - 785, Replace the hardcoded button label and icon: wrap "Test Connection" with the i18n translator t(...) using the authenticationProvider namespace (e.g. t('authenticationProvider:connection.testButton')) where the SecondaryButton is rendered (see SecondaryButton, handleTestConnection, isTestingConnection, ConnectionsManagementUtils.isConnectorIdentityProvider); and replace the Semantic UI Icon name="flask" with an icon imported from `@oxygen-ui/react-icons` (choose an appropriate symbol such as PlayArrow or a test-related icon) to follow the project's icon library guidelines, or if you intentionally keep semantic-ui-react, note that decision in the PR description.features/admin.connections.v1/pages/connection-test.tsx (4)
70-127: ⚡ Quick winReplace raw
window.localStoragewith the sharedLocalStorageUtilswrapper.
getCachedResult/cacheResult/clearCachedResultgo directly towindow.localStorage, which:
- skips the standardized JSON envelope/error handling pattern used elsewhere in the console,
- makes it harder to centrally clear test caches on tenant switch / sign-out,
- requires the per-call
typeof window === "undefined"guard you already write.Use
LocalStorageUtils(orSessionStorageUtilsif these results shouldn't survive a browser restart — debug results probably shouldn't persist across sessions).As per coding guidelines: "Use wrapper utilities from
@wso2is/core/utils(SessionStorageUtils,LocalStorageUtils) instead of raw browser APIs".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/admin.connections.v1/pages/connection-test.tsx` around lines 70 - 127, Replace direct window.localStorage usage in getCachedResult, cacheResult, and clearCachedResult with the shared wrapper (LocalStorageUtils or SessionStorageUtils) so the standardized JSON envelope and centralized clearing work; specifically, call the wrapper's getItem/setItem/removeItem (or equivalent) using the same resultCacheKey, remove the manual JSON.parse/JSON.stringify and the typeof window guards if the utility already handles SSR, and update imports to pull LocalStorageUtils/SessionStorageUtils from `@wso2is/core/utils` so getCachedResult, cacheResult, and clearCachedResult use the utility API instead of window.localStorage.
52-55: 💤 Low valueReplace the inline SVG with an icon from
@oxygen-ui/react-icons.
RotateIconrolls its own SVG markup for a refresh/rotate glyph. Oxygen UI already exposes refresh-style icons that match the design system; importing one keeps stroke/fill behavior, accessibility, and sizing consistent with the rest of the console, and removes ~50 chars of inlined path data.As per coding guidelines: "Import icons from
@oxygen-ui/react-icons".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/admin.connections.v1/pages/connection-test.tsx` around lines 52 - 55, The RotateIcon component should stop inlining raw SVG and instead import and render the matching icon from `@oxygen-ui/react-icons`; replace the RotateIcon function body (the inline <svg> path) with a direct render of the appropriate exported icon (e.g., Refresh/RefreshIcon from `@oxygen-ui/react-icons`), add any sizing/props used elsewhere (width/height or fontSize) and accessibility props (aria-hidden or role/title as required), and remove the large path data; keep the same component name RotateIcon so call sites remain unchanged.
316-363: ⚡ Quick winTighten the result-status evaluation and consider extracting it.
A couple of small things in this effect:
- Line 340:
steps.accountLinkingStatus && steps.accountLinkingStatus !== "success" && steps.accountLinkingStatus !== "pending"is missing the parentheses that wrap the other three branches. JS precedence makes it functionally equivalent (&&binds tighter than||), but the visual asymmetry is easy to misread when extending this list.- The whole
result → {hasError, hasPartial, activeTab}derivation is pure and can be a memoized helper (useMemo) plus a tinygetStatusForResult(result)function in autils/file — easier to unit-test the partial/failure rules, and avoids re-deriving on unrelated re-renders.setActiveTab(2)referencing the Diagnosis tab by numeric index is fragile; defineenum ConnectionTestTab { ID_TOKEN = 0, CLAIM_MAPPINGS = 1, DIAGNOSIS = 2 }and use it both here and in<Tabs value={...}>below.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/admin.connections.v1/pages/connection-test.tsx` around lines 316 - 363, The effect that derives hasError/hasPartial/activeTab from result is fragile and noisy: add missing parentheses around the accountLinkingStatus condition in the hasStepError check to match the other branches (reference: steps.accountLinkingStatus in the useEffect), extract the pure derivation into a testable helper getStatusForResult(result) (or move to utils/) and memoize it with useMemo to avoid recomputing on unrelated renders, and replace magic numeric tab index 2 with a named enum ConnectionTestTab { ID_TOKEN = 0, CLAIM_MAPPINGS = 1, DIAGNOSIS = 2 } then call setActiveTab(ConnectionTestTab.DIAGNOSIS) (update Tabs value usage accordingly) so the intent is clear and maintainable.
419-1078: 🏗️ Heavy liftLocalize all user-facing strings on this page.
A large number of strings rendered on this page are hardcoded English:
- Tab labels:
"ID Token","Claim Mappings","Diagnosis".- Empty/info states:
"Unable to decode token or not a valid JWT.","No claim mappings configured.","No log information available.","No diagnostic message provided.","Timestamp unavailable".- Table headers:
"Local Claim (URI)","IDP Claim","Value".- Status banner:
"Test Failed"/"Test Passed Partially"/"Test Passed"and their descriptions.- Error/loading copy:
"Error","Retry","Loading test results...","Waiting for authentication to complete...","Connection ID is missing. Cannot run test.","Failed to fetch debug results.","Failed to run test.","Unable to retrieve test results for this session.","No debug session id found...","Test Results","Results for the connection test session.","Rerun Test".Move all of these into the i18n namespaces (likely
authenticationProvider:/console:develop.pages.idpTest.*to mirror the back-button key already present at line 986). Hardcoded copy here will leak through to translated builds and downstream consumers of@wso2is/i18n.As per coding guidelines: "Use
useTranslation()fromreact-i18nextand access keys with format<namespace>:<path.to.key>".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/admin.connections.v1/pages/connection-test.tsx` around lines 419 - 1078, Page contains many hardcoded user strings; import and use useTranslation() and replace all literal strings in renderResultsTabs/tabPanes (tab labels in tabPanes with keys "id-token", "claim-mappings", "diagnosis"), decodeJWT alert message, claim mappings empty state, table headers ("Local Claim (URI)","IDP Claim","Value"), diagnosis messages ("No log information available.","No diagnostic message provided.","Timestamp unavailable"), status banner texts and descriptions, error/loading card labels ("Error","Retry","Loading test results...","Waiting for authentication to complete..."), page title/description and action button ("Test Results","Results for the connection test session.","Rerun Test") with t("console:develop.pages.idpTest.<key>") or appropriate authenticationProvider: keys; add matching keys to i18n namespaces, keep existing data-testid and behavior unchanged, and ensure imports (useTranslation) and use of t are added near the component (e.g., where renderResultsTabs, decodeJWT, formatLogs are defined).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/console/src/configs/routes.tsx`:
- Line 591: The parent route object identityProviders currently has exact: false
which is inconsistent with the sibling applications route; update the routing so
matching for child routes is driven by the child definitions (e.g.,
identityProviderTest) not by making the parent non-exact: either change
identityProviders to exact: true to match applications, or if the test child
needs looser matching, set its own path/exact appropriately (adjust
identityProviderTest's path or exact flag to explicitly allow
/connections/:id/test) so parent exactness is consistent and route matching for
/connections/:id and /connections/:id/test behaves correctly.
In `@features/admin.connections.v1/configs/endpoints.ts`:
- Around line 41-42: The two endpoint fields debug and debugResult in the
endpoints config are identical causing redundancy and confusion; either remove
debugResult from the interface and all usages (since consumer pages use
resourceEndpoints.debug for both `${resourceEndpoints.debug}/idp` and
`${resourceEndpoints.debug}/${sid}/result`), or change debugResult to the actual
result URL pattern (e.g., include the `{sid}/result` segment) and update the
consumer pages to use resourceEndpoints.debug for the base and
resourceEndpoints.debugResult for the result retrieval; modify the endpoints
object (debug, debugResult) and the interface that defines them, and update any
consumer references to use the chosen single source of truth
(resourceEndpoints.debug or resourceEndpoints.debugResult) consistently.
In `@features/admin.connections.v1/pages/connection-edit.tsx`:
- Around line 696-759: handleTestConnection (and similar functions like
fetchResult/handleRunTests in connection-test.tsx) currently imports axios
directly, builds hardcoded tenant paths, uses English strings, and types the
catch as any; fix by extracting the POST call into a small api helper (e.g.,
api/connection-debug.ts) that uses AsgardeoSPAClient.getInstance().httpRequest
to call `${resourceEndpoints.debug}/idp`, replace manual path construction with
AppConstants.getPaths().get("IDP_TEST").replace(":id", connector.id) and use
AppConstants.getTenant() for tenant resolution, replace hardcoded messages
("Connection ID is not available.", "Debug session data is incomplete.") with
i18n keys via t("authenticationProvider:notifications.getIDP.*"), and change
catch (error: any) to catch (error: unknown) and narrow it with an
AxiosError/type-guard before reading response.data/message.
In `@features/admin.connections.v1/pages/connection-test.tsx`:
- Around line 384-417: renderCodeBlock and the surrounding table/accordion
renderers use many hardcoded hex colors and fixed spacings; replace those with
Oxygen UI styled components using MUI's styled API (e.g., create StyledCodeBlock
= styled(Box)(({ theme }) => ({ background: theme.palette.background.paper,
border: `1px solid ${theme.palette.divider}`, borderRadius:
theme.shape.borderRadius, padding: theme.spacing(1.5), fontFamily: 'monospace',
fontSize: 13, color: theme.palette.text.primary, whiteSpace: 'pre-wrap',
wordBreak: 'break-word' })) and similar StyledCard/StyledCell components, pull
success/warning/error backgrounds from
theme.palette.success.light/warning.light/error.light, use theme.spacing() for
margins and theme.shape.borderRadius for radii, and reduce inline sx usage to
only 2–3 layout props; update renderCodeBlock to return the styled component and
replace all hardcoded hex values in the table/accordion renderers with theme
palette references.
- Around line 263-313: Guard against null/undefined response shape and missing
debugId: replace direct access to response.data.metadata.authorizationUrl with
optional chaining (response.data?.metadata?.authorizationUrl) and validate
newDebugId after parsing (const newDebugId = response.data?.debugId) before
calling setDebugId, fetchResult or starting timers; if newDebugId is falsy, call
clearTimers(), setError with a helpful message and return early instead of
proceeding to open the popup or schedule fetches. Also ensure
popupInterval.current and fetchTimer.current are only created/cleared when
newDebugId is valid to avoid scheduling calls to fetchResult(undefined).
- Around line 368-376: Replace the hard-coded back URL in handleBackButtonClick
with the canonical path from AppConstants: construct the tenantDomain as
currently done, then call
history.push(AppConstants.getPaths().get("IDP_EDIT").replace("{tenantDomain}",
tenantDomain).replace("{id}", connectorId)) (or otherwise interpolate
connectorId into the returned pattern) so the code uses
AppConstants.getPaths().get("IDP_EDIT") instead of the literal
`/t/${tenantDomain}/console/connections/${connectorId}`; keep tenant extraction
and connectorId usage intact.
- Around line 488-498: The code mutates result.metadata.mappedClaims by calling
claimsArray.sort in place; fix by cloning before sorting (e.g., create
sortedClaims from [...(result?.metadata?.mappedClaims ?? [])].sort(...)) so the
original cached API array is not mutated, and add explicit types for the items
used in the comparator (introduce/consume a MappedClaim interface in models/ and
annotate claimsArray and the comparator parameters a: MappedClaim, b:
MappedClaim) to remove any/implicit typing.
- Around line 63-67: Replace the banned React.FC typing and any/implicit types:
declare a props interface named ConnectionTestPagePropsInterface that extends
IdentifiableComponentInterface and RouteComponentProps<RouteParams>, add a
ConnectionTestLocationStateInterface (debugId?: string; authorizationUrl?:
string) and type locationState as ConnectionTestLocationStateInterface |
undefined, change the component signature to
FunctionComponent<ConnectionTestPagePropsInterface> returning ReactElement
(ConnectionTestPage), add explicit useState generics for debugId (string|null),
result (Record<string, unknown>|null) or a more specific shape, error
(string|null), loading (boolean), hasError (boolean), hasPartial (boolean),
activeTab (number), autoRunTriggered (boolean), isStatusBannerVisible (boolean),
expandedDiagnosticLogs (number[]), and replace useRef<any>(null) with typed refs
like useRef<NodeJS.Timeout | null>(null) for popupInterval and fetchTimer (or
appropriate timer type).
- Around line 153-193: The useEffect in the test page currently calls
window.open(state.authorizationUrl, "_blank") from an async/automatic context
which triggers popup blocking; move the popup open logic into the user click
handler on the edit page (handleTestConnection) so the popup is opened
synchronously before calling history.push to the test page and pass only debugId
in location state; update the test page to only read debugId from locationState
and poll/fetchResult without re-opening the popup (identifiers: useEffect,
locationState, autoRunTriggered, debugId, fetchResult, popupInterval,
fetchTimer). Also make the authorizationUrl access defensive by changing
response.data.metadata.authorizationUrl to use optional chaining
(response.data?.metadata?.authorizationUrl) where the edit page reads it to
avoid NPEs.
In
`@identity-apps-core/apps/authentication-portal/src/main/webapp/debugError.jsp`:
- Around line 1-11: Replace the abridged license block with the full WSO2
Apache-2.0 boilerplate to match sibling JSPs; add the JSP page directive <%@
page language="java" contentType="text/html; charset=UTF-8" pageEncoding="UTF-8"
%> at the top to set response content-type/encoding; remove the unused import of
org.apache.commons.text.StringEscapeUtils (it is currently unused in this file);
and ensure AuthenticationEndpointUtil is available by confirming
includes/localize.jsp exposes it or by adding an explicit import for
AuthenticationEndpointUtil in this JSP if it is not provided transitively so
references to AuthenticationEndpointUtil compile.
- Around line 50-54: The "Back to Connection" button label is hardcoded; replace
the literal text with the i18n helper used elsewhere by calling
AuthenticationEndpointUtil.i18n(...) for that label so it is translated
consistently (update the button with id "backBtn" to render the localized string
via AuthenticationEndpointUtil.i18n with an appropriate key or the English text
as the default).
- Around line 85-121: The script currently calls window.close() unconditionally
(after wiring document.getElementById("backBtn").onclick) which will close popup
windows immediately and make the "Back to Connection" button unreachable; change
the code to only auto-close when the page is a popup (e.g., if window.opener is
truthy) or remove the auto-close entirely so the user can click the backBtn that
uses connectionUrl; also fix the catch block by guarding the
document.getElementById("json") reference (check for non-null before assigning
textContent) to avoid a secondary TypeError hiding the original error.
- Around line 90-105: The IIFE that computes baseUrl injects unescaped JSP
attributes (consoleUrlAttr, tenanted, serverUrl, tenantPrefix) into
single-quoted JS strings which risks XSS and syntax breakage; update each
attribute injection to use OWASP encoding (e.g., Encode.forJavaScript(<%=
request.getAttribute(...) %>)) like debugSuccess.jsp does, so consoleUrlAttr,
tenanted, serverUrl and tenantPrefix are safely escaped before use in the
baseUrl function; also remove the hardcoded "/t/carbon.super" fallback in the
baseUrl IIFE and make the fallback fail closed (e.g., return an empty string or
null) so tenanted deployments are not forced to that path.
In
`@identity-apps-core/apps/authentication-portal/src/main/webapp/debugSuccess.jsp`:
- Around line 119-254: This file contains two complete JSP pages; remove the
first duplicate document (the top half up to before the second <!doctype html>)
so only the intended debug-success page remains (keep the variant that uses
AuthenticationEndpointUtil.i18n("Debug Request was Completed"), the countdown
element with id="countdown", and the loadFunc() that decrements seconds from 2
and calls window.close()). Ensure you delete the earlier duplicated directives,
taglibs, header/footer includes, layout:main block, and the first loadFunc()
definition so there is only one set of page directives and one
loadFunc/countdown implementation.
- Around line 202-205: The i18n text is split around the countdown span which
breaks translation ordering and leaves the " s" unit unlocalized; replace the
fragmented message with a single localized template via
AuthenticationEndpointUtil.i18n that includes a placeholder for the seconds
(e.g., "You will be redirected to results page in {0}") or a full localized
format including the unit, then inject the numeric value into the placeholder
(keep the span id="countdown" for JS updates) so translators can
reorder/localize the unit correctly; ensure the template string used by
AuthenticationEndpointUtil.i18n is updated accordingly.
- Around line 235-252: The countdown currently decrements before the first paint
in loadFunc, so the initial "2" is skipped; fix by writing the initial value to
countdownElem before starting the interval (i.e., after obtaining countdownElem
set countdownElem.textContent = seconds), then start the setInterval as-is (or
alternatively move the decrement to after updating the DOM inside the interval)
so the displayed value (seconds) is visible for the first second; refer to
loadFunc, seconds, countdownElem, interval, and window.close when making the
change.
---
Nitpick comments:
In `@features/admin.connections.v1/pages/connection-edit.tsx`:
- Around line 775-785: Replace the hardcoded button label and icon: wrap "Test
Connection" with the i18n translator t(...) using the authenticationProvider
namespace (e.g. t('authenticationProvider:connection.testButton')) where the
SecondaryButton is rendered (see SecondaryButton, handleTestConnection,
isTestingConnection, ConnectionsManagementUtils.isConnectorIdentityProvider);
and replace the Semantic UI Icon name="flask" with an icon imported from
`@oxygen-ui/react-icons` (choose an appropriate symbol such as PlayArrow or a
test-related icon) to follow the project's icon library guidelines, or if you
intentionally keep semantic-ui-react, note that decision in the PR description.
In `@features/admin.connections.v1/pages/connection-test.tsx`:
- Around line 70-127: Replace direct window.localStorage usage in
getCachedResult, cacheResult, and clearCachedResult with the shared wrapper
(LocalStorageUtils or SessionStorageUtils) so the standardized JSON envelope and
centralized clearing work; specifically, call the wrapper's
getItem/setItem/removeItem (or equivalent) using the same resultCacheKey, remove
the manual JSON.parse/JSON.stringify and the typeof window guards if the utility
already handles SSR, and update imports to pull
LocalStorageUtils/SessionStorageUtils from `@wso2is/core/utils` so
getCachedResult, cacheResult, and clearCachedResult use the utility API instead
of window.localStorage.
- Around line 52-55: The RotateIcon component should stop inlining raw SVG and
instead import and render the matching icon from `@oxygen-ui/react-icons`; replace
the RotateIcon function body (the inline <svg> path) with a direct render of the
appropriate exported icon (e.g., Refresh/RefreshIcon from
`@oxygen-ui/react-icons`), add any sizing/props used elsewhere (width/height or
fontSize) and accessibility props (aria-hidden or role/title as required), and
remove the large path data; keep the same component name RotateIcon so call
sites remain unchanged.
- Around line 316-363: The effect that derives hasError/hasPartial/activeTab
from result is fragile and noisy: add missing parentheses around the
accountLinkingStatus condition in the hasStepError check to match the other
branches (reference: steps.accountLinkingStatus in the useEffect), extract the
pure derivation into a testable helper getStatusForResult(result) (or move to
utils/) and memoize it with useMemo to avoid recomputing on unrelated renders,
and replace magic numeric tab index 2 with a named enum ConnectionTestTab {
ID_TOKEN = 0, CLAIM_MAPPINGS = 1, DIAGNOSIS = 2 } then call
setActiveTab(ConnectionTestTab.DIAGNOSIS) (update Tabs value usage accordingly)
so the intent is clear and maintainable.
- Around line 419-1078: Page contains many hardcoded user strings; import and
use useTranslation() and replace all literal strings in
renderResultsTabs/tabPanes (tab labels in tabPanes with keys "id-token",
"claim-mappings", "diagnosis"), decodeJWT alert message, claim mappings empty
state, table headers ("Local Claim (URI)","IDP Claim","Value"), diagnosis
messages ("No log information available.","No diagnostic message
provided.","Timestamp unavailable"), status banner texts and descriptions,
error/loading card labels ("Error","Retry","Loading test results...","Waiting
for authentication to complete..."), page title/description and action button
("Test Results","Results for the connection test session.","Rerun Test") with
t("console:develop.pages.idpTest.<key>") or appropriate authenticationProvider:
keys; add matching keys to i18n namespaces, keep existing data-testid and
behavior unchanged, and ensure imports (useTranslation) and use of t are added
near the component (e.g., where renderResultsTabs, decodeJWT, formatLogs are
defined).
In
`@identity-apps-core/apps/authentication-portal/src/main/webapp/debugSuccess.jsp`:
- Around line 21-22: Remove the unused imports Encode and StringEscapeUtils from
the JSP or replace their removal by applying proper encoding where needed;
specifically either delete the lines importing org.owasp.encoder.Encode and
org.apache.commons.text.StringEscapeUtils, or locate uses of request
attributes/output in debugSuccess.jsp and wrap them with Encode.forHtml(...) or
Encode.forJavaScript(...) (or StringEscapeUtils equivalents) to properly escape
user-controlled content before rendering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a9863fd1-3fc8-458a-9d76-d31afa11c96c
📒 Files selected for processing (9)
apps/console/src/configs/routes.tsxfeatures/admin.connections.v1/configs/endpoints.tsfeatures/admin.connections.v1/models/endpoints.tsfeatures/admin.connections.v1/pages/connection-edit.tsxfeatures/admin.connections.v1/pages/connection-test.tsxfeatures/admin.core.v1/constants/app-constants.tsfeatures/admin.core.v1/store/reducers/config.tsidentity-apps-core/apps/authentication-portal/src/main/webapp/debugError.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/debugSuccess.jsp
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
identity-apps-core/apps/authentication-portal/src/main/webapp/debugError.jsp (1)
113-116:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded
/t/carbon.superfallback is inappropriate for tenanted deployments.The baseUrl IIFE falls back to
window.location.origin + "/t/carbon.super"when no server attributes are provided. This forces multi-tenant deployments to the super-tenant path, which will break navigation for users in other tenants. Consider failing closed (return empty string or null) or using a tenant-aware fallback if available from the session context.🛡️ Suggested guard
return (window.location && window.location.origin ? window.location.origin - : (window.location.protocol + "//" + window.location.hostname + (window.location.port ? ":" + window.location.port : ""))) - + "/t/carbon.super"; + : (window.location.protocol + "//" + window.location.hostname + (window.location.port ? ":" + window.location.port : "")));Or if a tenant qualifier is always required, return
nulland guardconnectionUrlconstruction below.Based on learnings: Enforce consistent tenant URL pattern across the codebase: all tenant-scoped paths use /t//, including carbon.super as the super-tenant. Avoid assuming a root path for multi-tenant environments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@identity-apps-core/apps/authentication-portal/src/main/webapp/debugError.jsp` around lines 113 - 116, The current baseUrl IIFE returns a hardcoded "/t/carbon.super" fallback which forces multi-tenant users into the super-tenant; update the IIFE (the baseUrl computation) to avoid returning "/t/carbon.super" by default—either return null/empty string or derive the tenant qualifier from a session/context variable if available, and update any downstream usage (e.g., connectionUrl construction) to guard against a null/empty baseUrl; specifically modify the baseUrl IIFE expression so it fails closed (null/""), or uses a tenant-aware value from sessionTenant (or equivalent) instead of the hardcoded "/t/carbon.super", and ensure code that builds connectionUrl checks for the new null/empty value before concatenation.
🧹 Nitpick comments (4)
features/admin.connections.v1/pages/connection-test.tsx (1)
771-771: ⚡ Quick winReplace
anywith the proper diagnostic log interface.Line 771 uses
anyfor thelogparameter in thediagnostics.mapcallback, which violates the no-anyguideline. Use the already-definedConnectionTestDiagnosticLogInterfaceinstead.♻️ Proposed fix
- { diagnostics.map((log: any, idx: number) => { + { diagnostics.map((log: ConnectionTestDiagnosticLogInterface, idx: number) => {As per coding guidelines: "Never use
any; use proper types orunknownwith type guards".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/admin.connections.v1/pages/connection-test.tsx` at line 771, The map callback uses an `any` type for `log`; replace it with the specific interface `ConnectionTestDiagnosticLogInterface` by changing the signature in the diagnostics.map call (e.g., `diagnostics.map((log: ConnectionTestDiagnosticLogInterface, idx: number) => { ... })`) and ensure `ConnectionTestDiagnosticLogInterface` is imported or available in this module; also update any local destructuring/uses of `log` to match the interface fields and remove other `any` usages tied to this mapping.identity-apps-core/apps/authentication-portal/src/main/webapp/debugError.jsp (1)
22-22: 💤 Low valueRemove unused import.
StringEscapeUtilsis imported on line 22 but never used in this file. Remove it to keep the imports clean.♻️ Proposed fix
<%@ page import="org.owasp.encoder.Encode" %> -<%@ page import="org.apache.commons.text.StringEscapeUtils" %> <%@ page language="java" contentType="text/html; charset=UTF-8" pageEncoding="UTF-8"%>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@identity-apps-core/apps/authentication-portal/src/main/webapp/debugError.jsp` at line 22, Remove the unused import of StringEscapeUtils from debugError.jsp: locate the page/import directive that references StringEscapeUtils (the import of org.apache.commons.lang3.StringEscapeUtils or similar) and delete that import line so the JSP no longer imports an unused class; keep the other directives like the jsp:directive.include for includes/init-url.jsp unchanged.features/admin.connections.v1/pages/connection-edit.tsx (1)
769-779: ⚡ Quick winReplace
data-testidwithdata-componentidand internationalize the button label.Line 771 uses the deprecated
data-testidattribute; per coding guidelines, new components should usedata-componentid. Additionally, the "Test Connection" label on line 777 is hardcoded English and should be wrapped int(...)to support i18n.♻️ Proposed fix
action={ ConnectionsManagementUtils.isConnectorIdentityProvider(connector) ? ( <SecondaryButton - data-testid="idp-test-connection-button" + data-componentid="idp-test-connection-button" onClick={ handleTestConnection } loading={ isTestingConnection } disabled={ isTestingConnection } > <Icon name="flask"/> - Test Connection + { t("authenticationProvider:buttons.testConnection") } </SecondaryButton> ) : null }Based on learnings: In the identity-apps repository, prefer using the data-componentid attribute for component identification in tests. The data-testid attribute is deprecated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/admin.connections.v1/pages/connection-edit.tsx` around lines 769 - 779, Replace the deprecated data-testid with data-componentid on the SecondaryButton used in the ConnectionsManagementUtils.isConnectorIdentityProvider branch and internationalize its label by wrapping "Test Connection" with the i18n translator (t(...)); update the attribute on the SecondaryButton rendered in that branch (the element using handleTestConnection, isTestingConnection and loading/disabled props) and ensure the translator function (e.g., t from useTranslation) is in scope or imported so the button label is rendered via t('...') instead of a hardcoded string.features/admin.connections.v1/api/connection-test-api.ts (1)
92-99: ⚡ Quick winStrengthen the type guard by checking the
isAxiosErrorvalue.The type guard checks for the presence of the
isAxiosErrorproperty but not its value. Axios setsisAxiosError: trueon error instances, so verifying the boolean value would make the guard more precise.♻️ Suggested improvement
const isConnectionTestAxiosError = ( error: unknown ): error is AxiosError<ConnectionTestErrorResponseInterface> => { const candidate: AxiosError<ConnectionTestErrorResponseInterface> = error as AxiosError<ConnectionTestErrorResponseInterface>; - return Boolean(candidate && typeof candidate === "object" && "isAxiosError" in candidate); + return Boolean( + candidate && + typeof candidate === "object" && + "isAxiosError" in candidate && + candidate.isAxiosError === true + ); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/admin.connections.v1/api/connection-test-api.ts` around lines 92 - 99, The type guard isConnectionTestAxiosError currently only checks for the presence of isAxiosError; update it to verify the property value is true to make the guard precise (e.g., after casting to candidate: AxiosError<ConnectionTestErrorResponseInterface>, ensure candidate && typeof candidate === "object" && candidate.isAxiosError === true). Keep the same return type and use the existing candidate variable and AxiosError<ConnectionTestErrorResponseInterface> type in the condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@features/admin.connections.v1/api/connection-test-api.ts`:
- Around line 24-26: The httpClient initialization binds
AsgardeoSPAClient.getInstance().httpRequest twice which is redundant; update the
initialization of httpClient to bind httpRequest only once to the
AsgardeoSPAClient instance (use
AsgardeoSPAClient.getInstance().httpRequest.bind(AsgardeoSPAClient.getInstance()))
so the httpRequest method has the correct this context without the extra .bind()
call; reference AsgardeoSPAClient, httpRequest, and the httpClient variable when
making the change.
---
Duplicate comments:
In
`@identity-apps-core/apps/authentication-portal/src/main/webapp/debugError.jsp`:
- Around line 113-116: The current baseUrl IIFE returns a hardcoded
"/t/carbon.super" fallback which forces multi-tenant users into the
super-tenant; update the IIFE (the baseUrl computation) to avoid returning
"/t/carbon.super" by default—either return null/empty string or derive the
tenant qualifier from a session/context variable if available, and update any
downstream usage (e.g., connectionUrl construction) to guard against a
null/empty baseUrl; specifically modify the baseUrl IIFE expression so it fails
closed (null/""), or uses a tenant-aware value from sessionTenant (or
equivalent) instead of the hardcoded "/t/carbon.super", and ensure code that
builds connectionUrl checks for the new null/empty value before concatenation.
---
Nitpick comments:
In `@features/admin.connections.v1/api/connection-test-api.ts`:
- Around line 92-99: The type guard isConnectionTestAxiosError currently only
checks for the presence of isAxiosError; update it to verify the property value
is true to make the guard precise (e.g., after casting to candidate:
AxiosError<ConnectionTestErrorResponseInterface>, ensure candidate && typeof
candidate === "object" && candidate.isAxiosError === true). Keep the same return
type and use the existing candidate variable and
AxiosError<ConnectionTestErrorResponseInterface> type in the condition.
In `@features/admin.connections.v1/pages/connection-edit.tsx`:
- Around line 769-779: Replace the deprecated data-testid with data-componentid
on the SecondaryButton used in the
ConnectionsManagementUtils.isConnectorIdentityProvider branch and
internationalize its label by wrapping "Test Connection" with the i18n
translator (t(...)); update the attribute on the SecondaryButton rendered in
that branch (the element using handleTestConnection, isTestingConnection and
loading/disabled props) and ensure the translator function (e.g., t from
useTranslation) is in scope or imported so the button label is rendered via
t('...') instead of a hardcoded string.
In `@features/admin.connections.v1/pages/connection-test.tsx`:
- Line 771: The map callback uses an `any` type for `log`; replace it with the
specific interface `ConnectionTestDiagnosticLogInterface` by changing the
signature in the diagnostics.map call (e.g., `diagnostics.map((log:
ConnectionTestDiagnosticLogInterface, idx: number) => { ... })`) and ensure
`ConnectionTestDiagnosticLogInterface` is imported or available in this module;
also update any local destructuring/uses of `log` to match the interface fields
and remove other `any` usages tied to this mapping.
In
`@identity-apps-core/apps/authentication-portal/src/main/webapp/debugError.jsp`:
- Line 22: Remove the unused import of StringEscapeUtils from debugError.jsp:
locate the page/import directive that references StringEscapeUtils (the import
of org.apache.commons.lang3.StringEscapeUtils or similar) and delete that import
line so the JSP no longer imports an unused class; keep the other directives
like the jsp:directive.include for includes/init-url.jsp unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2b44b838-3aea-4386-9f29-74cdce12c1f5
📒 Files selected for processing (10)
apps/console/src/configs/routes.tsxfeatures/admin.connections.v1/api/connection-test-api.tsfeatures/admin.connections.v1/components/connection-test-styles.tsxfeatures/admin.connections.v1/configs/endpoints.tsfeatures/admin.connections.v1/models/connection-test.tsfeatures/admin.connections.v1/models/connection.tsfeatures/admin.connections.v1/pages/connection-edit.tsxfeatures/admin.connections.v1/pages/connection-test.tsxidentity-apps-core/apps/authentication-portal/src/main/webapp/debugError.jspidentity-apps-core/apps/authentication-portal/src/main/webapp/debugSuccess.jsp
✅ Files skipped from review due to trivial changes (1)
- features/admin.connections.v1/components/connection-test-styles.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- features/admin.connections.v1/configs/endpoints.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
features/admin.connections.v1/pages/connection-test.tsx (1)
898-921: 💤 Low valueUse i18n translation keys and
data-componentidattribute.Several UI strings are hardcoded and the component uses deprecated
data-testidinstead ofdata-componentid:
Replace hardcoded strings with translation keys:
- Line 898:
pageTitle="Test Results"→ uset("...")- Line 900:
description="Results for the connection test session."→ uset("...")- Line 915:
Rerun Test→ uset("...")Replace
data-testidwithdata-componentidthroughout (lines 902, 913, 921, 947, 958, 970, 980).Based on learnings: "prefer using the data-componentid attribute... data-testid is deprecated."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/admin.connections.v1/pages/connection-test.tsx` around lines 898 - 921, Replace hardcoded UI strings with i18n lookups and swap deprecated data-testid attributes for data-componentid: change pageTitle="Test Results" to pageTitle={t("console:develop.pages.idpTest.testResults.pageTitle", "Test Results")}, description="Results for the connection test session." to description={t("console:develop.pages.idpTest.testResults.description", "Results for the connection test session.")}, and the Button label "Rerun Test" inside the action prop to t("console:develop.pages.idpTest.testResults.rerunButton", "Rerun Test"); also replace all data-testid props shown in this JSX (the backButton object key, the Button prop, and the outer PageLayout prop currently using `${testId}-...`) with data-componentid preserving the same identifier string pattern (e.g. data-componentid={`${testId}-back-button`} etc.) so the Back button, rerun Button, page layout and other occurrences use data-componentid instead of data-testid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@features/admin.connections.v1/pages/connection-test.tsx`:
- Line 770: Replace the use of the any type in the diagnostics.map callback by
typing the log parameter as ConnectionTestDiagnosticLogInterface; update the
arrow function signature where diagnostics.map((log: any, idx: number) => { ...)
to diagnostics.map((log: ConnectionTestDiagnosticLogInterface, idx: number) => {
...) so the component uses the imported ConnectionTestDiagnosticLogInterface for
proper typing.
In `@features/admin.core.v1/store/reducers/config.ts`:
- Around line 413-420: The object literal commonConfigReducerInitialState has
missing closing braces/commas around nested properties (e.g., after the users
block before enterpriseSAML and before microsoft) causing syntax errors; fix by
closing each nested object with a } and adding a trailing comma between sibling
properties (ensure users: null is followed by }, or if users is an object it is
closed, then add , before enterpriseSAML, and likewise close the block preceding
microsoft and add a comma), validate the whole commonConfigReducerInitialState
for matching braces and commas, and re-run the TypeScript/lint build to confirm
the syntax errors are resolved.
- Around line 154-155: The config reducer is missing a closing brace and comma
after the endpoints object, causing the object containing workflows and adminApp
to be malformed; close the endpoints object (add the missing "}" ), ensure
there's a comma after workflows: "" if needed, and confirm adminApp: { ... } is
correctly nested inside the top-level config object (check symbols: endpoints,
workflows, and adminApp in the reducer) so the TypeScript syntax is valid and
compilation errors are resolved.
---
Nitpick comments:
In `@features/admin.connections.v1/pages/connection-test.tsx`:
- Around line 898-921: Replace hardcoded UI strings with i18n lookups and swap
deprecated data-testid attributes for data-componentid: change pageTitle="Test
Results" to pageTitle={t("console:develop.pages.idpTest.testResults.pageTitle",
"Test Results")}, description="Results for the connection test session." to
description={t("console:develop.pages.idpTest.testResults.description", "Results
for the connection test session.")}, and the Button label "Rerun Test" inside
the action prop to t("console:develop.pages.idpTest.testResults.rerunButton",
"Rerun Test"); also replace all data-testid props shown in this JSX (the
backButton object key, the Button prop, and the outer PageLayout prop currently
using `${testId}-...`) with data-componentid preserving the same identifier
string pattern (e.g. data-componentid={`${testId}-back-button`} etc.) so the
Back button, rerun Button, page layout and other occurrences use
data-componentid instead of data-testid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 4ac56e99-0614-4f16-90f4-a186b07db8e6
📒 Files selected for processing (11)
apps/console/src/configs/routes.tsxfeatures/admin.connections.v1/api/connection-test-api.tsfeatures/admin.connections.v1/components/connection-test-styles.tsxfeatures/admin.connections.v1/configs/endpoints.tsfeatures/admin.connections.v1/models/connection-test.tsfeatures/admin.connections.v1/models/connection.tsfeatures/admin.connections.v1/models/endpoints.tsfeatures/admin.connections.v1/pages/connection-edit.tsxfeatures/admin.connections.v1/pages/connection-test.tsxfeatures/admin.core.v1/constants/app-constants.tsfeatures/admin.core.v1/store/reducers/config.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- features/admin.connections.v1/models/endpoints.ts
- features/admin.core.v1/constants/app-constants.ts
- features/admin.connections.v1/models/connection.ts
- apps/console/src/configs/routes.tsx
- features/admin.connections.v1/pages/connection-edit.tsx
- features/admin.connections.v1/components/connection-test-styles.tsx
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10277 +/- ##
=======================================
Coverage 55.70% 55.70%
=======================================
Files 42 42
Lines 1016 1016
Branches 254 231 -23
=======================================
Hits 566 566
Misses 450 450
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@features/admin.connections.v1/pages/connection-test.tsx`:
- Around line 129-131: The cached ConnectionTestResult is being read/written
using local persistence (getCachedResult/LocalStorage), which persists sensitive
debug payloads across browser restarts; change the cache to use
SessionStorageUtils instead so results persist only for the session. Update any
getCachedResult/setCachedResult helpers (or the initialization of result via
getCachedResult and the places that write the cache around the
ConnectionTestResultInterface) to use SessionStorageUtils APIs (get/set/remove)
rather than LocalStorageUtils or raw localStorage; ensure the same change is
applied for the other cache usage referenced near the component (the second
cached block around the other initialization at lines 284-287) and keep the
debugId state handling unchanged.
- Around line 688-695: The current empty-state check is wrong because steps
always has fixed keys; update the condition used to render the "no logs" empty
state in connection-test.tsx to verify whether any step values are actually
populated (e.g., use Object.values(steps).every(v => !v) or check
diagnostics.length === 0) instead of Object.keys(steps).length === 0; apply the
same fix for the second occurrence around the block that references the same
steps/diagnostics logic (the second occurrence you noted around lines 795-803),
ensuring you reference the steps object, stepStatus, metadataObj and the
diagnostics array when deciding to show the empty-state alert.
- Around line 320-323: The handleRunTests() flow allows overlapping runs because
loading is only set later in fetchResult()/pollForResult(), so buttons remain
enabled and multiple startConnectionTestSession() calls/timers can be created;
fix by setting the loading state at the very beginning of handleRunTests(),
immediately clearing any existing timers (e.g., those used by
pollForResult()/fetchResult()) before calling
startConnectionTestSession(connectorId, resourceEndpoints), and ensure every
early-return or catch path resets loading to false and cleans timers so stale
timers cannot update the page; apply the same pattern to the other similar
blocks (around lines where startConnectionTestSession is called, and the
pollForResult()/fetchResult logic).
- Around line 276-289: The poll handler currently falls through and treats
DEBUG_STATUS_INCOMPLETE as a finished result when attempt === POLL_MAX_ATTEMPTS;
change the logic in the polling branch (where DEBUG_STATUS_INCOMPLETE,
POLL_MAX_ATTEMPTS, pollForResult are used) so that if response.data?.status ===
DEBUG_STATUS_INCOMPLETE AND attempt >= POLL_MAX_ATTEMPTS you do NOT call
setResult/cacheResult as-is; instead set an explicit timeout/error result (e.g.
setResult({ status: 'TIMEOUT', message: 'Polling timed out' }) or flip an error
state), call clearCachedResult(), setIsStatusBannerVisible(true) to show an
error/timeout banner, avoid caching the incomplete payload, and ensure loading
is set to false; leave normal pollForResult behavior unchanged when attempt <
POLL_MAX_ATTEMPTS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 7754f041-3ded-40cf-9100-78c4d74abd83
📒 Files selected for processing (2)
features/admin.connections.v1/pages/connection-test.tsxfeatures/admin.core.v1/store/reducers/config.ts
* Add marketing consent view * Update text * Bug fixes * Update MyAccount UI * Add changeset 🦋 * Fix review comments * Update knip.json * Refactor * Show policy elements in suborgs * Fix review comments * Fix review comments * UI improvement * Enable suborg pages * Rename to preference management * Fix review comments * Fix review comments
Purpose
Adds a debug flow for federated IdPs (FIdPs), enabling administrators to initiate and inspect authentication sessions for a given connection. This helps diagnose misconfigurations and protocol-level issues (e.g. OIDC claim mapping, token exchange failures).
Related Issues
Related PRs
Checklist
Security checks
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.