Refactor password reset link domain flow - #13209
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13209 +/- ##
==========================================
Coverage 18.08% 18.09%
- Complexity 16718 16730 +12
==========================================
Files 6037 6037
Lines 542611 542803 +192
Branches 66433 66465 +32
==========================================
+ Hits 98136 98224 +88
- Misses 433448 433530 +82
- Partials 11027 11049 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@winterhazel a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18204 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16326)
|
There was a problem hiding this comment.
🟡 Changes recommended
The new flow trusts a request-derived host value for password-reset link generation without sufficient validation/port-scheme handling and lacks unit tests for the added selection/formatting branches.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
server/src/main/java/org/apache/cloudstack/user/UserPasswordResetManagerImpl.java:235
- Avoid calling
UserPasswordResetDomainURL.value()twice in the same branch; use the already-fetchedconfigurationDomainfor the log message to prevent redundant config lookups and potential inconsistencies if the value changes concurrently.
String configurationDomain = UserPasswordResetDomainURL.value();
if (StringUtils.isNotBlank(configurationDomain)) {
logger.debug("Defaulting reset link's domain to the [{}] configuration value: [{}].", UserPasswordResetDomainURL.key(), UserPasswordResetDomainURL.value());
return configurationDomain;
}
server/src/main/java/org/apache/cloudstack/user/UserPasswordResetManagerImpl.java:225
commonNameDetailsis aList<Long>of GUI theme IDs, not details. Renaming it makes the intent clearer and avoids confusion when reading the domain-selection logic and logs.
logger.debug("Searching for GUI theme with common name that matches the request's domain: [{}]", requestDomain);
List<Long> commonNameDetails = guiThemeDetailsDao.listGuiThemeIdsByCommonName(requestDomain);
if (!commonNameDetails.isEmpty()) {
logger.debug("GUI theme with ID {} was found; using request's domain for password reset link.", commonNameDetails.get(0));
return requestDomain;
server/src/main/java/org/apache/cloudstack/user/UserPasswordResetManagerImpl.java:251
- Scheme detection in
formatResetLinkDomainusesstartsWith("http"), which is both overly broad and differs from the previous explicithttp:///https://checks (e.g., it would treathttpx://...as already having a scheme). Prefer explicitly checkinghttp://andhttps://.
if (!resetLinkDomain.startsWith("http")) {
resetLinkDomain = protocol + "://" + resetLinkDomain;
}
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Lite
| HttpUtils.RESPONSE_TYPE_XML, ApiServer.JSONcontentType.value()); | ||
| return; | ||
| } | ||
| CallContext.current().setRequestRemoteAddress(req.getServerName()); |
| String requestDomain = CallContext.current().getRequestRemoteAddress(); | ||
| String resetLinkDomain = getResetLinkDomain(requestDomain); | ||
| String formattedResetLinkDomain = formatResetLinkDomain(resetLinkDomain); | ||
| String resetLink = String.format("%s/client/#/user/resetPassword?username=%s&token=%s", | ||
| domainUrl, username, resetToken); | ||
| formattedResetLinkDomain, username, resetToken); |
| private String requestRemoteAddress; | ||
|
|
| private String getResetLinkDomain(String requestDomain) { | ||
| if (StringUtils.isNotBlank(requestDomain)) { | ||
| logger.debug("Searching for GUI theme with common name that matches the request's domain: [{}]", requestDomain); | ||
| List<Long> commonNameDetails = guiThemeDetailsDao.listGuiThemeIdsByCommonName(requestDomain); | ||
|
|
||
| if (!commonNameDetails.isEmpty()) { | ||
| logger.debug("GUI theme with ID {} was found; using request's domain for password reset link.", commonNameDetails.get(0)); | ||
| return requestDomain; | ||
| } else { | ||
| logger.debug("No GUI theme was found with a common name that matches the request's domain."); | ||
| } | ||
| } | ||
|
|
||
| String configurationDomain = UserPasswordResetDomainURL.value(); | ||
| if (StringUtils.isNotBlank(configurationDomain)) { | ||
| logger.debug("Defaulting reset link's domain to the [{}] configuration value: [{}].", UserPasswordResetDomainURL.key(), UserPasswordResetDomainURL.value()); | ||
| return configurationDomain; | ||
| } | ||
|
|
||
| logger.debug("Using the first IP address in the [{}] configuration for the reset password email domain because the [{}] configuration is not defined.", ManagementServerAddresses.key(), UserPasswordResetDomainURL.key()); | ||
| return ManagementServerAddresses.value().split(",")[0]; | ||
| } | ||
|
|
||
| private String formatResetLinkDomain(String resetLinkDomain) { | ||
| String protocol = ServerProperties.isHttpsEnabled() ? "https" : "http"; | ||
|
|
||
| if (InetAddressUtils.isIPv4Address(resetLinkDomain)) { | ||
| int port = protocol.equals("https") ? ServerProperties.getHttpsPort() : ServerProperties.getHttpPort(); | ||
| resetLinkDomain = resetLinkDomain + ":" + port; | ||
| } | ||
|
|
||
| if (!resetLinkDomain.startsWith("http")) { | ||
| resetLinkDomain = protocol + "://" + resetLinkDomain; | ||
| } | ||
|
|
||
| return resetLinkDomain.replaceAll("/+$", ""); | ||
| } |
Description
Currently, when a user tries to reset their password, an email is sent with a link that leads to a page where the user can redefine its password. However, this link uses the first value of the
hostconfiguration as the Management Server's domain. For some environments, it may not be interesting to expose the Management Server's IP in the URL.In order to fix this, the Management Server's domain definition workflow was refactored. Now, the request's domain is obtained. Then, GUI themes whose common names match the request's domain are fetched (being the common name a wildcard or the domain itself). If a theme is found, the request's domain is used for the password reset link.
Nevertheless, if no theme is found, the value of the
user.password.reset.mail.domain.urlglobal configuration is obtained. If the configuration has a defined value, it is used as the email's domain. But, if no value is defined, the current behavior is maintained and the first IP address of thehostconfiguration is used.With the refactoring, logs were also added to the domain selection process, allowing an easier troubleshooting process.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Screenshots (if appropriate):
How Has This Been Tested?
Without any GUI theme and the
user.password.reset.mail.domain.urlset asnull, I requested a password reset link. Then, I validated that thehostconfiguration first IP was used, and the right protocol and port were set automatically.Management's IP test
Then, I set the
user.password.reset.mail.domain.urlconfiguration to another domain and requested a new password reset link. After checking the email, It was possible to observe that the configuration's value was used.Configuration test
At last, I created a GUI theme with a common name and also mapped it to my local
/etc/hostsfile. I used the configured common name to access the application login page, and requested a password reset link. When I received it, it was possible to observe that the reset link's domain was the GUI theme's configured common name.GUI theme test