Fix #250: Reject empty/whitespace-only roleName in role create/update endpoints#561
Open
SasinduDilshara wants to merge 5 commits intowso2:mainfrom
Open
Fix #250: Reject empty/whitespace-only roleName in role create/update endpoints#561SasinduDilshara wants to merge 5 commits intowso2:mainfrom
SasinduDilshara wants to merge 5 commits intowso2:mainfrom
Conversation
…ole endpoints Add validation in `auth_service.bal` to trim and check `roleName` length before persisting, returning HTTP 400 for empty or whitespace-only values. Also add validation to the PUT update handler and extend `auth_tests_v2.bal` with negative tests covering empty, whitespace-only, and tab/newline names for both create and update paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Author
Issue Analysis — [Issue #250]: POST
|
Contributor
WalkthroughThis PR introduces comprehensive project documentation (CLAUDE.md and five specs files covering architecture, contributing guidelines, deployment, features, and testing) and implements input validation for RBAC v2 role names (rejecting empty or whitespace-only values) with corresponding test coverage. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 Warning |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+2509
to
+2512
| // Validate roleName is not empty or whitespace-only | ||
| if roleInput.roleName.trim().length() == 0 { | ||
| return utils:createBadRequestError("roleName must not be empty or whitespace-only"); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| // Validate roleName is not empty or whitespace-only | |
| if roleInput.roleName.trim().length() == 0 { | |
| return utils:createBadRequestError("roleName must not be empty or whitespace-only"); | |
| } | |
| // Validate roleName is not empty or whitespace-only | |
| if roleInput.roleName.trim().length() == 0 { | |
| log:printWarn("Attempt to create role with empty or whitespace-only name", orgHandle = orgHandle); | |
| return utils:createBadRequestError("roleName must not be empty or whitespace-only"); | |
| } |
Comment on lines
+2670
to
+2673
| // Validate roleName is not empty or whitespace-only | ||
| if roleInput.roleName.trim().length() == 0 { | ||
| return utils:createBadRequestError("roleName must not be empty or whitespace-only"); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| // Validate roleName is not empty or whitespace-only | |
| if roleInput.roleName.trim().length() == 0 { | |
| return utils:createBadRequestError("roleName must not be empty or whitespace-only"); | |
| } | |
| // Validate roleName is not empty or whitespace-only | |
| if roleInput.roleName.trim().length() == 0 { | |
| log:printWarn("Attempt to update role with empty or whitespace-only name", orgHandle = orgHandle, roleId = roleId); | |
| return utils:createBadRequestError("roleName must not be empty or whitespace-only"); | |
| } |
Contributor
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | ||
| #### Log Improvement Suggestion No: 2 |
![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: 1
🧹 Nitpick comments (4)
specs/Architecture.md (1)
5-18: Consider adding language specifier to code block.The connection diagram code block lacks a language identifier. Adding one (e.g.,
textorplaintext) improves rendering consistency and satisfies markdown linting.Add language specifier
-``` +```text Browser -> ICP-AUTH/HB:9445🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/Architecture.md` around lines 5 - 18, The fenced code block containing the connection diagram (starting with the line "Browser -> ICP-AUTH/HB:9445") lacks a language identifier; update the opening backticks to include a plain text specifier (for example change ``` to ```text or ```plaintext) so the block is rendered consistently and satisfies markdown linting, leaving the diagram lines (e.g., "ICP-GraphQL -> DB:5432", "ICP-OBS-ADPT -> OpenSearch:9200") unchanged.CLAUDE.md (1)
29-29: Typo in filename: "Deployement" should be "Deployment".The path references
specs/Deployement.mdwhich matches the actual filename, but both contain a typo. Consider renaming the file tospecs/Deployment.mdfor correctness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 29, Update the typo in the referenced filename inside CLAUDE.md by changing the string 'specs/Deployement.md' to the correct 'specs/Deployment.md' and rename the actual file on disk from Deployement.md to Deployment.md so the link and filename match; ensure any other references to 'Deployement.md' in the repo are likewise updated to 'Deployment.md'.specs/Deployement.md (1)
1-1: Typo in filename: "Deployement" should be "Deployment".Consider renaming the file to
specs/Deployment.md.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/Deployement.md` at line 1, Rename the file "specs/Deployement.md" to "specs/Deployment.md" to fix the typo; update any code, build scripts, README links, imports, or CI references that point to "specs/Deployement.md" to use "specs/Deployment.md" instead so no broken references remain (search for the exact filename string "Deployement.md" across the repo and replace).icp_server/tests/auth_tests_v2.bal (1)
495-535: Consider verifying specific error message content.The tests assert
responseBody.message is stringbut don't verify the message text matches the expected"roleName must not be empty or whitespace-only". This could help catch regressions where the validation exists but the message changes unexpectedly.Optional: Assert specific message content
test:assertEquals(response.statusCode, 400, "Expected 400 for empty roleName on update"); json responseBody = check response.getJsonPayload(); test:assertTrue(responseBody.message is string, "Error message should be present"); + string message = check responseBody.message; + test:assertTrue(message.includes("roleName must not be empty"), "Error message should mention roleName validation");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@icp_server/tests/auth_tests_v2.bal` around lines 495 - 535, Update the two tests testUpdateRoleWithEmptyName and testUpdateRoleWithWhitespaceName to assert the exact validation message instead of only checking the type: after extracting json responseBody (responseBody.message) replace the loose test:assertTrue(responseBody.message is string) with an equality assertion that responseBody.message equals the expected string "roleName must not be empty or whitespace-only" (use your test framework's assertEquals or equivalent to compare the message), so both tests validate the precise error text returned on update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specs/Deployement.md`:
- Around line 54-57: The UI port is inconsistent: the "UI" section lists
https://localhost:9460 while the earlier spec lists "9445: Auth REST + SPA +
Heartbeat"; verify which port actually serves the web UI, update the "UI"
heading to the correct port (either change 9460 to 9445 or vice‑versa), and make
the deployment docs consistent by aligning the port number in the "UI" section
and the "9445: Auth REST + SPA + Heartbeat" entry and, if applicable, adjust the
corresponding deployment configuration (docker-compose/k8s service) to match the
corrected port.
---
Nitpick comments:
In `@CLAUDE.md`:
- Line 29: Update the typo in the referenced filename inside CLAUDE.md by
changing the string 'specs/Deployement.md' to the correct 'specs/Deployment.md'
and rename the actual file on disk from Deployement.md to Deployment.md so the
link and filename match; ensure any other references to 'Deployement.md' in the
repo are likewise updated to 'Deployment.md'.
In `@icp_server/tests/auth_tests_v2.bal`:
- Around line 495-535: Update the two tests testUpdateRoleWithEmptyName and
testUpdateRoleWithWhitespaceName to assert the exact validation message instead
of only checking the type: after extracting json responseBody
(responseBody.message) replace the loose test:assertTrue(responseBody.message is
string) with an equality assertion that responseBody.message equals the expected
string "roleName must not be empty or whitespace-only" (use your test
framework's assertEquals or equivalent to compare the message), so both tests
validate the precise error text returned on update.
In `@specs/Architecture.md`:
- Around line 5-18: The fenced code block containing the connection diagram
(starting with the line "Browser -> ICP-AUTH/HB:9445") lacks a language
identifier; update the opening backticks to include a plain text specifier (for
example change ``` to ```text or ```plaintext) so the block is rendered
consistently and satisfies markdown linting, leaving the diagram lines (e.g.,
"ICP-GraphQL -> DB:5432", "ICP-OBS-ADPT -> OpenSearch:9200") unchanged.
In `@specs/Deployement.md`:
- Line 1: Rename the file "specs/Deployement.md" to "specs/Deployment.md" to fix
the typo; update any code, build scripts, README links, imports, or CI
references that point to "specs/Deployement.md" to use "specs/Deployment.md"
instead so no broken references remain (search for the exact filename string
"Deployement.md" across the repo and replace).
🪄 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: f2d4c74c-5f15-4ffe-811c-73ea54d0f66f
📒 Files selected for processing (8)
CLAUDE.mdicp_server/auth_service.balicp_server/tests/auth_tests_v2.balspecs/Architecture.mdspecs/Contributing.mdspecs/Deployement.mdspecs/Features.mdspecs/Testing.md
Comment on lines
+54
to
+57
| ## UI | ||
|
|
||
| `https://localhost:9460` admin:admin | ||
| Three levels: organizations → projects → components. |
Contributor
There was a problem hiding this comment.
Port inconsistency with earlier documentation.
Line 20 states 9445: Auth REST + SPA + Heartbeat serves the frontend, but Line 56 references https://localhost:9460 for the UI. This inconsistency could confuse users. Please verify which port serves the web UI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/Deployement.md` around lines 54 - 57, The UI port is inconsistent: the
"UI" section lists https://localhost:9460 while the earlier spec lists "9445:
Auth REST + SPA + Heartbeat"; verify which port actually serves the web UI,
update the "UI" heading to the correct port (either change 9460 to 9445 or
vice‑versa), and make the deployment docs consistent by aligning the port number
in the "UI" section and the "9445: Auth REST + SPA + Heartbeat" entry and, if
applicable, adjust the corresponding deployment configuration
(docker-compose/k8s service) to match the corrected port.
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.
Summary
roleName.trim().length() == 0guard inPOST /auth/orgs/{orgHandle}/roleshandler (auth_service.bal:2506) to return HTTP 400 for empty or whitespace-only role names.PUT /auth/orgs/{orgHandle}/roles/{roleId}update handler, and addhttp:BadRequestto its return type union.auth_tests_v2.balcovering: empty name on create, whitespace-only name on create, tab/newline name on create, empty name on update, whitespace-only name on update.Test plan
testCreateRoleWithEmptyName— POST withroleName:"", assert HTTP 400testCreateRoleWithWhitespaceName— POST withroleName:" ", assert HTTP 400testCreateRoleWithTabNewlineName— POST withroleName:"\t\n", assert HTTP 400testUpdateRoleWithEmptyName— PUT withroleName:"", assert HTTP 400testUpdateRoleWithWhitespaceName— PUT withroleName:" ", assert HTTP 400testCreateRole,testUpdateRole) continue to pass🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Documentation