add test ldap test command and button - #13954
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13954 +/- ##
============================================
+ Coverage 19.72% 19.74% +0.01%
- Complexity 19941 19963 +22
============================================
Files 6371 6372 +1
Lines 575738 575782 +44
Branches 70471 70472 +1
============================================
+ Hits 113582 113684 +102
+ Misses 449810 449740 -70
- Partials 12346 12358 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.61% |
| Branch coverage | 18.81% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
c8a8518 to
e031775
Compare
|
| import com.cloud.user.Account; | ||
|
|
||
| @APICommand(name = "testLdapConfiguration", description = "Tests connectivity to an LDAP server without saving a configuration", responseObject = SuccessResponse.class, | ||
| since = "4.23.0", requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) |
There was a problem hiding this comment.
| since = "4.23.0", requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) | |
| since = "24.0", requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) |
There was a problem hiding this comment.
awaiting 4.23 release
| import com.cloud.exception.InvalidParameterValueException; | ||
| import com.cloud.user.Account; | ||
|
|
||
| @APICommand(name = "testLdapConfiguration", description = "Tests connectivity to an LDAP server without saving a configuration", responseObject = SuccessResponse.class, |
There was a problem hiding this comment.
Why isn't the command name "ldapTestConfiguration" (same as class name)? Would keep consistency and group it with other Ldap APIs
There was a problem hiding this comment.
good point, mainly because I didn’t review the AI output enough, thanks. I will revisit later,
DaanHoogland
moved this from Backlog
to conflict/waiting for author
in CloudStack Testing
There was a problem hiding this comment.
🟡 Changes recommended
The new/modified LDAP bind exception handling can throw an NPE on null exception messages, and the UI action is duplicated across list/data views in a way that risks divergence.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds a “test LDAP connection” capability end-to-end: a new backend API command that performs an LDAP bind without persisting configuration, plus corresponding UI actions and label text so admins can test connectivity from the LDAP configuration screen.
Changes:
- UI: adds a new “Test LDAP Connection” action/button for LDAP configuration and registers the required icon.
- Backend: introduces
testLdapConfigurationAPI command and refactors LDAP bind logic into a shared helper used by both add-config and test-connection paths. - Tests: adds unit tests to ensure test-connection binds (with default port behavior) and never persists configuration.
File summaries
| File | Description |
|---|---|
| ui/src/core/lazy_lib/icons_use.js | Registers ExperimentOutlined so the new UI action can render its icon. |
| ui/src/config/section/config.js | Adds UI actions for invoking testLdapConfiguration from LDAP settings. |
| ui/public/locales/en.json | Adds the English label for the new LDAP test action. |
| plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java | Adds unit tests validating bind + non-persistence behavior for the new test flow. |
| plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java | Refactors bind logic and implements testConnection for the new API command. |
| plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManager.java | Extends the LDAP manager interface with the new testConnection entry point. |
| plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapTestConfigurationCmd.java | Introduces the testLdapConfiguration API command returning a SuccessResponse. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } catch (RuntimeException e) { | ||
| if (e.getMessage().contains("Invalid truststore")) { | ||
| throw new InvalidParameterValueException("Invalid truststore or truststore password"); | ||
| } | ||
| throw e; | ||
| } finally { |
| api: 'testLdapConfiguration', | ||
| icon: 'ExperimentOutlined', | ||
| label: 'label.test.ldap.configuration', | ||
| docHelp: 'adminguide/accounts.html#using-an-ldap-server-for-user-authentication', | ||
| listView: true, |
Description
This PR...
Fixes: #6934
a new test ldap connection command and button.
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?