Introduce New Device Management API Endpoint - #1150
Open
kaviska wants to merge 4 commits into
Open
Conversation
Contributor
📝 WalkthroughSummary
WalkthroughAdds a new Maven device-management module with shared constants, error definitions, and OSGi service access. Defines an OpenAPI 3.0 contract for listing, retrieving, renaming, and deleting devices, including pagination, security schemes, schemas, and generated CXF sources. Adds REST delegation and a core service that validates requests, applies tenant scoping, maps device data, builds pagination links, and converts backend exceptions into API errors. Sequence Diagram(s)sequenceDiagram
participant Client
participant DevicesApiServiceImpl
participant DeviceManagementApiService
participant DeviceManagementService
Client->>DevicesApiServiceImpl: Request device operation
DevicesApiServiceImpl->>DeviceManagementApiService: Delegate request
DeviceManagementApiService->>DeviceManagementService: Query or modify device
DeviceManagementService-->>DeviceManagementApiService: Return result or exception
DeviceManagementApiService-->>DevicesApiServiceImpl: Return API model or APIError
DevicesApiServiceImpl-->>Client: Return HTTP response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/main/java/org/wso2/carbon/identity/api/server/device/mgt/v1/core/DeviceManagementApiService.java`:
- Around line 134-170: The pagination logic in the previous-link path can
overflow and recursively traverse many pages. Update the relevant calculations
in the link-building method and calculateOffsetForPreviousLink to use long
arithmetic for offset, limit, and total comparisons, and replace recursive
page-by-page stepping with a bounded or constant-time calculation that returns a
valid previous offset. Preserve existing link behavior, including the
zero-offset branch and valid limit handling.
In
`@components/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/main/resources/device-mgt.yaml`:
- Around line 30-32: Update the OpenAPI security configuration in the device
management specification to declare the documented internal_device_mgt_* OAuth2
scopes instead of an empty scope list, then add the applicable OAuth2 scope
requirement to each affected operation, including the operations in the
referenced range. Keep BasicAuth unchanged and align every operation’s required
scope with its named permission.
- Around line 43-81: Update the pagination parameters in the OpenAPI schema by
adding maximum: 100 to the limit schema, matching
Constants.ErrorMessage.ERROR_CODE_INVALID_PAGINATION validation. Add a 400
response to the operation’s responses, documenting the invalid-pagination error
using the existing Error schema.
🪄 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 Plus
Run ID: 51d36a2e-8df5-4d22-b02c-9d42bae2870e
⛔ Files ignored due to path filters (8)
components/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/gen/java/org/wso2/carbon/identity/api/server/device/mgt/v1/DevicesApi.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/gen/java/org/wso2/carbon/identity/api/server/device/mgt/v1/DevicesApiService.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/gen/java/org/wso2/carbon/identity/api/server/device/mgt/v1/factories/DevicesApiServiceFactory.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/gen/java/org/wso2/carbon/identity/api/server/device/mgt/v1/model/DeviceListLink.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/gen/java/org/wso2/carbon/identity/api/server/device/mgt/v1/model/DeviceListResponse.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/gen/java/org/wso2/carbon/identity/api/server/device/mgt/v1/model/DevicePatchRequest.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/gen/java/org/wso2/carbon/identity/api/server/device/mgt/v1/model/DeviceResponse.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/gen/java/org/wso2/carbon/identity/api/server/device/mgt/v1/model/Error.javais excluded by!**/gen/**
📒 Files selected for processing (11)
components/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.common/pom.xmlcomponents/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.common/src/main/java/org/wso2/carbon/identity/api/server/device/mgt/common/Constants.javacomponents/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.common/src/main/java/org/wso2/carbon/identity/api/server/device/mgt/common/DeviceMgtServiceHolder.javacomponents/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/pom.xmlcomponents/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/main/java/org/wso2/carbon/identity/api/server/device/mgt/v1/core/DeviceManagementApiService.javacomponents/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/main/java/org/wso2/carbon/identity/api/server/device/mgt/v1/factories/DeviceServiceFactory.javacomponents/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/main/java/org/wso2/carbon/identity/api/server/device/mgt/v1/function/DeviceResponseBuilder.javacomponents/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/main/java/org/wso2/carbon/identity/api/server/device/mgt/v1/impl/DevicesApiServiceImpl.javacomponents/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/main/resources/device-mgt.yamlcomponents/org.wso2.carbon.identity.api.server.device.mgt/pom.xmlpom.xml
Comment on lines
+134
to
+170
| if ((offset + limit) < totalResults) { | ||
| links.add(buildPageLink(baseUrl, Constants.PAGE_LINK_REL_NEXT, offset + limit, limit, userId)); | ||
| } | ||
|
|
||
| /* | ||
| Previous link. | ||
| Previous link matters only if offset is greater than 0. | ||
| */ | ||
| if (offset > 0) { | ||
| if ((offset - limit) >= 0) { // A previous page of size 'limit' exists. | ||
| links.add(buildPageLink(baseUrl, Constants.PAGE_LINK_REL_PREVIOUS, | ||
| calculateOffsetForPreviousLink(offset, limit, totalResults), limit, userId)); | ||
| } else { // A previous page exists but it's size is less than the specified limit. | ||
| links.add(buildPageLink(baseUrl, Constants.PAGE_LINK_REL_PREVIOUS, 0, offset, userId)); | ||
| } | ||
| } | ||
|
|
||
| return links; | ||
| } | ||
|
|
||
| private DeviceListLink buildPageLink(String baseUrl, String rel, int offset, int limit, String userId) { | ||
|
|
||
| String query = StringUtils.isNotBlank(userId) | ||
| ? String.format(Constants.PAGINATION_WITH_USER_ID_LINK_FORMAT, offset, limit, userId) | ||
| : String.format(Constants.PAGINATION_LINK_FORMAT, offset, limit); | ||
| return new DeviceListLink().rel(rel).href(ContextLoader.buildURIForBody(baseUrl + query).toString()); | ||
| } | ||
|
|
||
| private int calculateOffsetForPreviousLink(int offset, int limit, int total) { | ||
|
|
||
| int newOffset = offset - limit; | ||
| if (newOffset < total) { | ||
| return newOffset; | ||
| } | ||
|
|
||
| // If offset is greater than total, go back by the chunks of limit until a proper page is found. | ||
| return calculateOffsetForPreviousLink(newOffset, limit, total); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Avoid overflow and recursive previous-link calculation.
Line 134 can overflow for large valid offsets, and Lines 162-170 recursively step backward one page at a time. Use long for offset arithmetic and a bounded or constant-time previous-offset calculation so pagination links remain valid for large inputs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/main/java/org/wso2/carbon/identity/api/server/device/mgt/v1/core/DeviceManagementApiService.java`
around lines 134 - 170, The pagination logic in the previous-link path can
overflow and recursively traverse many pages. Update the relevant calculations
in the link-building method and calculateOffsetForPreviousLink to use long
arithmetic for offset, limit, and total comparisons, and replace recursive
page-by-page stepping with a bounded or constant-time calculation that returns a
valid previous offset. Preserve existing link behavior, including the
zero-offset branch and valid limit handling.
Comment on lines
+30
to
+32
| security: | ||
| - OAuth2: [] | ||
| - BasicAuth: [] |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Declare and require the documented OAuth2 permissions.
The global OAuth2 requirement uses an empty scope list, and the declared scope names do not match the internal_device_mgt_* permissions named by the operations. Define those permission names as scopes and require the applicable scope on each operation.
Also applies to: 212-221
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/main/resources/device-mgt.yaml`
around lines 30 - 32, Update the OpenAPI security configuration in the device
management specification to declare the documented internal_device_mgt_* OAuth2
scopes instead of an empty scope list, then add the applicable OAuth2 scope
requirement to each affected operation, including the operations in the
referenced range. Keep BasicAuth unchanged and align every operation’s required
scope with its named permission.
Comment on lines
+43
to
+81
| - name: limit | ||
| in: query | ||
| description: Maximum number of records to return. | ||
| required: false | ||
| schema: | ||
| type: integer | ||
| minimum: 1 | ||
| default: 30 | ||
| - name: offset | ||
| in: query | ||
| description: Number of records to skip for pagination. | ||
| required: false | ||
| schema: | ||
| type: integer | ||
| minimum: 0 | ||
| default: 0 | ||
| - name: userId | ||
| in: query | ||
| description: Filter devices by the ID of the user who registered them. Returns devices of any status. Pagination applies as usual. | ||
| required: false | ||
| schema: | ||
| type: string | ||
| responses: | ||
| '200': | ||
| description: Successful Response | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/DeviceListResponse' | ||
| '401': | ||
| description: Unauthorized | ||
| '403': | ||
| description: Forbidden | ||
| '500': | ||
| description: Server Error | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/Error' |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Align the pagination contract with server validation.
Constants.ErrorMessage.ERROR_CODE_INVALID_PAGINATION limits limit to 1–100, but this schema has no maximum, and the operation omits its 400 error response. Add maximum: 100 and document the invalid-pagination response.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/org.wso2.carbon.identity.api.server.device.mgt/org.wso2.carbon.identity.api.server.device.mgt.v1/src/main/resources/device-mgt.yaml`
around lines 43 - 81, Update the pagination parameters in the OpenAPI schema by
adding maximum: 100 to the limit schema, matching
Constants.ErrorMessage.ERROR_CODE_INVALID_PAGINATION validation. Add a 400
response to the operation’s responses, documenting the invalid-pagination error
using the existing Error schema.
Contributor
|
Why we need this functionality |
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This PR adds a new Device Management REST API (
/api/server/v1/devices) to the Identity Server API layer. It exposes the underlyingDeviceManagementServiceover HTTP, allowing tenant administrators to manage registered devices through the Admin API.Goals
The API provides the following device management operations:
limitandoffset).userId) while preserving pagination and total result counts.Implementation
A new Maven component,
org.wso2.carbon.identity.api.server.device.mgt, has been introduced following the existing API Server structure.Modules
.commonDeviceMgtServiceHolderfor accessing theDeviceManagementServiceOSGi service..v1device-mgt.yamldefines the OpenAPI specification, with API models and interfaces generated from it.DevicesApiServiceImplimplements the JAX-RS endpoints.DeviceManagementApiServicecontains the API logic, including request validation, tenant resolution, and delegation toDeviceManagementService.DeviceServiceFactoryinitializes and provides the backend service instance.User stories
Developer Checklist (Mandatory)
product-isissue to capture any behavioral changes or migration impacts.Release note
N/A
Documentation
N/A
Training
N/A
Certification
N/A
Marketing
N/A
Automation tests
N/A
Security checks
Samples
N/A
Related PRs
wso2/carbon-identity-framework#8202
Migrations
N/A – This introduces a new API and does not require any migration.
Test environment
N/A
Learning
N/A