fix-publisher routes are not blocked for read only user #1212
Conversation
GavinHemsada
requested review from
Krishanx92,
lasanthaS,
piyumaldk and
tharikaGitHub
as code owners
|
|
|
Hi @GavinHemsada, I tried this fix locally. Noticed the following. When we log in as the admin or a privileged user than a read only user, I get the following screen when clicking on the scope tile. Shall we check the behaviour in other places too and fix it? 
Accessing 
|
|
Hi @tharikaGitHub, i create new readonly user using https://localhost:9443/carbon and then try to create a REST(https://localhost:9443/publisher/apis/create/rest). It looks like this image below (I used the default one; I did undo all changes). issue: "Some paths, such as {basePath}/publisher/apis/create/rest, are still accessible to read-only users, even though create/edit operations are not allowed for the user." |
Hi @GavinHemsada, Yes that is the current behaviour. With your fix the readonly user will not be able to access this page. So that will not be an issue. |
|
Hi @tharikaGitHub, I only made AuthManager changes. |
| } else { | ||
| return true; | ||
| } | ||
| return !user.scopes.includes('apim:api_publish'); |
There was a problem hiding this comment.
@GavinHemsada the user object here return !user.scopes.includes('apim:api_publish'); does not seem to have been initialised.
WalkthroughAdded an early denial in AuthManager.isRestricted that returns restricted when a retrieved user is read-only; documentation for isRestricted was expanded to describe parameters and return value. No signature changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as UI/Controller
participant AuthMgr as AuthManager.isRestricted
participant UserSvc as User Token/Scopes
Caller->>AuthMgr: isRestricted(scopesAllowedToEdit, api)
AuthMgr->>UserSvc: retrieve current user & check isReadOnlyUser()
alt user exists AND isReadOnlyUser
AuthMgr-->>Caller: deny (restricted) %% early deny added
else
AuthMgr->>AuthMgr: continue existing scope/lifecycle checks
AuthMgr-->>Caller: allow/deny (existing logic)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
|
Hi @tharikaGitHub I fixed it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| if (Object.keys(api).length === 0 && api.constructor === Object) { | ||
| // Only allow users with create permission on create pages | ||
| return !user.scopes.includes('apim:api_create'); | ||
| } |
There was a problem hiding this comment.
Guard against api being null before calling Object.keys.
Several callers pass null while the API payload is still loading (e.g., initial React renders). With the new logic, Object.keys(api) will throw TypeError: Cannot convert undefined or null to object, breaking those routes before the user even hits the create-page path. Please short-circuit when api is falsy before touching Object.keys.
Apply this diff to avoid the crash:
- if (Object.keys(api).length === 0 && api.constructor === Object) {
- // Only allow users with create permission on create pages
- return !user.scopes.includes('apim:api_create');
- }
+ if (
+ !api
+ || (api.constructor === Object && Object.keys(api).length === 0)
+ ) {
+ // Only allow users with create permission on create pages
+ return !user.scopes.includes('apim:api_create');
+ }📝 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.
| if (Object.keys(api).length === 0 && api.constructor === Object) { | |
| // Only allow users with create permission on create pages | |
| return !user.scopes.includes('apim:api_create'); | |
| } | |
| if ( | |
| !api | |
| || (api.constructor === Object && Object.keys(api).length === 0) | |
| ) { | |
| // Only allow users with create permission on create pages | |
| return !user.scopes.includes('apim:api_create'); | |
| } |
🤖 Prompt for AI Agents
In portals/publisher/src/main/webapp/source/src/app/data/AuthManager.js around
lines 235 to 238, guard against api being null/undefined before calling
Object.keys by short-circuiting when api is falsy; change the condition to first
check if (!api) return <appropriate value> or combine as if (!api ||
(Object.keys(api).length === 0 && api.constructor === Object)) { ... } so you
never call Object.keys on null/undefined and preserve the existing behavior of
only allowing users with 'apim:api_create' on create pages.
Hi @GavinHemsada, thank you for addressing the previous comment. I reviewed the latest changes. Seems with this change, publishers are unable to delete API Products created by them. Shall we check and fix it? Shall we also guard against the user object being null? |
|
Hi @tharikaGitHub, I fixed it. |
portals/publisher/src/main/webapp/source/src/app/data/AuthManager.js
Outdated
Show resolved
Hide resolved
|
Hi @tharikaGitHub, I've done it. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
portals/publisher/src/main/webapp/source/src/app/data/AuthManager.js (1)
237-240: Guard againstapibeingnullorundefinedbefore callingObject.keys.This critical issue was flagged in previous reviews and remains unresolved. When
apiis explicitly passed asnull(e.g., during initial React renders before the API payload loads),Object.keys(api)throwsTypeError: Cannot convert undefined or null to object, breaking routes before they can render.Apply this diff to guard against null/undefined:
- if (Object.keys(api).length === 0 && api.constructor === Object) { + if ( + !api + || (api.constructor === Object && Object.keys(api).length === 0) + ) { // Only allow users with create permission on create pages return !user.scopes.includes('apim:api_create'); }This ensures the null/undefined check happens before
Object.keysis called, preventing runtime errors while preserving the existing create-page behavior.
🧹 Nitpick comments (1)
portals/publisher/src/main/webapp/source/src/app/data/AuthManager.js (1)
242-245: Add a null guard forapi.lifeCycleStatusaccess.While accessing
api.lifeCycleStatuson a null object won't throw an error (it evaluates toundefinedand the condition becomes false), relying on this behavior is fragile. Once lines 237–240 are fixed to properly handle nullapi, consider adding an explicit null check here as well for clarity and defensive coding.Apply this diff for defensive null checking:
+ // Guard against null api + if (!api) { + return true; + } + // Allow creator access based on lifecycle status if (api.lifeCycleStatus === 'CREATED' || api.lifeCycleStatus === 'PROTOTYPED') { return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
portals/publisher/src/main/webapp/source/src/app/data/AuthManager.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
portals/publisher/src/main/webapp/source/src/app/data/AuthManager.js (4)
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/NewOverview/CustomizedStepper.jsx (1)
api(153-153)portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/TryOut/TryOutConsole.jsx (1)
api(147-147)portals/publisher/src/main/webapp/source/src/app/components/Apis/Listing/components/ImageGenerator/APICards/APIThumbPlain.jsx (1)
api(164-166)portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/components/leftMenu/DevelopSectionMenu.jsx (1)
user(122-122)
🔇 Additional comments (3)
portals/publisher/src/main/webapp/source/src/app/data/AuthManager.js (3)
211-216: LGTM! Addresses the PR objective.The early check for missing or read-only users directly addresses the reported issue where read-only users could access publisher routes. This prevents downstream access checks from running for unauthorized users.
218-220: Verify that bypassingscopesAllowedToEditfor API PRODUCT is intentional.For
apiType === 'APIPRODUCT', the function returns early based solely on theapim:api_publishscope, ignoring thescopesAllowedToEditparameter entirely. While the comment suggests this is intended behavior, please confirm this design choice is correct—particularly if callers expectscopesAllowedToEditto be honored for API PRODUCTs as well.
221-226: LGTM! Clean scope validation.The logic correctly restricts access when the user lacks any of the required scopes from
scopesAllowedToEdit, preventing unauthorized users from proceeding further in the access-control chain.
|
There is a UI build failure, in particular, Thanks, |
|
|
@tharikaGitHub Hi, now the code is working without failing the test cases. |
3 participants
Summary by CodeRabbit