Mark LDAP user query timeout as incorrect login instead of disabling user immediately by sureshanaparti · Pull Request #11220 · apache/cloudstack

sureshanaparti · 2025-07-16T09:59:23Z

Description

This PR marks the LDAP user query timeout as incorrect login instead of disabling user immediately.

Fixes #11199

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)
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
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

…user immediately

Copilot

Pull Request Overview

This PR modifies LDAP authentication to treat user query timeouts as incorrect login attempts rather than immediately disabling the user. Key changes include:

Enhanced exception messages and logging in LdapManagerImpl.
Added timeout detection in LdapAuthenticator to increment incorrect login counts on read timeouts.
Minor logging addition in LdapListUsersCmd for empty-list cases.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
LdapManagerImpl.java	Improved exception logging and enriched error messages with cause detail.
LdapAuthenticator.java	Introduced `LDAP_READ_TIMED_OUT_MESSAGE`, updated authentication flows to handle timeouts, and refactored `processResultAndAction`.
LdapListUsersCmd.java	Added debug logging for LDAP lookup failures.

Comments suppressed due to low confidence (2)

plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapAuthenticator.java:178

There’s no existing unit test for the LDAP read timeout path. Add tests to verify that a timeout increments the incorrect login count and does not disable the user.

            logger.debug(e.getMessage());

plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapAuthenticator.java:298

The condition (user != null || timedOut) causes all failures for non-null users to increment the login count, not just timeouts. Verify this matches intended behavior and consider separating the two cases.

        return (!result && (user != null || timedOut)) ?

...er-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java

...ins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapAuthenticator.java

plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java

sureshanaparti · 2025-07-16T10:03:28Z

@blueorangutan package

blueorangutan · 2025-07-16T10:04:03Z

@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 · 2025-07-16T10:06:59Z

Codecov Report

Attention: Patch coverage is 9.75610% with 37 lines in your changes missing coverage. Please review.

Project coverage is 16.16%. Comparing base (0d65c8c) to head (f7884c5).
Report is 13 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
.../org/apache/cloudstack/ldap/LdapAuthenticator.java	10.34%	17 Missing and 9 partials ⚠️
...va/org/apache/cloudstack/ldap/LdapManagerImpl.java	0.00%	11 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20   #11220      +/-   ##
============================================
- Coverage     16.16%   16.16%   -0.01%     
+ Complexity    13276    13275       -1     
============================================
  Files          5656     5656              
  Lines        497791   497800       +9     
  Branches      60366    60367       +1     
============================================
- Hits          80456    80446      -10     
- Misses       408381   408401      +20     
+ Partials       8954     8953       -1

Flag	Coverage Δ
uitests	`4.00% <ø> (ø)`
unittests	`17.01% <9.75%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

DaanHoogland

clgtm

...er-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java

shwstppr

code looks good. One minor comment

blueorangutan · 2025-07-16T11:56:34Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 14207

...ins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapAuthenticator.java

sureshanaparti · 2025-07-17T05:56:09Z

@blueorangutan package

blueorangutan · 2025-07-17T05:58:03Z

@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.

blueorangutan · 2025-07-17T07:17:35Z

Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 14227

sureshanaparti · 2025-07-17T12:24:34Z

@blueorangutan package

blueorangutan · 2025-07-17T12:26:05Z

@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.

blueorangutan · 2025-07-17T13:55:40Z

Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 14229

sureshanaparti · 2025-07-18T05:00:23Z

@blueorangutan package

blueorangutan · 2025-07-18T05:02:07Z

@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.

blueorangutan · 2025-07-18T06:04:52Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 14251

sureshanaparti · 2025-07-20T15:34:21Z

@blueorangutan test

blueorangutan · 2025-07-20T15:36:04Z

@sureshanaparti a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2025-07-21T07:05:07Z

[SF] Trillian test result (tid-13822)
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

kiranchavala

LGTM

Tested with a docker image of Openldap (https://github.com/osixia/docker-openldap) and we are not disable the user when there is certain timeout

…user immediately (apache#11220) * Mark LDAP user query timeout as incorrect login instead of disabling user immediately * code improvements

Mark LDAP user query timeout as incorrect login instead of disabling …

e61bd8b

…user immediately

boring-cyborg bot added the component:LDAP label Jul 16, 2025

sureshanaparti linked an issue Jul 16, 2025 that may be closed by this pull request

LDAP user disabled after response timed out #11199

Closed

sureshanaparti added the status:needs-testing label Jul 16, 2025

sureshanaparti requested review from DaanHoogland, Copilot, harikrishna-patnala, kiranchavala, nvazquez and shwstppr July 16, 2025 10:00

Copilot AI reviewed Jul 16, 2025

View reviewed changes

DaanHoogland approved these changes Jul 16, 2025

View reviewed changes

...er-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java Outdated Show resolved Hide resolved

shwstppr approved these changes Jul 16, 2025

View reviewed changes

...ins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapAuthenticator.java Outdated Show resolved Hide resolved

code improvements

f7884c5

sureshanaparti assigned kiranchavala Jul 17, 2025

sureshanaparti marked this pull request as ready for review July 22, 2025 06:00

kiranchavala approved these changes Jul 25, 2025

View reviewed changes

kiranchavala removed the status:needs-testing label Jul 25, 2025

sureshanaparti merged commit ed6ee6b into apache:4.20 Jul 25, 2025
25 of 26 checks passed

sureshanaparti mentioned this pull request Jul 25, 2025

LDAP user disabled after response timed out #11199

Closed

sureshanaparti deleted the mark-ldap-timeout-as-incorrect-login branch July 25, 2025 14:30

Conversation

sureshanaparti commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sureshanaparti commented Jul 16, 2025

Uh oh!

blueorangutan commented Jul 16, 2025

Uh oh!

codecov bot commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jul 16, 2025

Uh oh!

Uh oh!

sureshanaparti commented Jul 17, 2025

Uh oh!

blueorangutan commented Jul 17, 2025

Uh oh!

blueorangutan commented Jul 17, 2025

Uh oh!

sureshanaparti commented Jul 17, 2025

Uh oh!

blueorangutan commented Jul 17, 2025

Uh oh!

blueorangutan commented Jul 17, 2025

Uh oh!

sureshanaparti commented Jul 18, 2025

Uh oh!

blueorangutan commented Jul 18, 2025

Uh oh!

blueorangutan commented Jul 18, 2025

Uh oh!

sureshanaparti commented Jul 20, 2025

Uh oh!

blueorangutan commented Jul 20, 2025

Uh oh!

blueorangutan commented Jul 21, 2025

Uh oh!

kiranchavala left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sureshanaparti commented Jul 16, 2025 •

edited

Loading

codecov bot commented Jul 16, 2025 •

edited

Loading