Add preferences API resource with POST method#8150
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR introduces a ResourceIdentifier-based default configuration resolver framework that allows components to provide default configurations for resource types. The framework registers and retrieves resolvers through OSGi dynamic references and thread-safe storage. A new access control rule protects the server preferences endpoint, and push device management configuration limits and email templates are added to feature defaults. ChangesServer Preferences Endpoint Protection
Default Configuration Resolver Framework
Push Device Management Configuration
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8150 +/- ##
============================================
+ Coverage 52.60% 52.91% +0.30%
+ Complexity 21135 20999 -136
============================================
Files 2197 2198 +1
Lines 130930 129703 -1227
Branches 19453 19337 -116
============================================
- Hits 68875 68631 -244
+ Misses 53635 52694 -941
+ Partials 8420 8378 -42
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:
|
| @Override | ||
| public Resource getDefaultResource(String resourceType, String resourceName) | ||
| throws ConfigurationManagementException { |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
| @Override | |
| public Resource getDefaultResource(String resourceType, String resourceName) | |
| throws ConfigurationManagementException { | |
| @Override | |
| public Resource getDefaultResource(String resourceType, String resourceName) | |
| throws ConfigurationManagementException { | |
| log.info("Retrieving default resource for type: " + resourceType + ", name: " + resourceName); |
| } | ||
| return resolver.getDefaultConfigs(resourceType, resourceName); | ||
| } | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
| } | |
| return resolver.getDefaultConfigs(resourceType, resourceName); | |
| } | |
| } | |
| } | |
| return resolver.getDefaultConfigs(resourceType, resourceName); | |
| } | |
| } | |
| log.warn("No resolver found to handle resource type: " + resourceType + ", name: " + resourceName); |
| protected void setDefaultConfigResolver(DefaultConfigResolver defaultConfigResolver) { | ||
|
|
||
| if (log.isDebugEnabled()) { | ||
| log.debug("DefaultConfigResolver registered: " + defaultConfigResolver.getClass().getName()); | ||
| } | ||
| ConfigurationManagerComponentDataHolder.getInstance().addDefaultConfigResolver(defaultConfigResolver); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
| protected void setDefaultConfigResolver(DefaultConfigResolver defaultConfigResolver) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("DefaultConfigResolver registered: " + defaultConfigResolver.getClass().getName()); | |
| } | |
| ConfigurationManagerComponentDataHolder.getInstance().addDefaultConfigResolver(defaultConfigResolver); | |
| protected void setDefaultConfigResolver(DefaultConfigResolver defaultConfigResolver) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("DefaultConfigResolver registered: " + defaultConfigResolver.getClass().getName()); | |
| } | |
| log.info("Registering DefaultConfigResolver: " + defaultConfigResolver.getClass().getSimpleName()); | |
| ConfigurationManagerComponentDataHolder.getInstance().addDefaultConfigResolver(defaultConfigResolver); |
| protected void unsetDefaultConfigResolver(DefaultConfigResolver defaultConfigResolver) { | ||
|
|
||
| if (log.isDebugEnabled()) { | ||
| log.debug("DefaultConfigResolver unregistered: " + defaultConfigResolver.getClass().getName()); | ||
| } | ||
| ConfigurationManagerComponentDataHolder.getInstance().removeDefaultConfigResolver(defaultConfigResolver); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
| protected void unsetDefaultConfigResolver(DefaultConfigResolver defaultConfigResolver) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("DefaultConfigResolver unregistered: " + defaultConfigResolver.getClass().getName()); | |
| } | |
| ConfigurationManagerComponentDataHolder.getInstance().removeDefaultConfigResolver(defaultConfigResolver); | |
| protected void unsetDefaultConfigResolver(DefaultConfigResolver defaultConfigResolver) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("DefaultConfigResolver unregistered: " + defaultConfigResolver.getClass().getName()); | |
| } | |
| log.info("Unregistering DefaultConfigResolver: " + defaultConfigResolver.getClass().getSimpleName()); | |
| ConfigurationManagerComponentDataHolder.getInstance().removeDefaultConfigResolver(defaultConfigResolver); |
| */ | ||
| public void addDefaultConfigResolver(DefaultConfigResolver resolver) { | ||
|
|
||
| defaultConfigResolvers.add(resolver); | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
| */ | |
| public void addDefaultConfigResolver(DefaultConfigResolver resolver) { | |
| defaultConfigResolvers.add(resolver); | |
| } | |
| public void addDefaultConfigResolver(DefaultConfigResolver resolver) { | |
| defaultConfigResolvers.add(resolver); | |
| log.debug("DefaultConfigResolver added: " + resolver.getClass().getName()); | |
| } |
| public void removeDefaultConfigResolver(DefaultConfigResolver resolver) { | ||
|
|
||
| defaultConfigResolvers.remove(resolver); | ||
| } | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
| public void removeDefaultConfigResolver(DefaultConfigResolver resolver) { | |
| defaultConfigResolvers.remove(resolver); | |
| } | |
| } | |
| public void removeDefaultConfigResolver(DefaultConfigResolver resolver) { | |
| defaultConfigResolvers.remove(resolver); | |
| log.debug("DefaultConfigResolver removed: " + resolver.getClass().getName()); | |
| } |
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.
d5107d2 to
e7a1a5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
components/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/ConfigurationManagerImpl.java (1)
626-626: ⚡ Quick winAdd a log at the no-resolver error return boundary.
Line 626 throws a handled client error without a corresponding diagnostic log in this new critical resolution path. Add a guarded DEBUG/WARN log (not ERROR) before throwing to improve traceability.
As per coding guidelines, Java changes should log meaningful decision/error-return points and avoid ERROR for client-driven failures.
🤖 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/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/ConfigurationManagerImpl.java` at line 626, Line 626 in the ConfigurationManagerImpl.java file throws a handled client exception for a non-existent resource without a corresponding diagnostic log. Add a guarded DEBUG or WARN level log statement before the throw statement that documents the resolution failure, including the resourceName and any relevant context to improve traceability. Use a guarded log (e.g., if (log.isDebugEnabled())) to ensure this DEBUG/WARN level logging aligns with coding guidelines and does not inappropriately treat client-driven errors as ERROR level issues.Source: Coding guidelines
🤖 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/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/ConfigurationManagerImpl.java`:
- Around line 613-627: The getDefaultResource method does not validate the
resourceType and resourceName parameters before using them in resolver dispatch,
which can lead to inconsistent errors or null pointer exceptions. Add input
validation at the beginning of the getDefaultResource method to check that both
resourceType and resourceName are not null and not empty strings, throwing an
appropriate ConfigurationManagementException with a standard error code if
validation fails. This ensures consistent error handling aligned with the
standard invalid-request flow used elsewhere in the codebase.
In
`@components/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/internal/ConfigurationManagerComponentDataHolder.java`:
- Around line 123-126: The getDefaultConfigResolvers() method in
ConfigurationManagerComponentDataHolder exposes the internal mutable list
directly, allowing callers to modify the resolver registry outside the DS
bind/unbind flow. Return an immutable/unmodifiable copy of the
defaultConfigResolvers list instead of the raw list by wrapping it with
Collections.unmodifiableList() or similar immutability wrapper to prevent
unauthorized modifications to the resolver registry.
In
`@components/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/test/java/org/wso2/carbon/identity/configuration/mgt/core/ConfigurationManagerTest.java`:
- Around line 962-966: The testGetDefaultResourceThrowsWhenNoResolversRegistered
test is flaky because it depends on shared singleton state (an empty resolver
registry) that may be polluted by other tests. To fix this, add a `@BeforeMethod`
setup method in the ConfigurationManagerTest class that explicitly clears all
resolver registrations before each test runs, ensuring this test always starts
with a clean state. Alternatively, add a `@AfterMethod` teardown method to enforce
cleanup of resolver registrations after each test completes, preventing state
leakage to subsequent tests.
---
Nitpick comments:
In
`@components/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/ConfigurationManagerImpl.java`:
- Line 626: Line 626 in the ConfigurationManagerImpl.java file throws a handled
client exception for a non-existent resource without a corresponding diagnostic
log. Add a guarded DEBUG or WARN level log statement before the throw statement
that documents the resolution failure, including the resourceName and any
relevant context to improve traceability. Use a guarded log (e.g., if
(log.isDebugEnabled())) to ensure this DEBUG/WARN level logging aligns with
coding guidelines and does not inappropriately treat client-driven errors as
ERROR level issues.
🪄 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: 06cb3072-099d-4f1c-96be-7fbe4a65f7ac
📒 Files selected for processing (7)
components/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/ConfigurationManager.javacomponents/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/ConfigurationManagerImpl.javacomponents/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/DefaultConfigResolver.javacomponents/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/internal/ConfigurationManagerComponent.javacomponents/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/internal/ConfigurationManagerComponentDataHolder.javacomponents/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/test/java/org/wso2/carbon/identity/configuration/mgt/core/ConfigurationManagerTest.javafeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/resource-access-control-v2.xml.j2
🚧 Files skipped from review as they are similar to previous changes (1)
- features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/resource-access-control-v2.xml.j2
| @Test(priority = 47, expectedExceptions = ConfigurationManagementException.class) | ||
| public void testGetDefaultResourceThrowsWhenNoResolversRegistered() throws Exception { | ||
|
|
||
| configurationManager.getDefaultResource("anyType", "anyName"); | ||
| } |
There was a problem hiding this comment.
Make the “no resolvers registered” test isolated from shared singleton state.
This test assumes an empty global resolver registry. If another test leaves a resolver behind, Line 965 can become flaky. Clear resolver registrations in @BeforeMethod (or enforce cleanup in @AfterMethod) to guarantee isolation.
🤖 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/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/test/java/org/wso2/carbon/identity/configuration/mgt/core/ConfigurationManagerTest.java`
around lines 962 - 966, The
testGetDefaultResourceThrowsWhenNoResolversRegistered test is flaky because it
depends on shared singleton state (an empty resolver registry) that may be
polluted by other tests. To fix this, add a `@BeforeMethod` setup method in the
ConfigurationManagerTest class that explicitly clears all resolver registrations
before each test runs, ensuring this test always starts with a clean state.
Alternatively, add a `@AfterMethod` teardown method to enforce cleanup of resolver
registrations after each test completes, preventing state leakage to subsequent
tests.
…nfigResolver management
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/ConfigurationManager.java`:
- Line 397: The getDefaultResource method declaration at line 397 is missing the
throws ConfigurationManagementException clause from its signature. Add the
throws ConfigurationManagementException declaration to the getDefaultResource
method signature to restore the checked-exception contract and maintain
consistency with the interface's API contract for standardized error propagation
from implementations.
In
`@components/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/ConfigurationManagerImpl.java`:
- Around line 613-619: The getDefaultResource method in ConfigurationManagerImpl
has a potential NullPointerException on line 618 where
resolver.getDefaultConfigs is called. The getDefaultConfigResolver call can
return null when no resolver is registered for the given resourceType and
resourceName combination, but there is no null check before dereferencing the
resolver variable. Add an explicit null check immediately after obtaining the
resolver from
ConfigurationManagerComponentDataHolder.getInstance().getDefaultConfigResolver(resourceType,
resourceName), and throw a ConfigurationManagementException with an appropriate
error message if the resolver is null, rather than allowing the code to proceed
and cause a NullPointerException.
In
`@components/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/internal/ConfigurationManagerComponentDataHolder.java`:
- Around line 124-127: The addDefaultConfigResolver method does not validate
whether the resolver's getResourceIdentifier() returns null before using it as a
key in the ConcurrentHashMap, which will throw a NullPointerException at runtime
since ConcurrentHashMap rejects null keys. Add a null check after calling
resolver.getResourceIdentifier() to guard against null identifiers and either
skip the operation with appropriate logging or throw a meaningful exception.
Apply the same null-checking guard to the corresponding unbind/remove method
that handles DefaultConfigResolver removal (referenced at lines 135-138) to
ensure consistency across both operations.
In
`@components/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/model/ResourceIdentifier.java`:
- Around line 25-52: The ResourceIdentifier class is mutable due to the presence
of setter methods setResourceType and setResourceName, which allows the object's
internal state to be modified after instantiation. Since ResourceIdentifier is
used as a key in ConcurrentHashMap in ConfigurationManagerComponentDataHolder,
this violates the requirement that map keys must remain immutable, as mutations
after insertion can cause hash code changes and lead to lookup/removal failures.
Remove the setResourceType and setResourceName methods from the
ResourceIdentifier class to make it immutable, ensuring objects can only be
initialized through the constructor and cannot be modified afterward.
In
`@features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json`:
- Around line 1324-1325: Fix the typo in the description field for the Push
Device Registration email template configuration. The word "reigistered" in the
description string is misspelled and should be corrected to "registered". Locate
the description property that contains "This email is sent to the user to inform
when a new device has been reigistered for push authentication." and correct the
misspelled word to ensure proper user-facing documentation.
- Line 1326: The push device registration ID `UHVzaERldmljZVJlZ2lzdHJhdGlvbg==`
contains base64 padding characters (`==`) at the end, which is inconsistent with
all other 67 template IDs in the file that use unpadded base64 format. Remove
the padding from this ID to match the existing convention and prevent backend
lookup failures due to exact string matching. Change the ID from its current
padded form to the unpadded equivalent.
🪄 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: e4c9c261-b4cb-4306-9ac5-9d45c4e51f80
📒 Files selected for processing (7)
components/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/ConfigurationManager.javacomponents/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/ConfigurationManagerImpl.javacomponents/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/DefaultConfigResolver.javacomponents/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/internal/ConfigurationManagerComponentDataHolder.javacomponents/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/model/ResourceIdentifier.javafeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json
🚧 Files skipped from review as they are similar to previous changes (1)
- components/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/DefaultConfigResolver.java
…ion configurations
…or DefaultConfigResolvers
…resolver validation
ad69cfa to
de33c82
Compare
| @@ -16,22 +16,34 @@ | |||
|
|
|||
There was a problem hiding this comment.
update license headers on changed files. check on all other places
| if (!(o instanceof ResourceIdentifier that)) { | ||
| return false; | ||
| } | ||
| return Objects.equals(resourceType, that.resourceType) && |
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { |
| ('08fbc096-56c5-4ae6-9edc-54198a07e0dc', 'ISSUER_USAGE_SCOPE', 'A resource type to store issuer usage scope for organizations.'), | ||
| ('12c78d11-65cd-4b6e-b482-98538ecd7a5c', 'FAPI_CONFIGURATION', 'A resource type to keep the FAPI configurations.') | ||
| ('12c78d11-65cd-4b6e-b482-98538ecd7a5c', 'FAPI_CONFIGURATION', 'A resource type to keep the FAPI configurations.'), | ||
| ('4fed2813-cfa8-40b1-83e6-4ab85d7fcb16', 'DEVICE_MANAGEMENT', 'A resource type to keep tenant level configurations for push authentication') |
There was a problem hiding this comment.
| ('4fed2813-cfa8-40b1-83e6-4ab85d7fcb16', 'DEVICE_MANAGEMENT', 'A resource type to keep tenant level configurations for push authentication') | |
| ('4fed2813-cfa8-40b1-83e6-4ab85d7fcb16', 'DEVICE_MANAGEMENT', 'A resource type to keep tenant level configurations for user device management') |
| ('08fbc096-56c5-4ae6-9edc-54198a07e0dc', 'ISSUER_USAGE_SCOPE', 'A resource type to store issuer usage scope for organizations.'), | ||
| ('12c78d11-65cd-4b6e-b482-98538ecd7a5c', 'FAPI_CONFIGURATION', 'A resource type to keep the FAPI configurations.'); | ||
| ('12c78d11-65cd-4b6e-b482-98538ecd7a5c', 'FAPI_CONFIGURATION', 'A resource type to keep the FAPI configurations.'), | ||
| ('4fed2813-cfa8-40b1-83e6-4ab85d7fcb16', 'DEVICE_MANAGEMENT', 'A resource type to keep tenant level configurations for push authentication'); |
There was a problem hiding this comment.
setup other dbs as well
| </PushAuthenticator> | ||
|
|
||
| <PushDeviceManagement> | ||
| <MaxDeviceLimitUpperBound>{{push_device_management.max_device_limit_upper_bound}}</MaxDeviceLimitUpperBound> |
There was a problem hiding this comment.
| <MaxDeviceLimitUpperBound>{{push_device_management.max_device_limit_upper_bound}}</MaxDeviceLimitUpperBound> | |
| <MaxDeviceLimitPerUser>{{push_device_management.max_device_limit_per_user}}</MaxDeviceLimitPerUser> |
| <Resource context="(.*)/api/server/v1/configs/home-realm-identifiers" secured="true" http-method="GET"> | ||
| <Scopes>internal_login</Scopes> | ||
| </Resource> | ||
| <Resource context="(.*)/api/server/v1/configs/preferences" secured="true" http-method="POST"> |
638cf5a to
5d5436d
Compare
5d5436d to
bdd31dc
Compare
|



This pull request introduces a new secured API resource for managing user preferences via a POST request. The change ensures that access to this endpoint is restricted to users with the
internal_loginscope.Access control update:
resource-access-control-v2.xml.j2to secure thePOSTmethod for the/api/server/v1/configs/preferencesendpoint, requiring theinternal_loginscope.Related Issues