Add device registration support and email template for notifications#390
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a push device registration notification feature: new scenario, template enum, and placeholder constants are defined in ChangesPush Device Registration Notification
Sequence DiagramsequenceDiagram
participant EventData as Push Event
participant Validate as validatePushEventProperties()
participant BuildData as buildPushNotificationData()
participant GetPlaceholders as getPlaceHolderValues()
participant Builder as PushNotificationData.Builder
EventData->>Validate: check DEVICE_REGISTRATION scenario
Validate->>Validate: require device name/model/time properties
EventData->>BuildData: populate builder from properties
BuildData->>GetPlaceholders: fetch placeholder values
GetPlaceholders->>GetPlaceholders: normalize null to EMPTY
GetPlaceholders-->>BuildData: placeholder map
BuildData->>Builder: set pushDeviceName, pushDeviceModel, registrationTime
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PMD (7.25.0)components/event-handler-notification/org.wso2.carbon.identity.event.handler.notification/src/main/java/org/wso2/carbon/identity/event/handler/notification/PushNotificationHandler.java[ERROR] Cannot load ruleset .coderabbit-pmd-ruleset.xml: Cannot resolve rule/ruleset reference '.coderabbit-pmd-ruleset.xml'. Make sure the resource is a valid file or URL and is on the CLASSPATH. Use --debug (or a fine log level) to see the current classpath. 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: 5
🤖 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/event-handler-notification/org.wso2.carbon.identity.event.handler.notification/src/main/java/org/wso2/carbon/identity/event/handler/notification/NotificationConstants.java`:
- Line 205: The last enum constant REGISTRATION_TIME in the
PushNotificationPlaceholder enum has invalid syntax with a trailing comma before
the semicolon (,;). Remove the trailing comma after
REGISTRATION_TIME("registration-time") so that only the semicolon remains,
making it REGISTRATION_TIME("registration-time");
In
`@components/event-handler-notification/org.wso2.carbon.identity.event.handler.notification/src/main/java/org/wso2/carbon/identity/event/handler/notification/PushNotificationHandler.java`:
- Around line 324-329: The setPushDeviceName, setPushDeviceModel, and
setRegistrationTime methods are receiving values directly from eventProperties
without validation, which can result in null values being set. These null values
will cause a NullPointerException when placeholder replacement occurs later at
line 392 during String#replace operations. Add validation in the
validatePushEventProperties method to check that the
NotificationConstants.PushNotificationPlaceholder.PUSH_DEVICE_NAME,
PUSH_DEVICE_MODEL, and REGISTRATION_TIME fields are present and not null when
the event scenario is DEVICE_REGISTRATION, and throw an appropriate exception if
any are missing to prevent the event flow from reaching the placeholder
replacement logic with incomplete data.
In
`@features/org.wso2.carbon.email.mgt.server.feature/resources/email-admin-config.xml`:
- Line 26127: The XML document has duplicate id attributes with the value
templateBody appearing multiple times, which violates XML/HTML uniqueness
requirements and breaks DOM and CSS targeting in mail clients. Locate all
instances of id="templateBody" in the email-admin-config.xml file (specifically
at the noted line ranges) and make each ID unique by appending a descriptive
suffix or number to each occurrence, such as templateBody1, templateBody2, etc.,
ensuring each id attribute value appears only once in the document.
- Line 26128: The style attribute contains an invalid CSS unit for the
padding-top property. Change `padding-top: 0p` to `padding-top: 0` or
`padding-top: 0px` since "p" is not a valid CSS unit. The zero value is valid
without a unit, or use the standard pixel unit "px" instead.
- Around line 26104-26110: The device metadata placeholders
({{push-device-name}} and {{push-device-model}}) in the email template are being
inserted into HTML content without proper HTML entity encoding. The replaceTags
method in Notification.java currently uses Matcher.quoteReplacement() which only
escapes regex special characters but not HTML entities. Apply HTML entity
encoding (such as StringEscapeUtils.escapeHtml4() from Apache Commons Lang) to
the device metadata values before they are substituted into the email body in
the replaceTags method or at the point where PushNotificationHandler.java
populates these values into event properties. This prevents HTML/JavaScript
injection attacks when unescaped user-controllable device names or models are
rendered in email clients.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a44f6076-0dd3-4d41-9dda-396178943a23
📒 Files selected for processing (3)
components/event-handler-notification/org.wso2.carbon.identity.event.handler.notification/src/main/java/org/wso2/carbon/identity/event/handler/notification/NotificationConstants.javacomponents/event-handler-notification/org.wso2.carbon.identity.event.handler.notification/src/main/java/org/wso2/carbon/identity/event/handler/notification/PushNotificationHandler.javafeatures/org.wso2.carbon.email.mgt.server.feature/resources/email-admin-config.xml
| <li><b>Device name:</b> {{push-device-name}}</li> | ||
| <li><b>Model:</b> {{push-device-model}}</li> | ||
| <li> | ||
| <b>Registered at:</b> {{registration-time}} | ||
| </li> | ||
| </ul> | ||
| </td> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: locate rendering and escaping path for email placeholders.
set -e
echo "== Find placeholder and scenario wiring =="
rg -n -C3 'push-device-name|push-device-model|registration-time|PushDeviceRegistration|PUSH_DEVICE_REGISTRATION_SCENARIO'
echo
echo "== Find notification template rendering/placeholder replacement code =="
rg -n -C3 'replace|interpolate|placeholder|mustache|velocity|template engine|escapeHtml|htmlEscape|StringEscapeUtils'
echo
echo "== Focus on handler path for device registration payload construction =="
rg -n -C5 'buildPushNotificationData|pushDeviceName|pushDeviceModel|registrationTime|event properties'Repository: wso2-extensions/identity-event-handler-notification
Length of output: 50408
🏁 Script executed:
#!/bin/bash
# Find where placeholders are actually replaced/interpolated in templates
echo "== Find template rendering/interpolation implementation =="
rg -n "resolveTemplate|renderTemplate|interpolate|replaceAll|replace.*placeholder|setBody|setContent" -A 5 --max-count=20
echo
echo "== Find PushNotificationHandler and device registration data flow =="
fd -t f "PushNotificationHandler\|DeviceRegistration" --max-results=10
rg -n "PushNotificationHandler\|buildNotification\|buildEventProperties" -B 2 -A 8 --max-count=15
echo
echo "== Check for HTML escaping in NotificationUtil =="
rg -n "htmlEscape|StringEscapeUtils|HtmlUtils|Apache.*escape|escape.*Html" -B 2 -A 5Repository: wso2-extensions/identity-event-handler-notification
Length of output: 50407
🏁 Script executed:
#!/bin/bash
# Search more specifically for template body rendering
echo "== Find where email body/template is rendered with placeholders =="
rg -n "setBody|setText|setHtmlBody|emailTemplate\.getBody|template\.getBody" -B 3 -A 8 --max-count=25
echo
echo "== Look for velocity, freemarker, or mustache templating =="
rg -n "VelocityEngine|Template\.process|evaluate|render" -B 2 -A 5 --max-count=20Repository: wso2-extensions/identity-event-handler-notification
Length of output: 50407
🏁 Script executed:
#!/bin/bash
# Find the complete replaceTags method in Notification.java
fd -t f "Notification.java" -x rg -l "class Notification|replaceTags" {} \; 2>/dev/null | head -1 | xargs cat -n | head -150
echo
echo "== Search for replaceTags method signature and full implementation =="
rg -n "private.*replaceTags|public.*replaceTags" -A 30 components/event-handler-notification/Repository: wso2-extensions/identity-event-handler-notification
Length of output: 12399
🏁 Script executed:
#!/bin/bash
# Find where device properties come from - look at device registration event handlers
echo "== Find device registration event handling =="
fd -t f -i "device.*registration\|registration.*handler" --max-results=10
echo
echo "== Search for where push-device-name and push-device-model are populated in event properties =="
rg -n "push-device-name|push-device-model|PUSH_DEVICE_NAME|PUSH_DEVICE_MODEL" -B 5 -A 5 \
components/event-handler-notification/ --max-count=20Repository: wso2-extensions/identity-event-handler-notification
Length of output: 8855
Confirm: Device metadata placeholders require HTML escaping before email rendering.
The replaceTags method in Notification.java (line 70-71) uses Matcher.quoteReplacement(), which only escapes regex special characters (backslashes and dollar signs) but does NOT perform HTML escaping. Device metadata values for {{push-device-name}} and {{push-device-model}} are sourced from event properties (PushNotificationHandler.java, lines 324-327) and inserted directly into the email body without HTML entity encoding. Since the email content type is text/html, unescaped user-controllable device names/models could enable HTML/JavaScript injection in email clients that render the content.
Recommendation: Apply HTML entity encoding (e.g., StringEscapeUtils.escapeHtml4() from Apache Commons Lang) to device metadata values before placeholder substitution, or use a templating engine that provides context-aware auto-escaping.
🤖 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
`@features/org.wso2.carbon.email.mgt.server.feature/resources/email-admin-config.xml`
around lines 26104 - 26110, The device metadata placeholders
({{push-device-name}} and {{push-device-model}}) in the email template are being
inserted into HTML content without proper HTML entity encoding. The replaceTags
method in Notification.java currently uses Matcher.quoteReplacement() which only
escapes regex special characters but not HTML entities. Apply HTML entity
encoding (such as StringEscapeUtils.escapeHtml4() from Apache Commons Lang) to
the device metadata values before they are substituted into the email body in
the replaceTags method or at the point where PushNotificationHandler.java
populates these values into event properties. This prevents HTML/JavaScript
injection attacks when unescaped user-controllable device names or models are
rendered in email clients.
| > | ||
| <!-- BEGIN BODY // --> | ||
| <table | ||
| id="templateBody" |
There was a problem hiding this comment.
Use unique IDs instead of repeating templateBody.
id="templateBody" is declared twice in the same document, which is invalid HTML and can cause inconsistent CSS/DOM targeting in mail clients.
Suggested fix
- <table
- id="templateBody"
+ <table
+ id="templateFooter"Also applies to: 26257-26257
🤖 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
`@features/org.wso2.carbon.email.mgt.server.feature/resources/email-admin-config.xml`
at line 26127, The XML document has duplicate id attributes with the value
templateBody appearing multiple times, which violates XML/HTML uniqueness
requirements and breaks DOM and CSS targeting in mail clients. Locate all
instances of id="templateBody" in the email-admin-config.xml file (specifically
at the noted line ranges) and make each ID unique by appending a descriptive
suffix or number to each occurrence, such as templateBody1, templateBody2, etc.,
ensuring each id attribute value appears only once in the document.
raviendalpatadu
force-pushed
the
feature/multiple-device-support-for-push-auth
branch
from
614625f to
364de0e
Compare
3 participants
This pull request adds support for push notification templates and placeholders related to device registration events. It introduces new constants, templates, and data fields to handle notifications when a new device is registered to a user's account.
Push Notification Template and Placeholder Enhancements:
PushNotification(PUSH_DEVICE_REGISTRATION_SCENARIO).PushNotificationTemplatefor device registration, including a title and message with placeholders for device name, model, and registration time.push-device-name,push-device-model,registration-time) toPushNotificationPlaceholderfor use in device registration notifications.Push Notification Data Handling:
buildPushNotificationDatainPushNotificationHandler.javato populate the new device registration fields (pushDeviceName,pushDeviceModel,registrationTime) from event properties.Related Issues
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes