Fix url in password reset email #12078
Conversation
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12078 +/- ##
=========================================
Coverage 17.56% 17.56%
Complexity 15538 15538
=========================================
Files 5909 5909
Lines 529099 529102 +3
Branches 64623 64624 +1
=========================================
+ Hits 92913 92914 +1
- Misses 425732 425734 +2
Partials 10454 10454
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a regression in the password reset email URL functionality introduced in PR #11379. The URL construction was broken because the domain URL was being included twice in the email template.
Key changes:
- Consolidated URL construction logic to build the complete reset link in code rather than in the email template
- Added fallback to use ManagementServerAddresses when UserPasswordResetDomainURL is not configured
- Added trailing slash removal to ensure clean URL formatting
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
server/src/main/java/org/apache/cloudstack/user/UserPasswordResetManagerImpl.java
Show resolved
Hide resolved
server/src/main/java/org/apache/cloudstack/user/UserPasswordResetManagerImpl.java
Outdated
Show resolved
Hide resolved
…setManagerImpl.java Co-authored-by: Copilot <[email protected]>
|
@blueorangutan package |
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15767 |
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
looks generally good, one question.
| if (StringUtils.isBlank(domainUrl)) { | ||
| domainUrl = ManagementServerAddresses.value().split(",")[0]; | ||
| } | ||
| domainUrl = domainUrl.trim().replaceAll("/+$", ""); |
There was a problem hiding this comment.
yes @DaanHoogland , in case the domain url setting has value like "https://xyz.com/" , trail / are not needed as it's considered while formatting the reset link.
There was a problem hiding this comment.
but it will not be in the way either, will it?
There was a problem hiding this comment.
Good point, @DaanHoogland. While the extra / usually won’t cause issues, it could create problems in environments where admins use a reverse proxy with strict access rules for the reset-password URL.
For example, some organization could only allow access to password-reset endpoints from the office network or over the company VPN. This would reduce the attack surface to a very sensitive endpoint that is often forgotten.
|
@blueorangutan test |
|
@sureshanaparti a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian Build Failed (tid-14834) |
4 participants
Description
This PR fixes the url in password reset email. (regression from #11379)
Fixes #12050
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?