Skip to content

Fix unresolved password recovery email placeholders - #1162

Open
thuvarahan-t wants to merge 1 commit into
wso2-extensions:masterfrom
thuvarahan-t:fix-password-recovery-placeholders
Open

thuvarahan-t wants to merge 1 commit into
wso2-extensions:masterfrom
thuvarahan-t:fix-password-recovery-placeholders

Conversation

@thuvarahan-t

@thuvarahan-t thuvarahan-t commented Sep 13, 2026 •

Copy link
Copy Markdown

Purpose

Addresses wso2/product-is#28439.

Password recovery emails could contain unresolved {{sp}} and {{callback}} placeholders when those optional properties were not included in the password recovery initiation request.

Goals

Ensure optional sp and callback template properties resolve safely when omitted, while preserving supplied values and existing callback validation.

Approach

NotificationPasswordRecoveryManager previously copied only non-blank request metadata into the notification event. When sp or callback was absent, the property was missing entirely and the template engine could leave the placeholder unresolved.

This change:

  • Adds and uses the notification-template property constant for sp.
  • Adds safe empty defaults for missing sp and callback properties with putIfAbsent.
  • Preserves supplied values.
  • Leaves callback validation unchanged.
  • Adds parameterized regression tests for all supplied/missing combinations.

User stories

As a user receiving a password recovery email, I should not see unresolved optional-property placeholders when the initiation request omits sp or callback.

Developer Checklist (Mandatory)

  • Complete the Developer Checklist in the related product-is issue to track any behavioral change or migration impact.

Release note

Prevent unresolved service-provider and callback placeholders in password recovery emails when those optional values are omitted.

Documentation

N/A - this corrects notification template property resolution without changing public APIs or documented configuration.

Training

N/A - no training content impact.

Certification

N/A - no certification exam impact.

Marketing

N/A - no marketing content impact.

Automation tests

  • Unit tests
    • Added four parameterized regression combinations covering sp and callback supplied and missing states.
    • New regression combinations: 4 passed.
    • NotificationPasswordRecoveryManagerTest: 15 passed.
    • Complete org.wso2.carbon.identity.recovery module: 530 passed.
    • git diff --check: passed.
    • Relevant unresolved-placeholder search: passed.
  • Integration tests
    • Not run; the change is covered by unit and module tests.

SpotBugs reports existing unrelated findings; none are introduced on the changed lines.

Security checks

  • Followed secure coding standards in http://wso2.com/technical-reports/wso2-secure-engineering-guidelines? yes
  • Ran FindSecurityBugs plugin and verified report? no - SpotBugs was run and reported only existing unrelated findings; FindSecurityBugs was not separately run.
  • Confirmed that this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets? yes

Samples

N/A - no sample changes are required.

Related PRs

N/A.

Migrations (if applicable)

N/A - no migration is required.

Test environment

Local development environment using the repository's Maven test configuration.

Learning

Reviewed the password recovery notification metadata flow and existing template-property behavior.

Summary by CodeRabbit

  • Bug Fixes
    • Improved password recovery notifications by ensuring service provider and callback information is consistently available, including when these values are not provided.
    • Prevented missing notification properties from causing inconsistent recovery notification processing.

@CLAassistant

CLAassistant commented Sep 13, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented Sep 13, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: d8b441aa-4a8f-4fcf-ab82-37f0a4e54efc

📥 Commits

Reviewing files that changed from the base of the PR and between 230dd27 and 11410e5.

📒 Files selected for processing (3)
  • components/org.wso2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/IdentityRecoveryConstants.java
  • components/org.wso2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/password/NotificationPasswordRecoveryManager.java
  • components/org.wso2.carbon.identity.recovery/src/test/java/org/wso2/carbon/identity/recovery/password/NotificationPasswordRecoveryManagerTest.java

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Changes

Recovery notification properties

Layer / File(s) Summary
Property contract and notification defaults
components/org.wso2.carbon.identity.recovery/src/main/java/...
Adds the SERVICE_PROVIDER constant. Defaults missing SERVICE_PROVIDER and CALLBACK event properties to empty strings.
Property normalization validation
components/org.wso2.carbon.identity.recovery/src/test/java/...
Tests four null and non-null property combinations and verifies the captured event properties.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: jayashakthi97

Merge Risk: ⚪ Minimal · up to 11410

Recovery emails now receive empty values for omitted optional properties while preserving supplied values. The change is ready to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: fixing unresolved password recovery email placeholders.
Description check ✅ Passed The description is complete and aligned with the template. It explains the problem, goals, implementation, testing, release impact, and non-applicable areas. The mandatory Developer Checklist remains …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants