Return 400 for nonexistent user roles without partial grants

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)

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<User> 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<String> 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<String> 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);

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)