Improve LDAP authentication error handling and resource management (#2949)

* Improve LDAP authentication error handling and resource management

This commit enhances the LDAP authentication implementation with several
important improvements for robustness, security, and maintainability:

## Error Handling Improvements
- Added more specific exception handling in validate(), login(), and
  changePassword() methods
- Separated LdapConfigurationException, LdapOperationException, and
  generic Exception handling for better error diagnosis
- Improved error logging with more descriptive messages including
  context information (e.g., username, operation type)

## Resource Management
- Enhanced ThreadLocal cleanup in DirContextHolder.close() with
  try-finally block to ensure contextLocal.remove() is always called
- Prevents potential ThreadLocal memory leaks in application servers
- Added debug logging for context close failures

## Defensive Programming
- Added null/blank checks in critical methods:
  * login(username, password) - validates both parameters
  * login(username) - validates username parameter
  * changePassword() - validates username and password
  * getSAMAccountGroupName() - validates bindDn and groupName
- Returns empty results gracefully instead of failing with NPE
- Added early returns for invalid input to avoid unnecessary processing

## Documentation Enhancements
- Expanded JavaDoc for escapeLDAPSearchFilter() with:
  * Detailed character escaping reference per RFC 4515
  * Security notes emphasizing LDAP injection prevention
  * RFC reference link for further reading
- Enhanced changePassword() JavaDoc with:
  * HTML-formatted documentation
  * Detailed validation steps list
  * Parameter constraints clearly documented
- Added null handling in escapeLDAPSearchFilter() for safety
- Optimized StringBuilder initial capacity for better performance

## Minor Code Quality Improvements
- Changed local variables to final where appropriate for immutability
- Improved code organization with better nesting and try-catch structure
- Consistent error message formatting across methods

These changes improve the security, reliability, and maintainability of
the LDAP authentication module without breaking existing functionality.

* Fix JavaDoc to match null handling in escapeLDAPSearchFilter

Updated the JavaDoc documentation for escapeLDAPSearchFilter() to accurately
reflect that null input is treated as an empty string, rather than stating
'must not be null' which conflicted with the implementation.

This change resolves the inconsistency between documentation and
implementation, maintaining defensive programming while being clear about
the method's behavior.

* Add comprehensive tests for LDAP authentication improvements

This commit adds extensive unit tests for the LDAP authentication
improvements made in previous commits, covering:

## LDAP Injection Prevention Tests (11 tests)
- Null and empty string handling
- Normal input (alphanumeric, dots)
- Special character escaping:
  * Backslash (\) → \5c
  * Asterisk (*) → \2a (prevents wildcard injection)
  * Parentheses ((), )) → \28, \29 (prevents filter injection)
  * Null byte (\0) → \00
- Complex injection attempt simulation
- All special characters combined
- Unicode character preservation
- Mixed content handling

## Defensive Null/Blank Check Tests (11 tests)
- getSAMAccountGroupName():
  * Null bindDn handling
  * Blank bindDn handling (empty string and whitespace)
  * Null groupName handling
  * Blank groupName handling
- changePassword():
  * Null username handling
  * Blank username handling
  * Null password handling
  * Blank password handling
  * Admin disabled scenario

## Error Handling Tests (3 tests)
- normalizePermissionName():
  * Null input handling
  * Lowercase enabled behavior
  * Lowercase disabled behavior

## Edge Case Tests (3 tests)
- getSearchRoleName() with various invalid inputs
- replaceWithUnderscores() with edge cases
- escapeLDAPSearchFilter() with Unicode characters

Total: 28 new comprehensive unit tests added

Test Coverage:
- Security: LDAP injection prevention
- Robustness: Null/blank input handling
- Correctness: Expected behavior validation
- Edge cases: Boundary condition testing

All tests follow the existing test structure using UnitFessTestCase
and verify the improvements made to error handling, defensive
programming, and security enhancements.

* Fix test_replaceWithUnderscores_withEdgeCases assertion

Corrected the expected value in test_replaceWithUnderscores_withEdgeCases.
The input string "//\\[]:;" contains 8 special characters:
- // (2 slashes) → __
- \\ (2 backslashes) → __
- []:; (4 characters) → ____

Total: 8 underscores, not 10 as previously expected.

This fixes the test failure:
expected:<________[__]> but was:<________[]>

* Fix flaky test_startProcess_replaceExistingProcess test

The test was failing intermittently because it used 'echo' commands
which complete almost instantaneously. By the time the assertion
checked if the process was running, it had already terminated.

Changes:
- Replace 'echo' with 'cat' command which waits for input, making
  it a long-running process suitable for testing
- Add polling logic (max 5 seconds) to wait for process to become
  running before assertions
- Add descriptive failure messages to help diagnose timeout issues

This makes the test more reliable across different system loads and
timing conditions while still testing the intended behavior of
replacing an existing process with the same session ID.

Note: This fix is independent of LDAP authentication improvements
and addresses a pre-existing test stability issue.

* Change LDAP error logs to warn level per logging policy

Updated logger.error() calls to logger.warn() in LDAP authentication
code to follow the Fess logging policy:

- error: System-stopping errors where service cannot be provided
- warn: Issues where system remains usable (e.g., search still works)

Changes:
- validate(): LdapConfigurationException now logs at warn level
- changePassword(): Both LdapOperationException and general exceptions
  now log at warn level

Rationale: LDAP authentication failures do not prevent the search
functionality from working, so these should be warnings rather than
errors.

* Enhance logging for LDAP errors

---------

Co-authored-by: Claude <[email protected]>

Author: marevol

src/main/java/org/codelibs/fess/ldap/LdapManager.java

@@ -195,15 +195,21 @@ protected boolean validate() {
                 // no credentials
                 return !fessConfig.isLdapAuthValidation();
             }
-            final Hashtable<String, String> env = createAdminEnv();
-            try (DirContextHolder holder = getDirContext(() -> env)) {
-                final DirContext context = holder.get();
-                if (logger.isDebugEnabled()) {
-                    logger.debug("Logged in as Bind DN. {}", context);
+            try {
+                final Hashtable<String, String> env = createAdminEnv();
+                try (DirContextHolder holder = getDirContext(() -> env)) {
+                    final DirContext context = holder.get();
+                    if (logger.isDebugEnabled()) {
+                        logger.debug("Logged in as Bind DN. {}", context);
+                    }
+                    isBind = true;
                 }
-                isBind = true;
+            } catch (final LdapConfigurationException e) {
+                logger.warn("LDAP configuration error: {}", e.getMessage(), e);
+            } catch (final LdapOperationException e) {
+                logger.warn("LDAP connection failed: {}", e.getMessage(), e);
             } catch (final Exception e) {
-                logger.warn("LDAP configuration is wrong.", e);
+                logger.warn("Unexpected error during LDAP validation: {}", e.getMessage(), e);
             }
         }
         return isBind;
@@ -217,26 +223,38 @@ protected boolean validate() {
      * @return an optional containing the authenticated user if successful, empty otherwise
      */
     public OptionalEntity<FessUser> login(final String username, final String password) {
+        // Add defensive null/blank checks
+        if (StringUtil.isBlank(username) || StringUtil.isBlank(password)) {
+            if (logger.isDebugEnabled()) {
+                logger.debug("Login failed: username or password is blank");
+            }
+            return OptionalEntity.empty();
+        }
+
         if (StringUtil.isBlank(fessConfig.getLdapProviderUrl()) || !validate()) {
             return OptionalEntity.empty();
         }
 
-        final Hashtable<String, String> env = createSearchEnv(username, password);
-        try (DirContextHolder holder = getDirContext(() -> env)) {
-            final DirContext context = holder.get();
-            final LdapUser ldapUser = createLdapUser(username, env);
-            if (!allowEmptyGroupAndRole(ldapUser)) {
+        try {
+            final Hashtable<String, String> env = createSearchEnv(username, password);
+            try (DirContextHolder holder = getDirContext(() -> env)) {
+                final DirContext context = holder.get();
+                final LdapUser ldapUser = createLdapUser(username, env);
+                if (!allowEmptyGroupAndRole(ldapUser)) {
+                    if (logger.isDebugEnabled()) {
+                        logger.debug("Login failed. No permissions. {}", context);
+                    }
+                    return OptionalEntity.empty();
+                }
                 if (logger.isDebugEnabled()) {
-                    logger.debug("Login failed. No permissions. {}", context);
+                    logger.debug("Logged in. {}", context);
                 }
-                return OptionalEntity.empty();
-            }
-            if (logger.isDebugEnabled()) {
-                logger.debug("Logged in. {}", context);
+                return OptionalEntity.of(ldapUser);
             }
-            return OptionalEntity.of(ldapUser);
+        } catch (final LdapOperationException e) {
+            logger.debug("LDAP operation failed during login for user: {}", username, e);
         } catch (final Exception e) {
-            logger.debug("Login failed.", e);
+            logger.debug("Login failed for user: {}", username, e);
         }
         return OptionalEntity.empty();
     }
@@ -248,22 +266,34 @@ public OptionalEntity<FessUser> login(final String username, final String passwo
      * @return an optional containing the authenticated user if successful, empty otherwise
      */
     public OptionalEntity<FessUser> login(final String username) {
-        final Hashtable<String, String> env = createSearchEnv();
-        try (DirContextHolder holder = getDirContext(() -> env)) {
-            final DirContext context = holder.get();
-            final LdapUser ldapUser = createLdapUser(username, env);
-            if (!allowEmptyGroupAndRole(ldapUser)) {
+        // Add defensive null/blank check
+        if (StringUtil.isBlank(username)) {
+            if (logger.isDebugEnabled()) {
+                logger.debug("Login failed: username is blank");
+            }
+            return OptionalEntity.empty();
+        }
+
+        try {
+            final Hashtable<String, String> env = createSearchEnv();
+            try (DirContextHolder holder = getDirContext(() -> env)) {
+                final DirContext context = holder.get();
+                final LdapUser ldapUser = createLdapUser(username, env);
+                if (!allowEmptyGroupAndRole(ldapUser)) {
+                    if (logger.isDebugEnabled()) {
+                        logger.debug("Login failed. No permissions. {}", context);
+                    }
+                    return OptionalEntity.empty();
+                }
                 if (logger.isDebugEnabled()) {
-                    logger.debug("Login failed. No permissions. {}", context);
+                    logger.debug("Logged in. {}", context);
                 }
-                return OptionalEntity.empty();
-            }
-            if (logger.isDebugEnabled()) {
-                logger.debug("Logged in. {}", context);
+                return OptionalEntity.of(ldapUser);
             }
-            return OptionalEntity.of(ldapUser);
+        } catch (final LdapOperationException e) {
+            logger.debug("LDAP operation failed during login for user: {}", username, e);
         } catch (final Exception e) {
-            logger.debug("Login failed.", e);
+            logger.debug("Login failed for user: {}", username, e);
         }
         return OptionalEntity.empty();
     }
@@ -370,6 +400,14 @@ public String[] getRoles(final LdapUser ldapUser, final String bindDn, final Str
      * @return an optional containing the sAMAccountName if found, empty otherwise
      */
     protected OptionalEntity<String> getSAMAccountGroupName(final String bindDn, final String groupName) {
+        // Add defensive null/blank checks
+        if (StringUtil.isBlank(bindDn) || StringUtil.isBlank(groupName)) {
+            if (logger.isDebugEnabled()) {
+                logger.debug("bindDn or groupName is blank: bindDn={}, groupName={}", bindDn, groupName);
+            }
+            return OptionalEntity.empty();
+        }
+
         final Hashtable<String, String> env = createSearchEnv();
         try (DirContextHolder holder = getDirContext(() -> env)) {
             final DirContext context = holder.get();
@@ -390,8 +428,10 @@ protected OptionalEntity<String> getSAMAccountGroupName(final String bindDn, fin
                     return OptionalEntity.of(sAMAccountName);
                 }
             }
+        } catch (final NamingException e) {
+            logger.warn("LDAP naming exception while getting sAMAccountName for group: {}", groupName, e);
         } catch (final Exception e) {
-            logger.warn("Failed to get sAMAccountName: {}", groupName, e);
+            logger.warn("Unexpected exception while getting sAMAccountName for group: {}", groupName, e);
         }
         return OptionalEntity.empty();
     }
@@ -463,15 +503,31 @@ protected String updateSearchRoles(final Set<String> roleSet, final String entry
     }
 
     /**
-     * Escapes special characters in an LDAP search filter to prevent injection attacks.
+     * Escapes special characters in an LDAP search filter to prevent LDAP injection attacks.
+     *
+     * <p>This method escapes the following characters as per RFC 4515:
+     * <ul>
+     * <li>\ (backslash) → \5c</li>
+     * <li>* (asterisk) → \2a</li>
+     * <li>( (left parenthesis) → \28</li>
+     * <li>) (right parenthesis) → \29</li>
+     * <li>\0 (null character) → \00</li>
+     * </ul>
      *
-     * @param filter the LDAP search filter to escape
-     * @return the escaped filter string
+     * <p><strong>Security Note:</strong> This method MUST be called on all user-supplied
+     * input before using it in LDAP search filters to prevent LDAP injection vulnerabilities.
+     *
+     * @param filter the LDAP search filter to escape (null is treated as empty string)
+     * @return the escaped filter string safe for use in LDAP queries (empty string if filter is null)
+     * @see <a href="https://tools.ietf.org/html/rfc4515">RFC 4515 - LDAP String Representation of Search Filters</a>
      */
-    protected String escapeLDAPSearchFilter(String filter) {
-        final StringBuilder sb = new StringBuilder();
+    protected String escapeLDAPSearchFilter(final String filter) {
+        if (filter == null) {
+            return "";
+        }
+        final StringBuilder sb = new StringBuilder(filter.length() * 2);
         for (int i = 0; i < filter.length(); i++) {
-            char curChar = filter.charAt(i);
+            final char curChar = filter.charAt(i);
             switch (curChar) {
             case '\\':
                 sb.append("\\5c");
@@ -1458,26 +1514,51 @@ public void delete(final Group group) {
     /**
      * Changes the password for a user in the LDAP directory.
      *
-     * @param username the username of the user
-     * @param password the new password
+     * <p>This method performs the following validations:
+     * <ul>
+     * <li>Checks if username and password are not blank</li>
+     * <li>Verifies LDAP admin is enabled for the user</li>
+     * <li>Confirms the user exists in LDAP directory</li>
+     * </ul>
+     *
+     * @param username the username of the user (must not be null or blank)
+     * @param password the new password (must not be null or blank)
      * @return true if the password was changed successfully, false otherwise
+     * @throws LdapOperationException if the user is not found in LDAP
      */
     public boolean changePassword(final String username, final String password) {
-        if (!fessConfig.isLdapAdminEnabled(username)) {
+        // Add defensive null/blank checks
+        if (StringUtil.isBlank(username) || StringUtil.isBlank(password)) {
+            logger.warn("Cannot change password: username or password is blank");
             return false;
         }
 
-        final Supplier<Hashtable<String, String>> adminEnv = this::createAdminEnv;
-        final String userDN = fessConfig.getLdapAdminUserSecurityPrincipal(username);
-        search(fessConfig.getLdapAdminUserBaseDn(), fessConfig.getLdapAdminUserFilter(username), null, adminEnv, result -> {
-            if (result.isEmpty()) {
-                throw new LdapOperationException("User is not found: " + username);
+        if (!fessConfig.isLdapAdminEnabled(username)) {
+            if (logger.isDebugEnabled()) {
+                logger.debug("LDAP admin not enabled for user: {}", username);
             }
-            final List<ModificationItem> modifyList = new ArrayList<>();
-            modifyReplaceEntry(modifyList, "userPassword", password);
-            modify(userDN, modifyList, adminEnv);
-        });
-        return true;
+            return false;
+        }
+
+        try {
+            final Supplier<Hashtable<String, String>> adminEnv = this::createAdminEnv;
+            final String userDN = fessConfig.getLdapAdminUserSecurityPrincipal(username);
+            search(fessConfig.getLdapAdminUserBaseDn(), fessConfig.getLdapAdminUserFilter(username), null, adminEnv, result -> {
+                if (result.isEmpty()) {
+                    throw new LdapOperationException("User is not found: " + username);
+                }
+                final List<ModificationItem> modifyList = new ArrayList<>();
+                modifyReplaceEntry(modifyList, "userPassword", password);
+                modify(userDN, modifyList, adminEnv);
+            });
+            return true;
+        } catch (final LdapOperationException e) {
+            logger.warn("Failed to change password for user: {}", username, e);
+            throw e;
+        } catch (final Exception e) {
+            logger.warn("Unexpected error while changing password for user: {}", username, e);
+            return false;
+        }
     }
 
     /**
@@ -1671,13 +1752,19 @@ public void close() {
             if (counter > 1) {
                 counter--;
             } else {
-                contextLocal.remove();
-                if (context != null) {
-                    try {
-                        context.close();
-                    } catch (final NamingException e) {
-                        // ignored
+                try {
+                    if (context != null) {
+                        try {
+                            context.close();
+                        } catch (final NamingException e) {
+                            if (logger.isDebugEnabled()) {
+                                logger.debug("Failed to close LDAP context", e);
+                            }
+                        }
                     }
+                } finally {
+                    // Ensure ThreadLocal is always cleaned up, even if context.close() fails
+                    contextLocal.remove();
                 }
             }
         }

src/test/java/org/codelibs/fess/helper/ProcessHelperTest.java

@@ -149,8 +149,10 @@ public void test_startProcess_invalidCommand() {
 
     public void test_startProcess_replaceExistingProcess() {
         String sessionId = "test_replace";
-        List<String> cmdList1 = Arrays.asList("echo", "first");
-        List<String> cmdList2 = Arrays.asList("echo", "second");
+        // Use 'cat' which waits for input, making it a long-running process
+        // This is more reliable than 'echo' which completes immediately
+        List<String> cmdList1 = Arrays.asList("cat");
+        List<String> cmdList2 = Arrays.asList("cat");
         Consumer<ProcessBuilder> pbCall = pb -> {
             pb.redirectErrorStream(true);
         };
@@ -159,15 +161,32 @@ public void test_startProcess_replaceExistingProcess() {
             // Start first process
             JobProcess jobProcess1 = processHelper.startProcess(sessionId, cmdList1, pbCall);
             assertNotNull(jobProcess1);
-            assertTrue(processHelper.isProcessRunning(sessionId));
+
+            // Poll for process to be running (max 50 times, 100ms interval = 5 seconds max)
+            boolean isRunning = false;
+            for (int i = 0; i < 50; i++) {
+                if (processHelper.isProcessRunning(sessionId)) {
+                    isRunning = true;
+                    break;
+                }
+                Thread.sleep(100);
+            }
+            assertTrue("First process did not become running within timeout", isRunning);
 
             // Start second process with same session ID (should replace first)
             JobProcess jobProcess2 = processHelper.startProcess(sessionId, cmdList2, pbCall);
             assertNotNull(jobProcess2);
-            assertTrue(processHelper.isProcessRunning(sessionId));
 
-            // Wait a bit for the processes to complete
-            Thread.sleep(100);
+            // Poll for the replacement process to be running
+            isRunning = false;
+            for (int i = 0; i < 50; i++) {
+                if (processHelper.isProcessRunning(sessionId)) {
+                    isRunning = true;
+                    break;
+                }
+                Thread.sleep(100);
+            }
+            assertTrue("Replacement process did not become running within timeout", isRunning);
 
             // Clean up
             processHelper.destroyProcess(sessionId);