Enhance key manager handling by fetching production and sandbox key managers in parallel#648
Open
Thushani-Jayasekera wants to merge 2 commits into
Open
Enhance key manager handling by fetching production and sandbox key managers in parallel#648Thushani-Jayasekera wants to merge 2 commits into
Thushani-Jayasekera wants to merge 2 commits into
Conversation
…anagers in parallel
WalkthroughFetch and enrich key managers for both PROD and SANDBOX concurrently, add OAuth2 endpoint settings to config, tag key managers by environment and associate environment-specific application keys, and update templates to render environment-gated key-manager sections. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller as applicationsContentController
participant Service as adminService
participant Config as sample_config.json
participant Template as manage-keys.hbs
Client->>Controller: loadApplicationData(req)
par Fetch environments in parallel
Controller->>Service: getAPIMKeyManagers(req, 'prod')
Service->>Config: read residentKeyManager OAuth2 endpoints
Service-->>Controller: prod key managers
and
Controller->>Service: getAPIMKeyManagers(req, 'sandbox')
Service->>Config: read residentKeyManager OAuth2 endpoints
Service-->>Controller: sandbox key managers
end
Controller->>Controller: filterKeyManagers(prod, 'PROD')
Controller->>Controller: enrichKeyManager(prod list) → tag devPortalAppEnv='PROD'
Controller->>Controller: filterKeyManagers(sandbox, 'SANDBOX')
Controller->>Controller: enrichKeyManager(sandbox list) → tag devPortalAppEnv='SANDBOX'
Controller->>Controller: combine keyManagersMetadata and attach env-specific applicationKeys
Controller->>Template: render manage-keys with keyManagersMetadata and hasProd/hasSandbox flags
Template-->>Client: HTML with env-gated key-manager sections
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/controllers/applicationsContentController.js`:
- Around line 177-180: The current Promise.all call makes rendering depend on
both getAPIMKeyManagers('prod') and getAPIMKeyManagers('sandbox') succeeding;
change this so each call is handled independently (use Promise.allSettled or
separate try/catch for getAPIMKeyManagers) and ensure rawProdKeyManagers and
rawSandboxKeyManagers each fall back to [] on failure, leaving the successful
tab unaffected; update the code around the Promise.all usage and any downstream
logic that assumes arrays to tolerate the [] fallback.
In `@src/pages/application/partials/manage-keys.hbs`:
- Around line 45-47: The outer guard using keyManagersMetadata is too broad
because keyManagersMetadata now mixes PROD+SANDBOX; update the template logic to
filter or detect per-environment presence instead of relying on the global
array: create an environment-filtered list or compute a per-env boolean (e.g.,
hasResidentKM or hasKeyManagersForEnv) and use that flag in the conditional
rendering (replace the current {{`#if` keyManagersMetadata}} and the per-item
{{`#if` (eq devPortalAppEnv 'PROD')}} checks with a single env-aware predicate) so
each tab shows either the key generation UI or the fallback message correctly;
apply the same change to the other repeated locations where keyManagersMetadata
is used.
In `@src/services/adminService.js`:
- Around line 1604-1611: The current merge of prodResponse and sandboxResponse
into a single anonymous list loses each key-manager's environment, which breaks
downstream validation (e.g., devportalController.js). Fix by preserving the
devPortalAppEnv for each item: map over prodResponse.data.list and
sandboxResponse.data.list (from the util.apiRequest calls) and add a property
like devPortalAppEnv: 'prod' or 'sandbox' (or alternatively return an object
with separate prod and sandbox arrays), then return the combined list so every
returned key-manager includes its originating environment for correct
environment-aware validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 384b161b-2430-416e-9413-c38c4cad4846
📒 Files selected for processing (4)
sample_config.jsonsrc/controllers/applicationsContentController.jssrc/pages/application/partials/manage-keys.hbssrc/services/adminService.js
Comment on lines
+45
to
+47
| {{#if keyManagersMetadata}} | ||
| {{#each keyManagersMetadata}} | ||
| {{#if (eq devPortalAppEnv 'PROD')}} |
There was a problem hiding this comment.
The OAuth2 fallback can disappear per tab.
keyManagersMetadata is now a combined prod+sandbox array, but the outer guard is still global. If one tab has no matching key managers and the other does, these new devPortalAppEnv checks render nothing and the user gets a blank OAuth2 section instead of the fallback message. Drive each tab from an environment-filtered list or a per-env presence flag.
Based on learnings, if keyManagersMetadata includes the Resident key manager, use a presence check such as hasResidentKM as the sole determinant for whether to show the key generation UI or the fallback message.
Also applies to: 263-264, 612-615, 832-833
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/application/partials/manage-keys.hbs` around lines 45 - 47, The
outer guard using keyManagersMetadata is too broad because keyManagersMetadata
now mixes PROD+SANDBOX; update the template logic to filter or detect
per-environment presence instead of relying on the global array: create an
environment-filtered list or compute a per-env boolean (e.g., hasResidentKM or
hasKeyManagersForEnv) and use that flag in the conditional rendering (replace
the current {{`#if` keyManagersMetadata}} and the per-item {{`#if` (eq
devPortalAppEnv 'PROD')}} checks with a single env-aware predicate) so each tab
shows either the key generation UI or the fallback message correctly; apply the
same change to the other repeated locations where keyManagersMetadata is used.
Comment on lines
+1604
to
+1611
| const [prodResponse, sandboxResponse] = await Promise.all([ | ||
| util.apiRequest('GET', `${controlPlaneGwUrl}/key-managers?devPortalAppEnv=prod`, headers, null, cpOrgId), | ||
| util.apiRequest('GET', `${controlPlaneGwUrl}/key-managers?devPortalAppEnv=sandbox`, headers, null, cpOrgId) | ||
| ]); | ||
| return [ | ||
| ...(prodResponse.data.list || []), | ||
| ...(sandboxResponse.data.list || []) | ||
| ]; |
There was a problem hiding this comment.
Keep the environment on each returned key manager.
This helper now flattens prod and sandbox into one anonymous list. Callers like src/controllers/devportalController.js:472-481 only validate by km.name, so a match from the wrong environment can still pass. Return devPortalAppEnv with each item, or keep separate arrays, so downstream validation stays environment-aware.
Possible direction
- return [
- ...(prodResponse.data.list || []),
- ...(sandboxResponse.data.list || [])
- ];
+ return [
+ ...((prodResponse.data.list || []).map(km => ({ ...km, devPortalAppEnv: 'PROD' }))),
+ ...((sandboxResponse.data.list || []).map(km => ({ ...km, devPortalAppEnv: 'SANDBOX' }))),
+ ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/adminService.js` around lines 1604 - 1611, The current merge of
prodResponse and sandboxResponse into a single anonymous list loses each
key-manager's environment, which breaks downstream validation (e.g.,
devportalController.js). Fix by preserving the devPortalAppEnv for each item:
map over prodResponse.data.list and sandboxResponse.data.list (from the
util.apiRequest calls) and add a property like devPortalAppEnv: 'prod' or
'sandbox' (or alternatively return an object with separate prod and sandbox
arrays), then return the combined list so every returned key-manager includes
its originating environment for correct environment-aware validation.
…nt error handling
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/pages/application/partials/manage-keys.hbs (1)
829-833:⚠️ Potential issue | 🟡 MinorSame error container placement issue in Sandbox section.
Similar to the Production section, the error container at lines 830-831 is outside the
{{#if(eq devPortalAppEnv 'SANDBOX')}}closing tag, which may cause duplicate error containers.🔧 Move error container inside the devPortalAppEnv check
{{/in}} <!-- Error message container for key generation and token generation --> <div id="keyGenerationErrorContainer-SANDBOX" class="mt-2 text-danger" style="display: none; font-size: 0.75rem; word-wrap: break-word;"></div> + {{/if}} - {{/if}} {{/each}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/application/partials/manage-keys.hbs` around lines 829 - 833, The error container with id "keyGenerationErrorContainer-SANDBOX" is placed outside the {{`#if` (eq devPortalAppEnv 'SANDBOX')}} block causing potential duplicate containers; move this div so it sits inside the matching {{`#if` (eq devPortalAppEnv 'SANDBOX')}} ... {{/if}} block (mirroring how the Production section handles "keyGenerationErrorContainer-PRODUCTION") so the error container is only rendered for the SANDBOX environment and not duplicated.
🧹 Nitpick comments (1)
src/controllers/applicationsContentController.js (1)
176-182: Consider logging warnings when key manager fetches fail.When
prodResultorsandboxResultis rejected, the failure is silently swallowed. Adding a warning log would help with debugging and operational visibility.🔧 Proposed logging for failures
const [prodResult, sandboxResult] = await Promise.allSettled([ getAPIMKeyManagers(req, 'prod'), getAPIMKeyManagers(req, 'sandbox') ]); - const rawProdKeyManagers = prodResult.status === 'fulfilled' ? prodResult.value : []; - const rawSandboxKeyManagers = sandboxResult.status === 'fulfilled' ? sandboxResult.value : []; + const rawProdKeyManagers = prodResult.status === 'fulfilled' ? prodResult.value : (() => { + logger.warn('Failed to load production key managers', { error: prodResult.reason?.message }); + return []; + })(); + const rawSandboxKeyManagers = sandboxResult.status === 'fulfilled' ? sandboxResult.value : (() => { + logger.warn('Failed to load sandbox key managers', { error: sandboxResult.reason?.message }); + return []; + })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controllers/applicationsContentController.js` around lines 176 - 182, The prod/sandbox key manager fetch failures (prodResult/sandboxResult from Promise.allSettled calling getAPIMKeyManagers) are currently ignored; update the then-handling to log a warning when either result.status === 'rejected' using the module's logger (e.g., logger.warn or processLogger.warn), include which environment ('prod' or 'sandbox') and the rejection reason/error, and keep the existing fallback to [] so rawProdKeyManagers/rawSandboxKeyManagers remain safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/controllers/applicationsContentController.js`:
- Around line 800-809: The enrichKeyManager function currently fills resident KM
endpoints with hardcoded Choreo URLs when config.controlPlane.residentKeyManager
is missing; update enrichKeyManager (and the branch that checks
constants.KEY_MANAGERS.RESIDENT_KEY_MANAGER) to stop silently defaulting to
Choreo-specific URLs: either require the endpoints (throw or return an error if
residentKMConfig.tokenEndpoint/authorizeEndpoint/revokeEndpoint are not present)
or, if you prefer a fallback, keep the assignment but add a clear warning via
the logger (e.g., processLogger.warn) whenever any of the
tokenEndpoint/authorizeEndpoint/revokeEndpoint values are missing and the
fallback is used; leave the subsequent calls to mapGrants and mapDefaultValues
untouched.
In `@src/pages/application/partials/manage-keys.hbs`:
- Around line 260-264: The error container div with id
"keyGenerationErrorContainer-PRODUCTION" is currently rendered for every key
manager because it's placed outside the {{`#if` (eq devPortalAppEnv 'PROD')}}
block; move that <div> so it sits inside the {{`#if` (eq devPortalAppEnv
'PROD')}}...{{/if}} conditional (and still inside the {{`#each`
keyManagersMetadata}} / corresponding {{/in}} block) so the
keyGenerationErrorContainer-PRODUCTION is only emitted when devPortalAppEnv
equals 'PROD' and avoids duplicate containers per key manager.
---
Duplicate comments:
In `@src/pages/application/partials/manage-keys.hbs`:
- Around line 829-833: The error container with id
"keyGenerationErrorContainer-SANDBOX" is placed outside the {{`#if` (eq
devPortalAppEnv 'SANDBOX')}} block causing potential duplicate containers; move
this div so it sits inside the matching {{`#if` (eq devPortalAppEnv 'SANDBOX')}}
... {{/if}} block (mirroring how the Production section handles
"keyGenerationErrorContainer-PRODUCTION") so the error container is only
rendered for the SANDBOX environment and not duplicated.
---
Nitpick comments:
In `@src/controllers/applicationsContentController.js`:
- Around line 176-182: The prod/sandbox key manager fetch failures
(prodResult/sandboxResult from Promise.allSettled calling getAPIMKeyManagers)
are currently ignored; update the then-handling to log a warning when either
result.status === 'rejected' using the module's logger (e.g., logger.warn or
processLogger.warn), include which environment ('prod' or 'sandbox') and the
rejection reason/error, and keep the existing fallback to [] so
rawProdKeyManagers/rawSandboxKeyManagers remain safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7cec1f7-9652-46bf-873a-f2ac40131d42
📒 Files selected for processing (2)
src/controllers/applicationsContentController.jssrc/pages/application/partials/manage-keys.hbs
Comment on lines
+800
to
+809
| async function enrichKeyManager(keyManager) { | ||
| if (keyManager.name === constants.KEY_MANAGERS.RESIDENT_KEY_MANAGER) { | ||
| const residentKMConfig = config.controlPlane?.residentKeyManager || {}; | ||
| keyManager.tokenEndpoint = residentKMConfig.tokenEndpoint || 'https://sts.choreo.dev/oauth2/token'; | ||
| keyManager.authorizeEndpoint = residentKMConfig.authorizeEndpoint || 'https://sts.choreo.dev/oauth2/authorize'; | ||
| keyManager.revokeEndpoint = residentKMConfig.revokeEndpoint || 'https://sts.choreo.dev/oauth2/revoke'; | ||
| } | ||
| keyManager.availableGrantTypes = await mapGrants(keyManager.availableGrantTypes); | ||
| keyManager.applicationConfiguration = await mapDefaultValues(keyManager.applicationConfiguration); | ||
| } |
There was a problem hiding this comment.
Hardcoded Choreo-specific URLs may break non-Choreo deployments.
The fallback URLs (https://sts.choreo.dev/oauth2/...) are Choreo-specific. If config.controlPlane.residentKeyManager is not configured in a non-Choreo environment, users will see incorrect endpoint URLs.
Consider either:
- Making these URLs required configuration (no fallback)
- Logging a warning when using fallback URLs
🔧 Suggested improvement
async function enrichKeyManager(keyManager) {
if (keyManager.name === constants.KEY_MANAGERS.RESIDENT_KEY_MANAGER) {
const residentKMConfig = config.controlPlane?.residentKeyManager || {};
- keyManager.tokenEndpoint = residentKMConfig.tokenEndpoint || 'https://sts.choreo.dev/oauth2/token';
- keyManager.authorizeEndpoint = residentKMConfig.authorizeEndpoint || 'https://sts.choreo.dev/oauth2/authorize';
- keyManager.revokeEndpoint = residentKMConfig.revokeEndpoint || 'https://sts.choreo.dev/oauth2/revoke';
+ if (!residentKMConfig.tokenEndpoint) {
+ logger.warn('Resident Key Manager token endpoint not configured, using Choreo default');
+ }
+ keyManager.tokenEndpoint = residentKMConfig.tokenEndpoint || 'https://sts.choreo.dev/oauth2/token';
+ keyManager.authorizeEndpoint = residentKMConfig.authorizeEndpoint || 'https://sts.choreo.dev/oauth2/authorize';
+ keyManager.revokeEndpoint = residentKMConfig.revokeEndpoint || 'https://sts.choreo.dev/oauth2/revoke';
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/controllers/applicationsContentController.js` around lines 800 - 809, The
enrichKeyManager function currently fills resident KM endpoints with hardcoded
Choreo URLs when config.controlPlane.residentKeyManager is missing; update
enrichKeyManager (and the branch that checks
constants.KEY_MANAGERS.RESIDENT_KEY_MANAGER) to stop silently defaulting to
Choreo-specific URLs: either require the endpoints (throw or return an error if
residentKMConfig.tokenEndpoint/authorizeEndpoint/revokeEndpoint are not present)
or, if you prefer a fallback, keep the assignment but add a clear warning via
the logger (e.g., processLogger.warn) whenever any of the
tokenEndpoint/authorizeEndpoint/revokeEndpoint values are missing and the
fallback is used; leave the subsequent calls to mapGrants and mapDefaultValues
untouched.
Comment on lines
260
to
264
| <!-- Error message container for key generation and token generation --> | ||
| <div id="keyGenerationErrorContainer-PRODUCTION" class="mt-2 text-danger" | ||
| style="display: none; font-size: 0.75rem; word-wrap: break-word;"></div> | ||
| {{/if}} | ||
| {{/each}} |
There was a problem hiding this comment.
Verify error container placement relative to new conditionals.
The error container at lines 261-262 is now inside the {{/in}} block but outside the {{#if (eq devPortalAppEnv 'PROD')}} closing tag at line 263. This means the error container will render for every key manager in keyManagersMetadata regardless of environment, which may cause duplicate error containers.
🔧 Move error container inside the devPortalAppEnv check
{{/in}}
<!-- Error message container for key generation and token generation -->
<div id="keyGenerationErrorContainer-PRODUCTION" class="mt-2 text-danger"
style="display: none; font-size: 0.75rem; word-wrap: break-word;"></div>
+ {{/if}}
- {{/if}}
{{/each}}The error container should be moved inside the {{#if (eq devPortalAppEnv 'PROD')}} block to avoid duplicate rendering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/application/partials/manage-keys.hbs` around lines 260 - 264, The
error container div with id "keyGenerationErrorContainer-PRODUCTION" is
currently rendered for every key manager because it's placed outside the {{`#if`
(eq devPortalAppEnv 'PROD')}} block; move that <div> so it sits inside the {{`#if`
(eq devPortalAppEnv 'PROD')}}...{{/if}} conditional (and still inside the
{{`#each` keyManagersMetadata}} / corresponding {{/in}} block) so the
keyGenerationErrorContainer-PRODUCTION is only emitted when devPortalAppEnv
equals 'PROD' and avoids duplicate containers per key manager.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
$subject
https://github.com/wso2-enterprise/apim-saas/issues/2304
For new organisations, at API Invocation, token validation fails when the Production Default STS KM was used to generate the SANDBOX key.
Therefore, we will need to support generating the sandbox key using the sandbox KM defined.
Summary by CodeRabbit
New Features
Configuration