Fix multiple UI UX issues - #3062
Conversation
📝 WalkthroughWalkthroughThe API Portal removes static service API-key authentication, adds role-mode authorization integration tests, improves page and Markdown error status handling, supports unlabeled views, updates immutable resource handles, and changes Cypress setup to session-based authentication. ChangesAPI Portal authentication and integration coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
portals/api-portal/src/middlewares/authMiddleware.js (2)
400-412: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the stale "org resolved from header" comment.
The comment on line 400 says the mTLS branch resolves the org "from the
organizationrequest header." The function it calls,resolvePortalOrg(documented at lines 246-253), does the opposite: the header no longer selects the organization, it is only checked for a mismatch. Update the line 400 comment to match the documented behavior ofresolvePortalOrg, or a future reader will misunderstand how org selection works for mTLS credentials.📝 Proposed fix
- // 4. mTLS — org resolved from the `organization` request header + // 4. mTLS — org resolved from the portal's configured organization; + // the `organization` header, if present, is only checked for a mismatch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/api-portal/src/middlewares/authMiddleware.js` around lines 400 - 412, Update the comment immediately above the mTLS branch in the authentication middleware to state that resolvePortalOrg validates the organization header for mismatches rather than selecting the organization from it; leave the resolvePortalOrg call and authentication behavior unchanged.
465-479: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCorrect the
apiKeyAuthdocstring — it inverts the actual acceptance rule.The docstring says this handler "Accepts any preauthorized non-OAuth mode (mTLS, role-mode session)." That does not match
req.auth?.preauthorized:
- Role-mode sessions set
preauthorized: !isRoleMode(), which isfalsein role mode (line 347) — they are rejected, not accepted, contradicting "role-mode session" in the doc.- Scope-mode IDP sessions have
mode: 'oauth2'andpreauthorized: true— they are accepted, contradicting "non-OAuth mode" in the doc.This handler is currently unreachable (no operation declares
apiKeyAuth), so there is no live exposure. Still, fix the wording now so a future engineer who addssecurity: [apiKeyAuth]to an operation does not rely on an incorrect mental model of which sessions pass.📝 Proposed fix
- * Accepts any preauthorized non-OAuth mode (mTLS, role-mode session). + * Accepts any mode with req.auth.preauthorized === true — currently mTLS, + * and scope-mode OAuth2 sessions (preauthorized is false for role-mode + * sessions, platform-jwt sessions, and bearer tokens).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/api-portal/src/middlewares/authMiddleware.js` around lines 465 - 479, Correct the apiKeyAuth docstring to state that the handler accepts requests only when req.auth?.preauthorized is true, including scope-mode OAuth2 sessions, and does not accept role-mode sessions when they set it false. Remove the inaccurate “non-OAuth” and “role-mode session” wording while preserving the surrounding credential-store and scope-check guidance.
🧹 Nitpick comments (1)
portals/api-portal/it/rest-api/views-and-labels/views.spec.js (1)
63-67: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify persistence for an explicit empty label array.
The test checks only
201forlabels: []. It does not prove that the stored view has an empty label set. Fetch/views/${id}and assertfetched.body.labelsis[], as in the omitted-label test.Proposed test assertion
expect(res.status).toBe(201); + const fetched = await client.as('admin').get(`/views/${id}`); + expect(fetched.status).toBe(200); + expect(fetched.body.labels).toEqual([]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/api-portal/it/rest-api/views-and-labels/views.spec.js` around lines 63 - 67, Update the creates-a-view-with-an-empty-label-array test around uniqueHandle and the POST request to fetch /views/${id} after confirming creation, then assert the fetched response’s body.labels is an empty array, matching the omitted-label persistence test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@portals/api-portal/src/controllers/apiContentController.js`:
- Around line 1139-1143: Update sendMarkdownError to set the response
Content-Type to the caller’s success media type before sending either the 404 or
500 markdown body, passing that media type into the helper if necessary and
updating its callers accordingly.
In `@portals/api-portal/src/controllers/customContentController.js`:
- Line 105: Update the locally generated missing-page errors in the controller
to include a 404 status before they are passed to next(error), ensuring
pageErrorStatus(error) maps both missing-file branches to HTTP 404 instead of
500.
In `@portals/api-portal/src/pages/application/partials/manage-keys-km-card.hbs`:
- Around line 65-74: Update the removal messaging in the manage-keys footer and
the related confirmation flows in confirmAndRemoveKeys and
oauth2-key-generation.js to accurately describe the DELETE route: it removes
only the portal’s local OAuth client_id mapping, while external OAuth clients
and already-issued tokens remain active. Remove wording that claims credentials
are deleted or tokens are revoked, and synchronize any related documentation
with this behavior.
In `@portals/api-portal/src/styles/settings-layout.css`:
- Around line 612-615: Update the .cfg-form-input[readonly]:focus rule to retain
a clearly visible keyboard-focus indicator instead of removing all focus
styling. Add or preserve a distinct outline, border, or shadow that contrasts
with the base read-only input appearance.
---
Outside diff comments:
In `@portals/api-portal/src/middlewares/authMiddleware.js`:
- Around line 400-412: Update the comment immediately above the mTLS branch in
the authentication middleware to state that resolvePortalOrg validates the
organization header for mismatches rather than selecting the organization from
it; leave the resolvePortalOrg call and authentication behavior unchanged.
- Around line 465-479: Correct the apiKeyAuth docstring to state that the
handler accepts requests only when req.auth?.preauthorized is true, including
scope-mode OAuth2 sessions, and does not accept role-mode sessions when they set
it false. Remove the inaccurate “non-OAuth” and “role-mode session” wording
while preserving the surrounding credential-store and scope-check guidance.
---
Nitpick comments:
In `@portals/api-portal/it/rest-api/views-and-labels/views.spec.js`:
- Around line 63-67: Update the creates-a-view-with-an-empty-label-array test
around uniqueHandle and the POST request to fetch /views/${id} after confirming
creation, then assert the fetched response’s body.labels is an empty array,
matching the omitted-label persistence test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c30c0454-113e-4dee-90a7-548a1c8afd22
⛔ Files ignored due to path filters (2)
portals/api-portal/src/defaultContent/images/api-portal-logo-white.pngis excluded by!**/*.pngportals/api-portal/src/defaultContent/images/api-portal-logo-white.svgis excluded by!**/*.svg
📒 Files selected for processing (54)
portals/api-portal/configs/config-template.tomlportals/api-portal/docs/api-portal-openapi-spec-v0.9.yamlportals/api-portal/docs/consume-an-api/consume-with-oauth2.mdportals/api-portal/it/README.mdportals/api-portal/it/configs/portal-roles-role-mode-it.yamlportals/api-portal/it/docker-compose.test.postgres.yamlportals/api-portal/it/docker-compose.test.yamlportals/api-portal/it/rest-api/ai-discovery/apis-md.spec.jsportals/api-portal/it/rest-api/auth/role-mode-authorization.spec.jsportals/api-portal/it/rest-api/organizations/single-org-isolation.spec.jsportals/api-portal/it/rest-api/support/fixtures.jsportals/api-portal/it/rest-api/views-and-labels/views.spec.jsportals/api-portal/it/test-config.tomlportals/api-portal/it/ui/cypress/e2e/001-basic/001-portal-access.cy.jsportals/api-portal/it/ui/cypress/e2e/002-apis/001-api-listing.cy.jsportals/api-portal/it/ui/cypress/e2e/002-apis/002-rest-api-details.cy.jsportals/api-portal/it/ui/cypress/e2e/003-mcp-servers/001-mcp-listing.cy.jsportals/api-portal/it/ui/cypress/e2e/applications/application-flows.cy.jsportals/api-portal/it/ui/cypress/e2e/settings/001-views-labels.cy.jsportals/api-portal/it/ui/cypress/e2e/settings/002-key-managers.cy.jsportals/api-portal/it/ui/cypress/support/commands/portal.jsportals/api-portal/it/ui/cypress/support/commands/seed.jsportals/api-portal/src/config/configDefaults.jsportals/api-portal/src/controllers/apiContentController.jsportals/api-portal/src/controllers/apiKeysOverviewController.jsportals/api-portal/src/controllers/apiWorkflowsController.jsportals/api-portal/src/controllers/applicationsContentController.jsportals/api-portal/src/controllers/customContentController.jsportals/api-portal/src/controllers/orgContentController.jsportals/api-portal/src/controllers/subscriptionsContentController.jsportals/api-portal/src/dao/organizationDao.jsportals/api-portal/src/defaultContent/pages/apis/partials/apis-md.hbsportals/api-portal/src/defaultContent/pages/docs/page.hbsportals/api-portal/src/defaultContent/pages/mcp-landing/page.hbsportals/api-portal/src/defaultContent/pages/mcps/partials/mcps-md.hbsportals/api-portal/src/middlewares/authMiddleware.jsportals/api-portal/src/middlewares/csrfProtection.jsportals/api-portal/src/middlewares/ensureAuthenticated.jsportals/api-portal/src/middlewares/registerPartials.jsportals/api-portal/src/pages/application/partials/manage-keys-km-card.hbsportals/api-portal/src/pages/login-page/layout.hbsportals/api-portal/src/pages/settings/partials/cfg-apis-panel.hbsportals/api-portal/src/pages/settings/partials/cfg-labels-panel.hbsportals/api-portal/src/pages/settings/partials/cfg-modals.hbsportals/api-portal/src/pages/settings/partials/cfg-webhook-form.hbsportals/api-portal/src/pages/settings/partials/cfg-webhooks-panel.hbsportals/api-portal/src/scripts/oauth2-key-generation.jsportals/api-portal/src/scripts/settings-apis.jsportals/api-portal/src/scripts/settings-labels.jsportals/api-portal/src/scripts/settings-organization.jsportals/api-portal/src/scripts/settings-webhooks.jsportals/api-portal/src/services/apiMetadataService.jsportals/api-portal/src/styles/settings-layout.cssportals/api-portal/src/utils/util.js
💤 Files with no reviewable changes (3)
- portals/api-portal/src/config/configDefaults.js
- portals/api-portal/configs/config-template.toml
- portals/api-portal/src/middlewares/ensureAuthenticated.js
| function sendMarkdownError(res, error, failureMessage) { | ||
| if (util.pageErrorStatus(error) === 404) { | ||
| return res.status(404).send('# Not Found\n\nThe requested resource does not exist.'); | ||
| } | ||
| return res.status(500).send(`# Error\n\n${failureMessage}`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 3 \
'function sendMarkdownError|sendMarkdownError\(|Content-Type.*text/(markdown|plain)' \
portals/api-portalRepository: wso2/api-platform
Length of output: 10952
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Relevant helper and callers:\n'
sed -n '1128,1148p;1250,1272p;1332,1348p;1356,1372p;1404,1427p;1448,1468p;1560,1580p' portals/api-portal/src/controllers/apiContentController.js
printf '\nResponse ContentType headers around sendMarkdownError call sites:\n'
python3 - <<'PY'
from pathlib import Path
p = Path('portals/api-portal/src/controllers/apiContentController.js')
lines = p.read_text().splitlines()
for name_start, name_end in [
('sendMarkdownError', 1148),
('apiContentDetailMarkdown', 1272),
('portalIndexMarkdown', 1348),
('portalIndexMarkdownPreview', 1372),
('apisMarkdown', 1428),
('mcpsMarkdown', 1469),
('documentMarkdown', 1580),
]:
print(f'-- {name_start} --')
for i in range(name_start, name_end + 1):
if 'setHeader' in lines[i-1] or 'sendMarkdownError' in lines[i-1] or 'Content-Type' in lines[i-1]:
print(f'{i}: {lines[i-1]}')
PY
printf '\nCheck if a Content-Type has already been sent before sendMarkdownError in each block by simulating Express response setHeader semantics across line range:\n'
python3 - <<'PY'
from pathlib import Path
p = Path('portals/api-portal/src/controllers/apiContentController.js')
lines = p.read_text().splitlines()
callers = [
('apiContentDetailMarkdown', 1250, 1270),
('portalIndexMarkdown', 1334, 1346),
('portalIndexMarkdownPreview', 1358, 1370),
('apisMarkdown', 1410, 1426),
('mcpsMarkdown', 1450, 1466),
('documentMarkdown', 1564, 1578),
]
for name, start, end in callers:
header_set_before_error = any(i < end and 'Content-Type' in (lines[i-1] if i <= len(lines) else '') for i in range(start, end + 1))
calls = [i for i in range(start, end + 1) if 'sendMarkdownError' in lines[i-1]]
print(f'{name}: header_set_before_error={header_set_before_error}, calls={calls}')
PYRepository: wso2/api-platform
Length of output: 6311
🌐 Web query:
Express.js response.send string default Content-Type text/html
💡 Result:
In Express.js, when you pass a string to the res.send method, the framework automatically sets the Content-Type response header to text/html [1][2][3]. Furthermore, Express also appends a charset parameter of utf-8 to this header, resulting in a final Content-Type value of text/html; charset=utf-8 [4]. This behavior is designed to ensure proper character encoding handling, as Express defaults to UTF-8 for string responses [5]. If you wish to override this default, you can explicitly set the Content-Type header using methods like res.set or res.type before calling res.send [1][2]. For example: res.set('Content-Type', 'text/plain'); res.send('Your string here'); If you need to send a response without Express automatically setting or modifying the Content-Type or charset, you can bypass the convenience logic of res.send by using the lower-level res.write and res.end methods instead [6].
Citations:
- 1: https://expressjs.com/en/4x/api/response/
- 2: https://expressjs.com/en/5x/api/response/
- 3: https://expressjs.com/en/3x/api/response/
- 4: https://masteringjs.io/tutorials/express/res
- 5: How do I use res.send without setting charset? expressjs/express#2238
- 6: How to use res.send([buffer]) without a content-type? expressjs/express#6224
Set the response media type in sendMarkdownError.
sendMarkdownError returns markdown/plain bodies through res.send, and Express defaults string bodies to text/html; charset=utf-8. Use the caller’s success media type in error responses, either by passing the media type into this helper or by setting the header before each call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portals/api-portal/src/controllers/apiContentController.js` around lines 1139
- 1143, Update sendMarkdownError to set the response Content-Type to the
caller’s success media type before sending either the 404 or 500 markdown body,
passing that media type into the helper if necessary and updating its callers
accordingly.
| filePath: req.params.filePath, | ||
| }); | ||
| error.status = 500; | ||
| error.status = pageErrorStatus(error); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Set 404 status on locally generated missing-page errors.
At Lines 79 and 81, the controller throws plain Error objects when the page file is missing. pageErrorStatus(error) only preserves error.statusCode or error.status, so these requests still return HTTP 500 instead of HTTP 404.
Set status: 404 on both errors before calling next(error).
Proposed fix
- throw new Error(`Manage keys page not found. This route should be handled by the application controller.`);
+ throw Object.assign(new Error(`Manage keys page not found. This route should be handled by the application controller.`), { status: 404 });
...
- throw new Error(`Content page not found at ${resolvedPagePath}`);
+ throw Object.assign(new Error(`Content page not found at ${resolvedPagePath}`), { status: 404 });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portals/api-portal/src/controllers/customContentController.js` at line 105,
Update the locally generated missing-page errors in the controller to include a
404 status before they are passed to next(error), ensuring
pageErrorStatus(error) maps both missing-file branches to HTTP 404 instead of
500.
| {{!-- Remove-keys footer — part of Credentials tab --}} | ||
| <div class="mk-km-footer" id="keyActionsContainer-{{keyType}}"> | ||
| <span class="mk-secret-note"> | ||
| <i class="bi bi-info-circle"></i> | ||
| Revoking removes all credentials. Tokens already issued remain valid until they expire. | ||
| Removing deletes all credentials. Tokens already issued remain valid until they expire. | ||
| </span> | ||
| <div class="mk-footer-actions"> | ||
| <button class="mk-btn-danger" | ||
| onclick="confirmAndRevokeKeys('{{appId}}', '{{keys.keyMappingId}}', '{{keyType}}')"> | ||
| <i class="bi bi-trash3"></i> Revoke keys | ||
| onclick="confirmAndRemoveKeys('{{appId}}', '{{keys.keyMappingId}}', '{{keyType}}')"> | ||
| <i class="bi bi-trash3"></i> Remove keys |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 12 \
'oauth-keys|removeApplicationKeys|Remove Application Keys|remove this client ID' .Repository: wso2/api-platform
Length of output: 31522
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Locate and inspect the relevant implementation and route handling without running repository code.
printf 'Files:\n'
git ls-files | rg 'appDao|oauthKey|oauth-key|application-keys|apiPortalController|consume-with-oauth2' || true
printf '\nRelevant implementations:\n'
for f in $(git ls-files | rg 'appDao|oauthKey|oauth-key|application-keys|apiPortalController' | head -20); do
echo '--- $f ---'
wc -l "$f"
rg -n -C 8 'deleteOAuth|revokeOAuth|removeOAuth|OAuth|oauth-keys|keyMappingId|client_id|local|external|token|credentials' "$f" || true
done
printf '\nRoute/server registration references:\n'
rg -n -C 5 'oauth-keys|/applications/.*/oauth-keys|revokeOAuthKeys|appDao|removeApplicationKeys' .Repository: wso2/api-platform
Length of output: 50375
Align the removal message with the route contract.
DELETE /applications/{applicationId}/oauth-keys/{keyMappingId} removes the portal’s local OAuth client_id mapping and returns Application key mapping removed successfully; it does not delete the external OAuth client or revoke already-issued tokens. Update portals/api-portal/src/pages/application/partials/manage-keys-km-card.hbs:69, portals/api-portal/src/scripts/warning.js:108, portals/api-portal/src/scripts/oauth2-key-generation.js:66, and related docs so the UI does not say credentials are revoked or deleted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portals/api-portal/src/pages/application/partials/manage-keys-km-card.hbs`
around lines 65 - 74, Update the removal messaging in the manage-keys footer and
the related confirmation flows in confirmAndRemoveKeys and
oauth2-key-generation.js to accurately describe the DELETE route: it removes
only the portal’s local OAuth client_id mapping, while external OAuth clients
and already-issued tokens remain active. Remove wording that claims credentials
are deleted or tokens are revoked, and synchronize any related documentation
with this behavior.
| .cfg-form-input[readonly]:focus { | ||
| border-color: var(--border-strong); | ||
| box-shadow: none; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep a visible focus indicator for read-only inputs.
Read-only inputs remain keyboard-focusable. Lines 612-615 remove the border and shadow supplied by the normal focus rule, while the base input style also removes the browser outline. A keyboard user can tab to a handle without a visible focus state. Preserve a focus outline or add a distinct focus style.
Proposed focus style
.cfg-form-input[readonly]:focus {
- border-color: var(--border-strong);
- box-shadow: none;
+ border-color: var(--primary);
+ box-shadow: 0 0 0 3px color-mix(in srgb, var(--primary) 12%, transparent);
}📝 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.
| .cfg-form-input[readonly]:focus { | |
| border-color: var(--border-strong); | |
| box-shadow: none; | |
| } | |
| .cfg-form-input[readonly]:focus { | |
| border-color: var(--primary); | |
| box-shadow: 0 0 0 3px color-mix(in srgb, var(--primary) 12%, transparent); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portals/api-portal/src/styles/settings-layout.css` around lines 612 - 615,
Update the .cfg-form-input[readonly]:focus rule to retain a clearly visible
keyboard-focus indicator instead of removing all focus styling. Add or preserve
a distinct outline, border, or shadow that contrasts with the base read-only
input appearance.
1 participant
Purpose
Goals
Approach
User stories
Documentation
Automation tests
Security checks
Samples
Related PRs
Test environment