Fix NPE in LDAPAuthorizationBackend when rolesearch disabled

[tronboto]

Description

• Bug fix

When rolesearch_enabled: false is configured and the user authenticated via ldap, an NPE is thrown:

Cannot invoke "org.ldaptive.Connection.getProviderConnection()" because
the return value of "org.ldaptive.SearchOperation.getConnection()" is null

This happens because a connection to ldap is never opened in the user lookup block as the user's DN is already known. The code then falls through to the else branch that calls getRoleFromEntry() to resolve role names, but connection is still null at this point, causing the NPE.

The rolesearch block (and the nested roles block) already have null guards which explains why enabling rolesearch worked as a workaround.

Issues Resolved

Fixes #5832

Is this a backport? No

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? No

Testing

Added testLdapAuthorizationRolesearchDisabledWithLdapAuthContext which authenticates the user with an LDAPAuthenticationBackend then calls addRoles with rolesearch_enabled: false.

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• Commits are signed per the DCO using --signoff

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.79%. Comparing base (ea6087c) to head (055ae72).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6112      +/-   ##
==========================================
+ Coverage   74.78%   74.79%   +0.01%     
==========================================
  Files         447      447              
  Lines       28467    28467              
  Branches     4328     4331       +3     
==========================================
+ Hits        21289    21293       +4     
+ Misses       5184     5180       -4     
  Partials     1994     1994              

Files with missing lines	Coverage Δ
...ty/auth/ldap/backend/LDAPAuthorizationBackend.java	67.76% <100.00%> (ø)

... and 11 files with indirect coverage changes

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

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit aeeab94)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Connection Lifecycle The fix moves the null-check for connection to before the rolesearchEnabled block, ensuring a connection is always opened. However, it's worth verifying that this newly opened connection is properly closed in all code paths (including exception paths) when rolesearchEnabled is false, since the original code only opened a connection inside the rolesearchEnabled block where cleanup was presumably handled. if (connection == null) { connection = getConnection(settings, configPath); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to aeeab94
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid unnecessary connection acquisition Moving getConnection outside the rolesearchEnabled block means a connection is always established, even when it may not be needed (e.g., when neither role search nor nested roles require it). This could introduce unnecessary LDAP connections and potential resource leaks. Consider only acquiring the connection when it is actually required by the subsequent logic. src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [835-839] -if (connection == null) { - connection = getConnection(settings, configPath); -} +if (rolesearchEnabled) { + if (connection == null) { + connection = getConnection(settings, configPath); + } -if (rolesearchEnabled) { - Suggestion importance[1-10]: 6 __ Why: The PR moves getConnection outside the rolesearchEnabled block, which could establish unnecessary LDAP connections when role search is disabled. The suggestion to keep it inside the block is valid for resource efficiency, though the original code also had a second getConnection call inside the nested roles loop that was removed, suggesting the intent was to consolidate connection acquisition.	Low
	Improve assertion clarity in test The test asserts that the roles set contains exactly 2 roles, but uses separate assertTrue calls that don't fail with a descriptive message. Using assertThat with containsInAnyOrder or hasItems would make the test more robust and provide better failure messages. src/test/java/org/opensearch/security/auth/ldap/LdapBackendTestNewStyleConfig.java [746-748] assertThat(user.getRoles().size(), is(2)); -Assert.assertTrue(user.getRoles().contains("dummyempty")); -Assert.assertTrue(user.getRoles().contains("rolemo4")); +MatcherAssert.assertThat(user.getRoles(), hasItems("dummyempty", "rolemo4")); Suggestion importance[1-10]: 3 __ Why: The suggestion to use hasItems instead of separate assertTrue calls is a minor style improvement that provides better failure messages, but the existing assertions are functionally correct and the improvement is marginal.	Low

---

Previous suggestions

Suggestions up to commit aeeab94

Category	Suggestion	Impact
General	Avoid unnecessary connection acquisition Moving the connection = getConnection(settings, configPath) call outside the rolesearchEnabled block means a connection is always established, even when it may not be needed (e.g., when neither role search nor nested roles require it). This could introduce unnecessary LDAP connections and potential resource leaks. Consider only acquiring the connection when it is actually required by the subsequent logic. src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [835-839] -if (connection == null) { - connection = getConnection(settings, configPath); -} +if (rolesearchEnabled) { + if (connection == null) { + connection = getConnection(settings, configPath); + } -if (rolesearchEnabled) { - Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about acquiring a connection even when rolesearchEnabled is false. However, the PR's intent appears to be fixing a bug where the connection was not available when needed for non-rolesearch paths (e.g., user role attributes). The suggestion's improved_code doesn't fully address the case where connection is needed outside rolesearchEnabled.	Low
	Consistent assertion style in tests The test asserts that user.getRoles().size() is 2 but only checks for two specific roles. If the roles set contains unexpected extra roles, the size assertion would catch it, but the test doesn't verify that no unexpected roles are present. Consider using assertThat(user.getRoles(), containsInAnyOrder("dummyempty", "rolemo4")) to make the assertion both complete and readable. src/test/java/org/opensearch/security/auth/ldap/LdapBackendTestNewStyleConfig.java [746-748] assertThat(user.getRoles().size(), is(2)); -Assert.assertTrue(user.getRoles().contains("dummyempty")); -Assert.assertTrue(user.getRoles().contains("rolemo4")); +MatcherAssert.assertThat(user.getRoles(), hasItem("dummyempty")); +MatcherAssert.assertThat(user.getRoles(), hasItem("rolemo4")); Suggestion importance[1-10]: 2 __ Why: The suggestion proposes using containsInAnyOrder for completeness, but the improved_code only changes Assert.assertTrue to MatcherAssert.assertThat with hasItem, which doesn't match the described improvement. The existing size check already ensures no unexpected roles are present.	Low

[github-actions]

Persistent review updated to latest commit aeeab94