Introduce the device registration component#8207
Open
kaviska wants to merge 4 commits into
Open
Conversation
Introduce org.wso2.carbon.identity.device.registration, which implements the device registration challenge-response protocol and the registration flow executor. Wire the module into the build via a new components/device-mgt aggregator and the root pom. The module depends on org.wso2.carbon.identity.device.policy, org.wso2.carbon.identity.policy.management and org.wso2.carbon.identity.policy.evaluation, which are not yet on master, so it will compile once those modules land.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughChangesAdds a device registration bundle with challenge-response verification, optional device policy evaluation, deferred persistence through a flow listener, OSGi service wiring, Maven integration, and TestNG and runtime configuration. Device registration
🚥 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 |
|
![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: 5
🧹 Nitpick comments (2)
components/device-mgt/org.wso2.carbon.identity.device.registration/src/test/java/org/wso2/carbon/identity/device/registration/executor/DeviceRegistrationExecutorTest.java (2)
115-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public TestNG lifecycle methods.
setUpClass,tearDownClass,setUp, andtearDownMethodare public but undocumented. The repository’s test exemption applies to methods annotated with@Test, not lifecycle methods.As per coding guidelines, “All public methods should have a docstring.” Based on learnings, public test methods annotated with
Testcan be exempt from Javadoc requirements.🤖 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.registration/src/test/java/org/wso2/carbon/identity/device/registration/executor/DeviceRegistrationExecutorTest.java` around lines 115 - 174, Add Javadoc documentation to the public TestNG lifecycle methods setUpClass, tearDownClass, setUp, and tearDownMethod in DeviceRegistrationExecutorTest. Keep the documentation concise and describe each method’s setup or cleanup responsibility without changing lifecycle behavior.Sources: Coding guidelines, Learnings
561-566: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMatch the security-critical verification inputs explicitly.
Using
any()for every argument allows these tests to pass if the executor forwards the wrong registration ID, challenge, public key, or signature.Proposed tightening
mocked.when(() -> DeviceRegistrationHandler.verify( - any(), any(), any(), any(), any(), any(), any())) + eq(REGISTRATION_ID), + eq(CHALLENGE), + eq("base64PublicKey"), + eq("base64Signature"), + any(), any(), any())) .thenReturn(device);🤖 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.registration/src/test/java/org/wso2/carbon/identity/device/registration/executor/DeviceRegistrationExecutorTest.java` around lines 561 - 566, Update mockVerifySuccess to stub DeviceRegistrationHandler.verify with the exact verification inputs expected by each test—especially registration ID, challenge, public key, and signature—instead of any() matchers. Pass or expose those expected values through the helper as needed while preserving the returned VerifiedDevice.
🤖 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.registration/src/main/java/org/wso2/carbon/identity/device/registration/executor/DeviceRegistrationExecutor.java`:
- Around line 221-239: Update the failure handling around the challenge removal
in DeviceRegistrationExecutor so policy validation failures do not leave a
registration context with its challenge absent. When device-data validation or
evaluatePolicy returns an error, clear the remaining registration context and
issue a new challenge before returning, while preserving successful single-use
challenge consumption.
- Around line 141-144: Remove the setUsername call on the thread-local Carbon
context in the DeviceRegistrationExecutor flow before
DeviceRegistrationHandler.initiate; pass the existing username directly as
already done, without modifying pooled-thread context state.
In
`@components/device-mgt/org.wso2.carbon.identity.device.registration/src/main/java/org/wso2/carbon/identity/device/registration/internal/handler/DeviceRegistrationHandler.java`:
- Around line 75-79: Update the debug log in DeviceRegistrationHandler to remove
the raw username from the “Device registration initiated” message. Retain only
the tenantDomain and registrationId, or apply the project’s established
username-masking utility if one exists.
In
`@components/device-mgt/org.wso2.carbon.identity.device.registration/src/main/java/org/wso2/carbon/identity/device/registration/model/VerifiedDevice.java`:
- Around line 48-55: Update the VerifiedDevice constructor to defensively copy
builder.registeredAt rather than storing the mutable Timestamp reference
directly. Also ensure the class’s getter and any reuse or builder paths return
or retain independent Timestamp copies, preserving the registration time from
external mutation.
In
`@components/device-mgt/org.wso2.carbon.identity.device.registration/src/test/java/org/wso2/carbon/identity/device/registration/executor/DeviceRegistrationExecutorTest.java`:
- Around line 413-435: Extend
testExecuteCompletionPolicyConfiguredButNoDeviceDataReturnsDeviceDataRequiredError
to retry the same flow with FIELD_DEVICE_DATA restored and assert successful
completion. In DeviceRegistrationExecutor.execute, preserve CTX_CHALLENGE when
FIELD_DEVICE_DATA is missing or policy validation fails, and remove it only
after successful device-data and policy validation so the corrected retry can
verify successfully.
---
Nitpick comments:
In
`@components/device-mgt/org.wso2.carbon.identity.device.registration/src/test/java/org/wso2/carbon/identity/device/registration/executor/DeviceRegistrationExecutorTest.java`:
- Around line 115-174: Add Javadoc documentation to the public TestNG lifecycle
methods setUpClass, tearDownClass, setUp, and tearDownMethod in
DeviceRegistrationExecutorTest. Keep the documentation concise and describe each
method’s setup or cleanup responsibility without changing lifecycle behavior.
- Around line 561-566: Update mockVerifySuccess to stub
DeviceRegistrationHandler.verify with the exact verification inputs expected by
each test—especially registration ID, challenge, public key, and
signature—instead of any() matchers. Pass or expose those expected values
through the helper as needed while preserving the returned VerifiedDevice.
🪄 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: 355fafe9-dabc-477c-aded-7f462e5680d4
📒 Files selected for processing (24)
components/device-mgt/org.wso2.carbon.identity.device.registration/pom.xmlcomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/main/java/org/wso2/carbon/identity/device/registration/executor/DeviceRegistrationExecutor.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/main/java/org/wso2/carbon/identity/device/registration/internal/component/DeviceRegistrationComponentServiceHolder.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/main/java/org/wso2/carbon/identity/device/registration/internal/component/DeviceRegistrationServiceComponent.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/main/java/org/wso2/carbon/identity/device/registration/internal/constant/DeviceRegistrationConstants.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/main/java/org/wso2/carbon/identity/device/registration/internal/constant/ErrorMessage.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/main/java/org/wso2/carbon/identity/device/registration/internal/exception/DeviceRegistrationException.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/main/java/org/wso2/carbon/identity/device/registration/internal/handler/DeviceRegistrationHandler.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/main/java/org/wso2/carbon/identity/device/registration/internal/model/DeviceRegistrationChallenge.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/main/java/org/wso2/carbon/identity/device/registration/internal/util/DeviceRegistrationDiagnosticLogger.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/main/java/org/wso2/carbon/identity/device/registration/internal/util/DeviceRegistrationExceptionHandler.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/main/java/org/wso2/carbon/identity/device/registration/internal/util/DeviceSignatureVerifier.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/main/java/org/wso2/carbon/identity/device/registration/listener/RegistrationFlowCompletionListener.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/main/java/org/wso2/carbon/identity/device/registration/model/VerifiedDevice.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/test/java/org/wso2/carbon/identity/device/registration/executor/DeviceRegistrationExecutorTest.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/test/java/org/wso2/carbon/identity/device/registration/internal/handler/DeviceRegistrationHandlerTest.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/test/java/org/wso2/carbon/identity/device/registration/internal/util/DeviceRegistrationExceptionHandlerTest.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/test/java/org/wso2/carbon/identity/device/registration/internal/util/DeviceSignatureVerifierTest.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/test/java/org/wso2/carbon/identity/device/registration/listener/RegistrationFlowCompletionListenerTest.javacomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/test/resources/repository/conf/carbon.xmlcomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/test/resources/repository/conf/identity/identity.xmlcomponents/device-mgt/org.wso2.carbon.identity.device.registration/src/test/resources/testng.xmlcomponents/device-mgt/pom.xmlpom.xml
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 the
org.wso2.carbon.identity.device.registrationcomponent, which handles device registration as a flow executor. It implements a two-phase ECDSA challenge-response handshake to verify that a device owns the private key corresponding to the public key it is registering.The registration flow works as follows:
registrationId, then requests the device'spublicKeyandsignature.policyNameis configured, it also evaluates the device against the configured device policy.STATUS_COMPLETE, the verified device is associated with the authenticated user and persisted throughDeviceManagementService.persistDevice(...).Persistence is intentionally handled outside the executor. During flows such as user registration, the user record may not exist when the device registration step executes, so the device cannot be persisted until the flow completes successfully.
This component includes
DeviceRegistrationExecutor– Implements the two-phase registration protocol as anExecutorOSGi service. It also supports an optionalpolicyNameexecutor metadata property to enforce device policy validation during registration.RegistrationFlowCompletionListener– Persists the verified device after successful flow completion, regardless of flow type. Since persistence happens only after the flow completes successfully, there is no need for rollback logic if a later step fails.VerifiedDevice– Represents a device whose ownership has been verified but has not yet been associated with a user. It can only be converted into a persistableDevicethroughbindTo(userId), ensuring that only user-bound devices can be stored.DeviceRegistrationException– A module-specific checked exception withCLIENTandSERVERerror types, following the same pattern used by other identity framework components.FlowExecutionContextfor the lifetime of the handshake. After a successful verification, the challenge is removed to prevent reuse. If verification fails, it is retained so the client can retry.Usage
DeviceRegistrationExecutoris registered as anExecutorOSGi service. The flow engine discovers it automatically and makes it available to any flow definition that referencesDeviceRegistrationExecutorin anEXECUTIONstep.Integration with other components
This component builds on existing device management and policy components rather than duplicating their responsibilities.
device.mgt– Responsible for device persistence and lifecycle management. This component persists devices throughDeviceManagementService.persistDevice(...).device.policy– Responsible for device compliance evaluation. When apolicyNameis configured, the executor invokesDevicePolicyEvaluator.evaluate(...)before the device is persisted.Testing
Unit tests cover:
When should this PR be merged
This component depends on:
org.wso2.carbon.identity.device.mgtorg.wso2.carbon.identity.device.policyorg.wso2.carbon.identity.policy.managementorg.wso2.carbon.identity.policy.evaluationFollow up actions
N/A