NMS-20106: versioned user management API and PrimeVue Manage Users page - #8713
NMS-20106: versioned user management API and PrimeVue Manage Users page#8713joseanesONMS wants to merge 21 commits into
Conversation
Adds /api/v2/users (interface + impl following the v2 conventions): list/get/create/update, dedicated password and rename endpoints, delete, and available-roles — all admin-only via new Spring Security rules plus in-code checks. users.xml stays the system of record: updates apply only the exposed fields, so contact types the API does not carry (XMPP among them) and the password survive untouched, and passwords are stored salted via the existing UserManager hashing. Unlike the legacy JSPs, which only hid the buttons, the admin/rtc delete and rename protections are enforced server-side; responses never include the password hash (the v1 API returns it to admins). The new Manage Users page (ui/#/admin/users) is a straight visualization of that API: users table without the XMPP column, add/edit dialog with role assignment, password and rename dialogs, and delete disabled for the protected system accounts. The Manage Users menu entry now points at the new page.
Fixes from self-review. The list fields of UserDto now default to null so a request that omits them genuinely preserves roles and duty schedules (empty-list defaults made the preservation guards dead code and a partial update wiped both). Mutations validate the whole request first and then apply it to a detached copy of the stored user, so a rejected request can no longer leave partial changes in UserManager's shared in-memory state; a failed create rolls the phantom map entry back, and a service-level lock closes the check-then-act races. Duty schedule validation accepts overnight ranges (legacy wrote MoTu2000-800 and hand-edited files contain them — rejecting those made such users uneditable), user ids reject characters that cannot appear in a URL path segment, a body/path user-id mismatch is rejected instead of ignored, and HEAD joins the admin-only security rules. The UI keeps the previous user list when a refresh fails instead of blanking the table. The IT grows to 14 tests including regressions for omitted-field preservation, rejected-update atomicity, and overnight schedules.
Follow-up from the group-management review, applying the same findings here. Duty schedules with begin after end are rejected for new entries (DutySchedule.isInSchedule compares within one calendar day, so an overnight range never matches); strings already stored on the record keep round-tripping so hand-edited files never make a user uneditable. Dot-segment user ids are rejected. Emptied form fields now clear the value instead of silently preserving it. The service serializes on the same shared monitor as the groups API because user mutations cascade into GroupManager. Adds a collapsed "?" help panel explaining what user accounts are for and how to use the page.
…dialogs The dialogs previously reported failures only through a toast that renders behind the modal overlay, so a rejected save looked like nothing happened. Mutating service calls now return the server's error message, which the dialogs display inside the form while staying open; new client-side checks (shared lib/adminValidation.ts) mirror the server rules for user-ids and flag malformed email addresses before submitting. Users whose ids contain / \ or % (possible in hand-edited users.xml) get their row actions replaced with a file-managed note, since the security filter rejects those characters in URL paths. Error toasts now use the error styling.
The markup regexes used full-string matches that a newline anywhere bypassed; they now scan for the characters directly. The admin account can no longer have ROLE_ADMIN removed (it would lock every administrator out). applyDto treats omitted scalar fields as preserve, matching the documented contract and the sibling APIs, so a roles-only PUT no longer wipes names, comments and contacts. User comments now reject markup like group comments do, with unchanged hand-edited values grandfathered. Deleting a user who supervises an on-call role is rejected, since GroupManager leaves the supervisor attribute dangling. The client email check accepts display-name and comma-separated recipient forms and, like the comments check, only fires on changed input so hand-edited values never block the editor.
|
@marshallmassengill could you take a review pass when you get a chance? |
…HeaderIT The menu entry now lands on the Vue page, so the smoke test waits for its page title instead of the legacy JSP breadcrumb.
Swap direct PrimeVue components for the Onms-XXX seam wrappers across the users table and its dialogs: Button->OnmsButton (text/outlined mapped to variant), Dialog->OnmsDialog, InputText->OnmsInputText, Password->OnmsPassword, MultiSelect->OnmsMultiSelect, DataTable->OnmsTable, Column->OnmsColumn, Tag->OnmsTag. Message and IftaLabel have no wrapper yet and stay on PrimeVue. No behaviour change.
|
@synqotik @marshallmassengill — ready for review. This page now uses the |
…g width prop
Replace :style="{ width, maxWidth }" on OnmsDialog (which rode $attrs into
PrimeVue) with the seam's declared width prop, using min(Npx, 95vw) to keep
the responsive cap.
marshallmassengill
left a comment
marshallmassengill
left a comment
There was a problem hiding this comment.
One blocker: Rename leaves on-call role supervisors dangling, which is the exact case delete guards against. deleteUser walks m_groupManager.getRoles() and returns 400 if the user supervises any role, with a comment noting that GroupManager.deleteUser "leaves role supervisors dangling". GroupManager.renameUser has the same hole: it rewrites group memberships and Schedule names but never touches role.getSupervisor() (GroupManager.java:498-528). So POST /api/v2/users/{id}/rename on a supervisor silently points the rota at a user id that no longer exists, killing the supervisor fallback with no error. Either apply the same 400 guard or fix up the supervisor as part of the rename.
GroupManager.renameUser rewrote group memberships and schedule names but left role.getSupervisor() pointing at the old id, so renaming a supervisor silently killed the rota's supervisor fallback -- the same dangling-reference case the delete path guards against. Rewrite the supervisor as part of the rename so every reference follows the user to the new id.
Drop the nested primevue IftaLabel from every field and use FormField's own label/for/required/error props, and replace the direct primevue Message error banners with a themed role=alert element, so the dialogs no longer import any un-wrapped PrimeVue primitive. Field errors now render through FormField (the two test selectors follow the FormField-generated error ids).
|
@marshallmassengill Addressed the blocker: Rename leaves on-call role supervisors dangling. Fixed at the root in Also brought the dialogs onto the current |
Drop the JAXB annotations from the v2 user model DTOs so they serialize as plain camelCase JSON (userId, fullName, pagerEmail, userComments, dutySchedules, roles, readOnly, newUserId) instead of the hyphenated XML element names, and carry that through the UI types, service, dialogs, and table. The XML mapping to/from users.xml already lives in the REST service. Also swap the hand-rolled isBlank/trimToNull helpers for org.apache.commons.lang3.StringUtils and match the request-field names in the validation messages.
|
@synqotik Addressed the API-shape feedback:
Verified: full I'm applying the same camelCase-JSON convention to #8714 (groups) and auditing the notification PRs' v2 DTOs for consistency. |
The Full Name and Pager Email columns still bound to the old hyphenated field names after the camelCase migration, so they rendered blank; point them at fullName/pagerEmail. Show '--' for an empty roles cell.
|
Thanks @synqotik — went through all six: Fixed now (pushed):
Needs the seam, not on this branch:
Design points — agree, and they're cross-cutting, so let's align before I change them:
I left these three as-is pending your steer rather than guessing. The camelCase-JSON convention is also applied to #8714 (groups) now. |
Replace the inline About panel with an Info-icon button in the table header that opens the help copy in an OnmsDialog (new shared AboutDialogButton), and collapse the four per-row action buttons to inline Edit and Delete plus a overflow OnmsMenu carrying Change Password and Rename. Protected-account disabling and the file-managed note for unaddressable ids are unchanged.
|
@synqotik Implemented the three UI patterns we discussed, on Manage Users as the template:
eslint clean, full vitest suite (2191) green, If this shape looks right to you, I'll mirror the same |
|
@joseanesONMS thanks changes look good! One more thing, make same change to |
…efault template too menu-template-default.json must stay identical to menu-template.json.
|
Done — Noted on the Drawers/Dialogs/page UX — happy to have that broader discussion separately; leaving the dialogs as-is for now. I'll mirror the Info-dialog + overflow-menu patterns onto #8714 (Groups) next, now that the shape's confirmed. |
synqotik
left a comment
synqotik
left a comment
There was a problem hiding this comment.
LGTM! Let's try this out and we can make adjustments as needed.
…age-users-primevue
marshallmassengill
left a comment
marshallmassengill
left a comment
There was a problem hiding this comment.
- Every write re-HTML-escapes full name and all contact addresses, cumulatively. copyOf (UsersRestService.java:325) rebuilds the user through setFullName / Contact.setInfo, and both
run WebSecurityUtils.sanitizeString Encode.forHtml, which is not idempotent. Verified against the real config-model classes:
stored: O'Brien & Sons | bill&ted@example.com save 1: O'Brien & Sons | bill&ted@example.com save 2: O'Brien ...
So PUT /users/{id} and PUT /users/{id}/password mangle fullName, email, pagerEmail, and every unexposed contact (XMPP, phones, numeric/text pager PINs and service providers) one layer deeper each time, for any value containing & < > " '. O'Brien becomes O'Brien in the table after a single unrelated role edit. This is new: the legacy JSP is stable at one layer because the browser decodes the entities out of the value= attribute before POSTing them back; a JSON round-trip doesn't. It also contradicts "the password survives updates untouched"-style claims in the PR body and in UsersAbout.vue ("preserved untouched"). Fix by cloning without the sanitizing setters (JAXB marshal/unmarshal uses field access, @XmlAccessorType(NONE) + field annotations) and only writing fullName/contacts when the DTO value actually differs from stored, the way userComments is already grandfathered. testUpdatePreservesUnexposedContacts misses it only because xmpp@jabber.example.com has nothing escapable.
- Repointing the menu drops the only admin path to half the user record. modifyUser.jsp edits duty schedules, numeric/text paging service + PIN, work/mobile/home phone, telephone PIN,
and time zone; the new page edits none of them and neither does the API surface them for editing. After this change, "User Management Manage Users" reaches the lesser editor, and the
full one survives only under Admin "Configure Users, Groups and On-Call Roles" "Configure Users". Duty schedules and pager contacts are load-bearing for notifd. Either keep the menu
on the JSP until parity, or add a link from the new page to the legacy editor for the uncovered fields.
… write copyOf rebuilt the user through the config-model setters (setFullName, Contact.setInfo), which run a non-idempotent Encode.forHtml, so every write — a password change or any unrelated edit — re-escaped the stored value one layer deeper (O'Brien -> O'Brien -> O&#39;Brien ...). Clone via a JAXB round-trip (field access bypasses the sanitizing setters), and in applyDto only write full name / email / pager-email through the setter when the value actually changed. New IT stores escapable values and asserts a password change and an unrelated edit leave them byte-for-byte unchanged. Also fixes the build-ui lint failure (arrow-parens/quote style).
The new page (now the menu target) edited only name, email, pager-email, comments and roles, dropping the legacy modifyUser.jsp fields that notifd relies on. Surface the rest so the menu no longer routes to a lesser editor: - API: UserDto gains work/mobile/home phone and numeric/text pager service + PIN; UsersRestService maps them to the workPhone/mobilePhone/homePhone and numericPage/textPage contacts (info + serviceProvider), writing through the sanitizing setters only when a value changed. tui-pin, time-zone-id and duty schedules were already on the DTO. - UI: the editor adds phones, pager service/PIN, telephone PIN, a time-zone picker, and an add/remove duty-schedule list. ITs cover create + partial-update round-trips of the new contacts; editor tests cover load/save of the new fields and the duty-schedule list.
|
@marshallmassengill — both blockers addressed, plus the red build-ui check. Cumulative HTML-escaping of full name and contacts. Root-caused to Menu routed to a lesser editor. Rather than repoint the menu, I brought the new page to parity so it's no longer lesser: build-ui was failing on lint (arrow-parens/quote style) — fixed,
|
Replace the raw MoWeFr800-1700 text inputs in the user editor with a DutyScheduleEditor: each coverage window is a row of day toggles (Mo–Su) plus native begin/end time pickers, parsed from and serialized back to the canonical schedule string. A hand-edited entry the structured form can't represent stays editable as raw text so it is never silently dropped.
|
@marshallmassengill friendly ping — the cumulative HTML-escaping fix (JAXB copy + change-only writes), the field-parity work (phones, pager service/PIN, telephone PIN, time zone, and a day/time duty-schedule editor), and the build-ui lint failure are all pushed and green. Ready for another pass when you have a moment. |
# Conflicts: # opennms-webapp/src/main/webapp/WEB-INF/applicationContext-spring-security.xml # ui/src/components/Common/AboutDialogButton.vue # ui/src/main/router/index.ts # ui/src/services/index.ts
|
Merged current |
marshallmassengill
left a comment
marshallmassengill
left a comment
There was a problem hiding this comment.
Couple items:
- UsersRestService.java:403 (high) — tuiPin is the only field written with no validation, but users.xsd constrains tui-pin to [0-9]+. A PUT with 1234x gets into m_users via _writeUser, then _saveCurrent's schema-validating marshal throws: 500, users.xml unwritten, and since mtime/size are unchanged UserFactory.isUpdateNeeded() stays false, so the bad user is pinned in memory and every later user create/update/delete fails until restart.
- UsersRestService.java:213 (medium) — updateUser/setPassword have no rollbackPhantomUser equivalent (which createUser does have at :173). A failed setPassword returns 500 with the new hash live in memory, so the user logs in with it until a restart silently reverts. Same at :235.
- DutyScheduleEditor.vue:134 (medium) — serialize() canonicalizes day order and strips zero-padding, and the mount-time watch emits, so merely opening the dialog rewrites MoTu2000-0800 to MoTu2000-800. The server grandfathers only byte-identical strings, so validateDutySchedule then rejects it and the admin can't save even an email-only edit. The existing test passes only because its fixture is already canonical.
…tes !smoke users.xsd constrains tui-pin to [0-9]+ but it was the only field written unvalidated: a bad PIN entered the in-memory map, failed the schema-validating marshal, and — with isUpdateNeeded() seeing an unchanged file — wedged every later user mutation until restart. It is now rejected with a 400 before it touches the map, and the IT proves a valid write still succeeds after the rejected one. updateUser and setPassword get the same rollback discipline createUser already had: a failed save re-saves the previous state, so a 500 no longer leaves a new password hash live in memory until a restart quietly reverts it. The duty-schedule editor no longer rewrites schedules on open: parsed rows keep their incoming string and serialize it byte-identically until the row itself is edited, and the watcher only emits when the value actually changes. The server grandfathers legacy strings byte-identically, so the old mount-time canonicalization (day order, zero-padding) made such users unsavable for unrelated edits. Tests cover the non-canonical shape from the review: no emit on mount, untouched rows survive verbatim, canonicalization only on a real edit.
|
@marshallmassengill all three addressed in
|
3 participants
NMS-20106: a versioned user management API (
/api/v2/users) so users can be provisioned by external tooling, and a PrimeVue Manage Users page that is a straight visualization of it.users.xmlstays the system of record via the existingUserManager; fields the API does not expose (XMPP among them, deliberately hidden in the UI) and the password survive updates untouched.users.xmlnever touched) plus Vitest store/dialog tests; verified end to end against a local instance including byte-levelusers.xmlrestoration.