Skip to content

Onboarding notification-senders/email PATCH endpoint #1034

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lashinijay wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Onboarding notification-senders/email PATCH endpoint #1034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4281875
email provider patch endpoint
lashinijay Oct 31, 2025
644447a
update method name in findbug exclusion
lashinijay Oct 31, 2025
7b51ae6
Merge branch 'master' into email-provider-patch-endpoint
lashinijay Dec 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions .../identity/api/server/notification/sender/v1/core/NotificationSenderManagementService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.wso2.carbon.identity.api.server.notification.sender.v1.model.SMSSender;
import org.wso2.carbon.identity.api.server.notification.sender.v1.model.SMSSenderAdd;
import org.wso2.carbon.identity.api.server.notification.sender.v1.model.SMSSenderUpdateRequest;
import org.wso2.carbon.identity.notification.sender.tenant.config.NotificationSenderManagementConstants;
import org.wso2.carbon.identity.notification.sender.tenant.config.dto.EmailSenderDTO;
import org.wso2.carbon.identity.notification.sender.tenant.config.dto.PushSenderDTO;
import org.wso2.carbon.identity.notification.sender.tenant.config.dto.SMSSenderDTO;
Expand All @@ -59,7 +60,7 @@ public class NotificationSenderManagementService {

public NotificationSenderManagementService(
org.wso2.carbon.identity.notification.sender.tenant.config.NotificationSenderManagementService
notificationSenderManagementService) {
notificationSenderManagementService) {

this.notificationSenderManagementService = notificationSenderManagementService;
}
Expand Down Expand Up @@ -126,7 +127,7 @@ public void deleteNotificationSender(String notificationSenderName) {
try {
notificationSenderManagementService.deleteNotificationSender(notificationSenderName);
} catch (NotificationSenderManagementException e) {
throw handleException(e);
throw handleException(e);
}
}

Expand Down Expand Up @@ -264,7 +265,7 @@ public SMSSender updateSMSSender(String senderName, SMSSenderUpdateRequest smsSe
/**
* Update push sender details by name.
*
* @param senderName Push sender's name.
* @param senderName Push sender's name.
* @param pushSenderUpdateRequest Push sender's updated configurations.
* @return Updated push sender.
*/
Expand Down Expand Up @@ -320,6 +321,10 @@ private EmailSender buildEmailSenderFromDTO(EmailSenderDTO dto) {
emailSender.setSmtpServerHost(dto.getSmtpServerHost());
List<Properties> properties = new ArrayList<>();
dto.getProperties().forEach((key, value) -> {
if (NotificationSenderManagementConstants.CLIENT_SECRET.equalsIgnoreCase(key)) {
log.debug("Skipping client secret property from email sender configuration");
return;
}
Properties prop = new Properties();
prop.setKey(key);
prop.setValue(value);
Expand Down
25 changes: 25 additions & 0 deletions ...va/org/wso2/carbon/identity/api/server/notification/sender/v2/NotificationSendersApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,31 @@ public Response getSMSSenders() {
return delegate.getSMSSenders();
}

@Valid
@PATCH
@Path("/email/{sender-name}")
@Consumes({ "application/json" })
@Produces({ "application/json" })
@ApiOperation(value = "Update an email sender", notes = "This API provides the capability to update an email sender by name. The URL encoded email sender name is used as sender-name.<br> <b>Permission required:</b> <br> * /permission/admin/manage/identity/configmgt/update <br> <b>Scope required:</b> <br> * internal_config_mgt_update ", response = EmailSender.class, authorizations = {
@Authorization(value = "BasicAuth"),
@Authorization(value = "OAuth2", scopes = {

})
}, tags={ "Email Senders", })
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Successful Response", response = EmailSender.class),
@ApiResponse(code = 400, message = "Bad Request", response = Error.class),
@ApiResponse(code = 401, message = "Unauthorized", response = Void.class),
@ApiResponse(code = 403, message = "Forbidden", response = Void.class),
@ApiResponse(code = 404, message = "Not Found", response = Error.class),
@ApiResponse(code = 405, message = "Method Not Allowed.", response = Error.class),
@ApiResponse(code = 500, message = "Server Error", response = Error.class)
})
public Response patchEmailSender(@ApiParam(value = "name of the email sender",required=true) @PathParam("sender-name") String senderName, @ApiParam(value = "" ,required=true) @Valid EmailSenderUpdateRequest emailSenderUpdateRequest) {

return delegate.patchEmailSender(senderName, emailSenderUpdateRequest );
}

@Valid
@PUT
@Path("/email/{sender-name}")
Expand Down
2 changes: 2 additions & 0 deletions ...wso2/carbon/identity/api/server/notification/sender/v2/NotificationSendersApiService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ public interface NotificationSendersApiService {

public Response getSMSSenders();

public Response patchEmailSender(String senderName, EmailSenderUpdateRequest emailSenderUpdateRequest);

public Response updateEmailSender(String senderName, EmailSenderUpdateRequest emailSenderUpdateRequest);

public Response updatePushSender(String senderName, PushSenderUpdateRequest pushSenderUpdateRequest);
Expand Down
55 changes: 55 additions & 0 deletions .../identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,24 @@ public EmailSender updateEmailSender(String senderName, EmailSenderUpdateRequest
}
}

/**
* Patch email sender details by name.
*
* @param senderName Email sender's name.
* @param emailSenderUpdateRequest Email sender's updated configurations.
* @return Updated email sender.
*/
public EmailSender patchEmailSender(String senderName, EmailSenderUpdateRequest emailSenderUpdateRequest) {

try {
EmailSenderDTO dto = updateEmailSenderDTOFromPatchRequest(senderName, emailSenderUpdateRequest);
EmailSenderDTO emailSenderDTO = notificationSenderManagementService.updateEmailSender(dto);
return buildEmailSenderFromDTO(emailSenderDTO);
} catch (NotificationSenderManagementException e) {
throw handleException(e);
}
}

/**
* Update sms sender details by name.
*
Expand Down Expand Up @@ -333,6 +351,43 @@ private EmailSenderDTO buildEmailSenderDTO(String senderName, EmailSenderUpdateR
return dto;
}

private EmailSenderDTO updateEmailSenderDTOFromPatchRequest(String senderName,
EmailSenderUpdateRequest emailSenderUpdateRequest) {

EmailSenderDTO existingEmailSenderDTO;
try {
existingEmailSenderDTO = notificationSenderManagementService.getEmailSender(senderName);
} catch (NotificationSenderManagementException e) {
throw handleException(e);
}
existingEmailSenderDTO.setName(senderName);
if (emailSenderUpdateRequest == null) {
return existingEmailSenderDTO;
}
existingEmailSenderDTO.setFromAddress(emailSenderUpdateRequest.getFromAddress() != null ?
emailSenderUpdateRequest.getFromAddress() : existingEmailSenderDTO.getFromAddress());
existingEmailSenderDTO.setSmtpPort(emailSenderUpdateRequest.getSmtpPort() != null ?
emailSenderUpdateRequest.getSmtpPort() : existingEmailSenderDTO.getSmtpPort());
existingEmailSenderDTO.setSmtpServerHost(emailSenderUpdateRequest.getSmtpServerHost() != null ?
emailSenderUpdateRequest.getSmtpServerHost() : existingEmailSenderDTO.getSmtpServerHost());
existingEmailSenderDTO.setAuthType(emailSenderUpdateRequest.getAuthType() != null ?
emailSenderUpdateRequest.getAuthType() : existingEmailSenderDTO.getAuthType());
if (emailSenderUpdateRequest.getProperties() != null && !emailSenderUpdateRequest.getProperties().isEmpty()) {
emailSenderUpdateRequest.getProperties().forEach((prop) -> {
if (StringUtils.isNotBlank(prop.getKey()) && StringUtils.isNotBlank(prop.getValue())) {
existingEmailSenderDTO.getProperties().put(prop.getKey(), prop.getValue());
}
});
}
Comment on lines +375 to +381
Copy link

@coderabbitai coderabbitai bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Search for property deletion or removal patterns in the codebase
rg -nP -C3 'property.*delet|remov.*property|Properties.*clear|properties\.remove' --type=java

Repository: wso2/identity-api-server

Length of output: 19723

🏁 Script executed:

# Also check the API specification files
fd -e yaml -e yml -e json | xargs rg -l 'notification.*sender|NotificationSender' 2>/dev/null | head -20

Repository: wso2/identity-api-server

Length of output: 407

🏁 Script executed:

# Look for OpenAPI/Swagger specifications
fd -e yaml -e yml | head -30

Repository: wso2/identity-api-server

Length of output: 5079

🏁 Script executed:

# Examine the NotificationSenderManagementService file more broadly to understand the update pattern
wc -l "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"

Repository: wso2/identity-api-server

Length of output: 316

🏁 Script executed:

# Get context around the property merge logic and the update method signature
cat -n "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" | sed -n '350,400p'

Repository: wso2/identity-api-server

Length of output: 3260

🏁 Script executed:

# Check the OpenAPI spec for the PATCH operation
cat "components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml" | head -200

Repository: wso2/identity-api-server

Length of output: 6233

🏁 Script executed:

# Search for PATCH operation in the spec
rg -A 20 'patch:' "components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml"

Repository: wso2/identity-api-server

Length of output: 777

🏁 Script executed:

# Check for test files that demonstrate expected property removal behavior
fd -e java -path '*test*' | xargs rg -l 'updateEmailSender|NotificationSenderManagementService' 2>/dev/null | head -10

Repository: wso2/identity-api-server

Length of output: 2542

🏁 Script executed:

# Look at the broader context - check if there's any null handling or clearing logic for properties
cat -n "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" | sed -n '340,390p'

Repository: wso2/identity-api-server

Length of output: 3275

🏁 Script executed:

# Get the complete PATCH operation definition including requestBody
rg -A 40 'patch:' "components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml"

Repository: wso2/identity-api-server

Length of output: 1372

🏁 Script executed:

# Check v1 implementation for property handling
cat "components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v1/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v1/core/NotificationSenderManagementService.java" | grep -A 20 "updateEmailSenderDTOFromPatchRequest" | head -30

Repository: wso2/identity-api-server

Length of output: 50

🏁 Script executed:

# Check if there's any documentation or comments about property behavior
rg -B 5 -A 10 "properties.*forEach|properties.*put" "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"

Repository: wso2/identity-api-server

Length of output: 4075

🏁 Script executed:

# Search for any comments or documentation about property behavior in the context of PATCH
rg -B 10 'updateEmailSenderDTOFromPatchRequest' "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" | head -20

Repository: wso2/identity-api-server

Length of output: 839

🏁 Script executed:

# Check the schema definition for EmailSenderUpdateRequest to understand property semantics
rg -A 30 'EmailSenderUpdateRequest:' "components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml"

Repository: wso2/identity-api-server

Length of output: 857

The property merge logic does not support property removal via PATCH requests.

The forEach loop (lines 375-381) only adds or updates properties; there is no mechanism to remove existing properties. This is inconsistent with standard PATCH semantics where clients should be able to modify properties completely. Consider whether the API should support property removal by:

  • Allowing null/empty property values to signal deletion, or
  • Implementing a separate removal mechanism

Note that scalar fields use conditional preservation (value != null ? value : existing), but properties use unconditional addition, creating an inconsistency in update semantics.

🤖 Prompt for AI Agents 
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
around lines 375-381: the current property merge only adds/updates entries and
never removes properties, which breaks PATCH semantics; update the merge logic
so that each incoming prop with a blank or null value removes the key from
existingEmailSenderDTO.getProperties(), while non-blank values continue to
upsert, or alternatively accept an explicit removal list in the request and
apply removals before additions—implement one of these behaviors and make it
consistent with how scalar fields are conditionally preserved.

if (BASIC.equalsIgnoreCase(existingEmailSenderDTO.getAuthType())) {
existingEmailSenderDTO.getProperties().put(USERNAME, existingEmailSenderDTO.getUsername());
existingEmailSenderDTO.getProperties().put(PASSWORD, existingEmailSenderDTO.getPassword());
existingEmailSenderDTO.setUsername(null);
existingEmailSenderDTO.setPassword(null);
}
Comment on lines +382 to +387
Copy link

@coderabbitai coderabbitai bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Potential credential leak when switching auth types.

When the auth type is changed from BASIC to another type (e.g., OAUTH), the old username/password credentials remain in the properties map since this cleanup only executes when the final authType is BASIC. Old credentials could be returned in API responses or persisted unnecessarily.

Consider adding cleanup logic before the BASIC check:

+        // Clean up BASIC auth credentials if switching away from BASIC
+        if (!BASIC.equalsIgnoreCase(existingEmailSenderDTO.getAuthType())) {
+            existingEmailSenderDTO.getProperties().remove(USERNAME);
+            existingEmailSenderDTO.getProperties().remove(PASSWORD);
+        }
+
         if (BASIC.equalsIgnoreCase(existingEmailSenderDTO.getAuthType())) {
             existingEmailSenderDTO.getProperties().put(USERNAME, existingEmailSenderDTO.getUsername());
             existingEmailSenderDTO.getProperties().put(PASSWORD, existingEmailSenderDTO.getPassword());
             existingEmailSenderDTO.setUsername(null);
             existingEmailSenderDTO.setPassword(null);
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (BASIC.equalsIgnoreCase(existingEmailSenderDTO.getAuthType())) {
existingEmailSenderDTO.getProperties().put(USERNAME, existingEmailSenderDTO.getUsername());
existingEmailSenderDTO.getProperties().put(PASSWORD, existingEmailSenderDTO.getPassword());
existingEmailSenderDTO.setUsername(null);
existingEmailSenderDTO.setPassword(null);
}
// Clean up BASIC auth credentials if switching away from BASIC
if (!BASIC.equalsIgnoreCase(existingEmailSenderDTO.getAuthType())) {
existingEmailSenderDTO.getProperties().remove(USERNAME);
existingEmailSenderDTO.getProperties().remove(PASSWORD);
}
if (BASIC.equalsIgnoreCase(existingEmailSenderDTO.getAuthType())) {
existingEmailSenderDTO.getProperties().put(USERNAME, existingEmailSenderDTO.getUsername());
existingEmailSenderDTO.getProperties().put(PASSWORD, existingEmailSenderDTO.getPassword());
existingEmailSenderDTO.setUsername(null);
existingEmailSenderDTO.setPassword(null);
}
🤖 Prompt for AI Agents 
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
lines 382-387: the current logic only moves username/password into properties
when authType is BASIC, which leaves stale credentials in properties if authType
is changed away from BASIC; before handling the BASIC branch, remove any
existing USERNAME and PASSWORD entries from
existingEmailSenderDTO.getProperties() and also clear
existingEmailSenderDTO.setUsername(null)/setPassword(null) when switching away
so no old credentials remain in properties or DTO fields; add this cleanup at
the start of the auth-type handling to ensure credentials are never retained for
non-BASIC types.

return existingEmailSenderDTO;
}

private EmailSender buildEmailSenderFromDTO(EmailSenderDTO dto) {

EmailSender emailSender = new EmailSender();
Expand Down
11 changes: 11 additions & 0 deletions ...on/identity/api/server/notification/sender/v2/impl/NotificationSendersApiServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,17 @@ public Response getSMSSenders() {
return Response.ok().entity(notificationSenderManagementService.getSMSSenders()).build();
}

@Override
public Response patchEmailSender(String senderName, EmailSenderUpdateRequest emailSenderUpdateRequest) {

if (StringUtils.equals(getTenantDomainFromContext(), MultitenantConstants.SUPER_TENANT_DOMAIN_NAME)) {
return Response.status(Response.Status.METHOD_NOT_ALLOWED).build();
}
return Response.ok()
.entity(notificationSenderManagementService.patchEmailSender(senderName, emailSenderUpdateRequest))
.build();
}

@Override
public Response updateEmailSender(String senderName, EmailSenderUpdateRequest emailSenderUpdateRequest) {

Expand Down
67 changes: 67 additions & 0 deletions ...on.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,73 @@ paths:
application/json:
schema:
$ref: '#/components/schemas/Error'
patch:
tags:
- Email Senders
summary: Update an email sender
description: |
This API provides the capability to update an email sender by name.
The URL encoded email sender name is used as sender-name.<br>
<b>Permission required:</b> <br>
* /permission/admin/manage/identity/configmgt/update <br>
<b>Scope required:</b> <br>
* internal_config_mgt_update
operationId: patchEmailSender
parameters:
- name: sender-name
in: path
description: name of the email sender
required: true
style: simple
explode: false
schema:
type: string
requestBody:
content:
application/json:
schema:
$ref: '#/components/schemas/EmailSenderUpdateRequest'
example:
fromAddress: iam@gmail.com
properties:
- key: mail.smtp.starttls.enable
value: true
required: true
responses:
"200":
description: Successful Response
content:
application/json:
schema:
$ref: '#/components/schemas/EmailSender'
"400":
description: Bad Request
content:
application/json:
schema:
$ref: '#/components/schemas/Error'
"401":
description: Unauthorized
"403":
description: Forbidden
"404":
description: Not Found
content:
application/json:
schema:
$ref: '#/components/schemas/Error'
"405":
description: Method Not Allowed.
content:
application/json:
schema:
$ref: '#/components/schemas/Error'
"500":
description: Server Error
content:
application/json:
schema:
$ref: '#/components/schemas/Error'
delete:
tags:
- Email Senders
Expand Down
Loading