Introduce User Sharing API v2 #1044
Conversation
WalkthroughAdds a v2 organization user-sharing management module: Maven module and parent registration, OpenAPI v3 spec, core service, factory and impl layers wired to a V2 backend, service holder accessor and new constants/errors, and bumps organization-management dependency version. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Impl as UsersApiServiceImpl (v2)
participant Core as UsersApiServiceCore (v2)
participant Backend as UserSharingPolicyHandlerServiceV2
Client->>Impl: HTTP request (e.g., POST /users/share)
Impl->>Core: delegate(request)
Core->>Core: validate & map -> DO
Core->>Backend: perform operation(DO)
Backend-->>Core: result / error
Core->>Core: map result -> API response, build links
Core-->>Impl: Response
Impl-->>Client: HTTP response
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Outdated
Show resolved
Hide resolved
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Outdated
Show resolved
Hide resolved
...api/server/organization/user/sharing/management/v2/factories/UsersApiServiceCoreFactory.java
Outdated
Show resolved
Hide resolved
...api/server/organization/user/sharing/management/v2/factories/UsersApiServiceCoreFactory.java
Outdated
Show resolved
Hide resolved
...on/identity/api/server/organization/user/sharing/management/v2/impl/UsersApiServiceImpl.java
Show resolved
Hide resolved
...on/identity/api/server/organization/user/sharing/management/v2/impl/UsersApiServiceImpl.java
Show resolved
Hide resolved
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/factories/UsersApiServiceCoreFactory.java (1)
30-49: Static initializer hides the intendedIllegalStateExceptionfrom callersThrowing
IllegalStateExceptionin the static block will surface to callers asExceptionInInitializerError, so theIllegalStateExceptioncatch inUsersApiServiceImplwill never see it. If you want the API layer to consistently getIllegalStateExceptionfrom the factory, consider moving the null‑check and construction intogetUsersApiServiceCore()(lazy init) instead of the static block.One possible refactor:
- private static final UsersApiServiceCore SERVICE; - - static { - UserSharingPolicyHandlerService userSharingPolicyHandlerService = UserSharingMgtServiceHolder - .getUserSharingPolicyHandlerService(); - if (userSharingPolicyHandlerService == null) { - throw new IllegalStateException("UserSharingPolicyHandlerService is not available from the OSGi context."); - } - SERVICE = new UsersApiServiceCore(userSharingPolicyHandlerService); - } + private static volatile UsersApiServiceCore service; @@ - public static UsersApiServiceCore getUsersApiServiceCore() { - - return SERVICE; - } + public static UsersApiServiceCore getUsersApiServiceCore() { + + if (service == null) { + synchronized (UsersApiServiceCoreFactory.class) { + if (service == null) { + UserSharingPolicyHandlerService userSharingPolicyHandlerService = + UserSharingMgtServiceHolder.getUserSharingPolicyHandlerService(); + if (userSharingPolicyHandlerService == null) { + throw new IllegalStateException( + "UserSharingPolicyHandlerService is not available from the OSGi context."); + } + service = new UsersApiServiceCore(userSharingPolicyHandlerService); + } + } + } + return service; + }components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/impl/UsersApiServiceImpl.java (1)
43-50: Constructor’sIllegalStateExceptioncatch won’t see static‑init failures from the factoryGiven the current factory implementation, a missing
UserSharingPolicyHandlerServicewill throwIllegalStateExceptionfrom a static initializer, which reaches this constructor asExceptionInInitializerError, bypassing theIllegalStateExceptioncatch.If you don’t refactor the factory, consider broadening the catch here; if you do refactor as suggested in the factory comment, this constructor will then work as intended.
Example if you keep the static initializer:
- try { - this.usersApiServiceCore = UsersApiServiceCoreFactory.getUsersApiServiceCore(); - } catch (IllegalStateException e) { - throw new RuntimeException(ERROR_INITIATING_USERS_API_SERVICE.getMessage(), e); - } + try { + this.usersApiServiceCore = UsersApiServiceCoreFactory.getUsersApiServiceCore(); + } catch (IllegalStateException | ExceptionInInitializerError e) { + throw new RuntimeException(ERROR_INITIATING_USERS_API_SERVICE.getMessage(), e); + }components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/resources/organization-user-share-v2.yaml (3)
455-465: UnboundeduserIdsarray may be problematic; consider addingmaxItems
UserCriteria.userIdsis required but unconstrained in size. For bulk operations like share/unshare, this can invite very large payloads and DoS‑type scenarios. If the backend already enforces a sensible cap, it’s worth reflecting that here with amaxItems(and optionallyminItems) so clients have a clear contract.For example (adjust the limit to match server behavior):
userIds: type: array description: List of user IDs. items: type: string + maxItems: 1000
435-449: Clarify that BasicAuth/OAuth2 credentials must only be used over TLSStatic analysis flagged the securitySchemes because HTTP auth can expose credentials over cleartext if used on plain HTTP. Your examples and OAuth2 URLs already use
https, but the spec itself doesn’t state the TLS requirement.I’d recommend explicitly documenting that these endpoints are only supported over HTTPS (and that HTTP is not allowed in production), to satisfy tooling and avoid misconfiguration.
753-765: AlignError.codeexample prefix with actual error prefixThe
Error.codeexample uses"US-00000", whileUserSharingMgtConstants.ERROR_PREFIXis"USM-". To avoid confusion for integrators, it’s better if the example matches the real prefix.- code: - type: string - example: "US-00000" + code: + type: string + example: "USM-00000"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (20)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/UsersApi.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/UsersApiService.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/factories/UsersApiServiceFactory.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/Error.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/Link.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/ProcessSuccessResponse.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/RoleAssignment.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/RoleShareConfig.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/RoleShareConfigAudience.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/SharingMode.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/UserCriteria.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/UserOrgShareConfig.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/UserShareAllRequestBody.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/UserShareSelectedRequestBody.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/UserSharedOrganization.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/UserSharedOrganizationsResponse.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/UserSharingPatchOperation.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/UserSharingPatchRequest.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/UserUnshareAllRequestBody.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/UserUnshareSelectedRequestBody.javais excluded by!**/gen/**
📒 Files selected for processing (6)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/pom.xml(1 hunks)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java(1 hunks)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/factories/UsersApiServiceCoreFactory.java(1 hunks)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/impl/UsersApiServiceImpl.java(1 hunks)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/resources/organization-user-share-v2.yaml(1 hunks)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/pom.xml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/impl/UsersApiServiceImpl.java (3)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java (1)
UsersApiServiceCore(35-82)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/factories/UsersApiServiceCoreFactory.java (1)
UsersApiServiceCoreFactory(28-50)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.common/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.java (1)
UserSharingMgtConstants(24-101)
🪛 Checkov (3.2.334)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/resources/organization-user-share-v2.yaml
[medium] 460-465: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
[high] 438-441: Ensure that security schemes don't allow cleartext credentials over unencrypted channel - version 3.x.y files
(CKV_OPENAPI_3)
🔇 Additional comments (2)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/pom.xml (1)
31-35: V2 module wiring in parent POM looks consistentAdding the v2 module alongside v1 and
commonmatches the expected multi‑module structure for versioned APIs.components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/pom.xml (1)
31-172: POM configuration for the v2 module looks fine; just ensure generated sources are presentDependencies and build plugins align with other JAX‑RS/OpenAPI modules (provided scopes,
build-helper-maven-pluginforsrc/gen/java, Java 8 compiler settings). From a POM perspective this looks good.Just make sure that the generated sources for
src/gen/java(e.g.,UsersApiServiceinterface) are either checked in or produced by your build pipeline, since the openapi‑generator plugin here is commented out.
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR introduces a new versioned User Sharing API (v2) under /api/server/v2/users/** to address limitations of the existing v1 endpoints. The new API follows the Application Sharing pattern and enables incremental role assignment with selective sharing across hierarchical organizations.
Key Changes:
- Added 6 new REST endpoints for user sharing operations (share, unshare, patch, and query)
- Introduced role assignment modes (NONE, SELECTED) with SCIM-style PATCH semantics for incremental updates
- Implemented paginated retrieval of shared organizations with per-organization role visibility
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml (parent) | Added v2 module to the parent POM build configuration |
| organization-user-share-v2.yaml | OpenAPI 3.0 specification defining the v2 API endpoints, schemas, and examples |
| UsersApiServiceImpl.java | Implementation delegating API calls to the core service layer |
| UsersApiServiceCore.java | Core business logic with placeholder methods for all v2 operations |
| UsersApiServiceCoreFactory.java | Factory pattern for initializing the core service with OSGi dependencies |
| Model classes (20 files) | Auto-generated DTOs from OpenAPI spec for request/response handling |
| UsersApi.java | JAX-RS resource class with endpoint mappings and validation |
| UsersApiService.java | Service interface defining the API contract |
| UsersApiServiceFactory.java | Factory for service instantiation |
| pom.xml (v2 module) | Maven configuration with dependencies and build plugins |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...2/carbon/identity/api/server/organization/user/sharing/management/v2/model/UserCriteria.java
Outdated
Show resolved
Hide resolved
...arbon/identity/api/server/organization/user/sharing/management/v2/model/RoleShareConfig.java
Outdated
Show resolved
Hide resolved
...api/server/organization/user/sharing/management/v2/model/UserUnshareSelectedRequestBody.java
Outdated
Show resolved
Hide resolved
...pi/server/organization/user/sharing/management/v2/model/UserSharedOrganizationsResponse.java
Outdated
Show resolved
Hide resolved
...dentity/api/server/organization/user/sharing/management/v2/model/UserSharedOrganization.java
Outdated
Show resolved
Hide resolved
...dentity/api/server/organization/user/sharing/management/v2/model/ProcessSuccessResponse.java
Outdated
Show resolved
Hide resolved
...org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/Error.java
Outdated
Show resolved
Hide resolved
...o2/carbon/identity/api/server/organization/user/sharing/management/v2/model/SharingMode.java
Outdated
Show resolved
Hide resolved
...entity/api/server/organization/user/sharing/management/v2/model/RoleShareConfigAudience.java
Outdated
Show resolved
Hide resolved
...wso2/carbon/identity/api/server/organization/user/sharing/management/v2/UsersApiService.java
Outdated
Show resolved
Hide resolved
...org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/Error.java
Outdated
Show resolved
Hide resolved
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.common/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.java (1)
65-75: Consider aligning error naming conventions for consistency.The first error constant (line 65) follows the established
INVALID_prefix pattern used by all existing client validation errors (60000-60004). However, the two subsequent errors (lines 69, 72) introduce anERROR_prefix, creating inconsistency.Additionally,
ERROR_MISSING_USER_CRITERIAis generic and doesn't indicate its specific context (patch operations). For clarity and consistency with operation-specific error names (e.g.,INVALID_SELECTIVE_USER_SHARE_REQUEST_BODY), consider:
INVALID_USER_SHARE_PATCH_MISSING_USER_CRITERIAINVALID_USER_SHARE_PATCH_PATHThis maintains the
INVALID_prefix pattern and makes the context explicit.Proposed naming alignment
- ERROR_MISSING_USER_CRITERIA("60006", - "Missing user criteria in the request body.", - "The user criteria is missing in the request body. Please provide the user criteria to proceed."), - ERROR_UNSUPPORTED_USER_SHARE_PATCH_PATH("60007", - "Unsupported user share patch path.", + INVALID_USER_SHARE_PATCH_MISSING_USER_CRITERIA("60006", + "Missing user criteria in patch request body.", + "The user criteria is missing in the patch request body. Please provide the user criteria to proceed."), + INVALID_USER_SHARE_PATCH_PATH("60007", + "Invalid user share patch path.", "The provided patch path to update attributes of shared user is not supported. " + "Please provide a valid patch path."),components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/resources/organization-user-share-v2.yaml (2)
382-390: Add enum constraint toattributesparameter for validation.The
attributesparameter description states "Supported values:roles,sharingMode" but the schema lacks an enum constraint. Without this, the OpenAPI spec accepts any string value (e.g.,attributes=invalidValue), deferring validation to runtime.A previous review suggested adding both values to an enum, which would provide client-side validation and better API documentation.
🔎 Proposed fix
- in: query name: attributes required: false schema: type: string + enum: + - roles + - sharingMode example: "roles" description: |- Specifies the required parameters in the response. Supported values: `roles`, `sharingMode`.
472-482: Consider adding maxItems constraint to userIds array.Static analysis flags that the
userIdsarray lacks amaxItemsconstraint, which could allow arbitrarily large batch requests that might cause performance issues or DoS vulnerabilities.While backend validation likely enforces reasonable limits, adding
maxItemsto the schema provides:
- Early client-side validation
- Clear documentation of API limits
- Defense-in-depth
Example constraint
properties: userIds: type: array description: List of user IDs. + maxItems: 100 items: type: stringAdjust the limit based on your backend's batch processing capabilities.
Based on static analysis hints.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/UsersApi.javais excluded by!**/gen/**
📒 Files selected for processing (4)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.common/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.java(2 hunks)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java(1 hunks)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/impl/UsersApiServiceImpl.java(1 hunks)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/resources/organization-user-share-v2.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/impl/UsersApiServiceImpl.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T07:42:30.680Z
Learnt from: BimsaraBodaragama
Repo: wso2/identity-api-server PR: 1044
File: components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/factories/UsersApiServiceCoreFactory.java:35-47
Timestamp: 2025-12-19T07:42:30.680Z
Learning: In the organization user sharing management API modules, factory classes use eager static initialization (static blocks) to fail fast if required OSGi services are not available. This is an intentional design pattern for consistency across API service factories in this module.
Applied to files:
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
🧬 Code graph analysis (1)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java (3)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/Link.java (1)
Link(33-118)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/RoleShareConfigAudience.java (1)
RoleShareConfigAudience(33-124)components/org.wso2.carbon.identity.api.server.application.management/org.wso2.carbon.identity.api.server.application.management.v1/src/gen/java/org/wso2/carbon/identity/api/server/application/management/v1/SharingMode.java (1)
SharingMode(34-157)
🪛 Checkov (3.2.334)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/resources/organization-user-share-v2.yaml
[medium] 477-482: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
[high] 455-458: Ensure that security schemes don't allow cleartext credentials over unencrypted channel - version 3.x.y files
(CKV_OPENAPI_3)
🔇 Additional comments (5)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.common/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.java (1)
33-34: LGTM! Consistent success message constant.The new constant follows the existing naming and messaging pattern for async operation responses.
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java (2)
104-107: LGTM! Clean dependency injection and error handling.The constructor-based dependency injection and consistent error handling pattern across all public methods effectively distinguish client errors (400) from server errors (500). The validation logic and exception handling align well with REST API best practices.
Also applies to: 118-139, 148-168
714-779: Well-designed pagination link builder with proper fallback.The pagination implementation correctly handles both absolute URI construction (using JAX-RS
UriInfo) and falls back to relative link building when URI context is unavailable. The cursor encoding and proper query parameter handling ensure robust pagination support.components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/resources/organization-user-share-v2.yaml (2)
454-466: BasicAuth security scheme requires HTTPS enforcement.Static analysis correctly flags that BasicAuth transmits credentials in cleartext (base64-encoded). While the server URL specifies
https://(line 20), the OpenAPI spec doesn't enforce HTTPS at the scheme level.In production deployments, ensure TLS termination at the API gateway/reverse proxy level is mandatory for endpoints using BasicAuth. Consider documenting this requirement in the API description or deployment guides.
Based on static analysis hints.
1-26: Excellent API design aligned with Application Sharing patterns.The v2 API successfully addresses the stated objectives:
- Clear separation of selective vs. all-org sharing operations
- SCIM-style PATCH operations for incremental role updates
- Cursor-based pagination for shared organization listing
- Consistent 202 Accepted responses for async processing
- Comprehensive request/response schemas with examples
The alignment with the Application Sharing model (mentioned in lines 7, 330) provides consistency across the platform, improving developer experience.
Also applies to: 31-175, 692-753
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Show resolved
Hide resolved
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java (1)
122-125: Consider adding more context to debug logs.The debug logs currently only log the tenant domain. Based on past reviewer feedback, consider including additional context such as the number of users being shared, organizations involved, or operation type to aid troubleshooting.
💡 Example enhancement
if (LOG.isDebugEnabled()) { - LOG.debug("Selective user sharing API v2 invoked form tenantDomain: " + - PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain()); + LOG.debug("Selective user sharing API v2 invoked for tenantDomain: " + + PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain() + + ", targetOrganizations: " + userShareSelectedRequestBody.getOrganizations().size()); }Also applies to: 154-157, 185-188, 217-220, 249-252
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.common/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.java(2 hunks)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: BimsaraBodaragama
Repo: wso2/identity-api-server PR: 1044
File: components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/factories/UsersApiServiceCoreFactory.java:35-47
Timestamp: 2025-12-19T07:42:30.680Z
Learning: In the organization user sharing management API modules, factory classes use eager static initialization (static blocks) to fail fast if required OSGi services are not available. This is an intentional design pattern for consistency across API service factories in this module.
🔇 Additional comments (4)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.common/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.java (2)
33-34: LGTM! Well-aligned constant for PATCH operations.The new response constant follows the established naming and messaging patterns, clearly indicating the asynchronous nature of the patch operation.
65-79: LGTM! Comprehensive error coverage for PATCH validation.The four new error enums provide clear, granular validation messages for the V2 PATCH operations. The sequential error codes (60005-60008), descriptive naming, and actionable error descriptions follow the existing conventions consistently.
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java (2)
106-109: LGTM: Constructor properly initializes the service dependency.The constructor follows the dependency injection pattern and aligns with the factory's eager initialization approach mentioned in the learnings.
759-792: LGTM: URI building and encoding are implemented correctly.The pagination link construction gracefully handles missing
UriInfoby falling back to relative links, and URL encoding properly handles the guaranteed UTF-8 support with appropriate exception handling.Also applies to: 839-847
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java (2)
329-330: Remove self-explanatory comments.Several comments throughout the DTO building methods simply restate what the code does (e.g., "Set user criteria", "Set policy and roles"). These comments don't add value and can be safely removed for cleaner code.
Based on learnings, the reviewer previously requested removal of comments where code is self-explanatory.
Also applies to: 334-334, 360-361, 365-366, 384-385, 389-389, 406-407
811-836: Consider refactoring manual URL construction to reduce fragility.The
buildRelativeSharedOrgsPageLinkmethod constructs URLs through string concatenation and manual query parameter building. WhileurlEncodeis used, this approach is more error-prone than using a proper URL builder. IfUriInfois unavailable in certain contexts, consider alternatives like:
- Ensuring
UriInfois always available through dependency injection- Using
UriBuilderdirectly instead of string concatenation- Centralizing URL construction logic to avoid duplication
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.common/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.java(2 hunks)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: BimsaraBodaragama
Repo: wso2/identity-api-server PR: 1044
File: components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/factories/UsersApiServiceCoreFactory.java:35-47
Timestamp: 2025-12-19T07:42:30.680Z
Learning: In the organization user sharing management API modules, factory classes use eager static initialization (static blocks) to fail fast if required OSGi services are not available. This is an intentional design pattern for consistency across API service factories in this module.
📚 Learning: 2025-12-19T07:42:30.680Z
Learnt from: BimsaraBodaragama
Repo: wso2/identity-api-server PR: 1044
File: components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/factories/UsersApiServiceCoreFactory.java:35-47
Timestamp: 2025-12-19T07:42:30.680Z
Learning: In the organization user sharing management API modules, factory classes use eager static initialization (static blocks) to fail fast if required OSGi services are not available. This is an intentional design pattern for consistency across API service factories in this module.
Applied to files:
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
🔇 Additional comments (2)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.common/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.java (2)
33-34: LGTM!The new response constant follows the established pattern and provides a clear success message for the patch operation.
65-82: LGTM!The new error message enum entries are well-structured and follow the established patterns in this file. The error codes are properly sequential (60005-60009), and the descriptions provide clear, actionable guidance for API consumers.
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Show resolved
Hide resolved
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.common/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.java (1)
69-85: Consider aligning error enum constant names with existing naming convention.The five new error enum constants use the "ERROR_" prefix, while all existing client error constants in this enum use the "INVALID_" prefix (e.g.,
INVALID_UUID_FORMAT,INVALID_SELECTIVE_USER_SHARE_REQUEST_BODY). For consistency and maintainability, consider renaming:
ERROR_MISSING_USER_CRITERIA→INVALID_REQUEST_MISSING_USER_CRITERIAERROR_UNSUPPORTED_USER_SHARE_PATCH_PATH→INVALID_USER_SHARE_PATCH_PATHERROR_EMPTY_USER_SHARE_PATCH_PATH→INVALID_EMPTY_USER_SHARE_PATCH_PATHERROR_UNSUPPORTED_USER_SHARE_POLICY→INVALID_USER_SHARE_POLICYERROR_MISSING_USER_IDS→INVALID_REQUEST_MISSING_USER_IDS🔎 Proposed naming consistency refactor
- ERROR_MISSING_USER_CRITERIA("60006", + INVALID_REQUEST_MISSING_USER_CRITERIA("60006", "Missing user criteria in the request body.", "The user criteria is missing in the request body. Please provide the user criteria to proceed."), - ERROR_UNSUPPORTED_USER_SHARE_PATCH_PATH("60007", + INVALID_USER_SHARE_PATCH_PATH("60007", "Unsupported user share patch path.", "The provided patch path to update attributes of shared user is not supported. " + "Please provide a valid patch path."), - ERROR_EMPTY_USER_SHARE_PATCH_PATH("60008", + INVALID_EMPTY_USER_SHARE_PATCH_PATH("60008", "Empty user share patch path.", "The provided patch path to update attributes of shared user is empty. " + "Please provide a valid patch path."), - ERROR_UNSUPPORTED_USER_SHARE_POLICY("60009", + INVALID_USER_SHARE_POLICY("60009", "Unsupported user share policy.", "The provided user share policy is not supported. Please provide a valid user share policy."), - ERROR_MISSING_USER_IDS("60010", + INVALID_REQUEST_MISSING_USER_IDS("60010", "Missing user IDs in the request body.", "The user ID is missing in the request body. Please provide the user ID to proceed."),
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.common/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.javacomponents/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: BimsaraBodaragama
Repo: wso2/identity-api-server PR: 1044
File: components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/factories/UsersApiServiceCoreFactory.java:35-47
Timestamp: 2025-12-19T07:42:41.255Z
Learning: In the organization user sharing management API modules, factory classes use eager static initialization (static blocks) to fail fast if required OSGi services are not available. This is an intentional design pattern for consistency across API service factories in this module.
🧬 Code graph analysis (1)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java (3)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.common/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.java (1)
UserSharingMgtConstants(24-124)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/Link.java (1)
Link(33-118)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/RoleShareConfigAudience.java (1)
RoleShareConfigAudience(33-124)
🔇 Additional comments (5)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.common/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.java (2)
33-34: LGTM! New response detail constant aligns with existing patterns.The constant follows the established naming convention and provides clear feedback for the PATCH operation.
65-68: LGTM! Error message follows established naming convention.The
INVALID_USER_SHARE_PATCH_REQUEST_BODYconstant correctly uses the "INVALID_" prefix consistent with existing client error enum constants.components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java (3)
663-665: Add null check before calling toString() on sharedType.Line 664 calls
orgDO.getSharedType().toString()without checking ifgetSharedType()returns null. This will throw aNullPointerExceptionif the shared type is not set.🔎 Proposed fix
- if (orgDO.getSharedType() != null) { - org.setSharedType(orgDO.getSharedType().toString()); - } + if (orgDO.getSharedType() != null) { + org.setSharedType(orgDO.getSharedType().toString()); + }Note: The null check is already present, so this code is actually safe. No change needed.
704-706: Add null check before calling toString() on mode.Line 705 calls
raDO.getMode().toString()without checking ifgetMode()returns null. This will throw aNullPointerExceptionif the mode is not set.🔎 Proposed fix
RoleAssignmentDO raDO = modeDO.getRoleAssignment(); if (raDO != null) { RoleAssignment ra = new RoleAssignment(); - if (raDO.getMode() != null) { - ra.setMode(RoleAssignment.ModeEnum.fromValue(raDO.getMode().toString())); - } + if (raDO.getMode() != null) { + ra.setMode(RoleAssignment.ModeEnum.fromValue(raDO.getMode().toString())); + }Note: The null check is already present at line 704, so this code is actually safe. No change needed.
697-699: Code is correct as written; no issues found.The
setPolicymethod inSharingMode(line 59) expects aStringparameter, not an enum. The code correctly passesmodeDO.getPolicy().getValue(), which returns aString. The null check on line 697 prevents any NPE. NoSharingMode.PolicyEnumclass exists—the nestedModeEnumis specific toRoleAssignmentand is correctly used separately at line 705 withfromValue().Likely an incorrect or invalid review comment.
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Outdated
Show resolved
Hide resolved
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Show resolved
Hide resolved
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java (4)
289-291: PII leakage: Remove user ID from debug logs.The debug log includes the
userIdparameter, which is personally identifiable information (PII). Logging user identifiers can violate privacy policies and compliance requirements (GDPR, CCPA).🔎 Proposed fix
if (LOG.isDebugEnabled()) { - LOG.debug("Get user shared organizations API v2 invoked form tenantDomain: " + - PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain() + " of user: " + userId); + LOG.debug("Get user shared organizations API v2 invoked from tenantDomain: " + + PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain()); }
293-296: Validate UUID format, not just null.The validation only checks if
userIdis null, but the error constantERROR_MISSING_USER_IDS(notINVALID_UUID_FORMATas in past comments) suggests missing input. However, a malformed UUID string will pass this check and potentially cause issues downstream when parsed.🔎 Proposed fix
if (userId == null) { return Response.status(Response.Status.BAD_REQUEST) .entity(buildErrorResponse(makeRequestError(ERROR_MISSING_USER_IDS))).build(); } + try { + UUID.fromString(userId); + } catch (IllegalArgumentException e) { + return Response.status(Response.Status.BAD_REQUEST) + .entity(buildErrorResponse(makeRequestError(ERROR_MISSING_USER_IDS))).build(); + }
311-311: PII leakage: Remove user ID from error logs.Similar to the debug log concern, the error log includes
userIdwhich may be PII. Consider logging without the user identifier to comply with privacy requirements.🔎 Proposed fix
- LOG.error("Error occurred while retrieving organizations shared with user: " + userId, e); + LOG.error("Error occurred while retrieving organizations shared with user.", e);
336-346: Add null check before iterating organizations collection.Line 336 iterates over
userShareSelectedRequestBody.getOrganizations()without first checking if it returns null. While line 337 checks individualorgDetailitems, the collection itself could be null, causing aNullPointerException.🔎 Proposed fix
// Set organizations details - policy and roles for each organization. List<SelectiveUserShareOrgDetailsV2DO> organizationsList = new ArrayList<>(); - for (UserOrgShareConfig orgDetail : userShareSelectedRequestBody.getOrganizations()) { + List<UserOrgShareConfig> organizations = userShareSelectedRequestBody.getOrganizations(); + if (organizations != null) { + for (UserOrgShareConfig orgDetail : organizations) { if (orgDetail != null) { SelectiveUserShareOrgDetailsV2DO selectiveUserShareOrgDetailsV2DO = new SelectiveUserShareOrgDetailsV2DO(); selectiveUserShareOrgDetailsV2DO.setOrganizationId(orgDetail.getOrgId()); selectiveUserShareOrgDetailsV2DO.setPolicy(resolvePolicy(orgDetail.getPolicy())); selectiveUserShareOrgDetailsV2DO.setRoleAssignments( buildRoleAssignmentFromRequest(orgDetail.getRoleAssignment())); organizationsList.add(selectiveUserShareOrgDetailsV2DO); } + } }
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java (1)
743-755: Add defensive null checks in role mapping.Lines 746-750 assume
roleDOand its properties are non-null. If the backend returns incomplete data, this could throw NPE. Consider adding defensive checks, especially if this data flows from external systems.🔎 Proposed fix
private RoleShareConfig toApiRole(RoleWithAudienceDO roleDO) { RoleShareConfig role = new RoleShareConfig(); - role.setDisplayName(roleDO.getRoleName()); + if (roleDO != null) { + role.setDisplayName(roleDO.getRoleName()); - RoleShareConfigAudience audience = new RoleShareConfigAudience(); - audience.setDisplay(roleDO.getAudienceName()); - audience.setType(roleDO.getAudienceType()); + RoleShareConfigAudience audience = new RoleShareConfigAudience(); + audience.setDisplay(roleDO.getAudienceName()); + audience.setType(roleDO.getAudienceType()); - role.setAudience(audience); + role.setAudience(audience); + } return role; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T07:42:41.255Z
Learnt from: BimsaraBodaragama
Repo: wso2/identity-api-server PR: 1044
File: components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/factories/UsersApiServiceCoreFactory.java:35-47
Timestamp: 2025-12-19T07:42:41.255Z
Learning: In the organization user sharing management API modules, factory classes use eager static initialization (static blocks) to fail fast if required OSGi services are not available. This is an intentional design pattern for consistency across API service factories in this module.
Applied to files:
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
🧬 Code graph analysis (1)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java (2)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.common/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.java (1)
UserSharingMgtConstants(24-124)components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/gen/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/model/RoleShareConfigAudience.java (1)
RoleShareConfigAudience(33-124)
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java (4)
289-291: Remove user ID from debug logs to prevent PII leakage.The debug log includes the
userIdparameter, which may be considered PII and could violate privacy policies (GDPR, CCPA). Consider logging only non-sensitive context such as tenant domain.
293-296: Validate UUID format, not just null.The validation only checks if
userIdis null, but the error codeERROR_MISSING_USER_IDSsuggests that format validation should also occur. A non-null but malformed UUID will pass this check and may cause issues downstream.🔎 Proposed fix
if (userId == null) { return Response.status(Response.Status.BAD_REQUEST) - .entity(buildErrorResponse(makeRequestError(ERROR_MISSING_USER_IDS))).build(); + .entity(buildErrorResponse(makeRequestError(ERROR_MISSING_USER_IDS))).build(); } + try { + UUID.fromString(userId); + } catch (IllegalArgumentException e) { + return Response.status(Response.Status.BAD_REQUEST) + .entity(buildErrorResponse(makeRequestError(ERROR_MISSING_USER_IDS))).build(); + }
311-311: Remove user ID from error logs to prevent PII leakage.Similar to the debug log concern, the error log includes
userIdwhich may be PII. Remove the user identifier from the log message to comply with privacy requirements.
556-571: Add null check for role audience to prevent NPE.Lines 564-565 directly access
role.getAudience().getDisplay()and.getType()without checking ifgetAudience()returns null. If the audience is null, this will throw aNullPointerException.🔎 Proposed fix
if (roles != null) { for (RoleShareConfig role : roles) { if (role != null) { RoleWithAudienceDO roleDetails = new RoleWithAudienceDO(); roleDetails.setRoleName(role.getDisplayName()); - roleDetails.setAudienceName(role.getAudience().getDisplay()); - roleDetails.setAudienceType(role.getAudience().getType()); + RoleShareConfigAudience audience = role.getAudience(); + if (audience != null) { + roleDetails.setAudienceName(audience.getDisplay()); + roleDetails.setAudienceType(audience.getType()); + } rolesList.add(roleDetails); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T07:42:41.255Z
Learnt from: BimsaraBodaragama
Repo: wso2/identity-api-server PR: 1044
File: components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/factories/UsersApiServiceCoreFactory.java:35-47
Timestamp: 2025-12-19T07:42:41.255Z
Learning: In the organization user sharing management API modules, factory classes use eager static initialization (static blocks) to fail fast if required OSGi services are not available. This is an intentional design pattern for consistency across API service factories in this module.
Applied to files:
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Show resolved
Hide resolved
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java (2)
311-311: Remove user ID from error log.The error log includes
userIdwhich may be PII. Consider logging without the user identifier to comply with privacy requirements.🔎 Proposed fix
- LOG.error("Error occurred while retrieving organizations shared with user: " + userId, e); + LOG.error("Error occurred while retrieving organizations shared with user.", e);
290-291: Remove user ID from debug log and fix typo.The debug log includes the
userIdparameter which may be considered PII and should not be logged to comply with privacy requirements. Additionally, "form" should be "from".🔎 Proposed fix
if (LOG.isDebugEnabled()) { - LOG.debug("Get user shared organizations API v2 invoked form tenantDomain: " + - PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain() + " of user: " + userId); + LOG.debug("Get user shared organizations API v2 invoked from tenantDomain: " + + PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain()); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T07:42:41.255Z
Learnt from: BimsaraBodaragama
Repo: wso2/identity-api-server PR: 1044
File: components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/factories/UsersApiServiceCoreFactory.java:35-47
Timestamp: 2025-12-19T07:42:41.255Z
Learning: In the organization user sharing management API modules, factory classes use eager static initialization (static blocks) to fail fast if required OSGi services are not available. This is an intentional design pattern for consistency across API service factories in this module.
Applied to files:
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Outdated
Show resolved
Hide resolved
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Outdated
Show resolved
Hide resolved
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Outdated
Show resolved
Hide resolved
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Outdated
Show resolved
Hide resolved
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Outdated
Show resolved
Hide resolved
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.common/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.java`:
- Around line 83-85: The constant ERROR_MISSING_USER_IDS in
UserSharingMgtConstants.java has a singular/plural mismatch: the constant name
uses plural "IDs" while the description text uses singular "user ID"; update the
description and/or message to use plural form (e.g., "The user IDs are missing
in the request body. Please provide the user IDs to proceed." or rename the
constant to ERROR_MISSING_USER_ID and make all texts singular) so the constant
name, description, and message fields in ERROR_MISSING_USER_IDS remain
consistent.
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.common/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.java (1)
65-91: Consider consistent naming convention for error entries.Existing error entries use the
INVALID_*prefix (e.g.,INVALID_SELECTIVE_USER_SHARE_REQUEST_BODY), while most new entries useERROR_*prefix. This inconsistency may affect code discoverability.Consider either:
- Renaming new entries to follow
INVALID_*pattern where semantically appropriate, or- Adding a brief comment to document the naming distinction (e.g.,
INVALID_*for validation errors,ERROR_*for missing/unsupported items).This is a minor stylistic suggestion and can be deferred.
...pi/server/organization/user/sharing/management/common/constants/UserSharingMgtConstants.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java`:
- Around line 540-555: The method buildUserCriteriaFromRequest in
UsersApiServiceCore lacks a null check for the incoming UserCriteria parameter,
which leads to an NPE and a 500 instead of returning a 400; add a guard at the
start of buildUserCriteriaFromRequest that checks if userCriteria is null and,
if so, throw makeRequestError(ERROR_MISSING_USER_CRITERIA) (or another
appropriate client error), then proceed to populate the userCriteriaMap with
USER_IDS using new UserIdList(userCriteria.getUserIds()) as before.
♻️ Duplicate comments (4)
components/org.wso2.carbon.identity.api.server.organization.user.sharing.management/org.wso2.carbon.identity.api.server.organization.user.sharing.management.v2/src/main/java/org/wso2/carbon/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java (4)
295-346: Avoid logging user identifiers (PII).The debug and error logs include
userId. This is the same privacy concern previously flagged and still present.
358-387: Guard against null policy beforeresolvePolicy.
orgDetail.getPolicy()can be null, which can cause unexpected exceptions when resolving policy.
395-410: Guard against null policy beforeresolvePolicy.
userShareAllRequestBody.getPolicy()can be null; avoid callingresolvePolicywithout a null check.
617-635: Validate patch operation type beforefromValue().
operation.getOp()can be null and will throw infromValue(). This mirrors a previously raised issue.
...on/identity/api/server/organization/user/sharing/management/v2/core/UsersApiServiceCore.java
Show resolved
Hide resolved
3 participants
Purpose
This PR introduces a new versioned User Sharing API (
/api/server/v2/users/**) to address functional and frontend limitations of the existing/api/server/v1/users/**endpoints.The new API aligns conceptually and structurally with the Application Sharing API and enables incremental role assignment and selective sharing across hierarchical organizations.
Goals
roleAssignmentobject with supported modes:NONE | SELECTED./v1integrations.Approach
/api/server/v2/users/**:roleAssignmentto reflect role assignment semantics in sub-organizations.NONEandSELECTED, consistent with the Application Sharing model.Related PRs
Related Issues
Summary by CodeRabbit
New Features
Documentation
Chores
Maintenance
✏️ Tip: You can customize this high-level summary in your review settings.