Add initial changes for sandbox key management #600
Conversation
Thushani-Jayasekera
requested a review
from DivyaPremanantha
as a code owner
WalkthroughAdds a Manage Keys feature with per-keyType (PRODUCTION/SANDBOX) scoping across controllers, routes, templates, and client scripts; hardens warning modal handling and input sanitization; extends adminService.mapKeys to accept keyType; and adds OpenAPI server descriptions "Production" and "Sandbox". Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client (Browser)
participant Router as Router
participant Controller as ApplicationsController
participant Service as Backend Service
participant API as Backend API
Browser->>Router: GET /.../applications/:applicationId/manage-keys
Router->>Controller: loadApplicationKeys(req)
Controller->>Service: getAPIMKeyManagers(req)
Service->>API: fetch key managers
API-->>Service: key managers
Service-->>Controller: key managers
Controller->>Service: getAPIKeys(apiId, appId, "PRODUCTION")
Service->>API: fetch production keys
API-->>Service: production keys
Service-->>Controller: production keys
Controller->>Service: getAPIKeys(apiId, appId, "SANDBOX")
Service->>API: fetch sandbox keys
API-->>Service: sandbox keys
Service-->>Controller: sandbox keys
Controller-->>Browser: render manage-keys page (per-keyType UI)
Browser->>Router: POST /generateKey (payload includes keyType)
Router->>Controller: generateAPIKeys(req)
Controller->>Service: createAppKeyMapping(..., keyType)
Service->>API: create/map key with keyType
API-->>Service: success
Service-->>Controller: key data
Controller-->>Browser: updated key state (keyType-scoped)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/scripts/api-key-generation.js (1)
333-336: Inconsistent error message in regenerateAPIKey.The error message says "Failed to generate API Key" but this is the
regenerateAPIKeyfunction. Should be "Failed to regenerate API Key" for consistency with line 331.🔎 Proposed fix
} catch (error) { console.log(error); - await showAlert(`Failed to generate API Key. Please try again.\n${error}`, 'error'); + await showAlert(`Failed to regenerate API Key. Please try again.\n${error}`, 'error'); }src/pages/application/partials/generate-oauth-key.hbs (3)
91-91: Inconsistent ID suffix:tokenDisplay_{{../name}}missing keyType.The
tokenDisplay_{{../name}}ID on line 91 does not include thekeyTypesuffix, unlike similar elements such ascurlDisplay_{{../name}}_{{keyType}}on line 125 andtoken_{{../name}}_{{keyType}}on line 112. This could cause ID collisions when both PRODUCTION and SANDBOX keys are present.🔎 Suggested fix
- <div id="tokenDisplay_{{../name}}" style="display: none;"> + <div id="tokenDisplay_{{../name}}_{{keyType}}" style="display: none;">
142-142: Inconsistent ID suffix:curl_{{../name}}missing keyType.The
curl_{{../name}}ID does not include thekeyTypesuffix, which will cause collisions when rendering both PRODUCTION and SANDBOX keys.🔎 Suggested fix
- <code id="curl_{{../name}}"></code> + <code id="curl_{{../name}}_{{keyType}}"></code>
341-342: Inconsistent ID suffix:showAdvanced('KMData_{{../name}}')missing keyType.On line 342, the
showAdvancedcall usesKMData_{{../name}}without the keyType suffix, but the container on line 352 usesKMData_{{../name}}as well. This is inconsistent with the pattern used elsewhere (e.g., line 182 usesKMData_{{../name}}_{{keyType}}). For the "else" branch (when consumer key doesn't exist), ensure consistency with the per-keyType pattern.🔎 Suggested fix
- onclick="showAdvanced('KMData_{{../name}}')">Advanced Configuration + onclick="showAdvanced('KMData_{{../name}}_{{keyType}}')">Advanced ConfigurationAnd update the container ID on line 352:
- <div class="container KMConfig mt-4" id="KMData_{{../name}}" style="display: none;"> + <div class="container KMConfig mt-4" id="KMData_{{../name}}_{{keyType}}" style="display: none;">src/controllers/applicationsContentController.js (1)
306-312: Same issue exists inloadApplication.The same scoping issue exists in
loadApplicationwhereapplicationIdis declared at line 132 inside thetryblock but referenced at line 359 in thecatchblock.🔎 Suggested fix
Move the declaration before the try block:
const loadApplication = async (req, res) => { let html, templateContent, metaData, kMmetaData; const viewName = req.params.viewName; const orgName = req.params.orgName; const orgDetails = await adminDao.getOrganization(orgName); const devportalMode = orgDetails.ORG_CONFIG?.devportalMode || constants.API_TYPE.DEFAULT; + const applicationId = req.params.applicationId; try { - const applicationId = req.params.applicationId; if (config.mode === constants.DEV_MODE) {
🧹 Nitpick comments (15)
src/styles/manage-keys.css (2)
70-99: Consider reducing!importantusage.Multiple
!importantdeclarations on.accordion-buttonstates suggest potential specificity conflicts with Bootstrap or other frameworks. While functional, this can make future maintenance harder.If Bootstrap's accordion styles are the issue, consider increasing selector specificity instead (e.g.,
.keys-section .accordion-button) to avoid!importantoveruse.
124-140: Webkit-only scrollbar styles won't apply in Firefox.The
::-webkit-scrollbarpseudo-elements only work in Chromium-based browsers and Safari. Firefox users will see the default scrollbar.For broader support, consider adding
scrollbar-widthandscrollbar-colorproperties for Firefox:.accordion-body { /* Firefox */ scrollbar-width: thin; scrollbar-color: var(--primary-light-color) var(--secondary-bg-color); }src/defaultContent/styles/manage-keys.css (1)
1-189: Duplicate stylesheet detected.This file is identical to
src/styles/manage-keys.css. Maintaining two copies increases the risk of divergence during future updates.If both locations are required by the architecture (e.g., default content vs. customizable content), consider importing from a single source or documenting why duplication is necessary.
src/pages/application/partials/keys-generate.hbs (2)
196-196: Potential ID collision forgrant-types-{{keyType}}.The IDs
grant-types-{{keyType}}andgrant-types-adv-{{keyType}}don't include the key manager name ({{../name}}). If multiple key managers are enabled, this could cause duplicate IDs when the outer{{#each keyManagersMetadata}}loop iterates.Consider including the key manager identifier:
🔎 Proposed fix
-<div class="keygen-checkbox-group" id="grant-types-{{keyType}}"> +<div class="keygen-checkbox-group" id="grant-types-{{../name}}-{{keyType}}">And similarly for line 355:
-<div class="keygen-checkbox-group" id="grant-types-adv-{{keyType}}"> +<div class="keygen-checkbox-group" id="grant-types-adv-{{../name}}-{{keyType}}">Also applies to: 355-355
326-328: Long inline onclick handler reduces readability.The
generateApplicationKeycall on line 328 has many parameters, making it hard to maintain. While functional, consider extracting this to a helper function that accepts an options object in future refactoring.src/pages/application/page.hbs (1)
19-28: Approve the modal relocation; consider removing commented-out code.The refactor to serve key modals from the dedicated manage-keys page is clean. However, the commented-out code at lines 23-28 appears to be stale (oauth2-key-generation, throttling-reset-modal, api-key-modal, delete-confirmation). Consider removing this dead code to improve maintainability.
src/defaultContent/partials/sidebar.hbs (1)
65-70: Consider using a key-related icon for "Manage Keys".The current icon (
documentation.svg) is semantically appropriate for documentation links but less intuitive for key management. Consider using a key or lock icon to better convey the functionality.src/pages/manage-keys/page.hbs (1)
1-17: LGTM! Consider reviewing necessity of all included scripts.The page structure is correct. However, scripts like
subscription.js,application-dashboard.js, andedit-application-form.jsmay not be needed for a dedicated key management page. If these are unused on this page, consider removing them to reduce page load overhead.src/controllers/customContentController.js (1)
66-79: Path normalization and existence check logic is sound, with minor redundancy.The normalization correctly redirects application-specific manage-keys paths to the shared template. However, the check on line 75 (
filePath.includes('manage-keys')) will always be true after normalization on line 69, making the conditional somewhat redundant. Consider simplifying:🔎 Optional simplification
// Check if the file exists before attempting to render const resolvedPagePath = path.join(process.cwd(), filePrefix + filePath + '/page.hbs'); if (!fs.existsSync(resolvedPagePath)) { - // If it's a manage-keys route that doesn't exist, return 404 or redirect - if (filePath.includes('manage-keys')) { - throw new Error(`Manage keys page not found. This route should be handled by the application controller.`); - } - throw new Error(`Content page not found at ${resolvedPagePath}`); + throw new Error(`Content page not found at ${resolvedPagePath}`); }The manage-keys specific message isn't strictly necessary since the safety check at lines 36-48 should catch these cases first in the request flow.
src/pages/application/partials/keys-instructions.hbs (1)
14-16: Minor: Inconsistent ID separator convention.Element IDs use underscore (
tokenGenerationButtons_{{../name}}_{{keyType}},requestDetails_{{../name}}_{{keyType}}) while other IDs in this file and related templates use hyphens. Consider standardizing for consistency across the codebase.src/scripts/api-key-generation.js (1)
146-159: Potential issue: URL path manipulation may fail for nested routes.The regex
/\/[^\/]*$/replaces only the last path segment. If the current path has query parameters or the structure differs, the generated href could be incorrect.Consider using
URLAPI for safer path manipulation:🔎 Proposed fix
- manageLink.href = window.location.pathname.replace(/\/[^\/]*$/, '/manage-keys') + envHash; + const basePath = window.location.pathname.split('/').slice(0, -1).join('/'); + manageLink.href = basePath + '/manage-keys' + envHash;src/pages/application/partials/manage-keys.hbs (2)
1-6: Consider: External CDN dependency for Bootstrap Icons.Line 5 loads Bootstrap Icons from a CDN. If the CDN is unavailable, icons won't render. Consider bundling this locally for reliability, or ensure a fallback is in place.
31-149: Consider: Significant code duplication between Production and Sandbox sections.The OAuth2 Keys sections for Production (lines 31-149) and Sandbox (lines 264-388) are nearly identical, differing only in key type references. Consider extracting a shared partial to reduce maintenance burden and ensure consistency.
Also applies to: 264-388
src/controllers/applicationsContentController.js (1)
381-650: Significant code duplication betweenloadApplicationandloadApplicationKeys.The
loadApplicationKeysfunction (lines 381-650) duplicates most of the logic fromloadApplication(lines 125-379). Both functions share nearly identical code for:
- Fetching subscribed APIs and building
subList- Processing key managers and building production/sandbox keys
- Constructing template content
Consider extracting common logic into shared helper functions to improve maintainability and reduce the risk of divergence.
src/scripts/oauth2-key-generation.js (1)
926-950:validateGrantTypeskeyType parameter unused.The
keyTypeparameter is passed tovalidateGrantTypesbut not used in the function body. The function uses modal-scoped selectors instead. Consider removing the unused parameter or documenting why it's passed for future use.🔎 Suggested fix
If the parameter is not needed:
-function validateGrantTypes(modal, keyType) { - // Find the update button with keyType suffix +function validateGrantTypes(modal) { const updateButton = modal.querySelector('[id^="applicationKeyUpdateButton"]');And update the call sites at lines 915 and 921:
- validateGrantTypes(modal, keyType); + validateGrantTypes(modal);- validateGrantTypes(modal, keyType); + validateGrantTypes(modal);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/controllers/apiContentController.js(1 hunks)src/controllers/applicationsContentController.js(7 hunks)src/controllers/customContentController.js(2 hunks)src/controllers/devportalController.js(1 hunks)src/defaultContent/pages/manage-keys/page.hbs(1 hunks)src/defaultContent/partials/sidebar.hbs(1 hunks)src/defaultContent/styles/manage-keys.css(1 hunks)src/pages/application/page.hbs(1 hunks)src/pages/application/partials/api-key.hbs(2 hunks)src/pages/application/partials/apis.hbs(3 hunks)src/pages/application/partials/generate-oauth-key.hbs(21 hunks)src/pages/application/partials/keys-generate.hbs(20 hunks)src/pages/application/partials/keys-instructions.hbs(2 hunks)src/pages/application/partials/keys-modify.hbs(8 hunks)src/pages/application/partials/keys-token.hbs(3 hunks)src/pages/application/partials/keys-view.hbs(7 hunks)src/pages/application/partials/manage-keys.hbs(1 hunks)src/pages/application/partials/oauth2-key-generation.hbs(2 hunks)src/pages/application/partials/overview.hbs(5 hunks)src/pages/manage-keys/page.hbs(1 hunks)src/routes/applicationsContentRoute.js(1 hunks)src/routes/designModeRoute.js(1 hunks)src/scripts/api-key-generation.js(9 hunks)src/scripts/common.js(3 hunks)src/scripts/oauth2-key-generation.js(16 hunks)src/scripts/warning.js(1 hunks)src/services/adminService.js(2 hunks)src/styles/manage-keys.css(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/controllers/devportalController.js (2)
src/services/adminService.js (1)
cpAppID(1154-1154)src/middlewares/registerPartials.js (1)
apiHandle(165-165)
src/routes/designModeRoute.js (4)
src/routes/applicationsContentRoute.js (2)
router(2-2)registerPartials(4-4)src/routes/devportalRoute.js (1)
router(19-19)src/routes/apiContentRoute.js (2)
router(19-19)registerPartials(21-21)src/middlewares/registerPartials.js (1)
registerPartials(35-92)
src/controllers/customContentController.js (1)
src/controllers/applicationsContentController.js (4)
logger(21-21)path(24-24)filePrefix(29-29)fs(25-25)
src/routes/applicationsContentRoute.js (2)
src/controllers/customContentController.js (1)
req(34-34)src/middlewares/ensureAuthenticated.js (1)
ensureAuthenticated(114-239)
src/scripts/api-key-generation.js (1)
src/middlewares/ensureAuthenticated.js (1)
keyType(427-427)
src/scripts/oauth2-key-generation.js (1)
src/scripts/api-key-generation.js (12)
generateBtn(140-140)generateBtn(238-238)normalState(88-88)normalState(208-208)normalState(286-286)tokenBtn(87-87)tokenBtn(285-285)scopeContainer(45-45)scopeContainer(91-91)scopeContainer(310-310)modal(38-38)modal(124-124)
🪛 Biome (2.1.2)
src/controllers/applicationsContentController.js
[error] 570-570: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
🔇 Additional comments (36)
src/styles/manage-keys.css (1)
53-56: Focus state accessibility is preserved.The
:focusstate removes the default outline but provides a visiblebox-shadowring, which maintains keyboard accessibility. This approach is acceptable.src/services/adminService.js (1)
1280-1298: Function signature update looks correct.The
mapKeysfunction now properly accepts and uses thekeyTypeparameter in the request body. This change supports the per-key-type (PRODUCTION/SANDBOX) flow.src/pages/application/partials/apis.hbs (3)
48-49: Per-key-type partial inclusion looks correct.The
api-keypartial is now rendered twice per API (once for PRODUCTION, once for SANDBOX), enabling key-type-specific handling. This aligns with the broader per-key-type architecture.
70-117: Production/Sandbox key UI structure is well-implemented.The conditional rendering properly handles four states:
- Production key exists → show manage link
- Production key missing → show generate button
- Sandbox key exists → show manage link
- Sandbox key missing → show generate button
The
keyTypeparameter is correctly passed toopenApiKeyModalas the last argument.
219-219: MCP Servers section should support dual-key handling like REST/WS APIs.The MCP Servers section hardcodes
'PRODUCTION'as the keyType parameter. REST/WS APIs provide both PRODUCTION and SANDBOX key options via theapi-keypartial. If MCP servers should support sandbox keys for testing (similar to REST/WS APIs), implement dual-key generation to match the existing REST/WS pattern.src/pages/application/partials/keys-generate.hbs (2)
28-30: Per-keyType iteration structure is correctly implemented.The
{{#each applicationKeys}}loop with{{#let "keys" keys}}properly establishes the per-key-type context, enabling separate rendering for PRODUCTION and SANDBOX environments.
67-138: Token generation and display sections properly scoped.The token generation buttons, token display areas, and cURL sections all correctly include
{{keyType}}in their IDs, ensuring isolation between PRODUCTION and SANDBOX environments.src/controllers/apiContentController.js (1)
878-889: LGTM! Clear server descriptions improve API definition clarity.Adding "Production" and "Sandbox" descriptions to the OpenAPI server entries is a good enhancement that helps API consumers distinguish between environments.
src/defaultContent/pages/manage-keys/page.hbs (1)
1-14: LGTM! Verify asset parity with the other manage-keys page.The page structure is clean. Note that this template includes fewer assets compared to
src/pages/manage-keys/page.hbs(missingapplication.css,subscription.js,application-dashboard.js,edit-application-form.js). Verify this is intentional for the defaultContent context, or align the assets if both pages require the same functionality.src/routes/applicationsContentRoute.js (1)
23-28: LGTM! Route follows established patterns.The new manage-keys route correctly mirrors the existing route structure with the favicon.ico guard,
registerPartials, andensureAuthenticatedmiddleware chain. TheloadApplicationKeysfunction is properly exported fromapplicationsContentController.js.src/pages/application/partials/api-key.hbs (1)
1-50: LGTM! Per-key-type scoping correctly implemented.The template properly scopes all element IDs and event handlers using the
{{subID}}-{{keyType}}pattern, preventing ID collisions when multiple modals are rendered for different key types (PRODUCTION/SANDBOX). The change from "Done" to "Cancel" aligns better with the modal's action semantics.src/pages/application/partials/oauth2-key-generation.hbs (1)
254-254: Good improvement: Environment-specific button ID.Adding the production/sandbox suffix to the Update button ID prevents DOM ID collisions when both key types are rendered on the same page.
src/scripts/common.js (1)
1010-1013: Good defensive fix for accordion chevron toggle.Adding the null check before manipulating
icon.classListprevents potential runtime errors when the chevron element doesn't exist.src/controllers/customContentController.js (1)
36-48: Good safety net for routing edge cases.This defensive check ensures that if the manage-keys route unexpectedly falls through to the catch-all handler (due to Express route ordering or configuration issues), it's handled gracefully with appropriate logging rather than failing silently or rendering incorrect content.
src/routes/designModeRoute.js (1)
43-44: LGTM! Route additions follow established patterns.The parameter rename from
:applicationidto:applicationIdimproves naming consistency with JavaScript conventions. The new/manage-keysroute is correctly placed after the base application route, ensuring proper route matching order. TheloadApplicationKeyshandler is properly exported fromapplicationsContentController.src/pages/application/partials/keys-instructions.hbs (2)
1-11: LGTM! Per-keyType modal structure looks correct.The nested iteration over
keyManagersMetadataandapplicationKeysproperly scopes modal IDs and handlers bykeyType. The modal title clearly indicates the key type context.
48-53: LGTM! Curl copy functionality properly scoped.The copy button and curl code block IDs correctly include both the key manager id and keyType, preventing collisions between Production and Sandbox modals.
src/pages/application/partials/keys-view.hbs (3)
1-11: LGTM! Modal structure properly supports per-keyType credentials view.The modal IDs, close handlers, and body containers are correctly scoped by
keyType, enabling separate modals for Production and Sandbox keys.
196-234: Verify context navigation is intentional. The ID patterns use different depths based on their containing helpers:{{../../../id}}within{{#if}}blocks and{{../../../../id}}within{{#and}}blocks and attribute contexts. Confirm the{{#and}}helper changes context as expected for these paths to resolve correctly.
138-148: Remove the context depth verification—the code is correct.Lines 138 and 141 use different context paths because
keyType(defined at theapplicationKeysloop) andid(defined at thekeyManagersMetadataloop) exist at different nesting levels. Handlebars context resolution automatically walks the parent context chain, making{{keyType}}accessible without explicit../navigation, while{{../../../id}}correctly navigates to the outer loop context. This is not an inconsistency.src/scripts/api-key-generation.js (1)
3-29: LGTM! Hash-based tab navigation implementation.The DOMContentLoaded handler correctly activates Production/Sandbox tabs based on URL hash. Good defensive programming with existence checks for tab elements.
src/scripts/warning.js (3)
1-6: LGTM! Good defensive null check for modal element.Early return with error logging when the modal element doesn't exist prevents null reference errors downstream.
29-35: LGTM! Good error handling for Bootstrap Modal initialization.Wrapping the Modal instantiation in try-catch prevents unhandled exceptions if Bootstrap isn't loaded or the modal configuration is invalid.
50-64: LGTM! Improved security with function reference checks.Using
typeof === 'function'checks before invokingregenerateAPIKeyandrevokeAPIKeyprevents errors if these functions aren't defined in the current context. The onclick assignment approach is safer than inline HTML attributes.src/pages/application/partials/manage-keys.hbs (2)
89-122: LGTM! Key action buttons properly scoped by environment.The View, Modify, Generate, and Instructions buttons correctly pass the key type (
'PRODUCTION'or'SANDBOX') to their respective modal loader functions. Read-only mode is consistently applied.Also applies to: 329-362
248-258: LGTM! Empty state handling for no subscriptions.Both Production and Sandbox sections properly handle the case when no API subscriptions exist, displaying an informative message to the user.
Also applies to: 490-500
src/pages/application/partials/generate-oauth-key.hbs (1)
504-506: Verify template block nesting order.The closing blocks
{{/let}},{{/if}}, and{{/each}}at lines 504-506 close theapplicationKeysiteration, but this appears before the closing{{/each}}forkeyManagersMetadata. Ensure the intended nesting is correct—currently{{/if}}closes the{{#if enabled}}block fromkeyManagersMetadata, not fromapplicationKeys. The logic seems correct but is worth verifying during testing.src/pages/application/partials/keys-modify.hbs (2)
1-11: Per-keyType modal structure looks correct.The iteration over
keyManagersMetadataandapplicationKeysproperly scopes each modal to a specific keyType. The modal IDkeysModifyModal-{{keyType}}and the close button referencing the same ID ensure correct per-type modal management.
17-22: Form submission correctly includes keyType.The form's
onsubmithandler properly passeskeyTypetoupdateApplicationKey, ensuring the correct key type is updated.src/pages/application/partials/overview.hbs (2)
67-99: Per-keyType OAuth2 keys section is well-structured.The iteration over
applicationKeyswith conditional rendering based onkeys.consumerKeyexistence correctly handles both generated and ungenerated key states for each environment. The keyType-aware button IDs and handler calls ensure proper isolation.
103-156: Hardcoded 'PRODUCTION' keyType in non-internal key manager branch.The
elsebranch (lines 103-156) for non-internal key managers uses hardcodedPRODUCTIONfor IDs and handler calls (e.g.,generateKeyContainer-PRODUCTION,loadKeysViewModal('PRODUCTION')). Verify this is intentional and that sandbox key support is not required for external key managers. If sandbox support is needed later, this section will need refactoring.src/scripts/oauth2-key-generation.js (5)
2-6: Good defensive validation for required parameters.The upfront validation of required parameters prevents silent failures and provides clear error messages for debugging.
51-63: Good error handling for JSON parsing.Wrapping
JSON.parse(subscriptions)in a try-catch with proper error messaging and button state reset is a good practice.
117-118: Verify regex replacement for keyManagerId extraction.The regex
/-(SANDBOX|PRODUCTION)$/correctly strips the keyType suffix from the formId. However, ensure all formIds follow the expected patternkeysview-{id}-{keyType}, or this extraction could fail silently.
840-860: Modal loading functions properly handle keyType.The
loadKeysViewModal,loadKeysModifyModal,loadKeysTokenModal, andloadKeysInstructionsModalfunctions correctly construct modal IDs using the keyType parameter and include null checks for missing modals.
431-432: No ID mismatch found—consumer key and secret IDs match template pattern.The IDs on lines 431-432 use
formId.replace("applicationKeyGenerateForm-", ""), which produces{{../id}}-{{keyType}}, matching the template IDsconsumer-key-{{../id}}-{{keyType}}andconsumer-secret-{{../id}}-{{keyType}}. The regex replacement on line 412 is for a different variable (appKeyManagerId) and does not affect the consumer-key/secret ID construction.
| // Sanitize parameters to prevent XSS | ||
| const sanitizeParam = (param) => { | ||
| if (param === null || param === undefined) return ''; | ||
| return String(param).replace(/['"<>]/g, ''); | ||
| }; | ||
|
|
||
| const sanitizedParam1 = sanitizeParam(param1); | ||
| const sanitizedParam2 = sanitizeParam(param2); | ||
| const sanitizedParam3 = sanitizeParam(param3); | ||
| const sanitizedParam4 = sanitizeParam(param4); | ||
| const sanitizedParam5 = sanitizeParam(param5); | ||
| const sanitizedParam6 = sanitizeParam(param6); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the warning.js file and check its context
find . -type f -name "warning.js" | head -5
# Check apiKeyId usage patterns
rg -n 'apiKeyId|api-key-id|data-api-key-id' --type=js -A 3 2>/dev/null | head -50
# Check sanitization pattern usage
rg -n 'sanitizeParam|sanitize.*param' --type=js -A 2 2>/dev/null | head -50Repository: wso2/api-developer-portal-core
Length of output: 50386
Sanitization corrupts API key IDs—quotes are data, not XSS attempts.
API key IDs stored in data attributes include literal single quotes as part of their value (e.g., 'abc-123'). The sanitization removes these quotes, corrupting the data. The quotes are part of the intentional data structure, not XSS vectors. Either preserve the original unsanitized values separately or sanitize more carefully to avoid corrupting legitimate quote-wrapped values.
🤖 Prompt for AI Agents
In src/scripts/warning.js around lines 8 to 19 the current sanitizeParam
function strips quote characters and thus corrupts API key IDs that legitimately
include surrounding single quotes; stop removing quotes and preserve original
values for data attributes by (1) keeping rawParamX = paramX for
storage/attribute data, and (2) replace the sanitizeParam implementation used
for HTML insertion with proper escaping (HTML-entity encoding of &, <, >, " and
' to &, <, >, ", ') or only escape when injecting into HTML
attributes rather than mutating the raw value so API key IDs remain intact.
8d1bb00 to
4335b0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/application/partials/api-key.hbs (1)
35-39: Bug: "Done" button uses incorrect modal ID, missing-{{keyType}}suffix.The
closeModalcall on line 37 uses'apiKeyModal-{{subID}}'but the modal ID on line 1 isapiKeyModal-{{subID}}-{{keyType}}. This mismatch will prevent the modal from closing when the "Done" button is clicked.🔎 Proposed fix
<div class="d-flex justify-content-end mt-4 mb-3"> <button class="common-btn-outlined pr-3 me-3" type="button" - onClick="closeModal('apiKeyModal-{{subID}}')"> + onClick="closeModal('apiKeyModal-{{subID}}-{{keyType}}')"> Done </button>
♻️ Duplicate comments (8)
src/scripts/common.js (1)
133-153: Duplicate: Apply the past review's suggested fixes.The concerns flagged in the previous review remain unaddressed:
- Comments on lines 138 and 143 still incorrectly reference "API ID" when extracting
applicationId.- Line 134 still lacks optional chaining for
applicationsSubmenu.classList.add('show'), unlike the guardedapiSubmenu?.classList.add('show')on line 113.Please apply the diff provided in the past review comment.
src/services/adminService.js (1)
1244-1244: Duplicate: keyType remains undefined—apply the past review's fix.The critical issue flagged in the previous review persists:
keyTypeat line 1244 is undefined because it was never destructured fromreq.bodyat line 1160. This will cause themapKeyscall to fail.Apply the fix suggested in the past review comment by adding
keyTypeto the destructuring:-const { applicationName, apis, tokenType, tokenDetails, provider, clientID } = req.body; +const { applicationName, apis, tokenType, tokenDetails, provider, clientID, keyType } = req.body;src/scripts/warning.js (1)
8-19: Duplicate: Sanitization still corrupts API key IDs—apply the past review's fix.The concern raised in the previous review remains unaddressed. The
sanitizeParamfunction strips quote characters that are legitimate parts of API key IDs (e.g.,'abc-123'). This corrupts the data when passed to downstream handlers likeregenerateAPIKeyandrevokeAPIKey.Apply the fix suggested in the past review: either preserve original values separately for data attributes or use proper HTML entity encoding rather than character removal.
src/pages/application/partials/oauth2-key-generation.hbs (1)
86-86: Duplicate: JSON in onclick attribute risks breakage—apply the past review's fix.The concern flagged in the previous review remains unaddressed. Passing
{{json ../../subAPIs}}directly into theonClickattribute can break the handler when the JSON contains quotes or special characters, and may introduce XSS risks.Apply the data attribute approach suggested in the past review to safely pass the JSON and keyType to the handler.
src/controllers/applicationsContentController.js (2)
307-309: Unsafe optional chaining in for...of loop.This issue was flagged in a previous review and remains unaddressed. If
cpApplicationisundefined, the expressioncpApplication?.subscriptionScopesevaluates toundefined, and thefor...ofloop will throw aTypeError.🔎 Suggested fix
let subscriptionScopes = []; - if (Array.isArray(cpApplication?.subscriptionScopes)) { - for (const scope of cpApplication?.subscriptionScopes) { + if (cpApplication && Array.isArray(cpApplication.subscriptionScopes)) { + for (const scope of cpApplication.subscriptionScopes) { subscriptionScopes.push(scope.key); } }
569-572: Unsafe optional chaining in for...of loop (duplicate in loadApplicationKeys).The same unsafe optional chaining issue exists in the new
loadApplicationKeysfunction. IfcpApplicationisundefined, this will throw aTypeError.🔎 Suggested fix
let subscriptionScopes = []; - if (Array.isArray(cpApplication?.subscriptionScopes)) { - for (const scope of cpApplication?.subscriptionScopes) { + if (cpApplication && Array.isArray(cpApplication.subscriptionScopes)) { + for (const scope of cpApplication.subscriptionScopes) { subscriptionScopes.push(scope.key); } }src/scripts/api-key-generation.js (2)
1-1: Remove unused Express import.This issue was flagged in a previous review and remains unaddressed. The
require("express")statement is dead code in a client-side script.🔎 Proposed fix
-const e = require("express"); -
257-264: Incorrect loading state text for Generate button.This issue was flagged in a previous review and remains unaddressed. The loading state incorrectly shows "Revoking..." for a Generate Key button.
🔎 Proposed fix
generateButton.innerHTML = ` <span class="button-normal-state"> <i class="bi bi-key me-1"></i> ${envLabel} </span> <span class="button-loading-state" style="display: none;"> - <span class="spinner-border spinner-border-sm me-1" role="status"> Revoking...</span> + <span class="spinner-border spinner-border-sm me-1" role="status"></span> + Generating... </span> `;
🧹 Nitpick comments (3)
src/pages/application/partials/keys-generate.hbs (1)
194-210: Context reference{{../id}}works correctly but may benefit from clarity.At line 199,
{{../id}}within the{{#each @root.availableGrantTypes}}loop correctly resolves to thekeyManagersMetadataitem'sidfield. While Handlebars' context stack lookup mechanism allows this to work (traversing up through the{{#let}}and{{#if}}blocks), the reference path is not immediately obvious from the code structure. Consider documenting the context hierarchy or using explicit variable passing if templates become more complex, as similar patterns at deeper nesting levels (e.g., line 241 using{{../../../../id}}) already suggest this could be a maintainability concern.src/pages/application/partials/generate-oauth-key.hbs (2)
81-81: Consider refactoring to reduce parameter count.The
generateOauthKeyfunction call has 8 parameters. While functional, this makes the code harder to maintain and error-prone when parameters need to be reordered or added.💡 Consider using a data attribute approach
Instead of inline parameters, consider storing configuration in data attributes on the button:
-onClick="generateOauthKey('applicationKeyGenerateForm-{{../id}}-{{keyType}}', '{{keys.appRefID}}', '{{keys.keyMappingId}}', '{{../name}}', '{{keys.clientName}}', '{{keys.consumerSecret}}', '{{json @root.subAPIs}}', '{{keyType}}')" +data-form-id='applicationKeyGenerateForm-{{../id}}-{{keyType}}' +data-app-ref-id='{{keys.appRefID}}' +data-key-mapping-id='{{keys.keyMappingId}}' +data-key-manager='{{../name}}' +data-client-name='{{keys.clientName}}' +data-consumer-secret='{{keys.consumerSecret}}' +data-sub-apis='{{json @root.subAPIs}}' +data-key-type='{{keyType}}' +onClick="generateOauthKey(this)"Then read from
event.target.datasetin the function.
337-337: Consider refactoring to reduce parameter count.The
generateApplicationKeyfunction call has 9 parameters, making it difficult to maintain and prone to errors.💡 Consider using a data attribute approach
Similar to the previous comment, consider using data attributes instead of inline parameters for better maintainability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/controllers/apiContentController.js(1 hunks)src/controllers/applicationsContentController.js(7 hunks)src/controllers/customContentController.js(2 hunks)src/controllers/devportalController.js(1 hunks)src/defaultContent/pages/manage-keys/page.hbs(1 hunks)src/defaultContent/partials/sidebar.hbs(1 hunks)src/defaultContent/styles/manage-keys.css(1 hunks)src/pages/application/page.hbs(1 hunks)src/pages/application/partials/api-key.hbs(3 hunks)src/pages/application/partials/apis.hbs(3 hunks)src/pages/application/partials/generate-oauth-key.hbs(21 hunks)src/pages/application/partials/keys-generate.hbs(20 hunks)src/pages/application/partials/keys-instructions.hbs(2 hunks)src/pages/application/partials/keys-modify.hbs(8 hunks)src/pages/application/partials/keys-token.hbs(3 hunks)src/pages/application/partials/keys-view.hbs(7 hunks)src/pages/application/partials/manage-keys.hbs(1 hunks)src/pages/application/partials/oauth2-key-generation.hbs(2 hunks)src/pages/application/partials/overview.hbs(5 hunks)src/pages/manage-keys/page.hbs(1 hunks)src/routes/applicationsContentRoute.js(1 hunks)src/routes/designModeRoute.js(1 hunks)src/scripts/api-key-generation.js(9 hunks)src/scripts/common.js(3 hunks)src/scripts/oauth2-key-generation.js(16 hunks)src/scripts/warning.js(1 hunks)src/services/adminService.js(2 hunks)src/styles/manage-keys.css(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/pages/application/partials/manage-keys.hbs
🚧 Files skipped from review as they are similar to previous changes (10)
- src/defaultContent/pages/manage-keys/page.hbs
- src/controllers/devportalController.js
- src/pages/application/partials/keys-token.hbs
- src/controllers/apiContentController.js
- src/pages/application/partials/keys-view.hbs
- src/defaultContent/styles/manage-keys.css
- src/controllers/customContentController.js
- src/routes/designModeRoute.js
- src/defaultContent/partials/sidebar.hbs
- src/styles/manage-keys.css
🧰 Additional context used
🧬 Code graph analysis (5)
src/services/adminService.js (1)
src/middlewares/ensureAuthenticated.js (1)
keyType(427-427)
src/scripts/warning.js (3)
src/scripts/api-key-generation.js (2)
modal(38-38)modal(124-124)src/scripts/oauth2-key-generation.js (8)
modal(109-109)modal(506-506)modal(819-819)modal(844-844)modal(864-864)modal(954-954)modal(964-964)modal(1192-1192)src/scripts/subscription.js (3)
modal(10-10)modal(55-55)modal(166-166)
src/routes/applicationsContentRoute.js (3)
src/routes/devportalRoute.js (1)
router(19-19)src/middlewares/registerPartials.js (2)
req(203-203)registerPartials(35-92)src/middlewares/ensureAuthenticated.js (1)
ensureAuthenticated(114-239)
src/scripts/api-key-generation.js (2)
src/middlewares/ensureAuthenticated.js (1)
keyType(427-427)src/scripts/oauth2-key-generation.js (1)
scopes(579-579)
src/scripts/oauth2-key-generation.js (1)
src/scripts/api-key-generation.js (4)
generateBtn(140-140)generateBtn(238-238)modal(38-38)modal(124-124)
🪛 Biome (2.1.2)
src/controllers/applicationsContentController.js
[error] 570-570: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
🔇 Additional comments (24)
src/scripts/common.js (1)
1010-1013: LGTM: Null guard prevents runtime errors.The null check around chevron icon toggling is a good defensive practice.
src/pages/application/partials/apis.hbs (1)
48-49: LGTM: Per-keyType rendering paths implemented correctly.The introduction of separate production and sandbox key flows with distinct button labels, IDs, and modal invocations aligns with the PR's sandbox key management objectives. The changes correctly scope each key action to its environment.
Also applies to: 77-77, 81-81, 84-107, 210-210
src/pages/application/partials/overview.hbs (2)
67-99: LGTM: Per-keyType OAuth2 key rendering implemented correctly.The per-keyType iteration and conditional rendering for production/sandbox keys is well-structured. Button IDs and onclick handlers correctly incorporate the
keyTypeparameter.
12-13: LGTM: Proper disabled state handling for read-only mode.The conditional disabled styling and pointer-events: none for edit buttons when
@root.isReadOnlyModeis active provides good UX protection.Also applies to: 32-33, 37-38
src/pages/application/partials/oauth2-key-generation.hbs (1)
254-254: LGTM: Button ID correctly scoped by environment.The per-environment button ID suffix aligns with the PR's per-keyType management approach.
src/pages/application/page.hbs (1)
19-28: LGTM: Key modals correctly moved to manage-keys page.The architectural change to serve key-related modals from the dedicated manage-keys page aligns with the PR's objectives for sandbox key management. Retaining only the
keys-viewcomponent here is appropriate.src/routes/applicationsContentRoute.js (1)
23-28: LGTM: Manage-keys route follows established patterns.The new route for
/applications/:applicationId/manage-keyscorrectly applies the standard middleware stack and aligns with the PR's sandbox key management objectives.src/pages/application/partials/keys-instructions.hbs (1)
1-65: LGTM - Per-keyType modal structure is consistent.The template correctly iterates over
keyManagersMetadataandapplicationKeysto generate per-keyType instruction modals. The modal IDs, close handlers, and content element IDs all consistently use the-{{keyType}}suffix pattern.src/pages/application/partials/keys-modify.hbs (2)
1-4: LGTM - Per-keyType modal structure implemented correctly.The template correctly establishes per-keyType modals by iterating through
keyManagersMetadataandapplicationKeys. The modal IDs, form IDs, error containers, and all interactive elements consistently use the-{{keyType}}suffix pattern.
106-129: File does not exist in the repository.The file path
src/pages/application/partials/keys-modify.hbsdoes not exist in the repository. The review comment references a non-existent file, making the entire comment invalid. Verify the correct file path before proceeding with the review.Likely an incorrect or invalid review comment.
src/pages/application/partials/keys-generate.hbs (1)
28-30: Good addition of per-keyType iteration structure.The introduction of
{{#each applicationKeys}}with{{#let "keys" keys}}correctly enables per-keyType rendering of all key-related UI components within the OAuth modal.src/pages/manage-keys/page.hbs (1)
1-17: LGTM - Page template structure is consistent with per-keyType architecture.The template correctly loads the required assets and composes the necessary partials for the manage-keys workflow. The structure aligns with the broader per-keyType UI refactoring and follows the established pattern of partial page templates used throughout the codebase.
src/scripts/api-key-generation.js (2)
137-159: Approve the page-aware button management logic.The logic correctly detects whether the user is on the manage-keys page or APIs page and adjusts the UI accordingly. This separation ensures proper UX flow between the two contexts.
3-29: Approve the hash-based tab navigation.The DOMContentLoaded handler correctly implements hash-based navigation for production/sandbox tabs on the manage-keys page, improving deep-linking support.
src/controllers/applicationsContentController.js (2)
206-211: Approve the per-keyType API key fetching.The changes correctly fetch production and sandbox keys separately and organize them into the
apiDTO.apiKeysstructure, enabling proper isolation between key types.
721-731: All callers ofgetAPIKeyshave been properly updated with thekeyTypeparameter.Verification confirms all four call sites (lines 206, 207, 468, 469) pass the required
keyTypeparameter with values 'PRODUCTION' or 'SANDBOX'. No action needed.src/scripts/oauth2-key-generation.js (7)
2-21: Approve the defensive parameter and element validation.The added validation guards prevent runtime errors by checking for missing parameters, buttons, and button states before proceeding. The early returns ensure graceful failure without breaking the UI.
35-47: Approve the form existence check with user feedback.The guard prevents attempting to read from a non-existent form and provides clear user guidance to refresh the page. The button state reset ensures the UI remains usable.
49-63: Approve the subscriptions parsing error handling.The try/catch block safely handles malformed JSON data and provides clear error messaging to the user, preventing the application from crashing on invalid data.
117-129: Approve the safe element updates with existence checks.The code safely updates form fields only when the elements exist, preventing null reference errors and making the code more resilient to DOM variations.
535-572: Approve the keyType-aware token button selection.The logic correctly selects the appropriate token button (Sandbox or Production) based on the keyType parameter, and safely handles the scopes data with proper fallbacks.
840-860: Approve the environment-parameterized modal loading.The
loadKeysViewModalfunction properly accepts an environment parameter and includes error logging when the target modal is not found, improving debuggability.
862-924: Approve the keyType-aware modify modal with grant type validation.The
loadKeysModifyModalfunction correctly scopes modal operations by keyType and includes validation to ensure at least one grant type is selected before allowing updates.src/pages/application/partials/generate-oauth-key.hbs (1)
342-342: The onclick and container ID are consistent within each branch, but different patterns between branches should be intentional.Line 342's
showAdvanced('KMData_{{../name}}')correctly matches the container ID at line 352. Similarly, line 181's onclick correctly matches line 192's ID with the keyType suffix. The actual difference is architectural: whenkeys.consumerKeyexists ({{if}} branch at line 36), the ID includes keyType; when generating new credentials ({{else}} branch at line 177), it omits keyType. Verify this branch-level difference is intentional rather than an oversight, as the pattern is unusual compared to keys-generate.hbs which includes keyType in both branches.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/controllers/applicationsContentController.js (1)
731-741: Fix logic bug in getAPIKeys: loop overwrites multiple matching keys instead of collecting them.The endpoint integration is correct, but the filtering logic has a critical flaw. If the
/api-keys?apiId=${apiId}&keyType=${keyType}endpoint returns multiple keys matching the sameapplicationId, only the last one is preserved becauseapiKeys.keyandapiKeys.scopesare overwritten each iteration. Additionally, there's no null/undefined check onresponseDataorkey.applicationbefore use, which will cause errors if the response structure is unexpected. Consider refactoring to collect all matching keys in an array or add proper defensive checks.
♻️ Duplicate comments (3)
src/controllers/applicationsContentController.js (1)
391-660: Significant code duplication remains unaddressed.The new
loadApplicationKeysfunction (270 lines) duplicates almost all logic fromloadApplication(lines 125-389). The functions are nearly identical except for the template name ('manage-keys' vs 'application'). This creates substantial maintenance burden as bug fixes and feature additions must be synchronized across both functions.💡 Recommended refactoring approach
Extract the shared data-fetching and processing logic into a reusable helper function:
async function loadApplicationData(req, orgName, applicationId, viewName) { const orgID = await orgIDValue(orgName); let groupList = []; if (req.query.groups) { groupList.push(req.query.groups.split(" ")); } const subAPIs = await adminDao.getSubscribedAPIs(orgID, applicationId); const allAPIs = await apiMetadata.getAllAPIMetadata(orgID, groupList, viewName); // ... all the common data fetching/processing logic ... return { orgID, metaData, kMmetaData, subList, nonSubscribedAPIs, productionKeys, sandboxKeys, subscriptionScopes, isApiKey, otherAPICount, mcpAPICount, applicationReference }; }Then simplify both functions:
const loadApplication = async (req, res) => { const data = await loadApplicationData(req, orgName, applicationId, viewName); const templateContent = { ...data, applicationMetadata: { ...data.metaData, subscriptionCount: data.subList.length }, // ... application-specific fields ... }; // Render 'application' template }; const loadApplicationKeys = async (req, res) => { const data = await loadApplicationData(req, orgName, applicationId, viewName); const templateContent = { ...data, applicationMetadata: { ...data.metaData, subscriptionCount: data.subList.length }, // ... manage-keys-specific fields ... }; // Render 'manage-keys' template };src/pages/application/partials/manage-keys.hbs (2)
49-50: Duplicate element IDs remain unfixed.As flagged in the previous review,
id="consumerKey"(lines 49, 290) andid="consumerSecret"(lines 68, 309) are duplicated between Production and Sandbox sections. These duplicate IDs violate the HTML specification and will causegetElementByIdto return only the first match, breaking JavaScript functionality.🔎 Proposed fix
The IDs should be unique per environment. The inner input fields already use scoped IDs (e.g.,
consumer-key-{{../id}}-production), but the container divs do not. Since these container IDs don't appear to be referenced in the code, consider removing them entirely or making them unique:- <div id="consumerKey" + <div id="consumerKey-production" class="{{#unless keys.consumerSecret}} col-md-12 {{else}} col-md-6 {{/unless}}"> ... - <div id="consumerSecret" class="col-md-6" + <div id="consumerSecret-production" class="col-md-6"And for Sandbox (lines 290, 309):
- <div id="consumerKey" + <div id="consumerKey-sandbox" ... - <div id="consumerSecret" class="col-md-6" + <div id="consumerSecret-sandbox" class="col-md-6"Also applies to: 68-69
393-401: Duplicate accordion IDs remain unfixed.As flagged in the previous review, the Sandbox API Keys accordion (lines 393-401) reuses the same IDs (
headingFour,collapseFour) as the Production API Keys accordion (lines 151-158). This will cause Bootstrap's collapse functionality to malfunction, potentially causing both accordions to expand/collapse together or only one to work.🔎 Proposed fix
<div class="accordion-item"> - <h2 class="accordion-header" id="headingFour"> + <h2 class="accordion-header" id="headingSandboxApiKeys"> <button class="accordion-button collapsed" type="button" data-bs-toggle="collapse" - data-bs-target="#collapseFour" aria-expanded="false" aria-controls="collapseFour"> + data-bs-target="#collapseSandboxApiKeys" aria-expanded="false" aria-controls="collapseSandboxApiKeys"> API Keys </button> </h2> - <div id="collapseFour" class="accordion-collapse collapse" aria-labelledby="headingFour" + <div id="collapseSandboxApiKeys" class="accordion-collapse collapse" aria-labelledby="headingSandboxApiKeys" data-bs-parent="#sandboxAccordion">
🧹 Nitpick comments (4)
src/pages/application/partials/apis.hbs (2)
84-84: Refactor long parameter lists for better maintainability.The
openApiKeyModalcalls pass 9 parameters, making the code difficult to read, maintain, and validate. Consider refactoring to accept a configuration object instead.🔎 Suggested approach
Refactor the JavaScript function to accept a configuration object:
// In the related JavaScript file function openApiKeyModal(config) { const { projectId, refID, policyName, appRefID, appID, subID, subIDs, scopes, keyType } = config; // ... implementation }Then update the template:
-onclick="openApiKeyModal('{{projectId}}', '{{refID}}', '{{policyName}}', '{{../applicationMetadata.appMap.[0].appRefID}}', '{{../applicationMetadata.id}}', '{{subID}}', '{{getSubIDs ../subAPIs}}', '{{json scopes}}', 'PRODUCTION')" +onclick="openApiKeyModal({ projectId: '{{projectId}}', refID: '{{refID}}', policyName: '{{policyName}}', appRefID: '{{../applicationMetadata.appMap.[0].appRefID}}', appID: '{{../applicationMetadata.id}}', subID: '{{subID}}', subIDs: '{{getSubIDs ../subAPIs}}', scopes: '{{json scopes}}', keyType: 'PRODUCTION' })"Also applies to: 107-107
48-49: Render api-key modals conditionally based on key existence.The api-key partials at lines 48-49 are included unconditionally for each subscribed API (PRODUCTION and SANDBOX), adding 2N modal divs to the DOM. However, the conditional checks at lines 72 and 95 only display management buttons when
apiKeys.production.keyandapiKeys.sandbox.keyexist. Consider wrapping the partial includes in conditional blocks to avoid rendering unused modals in the DOM:src/pages/application/partials/manage-keys.hbs (1)
106-106: Consider refactoring functions with many inline parameters.Several onclick handlers pass many parameters inline, making them hard to read and maintain. For example:
- Line 106:
generateOauthKeywith 8 parameters- Line 129:
generateApplicationKeywith 10 parameters- Line 229, 471:
openApiKeyModalwith 9 parameters💡 Suggested approach
Consider using
data-*attributes to store parameters on the button elements and reading them in the handler function, similar to howdata-scopesis used on line 196:Then modify the JavaScript function to extract parameters from the button:
function generateOauthKey(button) { const appRefId = button.dataset.appRefId; const keyMappingId = button.dataset.keyMappingId; // ... etc }This improves maintainability and follows modern best practices.
Also applies to: 129-129, 229-229, 347-347, 370-370, 471-471
src/controllers/applicationsContentController.js (1)
579-583: Clarify optional chaining for static analysis.While the current code is technically safe (the
Array.isArraycheck guards against undefined), static analysis correctly flags that optional chaining in thefor...ofcan throw if it evaluates to undefined. For clarity and to silence the linter, explicitly check thatcpApplicationexists before accessing its properties.🔎 Proposed fix
let subscriptionScopes = []; - if (Array.isArray(cpApplication?.subscriptionScopes)) { - for (const scope of cpApplication?.subscriptionScopes) { + if (cpApplication && Array.isArray(cpApplication.subscriptionScopes)) { + for (const scope of cpApplication.subscriptionScopes) { subscriptionScopes.push(scope.key); } }This makes the null/undefined check explicit and matches the pattern used elsewhere in the codebase.
Note: The same pattern at lines 307-311 in
loadApplicationshould also be updated for consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/controllers/applicationsContentController.jssrc/defaultContent/partials/sidebar.hbssrc/pages/application/partials/api-key.hbssrc/pages/application/partials/apis.hbssrc/pages/application/partials/manage-keys.hbs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/defaultContent/partials/sidebar.hbs
- src/pages/application/partials/api-key.hbs
🧰 Additional context used
🪛 Biome (2.1.2)
src/controllers/applicationsContentController.js
[error] 580-580: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
🔇 Additional comments (8)
src/pages/application/partials/apis.hbs (1)
219-219: Consistent keyType parameter for MCP servers.The addition of the
'PRODUCTION'parameter to thegenerateAPIKeycall is consistent with the PR's objective of keyType-aware key management. MCP servers appropriately use only production keys.Ensure this parameter addition is verified as part of the function signature check in the earlier review comment.
src/controllers/applicationsContentController.js (6)
206-211: LGTM! API keys now correctly fetched per environment.The change to fetch production and sandbox keys separately and structure them as an object with
productionandsandboxproperties aligns well with the new manage-keys UI requirements. This enables environment-specific key display and management.
292-302: LGTM! Well-structured applicationKeys for template consumption.The per-keyManager
applicationKeysarray structure with production and sandbox objects provides a clean, consistent interface for the templates. The|| {}fallback ensures undefined values don't cause issues.
335-345: LGTM! Template content properly includes sandbox keys and applicationKeys.The addition of
sandboxKeysandapplicationKeysto the template content correctly exposes per-environment key data for the UI to render Production and Sandbox sections independently.
367-389: LGTM! Improved error handling with proper context.The enhanced error handling now differentiates between authentication errors (401) and general errors, providing appropriate error messages and maintaining context (orgName, applicationId, features) for the error page rendering.
804-806: LGTM! Module exports correctly updated.The new
loadApplicationKeysfunction is properly exported alongside the existing exports, making it available for use in routing.
722-724: No issues found with the endpoint change.The
/key-managersendpoint returns all key managers from the control plane, and client-side filtering adequately handles the selection of appropriate key managers by checking theenabledflag and filtering by name patterns. The claim about removing adevPortalAppEnvfilter cannot be substantiated as this parameter does not exist in the codebase.src/pages/application/partials/manage-keys.hbs (1)
2-4: Resource paths are correctly referenced and files exist in the repository.The template correctly references static assets at
/technical-styles/manage-keys.css,/technical-scripts/oauth2-key-generation.js, and/technical-scripts/api-key-generation.js. The corresponding source files exist in the codebase atsrc/styles/manage-keys.cssandsrc/scripts/for both JavaScript files.
|
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/scripts/oauth2-key-generation.js (1)
528-560: Critical: Const variable reassignment.Line 540 attempts to reassign
tokenBtn, which is declared asconston line 530. This will throw a TypeError in strict mode.🔎 Proposed fix
Change the declaration to
let:async function generateOauthKey(formId, appId, keyMappingId, keyManager, clientName, clientSecret, subscribedScopes, keyType) { // Use the clicked button if provided, otherwise fall back to default buttons - const tokenBtn = document.getElementById('tokenKeyBtn-' + keyType); + let tokenBtn = document.getElementById('tokenKeyBtn-' + keyType); const devAppId = tokenBtn?.dataset?.appId const scopeContainer = document.getElementById('scopeContainer-' + devAppId + '-' + keyType); const scopeInput = document.getElementById('scope-' + devAppId);
♻️ Duplicate comments (5)
src/pages/application/partials/keys-generate.hbs (1)
328-328: Critical: Handlebars syntax error remains unfixed.Line 328 still uses
{{@root.applicationMetadata/name}}with a forward slash instead of dot notation. This was flagged in a previous review and will cause undefined values or runtime errors.🔎 Proposed fix
- onClick="generateApplicationKey('applicationKeyGenerateForm-{{../id}}-{{keyType}}', '{{../../applicationMetadata.id}}', '{{keyType}}', '{{../name}}', '{{{../../applicationMetadata/name}}', '{{ json ../../subAPIs}}', '{{../../orgID}}', 'consumer-key-{{../id}}-{{keyType}}', 'consumer-secret-{{../id}}-{{keyType}}')"> + onClick="generateApplicationKey('applicationKeyGenerateForm-{{../id}}-{{keyType}}', '{{../../applicationMetadata.id}}', '{{keyType}}', '{{../name}}', '{{../../applicationMetadata.name}}', '{{ json ../../subAPIs}}', '{{../../orgID}}', 'consumer-key-{{../id}}-{{keyType}}', 'consumer-secret-{{../id}}-{{keyType}}')">src/pages/application/partials/oauth2-key-generation.hbs (1)
86-86: Major: Potential HTML injection/escaping issue remains unfixed.Line 86 still passes
{{json ../../subAPIs}}directly into theonClickattribute. This was flagged in a previous review as a potential XSS risk and can break the handler if the JSON contains quotes or special characters.🔎 Recommended approach
Use data attributes instead of inline JSON:
- onClick="generateOauthKey('applicationKeyGenerateForm-{{../id}}-{{#if ../../isProduction}}production{{else}}sandbox{{/if}}', '{{../../applicationMetadata.applicationId}}', '{{keys.keyMappingId}}', '{{../name}}', '{{keys.clientName}}', '{{json ../../subAPIs}}', '{{#if ../../isProduction}}PRODUCTION{{else}}SANDBOX{{/if}}')"> + data-sub-apis='{{json ../../subAPIs}}' + data-key-type='{{#if ../../isProduction}}PRODUCTION{{else}}SANDBOX{{/if}}' + onClick="generateOauthKey('applicationKeyGenerateForm-{{../id}}-{{#if ../../isProduction}}production{{else}}sandbox{{/if}}', '{{../../applicationMetadata.applicationId}}', '{{keys.keyMappingId}}', '{{../name}}', '{{keys.clientName}}', this)">Then update
generateOauthKeyto read frombutton.dataset.subApisandbutton.dataset.keyType.src/pages/application/partials/manage-keys.hbs (3)
49-50: Duplicate IDs remain unfixed.The
id="consumerKey"(Line 49) andid="consumerSecret"(Line 68) in the Production section are duplicated in the Sandbox section (Lines 290, 309). This violates the HTML spec and will causegetElementByIdto return only the first match, breaking JavaScript that targets these elements.🔎 Proposed fix
Since the inner input fields already use environment-scoped IDs like
consumer-key-{{../id}}-PRODUCTION, apply the same pattern to these container divs:- <div id="consumerKey" + <div id="consumerKey-PRODUCTION" class="{{#unless keys.consumerSecret}} col-md-12 {{else}} col-md-6 {{/unless}}">- <div id="consumerSecret" class="col-md-6" + <div id="consumerSecret-PRODUCTION" class="col-md-6"Apply corresponding changes in the Sandbox section (Lines 290, 309) using
-SANDBOXsuffix, and update any JavaScript selectors that reference these IDs.Also applies to: 68-69
151-158: Duplicate accordion IDs remain unfixed.The Production API Keys accordion uses
id="headingFour"(Line 151) andid="collapseFour"(Line 157), which are duplicated in the Sandbox API Keys accordion (Lines 397, 403). This will cause Bootstrap's collapse functionality to malfunction when toggling either accordion.🔎 Proposed fix
Rename the Production accordion IDs to be unique:
<div class="accordion-item"> - <h2 class="accordion-header" id="headingFour"> + <h2 class="accordion-header" id="headingProductionApiKeys"> <button class="accordion-button collapsed" type="button" data-bs-toggle="collapse" - data-bs-target="#collapseFour" aria-expanded="false" aria-controls="collapseFour"> + data-bs-target="#collapseProductionApiKeys" aria-expanded="false" aria-controls="collapseProductionApiKeys"> API Keys </button> </h2> - <div id="collapseFour" class="accordion-collapse collapse" aria-labelledby="headingFour" + <div id="collapseProductionApiKeys" class="accordion-collapse collapse" aria-labelledby="headingProductionApiKeys" data-bs-parent="#productionAccordion">Apply corresponding changes to Sandbox accordion IDs (Lines 397-403).
249-250: Duplicateno-subscriptionID remains unfixed.The
id="no-subscription"at Line 249 in the Production section is duplicated in the Sandbox section at Line 494. This violates the HTML spec and will cause JavaScript targeting this element to only find the first match.🔎 Proposed fix
Scope the IDs by environment:
- <div class="ps-3" id="no-subscription" + <div class="ps-3" id="no-subscription-production" style="{{#if subAPIs.length}}display:none{{/if}}">And for Sandbox at Line 494:
- <div class="ps-3" id="no-subscription" + <div class="ps-3" id="no-subscription-sandbox" style="{{#if subAPIs.length}}display:none{{/if}}">Update any JavaScript that references
#no-subscriptionto use the environment-specific IDs.
🧹 Nitpick comments (7)
src/controllers/applicationsContentController.js (1)
62-246: Excellent refactoring to eliminate code duplication.The new
loadApplicationDatahelper successfully addresses the code duplication issue flagged in previous reviews. It centralizes data loading logic for bothloadApplicationandloadApplicationKeys, making the codebase more maintainable.The function properly handles:
- API fetching and filtering
- Key management data assembly
- Per-keyType key segregation (PRODUCTION/SANDBOX)
- Subscription scope gathering
Minor suggestion: Consider adding JSDoc comments documenting the function's parameters and return structure, as this is now a core shared function.
src/pages/application/partials/manage-keys.hbs (6)
124-139: Add tooltip to Production Generate Key button for consistency.The Sandbox OAuth2 section includes a helpful tooltip explaining "This key is shared among all APIs" (Lines 380-383), but the Production section's Generate Key button (Lines 124-139) lacks this tooltip. This creates an inconsistency in the user experience.
🔎 Proposed fix
Add a tooltip container after the Generate Key button in Production, matching the Sandbox implementation:
</button> + <div class="tooltip-container ms-2"> + <i class="bi bi-info-circle"></i> + <span class="tooltip-text">This key is shared among all APIs</span> + </div> </div> </div>
337-342: Use consistent ID naming convention for Sandbox Modify button.The Sandbox Modify button uses
id="modifyKeyBtnSandbox"(Line 337, camelCase with no separator), while the Production Modify button usesid="modifyKeyBtn-PRODUCTION"(Line 96, dash-separated). This inconsistency makes the codebase harder to maintain and could lead to errors when referencing these IDs in JavaScript or CSS.🔎 Proposed fix
Rename the Sandbox button to match the Production naming pattern:
- <button id="modifyKeyBtnSandbox" + <button id="modifyKeyBtn-SANDBOX" class="common-btn-outlined btn-sm {{#if @root.isReadOnlyMode}}disabled{{/if}}" style="{{#if @root.isReadOnlyMode}}pointer-events: none; opacity: 0.5;{{/if}}" onclick="loadKeysModifyModal('SANDBOX')">Update any JavaScript or CSS selectors that reference
#modifyKeyBtnSandboxto use#modifyKeyBtn-SANDBOX.
126-137: Consider addingtitleattributes to Generate Key buttons.Per the PR feedback from Induwara04, adding a
titleattribute to the Generate Key buttons would improve accessibility and provide helpful hover tooltips. Currently, none of the Generate Key buttons (OAuth2 at Lines 126-137, 367-379; API Key at Lines 226-238, 471-483) havetitleattributes.🔎 Proposed enhancement
Add
titleattributes to all Generate Key buttons:For OAuth2 Generate Key buttons (Lines 126, 367):
<button data-cyid="generateKey-PRODUCTION" id="generateKeyBtn-PRODUCTION" class="common-btn-primary btn-sm {{#if @root.isReadOnlyMode}}disabled{{/if}}" + title="Generate OAuth2 application key for Production environment" style="{{#if @root.isReadOnlyMode}}pointer-events: none; opacity: 0.5;{{/if}}"For API Key Generate Key buttons (Lines 226, 471):
<button class="common-btn-outlined btn-sm me-1 {{#if @root.isReadOnlyMode}}disabled{{/if}}" id="generateKeyBtn-{{subID}}-PRODUCTION" + title="Generate API Key for this subscription" style="{{#if apiKeys.production.key}}display: none;{{else}}display: inline-flex;{{/if}}"Apply similar titles to Sandbox buttons with appropriate environment labels.
Also applies to: 226-238, 367-379, 471-483
254-255: Align no-subscription message text across environments.The "no subscriptions" message differs between Production (Lines 254-255: "Please subscribe to an API.") and Sandbox (Lines 499-500: "Please subscribe to an API to generate an API Key."). While the meaning is similar, the inconsistency may cause users to wonder if there's a functional difference between the two environments.
🔎 Proposed fix
Use identical wording in both sections. The more explicit Sandbox version is clearer:
<div class="info-title"> No subscriptions available for this application. Please subscribe to an - API. + API to generate an API Key. </div>Also applies to: 499-500
197-197: Remove unnecessary nested quotes fromdata-api-key-idattributes.The
data-api-key-idattributes contain nested single quotes:data-api-key-id="'{{apiKeys.production.key.[0].id}}'"(Lines 197, 211 for Production; Lines 442, 456 for Sandbox). While the JavaScript correctly sanitizes and strips these quotes before use, the pattern is unnecessarily convoluted. Simplify by removing the quotes from both the template and the dynamic assignment insrc/scripts/api-key-generation.js(lines 165, 172) to make the code clearer.Also applies to: 211-211, 442-442, 456-456
410-487: Add Policy column to Sandbox API Keys table for consistency with Production.The Sandbox API Keys table (lines 410-487) displays only 3 columns (Name, Version, API Key), while the Production table includes a 4th Policy column (line 189). Both sections use the
update-planpartial withsubscriptionPolicyDetailsandpolicyNamevariables, and both referencepolicyNamein theopenApiKeyModalfunction calls. This indicates policy information is available for Sandbox keys but simply not rendered. To maintain consistent UI patterns between environments, consider adding the Policy column to the Sandbox table header and rendering the policy name in each row.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/controllers/applicationsContentController.jssrc/controllers/devportalController.jssrc/pages/application/partials/apis.hbssrc/pages/application/partials/keys-generate.hbssrc/pages/application/partials/manage-keys.hbssrc/pages/application/partials/oauth2-key-generation.hbssrc/pages/application/partials/overview.hbssrc/scripts/common.jssrc/scripts/oauth2-key-generation.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/controllers/devportalController.js
- src/pages/application/partials/apis.hbs
- src/scripts/common.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/scripts/oauth2-key-generation.js (1)
src/scripts/api-key-generation.js (10)
generateBtn(140-140)generateBtn(238-238)normalState(88-88)normalState(208-208)normalState(286-286)tokenBtn(87-87)tokenBtn(285-285)scopeContainer(45-45)scopeContainer(91-91)scopeContainer(310-310)
🪛 Biome (2.1.2)
src/controllers/applicationsContentController.js
[error] 224-224: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
src/scripts/oauth2-key-generation.js
[error] 540-540: Can't assign tokenBtn because it's a constant.
This is where the variable is defined as constant.
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
🔇 Additional comments (4)
src/controllers/applicationsContentController.js (1)
46-56: LGTM: Clean profile builder helper.The new
buildProfilehelper cleanly extracts profile construction logic with appropriate null handling.src/scripts/oauth2-key-generation.js (3)
1-63: Good defensive programming with comprehensive validation.The added parameter validation, element existence checks, and error handling significantly improve robustness. The function now gracefully handles missing DOM elements and invalid data with clear error messages.
115-159: LGTM: Safe DOM manipulation with proper guards.The response handling code properly validates element existence before manipulation, preventing potential null reference errors. The environment suffix extraction using regex is clean and maintains the per-keyType scoping pattern.
823-949: Well-structured modal loading with keyType support.The modal loading functions consistently accept
keyTypeparameters and include proper existence checks with console error logging. This aligns well with the per-environment scoping pattern throughout the PR.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/scripts/oauth2-key-generation.js (1)
528-559: Cannot reassignconstvariabletokenBtn.The static analyzer correctly identified this issue.
tokenBtnis declared asconston line 529, but line 539 attempts to reassign it, which will throw aTypeErrorat runtime.🔎 Proposed fix
async function generateOauthKey(formId, appId, keyMappingId, keyManager, clientName, clientSecret, subscribedScopes, keyType) { - const tokenBtn = document.getElementById('tokenKeyBtn-' + keyType); + let tokenBtn = document.getElementById('tokenKeyBtn-' + keyType); const devAppId = tokenBtn?.dataset?.appId const scopeContainer = document.getElementById('scopeContainer-' + devAppId + '-' + keyType); const scopeInput = document.getElementById('scope-' + devAppId);
♻️ Duplicate comments (8)
src/scripts/api-key-generation.js (2)
1-1: Unused Express import still present.This issue was previously flagged. The
require("express")is unused in this client-side script andeis never referenced.
229-236: Loading state text still incorrect for the Generate button.This was previously discussed - the button being created is a "Generate Key" button (id:
generateKeyBtn-, title: "Generate {envLabel} Key"), but the loading state still says "Revoking...". Although this code is insiderevokeAPIKey, this button will be clicked by users to generate a new key after revocation, so users will see "Revoking..." when they click "Generate Key".🔎 Proposed fix
generateButton.innerHTML = ` <span class="button-normal-state"> <i class="bi bi-key me-1"></i> ${envLabel} </span> <span class="button-loading-state" style="display: none;"> - <span class="spinner-border spinner-border-sm me-1" role="status"> Revoking...</span> + <span class="spinner-border spinner-border-sm me-1" role="status"></span> + Generating... </span> `;src/pages/application/partials/manage-keys.hbs (5)
15-28: Reversedaria-selectedvalues on tab buttons.The Production tab is visually active (
class="key-btn active") but hasaria-selected="false", while the inactive Sandbox tab hasaria-selected="true". This accessibility issue was previously flagged and remains unresolved.🔎 Proposed fix
<li class="nav-item" role="presentation"> <button class="key-btn active" id="production-tab" data-bs-toggle="tab" data-bs-target="#production" - type="button" role="tab" aria-controls="production" aria-selected="false"> + type="button" role="tab" aria-controls="production" aria-selected="true"> Production </button> </li> <li class="nav-item" role="presentation"> <button class="key-btn" id="sandbox-tab" data-bs-toggle="tab" data-bs-target="#sandbox" type="button" - role="tab" aria-controls="sandbox" aria-selected="true"> + role="tab" aria-controls="sandbox" aria-selected="false"> Sandbox </button> </li>
49-50: DuplicateconsumerKeyandconsumerSecretIDs.The
id="consumerKey"andid="consumerSecret"are used in both Production (lines 49, 68) and Sandbox (lines 290, 309) sections. This violates HTML spec and causesgetElementByIdto return only the first match. This was previously flagged and remains unresolved.🔎 Proposed fix
For Production section:
- <div id="consumerKey" + <div id="consumerKey-PRODUCTION" ... - <div id="consumerSecret" class="col-md-6" + <div id="consumerSecret-PRODUCTION" class="col-md-6"For Sandbox section:
- <div id="consumerKey" + <div id="consumerKey-SANDBOX" ... - <div id="consumerSecret" class="col-md-6" + <div id="consumerSecret-SANDBOX" class="col-md-6"Also applies to: 68-69, 290-291, 309-310
163-172: Missing Policy column header in table.The table header defines Name, Version, and API Key columns, but the table body includes a Policy column at line 189. This causes column misalignment. This was previously flagged and remains unresolved.
🔎 Proposed fix
<thead> <tr> <th class="app-th">Name</th> <th class="app-th">Version</th> + <th class="app-th">Policy</th> <th class="app-th"> {{#if isApiKey}} API Key {{/if}} </th> </tr> </thead>Apply the same fix to the Sandbox table (lines 409-418).
249-250: Duplicateno-subscriptionID.The
id="no-subscription"is used in both Production (line 249) and Sandbox (line 494) sections. This was previously flagged and remains unresolved.🔎 Proposed fix
- <div class="ps-3" id="no-subscription" + <div class="ps-3" id="no-subscription-PRODUCTION"And for Sandbox:
- <div class="ps-3" id="no-subscription" + <div class="ps-3" id="no-subscription-SANDBOX"Also applies to: 494-495
396-404: Duplicate accordion IDs between Production and Sandbox API Keys sections.The Sandbox API Keys accordion uses the same IDs (
headingFour,collapseFour) as the Production API Keys accordion (lines 151-158). This will cause Bootstrap's collapse functionality to malfunction.🔎 Proposed fix
<div class="accordion-item"> - <h2 class="accordion-header" id="headingFour"> + <h2 class="accordion-header" id="headingSandboxApiKeys"> <button class="accordion-button collapsed" type="button" data-bs-toggle="collapse" - data-bs-target="#collapseFour" aria-expanded="false" aria-controls="collapseFour"> + data-bs-target="#collapseSandboxApiKeys" aria-expanded="false" aria-controls="collapseSandboxApiKeys"> API Keys </button> </h2> - <div id="collapseFour" class="accordion-collapse collapse" aria-labelledby="headingFour" + <div id="collapseSandboxApiKeys" class="accordion-collapse collapse" aria-labelledby="headingSandboxApiKeys" data-bs-parent="#sandboxAccordion">src/controllers/applicationsContentController.js (1)
220-228: Remove redundant optional chaining in for...of loop.While the
Array.isArrayguard prevents entering the loop with undefined, the static analyzer correctly flags the optional chaining on line 224 as unsafe. Since the condition already verifiedcpApplicationexists and has an arraysubscriptionScopes, the optional chaining is redundant and should be removed for clarity.🔎 Proposed fix
let subscriptionScopes = []; if (applicationReference) { let cpApplication = await getAPIMApplication(req, applicationReference); - if (cpApplication && Array.isArray(cpApplication?.subscriptionScopes)) { - for (const scope of cpApplication?.subscriptionScopes) { + if (cpApplication && Array.isArray(cpApplication.subscriptionScopes)) { + for (const scope of cpApplication.subscriptionScopes) { subscriptionScopes.push(scope.key); } } }
🧹 Nitpick comments (5)
src/services/adminService.js (1)
1280-1298: Consider adding parameter validation and logging forkeyType.The
mapKeysfunction now accepts akeyTypeparameter, which is a good addition for supporting sandbox vs. production key management. However, the function doesn't validate thatkeyTypeis defined or log its value for debugging purposes.🔎 Suggested improvements
async function mapKeys(req, clientID, keyManager, applicationId, keyType) { logger.info('Mapping existing client ID with application', { - applicationId + applicationId, + keyType }); + if (!keyType) { + throw new CustomError(400, "Bad Request", "keyType is required for key mapping"); + } const body = { "consumerKey": clientID, "keyManager": keyManager, "keyType": keyType, };src/styles/manage-keys.css (4)
3-5: Optional: Simplify zero value.
margin-top: 0rem;can be simplified tomargin-top: 0;since zero doesn't require a unit.🔎 Proposed fix
.keys-section { - margin-top: 0rem; + margin-top: 0; }
61-67: Avoid!importantin new stylesheets.The
!importantflag on line 64 suggests a specificity conflict. Since this is a new file, consider restructuring selectors to avoid needing!important, which reduces maintainability.
113-118: Prefer class selectors over hardcoded IDs.Hardcoding specific accordion IDs (
#headingOne,#headingTwo, etc.) doesn't scale if additional accordions are added. Consider using a class selector like.accordion-headerinstead.🔎 Proposed refactor
-#headingOne.accordion-header, -#headingTwo.accordion-header, -#headingThree.accordion-header, -#headingFour.accordion-header { +.accordion-header { margin-bottom: 0; }
130-146: Note: Scrollbar styling is webkit-only.The custom scrollbar styles (
::-webkit-scrollbar-*) only work in Chromium and Safari. Firefox and other browsers will display default scrollbars. This is acceptable as progressive enhancement, but ensure the default scrollbars provide adequate usability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/controllers/applicationsContentController.jssrc/defaultContent/styles/manage-keys.csssrc/pages/application/partials/manage-keys.hbssrc/scripts/api-key-generation.jssrc/scripts/oauth2-key-generation.jssrc/services/adminService.jssrc/styles/manage-keys.css
🧰 Additional context used
🧬 Code graph analysis (2)
src/scripts/api-key-generation.js (1)
src/middlewares/ensureAuthenticated.js (1)
keyType(427-427)
src/scripts/oauth2-key-generation.js (1)
src/scripts/api-key-generation.js (2)
modal(10-10)modal(96-96)
🪛 Biome (2.1.2)
src/controllers/applicationsContentController.js
[error] 224-224: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
src/scripts/oauth2-key-generation.js
[error] 539-539: Can't assign tokenBtn because it's a constant.
This is where the variable is defined as constant.
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
🔇 Additional comments (17)
src/scripts/api-key-generation.js (4)
3-55: LGTM - keyType parameter properly propagated in openApiKeyModal.The function signature update and keyType suffix additions to all DOM element IDs are correctly implemented, enabling per-environment key management.
57-175: LGTM - generateAPIKey function correctly handles keyType.The keyType is properly passed to the API payload (uppercased), and all DOM element lookups correctly include the keyType suffix. The conditional logic for manage-keys vs apis.hbs pages is well-structured.
177-253: LGTM - revokeAPIKey correctly handles per-environment UI state.The function properly targets keyType-specific buttons and handles UI state transitions for both manage-keys and apis.hbs pages.
255-311: LGTM - regenerateAPIKey correctly scoped to keyType.All element references properly include the keyType suffix, and scope handling is correctly per-environment.
src/scripts/oauth2-key-generation.js (5)
1-70: Good defensive parameter validation added.The upfront validation of required parameters with early returns and clear error messages improves robustness. The form parsing wrapped in try-catch is a good practice.
115-159: LGTM - Environment-scoped element updates.The keyType suffix is consistently applied to all element IDs (consumerKey, consumerSecret, app-ref, key-map, modalBody, curlDisplay, kmData), and null checks properly guard against missing elements.
388-409: LGTM - updateApplicationKey correctly uses keyType.The function properly extracts and uses keyType for scoped element lookups and closes the correct modal.
822-948: LGTM - Modal loading functions correctly accept keyType.All modal loading functions (
loadKeysViewModal,loadKeysModifyModal,loadKeysTokenModal,loadKeysInstructionsModal) properly construct modal IDs with keyType suffix and include good error handling for missing modals.
988-1005: LGTM - copyToken updated to use tokenId parameter.The function now accepts a generic
tokenIdparameter instead ofKMName, enabling environment-specific token copying.src/pages/application/partials/manage-keys.hbs (2)
89-122: LGTM - Key action containers and buttons correctly scoped by environment.The
keyActionsContainer,viewKeyBtn,tokenKeyBtn, andtokenInstructionsBtnelements now have proper environment-specific IDs (-PRODUCTION/-SANDBOX), addressing previous duplicate ID concerns.Also applies to: 330-363
143-145: LGTM - Error containers correctly scoped by environment.Both Production and Sandbox sections now have their own
keyGenerationErrorContainerwith environment-specific suffixes.Also applies to: 389-391
src/defaultContent/styles/manage-keys.css (1)
1-201: LGTM - Well-structured CSS with design tokens.The stylesheet properly uses CSS variables for theming consistency, includes responsive scrollbar styling, and has sensible layout rules for the accordion and tab components. The
!importantdeclarations on focus states are appropriate for overriding Bootstrap's default outline styles.src/controllers/applicationsContentController.js (5)
46-56: LGTM - Clean profile builder helper.The
buildProfilefunction properly extracts profile construction logic with a null check for missing user data.
62-246: LGTM - Excellent refactor to eliminate code duplication.The
loadApplicationDatahelper consolidates the shared data loading logic betweenloadApplicationandloadApplicationKeys, addressing the previous code duplication concern. The function properly handles:
- API subscriptions and key retrieval
- Key manager metadata processing
- Per-environment key separation (productionKeys/sandboxKeys)
- Scope extraction with proper null checks
327-417: LGTM - loadApplication properly refactored.The function now delegates data loading to
loadApplicationDataand focuses on template-specific content assembly and rendering. Error handling is comprehensive with proper logging context.
419-515: LGTM - loadApplicationKeys follows the same pattern.The new function correctly reuses
loadApplicationDataand assembles the manage-keys specific template content. Error handling mirrorsloadApplicationfor consistency.
586-596: LGTM - getAPIKeys now accepts keyType parameter.The function signature update enables per-environment API key retrieval, aligning with the broader keyType handling across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/pages/application/partials/manage-keys.hbs (2)
5-5: Add SRI integrity attribute to the CDN link.The Bootstrap Icons CDN link lacks Subresource Integrity (SRI) attributes, which protect against compromised CDN content.
🔎 Recommended enhancement
- <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/[email protected]/font/bootstrap-icons.css"> + <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/[email protected]/font/bootstrap-icons.css" + integrity="sha384-..." crossorigin="anonymous">You can generate the integrity hash at https://www.srihash.org/ or use a more recent version with built-in SRI support.
106-106: Consider refactoring complex onclick handlers.The inline
onclickhandlers pass 8-10 parameters, making them difficult to read and maintain. Consider usingdata-*attributes to store metadata and delegating to event handlers in JavaScript.Example approach
<button id="tokenKeyBtn-PRODUCTION" class="common-btn-outlined btn-sm" data-form-id="applicationKeyGenerateForm-{{../id}}-PRODUCTION" data-app-ref-id="{{keys.appRefID}}" data-key-mapping-id="{{keys.keyMappingId}}" data-key-manager-name="{{../name}}" data-app-name="{{../../applicationMetadata/name}}" data-consumer-secret="{{keys.consumerSecret}}" data-subscription-scopes="{{json @root.subscriptionScopes}}" data-key-type="PRODUCTION">Then attach a delegated event listener in JavaScript that reads the
data-*attributes.Also applies to: 129-129
Also applies to: 129-129
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/defaultContent/styles/manage-keys.csssrc/pages/application/partials/manage-keys.hbssrc/services/adminService.jssrc/styles/manage-keys.css
🚧 Files skipped from review as they are similar to previous changes (2)
- src/defaultContent/styles/manage-keys.css
- src/styles/manage-keys.css
🔇 Additional comments (3)
src/services/adminService.js (2)
1244-1247: LGTM! Validation and call site correctly updated.The validation check for
tokenDetails.keyTypehas been properly added before callingmapKeys, and the function call now passes the validatedkeyTypeparameter. This addresses both previous review comments:
- The undefined
keyTypeissue has been resolved by passingtokenDetails.keyType.- The requested validation has been implemented with an appropriate error message.
The validation placement inside the transaction ensures proper rollback if the check fails.
1283-1290: LGTM! Function signature correctly updated for per-key-type handling.The
mapKeysfunction signature now acceptskeyTypeas a parameter (line 1283) and correctly uses it in the request body (line 1290). This enables dynamic key-type assignment for PRODUCTION and SANDBOX keys, aligning with the PR objectives for sandbox key management.src/pages/application/partials/manage-keys.hbs (1)
15-28: Tab navigation accessibility is correctly implemented.The
aria-selectedattributes now correctly match the visual state: Production tab (active) hasaria-selected="true"and Sandbox tab (inactive) hasaria-selected="false".
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/scripts/api-key-generation.js (2)
1-1: Remove unused Express import (duplicate of past review).The
const e = require("express")import is still present despite being marked as addressed in commit 4335b0b. This is dead code—Express is a server-side framework and cannot be used in client-side browser scripts. The variableeis never referenced.🔎 Proposed fix
-const e = require("express"); -
227-234: Incorrect loading state text in dynamically created Generate button.The Generate Key button created after revocation (Lines 227-234) has loading state text "Revoking..." (Line 232), but this button will be used for generating a new key, not revoking. When a user clicks this button, they should see "Generating..." not "Revoking...".
The loading state text should reflect the button's action (generate), not the context where the button creation code is located (revoke function).
🔎 Proposed fix
<span class="button-loading-state" style="display: none;"> - <span class="spinner-border spinner-border-sm me-1" role="status"> Revoking...</span> + <span class="spinner-border spinner-border-sm me-1" role="status"></span> + Generating... </span>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/pages/application/partials/apis.hbssrc/pages/application/partials/keys-view.hbssrc/pages/application/partials/manage-keys.hbssrc/scripts/api-key-generation.jssrc/scripts/oauth2-key-generation.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/application/partials/keys-view.hbs
🧰 Additional context used
🧬 Code graph analysis (2)
src/scripts/oauth2-key-generation.js (1)
src/scripts/api-key-generation.js (15)
generateBtn(112-112)generateBtn(208-208)normalState(60-60)normalState(178-178)normalState(256-256)loadingState(61-61)loadingState(179-179)loadingState(257-257)tokenBtn(59-59)tokenBtn(255-255)scopeContainer(17-17)scopeContainer(63-63)scopeContainer(280-280)modal(10-10)modal(96-96)
src/scripts/api-key-generation.js (3)
src/controllers/devportalController.js (3)
apiID(151-151)apiKeyID(232-232)apiKeyID(248-248)src/middlewares/ensureAuthenticated.js (1)
keyType(427-427)src/controllers/apiContentController.js (4)
apiID(222-222)apiID(450-450)apiID(497-497)apiID(664-664)
🪛 Biome (2.1.2)
src/scripts/oauth2-key-generation.js
[error] 562-562: Can't assign tokenBtn because it's a constant.
This is where the variable is defined as constant.
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
🔇 Additional comments (5)
src/pages/application/partials/manage-keys.hbs (1)
1-520: Past duplicate ID issues have been successfully resolved.Previous review comments flagged multiple duplicate ID issues (consumerKey/consumerSecret, accordion IDs, data-api-key-id extra quotes, aria-selected). All of these have been properly fixed in the current implementation:
- Consumer key/secret IDs are now scoped with
-PRODUCTION/-SANDBOXsuffixes- Accordion IDs use unique identifiers per environment
- Data attributes no longer have extra quotes
- ARIA attributes are correctly set
The implementation properly supports per-environment key management with distinct UI elements.
src/pages/application/partials/apis.hbs (1)
48-50: LGTM: Per-key-type rendering correctly implemented.The dual invocation of the
api-keypartial for both PRODUCTION and SANDBOX key types (Lines 48-49) aligns with the PR's per-environment key management architecture. The addition of WEBSUB to the supported API types (Line 50) properly extends support to WebSub APIs.src/scripts/api-key-generation.js (1)
3-55: Well-structured per-environment key management.The addition of the
keyTypeparameter throughout the key lifecycle functions (openApiKeyModal, generateAPIKey, revokeAPIKey, regenerateAPIKey) properly implements per-environment scoping. All DOM selectors are correctly suffixed with keyType (e.g.,-PRODUCTION,-SANDBOX), ensuring Production and Sandbox keys are managed independently.The conditional page detection logic (Lines 109-129, 213-238) appropriately handles UI differences between the manage-keys and APIs pages.
src/scripts/oauth2-key-generation.js (2)
1-310: Excellent defensive programming in generateApplicationKey.The function has been significantly hardened with:
- Parameter validation and early returns (Lines 2-6)
- DOM element existence checks before operations (Lines 10-13, 18-21, 36-47)
- Error container management with user-friendly messages (Lines 24-28, 280-284, 297-301)
- Try-catch blocks for JSON operations (Lines 51-63)
- Consistent keyType-scoped element selectors throughout
This greatly improves reliability and user experience compared to the typical implementation.
845-971: Modal management properly updated for per-environment scoping.All modal loading functions (loadKeysViewModal, loadKeysModifyModal, loadKeysTokenModal, loadKeysInstructionsModal) correctly accept the
keyTypeparameter and use environment-scoped modal IDs. The null checks with error logging (Lines 847-850, 866-869, 956-959, 966-969) provide good defensive programming.The
copyTokensignature change fromKMNametotokenId(Line 1011) properly aligns with environment-specific token identifiers.
| style="{{#if @root.isReadOnlyMode}}pointer-events: none; opacity: 0.5;{{/if}}" | ||
| data-app-ref-id="" | ||
| title="Generate Production Key" | ||
| onclick="openApiKeyModal('{{projectId}}', '{{refID}}', '{{policyName}}', '{{../applicationMetadata.appMap.[0].appRefID}}', '{{../applicationMetadata.id}}', '{{subID}}', '{{getSubIDs ../subAPIs}}', '{{json scopes}}', 'PRODUCTION')"> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for appMap initialization and population to verify it always has at least one element
rg -n "appMap\s*[=:]" --type=js -A5 -B2Repository: wso2/api-developer-portal-core
Length of output: 507
🏁 Script executed:
# Also check the template file to understand the context of the array access
cat -n src/pages/application/partials/apis.hbs | sed -n '70,120p'Repository: wso2/api-developer-portal-core
Length of output: 4559
🏁 Script executed:
# Search for any appMap usage patterns to understand if it's guaranteed to have elements
rg -n "appMap" --type=js -B2 -A2 | head -100Repository: wso2/api-developer-portal-core
Length of output: 2438
Add defensive checks for appMap array access in onclick handlers.
Lines 84 and 107 directly access {{../applicationMetadata.appMap.[0].appRefID}} without verifying that appMap exists or has elements. Since appMap is only initialized when DP_APP_KEY_MAPPINGs is present (see src/dto/application.js:26), accessing index [0] can fail silently. The controller code already guards this access (src/controllers/applicationsContentController.js:83), but the template does not.
Wrap the array access in a Handlebars conditional:
🤖 Prompt for AI Agents
In src/pages/application/partials/apis.hbs around line 84 (and apply same fix at
line 107), the template directly reads
../applicationMetadata.appMap.[0].appRefID in onclick handlers which can throw
when appMap is missing or empty; wrap that access in a Handlebars conditional so
you output the value only when appMap exists and has at least one element and
otherwise emit an empty string — e.g. replace the direct expression with a
conditional block that checks ../applicationMetadata.appMap and uses
../applicationMetadata.appMap.[0].appRefID when present, else '' (apply
identical change to the other occurrence).
| </td> | ||
| <td onclick="window.location.href='{{@root.baseUrl}}/api/{{apiHandle}}'" | ||
| class="app-td" style="cursor: pointer;">{{version}}</td> | ||
| <td class="app-td"> |
There was a problem hiding this comment.
Add missing id attribute to policy cell for consistency.
The Sandbox API Keys table policy cell (Line 442) is missing the id="policy_{{subID}}" attribute that exists in the Production section (Line 192). This inconsistency may cause issues if JavaScript code expects to query this element by ID.
🔎 Proposed fix
- <td class="app-td">
+ <td id="policy_{{subID}}" class="app-td" style="display: none;">
+ {{policyName}}</td>
+ <td class="app-td">
{{#if (contains security 'api_key')}}🤖 Prompt for AI Agents
In src/pages/application/partials/manage-keys.hbs around line 442, the Sandbox
API Keys table policy cell is missing the id attribute used in Production; add
id="policy_{{subID}}" to the <td> at line 442 so it matches the Production row
and any JS that queries by that id; ensure the template variable (subID)
produces a unique id for each row and that the attribute name exactly matches
the Production implementation.
| const tokenBtn = document.getElementById('tokenKeyBtn-' + keyType); | ||
| const devAppId = tokenBtn?.dataset?.appId | ||
| const scopeContainer = document.getElementById('scopeContainer-' + devAppId); | ||
| const scopeContainer = document.getElementById('scopeContainer-' + devAppId + '-' + keyType); | ||
| const scopeInput = document.getElementById('scope-' + devAppId); | ||
|
|
||
| if (!(subscribedScopes)) { | ||
| // In the regenerate token request, the scopes are fetched from the span tags | ||
| const scopeElements = document.querySelectorAll(`#scopeContainer-${devAppId} .span-tag`); | ||
| const scopeElements = document.querySelectorAll(`#scopeContainer-${devAppId}-${keyType} .span-tag`); | ||
| subscribedScopes = Array.from(scopeElements).map(el => el.textContent.replace('×', '').trim()); | ||
| scopeContainer.setAttribute('data-scopes', JSON.stringify(subscribedScopes)); | ||
| tokenBtn = document.getElementById('regenerateKeyBtn'); | ||
| tokenBtn = document.getElementById('regenerateKeyBtn-' + keyType); |
There was a problem hiding this comment.
Fix const reassignment causing runtime error.
Line 552 declares tokenBtn as const, but Line 562 attempts to reassign it in the regenerate token path. This will throw a TypeError at runtime when users regenerate tokens.
🔎 Proposed fix
Change the declaration from const to let:
- const tokenBtn = document.getElementById('tokenKeyBtn-' + keyType);
+ let tokenBtn = document.getElementById('tokenKeyBtn-' + keyType);Based on static analysis hints.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const tokenBtn = document.getElementById('tokenKeyBtn-' + keyType); | |
| const devAppId = tokenBtn?.dataset?.appId | |
| const scopeContainer = document.getElementById('scopeContainer-' + devAppId); | |
| const scopeContainer = document.getElementById('scopeContainer-' + devAppId + '-' + keyType); | |
| const scopeInput = document.getElementById('scope-' + devAppId); | |
| if (!(subscribedScopes)) { | |
| // In the regenerate token request, the scopes are fetched from the span tags | |
| const scopeElements = document.querySelectorAll(`#scopeContainer-${devAppId} .span-tag`); | |
| const scopeElements = document.querySelectorAll(`#scopeContainer-${devAppId}-${keyType} .span-tag`); | |
| subscribedScopes = Array.from(scopeElements).map(el => el.textContent.replace('×', '').trim()); | |
| scopeContainer.setAttribute('data-scopes', JSON.stringify(subscribedScopes)); | |
| tokenBtn = document.getElementById('regenerateKeyBtn'); | |
| tokenBtn = document.getElementById('regenerateKeyBtn-' + keyType); | |
| let tokenBtn = document.getElementById('tokenKeyBtn-' + keyType); | |
| const devAppId = tokenBtn?.dataset?.appId | |
| const scopeContainer = document.getElementById('scopeContainer-' + devAppId + '-' + keyType); | |
| const scopeInput = document.getElementById('scope-' + devAppId); | |
| if (!(subscribedScopes)) { | |
| // In the regenerate token request, the scopes are fetched from the span tags | |
| const scopeElements = document.querySelectorAll(`#scopeContainer-${devAppId}-${keyType} .span-tag`); | |
| subscribedScopes = Array.from(scopeElements).map(el => el.textContent.replace('×', '').trim()); | |
| scopeContainer.setAttribute('data-scopes', JSON.stringify(subscribedScopes)); | |
| tokenBtn = document.getElementById('regenerateKeyBtn-' + keyType); |
🧰 Tools
🪛 Biome (2.1.2)
[error] 562-562: Can't assign tokenBtn because it's a constant.
This is where the variable is defined as constant.
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
🤖 Prompt for AI Agents
In src/scripts/oauth2-key-generation.js around lines 552 to 562, tokenBtn is
declared as const but later reassigned in the regenerate token branch which
causes a runtime TypeError; change the tokenBtn declaration from const to let so
it can be reassigned, and keep the rest of the logic the same (ensure any
subsequent references still use tokenBtn and that null/undefined checks remain
intact).
2 participants
Purpose
Design Doc: https://docs.google.com/document/d/1JtHmyJ3IFMZTa8hdwPdnyOSfO_hKzgkLSDdgHeWnCy8/edit?tab=t.0#heading=h.7h46fu1s6qhy
Application overview page
Application overview page with themeing
Manage Keys page with themeing
✏️ Tip: You can customize this high-level summary in your review settings.
Summary by CodeRabbit
New Features
UI Changes
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.