Update application REST APIs and fix multiple bugs#2234
Open
Piumal1999 wants to merge 8 commits into
Open
Conversation
…n handling and scope management
…ganization isolation checks
Piumal1999
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughAPI Route RestructuringThe Developer Portal REST API endpoints for applications have been standardized to use organization-scoped routing via the
Authentication and Authorization EnhancementsMultiple auth flow and authorization behaviors were updated to ensure correct organization scoping and more consistent request handling:
Backend Implementation Updates
Frontend and Template Updates
Configuration and Documentation Updates
WalkthroughThe pull request transforms the Developer Portal API to enforce explicit organization scoping, requiring all application and key-management operations to include an Suggested Reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
portals/developer-portal/src/controllers/authController.js (2)
224-230: ⚡ Quick winMisleading
awaiton callback-based API.
req.session.save(callback)uses callback style and does not return a Promise. Theawaitresolves immediately toundefinedrather than waiting for the save to complete. The code works correctly because all logic is inside the callback, but theawaitis misleading and could confuse future maintainers.Suggested fix
req.session.returnTo = req.originalUrl; req.session.silentAuthRedirected = true; - await req.session.save((err) => { + req.session.save((err) => { if (err) { logger.error('Session save failed during silent SSO', { error: err.message }); return next(); } passport.authenticate('oauth2', { prompt: 'none' })(req, res, next); });🤖 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/developer-portal/src/controllers/authController.js` around lines 224 - 230, The await keyword on the callback-based req.session.save() call is misleading because this API does not return a Promise, so await resolves immediately to undefined rather than waiting for the callback to complete. Remove the await keyword from the req.session.save invocation in the silent SSO authentication handler, as the callback pattern already properly handles the asynchronous flow and all logic is correctly contained within the callback.
163-170: 💤 Low valueRedundant header assignment.
Cache-Controlis already set at line 151 before entering the conditional branches. This duplicate set on line 167 is unnecessary.Suggested fix
req.session.destroy((destroyErr) => { if (destroyErr) { logger.error('Session destroy failed on local-auth logout', { error: destroyErr.message }); } - res.set('Cache-Control', 'no-store'); res.redirect(req.originalUrl.replace('/logout', '/login')); });🤖 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/developer-portal/src/controllers/authController.js` around lines 163 - 170, The Cache-Control header is being set twice in the logout flow: once at line 151 and again inside the req.session.destroy callback at line 167. Remove the duplicate res.set('Cache-Control', 'no-store'); statement from inside the session destroy callback since the header is already configured earlier in the control flow before entering the conditional branches.
🤖 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/developer-portal/src/controllers/devportalController.js`:
- Line 165: Fix the typo in the delete success response message in
devportalController.js where "Resouce" is misspelled and should be "Resource".
Update the response text in the res.status(200).send() call to correctly spell
"Resource Deleted Successfully" instead of "Resouce Deleted Successfully". This
typo appears in multiple locations (at lines 165 and 175), so make sure to
correct all occurrences throughout the file.
- Line 75: The request parameters being logged directly in error messages (such
as orgId in req.params.orgId) need to be normalized to remove CR/LF characters
before being attached to log metadata. Extract and normalize the route
parameters (req.params.orgId and any other request-path values) by removing
carriage returns and line feeds early in the request handler, then use these
sanitized values in all subsequent logger.error calls that reference them. This
normalization should be applied consistently across the error logging statements
at lines 75, 90, 109, and 178 to ensure logs remain safe and reliable for
downstream processing.
In `@portals/developer-portal/src/middlewares/authMiddleware.js`:
- Around line 96-131: The checkOrgIsolation function currently treats all errors
from orgDao.get() as internal server errors, but the upstream contract indicates
that a missing organization throws Sequelize.EmptyResultError rather than
returning a falsy value, making the !orgDetails check unreachable. Modify the
catch block that handles the orgDao.get() call to distinguish between
Sequelize.EmptyResultError (which should return a 404 status) and other errors
(which should return 500 status). This will ensure that missing organizations
are properly reported as 404 Not Found instead of 500 Internal Server Error, and
you can remove the now-reachable !orgDetails check.
---
Nitpick comments:
In `@portals/developer-portal/src/controllers/authController.js`:
- Around line 224-230: The await keyword on the callback-based
req.session.save() call is misleading because this API does not return a
Promise, so await resolves immediately to undefined rather than waiting for the
callback to complete. Remove the await keyword from the req.session.save
invocation in the silent SSO authentication handler, as the callback pattern
already properly handles the asynchronous flow and all logic is correctly
contained within the callback.
- Around line 163-170: The Cache-Control header is being set twice in the logout
flow: once at line 151 and again inside the req.session.destroy callback at line
167. Remove the duplicate res.set('Cache-Control', 'no-store'); statement from
inside the session destroy callback since the header is already configured
earlier in the control flow before entering the conditional branches.
🪄 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: d72555f5-946a-4ca6-b073-a5c622fb9b1b
📒 Files selected for processing (15)
docs/rest-apis/devportal/README.mddocs/rest-apis/devportal/application-keys.mddocs/rest-apis/devportal/applications.mdportals/developer-portal/configs/config.yaml.exampleportals/developer-portal/docs/README.mdportals/developer-portal/docs/administer/api-token-curl.mdportals/developer-portal/docs/administer/asgardeo-setup.mdportals/developer-portal/docs/devportal-openapi-spec-v1.yamlportals/developer-portal/src/controllers/authController.jsportals/developer-portal/src/controllers/devportalController.jsportals/developer-portal/src/middlewares/authMiddleware.jsportals/developer-portal/src/middlewares/ensureAuthenticated.jsportals/developer-portal/src/middlewares/passportConfig.jsportals/developer-portal/src/routes/api/handlers/applicationsHandler.jsportals/developer-portal/src/utils/tokenUtil.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/developer-portal/src/services/apiFlowService.js`:
- Around line 186-192: Move the orgDao.get(orgID) call and
sequelize.transaction() creation from before the try block into inside the try
block so that failures in these operations are caught by the existing catch
handler. Additionally, update the catch block to only perform transaction
rollback when the transaction variable t was successfully created, by checking
if t exists before calling rollback.
🪄 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: 4aa8be90-e794-4a29-ab76-06fe626b0f8f
📒 Files selected for processing (13)
portals/developer-portal/src/controllers/applicationsContentController.jsportals/developer-portal/src/controllers/authController.jsportals/developer-portal/src/controllers/devportalController.jsportals/developer-portal/src/defaultContent/layout/main.hbsportals/developer-portal/src/middlewares/authMiddleware.jsportals/developer-portal/src/middlewares/ensureAuthenticated.jsportals/developer-portal/src/scripts/add-application-form.jsportals/developer-portal/src/scripts/common.jsportals/developer-portal/src/scripts/edit-application-form.jsportals/developer-portal/src/scripts/oauth2-key-generation.jsportals/developer-portal/src/scripts/warning.jsportals/developer-portal/src/services/apiFlowService.jsportals/developer-portal/src/services/apiMetadataService.js
💤 Files with no reviewable changes (1)
- portals/developer-portal/src/services/apiMetadataService.js
🚧 Files skipped from review as they are similar to previous changes (4)
- portals/developer-portal/src/middlewares/authMiddleware.js
- portals/developer-portal/src/controllers/devportalController.js
- portals/developer-portal/src/middlewares/ensureAuthenticated.js
- portals/developer-portal/src/controllers/authController.js
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.
Purpose
Correcting the application related rest APIs
Updating docs
Fixing multiple authentication related bugs
Approach
o/{orgId}prefix similar to other APIs