Copilot commented on code in PR #11382:
URL: https://github.com/apache/cloudstack/pull/11382#discussion_r2425485457
##########
server/src/main/java/com/cloud/api/ApiServlet.java:
##########
@@ -84,9 +83,7 @@ public class ApiServlet extends HttpServlet {
private static final Logger ACCESSLOGGER =
LogManager.getLogger("apiserver." + ApiServlet.class.getName());
private static final String REPLACEMENT = "_";
private static final String LOGGER_REPLACEMENTS = "[\n\r\t]";
- private static final Pattern GET_REQUEST_COMMANDS =
Pattern.compile("^(get|list|query|find)(\\w+)+$");
- private static final HashSet<String> GET_REQUEST_COMMANDS_LIST = new
HashSet<>(Set.of("isaccountallowedtocreateofferingswithtags",
- "readyforshutdown", "cloudianisenabled", "quotabalance",
"quotasummary", "quotatarifflist", "quotaisenabled", "quotastatement",
"verifyoauthcodeandgetuser"));
+ public static final Pattern GET_REQUEST_COMMANDS =
Pattern.compile("^(get|list|query|find)(\\w+)+$");
Review Comment:
The regex pattern `(\\w+)+` is redundant - `\\w+` already matches one or
more word characters, so the outer `+` is unnecessary and could cause
performance issues with backtracking.
```suggestion
public static final Pattern GET_REQUEST_COMMANDS =
Pattern.compile("^(get|list|query|find)\\w+$");
```
##########
server/src/main/java/com/cloud/api/ApiServlet.java:
##########
@@ -485,13 +482,30 @@ private boolean checkIfAuthenticatorIsOf2FA(String
command) {
return verify2FA;
}
- private boolean isStateChangingCommandUsingPOST(String command, String
method, Map<String, Object[]> params) {
- if (command == null ||
(!GET_REQUEST_COMMANDS.matcher(command.toLowerCase()).matches() &&
!GET_REQUEST_COMMANDS_LIST.contains(command.toLowerCase())
- && !command.equalsIgnoreCase("updateConfiguration") &&
!method.equals("POST"))) {
+ protected boolean isStateChangingCommandNotUsingPOST(String command,
String method, Map<String, Object[]> params) {
+ if (BaseCmd.HTTPMethod.POST.toString().equalsIgnoreCase(method)) {
+ return false;
+ }
+ if (command == null || method == null) {
+ return true;
+ }
+ String commandHttpMethod = null;
+ try {
+ Class<?> cmdClass = apiServer.getCmdClass(command);
+ if (cmdClass != null) {
+ APICommand at = cmdClass.getAnnotation(APICommand.class);
+ if (at != null &&
org.apache.commons.lang3.StringUtils.isNotBlank(at.httpMethod())) {
+ commandHttpMethod = at.httpMethod();
+ }
+ }
+ } catch (CloudRuntimeException ignored) {}
Review Comment:
Empty catch block silently ignores exceptions. Consider logging the
exception or adding a comment explaining why it's safe to ignore.
##########
server/src/test/java/com/cloud/api/ApiServletTest.java:
##########
@@ -432,4 +436,88 @@ public void testVerify2FAWhenExpectedCommandIsNotCalled()
throws UnknownHostExce
Assert.assertEquals(false, result);
}
+
+ @Test
+ public void isStateChangingCommandNotUsingPOSTReturnsFalseForPostMethod() {
+ String command = "updateConfiguration";
+ String method = "POST";
+ Map<String, Object[]> params = new HashMap<>();
+
+ boolean result = servlet.isStateChangingCommandNotUsingPOST(command,
method, params);
+
+ Assert.assertFalse(result);
+ }
+
+ @Test
+ public void
isStateChangingCommandNotUsingPOSTReturnsTrueForNullCommandAndMethod() {
+ String command = null;
+ String method = null;
+ Map<String, Object[]> params = new HashMap<>();
+
+ boolean result = servlet.isStateChangingCommandNotUsingPOST(command,
method, params);
+
+ Assert.assertTrue(result);
+ }
+
+ @Test
+ public void
isStateChangingCommandNotUsingPOSTReturnsFalseForGetHttpMethodAnnotation() {
+ String command = "isAccountAllowedToCreateOfferingsWithTags";
+ String method = "GET";
+ Map<String, Object[]> params = new HashMap<>();
+ Class<?> cmdClass = IsAccountAllowedToCreateOfferingsWithTagsCmd.class;
+ APICommand apiCommand = cmdClass.getAnnotation(APICommand.class);
+ Mockito.doReturn(cmdClass).when(apiServer).getCmdClass(command);
+ Assert.assertNotNull(apiCommand);
+ Assert.assertEquals("GET", apiCommand.httpMethod());
+ boolean result = servlet.isStateChangingCommandNotUsingPOST(command,
method, params);
+ Assert.assertFalse(result);
+ }
+
+ @Test
+ public void
isStateChangingCommandNotUsingPOSTReturnsFalseForMatchingGetRequestPattern() {
+ String command = "listZones";
+ String method = "GET";
+ Map<String, Object[]> params = new HashMap<>();
+ boolean result = servlet.isStateChangingCommandNotUsingPOST(command,
method, params);
+ Assert.assertFalse(result);
+ }
+
+ @Test
+ public void
isStateChangingCommandNotUsingPOSTReturnsTrueForMissingNameParameter() {
+ String command = "updateConfiguration";
+ String method = "GET";
+ Map<String, Object[]> params = new HashMap<>();
+ boolean result = servlet.isStateChangingCommandNotUsingPOST(command,
method, params);
+ Assert.assertTrue(result);
+ }
+
+ @Test
+ public void
isStateChangingCommandNotUsingPOSTReturnsFalseForUpdateConfigurationEnforcePostRequestsKey()
{
+ String command = "updateConfiguration";
+ String method = "GET";
+ Map<String, Object[]> params = new HashMap<>();
+ params.put("name", new String[] {
ApiServer.EnforcePostRequestsAndTimestamps.key() });
Review Comment:
Hardcoded array creation could be simplified using a helper method or
constant for better readability and maintainability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
