Add push-device-mgt endpoints and preferences endpoint - #1138
Merged
Conversation
|
|
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:
📝 Walkthrough📝 Walkthrough🚥 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 |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+261
to
+264
| */ | ||
| public static DeviceHandlerService getDeviceHandlerService() { | ||
|
|
||
| return DeviceHandlerServiceHolder.SERVICE; |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| */ | |
| public static DeviceHandlerService getDeviceHandlerService() { | |
| return DeviceHandlerServiceHolder.SERVICE; | |
| public static DeviceHandlerService getDeviceHandlerService() { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Retrieving DeviceHandlerService from OSGi context"); | |
| } | |
| return DeviceHandlerServiceHolder.SERVICE; | |
| } |
Comment on lines
+271
to
+274
| */ | ||
| public static ConfigurationManager getConfigurationManager() { | ||
|
|
||
| return ConfigurationManagerHolder.SERVICE; |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| */ | |
| public static ConfigurationManager getConfigurationManager() { | |
| return ConfigurationManagerHolder.SERVICE; | |
| public static ConfigurationManager getConfigurationManager() { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Retrieving ConfigurationManager from OSGi context"); | |
| } | |
| return ConfigurationManagerHolder.SERVICE; | |
| } |
Comment on lines
+49
to
+59
| static { | ||
| Map<String, Map<String, Set<String>>> allowed = new HashMap<>(); | ||
|
|
||
| // Push authentication device management configuration. | ||
| Map<String, Set<String>> pushAuthResources = new HashMap<>(); | ||
| pushAuthResources.put(RESOURCE_NAME, | ||
| new HashSet<>(Arrays.asList(ATTR_ENABLE_DEVICE_MANAGEMENT, ATTR_MAX_DEVICE_LIMIT))); | ||
| allowed.put(RESOURCE_TYPE, Collections.unmodifiableMap(pushAuthResources)); | ||
|
|
||
| ALLOWED_CONFIG_PREFERENCES = Collections.unmodifiableMap(allowed); | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| static { | |
| Map<String, Map<String, Set<String>>> allowed = new HashMap<>(); | |
| // Push authentication device management configuration. | |
| Map<String, Set<String>> pushAuthResources = new HashMap<>(); | |
| pushAuthResources.put(RESOURCE_NAME, | |
| new HashSet<>(Arrays.asList(ATTR_ENABLE_DEVICE_MANAGEMENT, ATTR_MAX_DEVICE_LIMIT))); | |
| allowed.put(RESOURCE_TYPE, Collections.unmodifiableMap(pushAuthResources)); | |
| ALLOWED_CONFIG_PREFERENCES = Collections.unmodifiableMap(allowed); | |
| } | |
| static { | |
| Map<String, Map<String, Set<String>>> allowed = new HashMap<>(); | |
| // Push authentication device management configuration. | |
| Map<String, Set<String>> pushAuthResources = new HashMap<>(); | |
| pushAuthResources.put(RESOURCE_NAME, | |
| new HashSet<>(Arrays.asList(ATTR_ENABLE_DEVICE_MANAGEMENT, ATTR_MAX_DEVICE_LIMIT))); | |
| allowed.put(RESOURCE_TYPE, Collections.unmodifiableMap(pushAuthResources)); | |
| ALLOWED_CONFIG_PREFERENCES = Collections.unmodifiableMap(allowed); | |
| log.info("Initialized allowed config preferences for resource types: " + allowed.keySet()); | |
| } |
Comment on lines
+2596
to
+2600
| public List<ConfigPreferenceResp> getConfigPreferences( | ||
| List<ConfigPreferenceSearchAttribute> searchAttributes) { | ||
|
|
||
| ConfigurationManager configurationManager = ConfigsServiceHolder.getConfigurationManager(); | ||
| List<ConfigPreferenceResp> responseList = new ArrayList<>(); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| public List<ConfigPreferenceResp> getConfigPreferences( | |
| List<ConfigPreferenceSearchAttribute> searchAttributes) { | |
| ConfigurationManager configurationManager = ConfigsServiceHolder.getConfigurationManager(); | |
| List<ConfigPreferenceResp> responseList = new ArrayList<>(); | |
| public List<ConfigPreferenceResp> getConfigPreferences( | |
| List<ConfigPreferenceSearchAttribute> searchAttributes) { | |
| log.info("Retrieving config preferences for " + searchAttributes.size() + " search attribute(s)"); | |
| ConfigurationManager configurationManager = ConfigsServiceHolder.getConfigurationManager(); |
Comment on lines
+2657
to
+2660
| + ", name: " + resourceName); | ||
| } | ||
| resource = configurationManager.getResource(resourceType, resourceName); | ||
| } catch (ConfigurationManagementException e) { |
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
Suggested change
| + ", name: " + resourceName); | |
| } | |
| resource = configurationManager.getResource(resourceType, resourceName); | |
| } catch (ConfigurationManagementException e) { | |
| resource = configurationManager.getResource(resourceType, resourceName); | |
| } catch (ConfigurationManagementException e) { | |
| log.error("Error occurred while retrieving config preference for resource type: " + resourceType + ", message: " + e.getMessage()); | |
| throw handleException(Response.Status.INTERNAL_SERVER_ERROR, |
| ConfigsServiceHolder.getOAuth2OIDCConfigOrgUsageScopeMgtService(); | ||
| CompatibilitySettingsService compatibilitySettingsService = ConfigsServiceHolder | ||
| .getIdentityCompatibilitySettingsService(); | ||
| DeviceHandlerService deviceHandlerService = ConfigsServiceHolder.getDeviceHandlerService(); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
Suggested change
| DeviceHandlerService deviceHandlerService = ConfigsServiceHolder.getDeviceHandlerService(); | |
| DeviceHandlerService deviceHandlerService = ConfigsServiceHolder.getDeviceHandlerService(); | |
| log.debug("Retrieved DeviceHandlerService from ConfigsServiceHolder"); |
Comment on lines
+94
to
96
| compatibilitySettingsService, | ||
| deviceHandlerService | ||
| ); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
Suggested change
| compatibilitySettingsService, | |
| deviceHandlerService | |
| ); | |
| compatibilitySettingsService, | |
| deviceHandlerService | |
| ); | |
| log.info("ServerConfigManagementService instance created successfully"); |
Comment on lines
+359
to
+363
| @Override | ||
| public Response getPushDeviceMgtConfigs() { | ||
|
|
||
| return Response.ok().entity(configManagementService.getPushDeviceMgtConfigs()).build(); | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 8
Suggested change
| @Override | |
| public Response getPushDeviceMgtConfigs() { | |
| return Response.ok().entity(configManagementService.getPushDeviceMgtConfigs()).build(); | |
| } | |
| @Override | |
| public Response getPushDeviceMgtConfigs() { | |
| log.info("Retrieving push device management configurations"); | |
| return Response.ok().entity(configManagementService.getPushDeviceMgtConfigs()).build(); | |
| } |
Comment on lines
+365
to
+369
| @Override | ||
| public Response updatePushDeviceMgtConfigs(PushDeviceMgtConfig pushDeviceMgtConfig) { | ||
|
|
||
| return Response.ok().entity(configManagementService.updatePushDeviceMgtConfigs(pushDeviceMgtConfig)).build(); | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 9
Suggested change
| @Override | |
| public Response updatePushDeviceMgtConfigs(PushDeviceMgtConfig pushDeviceMgtConfig) { | |
| return Response.ok().entity(configManagementService.updatePushDeviceMgtConfigs(pushDeviceMgtConfig)).build(); | |
| } | |
| @Override | |
| public Response updatePushDeviceMgtConfigs(PushDeviceMgtConfig pushDeviceMgtConfig) { | |
| log.info("Updating push device management configurations"); | |
| return Response.ok().entity(configManagementService.updatePushDeviceMgtConfigs(pushDeviceMgtConfig)).build(); | |
| } |
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.
raviendalpatadu
changed the title
raviendalpatadu
marked this pull request as ready for review
June 16, 2026 08:51
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/factories/ServerConfigManagementServiceFactory.java (1)
43-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing null check for pushDeviceHandlerService.
All other OSGi services in this factory have null checks that throw
IllegalStateExceptionto fail fast during initialization. The newly addedpushDeviceHandlerServicelacks this validation, which could result in a null reference being passed to the constructor and cause NPE when service methods are invoked.Proposed fix
DeviceHandlerService pushDeviceHandlerService = ConfigsServiceHolder.getPushDeviceHandlerService(); if (applicationManagementService == null) { throw new IllegalStateException("ApplicationManagementService is not available from OSGi context."); } // ... existing null checks ... if (compatibilitySettingsService == null) { throw new IllegalStateException("CompatibilitySettingsService is not available from OSGi context."); } + if (pushDeviceHandlerService == null) { + throw new IllegalStateException("DeviceHandlerService is not available from OSGi context."); + } + SERVICE = new ServerConfigManagementService(applicationManagementService, identityProviderManager,Based on learnings: factory classes should use eager static initialization blocks to fail fast if required OSGi services are not available, ensuring consistency across API service factories.
🤖 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.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/factories/ServerConfigManagementServiceFactory.java` around lines 43 - 96, The pushDeviceHandlerService variable obtained from ConfigsServiceHolder.getPushDeviceHandlerService() is missing the null validation that all other OSGi services in this factory have. Add a null check for pushDeviceHandlerService after it is retrieved, and throw an IllegalStateException with the message "PushDeviceHandlerService is not available from OSGi context." if it is null, following the same pattern used for applicationManagementService, identityProviderManager, corsManagementService, and all other services in this initialization block.Source: Learnings
🤖 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.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/Constants.java`:
- Around line 290-292: The constant
ERROR_CODE_CLIENT_ERROR_PUSH_DEVICE_MGT_CONFIG_UPDATE is currently assigned the
error code "60008", which is already in use by
ERROR_CODE_CLIENT_ERROR_ISSUER_USAGE_SCOPE_UPDATE, causing a code duplication
conflict. Replace the error code in
ERROR_CODE_CLIENT_ERROR_PUSH_DEVICE_MGT_CONFIG_UPDATE with a unique, unused
error code to ensure each error constant has a distinct identifier for proper
error contract handling.
---
Outside diff comments:
In
`@components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/factories/ServerConfigManagementServiceFactory.java`:
- Around line 43-96: The pushDeviceHandlerService variable obtained from
ConfigsServiceHolder.getPushDeviceHandlerService() is missing the null
validation that all other OSGi services in this factory have. Add a null check
for pushDeviceHandlerService after it is retrieved, and throw an
IllegalStateException with the message "PushDeviceHandlerService is not
available from OSGi context." if it is null, following the same pattern used for
applicationManagementService, identityProviderManager, corsManagementService,
and all other services in this initialization block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71547cbf-d5f0-4409-aca2-47f62b2ee346
⛔ Files ignored due to path filters (6)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/ConfigsApi.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/ConfigsApiService.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/ConfigAttribute.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/ConfigPreferenceRequestDTO.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/ConfigPreferenceResponseDTO.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/PushDeviceMgtConfig.javais excluded by!**/gen/**
📒 Files selected for processing (10)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/pom.xmlcomponents/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/ConfigPreferencesAllowlist.javacomponents/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/ConfigsServiceHolder.javacomponents/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/Constants.javacomponents/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/pom.xmlcomponents/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.javacomponents/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/factories/ServerConfigManagementServiceFactory.javacomponents/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/impl/ConfigsApiServiceImpl.javacomponents/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/resources/configs.yamlpom.xml
raviendalpatadu
force-pushed
the
preference/endpoint
branch
4 times, most recently
from
June 23, 2026 09:45
2af6691 to
5cbd827
Compare
raviendalpatadu
commented
Jun 23, 2026
raviendalpatadu
commented
Jul 15, 2026
| return entry.allowedAttributes.contains(attributeName); | ||
| } | ||
|
|
||
| private static AllowedConfigList findEntry(String resourceType, String resourceName) { |
Contributor
Author
There was a problem hiding this comment.
getAllowedConfigs
raviendalpatadu
commented
Jul 15, 2026
| Resource resource = resolveConfigResource(configurationManager, resourceType, resourceName); | ||
|
|
||
| return new ConfigPreferenceResponseDTO() | ||
| .resourceType(resourceType) |
raviendalpatadu
commented
Jul 15, 2026
| return result; | ||
| } | ||
|
|
||
| private static List<DeviceRegistrationNotificationChannel> |
Contributor
Author
There was a problem hiding this comment.
Suggested change
| private static List<DeviceRegistrationNotificationChannel> | |
| private static List<DeviceRegistrationNotificationChannel> buildDeviceRegistrationNotificationChannel( | |
| Set<DeviceRegistrationNotificationChannelEnum> channels) { |
raviendalpatadu
force-pushed
the
preference/endpoint
branch
from
July 16, 2026 05:28
9f1f163 to
ccf5897
Compare
Add config preferences API and push device management configuration support, and refactor device registration notification channels to use an enum model.
raviendalpatadu
force-pushed
the
preference/endpoint
branch
from
July 16, 2026 12:01
ccf5897 to
ab9b171
Compare
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
approved these changes
Jul 16, 2026
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/29502659871
ZiyamSanthosh
approved these changes
Jul 17, 2026
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
4 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.
This pull request introduces new API endpoints and supporting infrastructure for managing push device management configurations and retrieving configuration preferences in the WSO2 Identity Server. It adds new dependencies, expands the service holder and constants, and defines new models and error codes to support these features.
Key changes include:
API Additions and Enhancements:
ConfigsApifor:/configs/preferences/configs/push-device-mgt/configs/push-device-mgtConfigsApiServiceinterface to include corresponding service methods for the new endpointsDependency Management:
org.wso2.carbon.identity.notification.push.device.handlerandorg.wso2.carbon.identity.configuration.mgt.corein relevantpom.xmlfiles to support device management and configuration management featuresService Holder and Constants:
ConfigsServiceHolderto provide access toDeviceHandlerServiceandConfigurationManagerOSGi services, allowing the new endpoints to interact with push device and configuration management logicALLOWED_CONFIG_PREFERENCESinConstants.javato define a whitelist for configuration preferences that can be accessed via the new endpoint, specifically allowing certain push authentication device management attributesError Handling:
Constants.javafor scenarios related to configuration preferences and push device management configuration retrieval and updatesModel Definitions:
ConfigAttributeto represent name-value pairs for configuration attributes, supporting the new API's data structuresThese changes collectively enable secure and extensible management of push device configurations and configuration preferences through the API, with proper error handling and clear boundaries on what configurations can be accessed or modified.
Related Issues