Introduce the device management component#8202
Open
kaviska wants to merge 6 commits into
Open
Conversation
Introduce the standalone device registration/management backend bundle: - New module components/device-mgt/org.wso2.carbon.identity.device.mgt with service, DAO, cache, audit logging and OSGi component. - Register the module in the root and device-mgt aggregator POMs (7.11.153-SNAPSHOT). - Add IDN_DEVICE and IDN_USER_DEVICE DDL to all central dbscripts.
- Remove the device registration protocol (initiate/verify, challenge cache and signature verification) from this component. It now lives with its only consumer in the device registration component. - Introduce Device.Status (ACTIVE, INACTIVE) with activateDevice() and deactivateDevice(), replacing the hardcoded status string. Device listing by user now returns only active devices, while admin listings return all. - Remove the unbounded getAllDevices(); the paginated getDevices() and getDeviceCount() pair covers the use case. - Guard against a null device after the post-update re-fetch, so a device removed mid-operation surfaces as DEVICE_NOT_FOUND instead of an NPE. - Add a cache layer (DeviceCache + CacheBackedDeviceManagementDAO) for device lookups by ID, with invalidation on update, status change and delete.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a tenant-scoped device management module with public APIs, JDBC persistence, caching, audit logging, OSGi registration, database schemas for multiple platforms, and TestNG-based DAO, service, cache, and exception-handler coverage. ChangesDevice Management
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
🚨 New Database Tables DetectedThis PR introduces new database table(s). Please ensure that the corresponding resource cleanup scripts are updated accordingly. New Tables:
Required Actions:
Database Support Checklist:Please verify your new table(s) are compatible with all supported databases:
This is an automated message. If this is a false positive or if you have questions, please reach out to the maintainers. |
![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: 7
🧹 Nitpick comments (4)
components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/api/service/DeviceManagementService.java (1)
31-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
@throwsJavadoc tags for consistency.
activateDeviceanddeactivateDevicedocument@throws DeviceMgtException, but the other seven methods that declarethrows DeviceMgtExceptionomit the tag. Adding@throwsto all methods ensures API consumers understand the failure contract uniformly.📝 Example for persistDevice
* `@param` device The verified device to persist. * `@param` tenantDomain Tenant domain. + * `@throws` DeviceMgtException If persistence fails. */ void persistDevice(Device device, String tenantDomain) throws DeviceMgtException;Also applies to: 47-48, 58-59, 71-72, 81-82, 92-93, 124-125
🤖 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/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/api/service/DeviceManagementService.java` around lines 31 - 37, Add `@throws` DeviceMgtException Javadoc tags to persistDevice and the other seven DeviceManagementService methods that declare this exception but currently omit the tag, matching the existing activateDevice and deactivateDevice documentation.components/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/java/org/wso2/carbon/identity/device/mgt/dao/DeviceManagementDAOImplTest.java (1)
39-374: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage is comprehensive.
The tests cover registration, retrieval, tenant isolation, status transitions, update/delete with wrong-tenant no-op, and full-field round-trip. Test ordering via
priorityanddependsOnMethodsis correct, and each test uses unique UUIDs to avoid cross-test interference.One minor gap: there is no test exercising actual pagination behavior (e.g.,
getDevices(tenantId, 1, 1)to verify offset/limit). Consider adding one if pagination correctness is a priority.🤖 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/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/java/org/wso2/carbon/identity/device/mgt/dao/DeviceManagementDAOImplTest.java` around lines 39 - 374, Add a focused pagination test alongside testGetDevicesReturnsAllForTenant, invoking getDevices with a nonzero offset and limited page size such as offset 1 and limit 1. Register multiple uniquely identified devices for one tenant, then assert the returned page contains exactly the expected single device, validating both offset and limit behavior.components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/CacheBackedDeviceManagementDAO.java (1)
162-168: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCache invalidation has a TOCTOU window.
The clear-before-write pattern (e.g., Line 164 clears the cache, then Line 168 performs the update) has a race window: a concurrent
getDeviceByIdbetween the cache clear and the DAO write can read the old value from the DAO and repopulate the cache with stale data. This is a known trade-off of cache-aside and is typically acceptable, but if strict consistency is required, consider clearing the cache after the write succeeds as well (double-invalidate), or using a short TTL on cache entries as a safety net.Also applies to: 185-189, 203-207
🤖 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/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/CacheBackedDeviceManagementDAO.java` around lines 162 - 168, Update the cache invalidation in updateDeviceName and the other identified device mutation methods to clear the deviceCache again after deviceManagementDAO persistence succeeds, while retaining the existing pre-write invalidation. Ensure post-write invalidation occurs only after a successful DAO update so concurrent reads cannot leave the updated entry permanently stale.features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/mssql.sql (1)
2343-2344: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider adding a composite index on
(TENANT_ID, REGISTERED_AT)for pagination performance.The existing
IDX_IDV_STATUS (STATUS, TENANT_ID)index supports status-filtered lookups, but the tenant-wide pagination query (GET_ALL_DEVICES_PAGINATED_MSSQL) filters byTENANT_IDand orders byREGISTERED_AT DESCwithout a matching index. The count query (GET_DEVICES_COUNT) also filters byTENANT_IDalone. For tenants with many devices, this will cause a full scan and sort. A composite index on(TENANT_ID, REGISTERED_AT)would enable an index range scan with sorted output, eliminating the sort step.This applies to all database scripts (mssql, mysql, mysql-cluster, h2, postgresql, oracle, db2).
♻️ Proposed index addition for MSSQL
CREATE INDEX IDX_IDV_STATUS ON IDN_DEVICE (STATUS, TENANT_ID); CREATE INDEX IDX_IUD_USER_TENANT ON IDN_USER_DEVICE (USER_ID, TENANT_ID); +CREATE INDEX IDX_IDV_TENANT_REGISTERED ON IDN_DEVICE (TENANT_ID, REGISTERED_AT);🤖 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 `@features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/mssql.sql` around lines 2343 - 2344, Add a composite index on IDN_DEVICE covering TENANT_ID and REGISTERED_AT in every database script variant: mssql, mysql, mysql-cluster, h2, postgresql, oracle, and db2. Keep the existing IDX_IDV_STATUS index unchanged and follow each script’s established index-creation syntax and naming conventions.
🤖 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/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/cache/DeviceCacheKey.java`:
- Around line 38-41: Prevent null deviceId failures in DeviceCacheKey equality
and hashing by validating deviceId in the DeviceCacheKey constructor, or by
making equals() and hashCode() null-safe with Objects.equals and Objects.hash.
Preserve consistent equality and hash behavior for valid and null keys.
In
`@components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/DeviceManagementDAOImpl.java`:
- Around line 160-196: Cap the requested page size before it reaches the
paginated query in getDevices, using the project’s established maximum page-size
constant if available. Apply the cap alongside the existing limit validation and
use the bounded value for bindPaginationParams, while preserving the current
empty-result behavior for nonpositive limits.
In
`@components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/service/impl/DeviceManagementServiceImpl.java`:
- Around line 61-75: Update the missing userId validation in persistDevice to
call DeviceManagementExceptionHandler.handleClientException instead of
handleServerException, matching the existing validateRequiredField behavior.
Preserve the null/blank check and ERROR_USER_ID_REQUIRED error message.
- Around line 61-67: Update persistDevice to validate that the Device argument
is non-null and that id, userId, deviceName, publicKey, and status are all
present before calling deviceManagementDAO.registerDevice. Reject missing or
blank string fields with the existing clear validation-error mechanism, and
handle null status without dereferencing it; only invoke the DAO after all
required fields pass validation.
- Around line 95-99: Update getDevices to validate offset and limit before
calling deviceManagementDAO.getDevices: reject negative offset values and
non-positive limit values, using the service’s established DeviceMgtException
pattern for invalid input. Preserve the existing tenant ID resolution and DAO
invocation for valid pagination parameters.
In
`@features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/oracle.sql`:
- Around line 3341-3352: Update the IDN_DEVICE.PUBLIC_KEY and
IDN_DEVICE.METADATA column definitions in the Oracle schema to match the shared
4096-byte contract used by the other database scripts, ensuring values up to
that limit are accepted consistently.
- Around line 3361-3364: Update the Oracle schema near IDX_IDV_STATUS to add an
index on IDN_DEVICE covering TENANT_ID and REGISTERED_AT, matching the
tenant-scoped ordering used by DeviceManagementDAO.getDevices. Preserve the
existing indexes and use the repository’s standard index naming and SQL
terminator conventions.
---
Nitpick comments:
In
`@components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/api/service/DeviceManagementService.java`:
- Around line 31-37: Add `@throws` DeviceMgtException Javadoc tags to
persistDevice and the other seven DeviceManagementService methods that declare
this exception but currently omit the tag, matching the existing activateDevice
and deactivateDevice documentation.
In
`@components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/CacheBackedDeviceManagementDAO.java`:
- Around line 162-168: Update the cache invalidation in updateDeviceName and the
other identified device mutation methods to clear the deviceCache again after
deviceManagementDAO persistence succeeds, while retaining the existing pre-write
invalidation. Ensure post-write invalidation occurs only after a successful DAO
update so concurrent reads cannot leave the updated entry permanently stale.
In
`@components/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/java/org/wso2/carbon/identity/device/mgt/dao/DeviceManagementDAOImplTest.java`:
- Around line 39-374: Add a focused pagination test alongside
testGetDevicesReturnsAllForTenant, invoking getDevices with a nonzero offset and
limited page size such as offset 1 and limit 1. Register multiple uniquely
identified devices for one tenant, then assert the returned page contains
exactly the expected single device, validating both offset and limit behavior.
In
`@features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/mssql.sql`:
- Around line 2343-2344: Add a composite index on IDN_DEVICE covering TENANT_ID
and REGISTERED_AT in every database script variant: mssql, mysql, mysql-cluster,
h2, postgresql, oracle, and db2. Keep the existing IDX_IDV_STATUS index
unchanged and follow each script’s established index-creation syntax and naming
conventions.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 25eccfe1-1f33-426b-82e3-9647a09d76ea
📒 Files selected for processing (36)
components/device-mgt/org.wso2.carbon.identity.device.mgt/pom.xmlcomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/api/constant/ErrorMessage.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/api/exception/DeviceMgtClientException.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/api/exception/DeviceMgtException.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/api/exception/DeviceMgtServerException.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/api/model/Device.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/api/service/DeviceManagementService.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/cache/DeviceCache.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/cache/DeviceCacheEntry.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/cache/DeviceCacheKey.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/component/DeviceMgtServiceComponent.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/constant/DeviceMgtSQLConstants.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/DeviceManagementDAO.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/CacheBackedDeviceManagementDAO.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/DeviceManagementDAOImpl.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/service/impl/DeviceManagementServiceImpl.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/util/DeviceManagementAuditLogger.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/util/DeviceManagementExceptionHandler.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/java/org/wso2/carbon/identity/device/mgt/dao/CacheBackedDeviceManagementDAOTest.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/java/org/wso2/carbon/identity/device/mgt/dao/DeviceManagementDAOImplTest.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/java/org/wso2/carbon/identity/device/mgt/service/DeviceManagementServiceImplTest.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/java/org/wso2/carbon/identity/device/mgt/util/DeviceManagementExceptionHandlerTest.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/resources/dbscripts/h2.sqlcomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/resources/repository/conf/carbon.xmlcomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/resources/repository/conf/identity/identity.xmlcomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/resources/testng.xmlcomponents/device-mgt/pom.xmlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/db2.sqlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/h2.sqlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/mssql.sqlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/mysql-cluster.sqlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/mysql.sqlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/oracle.sqlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/oracle_rac.sqlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/postgresql.sqlpom.xml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8202 +/- ##
============================================
+ Coverage 52.81% 53.54% +0.72%
+ Complexity 21555 21319 -236
============================================
Files 2199 2199
Lines 134728 130251 -4477
Branches 20723 19878 -845
============================================
- Hits 71154 69740 -1414
+ Misses 54776 52016 -2760
+ Partials 8798 8495 -303
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The caller supplied page size was passed to the database unchanged, so a large limit could load an unbounded number of devices into memory - the same problem the removal of getAllDevices was meant to avoid. Validate the limit at the service boundary against IdentityUtil.getMaximumItemPerPage(), capping an oversized page size to the configured maximum, and rejecting a negative page size as a client error instead of silently returning an empty list.
🚨 New Database Tables DetectedThis PR introduces new database table(s). Please ensure that the corresponding resource cleanup scripts are updated accordingly. New Tables:
Required Actions:
Database Support Checklist:Please verify your new table(s) are compatible with all supported databases:
This is an automated message. If this is a false positive or if you have questions, please reach out to the maintainers. |
🚨 New Database Tables DetectedThis PR introduces new database table(s). Please ensure that the corresponding resource cleanup scripts are updated accordingly. New Tables:
Required Actions:
Database Support Checklist:Please verify your new table(s) are compatible with all supported databases:
This is an automated message. If this is a false positive or if you have questions, please reach out to the maintainers. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/service/impl/DeviceManagementServiceImpl.java (1)
69-72: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAvoid logging
userIdin the debug statement; usedeviceIdfor consistency and PII safety.The debug log at line 70 includes
device.getUserId(), which may be an email or username depending on the WSO2 deployment. The LOG_ENHANCEMENT_GUIDELINES explicitly prohibit logging emails/PII. All other debug logs in this class (activate, deactivate, delete) use onlydeviceIdandtenantDomain, and the audit logger already masks the owner. ThedeviceIdis already present on line 71, so removing theuserIdportion aligns with both the PII guideline and the established pattern.🔒️ Proposed fix
if (LOG.isDebugEnabled()) { - LOG.debug("Device persisted for user: " + device.getUserId() + - " in tenant: " + tenantDomain + " with device ID: " + device.getId()); + LOG.debug("Device persisted with ID: " + device.getId() + + " in tenant: " + tenantDomain); }🤖 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/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/service/impl/DeviceManagementServiceImpl.java` around lines 69 - 72, Update the debug log in the device persistence flow to remove device.getUserId() and retain only the non-PII deviceId and tenantDomain details, matching the existing activate, deactivate, and delete logging pattern.Source: Path instructions
features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/oracle.sql (1)
3341-3364: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEnforce unique device public keys
The DAO/service only validate thatpublicKeyis present; duplicate keys can still be stored. Add a unique constraint/index onPUBLIC_KEY(or(PUBLIC_KEY, TENANT_ID)if uniqueness is tenant-scoped) in every supported DB script.🤖 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 `@features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/oracle.sql` around lines 3341 - 3364, Add a uniqueness constraint or unique index for IDN_DEVICE.PUBLIC_KEY in this Oracle schema, using the tenant-scoped key (PUBLIC_KEY, TENANT_ID) if device keys are intended to be unique per tenant; apply the equivalent change consistently across every supported database script.Source: Learnings
🧹 Nitpick comments (2)
components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/service/impl/DeviceManagementServiceImpl.java (2)
75-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd debug-level entry logs to read methods that perform DB calls.
getDeviceById,getDevicesByUserId,getDevices, andgetDeviceCountall perform database lookups but have no log statements. Per the logging guidelines, significant operations (DB calls) should include log statements. Consider adding guarded debug logs at entry to aid troubleshooting, consistent with the pattern used by the mutation methods.💡 Example for getDeviceById
public Device getDeviceById(String deviceId, String tenantDomain) throws DeviceMgtException { + if (LOG.isDebugEnabled()) { + LOG.debug("Retrieving device with ID: " + deviceId + " in tenant: " + tenantDomain); + } validateRequiredField(deviceId, "deviceId"); return deviceManagementDAO.getDeviceById( deviceId, IdentityTenantUtil.getTenantId(tenantDomain)); }🤖 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/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/service/impl/DeviceManagementServiceImpl.java` around lines 75 - 104, Add guarded debug-level entry logs to getDeviceById, getDevicesByUserId, getDevices, and getDeviceCount before their DAO calls, following the existing logging pattern used by mutation methods. Include relevant method inputs while avoiding sensitive data, and preserve the current validation and database-call behavior.Source: Path instructions
107-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a debug log to
updateDeviceNamefor consistency with other mutation methods.
persistDevice,activateDevice,deactivateDevice, anddeleteDeviceall emit guarded debug logs after their DAO operations.updateDeviceNameis the only mutation method that omits one, despite performing a significant DB update. Adding a debug log keeps the observability surface consistent across all write paths.💡 Proposed addition
AUDIT_LOGGER.printAuditLog(DeviceManagementAuditLogger.Operation.UPDATE, updated); + if (LOG.isDebugEnabled()) { + LOG.debug("Device name updated for ID: " + deviceId + " in tenant: " + tenantDomain); + } return updated;🤖 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/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/service/impl/DeviceManagementServiceImpl.java` around lines 107 - 124, Add a guarded debug log in updateDeviceName immediately after the successful deviceManagementDAO.updateDeviceName operation, consistent with the existing post-DAO debug logging in persistDevice, activateDevice, deactivateDevice, and deleteDevice. Include the relevant device update context without changing validation, error handling, audit logging, or return behavior.Source: Path instructions
🤖 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.
Outside diff comments:
In
`@components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/service/impl/DeviceManagementServiceImpl.java`:
- Around line 69-72: Update the debug log in the device persistence flow to
remove device.getUserId() and retain only the non-PII deviceId and tenantDomain
details, matching the existing activate, deactivate, and delete logging pattern.
In
`@features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/oracle.sql`:
- Around line 3341-3364: Add a uniqueness constraint or unique index for
IDN_DEVICE.PUBLIC_KEY in this Oracle schema, using the tenant-scoped key
(PUBLIC_KEY, TENANT_ID) if device keys are intended to be unique per tenant;
apply the equivalent change consistently across every supported database script.
---
Nitpick comments:
In
`@components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/service/impl/DeviceManagementServiceImpl.java`:
- Around line 75-104: Add guarded debug-level entry logs to getDeviceById,
getDevicesByUserId, getDevices, and getDeviceCount before their DAO calls,
following the existing logging pattern used by mutation methods. Include
relevant method inputs while avoiding sensitive data, and preserve the current
validation and database-call behavior.
- Around line 107-124: Add a guarded debug log in updateDeviceName immediately
after the successful deviceManagementDAO.updateDeviceName operation, consistent
with the existing post-DAO debug logging in persistDevice, activateDevice,
deactivateDevice, and deleteDevice. Include the relevant device update context
without changing validation, error handling, audit logging, or return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6ac18333-2ea0-4923-bb48-3d611ad40fc5
📒 Files selected for processing (12)
components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/api/constant/ErrorMessage.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/service/impl/DeviceManagementServiceImpl.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/java/org/wso2/carbon/identity/device/mgt/service/DeviceManagementServiceImplTest.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/resources/dbscripts/h2.sqlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/db2.sqlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/h2.sqlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/mssql.sqlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/mysql-cluster.sqlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/mysql.sqlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/oracle.sqlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/oracle_rac.sqlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/postgresql.sql
🚧 Files skipped from review as they are similar to previous changes (7)
- components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/api/constant/ErrorMessage.java
- components/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/resources/dbscripts/h2.sql
- features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/mssql.sql
- features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/h2.sql
- features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/mysql.sql
- features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/db2.sql
- components/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/java/org/wso2/carbon/identity/device/mgt/service/DeviceManagementServiceImplTest.java
🚨 New Database Tables DetectedThis PR introduces new database table(s). Please ensure that the corresponding resource cleanup scripts are updated accordingly. New Tables:
Required Actions:
Database Support Checklist:Please verify your new table(s) are compatible with all supported databases:
This is an automated message. If this is a false positive or if you have questions, please reach out to the maintainers. |
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/constant/DeviceMgtSQLConstants.java`:
- Around line 144-180: Update GET_ALL_DEVICES_PAGINATED_BY_USER and its MSSQL,
ORACLE, and DB2 variants to order tied REGISTERED_AT values deterministically
with D.ID DESC after the existing timestamp ordering. In
GET_ALL_DEVICES_PAGINATED_BY_USER_ORACLE and
GET_ALL_DEVICES_PAGINATED_BY_USER_DB2, add final outer ordering by rnum and rn
respectively, preserving newest-first pagination across all dialects.
In
`@components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/DeviceManagementDAOImpl.java`:
- Line 201: In DeviceManagementDAOImpl, update getDevices at
components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/DeviceManagementDAOImpl.java:201-201
to conditionally emit a debug log with the retrieved device count before
returning, and update getDeviceCount at the same file:234-234 with the
equivalent guarded count log. Preserve the existing return behavior and avoid
logging when debug logging is disabled.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1c87ad13-be81-44c8-b755-78b94b20f5d2
📒 Files selected for processing (10)
components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/api/service/DeviceManagementService.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/constant/DeviceMgtSQLConstants.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/DeviceManagementDAO.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/CacheBackedDeviceManagementDAO.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/DeviceManagementDAOImpl.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/service/impl/DeviceManagementServiceImpl.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/util/DeviceManagementAuditLogger.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/java/org/wso2/carbon/identity/device/mgt/dao/CacheBackedDeviceManagementDAOTest.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/java/org/wso2/carbon/identity/device/mgt/dao/DeviceManagementDAOImplTest.javacomponents/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/java/org/wso2/carbon/identity/device/mgt/service/DeviceManagementServiceImplTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- components/device-mgt/org.wso2.carbon.identity.device.mgt/src/test/java/org/wso2/carbon/identity/device/mgt/dao/CacheBackedDeviceManagementDAOTest.java
- components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/CacheBackedDeviceManagementDAO.java
- components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/util/DeviceManagementAuditLogger.java
- components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/service/impl/DeviceManagementServiceImpl.java
Comment on lines
+144
to
+180
| public static final String GET_ALL_DEVICES_PAGINATED_BY_USER = | ||
| "SELECT D.ID, UD.USER_ID, D.DEVICE_NAME, D.DEVICE_MODEL, D.PUBLIC_KEY, D.STATUS, " + | ||
| "D.REGISTERED_AT, D.METADATA, D.TENANT_ID " + | ||
| "FROM IDN_DEVICE D " + | ||
| "INNER JOIN IDN_USER_DEVICE UD ON D.ID = UD.DEVICE_ID AND D.TENANT_ID = UD.TENANT_ID " + | ||
| "WHERE D.TENANT_ID = :TENANT_ID; AND UD.USER_ID = :USER_ID; " + | ||
| "ORDER BY D.REGISTERED_AT DESC LIMIT :LIMIT; OFFSET :OFFSET;"; | ||
|
|
||
| // MS SQL Server. | ||
| public static final String GET_ALL_DEVICES_PAGINATED_BY_USER_MSSQL = | ||
| "SELECT D.ID, UD.USER_ID, D.DEVICE_NAME, D.DEVICE_MODEL, D.PUBLIC_KEY, D.STATUS, " + | ||
| "D.REGISTERED_AT, D.METADATA, D.TENANT_ID " + | ||
| "FROM IDN_DEVICE D " + | ||
| "INNER JOIN IDN_USER_DEVICE UD ON D.ID = UD.DEVICE_ID AND D.TENANT_ID = UD.TENANT_ID " + | ||
| "WHERE D.TENANT_ID = :TENANT_ID; AND UD.USER_ID = :USER_ID; " + | ||
| "ORDER BY D.REGISTERED_AT DESC OFFSET :OFFSET; ROWS FETCH NEXT :LIMIT; ROWS ONLY"; | ||
|
|
||
| // Oracle. | ||
| public static final String GET_ALL_DEVICES_PAGINATED_BY_USER_ORACLE = | ||
| "SELECT ID, USER_ID, DEVICE_NAME, DEVICE_MODEL, PUBLIC_KEY, STATUS, REGISTERED_AT, " + | ||
| "METADATA, TENANT_ID FROM (SELECT ID, USER_ID, DEVICE_NAME, DEVICE_MODEL, PUBLIC_KEY, " + | ||
| "STATUS, REGISTERED_AT, METADATA, TENANT_ID, rownum AS rnum FROM " + | ||
| "(SELECT D.ID, UD.USER_ID, D.DEVICE_NAME, D.DEVICE_MODEL, D.PUBLIC_KEY, D.STATUS, " + | ||
| "D.REGISTERED_AT, D.METADATA, D.TENANT_ID FROM IDN_DEVICE D " + | ||
| "INNER JOIN IDN_USER_DEVICE UD ON D.ID = UD.DEVICE_ID AND D.TENANT_ID = UD.TENANT_ID " + | ||
| "WHERE D.TENANT_ID = :TENANT_ID; AND UD.USER_ID = :USER_ID; ORDER BY D.REGISTERED_AT DESC) " + | ||
| "WHERE rownum <= :UPPER_BOUND;) WHERE rnum > :OFFSET;"; | ||
|
|
||
| // DB2. | ||
| public static final String GET_ALL_DEVICES_PAGINATED_BY_USER_DB2 = | ||
| "SELECT ID, USER_ID, DEVICE_NAME, DEVICE_MODEL, PUBLIC_KEY, STATUS, REGISTERED_AT, " + | ||
| "METADATA, TENANT_ID FROM (SELECT ROW_NUMBER() OVER(ORDER BY D.REGISTERED_AT DESC) AS rn, " + | ||
| "D.ID, UD.USER_ID, D.DEVICE_NAME, D.DEVICE_MODEL, D.PUBLIC_KEY, D.STATUS, " + | ||
| "D.REGISTERED_AT, D.METADATA, D.TENANT_ID FROM IDN_DEVICE D " + | ||
| "INNER JOIN IDN_USER_DEVICE UD ON D.ID = UD.DEVICE_ID AND D.TENANT_ID = UD.TENANT_ID " + | ||
| "WHERE D.TENANT_ID = :TENANT_ID; AND UD.USER_ID = :USER_ID;) " + | ||
| "WHERE rn BETWEEN :LOWER_BOUND; AND :UPPER_BOUND;"; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make pagination ordering deterministic across all dialects.
Ordering only by REGISTERED_AT can duplicate or omit records between pages when timestamps tie. Add a stable tiebreaker such as D.ID DESC; Oracle and DB2 also need final outer ordering by rnum/rn to guarantee the documented newest-first result order.
🤖 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/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/constant/DeviceMgtSQLConstants.java`
around lines 144 - 180, Update GET_ALL_DEVICES_PAGINATED_BY_USER and its MSSQL,
ORACLE, and DB2 variants to order tied REGISTERED_AT values deterministically
with D.ID DESC after the existing timestamp ordering. In
GET_ALL_DEVICES_PAGINATED_BY_USER_ORACLE and
GET_ALL_DEVICES_PAGINATED_BY_USER_DB2, add final outer ordering by rnum and rn
respectively, preserving newest-first pagination across all dialects.
| } | ||
| bindPaginationParams(preparedStatement, style, safeOffset, limit); | ||
| })); | ||
| return devices != null ? devices : Collections.emptyList(); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add debug logging for database retrieval operations.
As per path instructions, functions that perform significant operations such as database calls should include log statements where possible. Adding a debug log for the successfully retrieved data improves observability without spamming the logs.
components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/DeviceManagementDAOImpl.java#L201-L201: Guard and log the count of retrieved devices before returning.components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/DeviceManagementDAOImpl.java#L234-L234: Guard and log the retrieved device count before returning.
📝 Proposed fixes
For getDevices (around line 201):
- return devices != null ? devices : Collections.emptyList();
+ List<Device> result = devices != null ? devices : Collections.emptyList();
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Retrieved " + result.size() + " devices for tenant ID: " + tenantId);
+ }
+ return result;For getDeviceCount (around line 234):
- return count != null ? count : 0;
+ int result = count != null ? count : 0;
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Retrieved device count: " + result + " for tenant ID: " + tenantId);
+ }
+ return result;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return devices != null ? devices : Collections.emptyList(); | |
| List<Device> result = devices != null ? devices : Collections.emptyList(); | |
| if (LOG.isDebugEnabled()) { | |
| LOG.debug("Retrieved " + result.size() + " devices for tenant ID: " + tenantId); | |
| } | |
| return result; |
Suggested change
| return devices != null ? devices : Collections.emptyList(); | |
| int result = count != null ? count : 0; | |
| if (LOG.isDebugEnabled()) { | |
| LOG.debug("Retrieved device count: " + result + " for tenant ID: " + tenantId); | |
| } | |
| return result; |
📍 Affects 1 file
components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/DeviceManagementDAOImpl.java#L201-L201(this comment)components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/DeviceManagementDAOImpl.java#L234-L234
🤖 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/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/DeviceManagementDAOImpl.java`
at line 201, In DeviceManagementDAOImpl, update getDevices at
components/device-mgt/org.wso2.carbon.identity.device.mgt/src/main/java/org/wso2/carbon/identity/device/mgt/internal/dao/impl/DeviceManagementDAOImpl.java:201-201
to conditionally emit a debug log with the retrieved device count before
returning, and update getDeviceCount at the same file:234-234 with the
equivalent guarded count log. Preserve the existing return behavior and avoid
logging when debug logging is disabled.
Source: Path instructions
🚨 New Database Tables DetectedThis PR introduces new database table(s). Please ensure that the corresponding resource cleanup scripts are updated accordingly. New Tables:
Required Actions:
Database Support Checklist:Please verify your new table(s) are compatible with all supported databases:
This is an automated message. If this is a false positive or if you have questions, please reach out to the maintainers. |
1 task
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.
Proposed changes in this pull request
This PR introduces a new component,
org.wso2.carbon.identity.device.mgt, which provides a generic registry for user-owned devices. A device is identified by the public key of a key pair whose private key never leaves the device, so a device can later prove its identity by signing a challenge or a token. The component owns the storage, retrieval, lifecycle and auditing of these device records.This is the base component. It intentionally contains only device persistence and lifecycle management —
the registration handshake (challenge generation and signature verification) and device policy evaluation are
separate components that consume this one and land in follow-up PRs.
The change adds a new aggregator module (
components/device-mgt), registers it in the root POM, and adds thedevice tables to the database scripts.
Database
Two tables are added to all supported database scripts (DB2, H2, MS SQL, MySQL, MySQL Cluster, Oracle,
Oracle RAC, PostgreSQL):
IDN_DEVICE— the device record: id, name, model, public key, status, registration time, metadata andtenant.
IDN_USER_DEVICE— the device-to-user mapping, so a user may own multiple devices.IDX_IDV_STATUSandIDX_IUD_USER_TENANT.The component provides
DeviceManagementService— the OSGi service contract:persistDevice(...)— store a verified device.getDeviceById(...),getDevicesByUserId(...)— device lookup.getDevices(tenantDomain, offset, limit)andgetDeviceCount(...)— paginated tenant-wide listing forthe REST API layer.
updateDeviceName(...)— rename a device.activateDevice(...)/deactivateDevice(...)— device lifecycle transitions.deleteDevice(...)— remove a device registration.pagination dialects (default
LIMIT/OFFSET, and variants for MS SQL, Oracle and DB2).CacheBackedDeviceManagementDAOdecorates the DAO and cachesDeviceobjects by deviceid, following the same pattern as
CacheBackedRuleManagementDAOin the rule management component. Lookup byid is the hot path (it is hit on every device-token validation), so it is read-through cached; the cache entry is invalidated on name update, status change and delete. Aggregate/list reads are not cached.
is masked and the public key is reduced to a non-reversible fingerprint — the raw public key and the free-form metadata are never written to the audit log. Read operations are intentionally not audited.
DeviceMgtExceptionwith client/server variants) and anErrorMessagecatalog, so callers get stable error codes rather than leaked lower-layer exceptions.When should this PR be merged
No preconditions. The component is self-contained
Follow up actions
N/A