You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "sureshanaparti (via GitHub)" <gi...@apache.org> on 2025/07/16 09:59:25 UTC
[PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
sureshanaparti opened a new pull request, #11220:
URL: https://github.com/apache/cloudstack/pull/11220
### Description
This PR marks the LDAP user query timeout as incorrect login instead of disabling user immediately.
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ******************************************************************************* -->
<!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ******************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] build/CI
- [ ] test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [x] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
#### How did you try to break this feature and the system with this change?
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3078249149
Packaging result [SF]: ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 14207
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3077854205
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#discussion_r2209869129
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java:
##########
@@ -140,9 +140,10 @@ public void execute() throws ServerApiException {
try {
final List<LdapUser> users = _ldapManager.getUsers(domainId);
ldapResponses = createLdapUserResponse(users);
-// now filter and annotate
+ // now filter and annotate
ldapResponses = applyUserFilter(ldapResponses);
} catch (final NoLdapUserMatchingQueryException ex) {
+ logger.debug(ex.getMessage());
// ok, we'll make do with the empty list ldapResponses = new ArrayList<LdapUserResponse>();
Review Comment:
The catch block logs the exception but never actually resets `ldapResponses` to a new empty list. Add `ldapResponses = new ArrayList<>();` to ensure you don't return stale or null data.
```suggestion
// ok, we'll make do with the empty list
ldapResponses = new ArrayList<>();
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java:
##########
@@ -303,8 +303,8 @@ public LdapUser getUser(final String username, Long domainId) throws NoLdapUserM
return _ldapUserManagerFactory.getInstance(_ldapConfiguration.getLdapProvider(null)).getUser(escapedUsername, context, domainId);
} catch (NamingException | IOException e) {
- logger.debug("ldap Exception: ",e);
- throw new NoLdapUserMatchingQueryException("No Ldap User found for username: "+username);
+ logger.debug("LDAP Exception: ", e);
+ throw new NoLdapUserMatchingQueryException("Unable to find LDAP User for username: " + username + ", due to " + e.getMessage());
Review Comment:
Wrap the caught exception as the cause when rethrowing to preserve stack trace, for example: `new NoLdapUserMatchingQueryException(message, e);`.
```suggestion
throw new NoLdapUserMatchingQueryException("Unable to find LDAP User for username: " + username + ", due to " + e.getMessage(), e);
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapAuthenticator.java:
##########
@@ -174,7 +176,11 @@ Pair<Boolean, ActionOnFailedAuthentication> authenticate(String username, String
}
} catch (NoLdapUserMatchingQueryException e) {
logger.debug(e.getMessage());
- disableUserInCloudStack(userAccount);
+ if (e.getMessage().contains(LDAP_READ_TIMED_OUT_MESSAGE) && !rc.first()) {
Review Comment:
Parsing exception messages with `contains(...)` is brittle and may break if the message text changes or is localized. Consider using a dedicated exception subclass or error code to detect timeouts reliably.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3094606719
@sureshanaparti a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3086748118
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3083870393
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3086753838
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3082898634
Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 14227
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3095491153
<b>[SF] Trillian test result (tid-13822)</b>
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 52563 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr11220-t13822-kvm-ol8.zip
Smoke tests completed. 141 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3082661988
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3083875921
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3077855794
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on code in PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#discussion_r2210228431
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapAuthenticator.java:
##########
@@ -229,23 +235,27 @@ private Pair<Boolean, ActionOnFailedAuthentication> authenticate(String username
processLdapUser(password, domainId, user, rc, ldapUser, accountType);
} catch (NoLdapUserMatchingQueryException e) {
logger.debug(e.getMessage());
- // no user in ldap ==>> disable user in cloudstack
- disableUserInCloudStack(user);
+ if (e.getMessage().contains(LDAP_READ_TIMED_OUT_MESSAGE) && !rc.first()) {
+ rc.second(ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT);
+ } else {
+ // no user in ldap ==>> disable user in cloudstack
+ disableUserInCloudStack(user);
+ }
Review Comment:
(Sorry I may not have pressed buttons correctly last time)
Can this be moved to a new method as it is being used twice?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3077864738
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/11220?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 4.00%. Comparing base [(`0d65c8c`)](https://app.codecov.io/gh/apache/cloudstack/commit/0d65c8c454310ddc0b66488b818c2cd1beb50a40?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`e61bd8b`)](https://app.codecov.io/gh/apache/cloudstack/commit/e61bd8b85bffc4b509adb7167c18c923c7fd9d79?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> :exclamation: There is a different number of reports uploaded between BASE (0d65c8c) and HEAD (e61bd8b). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (0d65c8c) | HEAD (e61bd8b) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 4.20 #11220 +/- ##
=============================================
- Coverage 16.16% 4.00% -12.17%
=============================================
Files 5656 402 -5254
Lines 497791 32612 -465179
Branches 60366 5791 -54575
=============================================
- Hits 80456 1305 -79151
+ Misses 408381 31158 -377223
+ Partials 8954 149 -8805
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/11220/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/11220/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.00% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/11220/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/11220?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#discussion_r2209952690
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java:
##########
@@ -140,9 +140,10 @@ public void execute() throws ServerApiException {
try {
final List<LdapUser> users = _ldapManager.getUsers(domainId);
ldapResponses = createLdapUserResponse(users);
-// now filter and annotate
+ // now filter and annotate
ldapResponses = applyUserFilter(ldapResponses);
} catch (final NoLdapUserMatchingQueryException ex) {
+ logger.debug(ex.getMessage());
// ok, we'll make do with the empty list ldapResponses = new ArrayList<LdapUserResponse>();
Review Comment:
maybe just remove the code in comment? it was already initialised on line 138.
```suggestion
// ok, we'll make do with the empty list
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3094605880
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3087088049
Packaging result [SF]: ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 14251
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti merged PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3082665158
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mark LDAP user query timeout as incorrect login instead of disabling user immediately [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#issuecomment-3084178926
Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 14229
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org