ui: link account to LDAP - #13950
ui: link account to LDAP#13950DaanHoogland wants to merge 4 commits into
Conversation
|
@DaanHoogland a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13950 +/- ##
============================================
+ Coverage 19.72% 19.89% +0.16%
- Complexity 19941 20143 +202
============================================
Files 6371 6371
Lines 575738 576834 +1096
Branches 70471 70629 +158
============================================
+ Hits 113582 114778 +1196
+ Misses 449810 449511 -299
- Partials 12346 12545 +199
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:
|
|
UI build: ✔️ |
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.81% |
| Branch coverage | 19.01% |
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
I believe this is a separate feature (also requested in some issue) |
sudo87
left a comment
sudo87
left a comment
There was a problem hiding this comment.
lgtm, tested with openldap locally.
few things to note:
- Link action remains active after successful linking to ldap, relink will error out with "Entity already exists" for the same account.
- No option to unlink the account
|
@DaanHoogland a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
|
UI build: ✔️ |
| "label.adding.user": "Adding User...", | ||
| "label.address": "Address", | ||
| "label.address.group": "Address group", | ||
| "label.admin": "Domain Admin", |
There was a problem hiding this comment.
not sure if this will cause issues anywhere else in the UI
There was a problem hiding this comment.
No, I grep -r’d for it. also some languages have admin and other domain admin. Unused.
There was a problem hiding this comment.
need to revert this removal
|
@sudo87 want to retest? |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are consistent with existing UI action patterns, include unit coverage for the new mapping behavior, and appear low-risk given current config usage.
Pull request overview
This PR adds UI support for the linkAccountToLdap API from the Account detail view, and extends the generic Autogen action form to properly render and submit select options provided as { id, description } objects (submitting the id while showing a translated label).
Changes:
- Add an Account action for
linkAccountToLdap, pre-fillingdomainidandaccountfrom the current record. - Update
AutogenViewto display translated select labels when an option has adescription, and submitoption.idfor object options. - Add unit test coverage for the new
{id, description}mapping behavior and introduce new i18n keys for account types + the new action label.
File summaries
| File | Description |
|---|---|
| ui/src/views/AutogenView.vue | Render select options using translated description when present; submit id for object-mapped options. |
| ui/tests/unit/views/AutogenView.spec.js | Add a unit test ensuring {id, description} option selections submit the id. |
| ui/src/config/section/domain.js | Switch LDAP link domain accounttype options to {id, description} objects for better UX/i18n. |
| ui/src/config/section/account.js | Add linkAccountToLdap action on Account detail view (LDAP-enabled only) with relevant mappings/options. |
| ui/public/locales/en.json | Add new label keys for account types and “Link account to LDAP”; remove unused label.admin. |
| ui/public/locales/ar.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/ca.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/de_DE.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/el_GR.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/es.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/fr_FR.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/hi.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/hu.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/it_IT.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/ja_JP.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/ko_KR.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/nb_NO.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/nl_NL.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/pl.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/pt_BR.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/ru_RU.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/te.json | Add account type label keys; remove unused label.admin. |
| ui/public/locales/zh_CN.json | Add account type label keys; remove unused label.admin. |
Review details
- Files reviewed: 23/23 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
sudo87
left a comment
sudo87
left a comment
There was a problem hiding this comment.
- Link Account to LDap works, however "admin" label needs to restored.
- Link Domain to LDap from UI is broken
| "label.adding.user": "Adding User...", | ||
| "label.address": "Address", | ||
| "label.address.group": "Address group", | ||
| "label.admin": "Domain Admin", |
There was a problem hiding this comment.
need to revert this removal
| }, | ||
| accounttype: { | ||
| options: ['0', '2'] | ||
| options: [ |
There was a problem hiding this comment.
this breaks the "Link Domain to Ldap" form, it wont work due to this change.
|
@vishesh92 a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
There was a problem hiding this comment.
🔵 Needs a closer look
Address authorization, parameter validation, and missing locale-label issues.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
ui/src/config/section/account.js:142
- Both
accounttypeandroleidare optional in the API metadata, so this form can be submitted with neither value selected, but the backend explicitly rejects that combination (Either account type or role ID must be given). Add cross-field validation or provide a valid default before allowing submission.
ui/src/config/section/account.js:151 typeis optional in the API metadata, so the generic form does not require a selection and omits it when this field is left blank. However,LdapManagerImpl.linkAccountToLdaprejects a null type before processing the request, so the new action can fail with its default/blank state; supply the documented GROUP default (or make the field required) in this action.
ui/src/config/section/account.js:131
linkAccountToLdapis authorized only forAdminandDomainAdmin(plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LinkAccountToLdapCmd.java:46-47), but this predicate exposes the action to every role whenever LDAP is enabled. A regular user can therefore open a form that will always be rejected by the API; gate the UI action on the same roles while retaining the server-side authorization.
show: (record, store) => {
return store.isLdapEnabled
ui/src/config/section/account.js:127
- This new label key is only defined in
en.json, while all supported locale files already carry the corresponding domain-link label. In a non-English UI, this action therefore falls back to English instead of being localized; please addlabel.link.account.to.ldapto every locale file as well.
label: 'label.link.account.to.ldap',
- Files reviewed: 24/24 changed files
- Comments generated: 0 new
- Review effort level: Lite
sudo87
left a comment
sudo87
left a comment
There was a problem hiding this comment.
lgtm, re-tested the flow.
|
UI build: ✔️ |
|
Description
This PR...
Fixes: #11473
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?