diff --git a/cwms-data-api/src/main/java/cwms/cda/api/auth/users/roles/AddRoleController.java b/cwms-data-api/src/main/java/cwms/cda/api/auth/users/roles/AddRoleController.java index 91f9c22e0a..345feae908 100644 --- a/cwms-data-api/src/main/java/cwms/cda/api/auth/users/roles/AddRoleController.java +++ b/cwms-data-api/src/main/java/cwms/cda/api/auth/users/roles/AddRoleController.java @@ -1,11 +1,13 @@ package cwms.cda.api.auth.users.roles; import static cwms.cda.api.Controllers.STATUS_204; +import static cwms.cda.api.Controllers.STATUS_400; import static cwms.cda.data.dao.JooqDao.getDslContext; import com.codahale.metrics.MetricRegistry; import cwms.cda.api.Controllers; +import cwms.cda.api.errors.CdaError; import cwms.cda.data.dao.AuthDao; import cwms.cda.data.dao.UserDao; import cwms.cda.formatters.Formats; @@ -34,9 +36,12 @@ public AddRoleController(MetricRegistry metrics) { @OpenApiParam(name = "user-name", required = true, description = "Name of the user to alter") }, - responses = @OpenApiResponse( - status = STATUS_204 - ), + responses = { + @OpenApiResponse(status = STATUS_204), + @OpenApiResponse(status = STATUS_400, + description = "One or more roles do not exist for the requested office.", + content = @OpenApiContent(from = CdaError.class, type = Formats.JSON)) + }, requestBody = @OpenApiRequestBody( content = { @OpenApiContent(from = String[].class, type = Formats.JSON, isArray = true) diff --git a/cwms-data-api/src/main/java/cwms/cda/data/dao/UserDao.java b/cwms-data-api/src/main/java/cwms/cda/data/dao/UserDao.java index 794aca4614..b0ed13ddcd 100644 --- a/cwms-data-api/src/main/java/cwms/cda/data/dao/UserDao.java +++ b/cwms-data-api/src/main/java/cwms/cda/data/dao/UserDao.java @@ -4,15 +4,19 @@ import static org.jooq.impl.DSL.*; import java.sql.CallableStatement; +import java.sql.Connection; import java.sql.PreparedStatement; import java.sql.ResultSet; +import java.sql.SQLException; import java.util.ArrayList; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.TreeSet; +import cwms.cda.api.errors.InvalidItemException; import cwms.cda.data.dto.auth.users.UsersPageCursor; import org.jooq.CommonTableExpression; import org.jooq.Condition; @@ -91,6 +95,7 @@ public Optional getByUniqueName(String uniqueName, String cac_role) { public void addRoles(DataApiPrincipal p, String user, String office, String[] roles) { dsl.connection(c -> { setOffice(c, office); + validateRoles(c, office, roles); try (CallableStatement addUser = c.prepareCall("call cwms_20.cwms_sec.add_user_to_group(?,?,?)")) { for (String role: roles) { addUser.setString(1, user); @@ -104,6 +109,31 @@ public void addRoles(DataApiPrincipal p, String user, String office, String[] ro logger.atInfo().log("Roles '%s' added for user '%s' and office '%s'", String.join(",", roles), user, office); } + private void validateRoles(Connection connection, String office, String[] roles) throws SQLException { + // The package commits each grant, so reject missing roles before executing any of them. + TreeSet availableRoles = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); + try (PreparedStatement statement = connection.prepareStatement( + "select user_group_id from cwms_20.at_sec_user_groups " + + "where db_office_code = cwms_20.cwms_util.get_db_office_code(?)")) { + statement.setString(1, office); + try (ResultSet result = statement.executeQuery()) { + while (result.next()) { + availableRoles.add(result.getString(1)); + } + } + } + List missingRoles = new ArrayList<>(); + for (String role : roles) { + if (role == null || !availableRoles.contains(role)) { + missingRoles.add(String.valueOf(role)); + } + } + if (!missingRoles.isEmpty()) { + throw new InvalidItemException("Roles do not exist for office " + office + ": " + + String.join(", ", missingRoles), null); + } + } + public void deleteRoles(DataApiPrincipal p, String user, String office, String[] roles) { dsl.connection(c -> { setOffice(c, office); diff --git a/cwms-data-api/src/test/java/cwms/cda/api/users/UserManagementTestIT.java b/cwms-data-api/src/test/java/cwms/cda/api/users/UserManagementTestIT.java index 10778109f2..e322c1de97 100644 --- a/cwms-data-api/src/test/java/cwms/cda/api/users/UserManagementTestIT.java +++ b/cwms-data-api/src/test/java/cwms/cda/api/users/UserManagementTestIT.java @@ -7,6 +7,8 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.sql.CallableStatement; +import java.sql.SQLException; import java.util.ArrayList; import cwms.cda.api.Controllers; @@ -23,6 +25,7 @@ import cwms.cda.api.DataApiTestIT; import cwms.cda.data.dto.auth.users.User; import cwms.cda.data.dto.auth.users.Users; +import fixtures.CwmsDataApiSetupCallback; import fixtures.KeyCloakExtension; import fixtures.TestAccounts; import fixtures.users.annotation.AuthType; @@ -96,11 +99,11 @@ void test_manage_user(String authType, TestAccounts.KeyUser theUser, RequestSpec .body("user-name", equalTo(userUnderTest.getName().toUpperCase())) .body("roles.SPK",hasItems("All Users", "CWMS Users", "TS ID Creator")) ; - // we can add a role + // Role lookup remains case insensitive. given() .log().ifValidationFails(LogDetail.ALL, true) .spec(authSpec) - .body("[\"CCP Mgr\"]") + .body("[\"ccp mgr\"]") .when() .post("/user/{user-name}/roles/{office-id}", userUnderTest.getName(), theUser.getOperatingOffice()) .then() @@ -147,6 +150,81 @@ void test_manage_user(String authType, TestAccounts.KeyUser theUser, RequestSpec ; } + @ParameterizedTest + @ArgumentsSource(UserSpecSource.class) + @AuthType(user = TestAccounts.KeyUser.SPK_NORMAL2) + void test_missing_roles_rejected_before_adding_valid_roles(String authType, TestAccounts.KeyUser theUser, + RequestSpecification authSpec) { + String user = TestAccounts.KeyUser.SPK_NORMAL.getName(); + try { + given().spec(authSpec).body("[\"CCP Mgr\"]") + .delete("/user/{user-name}/roles/SPK", user) + .then().statusCode(204); + + given().spec(authSpec) + .body("[\"CCP Mgr\",\"MISSING ROLE ONE\",\"MISSING ROLE TWO\"]") + .post("/user/{user-name}/roles/SPK", user) + .then().log().ifValidationFails() + .statusCode(400) + .body("details.message", + equalTo("Roles do not exist for office SPK: MISSING ROLE ONE, MISSING ROLE TWO")); + + given().spec(authSpec).get("/users/{user-name}", user) + .then().statusCode(200) + .body("roles.SPK", not(hasItem("CCP Mgr"))) + .body("roles.SPK", hasItems("All Users", "CWMS Users", "TS ID Creator")); + } finally { + given().spec(authSpec).body("[\"CCP Mgr\"]") + .delete("/user/{user-name}/roles/SPK", user) + .then().statusCode(204); + } + } + + @Test + void test_missing_role_with_api_key() { + given().header("Authorization", TestAccounts.KeyUser.SPK_NORMAL2.toHeaderValue()) + .body("[\"MISSING ROLE\"]") + .post("/user/{user-name}/roles/SPK", TestAccounts.KeyUser.SPK_NORMAL.getName()) + .then().statusCode(400) + .body("details.message", equalTo("Roles do not exist for office SPK: MISSING ROLE")); + } + + @ParameterizedTest + @ArgumentsSource(UserSpecSource.class) + @AuthType(user = TestAccounts.KeyUser.SPK_NORMAL2) + void test_role_from_another_office_is_rejected(String authType, TestAccounts.KeyUser theUser, + RequestSpecification authSpec) throws Exception { + String role = "CDA TEST SWT ONLY"; + CwmsDataApiSetupCallback.getDatabaseLink().connection(connection -> { + try (CallableStatement statement = connection.prepareCall( + "call cwms_20.cwms_sec.create_user_group(?, ?, ?)")) { + statement.setString(1, role); + statement.setString(2, "Office-specific role validation test"); + statement.setString(3, SWT); + statement.execute(); + } catch (SQLException ex) { + throw new RuntimeException(ex); + } + }, "cwms_20"); + try { + given().spec(authSpec).body("[\"" + role + "\"]") + .post("/user/{user-name}/roles/SPK", TestAccounts.KeyUser.SPK_NORMAL.getName()) + .then().log().ifValidationFails().statusCode(400) + .body("details.message", equalTo("Roles do not exist for office SPK: " + role)); + } finally { + CwmsDataApiSetupCallback.getDatabaseLink().connection(connection -> { + try (CallableStatement statement = connection.prepareCall( + "call cwms_20.cwms_sec.delete_user_group(?, ?)")) { + statement.setString(1, role); + statement.setString(2, SWT); + statement.execute(); + } catch (SQLException ex) { + throw new RuntimeException(ex); + } + }, "cwms_20"); + } + } + @ParameterizedTest @ArgumentsSource(UserSpecSource.class) @AuthType(user = TestAccounts.KeyUser.SPK_NORMAL2)