Fix NPE during reset password#12585
Conversation
|
@blueorangutan package |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.20 #12585 +/- ##
============================================
- Coverage 16.26% 16.25% -0.02%
+ Complexity 13428 13419 -9
============================================
Files 5660 5662 +2
Lines 499963 500149 +186
Branches 60708 60731 +23
============================================
- Hits 81330 81298 -32
- Misses 409559 409768 +209
- Partials 9074 9083 +9
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:
|
|
@blueorangutan package |
|
@DaanHoogland 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 16706 |
|
@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-15383)
|
|
@sureshanaparti as disscussed currently there is no exception message thrown when a ldap/saml user tries to click on forgot password logs |
@kiranchavala updated logs as discussed. This is usually logged when forgot password called without any username, to show/hide the ui button "Forgot password?" in login form (when the feature in enabled/disabled respectively) - added check to not log it to avoid any confusion. cloudstack/ui/src/views/auth/Login.vue Lines 318 to 326 in e22f842 |
|
@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 16827 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian Build Failed (tid-15457) |
|
@blueorangutan test |
|
@RosiKyu a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian Build Failed (tid-15470) |
|
[SF] Trillian test result (tid-15459)
|
There was a problem hiding this comment.
Pull request overview
Fixes an NPE encountered during the password reset/forgot-password flow (notably when no HTTP session exists) and adds guardrails for password reset operations for externally-managed users (SAML/OAuth2/LDAP).
Changes:
- Prevent
ApiServlet.invalidateHttpSessionfrom throwing whenHttpSessionis null. - Block
forgotPasswordfor users sourced from SAML2/OAUTH2/LDAP and add related logging. - Minor parameter parsing hardening/cleanup in login authenticators and SAML2 login.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/main/java/com/cloud/api/auth/DefaultResetPasswordAPIAuthenticatorCmd.java | Minor formatting cleanup. |
| server/src/main/java/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java | Refactors domain param parsing for login. |
| server/src/main/java/com/cloud/api/auth/DefaultForgotPasswordAPIAuthenticatorCmd.java | Introduces APINAME const; blocks forgot-password for external auth sources; adds debug logging. |
| server/src/main/java/com/cloud/api/ApiServlet.java | Avoid NPE by allowing null session in invalidateHttpSession; tweaks auth failure logging. |
| plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java | Adds null/length checks when reading array params. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/OauthLoginAPIAuthenticatorCmd.java | Refactors domainId param parsing. |
Comments suppressed due to low confidence (1)
server/src/main/java/com/cloud/api/auth/DefaultForgotPasswordAPIAuthenticatorCmd.java:120
_accountService.getActiveUserAccount(...)can return null; the code then calls_apiServer.forgotPassword(userAccount, userDomain)which dereferencesuserAccountand will throw an NPE. Please handle the null case explicitly (either return a generic success response without sending an email, or throw a controlledServerApiException).
final UserAccount userAccount = _accountService.getActiveUserAccount(username[0], domainId);
if (userAccount != null && List.of(User.Source.SAML2, User.Source.OAUTH2, User.Source.LDAP).contains(userAccount.getSource())) {
logger.debug("Forgot Password is not allowed for the user {} from source {}", username[0], userAccount.getSource());
throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Forgot Password is not allowed for this user");
}
boolean success = _apiServer.forgotPassword(userAccount, userDomain);
logger.debug("Forgot password request for user " + username[0] + " in domain " + domain + " is successful: " + success);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| domainIdArr = (String[])params.get(ApiConstants.DOMAIN__ID); | ||
| } | ||
| final String[] domainName = (String[])params.get(ApiConstants.DOMAIN); | ||
| final String[] domainIdArr = (String[])params.get(ApiConstants.DOMAIN_ID); |
There was a problem hiding this comment.
domainId (ApiConstants.DOMAIN__ID) is still an accepted/annotated parameter name for login, but this code now only reads domainid (ApiConstants.DOMAIN_ID). This is a backward-incompatible change: requests sending domainId will no longer resolve the domain and may fail authentication. Please restore the fallback to also read ApiConstants.DOMAIN__ID (or normalize parameter keys in one place).
| final String[] domainIdArr = (String[])params.get(ApiConstants.DOMAIN_ID); | |
| String[] domainIdArr = (String[])params.get(ApiConstants.DOMAIN_ID); | |
| if (domainIdArr == null || domainIdArr.length == 0) { | |
| domainIdArr = (String[])params.get(ApiConstants.DOMAIN__ID); | |
| } |
| @@ -177,12 +177,8 @@ private String doOauthAuthentication(HttpSession session, Long domainId, String | |||
|
|
|||
| protected Long getDomainIdFromParams(Map<String, Object[]> params, StringBuilder auditTrailSb, String responseType) { | |||
| String[] domainIdArr = (String[])params.get(ApiConstants.DOMAIN_ID); | |||
There was a problem hiding this comment.
getDomainIdFromParams now only checks ApiConstants.DOMAIN_ID (domainid) but this API command also declares/accepts ApiConstants.DOMAIN__ID (domainId). Removing the fallback breaks clients using domainId. Please handle both keys (or centralize key normalization) to keep compatibility.
| String[] domainIdArr = (String[])params.get(ApiConstants.DOMAIN_ID); | |
| String[] domainIdArr = (String[])params.get(ApiConstants.DOMAIN_ID); | |
| if (domainIdArr == null) { | |
| // Fallback to support clients using the camelCase parameter name "domainId" | |
| domainIdArr = (String[])params.get(ApiConstants.DOMAIN__ID); | |
| } |
| try { | ||
| final Domain userDomain = _domainService.findDomainByPath(domain); | ||
| if (userDomain != null) { | ||
| domainId = userDomain.getId(); | ||
| } else { | ||
| logger.debug("Unable to find the domain from the path {}", domain); | ||
| throw new ServerApiException(ApiErrorCode.PARAM_ERROR, String.format("Unable to find the domain from the path %s", domain)); | ||
| } |
There was a problem hiding this comment.
The parameter description says that if no domain is passed, ROOT (/) is assumed, but this implementation calls _domainService.findDomainByPath(domain) and throws when domain is null/blank. Please default to ROOT when domain is missing (e.g., use findDomainByIdOrPath(null, domain) or explicitly set domainId to Domain.ROOT_DOMAIN).
kiranchavala
left a comment
kiranchavala
left a comment
There was a problem hiding this comment.
LGTM
Tested manually , try forgot password on ldap user account ,
No NPE found
2026-02-18 12:04:36,869 DEBUG [c.c.a.ApiServlet] (qtp698741991-23:[ctx-654d9184]) (logid:ff1c91d0) ===START=== 10.0.3.251 -- POST command=forgotPassword&response=json
2026-02-18 12:04:36,872 DEBUG [c.c.a.a.DefaultForgotPasswordAPIAuthenticatorCmd] (qtp698741991-23:[ctx-654d9184]) (logid:ff1c91d0) Forgot Password is not allowed for the user kiran from source LDAP
2026-02-18 12:04:36,873 DEBUG [c.c.a.ApiServlet] (qtp698741991-23:[ctx-654d9184]) (logid:ff1c91d0) ===END=== 10.0.3.251 -- POST command=forgotPassword&response=json
2026-02-18 12:04:36,892 INFO [c.c.c.ClusterManagerImpl] (Cluster-Heartbeat-1:[ctx-33571f8a]) (logid:9e1f1b1a) No inactive management server node found
2026-02-18 12:04:36,893 DEBUG [c.c.c.ClusterManagerImpl] (Cluster-Heartbeat-1:[ctx-33571f8a]) (logid:9e1f1b1a) Peer scan is finished. profiler: Done. Duration: 1ms , profilerQueryActiveList: Done. Duration: 0ms, , profilerSyncClusterInfo: Done. Duration: 0ms, profilerInvalidatedNodeList: Done. Duration: 0ms, profilerRemovedList: Done. Duration: 0ms,, profilerNewList: Done. Duration: 0ms, profilerInactiveList: Done. Duration: 0ms
Normal user
2026-02-18 12:06:47,284 DEBUG [c.c.a.ApiServlet] (qtp698741991-22:[ctx-edc24db8]) (logid:3f0a9791) ===START=== 10.0.3.251 -- POST command=forgotPassword&response=json
2026-02-18 12:06:47,397 INFO [c.c.c.ClusterManagerImpl] (Cluster-Heartbeat-1:[ctx-dc0dc52a]) (logid:065d5f24) No inactive management server node found
2026-02-18 12:06:47,397 DEBUG [c.c.c.ClusterManagerImpl] (Cluster-Heartbeat-1:[ctx-dc0dc52a]) (logid:065d5f24) Peer scan is finished. profiler: Done. Duration: 4ms , profilerQueryActiveList: Done. Duration: 0ms, , profilerSyncClusterInfo: Done. Duration: 2ms, profilerInvalidatedNodeList: Done. Duration: 0ms, profilerRemovedList: Done. Duration: 0ms,, profilerNewList: Done. Duration: 0ms, profilerInactiveList: Done. Duration: 1ms
2026-02-18 12:06:47,400 DEBUG [o.a.c.h.H.HAManagerBgPollTask] (BackgroundTaskPollManager-3:[ctx-6359616b]) (logid:10e168a2) HA health check task is running...
2026-02-18 12:06:47,447 DEBUG [o.a.c.u.UserPasswordResetManagerImpl] (qtp698741991-22:[ctx-edc24db8]) (logid:3f0a9791) User password reset email for user UserAccount {"accountName":"kirannormal","id":6,"username":"kirannormal"}. account id: 6 domain id: 1 sent to test@example.com with token expiry at 2026-02-18T12:36:47.290+0000
2026-02-18 12:06:47,450 DEBUG [c.c.a.a.DefaultForgotPasswordAPIAuthenticatorCmd] (qtp698741991-22:[ctx-edc24db8]) (logid:3f0a9791) Forgot password request for user kirannormal in domain / is successful: true
|
it looks like simulator CI and smoke test failure are caused by this 
can you fix it @sureshanaparti ? |
@DaanHoogland @abh1sar fixed here - #12689 |
9 participants
Description
This PR fixes the NPE during reset password, and has some code improvements. Failed to throw proper error when there is no session while resetting password.
Fixes #12582
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?