From 03147ef25c1f0464e3bd41b288f3446e268c91f3 Mon Sep 17 00:00:00 2001 From: Jose Anes Date: Thu, 30 Jul 2026 08:37:52 -0400 Subject: [PATCH 1/8] NMS-20106: versioned user management API and PrimeVue Manage Users page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../opennms/web/rest/v2/UsersRestService.java | 323 ++++++++++++++++++ .../opennms/web/rest/v2/api/UsersRestApi.java | 97 ++++++ .../opennms/web/rest/v2/model/UserDto.java | 151 ++++++++ .../rest/v2/model/UserPasswordRequest.java | 43 +++ .../web/rest/v2/model/UserRenameRequest.java | 43 +++ .../web/rest/v2/model/UserWriteRequest.java | 49 +++ .../webapp/WEB-INF/menu/menu-template.json | 2 +- .../applicationContext-spring-security.xml | 6 + .../ManageUsers/UserEditorDialog.vue | 221 ++++++++++++ .../ManageUsers/UserPasswordDialog.vue | 124 +++++++ .../ManageUsers/UserRenameDialog.vue | 104 ++++++ ui/src/components/ManageUsers/UsersTable.vue | 237 +++++++++++++ ui/src/containers/ManageUsers.vue | 52 +++ ui/src/main/router/index.ts | 19 ++ ui/src/services/index.ts | 18 +- ui/src/services/userAdminService.ts | 138 ++++++++ ui/src/stores/userAdminStore.ts | 92 +++++ ui/src/types/userAdmin.ts | 47 +++ ui/tests/stores/userAdminStore.test.ts | 109 ++++++ 19 files changed, 1873 insertions(+), 2 deletions(-) create mode 100644 opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/UsersRestService.java create mode 100644 opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/api/UsersRestApi.java create mode 100644 opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserDto.java create mode 100644 opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserPasswordRequest.java create mode 100644 opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserRenameRequest.java create mode 100644 opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserWriteRequest.java create mode 100644 ui/src/components/ManageUsers/UserEditorDialog.vue create mode 100644 ui/src/components/ManageUsers/UserPasswordDialog.vue create mode 100644 ui/src/components/ManageUsers/UserRenameDialog.vue create mode 100644 ui/src/components/ManageUsers/UsersTable.vue create mode 100644 ui/src/containers/ManageUsers.vue create mode 100644 ui/src/services/userAdminService.ts create mode 100644 ui/src/stores/userAdminStore.ts create mode 100644 ui/src/types/userAdmin.ts create mode 100644 ui/tests/stores/userAdminStore.test.ts diff --git a/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/UsersRestService.java b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/UsersRestService.java new file mode 100644 index 000000000000..41ebc2c7ac2f --- /dev/null +++ b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/UsersRestService.java @@ -0,0 +1,323 @@ +/* + * Licensed to The OpenNMS Group, Inc (TOG) under one or more + * contributor license agreements. See the LICENSE.md file + * distributed with this work for additional information + * regarding copyright ownership. + * + * TOG licenses this file to You under the GNU Affero General + * Public License Version 3 (the "License") or (at your option) + * any later version. You may not use this file except in + * compliance with the License. You may obtain a copy of the + * License at: + * + * https://www.gnu.org/licenses/agpl-3.0.txt + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied. See the License for the specific + * language governing permissions and limitations under the + * License. + */ +package org.opennms.web.rest.v2; + +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.regex.Pattern; + +import javax.ws.rs.core.Response; +import javax.ws.rs.core.Response.Status; +import javax.ws.rs.core.SecurityContext; + +import org.opennms.netmgt.config.UserManager; +import org.opennms.netmgt.config.api.UserConfig.ContactType; +import org.opennms.netmgt.config.users.Contact; +import org.opennms.netmgt.config.users.User; +import org.opennms.web.api.Authentication; +import org.opennms.web.rest.v2.api.UsersRestApi; +import org.opennms.web.rest.v2.model.UserDto; +import org.opennms.web.rest.v2.model.UserPasswordRequest; +import org.opennms.web.rest.v2.model.UserRenameRequest; +import org.opennms.web.rest.v2.model.UserWriteRequest; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Component; + +/** + * Versioned user management on top of {@link UserManager}: users.xml remains + * the system of record and hand-editing keeps working. Unlike the legacy JSPs + * (which only hid the buttons), the admin/rtc delete and rename protections + * are enforced here, server-side. The password hash is never serialized. + */ +@Component("usersRestServiceV2") +public class UsersRestService implements UsersRestApi { + + private static final Logger LOG = LoggerFactory.getLogger(UsersRestService.class); + + /** System accounts that must not be deleted or renamed. */ + private static final Set PROTECTED_USERS = Set.of("admin", "rtc"); + + /** Mirrors the legacy servlets' markup check on user ids. */ + private static final Pattern INVALID_USER_ID = Pattern.compile(".*[&<>\"`']+.*"); + + @Autowired + private UserManager m_userManager; + + @Override + public Response listUsers(final SecurityContext securityContext) { + assertAdmin(securityContext); + try { + final List users = new ArrayList<>(); + for (final User user : m_userManager.getUsers().values()) { + users.add(toDto(user)); + } + users.sort(Comparator.comparing(UserDto::getUserId, String.CASE_INSENSITIVE_ORDER)); + return Response.ok(users).build(); + } catch (final Exception e) { + return serverError("Can't read users: %s", e); + } + } + + @Override + public Response getUser(final SecurityContext securityContext, final String userId) { + assertAdmin(securityContext); + try { + final User user = m_userManager.getUser(userId); + if (user == null) { + return Response.status(Status.NOT_FOUND).entity("User " + userId + " was not found.").build(); + } + return Response.ok(toDto(user)).build(); + } catch (final Exception e) { + return serverError("Can't read user: %s", e); + } + } + + @Override + public Response listAvailableRoles(final SecurityContext securityContext) { + assertAdmin(securityContext); + final List roles = new ArrayList<>(Authentication.getAvailableRoles()); + roles.sort(String.CASE_INSENSITIVE_ORDER); + return Response.ok(roles).build(); + } + + @Override + public Response createUser(final SecurityContext securityContext, final UserWriteRequest request) { + assertAdmin(securityContext); + if (request == null || isBlank(request.getUserId())) { + return Response.status(Status.BAD_REQUEST).entity("A user-id is required.").build(); + } + final String userId = request.getUserId().trim(); + if (INVALID_USER_ID.matcher(userId).matches()) { + return Response.status(Status.BAD_REQUEST).entity("The user-id must not contain any HTML markup.").build(); + } + if (isBlank(request.getPassword())) { + return Response.status(Status.BAD_REQUEST).entity("A password is required.").build(); + } + try { + if (m_userManager.hasUser(userId)) { + return Response.status(Status.BAD_REQUEST).entity("User " + userId + " already exists.").build(); + } + final User user = new User(); + user.setUserId(userId); + user.setPassword(m_userManager.encryptedPassword(request.getPassword(), true), Boolean.TRUE); + applyDto(user, request); + m_userManager.saveUser(userId, user); + LOG.info("User {} created by {}", userId, securityContext.getUserPrincipal() == null ? "?" : securityContext.getUserPrincipal().getName()); + return Response.status(Status.CREATED).build(); + } catch (final Exception e) { + return serverError("Can't create user: %s", e); + } + } + + @Override + public Response updateUser(final SecurityContext securityContext, final String userId, final UserDto dto) { + assertAdmin(securityContext); + if (dto == null) { + return Response.status(Status.BAD_REQUEST).entity("A user body is required.").build(); + } + try { + final User user = m_userManager.getUser(userId); + if (user == null) { + return Response.status(Status.NOT_FOUND).entity("User " + userId + " was not found.").build(); + } + applyDto(user, dto); + m_userManager.saveUser(userId, user); + return Response.noContent().build(); + } catch (final Exception e) { + return serverError("Can't update user: %s", e); + } + } + + @Override + public Response setPassword(final SecurityContext securityContext, final String userId, final UserPasswordRequest request) { + assertAdmin(securityContext); + if (request == null || isBlank(request.getPassword())) { + return Response.status(Status.BAD_REQUEST).entity("A password is required.").build(); + } + try { + final User user = m_userManager.getUser(userId); + if (user == null) { + return Response.status(Status.NOT_FOUND).entity("User " + userId + " was not found.").build(); + } + user.setPassword(m_userManager.encryptedPassword(request.getPassword(), true), Boolean.TRUE); + m_userManager.saveUser(userId, user); + LOG.info("Password changed for user {} by {}", userId, securityContext.getUserPrincipal() == null ? "?" : securityContext.getUserPrincipal().getName()); + return Response.noContent().build(); + } catch (final Exception e) { + return serverError("Can't change password: %s", e); + } + } + + @Override + public Response renameUser(final SecurityContext securityContext, final String userId, final UserRenameRequest request) { + assertAdmin(securityContext); + if (request == null || isBlank(request.getNewUserId())) { + return Response.status(Status.BAD_REQUEST).entity("A new-user-id is required.").build(); + } + if (PROTECTED_USERS.contains(userId)) { + return Response.status(Status.BAD_REQUEST).entity("The system user " + userId + " cannot be renamed.").build(); + } + final String newUserId = request.getNewUserId().trim(); + if (INVALID_USER_ID.matcher(newUserId).matches()) { + return Response.status(Status.BAD_REQUEST).entity("The user-id must not contain any HTML markup.").build(); + } + try { + if (!m_userManager.hasUser(userId)) { + return Response.status(Status.NOT_FOUND).entity("User " + userId + " was not found.").build(); + } + if (m_userManager.hasUser(newUserId)) { + return Response.status(Status.BAD_REQUEST).entity("User " + newUserId + " already exists.").build(); + } + m_userManager.renameUser(userId, newUserId); + LOG.info("User {} renamed to {} by {}", userId, newUserId, securityContext.getUserPrincipal() == null ? "?" : securityContext.getUserPrincipal().getName()); + return Response.noContent().build(); + } catch (final Exception e) { + return serverError("Can't rename user: %s", e); + } + } + + @Override + public Response deleteUser(final SecurityContext securityContext, final String userId) { + assertAdmin(securityContext); + if (PROTECTED_USERS.contains(userId)) { + return Response.status(Status.BAD_REQUEST).entity("The system user " + userId + " cannot be deleted.").build(); + } + try { + if (!m_userManager.hasUser(userId)) { + return Response.status(Status.NOT_FOUND).entity("User " + userId + " was not found.").build(); + } + m_userManager.deleteUser(userId); + LOG.info("User {} deleted by {}", userId, securityContext.getUserPrincipal() == null ? "?" : securityContext.getUserPrincipal().getName()); + return Response.noContent().build(); + } catch (final Exception e) { + return serverError("Can't delete user: %s", e); + } + } + + private UserDto toDto(final User user) { + final UserDto dto = new UserDto(); + dto.setUserId(user.getUserId()); + dto.setFullName(user.getFullName().orElse(null)); + dto.setUserComments(user.getUserComments().orElse(null)); + dto.setEmail(contactInfo(user, ContactType.email)); + dto.setPagerEmail(contactInfo(user, ContactType.pagerEmail)); + dto.setTuiPin(user.getTuiPin().orElse(null)); + dto.setTimeZoneId(user.getTimeZoneId().map(Objects::toString).orElse(null)); + dto.setDutySchedules(new ArrayList<>(user.getDutySchedules())); + dto.setRoles(new ArrayList<>(user.getRoles())); + dto.setReadOnly(user.getRoles().contains(Authentication.ROLE_READONLY)); + return dto; + } + + /** + * Applies the DTO onto the JAXB user. Only the exposed contact types + * (email, pagerEmail) are touched; every other contact — XMPP, microblog, + * phones, paging services — and the password survive untouched, so a v2 + * update can never corrupt hand-maintained users.xml entries. + */ + private void applyDto(final User user, final UserDto dto) { + user.setFullName(trimToNull(dto.getFullName())); + user.setUserComments(trimToNull(dto.getUserComments())); + user.setTuiPin(trimToNull(dto.getTuiPin())); + final String timeZoneId = trimToNull(dto.getTimeZoneId()); + if (timeZoneId == null) { + user.setTimeZoneId((java.time.ZoneId) null); + } else { + try { + user.setTimeZoneId(timeZoneId); + } catch (final RuntimeException e) { + throw new IllegalArgumentException("Invalid time-zone-id: " + timeZoneId); + } + } + setContact(user, ContactType.email, dto.getEmail()); + setContact(user, ContactType.pagerEmail, dto.getPagerEmail()); + if (dto.getDutySchedules() != null) { + user.setDutySchedules(new ArrayList<>(dto.getDutySchedules())); + } + if (dto.getRoles() != null) { + for (final String role : dto.getRoles()) { + if (!Authentication.isValidRole(role)) { + throw new IllegalArgumentException("Unknown security role: " + role); + } + } + user.setRoles(new ArrayList<>(dto.getRoles())); + } + } + + private static String contactInfo(final User user, final ContactType type) { + return user.getContacts().stream() + .filter(c -> type.name().equals(c.getType())) + .findFirst() + .flatMap(Contact::getInfo) + .filter(info -> !info.isEmpty()) + .orElse(null); + } + + private static void setContact(final User user, final ContactType type, final String value) { + final Optional existing = user.getContacts().stream() + .filter(c -> type.name().equals(c.getType())) + .findFirst(); + final String trimmed = trimToNull(value); + if (existing.isPresent()) { + existing.get().setInfo(trimmed == null ? "" : trimmed); + } else if (trimmed != null) { + final Contact contact = new Contact(type.name()); + contact.setInfo(trimmed); + user.getContacts().add(contact); + } + } + + private static void assertAdmin(final SecurityContext securityContext) { + if (securityContext == null || !securityContext.isUserInRole(Authentication.ROLE_ADMIN)) { + throw new javax.ws.rs.WebApplicationException( + Response.status(Status.FORBIDDEN).entity("User management requires the admin role.").build()); + } + } + + + + private Response serverError(final String format, final Exception e) { + if (e instanceof IllegalArgumentException) { + return Response.status(Status.BAD_REQUEST).entity(e.getMessage()).build(); + } + LOG.error(String.format(format, e.getMessage()), e); + return Response.status(Status.INTERNAL_SERVER_ERROR).entity(String.format(format, e.getMessage())).build(); + } + + private static boolean isBlank(final String value) { + return value == null || value.isBlank(); + } + + private static String trimToNull(final String value) { + if (value == null) { + return null; + } + final String trimmed = value.trim(); + return trimmed.isEmpty() ? null : trimmed; + } +} diff --git a/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/api/UsersRestApi.java b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/api/UsersRestApi.java new file mode 100644 index 000000000000..2fb48440c6bb --- /dev/null +++ b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/api/UsersRestApi.java @@ -0,0 +1,97 @@ +/* + * Licensed to The OpenNMS Group, Inc (TOG) under one or more + * contributor license agreements. See the LICENSE.md file + * distributed with this work for additional information + * regarding copyright ownership. + * + * TOG licenses this file to You under the GNU Affero General + * Public License Version 3 (the "License") or (at your option) + * any later version. You may not use this file except in + * compliance with the License. You may obtain a copy of the + * License at: + * + * https://www.gnu.org/licenses/agpl-3.0.txt + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied. See the License for the specific + * language governing permissions and limitations under the + * License. + */ +package org.opennms.web.rest.v2.api; + +import javax.ws.rs.Consumes; +import javax.ws.rs.DELETE; +import javax.ws.rs.GET; +import javax.ws.rs.POST; +import javax.ws.rs.PUT; +import javax.ws.rs.Path; +import javax.ws.rs.PathParam; +import javax.ws.rs.Produces; +import javax.ws.rs.core.Context; +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; +import javax.ws.rs.core.SecurityContext; + +import org.opennms.web.rest.v2.model.UserDto; +import org.opennms.web.rest.v2.model.UserPasswordRequest; +import org.opennms.web.rest.v2.model.UserRenameRequest; +import org.opennms.web.rest.v2.model.UserWriteRequest; + +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.tags.Tag; + +/** + * Versioned user management API backed by users.xml. Password hashes are + * never returned; admin-only (enforced by Spring Security and in-code). + */ +@Path("users") +@Tag(name = "Users", description = "User Management API") +public interface UsersRestApi { + + @GET + @Produces(MediaType.APPLICATION_JSON) + @Operation(summary = "List all users", operationId = "listUsers") + Response listUsers(@Context SecurityContext securityContext); + + @GET + @Path("{userId}") + @Produces(MediaType.APPLICATION_JSON) + @Operation(summary = "Get one user", operationId = "getUser") + Response getUser(@Context SecurityContext securityContext, @PathParam("userId") String userId); + + @GET + @Path("available-roles") + @Produces(MediaType.APPLICATION_JSON) + @Operation(summary = "List the assignable security roles", operationId = "listAvailableRoles") + Response listAvailableRoles(@Context SecurityContext securityContext); + + @POST + @Consumes(MediaType.APPLICATION_JSON) + @Operation(summary = "Create a user", operationId = "createUser") + Response createUser(@Context SecurityContext securityContext, UserWriteRequest request); + + @PUT + @Path("{userId}") + @Consumes(MediaType.APPLICATION_JSON) + @Operation(summary = "Update a user's details (contact types and password not covered here are preserved)", operationId = "updateUser") + Response updateUser(@Context SecurityContext securityContext, @PathParam("userId") String userId, UserDto user); + + @PUT + @Path("{userId}/password") + @Consumes(MediaType.APPLICATION_JSON) + @Operation(summary = "Set a user's password (stored salted)", operationId = "setUserPassword") + Response setPassword(@Context SecurityContext securityContext, @PathParam("userId") String userId, UserPasswordRequest request); + + @POST + @Path("{userId}/rename") + @Consumes(MediaType.APPLICATION_JSON) + @Operation(summary = "Rename a user (also updates group memberships)", operationId = "renameUser") + Response renameUser(@Context SecurityContext securityContext, @PathParam("userId") String userId, UserRenameRequest request); + + @DELETE + @Path("{userId}") + @Operation(summary = "Delete a user (also removes group memberships; admin and rtc are protected)", operationId = "deleteUser") + Response deleteUser(@Context SecurityContext securityContext, @PathParam("userId") String userId); +} diff --git a/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserDto.java b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserDto.java new file mode 100644 index 000000000000..1542f6e610b1 --- /dev/null +++ b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserDto.java @@ -0,0 +1,151 @@ +/* + * Licensed to The OpenNMS Group, Inc (TOG) under one or more + * contributor license agreements. See the LICENSE.md file + * distributed with this work for additional information + * regarding copyright ownership. + * + * TOG licenses this file to You under the GNU Affero General + * Public License Version 3 (the "License") or (at your option) + * any later version. You may not use this file except in + * compliance with the License. You may obtain a copy of the + * License at: + * + * https://www.gnu.org/licenses/agpl-3.0.txt + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied. See the License for the specific + * language governing permissions and limitations under the + * License. + */ +package org.opennms.web.rest.v2.model; + +import java.util.ArrayList; +import java.util.List; + +import javax.xml.bind.annotation.XmlAccessType; +import javax.xml.bind.annotation.XmlAccessorType; +import javax.xml.bind.annotation.XmlElement; +import javax.xml.bind.annotation.XmlRootElement; + +/** + * A user as exposed by the v2 user management API. Field names mirror + * users.xml where they overlap; the password hash is deliberately never part + * of this representation, and contact types the API does not expose (XMPP, + * microblog, phones, pager PINs) are preserved server-side on update. + */ +@XmlRootElement(name = "user") +@XmlAccessorType(XmlAccessType.FIELD) +public class UserDto { + + @XmlElement(name = "user-id") + private String userId; + + @XmlElement(name = "full-name") + private String fullName; + + @XmlElement(name = "user-comments") + private String userComments; + + @XmlElement(name = "email") + private String email; + + @XmlElement(name = "pager-email") + private String pagerEmail; + + @XmlElement(name = "tui-pin") + private String tuiPin; + + @XmlElement(name = "time-zone-id") + private String timeZoneId; + + @XmlElement(name = "duty-schedule") + private List dutySchedules = new ArrayList<>(); + + @XmlElement(name = "role") + private List roles = new ArrayList<>(); + + @XmlElement(name = "read-only") + private Boolean readOnly; + + public String getUserId() { + return userId; + } + + public void setUserId(final String userId) { + this.userId = userId; + } + + public String getFullName() { + return fullName; + } + + public void setFullName(final String fullName) { + this.fullName = fullName; + } + + public String getUserComments() { + return userComments; + } + + public void setUserComments(final String userComments) { + this.userComments = userComments; + } + + public String getEmail() { + return email; + } + + public void setEmail(final String email) { + this.email = email; + } + + public String getPagerEmail() { + return pagerEmail; + } + + public void setPagerEmail(final String pagerEmail) { + this.pagerEmail = pagerEmail; + } + + public String getTuiPin() { + return tuiPin; + } + + public void setTuiPin(final String tuiPin) { + this.tuiPin = tuiPin; + } + + public String getTimeZoneId() { + return timeZoneId; + } + + public void setTimeZoneId(final String timeZoneId) { + this.timeZoneId = timeZoneId; + } + + public List getDutySchedules() { + return dutySchedules; + } + + public void setDutySchedules(final List dutySchedules) { + this.dutySchedules = dutySchedules; + } + + public List getRoles() { + return roles; + } + + public void setRoles(final List roles) { + this.roles = roles; + } + + public Boolean getReadOnly() { + return readOnly; + } + + public void setReadOnly(final Boolean readOnly) { + this.readOnly = readOnly; + } +} diff --git a/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserPasswordRequest.java b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserPasswordRequest.java new file mode 100644 index 000000000000..da123fc68779 --- /dev/null +++ b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserPasswordRequest.java @@ -0,0 +1,43 @@ +/* + * Licensed to The OpenNMS Group, Inc (TOG) under one or more + * contributor license agreements. See the LICENSE.md file + * distributed with this work for additional information + * regarding copyright ownership. + * + * TOG licenses this file to You under the GNU Affero General + * Public License Version 3 (the "License") or (at your option) + * any later version. You may not use this file except in + * compliance with the License. You may obtain a copy of the + * License at: + * + * https://www.gnu.org/licenses/agpl-3.0.txt + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied. See the License for the specific + * language governing permissions and limitations under the + * License. + */ +package org.opennms.web.rest.v2.model; + +import javax.xml.bind.annotation.XmlAccessType; +import javax.xml.bind.annotation.XmlAccessorType; +import javax.xml.bind.annotation.XmlElement; +import javax.xml.bind.annotation.XmlRootElement; + +@XmlRootElement(name = "user-password-request") +@XmlAccessorType(XmlAccessType.FIELD) +public class UserPasswordRequest { + + @XmlElement(name = "password") + private String password; + + public String getPassword() { + return password; + } + + public void setPassword(final String password) { + this.password = password; + } +} diff --git a/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserRenameRequest.java b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserRenameRequest.java new file mode 100644 index 000000000000..6d08c3664ef3 --- /dev/null +++ b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserRenameRequest.java @@ -0,0 +1,43 @@ +/* + * Licensed to The OpenNMS Group, Inc (TOG) under one or more + * contributor license agreements. See the LICENSE.md file + * distributed with this work for additional information + * regarding copyright ownership. + * + * TOG licenses this file to You under the GNU Affero General + * Public License Version 3 (the "License") or (at your option) + * any later version. You may not use this file except in + * compliance with the License. You may obtain a copy of the + * License at: + * + * https://www.gnu.org/licenses/agpl-3.0.txt + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied. See the License for the specific + * language governing permissions and limitations under the + * License. + */ +package org.opennms.web.rest.v2.model; + +import javax.xml.bind.annotation.XmlAccessType; +import javax.xml.bind.annotation.XmlAccessorType; +import javax.xml.bind.annotation.XmlElement; +import javax.xml.bind.annotation.XmlRootElement; + +@XmlRootElement(name = "user-rename-request") +@XmlAccessorType(XmlAccessType.FIELD) +public class UserRenameRequest { + + @XmlElement(name = "new-user-id") + private String newUserId; + + public String getNewUserId() { + return newUserId; + } + + public void setNewUserId(final String newUserId) { + this.newUserId = newUserId; + } +} diff --git a/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserWriteRequest.java b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserWriteRequest.java new file mode 100644 index 000000000000..525ec0793052 --- /dev/null +++ b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserWriteRequest.java @@ -0,0 +1,49 @@ +/* + * Licensed to The OpenNMS Group, Inc (TOG) under one or more + * contributor license agreements. See the LICENSE.md file + * distributed with this work for additional information + * regarding copyright ownership. + * + * TOG licenses this file to You under the GNU Affero General + * Public License Version 3 (the "License") or (at your option) + * any later version. You may not use this file except in + * compliance with the License. You may obtain a copy of the + * License at: + * + * https://www.gnu.org/licenses/agpl-3.0.txt + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied. See the License for the specific + * language governing permissions and limitations under the + * License. + */ +package org.opennms.web.rest.v2.model; + +import javax.xml.bind.annotation.XmlAccessType; +import javax.xml.bind.annotation.XmlAccessorType; +import javax.xml.bind.annotation.XmlElement; +import javax.xml.bind.annotation.XmlRootElement; + +/** + * Create request for the v2 user management API: the UserDto fields plus the + * initial password, which is hashed (salted) before it reaches users.xml. + * Password changes on existing users go through the dedicated password + * endpoint instead. + */ +@XmlRootElement(name = "user-create-request") +@XmlAccessorType(XmlAccessType.FIELD) +public class UserWriteRequest extends UserDto { + + @XmlElement(name = "password") + private String password; + + public String getPassword() { + return password; + } + + public void setPassword(final String password) { + this.password = password; + } +} diff --git a/opennms-webapp-rest/src/main/webapp/WEB-INF/menu/menu-template.json b/opennms-webapp-rest/src/main/webapp/WEB-INF/menu/menu-template.json index d5a81390467e..a715f471e540 100644 --- a/opennms-webapp-rest/src/main/webapp/WEB-INF/menu/menu-template.json +++ b/opennms-webapp-rest/src/main/webapp/WEB-INF/menu/menu-template.json @@ -404,7 +404,7 @@ { "id": "manageUsers", "name": "Manage Users", - "url": "admin/userGroupView/users/list.jsp", + "url": "ui/index.html#/admin/users", "locationMatch": "", "roles": null }, diff --git a/opennms-webapp/src/main/webapp/WEB-INF/applicationContext-spring-security.xml b/opennms-webapp/src/main/webapp/WEB-INF/applicationContext-spring-security.xml index 4829fe6357f7..8769d00b10e8 100644 --- a/opennms-webapp/src/main/webapp/WEB-INF/applicationContext-spring-security.xml +++ b/opennms-webapp/src/main/webapp/WEB-INF/applicationContext-spring-security.xml @@ -193,6 +193,12 @@ + + + + + + diff --git a/ui/src/components/ManageUsers/UserEditorDialog.vue b/ui/src/components/ManageUsers/UserEditorDialog.vue new file mode 100644 index 000000000000..b6bb585eff77 --- /dev/null +++ b/ui/src/components/ManageUsers/UserEditorDialog.vue @@ -0,0 +1,221 @@ + + + + + diff --git a/ui/src/components/ManageUsers/UserPasswordDialog.vue b/ui/src/components/ManageUsers/UserPasswordDialog.vue new file mode 100644 index 000000000000..e1a5c11b7f4a --- /dev/null +++ b/ui/src/components/ManageUsers/UserPasswordDialog.vue @@ -0,0 +1,124 @@ + + + + + diff --git a/ui/src/components/ManageUsers/UserRenameDialog.vue b/ui/src/components/ManageUsers/UserRenameDialog.vue new file mode 100644 index 000000000000..a1413d1a64f4 --- /dev/null +++ b/ui/src/components/ManageUsers/UserRenameDialog.vue @@ -0,0 +1,104 @@ + + + + + diff --git a/ui/src/components/ManageUsers/UsersTable.vue b/ui/src/components/ManageUsers/UsersTable.vue new file mode 100644 index 000000000000..f17d5547e79e --- /dev/null +++ b/ui/src/components/ManageUsers/UsersTable.vue @@ -0,0 +1,237 @@ + + + + + diff --git a/ui/src/containers/ManageUsers.vue b/ui/src/containers/ManageUsers.vue new file mode 100644 index 000000000000..7f607ba8d1e6 --- /dev/null +++ b/ui/src/containers/ManageUsers.vue @@ -0,0 +1,52 @@ + + + + + diff --git a/ui/src/main/router/index.ts b/ui/src/main/router/index.ts index 52a1b1b07ba4..57c10adb438f 100644 --- a/ui/src/main/router/index.ts +++ b/ui/src/main/router/index.ts @@ -158,6 +158,25 @@ const router = createRouter({ } } }, + { + path: '/admin/users', + name: 'Manage Users', + component: () => import('@/containers/ManageUsers.vue'), + beforeEnter: (to, from) => { + const checkRoles = () => { + if (!adminRole.value) { + showSnackBar({ msg: 'Must be admin to manage users.' }) + router.push(from.path) + } + } + + if (rolesAreLoaded.value) { + checkRoles() + } else { + whenever(rolesAreLoaded, () => checkRoles()) + } + } + }, { path: '/map', name: 'Map', diff --git a/ui/src/services/index.ts b/ui/src/services/index.ts index 8b8c60f273ea..c19b572413fc 100644 --- a/ui/src/services/index.ts +++ b/ui/src/services/index.ts @@ -74,6 +74,15 @@ import { setUsageStatisticsStatus } from './usageStatisticsService' import { addZenithRegistration, getZenithRegistrations } from './zenithConnectService' +import { + createManagedUser, + deleteManagedUser, + getAvailableUserRoles, + getManagedUsers, + renameManagedUser, + setManagedUserPassword, + updateManagedUser +} from './userAdminService' export default { search, @@ -135,5 +144,12 @@ export default { setUsageStatisticsStatus, addZenithRegistration, getZenithRegistrations, - performLogout + performLogout, + createManagedUser, + deleteManagedUser, + getAvailableUserRoles, + getManagedUsers, + renameManagedUser, + setManagedUserPassword, + updateManagedUser } diff --git a/ui/src/services/userAdminService.ts b/ui/src/services/userAdminService.ts new file mode 100644 index 000000000000..79949f5e83ae --- /dev/null +++ b/ui/src/services/userAdminService.ts @@ -0,0 +1,138 @@ +/// +/// Licensed to The OpenNMS Group, Inc (TOG) under one or more +/// contributor license agreements. See the LICENSE.md file +/// distributed with this work for additional information +/// regarding copyright ownership. +/// +/// TOG licenses this file to You under the GNU Affero General +/// Public License Version 3 (the "License") or (at your option) +/// any later version. You may not use this file except in +/// compliance with the License. You may obtain a copy of the +/// License at: +/// +/// https://www.gnu.org/licenses/agpl-3.0.txt +/// +/// Unless required by applicable law or agreed to in writing, +/// software distributed under the License is distributed on an +/// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +/// either express or implied. See the License for the specific +/// language governing permissions and limitations under the +/// License. +/// + +import useSnackbar from '@/composables/useSnackbar' +import useSpinner from '@/composables/useSpinner' +import { ManagedUser, ManagedUserCreate } from '@/types/userAdmin' +import { v2 } from './axiosInstances' + +const { showSnackBar } = useSnackbar() +const { startSpinner, stopSpinner } = useSpinner() +const endpoint = '/users' + +const errorMessage = (err: any, fallback: string): string => { + const detail = err?.response?.data + return typeof detail === 'string' && detail ? detail : fallback +} + +const getManagedUsers = async (): Promise => { + try { + startSpinner() + const resp = await v2.get(endpoint) + return Array.isArray(resp.data) ? resp.data : [] + } catch (_err) { + showSnackBar({ msg: 'Failed to load users.' }) + return [] + } finally { + stopSpinner() + } +} + +const getAvailableUserRoles = async (): Promise => { + try { + const resp = await v2.get(`${endpoint}/available-roles`) + return Array.isArray(resp.data) ? resp.data : [] + } catch (_err) { + showSnackBar({ msg: 'Failed to load available roles.' }) + return [] + } +} + +const createManagedUser = async (user: ManagedUserCreate): Promise => { + try { + startSpinner() + await v2.post(endpoint, user) + showSnackBar({ msg: `User '${user['user-id']}' created.` }) + return true + } catch (err: any) { + showSnackBar({ msg: errorMessage(err, `Failed to create user '${user['user-id']}'.`) }) + return false + } finally { + stopSpinner() + } +} + +const updateManagedUser = async (user: ManagedUser): Promise => { + try { + startSpinner() + await v2.put(`${endpoint}/${encodeURIComponent(user['user-id'])}`, user) + showSnackBar({ msg: `User '${user['user-id']}' updated.` }) + return true + } catch (err: any) { + showSnackBar({ msg: errorMessage(err, `Failed to update user '${user['user-id']}'.`) }) + return false + } finally { + stopSpinner() + } +} + +const setManagedUserPassword = async (userId: string, password: string): Promise => { + try { + startSpinner() + await v2.put(`${endpoint}/${encodeURIComponent(userId)}/password`, { password }) + showSnackBar({ msg: `Password changed for '${userId}'.` }) + return true + } catch (err: any) { + showSnackBar({ msg: errorMessage(err, `Failed to change the password for '${userId}'.`) }) + return false + } finally { + stopSpinner() + } +} + +const renameManagedUser = async (userId: string, newUserId: string): Promise => { + try { + startSpinner() + await v2.post(`${endpoint}/${encodeURIComponent(userId)}/rename`, { 'new-user-id': newUserId }) + showSnackBar({ msg: `User '${userId}' renamed to '${newUserId}'.` }) + return true + } catch (err: any) { + showSnackBar({ msg: errorMessage(err, `Failed to rename user '${userId}'.`) }) + return false + } finally { + stopSpinner() + } +} + +const deleteManagedUser = async (userId: string): Promise => { + try { + startSpinner() + await v2.delete(`${endpoint}/${encodeURIComponent(userId)}`) + showSnackBar({ msg: `User '${userId}' deleted.` }) + return true + } catch (err: any) { + showSnackBar({ msg: errorMessage(err, `Failed to delete user '${userId}'.`) }) + return false + } finally { + stopSpinner() + } +} + +export { + createManagedUser, + deleteManagedUser, + getAvailableUserRoles, + getManagedUsers, + renameManagedUser, + setManagedUserPassword, + updateManagedUser +} diff --git a/ui/src/stores/userAdminStore.ts b/ui/src/stores/userAdminStore.ts new file mode 100644 index 000000000000..abcc7cf0ecbb --- /dev/null +++ b/ui/src/stores/userAdminStore.ts @@ -0,0 +1,92 @@ +/// +/// Licensed to The OpenNMS Group, Inc (TOG) under one or more +/// contributor license agreements. See the LICENSE.md file +/// distributed with this work for additional information +/// regarding copyright ownership. +/// +/// TOG licenses this file to You under the GNU Affero General +/// Public License Version 3 (the "License") or (at your option) +/// any later version. You may not use this file except in +/// compliance with the License. You may obtain a copy of the +/// License at: +/// +/// https://www.gnu.org/licenses/agpl-3.0.txt +/// +/// Unless required by applicable law or agreed to in writing, +/// software distributed under the License is distributed on an +/// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +/// either express or implied. See the License for the specific +/// language governing permissions and limitations under the +/// License. +/// + +import API from '@/services' +import { ManagedUser, ManagedUserCreate } from '@/types/userAdmin' +import { defineStore } from 'pinia' +import { ref } from 'vue' + +export const useUserAdminStore = defineStore('userAdminStore', () => { + const users = ref([] as ManagedUser[]) + const availableRoles = ref([] as string[]) + + const getUsers = async () => { + users.value = await API.getManagedUsers() + } + + const getAvailableRoles = async () => { + availableRoles.value = await API.getAvailableUserRoles() + } + + const createUser = async (user: ManagedUserCreate) => { + const ok = await API.createManagedUser(user) + if (ok) { + await getUsers() + } + return ok + } + + const updateUser = async (user: ManagedUser) => { + const ok = await API.updateManagedUser(user) + if (ok) { + await getUsers() + } + return ok + } + + const setPassword = async (userId: string, password: string) => { + return await API.setManagedUserPassword(userId, password) + } + + const renameUser = async (userId: string, newUserId: string) => { + const ok = await API.renameManagedUser(userId, newUserId) + if (ok) { + await getUsers() + } + return ok + } + + const deleteUser = async (userId: string) => { + const ok = await API.deleteManagedUser(userId) + if (ok) { + await getUsers() + } + return ok + } + + const populate = async () => { + await Promise.all([getUsers(), getAvailableRoles()]) + } + + return { + users, + availableRoles, + getUsers, + getAvailableRoles, + createUser, + updateUser, + setPassword, + renameUser, + deleteUser, + populate + } +}) diff --git a/ui/src/types/userAdmin.ts b/ui/src/types/userAdmin.ts new file mode 100644 index 000000000000..1abb6d6bfdc6 --- /dev/null +++ b/ui/src/types/userAdmin.ts @@ -0,0 +1,47 @@ +/// +/// Licensed to The OpenNMS Group, Inc (TOG) under one or more +/// contributor license agreements. See the LICENSE.md file +/// distributed with this work for additional information +/// regarding copyright ownership. +/// +/// TOG licenses this file to You under the GNU Affero General +/// Public License Version 3 (the "License") or (at your option) +/// any later version. You may not use this file except in +/// compliance with the License. You may obtain a copy of the +/// License at: +/// +/// https://www.gnu.org/licenses/agpl-3.0.txt +/// +/// Unless required by applicable law or agreed to in writing, +/// software distributed under the License is distributed on an +/// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +/// either express or implied. See the License for the specific +/// language governing permissions and limitations under the +/// License. +/// + +// Wire shapes of the v2 user management API (/api/v2/users). Field names +// follow users.xml; the password hash is never part of any response, and +// contact types the API does not expose (XMPP among them) are preserved +// server-side on update. + +export interface ManagedUser { + 'user-id': string + 'full-name'?: string | null + 'user-comments'?: string | null + email?: string | null + 'pager-email'?: string | null + 'tui-pin'?: string | null + 'time-zone-id'?: string | null + 'duty-schedule'?: string[] + role?: string[] + 'read-only'?: boolean +} + +export interface ManagedUserCreate extends ManagedUser { + password: string +} + +// System accounts the server refuses to delete or rename; mirrored here so +// the UI can disable the controls with an explanation instead of a 400. +export const PROTECTED_USER_IDS = ['admin', 'rtc'] diff --git a/ui/tests/stores/userAdminStore.test.ts b/ui/tests/stores/userAdminStore.test.ts new file mode 100644 index 000000000000..7608e3873733 --- /dev/null +++ b/ui/tests/stores/userAdminStore.test.ts @@ -0,0 +1,109 @@ +import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest' +import { setActivePinia, createPinia } from 'pinia' +import { useUserAdminStore } from '@/stores/userAdminStore' +import API from '@/services' +import { ManagedUser } from '@/types/userAdmin' + +vi.mock('@/services', () => ({ + default: { + getManagedUsers: vi.fn(), + getAvailableUserRoles: vi.fn(), + createManagedUser: vi.fn(), + updateManagedUser: vi.fn(), + setManagedUserPassword: vi.fn(), + renameManagedUser: vi.fn(), + deleteManagedUser: vi.fn() + } +})) + +describe('useUserAdminStore', () => { + let store: ReturnType + + const mockUsers: ManagedUser[] = [ + { 'user-id': 'admin', 'full-name': 'Administrator', role: ['ROLE_ADMIN'] }, + { 'user-id': 'noc', 'full-name': 'NOC Operator', email: 'noc@example.com', role: ['ROLE_USER'] } + ] + + beforeEach(() => { + setActivePinia(createPinia()) + store = useUserAdminStore() + vi.clearAllMocks() + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + it('should start empty', () => { + expect(store.users).toEqual([]) + expect(store.availableRoles).toEqual([]) + }) + + it('populate should load users and roles', async () => { + vi.mocked(API.getManagedUsers).mockResolvedValue(mockUsers) + vi.mocked(API.getAvailableUserRoles).mockResolvedValue(['ROLE_ADMIN', 'ROLE_USER']) + + await store.populate() + + expect(store.users).toEqual(mockUsers) + expect(store.availableRoles).toEqual(['ROLE_ADMIN', 'ROLE_USER']) + }) + + it('createUser should refresh on success', async () => { + vi.mocked(API.createManagedUser).mockResolvedValue(true) + vi.mocked(API.getManagedUsers).mockResolvedValue(mockUsers) + + const ok = await store.createUser({ 'user-id': 'noc', password: 'secret' }) + + expect(ok).toBe(true) + expect(API.getManagedUsers).toHaveBeenCalledTimes(1) + }) + + it('createUser should not refresh on failure', async () => { + vi.mocked(API.createManagedUser).mockResolvedValue(false) + + const ok = await store.createUser({ 'user-id': 'noc', password: 'secret' }) + + expect(ok).toBe(false) + expect(API.getManagedUsers).not.toHaveBeenCalled() + }) + + it('updateUser should refresh on success', async () => { + vi.mocked(API.updateManagedUser).mockResolvedValue(true) + vi.mocked(API.getManagedUsers).mockResolvedValue(mockUsers) + + await store.updateUser(mockUsers[1]) + + expect(API.updateManagedUser).toHaveBeenCalledWith(mockUsers[1]) + expect(API.getManagedUsers).toHaveBeenCalledTimes(1) + }) + + it('renameUser should pass old and new ids and refresh', async () => { + vi.mocked(API.renameManagedUser).mockResolvedValue(true) + vi.mocked(API.getManagedUsers).mockResolvedValue(mockUsers) + + await store.renameUser('noc', 'noc2') + + expect(API.renameManagedUser).toHaveBeenCalledWith('noc', 'noc2') + expect(API.getManagedUsers).toHaveBeenCalledTimes(1) + }) + + it('deleteUser should refresh on success', async () => { + vi.mocked(API.deleteManagedUser).mockResolvedValue(true) + vi.mocked(API.getManagedUsers).mockResolvedValue([mockUsers[0]]) + + await store.deleteUser('noc') + + expect(store.users).toEqual([mockUsers[0]]) + }) + + it('setPassword should not trigger a reload', async () => { + vi.mocked(API.setManagedUserPassword).mockResolvedValue(true) + + const ok = await store.setPassword('noc', 'newpw') + + expect(ok).toBe(true) + expect(API.setManagedUserPassword).toHaveBeenCalledWith('noc', 'newpw') + expect(API.getManagedUsers).not.toHaveBeenCalled() + }) +}) From e66968079a834904007077ab53afc7e78e041021 Mon Sep 17 00:00:00 2001 From: Jose Anes Date: Thu, 30 Jul 2026 09:21:00 -0400 Subject: [PATCH 2/8] NMS-20106: validation, atomic updates, and tests for the users API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../opennms/web/rest/v2/UsersRestService.java | 269 +++++++++++---- .../opennms/web/rest/v2/model/UserDto.java | 7 +- .../web/rest/v2/UsersRestServiceIT.java | 310 ++++++++++++++++++ .../applicationContext-spring-security.xml | 1 + ui/src/services/userAdminService.ts | 5 +- ui/src/stores/userAdminStore.ts | 5 +- ui/tests/stores/userAdminStore.test.ts | 10 + 7 files changed, 540 insertions(+), 67 deletions(-) create mode 100644 opennms-webapp-rest/src/test/java/org/opennms/web/rest/v2/UsersRestServiceIT.java diff --git a/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/UsersRestService.java b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/UsersRestService.java index 41ebc2c7ac2f..74df18b37ab2 100644 --- a/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/UsersRestService.java +++ b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/UsersRestService.java @@ -27,6 +27,7 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.regex.Matcher; import java.util.regex.Pattern; import javax.ws.rs.core.Response; @@ -36,6 +37,7 @@ import org.opennms.netmgt.config.UserManager; import org.opennms.netmgt.config.api.UserConfig.ContactType; import org.opennms.netmgt.config.users.Contact; +import org.opennms.netmgt.config.users.Password; import org.opennms.netmgt.config.users.User; import org.opennms.web.api.Authentication; import org.opennms.web.rest.v2.api.UsersRestApi; @@ -53,6 +55,10 @@ * the system of record and hand-editing keeps working. Unlike the legacy JSPs * (which only hid the buttons), the admin/rtc delete and rename protections * are enforced here, server-side. The password hash is never serialized. + * + * Mutations validate the full request up front and then apply it to a + * detached copy of the stored user, so a rejected request can never leave + * partial changes in the manager's shared in-memory state. */ @Component("usersRestServiceV2") public class UsersRestService implements UsersRestApi { @@ -62,8 +68,27 @@ public class UsersRestService implements UsersRestApi { /** System accounts that must not be deleted or renamed. */ private static final Set PROTECTED_USERS = Set.of("admin", "rtc"); - /** Mirrors the legacy servlets' markup check on user ids. */ - private static final Pattern INVALID_USER_ID = Pattern.compile(".*[&<>\"`']+.*"); + /** + * Rejects markup (legacy servlet rule) plus characters that break how the + * id is used downstream: ':' (HTTP basic auth), whitespace (group + * references), and '/', '\', '%', '?', '#' (the id is a URL path segment + * in every per-user endpoint). + */ + private static final Pattern INVALID_USER_ID = Pattern.compile(".*[&<>\"`':/\\\\%?#\\s]+.*"); + + /** + * Day tokens + military begin-end times, e.g. MoWeFr800-1700. Overnight + * schedules (begin after end, e.g. MoTu2000-800) are legal — the legacy + * UI wrote them and hand-edited files contain them. + */ + private static final Pattern DUTY_SCHEDULE = Pattern.compile("^((?:Mo|Tu|We|Th|Fr|Sa|Su){1,7})(\\d{1,4})-(\\d{1,4})$"); + + /** + * Serializes this service's check-then-act sequences. UserManager's own + * lock is internal to each call, so without this two concurrent creates + * could both pass the hasUser() check. + */ + private final Object m_lock = new Object(); @Autowired private UserManager m_userManager; @@ -72,12 +97,14 @@ public class UsersRestService implements UsersRestApi { public Response listUsers(final SecurityContext securityContext) { assertAdmin(securityContext); try { - final List users = new ArrayList<>(); - for (final User user : m_userManager.getUsers().values()) { - users.add(toDto(user)); + synchronized (m_lock) { + final List users = new ArrayList<>(); + for (final User user : m_userManager.getUsers().values()) { + users.add(toDto(user)); + } + users.sort(Comparator.comparing(UserDto::getUserId, String.CASE_INSENSITIVE_ORDER)); + return Response.ok(users).build(); } - users.sort(Comparator.comparing(UserDto::getUserId, String.CASE_INSENSITIVE_ORDER)); - return Response.ok(users).build(); } catch (final Exception e) { return serverError("Can't read users: %s", e); } @@ -87,11 +114,13 @@ public Response listUsers(final SecurityContext securityContext) { public Response getUser(final SecurityContext securityContext, final String userId) { assertAdmin(securityContext); try { - final User user = m_userManager.getUser(userId); - if (user == null) { - return Response.status(Status.NOT_FOUND).entity("User " + userId + " was not found.").build(); + synchronized (m_lock) { + final User user = m_userManager.getUser(userId); + if (user == null) { + return Response.status(Status.NOT_FOUND).entity("User " + userId + " was not found.").build(); + } + return Response.ok(toDto(user)).build(); } - return Response.ok(toDto(user)).build(); } catch (final Exception e) { return serverError("Can't read user: %s", e); } @@ -112,22 +141,35 @@ public Response createUser(final SecurityContext securityContext, final UserWrit return Response.status(Status.BAD_REQUEST).entity("A user-id is required.").build(); } final String userId = request.getUserId().trim(); - if (INVALID_USER_ID.matcher(userId).matches()) { - return Response.status(Status.BAD_REQUEST).entity("The user-id must not contain any HTML markup.").build(); + final String userIdProblem = validateUserId(userId); + if (userIdProblem != null) { + return Response.status(Status.BAD_REQUEST).entity(userIdProblem).build(); } if (isBlank(request.getPassword())) { return Response.status(Status.BAD_REQUEST).entity("A password is required.").build(); } try { - if (m_userManager.hasUser(userId)) { - return Response.status(Status.BAD_REQUEST).entity("User " + userId + " already exists.").build(); + validateDtoFields(request); + } catch (final IllegalArgumentException e) { + return Response.status(Status.BAD_REQUEST).entity(e.getMessage()).build(); + } + try { + synchronized (m_lock) { + if (m_userManager.hasUser(userId)) { + return Response.status(Status.BAD_REQUEST).entity("User " + userId + " already exists.").build(); + } + final User user = new User(); + user.setUserId(userId); + user.setPassword(m_userManager.encryptedPassword(request.getPassword(), true), Boolean.TRUE); + applyDto(user, request); + try { + m_userManager.saveUser(userId, user); + } catch (final Exception e) { + rollbackPhantomUser(userId); + throw e; + } } - final User user = new User(); - user.setUserId(userId); - user.setPassword(m_userManager.encryptedPassword(request.getPassword(), true), Boolean.TRUE); - applyDto(user, request); - m_userManager.saveUser(userId, user); - LOG.info("User {} created by {}", userId, securityContext.getUserPrincipal() == null ? "?" : securityContext.getUserPrincipal().getName()); + LOG.info("User {} created by {}", userId, principal(securityContext)); return Response.status(Status.CREATED).build(); } catch (final Exception e) { return serverError("Can't create user: %s", e); @@ -140,13 +182,25 @@ public Response updateUser(final SecurityContext securityContext, final String u if (dto == null) { return Response.status(Status.BAD_REQUEST).entity("A user body is required.").build(); } + if (dto.getUserId() != null && !userId.equals(dto.getUserId())) { + return Response.status(Status.BAD_REQUEST) + .entity("The user-id in the body does not match the request path; use the rename endpoint to change ids.").build(); + } try { - final User user = m_userManager.getUser(userId); - if (user == null) { - return Response.status(Status.NOT_FOUND).entity("User " + userId + " was not found.").build(); + validateDtoFields(dto); + } catch (final IllegalArgumentException e) { + return Response.status(Status.BAD_REQUEST).entity(e.getMessage()).build(); + } + try { + synchronized (m_lock) { + final User existing = m_userManager.getUser(userId); + if (existing == null) { + return Response.status(Status.NOT_FOUND).entity("User " + userId + " was not found.").build(); + } + final User updated = copyOf(existing); + applyDto(updated, dto); + m_userManager.saveUser(userId, updated); } - applyDto(user, dto); - m_userManager.saveUser(userId, user); return Response.noContent().build(); } catch (final Exception e) { return serverError("Can't update user: %s", e); @@ -160,13 +214,16 @@ public Response setPassword(final SecurityContext securityContext, final String return Response.status(Status.BAD_REQUEST).entity("A password is required.").build(); } try { - final User user = m_userManager.getUser(userId); - if (user == null) { - return Response.status(Status.NOT_FOUND).entity("User " + userId + " was not found.").build(); + synchronized (m_lock) { + final User existing = m_userManager.getUser(userId); + if (existing == null) { + return Response.status(Status.NOT_FOUND).entity("User " + userId + " was not found.").build(); + } + final User updated = copyOf(existing); + updated.setPassword(m_userManager.encryptedPassword(request.getPassword(), true), Boolean.TRUE); + m_userManager.saveUser(userId, updated); } - user.setPassword(m_userManager.encryptedPassword(request.getPassword(), true), Boolean.TRUE); - m_userManager.saveUser(userId, user); - LOG.info("Password changed for user {} by {}", userId, securityContext.getUserPrincipal() == null ? "?" : securityContext.getUserPrincipal().getName()); + LOG.info("Password changed for user {} by {}", userId, principal(securityContext)); return Response.noContent().build(); } catch (final Exception e) { return serverError("Can't change password: %s", e); @@ -183,18 +240,21 @@ public Response renameUser(final SecurityContext securityContext, final String u return Response.status(Status.BAD_REQUEST).entity("The system user " + userId + " cannot be renamed.").build(); } final String newUserId = request.getNewUserId().trim(); - if (INVALID_USER_ID.matcher(newUserId).matches()) { - return Response.status(Status.BAD_REQUEST).entity("The user-id must not contain any HTML markup.").build(); + final String userIdProblem = validateUserId(newUserId); + if (userIdProblem != null) { + return Response.status(Status.BAD_REQUEST).entity(userIdProblem).build(); } try { - if (!m_userManager.hasUser(userId)) { - return Response.status(Status.NOT_FOUND).entity("User " + userId + " was not found.").build(); - } - if (m_userManager.hasUser(newUserId)) { - return Response.status(Status.BAD_REQUEST).entity("User " + newUserId + " already exists.").build(); + synchronized (m_lock) { + if (!m_userManager.hasUser(userId)) { + return Response.status(Status.NOT_FOUND).entity("User " + userId + " was not found.").build(); + } + if (m_userManager.hasUser(newUserId)) { + return Response.status(Status.BAD_REQUEST).entity("User " + newUserId + " already exists.").build(); + } + m_userManager.renameUser(userId, newUserId); } - m_userManager.renameUser(userId, newUserId); - LOG.info("User {} renamed to {} by {}", userId, newUserId, securityContext.getUserPrincipal() == null ? "?" : securityContext.getUserPrincipal().getName()); + LOG.info("User {} renamed to {} by {}", userId, newUserId, principal(securityContext)); return Response.noContent().build(); } catch (final Exception e) { return serverError("Can't rename user: %s", e); @@ -208,11 +268,13 @@ public Response deleteUser(final SecurityContext securityContext, final String u return Response.status(Status.BAD_REQUEST).entity("The system user " + userId + " cannot be deleted.").build(); } try { - if (!m_userManager.hasUser(userId)) { - return Response.status(Status.NOT_FOUND).entity("User " + userId + " was not found.").build(); + synchronized (m_lock) { + if (!m_userManager.hasUser(userId)) { + return Response.status(Status.NOT_FOUND).entity("User " + userId + " was not found.").build(); + } + m_userManager.deleteUser(userId); } - m_userManager.deleteUser(userId); - LOG.info("User {} deleted by {}", userId, securityContext.getUserPrincipal() == null ? "?" : securityContext.getUserPrincipal().getName()); + LOG.info("User {} deleted by {}", userId, principal(securityContext)); return Response.noContent().build(); } catch (final Exception e) { return serverError("Can't delete user: %s", e); @@ -234,13 +296,64 @@ private UserDto toDto(final User user) { return dto; } + /** Detached copy so mutations never touch the manager's live object. */ + private static User copyOf(final User user) { + final User copy = new User(); + copy.setUserId(user.getUserId()); + copy.setFullName(user.getFullName().orElse(null)); + copy.setUserComments(user.getUserComments().orElse(null)); + final Password password = user.getPassword(); + if (password != null) { + copy.setPassword(password.getEncryptedPassword(), password.getSalt()); + } + for (final Contact contact : user.getContacts()) { + final Contact contactCopy = new Contact(contact.getType()); + contactCopy.setInfo(contact.getInfo().orElse(null)); + contactCopy.setServiceProvider(contact.getServiceProvider().orElse(null)); + copy.getContacts().add(contactCopy); + } + copy.setDutySchedules(new ArrayList<>(user.getDutySchedules())); + copy.setRoles(new ArrayList<>(user.getRoles())); + copy.setTuiPin(user.getTuiPin().orElse(null)); + copy.setTimeZoneId(user.getTimeZoneId().orElse(null)); + return copy; + } + /** - * Applies the DTO onto the JAXB user. Only the exposed contact types - * (email, pagerEmail) are touched; every other contact — XMPP, microblog, - * phones, paging services — and the password survive untouched, so a v2 - * update can never corrupt hand-maintained users.xml entries. + * Validates every field of the request BEFORE anything is applied, so a + * rejected request cannot leave partial state anywhere. */ - private void applyDto(final User user, final UserDto dto) { + private static void validateDtoFields(final UserDto dto) { + final String timeZoneId = trimToNull(dto.getTimeZoneId()); + if (timeZoneId != null) { + try { + java.time.ZoneId.of(timeZoneId); + } catch (final RuntimeException e) { + throw new IllegalArgumentException("Invalid time-zone-id: " + timeZoneId); + } + } + if (dto.getRoles() != null) { + for (final String role : dto.getRoles()) { + if (!Authentication.isValidRole(role)) { + throw new IllegalArgumentException("Unknown security role: " + role); + } + } + } + if (dto.getDutySchedules() != null) { + for (final String schedule : dto.getDutySchedules()) { + validateDutySchedule(schedule); + } + } + } + + /** + * Applies the pre-validated DTO. Only the exposed contact types (email, + * pagerEmail) are touched; every other contact — XMPP, microblog, phones, + * paging services — and the password survive untouched, so a v2 update + * can never corrupt hand-maintained users.xml entries. List fields left + * out of the request body arrive as null and are preserved. + */ + private static void applyDto(final User user, final UserDto dto) { user.setFullName(trimToNull(dto.getFullName())); user.setUserComments(trimToNull(dto.getUserComments())); user.setTuiPin(trimToNull(dto.getTuiPin())); @@ -248,11 +361,7 @@ private void applyDto(final User user, final UserDto dto) { if (timeZoneId == null) { user.setTimeZoneId((java.time.ZoneId) null); } else { - try { - user.setTimeZoneId(timeZoneId); - } catch (final RuntimeException e) { - throw new IllegalArgumentException("Invalid time-zone-id: " + timeZoneId); - } + user.setTimeZoneId(timeZoneId); } setContact(user, ContactType.email, dto.getEmail()); setContact(user, ContactType.pagerEmail, dto.getPagerEmail()); @@ -260,15 +369,51 @@ private void applyDto(final User user, final UserDto dto) { user.setDutySchedules(new ArrayList<>(dto.getDutySchedules())); } if (dto.getRoles() != null) { - for (final String role : dto.getRoles()) { - if (!Authentication.isValidRole(role)) { - throw new IllegalArgumentException("Unknown security role: " + role); - } - } user.setRoles(new ArrayList<>(dto.getRoles())); } } + /** + * A failed save leaves the new user in UserManager's in-memory map + * (_writeUser puts before _saveCurrent), which would 400 every retry as + * "already exists". Best effort: remove the phantom again. + */ + private void rollbackPhantomUser(final String userId) { + try { + if (m_userManager.hasUser(userId)) { + m_userManager.deleteUser(userId); + } + } catch (final Exception rollbackFailure) { + LOG.warn("Could not roll back partially created user {}", userId, rollbackFailure); + } + } + + /** Returns a problem description, or null when the user id is acceptable. */ + private static String validateUserId(final String userId) { + if (INVALID_USER_ID.matcher(userId).matches()) { + return "The user-id must not contain markup, whitespace, or the characters : / \\ % ? #"; + } + return null; + } + + /** + * Duty schedules are stored as strings like MoWeFr800-1700 and parsed with + * unchecked exceptions all over notifd/group scheduling — an invalid string + * saved here would break duty evaluation at runtime. Overnight ranges + * (begin after end) are legal. + */ + private static void validateDutySchedule(final String schedule) { + final Matcher matcher = schedule == null ? null : DUTY_SCHEDULE.matcher(schedule); + if (matcher == null || !matcher.matches()) { + throw new IllegalArgumentException("Invalid duty schedule '" + schedule + "': expected day tokens followed by begin-end military times, e.g. MoWeFr800-1700"); + } + final int begin = Integer.parseInt(matcher.group(2)); + final int end = Integer.parseInt(matcher.group(3)); + if (begin > 2359 || end > 2359 || begin % 100 > 59 || end % 100 > 59) { + throw new IllegalArgumentException("Invalid duty schedule '" + schedule + "': times must be military clock values between 0 and 2359"); + } + } + private static String contactInfo(final User user, final ContactType type) { return user.getContacts().stream() .filter(c -> type.name().equals(c.getType())) @@ -299,7 +444,9 @@ private static void assertAdmin(final SecurityContext securityContext) { } } - + private static String principal(final SecurityContext securityContext) { + return securityContext.getUserPrincipal() == null ? "?" : securityContext.getUserPrincipal().getName(); + } private Response serverError(final String format, final Exception e) { if (e instanceof IllegalArgumentException) { diff --git a/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserDto.java b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserDto.java index 1542f6e610b1..688a3653ece6 100644 --- a/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserDto.java +++ b/opennms-webapp-rest/src/main/java/org/opennms/web/rest/v2/model/UserDto.java @@ -21,7 +21,6 @@ */ package org.opennms.web.rest.v2.model; -import java.util.ArrayList; import java.util.List; import javax.xml.bind.annotation.XmlAccessType; @@ -60,11 +59,13 @@ public class UserDto { @XmlElement(name = "time-zone-id") private String timeZoneId; + // null (not empty) defaults: a request body that omits these keys + // deserializes to null, which update semantics treat as "preserve" @XmlElement(name = "duty-schedule") - private List dutySchedules = new ArrayList<>(); + private List dutySchedules; @XmlElement(name = "role") - private List roles = new ArrayList<>(); + private List roles; @XmlElement(name = "read-only") private Boolean readOnly; diff --git a/opennms-webapp-rest/src/test/java/org/opennms/web/rest/v2/UsersRestServiceIT.java b/opennms-webapp-rest/src/test/java/org/opennms/web/rest/v2/UsersRestServiceIT.java new file mode 100644 index 000000000000..9703661221d9 --- /dev/null +++ b/opennms-webapp-rest/src/test/java/org/opennms/web/rest/v2/UsersRestServiceIT.java @@ -0,0 +1,310 @@ +/* + * Licensed to The OpenNMS Group, Inc (TOG) under one or more + * contributor license agreements. See the LICENSE.md file + * distributed with this work for additional information + * regarding copyright ownership. + * + * TOG licenses this file to You under the GNU Affero General + * Public License Version 3 (the "License") or (at your option) + * any later version. You may not use this file except in + * compliance with the License. You may obtain a copy of the + * License at: + * + * https://www.gnu.org/licenses/agpl-3.0.txt + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied. See the License for the specific + * language governing permissions and limitations under the + * License. + */ +package org.opennms.web.rest.v2; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import javax.ws.rs.core.MediaType; + +import org.json.JSONArray; +import org.json.JSONObject; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.opennms.core.test.MockLogAppender; +import org.opennms.core.test.OpenNMSJUnit4ClassRunner; +import org.opennms.core.test.db.annotations.JUnitTemporaryDatabase; +import org.opennms.core.test.rest.AbstractSpringJerseyRestTestCase; +import org.opennms.netmgt.config.UserManager; +import org.opennms.netmgt.config.users.Contact; +import org.opennms.netmgt.config.users.User; +import org.opennms.test.JUnitConfigurationEnvironment; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.web.WebAppConfiguration; + +@RunWith(OpenNMSJUnit4ClassRunner.class) +@WebAppConfiguration +@ContextConfiguration(locations={ + "classpath:/META-INF/opennms/applicationContext-soa.xml", + "classpath:/META-INF/opennms/applicationContext-commonConfigs.xml", + "classpath:/META-INF/opennms/applicationContext-minimal-conf.xml", + "classpath:/META-INF/opennms/applicationContext-dao.xml", + "classpath:/META-INF/opennms/applicationContext-mockConfigManager.xml", + "classpath*:/META-INF/opennms/component-service.xml", + "classpath*:/META-INF/opennms/component-dao.xml", + "classpath:/META-INF/opennms/applicationContext-databasePopulator.xml", + "classpath:/META-INF/opennms/mockEventIpcManager.xml", + "file:src/main/webapp/WEB-INF/applicationContext-svclayer.xml", + "file:src/main/webapp/WEB-INF/applicationContext-cxf-common.xml", + // in-memory user/group managers so users.xml is never touched + "classpath:/META-INF/opennms/applicationContext-mock-usergroup.xml", + "classpath:/applicationContext-rest-test.xml" +}) +@JUnitConfigurationEnvironment(systemProperties = "org.opennms.timeseries.strategy=integration") +@JUnitTemporaryDatabase +public class UsersRestServiceIT extends AbstractSpringJerseyRestTestCase { + + @Autowired + private UserManager m_userManager; + + public UsersRestServiceIT() { + super(CXF_REST_V2_CONTEXT_PATH); + } + + @Override + protected void beforeServletStart() { + MockLogAppender.setupLogging(); + } + + @Test + public void testListNeverContainsPasswordHashes() throws Exception { + final String json = getJson("/users", 200); + final JSONArray users = new JSONArray(json); + assertTrue(users.length() >= 1); + assertEquals("admin", users.getJSONObject(0).getString("user-id")); + // the v1 API leaks hashes to admins; the v2 contract is that no + // response ever carries the password in any form + assertFalse(json.contains("password")); + assertFalse(json.contains("21232F29")); + } + + @Test + public void testGetUser() throws Exception { + final JSONObject admin = new JSONObject(getJson("/users/admin", 200)); + assertEquals("admin", admin.getString("user-id")); + sendRequest(GET, "/users/idontexist", 404); + } + + @Test + public void testAvailableRoles() throws Exception { + final JSONArray roles = new JSONArray(getJson("/users/available-roles", 200)); + boolean foundAdmin = false; + for (int i = 0; i < roles.length(); i++) { + foundAdmin |= "ROLE_ADMIN".equals(roles.getString(i)); + } + assertTrue(foundAdmin); + } + + @Test + public void testCreateLifecycle() throws Exception { + final String body = "{\"user-id\":\"junituser\",\"password\":\"S3cret!pw\",\"full-name\":\"JUnit User\"," + + "\"email\":\"junit@example.com\",\"pager-email\":\"junit-pager@example.com\"," + + "\"duty-schedule\":[\"MoWeFr800-1700\"],\"role\":[\"ROLE_USER\"]}"; + sendData(POST, MediaType.APPLICATION_JSON, "/users", body, 201); + + final JSONObject created = new JSONObject(getJson("/users/junituser", 200)); + assertEquals("JUnit User", created.getString("full-name")); + assertEquals("junit@example.com", created.getString("email")); + assertEquals("junit-pager@example.com", created.getString("pager-email")); + assertEquals("MoWeFr800-1700", created.getJSONArray("duty-schedule").getString(0)); + assertEquals("ROLE_USER", created.getJSONArray("role").getString(0)); + assertTrue(m_userManager.comparePasswords("junituser", "S3cret!pw")); + + // creating the same user again must be rejected + sendData(POST, MediaType.APPLICATION_JSON, "/users", body, 400); + + sendRequest(DELETE, "/users/junituser", 204); + sendRequest(GET, "/users/junituser", 404); + } + + @Test + public void testCreateValidation() throws Exception { + // missing password + sendData(POST, MediaType.APPLICATION_JSON, "/users", "{\"user-id\":\"nopass\"}", 400); + // markup in the user id + sendData(POST, MediaType.APPLICATION_JSON, "/users", "{\"user-id\":\"bad + + diff --git a/ui/src/containers/ManageUsers.vue b/ui/src/containers/ManageUsers.vue index 7f607ba8d1e6..b661a4e3ccac 100644 --- a/ui/src/containers/ManageUsers.vue +++ b/ui/src/containers/ManageUsers.vue @@ -6,6 +6,7 @@

Manage Users

+
@@ -13,6 +14,7 @@ \"}", 400); + sendData(PUT, MediaType.APPLICATION_JSON, "/users/markup", + "{\"user-id\":\"markup\",\"user-comments\":\"plain text\"}", 204); + sendRequest(DELETE, "/users/markup", 204); + } + + @Test + public void testHandEditedMarkupCommentStaysEditable() throws Exception { + // a hand-edited users.xml comment with markup characters must not + // make the user uneditable when it round-trips unchanged + final User user = new User(); + user.setUserId("legacycomment"); + user.setPassword(m_userManager.encryptedPassword("pw", true), Boolean.TRUE); + user.setUserComments("Bob's R&D user"); + m_userManager.saveUser("legacycomment", user); + + sendData(PUT, MediaType.APPLICATION_JSON, "/users/legacycomment", + "{\"user-id\":\"legacycomment\",\"user-comments\":\"Bob's R&D user\",\"full-name\":\"Touched\"}", 204); + final JSONObject after = new JSONObject(getJson("/users/legacycomment", 200)); + assertEquals("Bob's R&D user", after.getString("user-comments")); + assertEquals("Touched", after.getString("full-name")); + sendRequest(DELETE, "/users/legacycomment", 204); + } + + @Test + public void testDeleteBlockedWhileSupervisingOnCallRole() throws Exception { + sendData(POST, MediaType.APPLICATION_JSON, "/users", + "{\"user-id\":\"rolesuper\",\"password\":\"pw\"}", 201); + final org.opennms.netmgt.config.groups.Role role = new org.opennms.netmgt.config.groups.Role(); + role.setName("super-role"); + role.setMembershipGroup("Admin"); + role.setSupervisor("rolesuper"); + m_groupManager.saveRole(role); + + // deleting the supervisor would leave the rota's fallback dangling + sendData(DELETE, MediaType.APPLICATION_JSON, "/users/rolesuper", "", 400); + + m_groupManager.deleteRole("super-role"); + sendRequest(DELETE, "/users/rolesuper", 204); + } + @Test public void testBodyPathUserIdMismatchRejected() throws Exception { sendData(PUT, MediaType.APPLICATION_JSON, "/users/admin", diff --git a/ui/src/components/ManageUsers/UserEditorDialog.vue b/ui/src/components/ManageUsers/UserEditorDialog.vue index 8dd26798eced..c30abfc8aa5a 100644 --- a/ui/src/components/ManageUsers/UserEditorDialog.vue +++ b/ui/src/components/ManageUsers/UserEditorDialog.vue @@ -60,10 +60,16 @@ + {{ commentsProblem }} @@ -143,7 +149,7 @@ import MultiSelect from 'primevue/multiselect' import Password from 'primevue/password' import FormField from '@/components/Common/FormField.vue' -import { validateAdminName, validateEmailShape } from '@/lib/adminValidation' +import { validateAdminComments, validateAdminName, validateEmailShape } from '@/lib/adminValidation' import { useUserAdminStore } from '@/stores/userAdminStore' import { ManagedUser } from '@/types/userAdmin' @@ -173,11 +179,21 @@ const isEditing = computed(() => props.user !== null) const originalUserId = computed(() => props.user?.['user-id'] ?? '') const userIdProblem = computed(() => (isEditing.value ? null : validateAdminName(form.userId, 'user-id'))) -const emailProblem = computed(() => validateEmailShape(form.email, 'email')) -const pagerEmailProblem = computed(() => validateEmailShape(form.pagerEmail, 'pager email')) + +// pre-existing values are never flagged (hand-edited users.xml may hold +// forms these checks don't model); only changed input is validated +const changedOnly = (value: string, original: string | undefined, problem: string | null) => + value.trim() === (original ?? '').trim() ? null : problem + +const emailProblem = computed(() => + changedOnly(form.email, props.user?.email ?? '', validateEmailShape(form.email, 'email'))) +const pagerEmailProblem = computed(() => + changedOnly(form.pagerEmail, props.user?.['pager-email'] ?? '', validateEmailShape(form.pagerEmail, 'pager email'))) +const commentsProblem = computed(() => + changedOnly(form.comments, props.user?.['user-comments'] ?? '', validateAdminComments(form.comments))) const isValid = computed(() => { - if (userIdProblem.value || emailProblem.value || pagerEmailProblem.value) { + if (userIdProblem.value || emailProblem.value || pagerEmailProblem.value || commentsProblem.value) { return false } if (isEditing.value) { diff --git a/ui/src/lib/adminValidation.ts b/ui/src/lib/adminValidation.ts index b7480aaed19e..c99e0cf35c86 100644 --- a/ui/src/lib/adminValidation.ts +++ b/ui/src/lib/adminValidation.ts @@ -26,7 +26,7 @@ const INVALID_NAME = /[&<>"`':/\\%?#\s]/ const INVALID_COMMENTS = /[&<>"`']/ -const EMAIL_SHAPE = /^[^\s@]+@[^\s@]+$/ +const EMAIL_SHAPE = /[^\s@]+@[^\s@]+/ /** * Validates a user-id, group name or on-call role name. @@ -62,10 +62,18 @@ export const validateAdminComments = (value: string): string | null => { */ export const isPathAddressable = (name: string): boolean => !/[/\\%]/.test(name) -/** Loose shape check: notification delivery needs at least local@domain. */ +/** + * Loose shape check: every comma-separated recipient must contain a + * local@domain somewhere, which also accepts RFC-5322 display-name forms + * like `Bill Smith `. + */ export const validateEmailShape = (value: string, label: string): string | null => { const trimmed = value.trim() - if (trimmed && !EMAIL_SHAPE.test(trimmed)) { + if (!trimmed) { + return null + } + const parts = trimmed.split(',').map((part) => part.trim()) + if (parts.some((part) => !part || !EMAIL_SHAPE.test(part))) { return `The ${label} must look like an email address (name@domain).` } return null diff --git a/ui/tests/lib/adminValidation.test.ts b/ui/tests/lib/adminValidation.test.ts index c6e458af65a8..04062da50be5 100644 --- a/ui/tests/lib/adminValidation.test.ts +++ b/ui/tests/lib/adminValidation.test.ts @@ -43,15 +43,17 @@ describe('validateAdminComments', () => { }) describe('validateEmailShape', () => { - it('accepts empty and name@domain shapes', () => { + it('accepts empty values and common deliverable forms', () => { expect(validateEmailShape('', 'email')).toBeNull() expect(validateEmailShape('noc@example.org', 'email')).toBeNull() + expect(validateEmailShape('Bill Smith ', 'email')).toBeNull() + expect(validateEmailShape('a@example.com, b@example.com', 'email')).toBeNull() }) - it('rejects values without an @ or with whitespace', () => { + it('rejects values without a local@domain part', () => { expect(validateEmailShape('not-an-email', 'email')).toContain('email') - expect(validateEmailShape('a b@example.org', 'pager email')).toContain('pager email') expect(validateEmailShape('a@', 'email')).not.toBeNull() + expect(validateEmailShape('a@example.com,,b@example.com', 'pager email')).toContain('pager email') }) }) From a16642698d7a3416bd7394bc0fba19cbf00260b3 Mon Sep 17 00:00:00 2001 From: Jose Anes Date: Thu, 30 Jul 2026 17:57:48 -0400 Subject: [PATCH 6/8] NMS-20106: follow the Manage Users menu entry to the new page in MenuHeaderIT The menu entry now lands on the Vue page, so the smoke test waits for its page title instead of the legacy JSP breadcrumb. --- .../src/test/java/org/opennms/smoketest/MenuHeaderIT.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/smoke-test/src/test/java/org/opennms/smoketest/MenuHeaderIT.java b/smoke-test/src/test/java/org/opennms/smoketest/MenuHeaderIT.java index 5949936dcf44..f221ee537a85 100644 --- a/smoke-test/src/test/java/org/opennms/smoketest/MenuHeaderIT.java +++ b/smoke-test/src/test/java/org/opennms/smoketest/MenuHeaderIT.java @@ -162,7 +162,8 @@ public void testMenuEntries() throws Exception { // User Management Menu clickMenuItem("User Management", "Manage Users"); - wait.until(ExpectedConditions.visibilityOfElementLocated(By.xpath("//ol[@class='breadcrumb']/li[contains(text()[normalize-space()], 'User List')]"))); + // now the Vue page (ui/index.html) + wait.until(ExpectedConditions.visibilityOfElementLocated(By.xpath("//h1[@class='page-title' and text()='Manage Users']"))); clickMenuItem("User Management", "Manage Groups"); wait.until(ExpectedConditions.visibilityOfElementLocated(By.xpath("//ol[@class='breadcrumb']/li[contains(text()[normalize-space()], 'Group List')]"))); From a823b51db7fe53605c854941c95c6407f6fca5e6 Mon Sep 17 00:00:00 2001 From: Jose Anes Date: Tue, 4 Aug 2026 08:45:35 -0400 Subject: [PATCH 7/8] NMS-20106: use @opennms/onms-ui wrappers on the Manage Users page 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. --- .../ManageUsers/UserEditorDialog.vue | 30 +++++------ .../ManageUsers/UserPasswordDialog.vue | 18 +++---- .../ManageUsers/UserRenameDialog.vue | 16 +++--- ui/src/components/ManageUsers/UsersTable.vue | 51 +++++++++---------- 4 files changed, 51 insertions(+), 64 deletions(-) diff --git a/ui/src/components/ManageUsers/UserEditorDialog.vue b/ui/src/components/ManageUsers/UserEditorDialog.vue index c30abfc8aa5a..bd0ab6e4afa5 100644 --- a/ui/src/components/ManageUsers/UserEditorDialog.vue +++ b/ui/src/components/ManageUsers/UserEditorDialog.vue @@ -1,5 +1,5 @@