Update /notification-senders/sms API to support sms service provider authentication #1032
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...bon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java
Show resolved
Hide resolved
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.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | N | Log is added to builder class itself. |
Thisara-Welmilla
changed the title
Thisara-Welmilla
force-pushed
the
improve-sms-provider-v1-api
branch
2 times, most recently
from
c56964a to
166d30d
Compare
WalkthroughRefactors SMS sender DTO construction to use Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Notification API
participant Service as NotificationSenderManagementService
participant Builder as SMSSenderDTO.Builder
participant Store as Persistence
Client->>API: add/update SMS sender request (may include authentication)
API->>Service: buildSMSSenderDTO(request)
Service->>Builder: create builder & set common fields
alt authentication present
Service->>Builder: buildNotificationSenderAuthentication(auth)
end
Service->>Builder: addProperty(...) for other properties
Service->>Builder: build() -- may throw NotificationSenderManagementException
Builder-->>Service: SMSSenderDTO
Service->>Store: save/update SMSSenderDTO
Store-->>Service: ack
Service-->>API: operation result
API-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–35 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml (1)
1206-1248: Well-designed Authentication schema supporting multiple auth types.The schema cleanly separates authentication type and properties, providing flexibility for different provider requirements. The documentation and examples are clear.
However, static analysis flagged the example UUIDs on lines 1224 and 1246 as potential API keys. These are false positives (they're clearly documentation examples), but to prevent future scanning noise, consider adding a comment in the YAML like
# Example UUIDs, not real credentialsor rewording the examples to be more obviously synthetic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/Authentication.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/SMSSender.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/SMSSenderAdd.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/SMSSenderUpdateRequest.javais excluded by!**/gen/**
📒 Files selected for processing (3)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java(6 hunks)components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml(4 hunks)pom.xml(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: PR Builder
pom.xml
[error] 1-1: Dependency resolution failed for artifact org.wso2.carbon.identity.framework:org.wso2.carbon.identity.core:7.8.596-SNAPSHOT
[error] 1-1: Could not find artifact org.wso2.carbon.identity.framework:org.wso2.carbon.identity.core:7.8.596-SNAPSHOT in specified repositories
[error] 1-1: Dependency org.wso2.carbon.identity.framework:org.wso2.carbon.idp.mgt:7.8.596-SNAPSHOT resolution failed
[error] 1-1: Could not find artifact org.wso2.carbon.identity.framework:org.wso2.carbon.idp.mgt:7.8.596-SNAPSHOT in specified repositories
[error] 1-1: Dependency org.wso2.carbon.identity.framework:org.wso2.carbon.user.mgt:7.8.596-SNAPSHOT resolution failed
[error] 1-1: Could not find artifact org.wso2.carbon.identity.framework:org.wso2.carbon.user.mgt:7.8.596-SNAPSHOT in specified repositories
[error] 1-1: Dependency org.wso2.carbon.identity.framework:org.wso2.carbon.user.mgt.common:7.8.596-SNAPSHOT resolution failed
[error] 1-1: Could not find artifact org.wso2.carbon.identity.framework:org.wso2.carbon.user.mgt.common:7.8.596-SNAPSHOT in specified repositories
[error] 1-1: Dependency org.wso2.carbon.identity.framework:org.wso2.carbon.claim.metadata.mgt:7.8.596-SNAPSHOT resolution failed
[error] 1-1: Could not find artifact org.wso2.carbon.identity.framework:org.wso2.carbon.claim.metadata.mgt:7.8.596-SNAPSHOT in specified repositories
[error] 1-1: Dependency org.wso2.carbon.identity.framework:org.wso2.carbon.identity.mgt:7.8.596-SNAPSHOT resolution failed
[error] 1-1: Could not find artifact org.wso2.carbon.identity.framework:org.wso2.carbon.identity.mgt:7.8.596-SNAPSHOT in specified repositories
[error] 1-1: Dependency org.wso2.carbon.identity.framework:org.wso2.carbon.identity.base:7.8.596-SNAPSHOT resolution failed
[error] 1-1: Could not find artifact org.wso2.carbon.identity.framework:org.wso2.carbon.identity.base:7.8.596-SNAPSHOT in specified repositories
[error] 1-1: Dependency org.wso2.carbon.identity.framework:org.wso2.carbon.identity.configuration.mgt.core:7.8.596-SNAPSHOT resolution failed
[error] 1-1: Could not find artifact org.wso2.carbon.identity.framework:org.wso2.carbon.identity.configuration.mgt.core:7.8.596-SNAPSHOT in specified repositories
[error] 1-1: Dependency org.wso2.carbon.identity.framework:org.wso2.carbon.identity.flow.execution.engine:7.8.596-SNAPSHOT resolution failed
[error] 1-1: Could not find artifact org.wso2.carbon.identity.framework:org.wso2.carbon.identity.flow.execution.engine:7.8.596-SNAPSHOT in specified repositories
[error] 1-1: Dependency org.wso2.carbon.identity.framework:org.wso2.carbon.identity.application.mgt:7.8.596-SNAPSHOT resolution failed
[error] 1-1: Could not find artifact org.wso2.carbon.identity.framework:org.wso2.carbon.identity.application.mgt:7.8.596-SNAPSHOT in specified repositories
[error] 1-1: Dependency org.wso2.carbon.identity.framework:org.wso2.carbon.identity.application.authentication.framework:7.8.596-SNAPSHOT resolution failed
[error] 1-1: Could not find artifact org.wso2.carbon.identity.framework:org.wso2.carbon.identity.application.authentication.framework:7.8.596-SNAPSHOT in specified repositories
[error] 1-1: Dependency org.wso2.carbon.identity.framework:org.wso2.carbon.identity.application.common:7.8.596-SNAPSHOT resolution failed
[error] 1-1: Could not find artifact org.wso2.carbon.identity.framework:org.wso2.carbon.identity.application.common:7.8.596-SNAPSHOT in specified repositories
[error] 1-1: Dependency org.wso2.carbon.identity.framework:org.wso2.carbon.identity.input.validation.mgt:7.8.596-SNAPSHOT resolution failed
[error] 1-1: Could not find artifact org.wso2.carbon.identity.framework:org.wso2.carbon.identity.input.validation.mgt:7.8.596-SNAPSHOT in specified repositories
[error] 1-1: Dependency org.wso2.carbon.identity.framework:org.wso2.carbon.identity.action.management:7.8.596-SNAPSHOT resolution failed
[error] 1-1: Could not find artifact org.wso2.carbon.identity.framework:org.wso2.carbon.identity.action.management:7.8.596-SNAPSHOT in specified repositories
🪛 Gitleaks (8.28.0)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml
[high] 1224-1224: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 1246-1246: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (2)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml (1)
967-968: Consistent addition of optional authentication field across SMS sender schemas.The
authenticationfield is correctly added as an optional property toSMSSenderAdd,SMSSender, andSMSSenderUpdateRequestschemas, maintaining backward compatibility with existing clients that use thekeyandsecretfields.Also applies to: 1063-1064, 1147-1148
pom.xml (1)
967-967: Dependency versions are intentional SNAPSHOT upgrades—verify external artifact availability.The SNAPSHOT versions in lines 967 and 971 are confirmed and represent intentional upgrades from released versions (7.8.547 → 7.8.596-SNAPSHOT; 1.9.48 → 1.9.75-SNAPSHOT). These dependencies are referenced in 36+ locations throughout the pom.xml.
However, no build failure evidence exists within the codebase—no CI/CD workflows, logs, or documented issues are present. The claim about "30+ dependency resolution failures" references an external pipeline state not captured in this repository.
Before merging, manually verify that:
- The WSO2 Maven snapshot repository (
https://maven.wso2.org/nexus/content/repositories/snapshots) contains these SNAPSHOT artifacts- Your build environment has access to the WSO2 snapshot repository in
settings.xml- A test build succeeds in your CI/CD system (external to this repo)
...bon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java
Show resolved
Hide resolved
Thisara-Welmilla
force-pushed
the
improve-sms-provider-v1-api
branch
from
166d30d to
f8cfd75
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java (1)
411-419: Guard against null authentication properties and type.This method assumes
authentication.getProperties()andauthentication.getType()are non-null. When a client supplies anauthenticationblock without the optionalpropertiesmap ortypefield (which the OpenAPI schema allows), the code throws aNullPointerException, resulting in a 500 error instead of a proper validation response.Apply this diff to guard against null values:
private void buildNotificationSenderAuthentication(SMSSenderDTO.Builder builder, Authentication authentication) { - if (authentication != null) { - builder.authType(authentication.getType().toString()); - for (String propKey : authentication.getProperties().keySet()) { - builder.addAuthProperty(propKey, (String) authentication.getProperties().get(propKey)); - } + if (authentication == null) { + return; + } + if (authentication.getType() != null) { + builder.authType(authentication.getType().toString()); + } + if (authentication.getProperties() != null) { + authentication.getProperties().forEach((propKey, value) -> { + if (StringUtils.isNotBlank(propKey) && value != null) { + builder.addAuthProperty(propKey, value.toString()); + } + }); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/Authentication.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/SMSSender.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/SMSSenderAdd.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/SMSSenderUpdateRequest.javais excluded by!**/gen/**
📒 Files selected for processing (3)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java(6 hunks)components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml(4 hunks)pom.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml
- pom.xml
🔇 Additional comments (4)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java (4)
27-27: LGTM!The new imports for
AuthenticationandHashMapare appropriate for the authentication support being added.Also applies to: 46-46
373-389: Builder pattern refactoring looks good.The refactoring to use the Builder pattern improves code clarity and allows for better validation in the builder itself. The method correctly throws
NotificationSenderManagementExceptionto align with the builder's contract.
392-409: Builder pattern refactoring looks good.Consistent with the add flow, this update method correctly uses the Builder pattern and properly declares the exception in its signature.
101-110: LGTM!Correctly moved the
buildSMSSenderDTOcalls inside the try blocks to handle the newly thrownNotificationSenderManagementException. This ensures that any validation or construction errors are properly caught and handled by the exception handler.Also applies to: 264-273
...bon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java
Show resolved
Hide resolved
Thisara-Welmilla
force-pushed
the
improve-sms-provider-v1-api
branch
from
f8cfd75 to
8c97e90
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java (1)
440-444: Guard against null type and properties from DTO.As flagged in the previous review,
dto.getAuthentication().getType()anddto.getAuthentication().getProperties()can be null, causing NPEs when calling.getName()or passing null to the HashMap constructor.Apply this diff to defensively read from the DTO:
if (dto.getAuthentication() != null) { - smsSender.setAuthentication(new Authentication() - .type(Authentication.TypeEnum.fromValue(dto.getAuthentication().getType().getName())) - .properties(new HashMap<>(dto.getAuthentication().getProperties()))); + Authentication authentication = new Authentication(); + if (dto.getAuthentication().getType() != null) { + authentication.type(Authentication.TypeEnum.fromValue(dto.getAuthentication().getType().getName())); + } + if (dto.getAuthentication().getProperties() != null) { + authentication.properties(new HashMap<>(dto.getAuthentication().getProperties())); + } + smsSender.setAuthentication(authentication); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/Authentication.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/SMSSender.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/SMSSenderAdd.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/SMSSenderUpdateRequest.javais excluded by!**/gen/**
📒 Files selected for processing (3)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java(6 hunks)components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml(4 hunks)pom.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pom.xml
🔇 Additional comments (4)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml (2)
967-968: Schema extensions align well with backward compatibility.The addition of the
authenticationfield as an optional reference across SMSSenderAdd, SMSSender, and SMSSenderUpdateRequest maintains backward compatibility while enabling the new authentication capability. The changes are consistent across all three request/response schemas.Also applies to: 1063-1064, 1147-1148
1-1263: Verify scope of authentication field additions across sender types.The AI-generated summary indicates that
PushSenderAddandPushSenderUpdateRequestschemas should also include theauthenticationfield, but these changes are not present in the provided code. Clarify whether authentication support should be limited to SMS senders only, or if Push senders should also be updated for consistency.components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java (2)
103-104: LGTM: Proper exception handling for builder method.The
buildSMSSenderDTOcall is correctly placed inside the try-catch block to handle the newNotificationSenderManagementExceptionthrown by the builder-based method.
267-267: LGTM: Consistent exception handling for update operation.The
buildSMSSenderDTOcall for the update flow is correctly placed inside the try-catch block.
...bon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java
Show resolved
Hide resolved
...bon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java
Show resolved
Hide resolved
...bon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java
Show resolved
Hide resolved
...arbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml
Show resolved
Hide resolved
Thisara-Welmilla
force-pushed
the
improve-sms-provider-v1-api
branch
from
8c97e90 to
61925ab
Compare
Thisara-Welmilla
force-pushed
the
improve-sms-provider-v1-api
branch
from
61925ab to
fbe2260
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java (1)
411-420: Consider using.toString()instead of casting for type safety.Line 417 uses an unsafe cast
(String) propValuethat could throwClassCastExceptionif the properties map contains non-String values. While the OpenAPI schema may define these as strings, defensive coding would be more robust.Consider this improvement:
private void buildNotificationSenderAuthentication(SMSSenderDTO.Builder builder, Authentication authentication) { if (authentication != null && authentication.getType() != null) { builder.authType(authentication.getType().toString()); if (authentication.getProperties() != null) { authentication.getProperties().forEach( - (propKey, propValue) -> builder.addAuthProperty(propKey, (String) propValue)); + (propKey, propValue) -> { + if (StringUtils.isNotBlank(propKey) && propValue != null) { + builder.addAuthProperty(propKey, propValue.toString()); + } + }); } } }This approach:
- Uses
propValue.toString()to safely handle any object type- Validates that
propKeyis not blank- Checks that
propValueis not null before processing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/Authentication.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/SMSSender.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/SMSSenderAdd.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/SMSSenderUpdateRequest.javais excluded by!**/gen/**
📒 Files selected for processing (3)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java(6 hunks)components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml(4 hunks)pom.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pom.xml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Thisara-Welmilla
Repo: wso2/identity-api-server PR: 1032
File: components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java:384-387
Timestamp: 2025-11-06T05:02:47.521Z
Learning: In components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java, SMS sender property validation is handled by the service layer builder class (SMSSenderDTO.Builder), not at the API layer, because the validation logic is complex and depends on other attributes of the object.
📚 Learning: 2025-11-06T05:02:47.521Z
Learnt from: Thisara-Welmilla
Repo: wso2/identity-api-server PR: 1032
File: components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java:384-387
Timestamp: 2025-11-06T05:02:47.521Z
Learning: In components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java, SMS sender property validation is handled by the service layer builder class (SMSSenderDTO.Builder), not at the API layer, because the validation logic is complex and depends on other attributes of the object.
Applied to files:
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.javacomponents/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml
🔇 Additional comments (9)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml (3)
967-968: SMS schema updates maintain backward compatibility and API consistency.The
authenticationfield is appropriately added as optional across all three SMS schemas (create, response, update). Existingkeyandsecretfields remain untouched, enabling smooth coexistence of legacy and new authentication patterns.Also applies to: 1063-1064, 1147-1148
1206-1263: Clarify property names for CLIENT_CREDENTIAL authentication type.The Authentication schema includes conflicting examples for the CLIENT_CREDENTIAL type:
- Description examples (lines 1213-1217): use
usernameandpassword- Standalone example (lines 1259-1262): use
clientId,clientSecret,tokenEndpoint,scopesThese represent different property structures. Clarify which set is correct or document both variants if the type supports either format.
Also consider making the description more SMS-specific. Currently it refers to "action's endpoint" (line 1209), which is generic. For SMS senders, this could be "SMS provider endpoint" for clarity.
1164-1180: Verify scope: Should PushSenderUpdateRequest also include authentication field?The AI summary states that
authenticationfield was added toPushSenderUpdateRequest, but the code (lines 1164-1180) shows no such change. If the authentication feature is meant to support multiple sender types, Push senders may also need this capability for consistency. Confirm whether the limitation to SMS-only is intentional.components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java (6)
27-27: LGTM!The new imports for
AuthenticationandHashMapare appropriate for the authentication field support being added.Also applies to: 46-46
101-109: LGTM!Correctly moved DTO building into the try block to handle the
NotificationSenderManagementExceptionnow thrown by the builder-based construction.
266-272: LGTM!Properly handles the checked exception from the builder-based DTO construction, consistent with the add method.
373-389: Excellent refactor to builder pattern with backward compatibility.The changes correctly:
- Use
SMSSenderDTO.Builderfor construction- Support both legacy
key/secretfields (lines 379-380) and the newauthenticationfield (line 383)- Delegate validation to the service layer builder as per the design decision
This maintains backward compatibility while enabling the new authentication model.
Based on learnings.
392-409: LGTM!The update method mirrors the add method's builder pattern implementation and correctly maintains backward compatibility by supporting both legacy and new authentication fields.
440-444: LGTM!The authentication mapping from the service layer DTO is implemented correctly. Based on your previous explanation, the service layer DTO guarantees that
getType()andgetProperties()are non-null whengetAuthentication()is non-null, enforced by the builder pattern.
...arbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml
Show resolved
Hide resolved
Thisara-Welmilla
force-pushed
the
improve-sms-provider-v1-api
branch
from
cf9af6a to
00ce6c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java (1)
433-452: Guard against invalid Property enum values.Line 447 uses
Property.valueOf(entry.getKey())which throwsIllegalArgumentExceptionif the authentication property key doesn't match anyPropertyenum constant. This will cause runtime failures when the DTO contains authentication properties with keys that aren't validPropertyenum values.Apply this diff to safely handle unknown property keys:
private SMSSender buildSMSSenderFromDTO(SMSSenderDTO dto) { Set<Property> authPropToExclude = new HashSet<>( Arrays.asList(Property.PASSWORD, Property.CLIENT_SECRET, Property.ACCESS_TOKEN, Property.VALUE)); SMSSender smsSender = new SMSSender(); smsSender.setName(dto.getName()); smsSender.setProvider(dto.getProvider()); smsSender.setProviderURL(dto.getProviderURL()); smsSender.setKey(dto.getKey()); smsSender.setSecret(dto.getSecret()); smsSender.setSender(dto.getSender()); smsSender.setContentType(SMSSender.ContentTypeEnum.valueOf(dto.getContentType())); if (dto.getAuthentication() != null) { - Map<String, Object> filteredAuthProp = dto.getAuthentication().getProperties().entrySet().stream() - .filter(entry -> !authPropToExclude.contains(Property.valueOf(entry.getKey()))) - .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + Map<String, Object> filteredAuthProp = dto.getAuthentication().getProperties().entrySet().stream() + .filter(entry -> { + try { + return !authPropToExclude.contains(Property.valueOf(entry.getKey())); + } catch (IllegalArgumentException e) { + // Include properties that don't match any Property enum + return true; + } + }) + .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); smsSender.setAuthentication(new Authentication() .type(Authentication.TypeEnum.fromValue(dto.getAuthentication().getType().getName())) .properties(filteredAuthProp)); }Alternatively, if all authentication property keys are guaranteed to be valid
Propertyenum values, document this invariant.
♻️ Duplicate comments (1)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java (1)
412-421: Unsafe cast can cause ClassCastException.Line 418 still contains the unsafe cast
(String) propValuethat was flagged in a previous review. If the OpenAPI schema allows non-String values in the authentication properties map, this cast will throw aClassCastExceptionat runtime.Apply this diff to safely handle any object type:
private void buildNotificationSenderAuthentication(SMSSenderDTO.Builder builder, Authentication authentication) { if (authentication != null && authentication.getType() != null) { builder.authType(authentication.getType().toString()); if (authentication.getProperties() != null) { authentication.getProperties().forEach( - (propKey, propValue) -> builder.addAuthProperty(propKey, (String) propValue)); + (propKey, propValue) -> { + if (propValue != null) { + builder.addAuthProperty(propKey, propValue.toString()); + } + }); } } }
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml (1)
1216-1216: Use more obviously fake values in documentation examples to avoid security tool false positives.Gitleaks is flagging the example UUIDs (lines 1216, 1264) and token (line 1233) as potential API keys. While these are clearly mock credentials for documentation, consider replacing them with more obviously fictional values (e.g.,
"YOUR_CLIENT_ID_HERE","YOUR_TOKEN_HERE") to prevent false positives in pipeline security scans and reduce confusion for API consumers who might copy-paste examples.For instance:
- "clientSecret": "83cdc120-ccf6-4163-a4a8-c1ba3e872daa", + "clientSecret": "YOUR_CLIENT_SECRET_HERE",- "token": "12345-abcde-67890" + "token": "YOUR_BEARER_TOKEN_HERE"Also applies to: 1233-1233, 1264-1264
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/model/Authentication.javais excluded by!**/gen/**
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java(8 hunks)components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Thisara-Welmilla
Repo: wso2/identity-api-server PR: 1032
File: components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java:384-387
Timestamp: 2025-11-06T05:02:47.521Z
Learning: In components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java, SMS sender property validation is handled by the service layer builder class (SMSSenderDTO.Builder), not at the API layer, because the validation logic is complex and depends on other attributes of the object.
📚 Learning: 2025-11-06T05:02:47.521Z
Learnt from: Thisara-Welmilla
Repo: wso2/identity-api-server PR: 1032
File: components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java:384-387
Timestamp: 2025-11-06T05:02:47.521Z
Learning: In components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java, SMS sender property validation is handled by the service layer builder class (SMSSenderDTO.Builder), not at the API layer, because the validation logic is complex and depends on other attributes of the object.
Applied to files:
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yamlcomponents/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java
🪛 Gitleaks (8.29.0)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml
[high] 1216-1216: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 1233-1233: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 1264-1264: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (7)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml (2)
967-968: Backward compatibility preserved with optionalauthenticationfield additions.The new
authenticationfield is correctly added as optional across all three SMS-related schemas (add, read, update), ensuring existing clients continue to work without modification. The consistent use of$refto the centralized Authentication schema maintains API coherence.Also applies to: 1063-1064, 1147-1148
1206-1266: Well-structured Authentication schema with comprehensive documentation.The new schema clearly documents four authentication types with inline examples showing the expected property structure for each. The use of
additionalProperties: truefor thepropertiesobject provides necessary flexibility for different auth type requirements.components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java (5)
27-27: LGTM! Necessary imports for authentication support.The new imports correctly support the authentication feature: the API model (line 27), service layer Property enum (line 38), and Map for properties handling (line 49).
Also applies to: 38-38, 49-49
102-111: LGTM! Proper exception handling for builder-based construction.The refactoring correctly wraps
buildSMSSenderDTOin a try-catch block to handle the checkedNotificationSenderManagementExceptionnow thrown by the builder pattern.
265-274: LGTM! Consistent exception handling in update flow.The update method correctly mirrors the add flow by wrapping the builder-based DTO construction in a try-catch block.
374-390: LGTM! Builder pattern correctly implemented.The refactoring to use
SMSSenderDTO.Builderis well-structured. The authentication is populated via the helper method, and properties are added through the builder. As per learnings, property validation is intentionally handled by the service layer builder class.Based on learnings
393-410: LGTM! Update method consistent with add flow.The update method correctly implements the builder pattern, mirroring the structure of the add method. Authentication and properties are properly populated through the builder.
Based on learnings
|
PR builder started |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java (1)
412-421: Verify the unsafe cast at line 418.The cast
(String) propValueat line 418 can throwClassCastExceptionif the OpenAPI-generatedAuthenticationmodel'spropertiesfield is typed asMap<String, Object>(typical foradditionalProperties). This was flagged in previous reviews and marked as "Addressed," but the unsafe cast remains.Run the following script to verify the return type of
authentication.getProperties():#!/bin/bash # Description: Check the type signature of Authentication.getProperties() in the generated model # Find and display the Authentication class properties field and getter ast-grep --pattern $'class Authentication { $$$ properties $$$ }' # Also search for the getProperties method signature rg -A 3 "getProperties\(\)" --type=java -g "**/Authentication.java"If
getProperties()returnsMap<String, Object>, replace the unsafe cast with safe conversion:if (authentication.getProperties() != null) { authentication.getProperties().forEach( - (propKey, propValue) -> builder.addAuthProperty(propKey, (String) propValue)); + (propKey, propValue) -> { + if (StringUtils.isNotBlank(propKey) && propValue != null) { + builder.addAuthProperty(propKey, propValue.toString()); + } + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java(8 hunks)pom.xml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Thisara-Welmilla
Repo: wso2/identity-api-server PR: 1032
File: components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java:384-387
Timestamp: 2025-11-06T05:02:47.521Z
Learning: In components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java, SMS sender property validation is handled by the service layer builder class (SMSSenderDTO.Builder), not at the API layer, because the validation logic is complex and depends on other attributes of the object.
📚 Learning: 2025-11-06T05:02:47.521Z
Learnt from: Thisara-Welmilla
Repo: wso2/identity-api-server PR: 1032
File: components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java:384-387
Timestamp: 2025-11-06T05:02:47.521Z
Learning: In components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java, SMS sender property validation is handled by the service layer builder class (SMSSenderDTO.Builder), not at the API layer, because the validation logic is complex and depends on other attributes of the object.
Applied to files:
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java
🔇 Additional comments (7)
pom.xml (1)
972-972: Verifyidentity.event.handler.versionupdate compatibility.Line 972 updates
identity.event.handler.versionfrom 1.9.48 to 1.9.75. Confirm that this version bump is compatible with the SMS sender authentication changes and doesn't introduce breaking changes to the notification event handler.components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java (6)
27-27: LGTM!The new imports are correctly added to support the authentication functionality.
Also applies to: 38-38, 49-49
374-390: LGTM!The refactoring to use the builder pattern is correct, and the method properly handles authentication via the new helper method. The exception declaration aligns with the builder's contract.
393-410: LGTM!The update method correctly mirrors the add method's refactoring to the builder pattern and properly integrates authentication support.
445-452: LGTM!The authentication mapping correctly filters out sensitive properties and converts the DTO authentication to the public API model. Based on the service layer contract (as explained in previous reviews), when
dto.getAuthentication()is non-null, itsgetProperties()andgetType()are guaranteed to be non-null.
104-105: LGTM!The exception handling is correct. Since
buildSMSSenderDTOmethods now throwNotificationSenderManagementException, the calls are properly wrapped in try-catch blocks.Also applies to: 268-268
433-434: Manual verification required for external dependency behavior.The Property enum at line 447 (
Property.valueOfName(entry.getKey())) comes from an external dependency (org.wso2.carbon.identity.notification.sender.tenant.config) that's not available in the repository for inspection. The concern about potential exception handling is valid—ifvalueOfName()throws an exception for unknown property keys, the filter will fail.Verify the behavior of
Property.valueOfName()in the external dependency's documentation or source code. If it throws exceptions for unknown keys, add defensive error handling in the filter to prevent runtime failures when encountering unexpected authentication properties.
|
PR builder completed |
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/19656123122
3 participants
Feature:
With this PR,
/notification-senders/smsresource for V2 to introduce a new first-class fieldauthenticationfor configuring authentication details of the SMS provider.Note that these changes do not introduce any breaking changes to the
/notification-senders/smsAPI and preserve full backward compatibility.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.