Quota must be checked with conditions over incoming status, not curre… (#1847)

* Quota must be checked with conditions over incoming status, not current persisted in db

* Fix Download_Only case where DOWNLOADED is threated as 'final'.
Fix ci build tests.

* Review findings

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaActionManagement.java

@@ -33,6 +33,7 @@ import java.util.List;
 import java.util.stream.Collectors;
 
 import static org.eclipse.hawkbit.repository.model.Action.ActionType.DOWNLOAD_ONLY;
+import static org.eclipse.hawkbit.repository.model.Action.Status.ERROR;
 import static org.eclipse.hawkbit.repository.model.Action.Status.FINISHED;
 
 /**
@@ -111,13 +112,12 @@ public class JpaActionManagement {
      */
     private boolean isUpdatingActionStatusAllowed(final JpaAction action, final JpaActionStatus actionStatus) {
 
-        final boolean isIntermediateFeedback = (FINISHED != actionStatus.getStatus())
+        final boolean intermediateStatus = isIntermediateStatus(actionStatus);
-                && (Action.Status.ERROR != actionStatus.getStatus());
 
-        final boolean isAllowedByRepositoryConfiguration = !repositoryProperties.isRejectActionStatusForClosedAction()
+        final boolean isAllowedByRepositoryConfiguration = intermediateStatus && !repositoryProperties.isRejectActionStatusForClosedAction();
-                && isIntermediateFeedback;
 
-        final boolean isAllowedForDownloadOnlyActions = isDownloadOnly(action) && !isIntermediateFeedback;
+        //in case of download_only action Status#DOWNLOADED is treated as 'final' already, so we accept one final status from device in case it sends
+        final boolean isAllowedForDownloadOnlyActions = isDownloadOnly(action) && action.getStatus() == Action.Status.DOWNLOADED && !intermediateStatus;
 
         return action.isActive() || isAllowedByRepositoryConfiguration || isAllowedForDownloadOnlyActions;
     }
@@ -141,7 +141,7 @@ public class JpaActionManagement {
      */
     private Action handleAddUpdateActionStatus(final JpaActionStatus actionStatus, final JpaAction action) {
         // information status entry - check for a potential DOS attack
-        assertActionStatusQuota(action);
+        assertActionStatusQuota(actionStatus, action);
         assertActionStatusMessageQuota(actionStatus);
         actionStatus.setAction(action);
 
@@ -157,9 +157,8 @@ public class JpaActionManagement {
      // can be overwritten to intercept the persistence of the action status
     }
     
-    protected void assertActionStatusQuota(final JpaAction action) {
+    protected void assertActionStatusQuota(final JpaActionStatus newActionStatus, final JpaAction action) {
-        final boolean intermediateStatus = FINISHED != action.getStatus() && Action.Status.ERROR != action.getStatus();
+        if (isIntermediateStatus(newActionStatus)) {// check for quota only for intermediate statuses
-        if (intermediateStatus) {// check for quota only for intermediate statuses
             QuotaHelper.assertAssignmentQuota(action.getId(), 1, quotaManagement.getMaxStatusEntriesPerAction(),
                     ActionStatus.class, Action.class, actionStatusRepository::countByActionId);
         }
@@ -169,4 +168,8 @@ public class JpaActionManagement {
         QuotaHelper.assertAssignmentQuota(actionStatus.getId(), actionStatus.getMessages().size(),
                 quotaManagement.getMaxMessagesPerActionStatus(), "Message", ActionStatus.class.getSimpleName(), null);
     }
+
+    private static boolean isIntermediateStatus(final JpaActionStatus actionStatus) {
+        return FINISHED != actionStatus.getStatus() && ERROR != actionStatus.getStatus();
+    }
 }

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaControllerManagement.java

@@ -9,37 +9,12 @@
  */
 package org.eclipse.hawkbit.repository.jpa.management;
 
-import static org.eclipse.hawkbit.repository.model.Action.Status.DOWNLOADED;
-import static org.eclipse.hawkbit.repository.model.Action.Status.FINISHED;
-import static org.eclipse.hawkbit.repository.model.Target.CONTROLLER_ATTRIBUTE_KEY_SIZE;
-import static org.eclipse.hawkbit.repository.model.Target.CONTROLLER_ATTRIBUTE_VALUE_SIZE;
-
-import java.net.URI;
-import java.time.Duration;
-import java.time.ZonedDateTime;
-import java.time.temporal.ChronoUnit;
-import java.time.temporal.TemporalUnit;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-import java.util.Set;
-import java.util.concurrent.BlockingDeque;
-import java.util.concurrent.LinkedBlockingDeque;
-import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.TimeUnit;
-import java.util.stream.Collectors;
-
 import jakarta.persistence.EntityManager;
 import jakarta.persistence.Query;
 import jakarta.persistence.criteria.CriteriaBuilder;
 import jakarta.persistence.criteria.CriteriaQuery;
 import jakarta.persistence.criteria.Root;
 import jakarta.validation.constraints.NotEmpty;
-
 import lombok.extern.slf4j.Slf4j;
 import org.apache.commons.collections4.ListUtils;
 import org.eclipse.hawkbit.repository.ConfirmationManagement;
@@ -115,6 +90,30 @@ import org.springframework.transaction.support.TransactionCallback;
 import org.springframework.util.StringUtils;
 import org.springframework.validation.annotation.Validated;
 
+import java.net.URI;
+import java.time.Duration;
+import java.time.ZonedDateTime;
+import java.time.temporal.ChronoUnit;
+import java.time.temporal.TemporalUnit;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.BlockingDeque;
+import java.util.concurrent.LinkedBlockingDeque;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+import static org.eclipse.hawkbit.repository.model.Action.Status.DOWNLOADED;
+import static org.eclipse.hawkbit.repository.model.Action.Status.FINISHED;
+import static org.eclipse.hawkbit.repository.model.Target.CONTROLLER_ATTRIBUTE_KEY_SIZE;
+import static org.eclipse.hawkbit.repository.model.Target.CONTROLLER_ATTRIBUTE_VALUE_SIZE;
+
 /**
  * JPA based {@link ControllerManagement} implementation.
  */
@@ -613,7 +612,7 @@ public class JpaControllerManagement extends JpaActionManagement implements Cont
             break;
         default:
             // information status entry - check for a potential DOS attack
-            assertActionStatusQuota(action);
+            assertActionStatusQuota(actionStatus, action);
             assertActionStatusMessageQuota(actionStatus);
             break;
         }
@@ -891,7 +890,7 @@ public class JpaControllerManagement extends JpaActionManagement implements Cont
         final JpaActionStatus statusMessage = create.build();
         statusMessage.setAction(action);
 
-        assertActionStatusQuota(action);
+        assertActionStatusQuota(statusMessage, action);
         assertActionStatusMessageQuota(statusMessage);
 
         return actionStatusRepository.save(statusMessage);

hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/ControllerManagementTest.java

@@ -9,33 +9,11 @@
  */
 package org.eclipse.hawkbit.repository.jpa.management;
 
-import static org.assertj.core.api.Assertions.assertThat;
+import io.qameta.allure.Description;
-import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+import io.qameta.allure.Feature;
-import static org.eclipse.hawkbit.im.authentication.SpPermission.SpringEvalExpressions.CONTROLLER_ROLE;
+import io.qameta.allure.Step;
-import static org.eclipse.hawkbit.im.authentication.SpPermission.SpringEvalExpressions.CONTROLLER_ROLE_ANONYMOUS;
+import io.qameta.allure.Story;
-import static org.eclipse.hawkbit.im.authentication.SpPermission.SpringEvalExpressions.IS_CONTROLLER;
-import static org.eclipse.hawkbit.repository.jpa.configuration.Constants.TX_RT_MAX;
-import static org.eclipse.hawkbit.repository.model.Action.ActionType.DOWNLOAD_ONLY;
-import static org.eclipse.hawkbit.repository.test.util.TestdataFactory.DEFAULT_CONTROLLER_ID;
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
-
-import java.io.ByteArrayInputStream;
-import java.net.URISyntaxException;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-import java.util.stream.Collectors;
-import java.util.stream.IntStream;
-
 import jakarta.validation.ConstraintViolationException;
-
 import org.apache.commons.lang3.RandomStringUtils;
 import org.apache.commons.lang3.RandomUtils;
 import org.assertj.core.api.Assertions;
@@ -55,8 +33,8 @@ import org.eclipse.hawkbit.repository.event.remote.entity.DistributionSetUpdated
 import org.eclipse.hawkbit.repository.event.remote.entity.SoftwareModuleCreatedEvent;
 import org.eclipse.hawkbit.repository.event.remote.entity.SoftwareModuleUpdatedEvent;
 import org.eclipse.hawkbit.repository.event.remote.entity.TargetCreatedEvent;
-import org.eclipse.hawkbit.repository.event.remote.entity.TargetUpdatedEvent;
 import org.eclipse.hawkbit.repository.event.remote.entity.TargetTypeCreatedEvent;
+import org.eclipse.hawkbit.repository.event.remote.entity.TargetUpdatedEvent;
 import org.eclipse.hawkbit.repository.exception.AssignmentQuotaExceededException;
 import org.eclipse.hawkbit.repository.exception.CancelActionNotAllowedException;
 import org.eclipse.hawkbit.repository.exception.EntityAlreadyExistsException;
@@ -80,19 +58,38 @@ import org.eclipse.hawkbit.repository.model.Target;
 import org.eclipse.hawkbit.repository.model.TargetUpdateStatus;
 import org.eclipse.hawkbit.repository.test.matcher.Expect;
 import org.eclipse.hawkbit.repository.test.matcher.ExpectEvents;
-import org.eclipse.hawkbit.repository.test.util.TargetTestData;
 import org.eclipse.hawkbit.repository.test.util.SecurityContextSwitch;
+import org.eclipse.hawkbit.repository.test.util.TargetTestData;
 import org.eclipse.hawkbit.repository.test.util.WithUser;
-
 import org.junit.jupiter.api.Test;
 import org.mockito.Mockito;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.dao.ConcurrencyFailureException;
 
-import io.qameta.allure.Description;
+import java.io.ByteArrayInputStream;
-import io.qameta.allure.Feature;
+import java.net.URISyntaxException;
-import io.qameta.allure.Step;
+import java.util.ArrayList;
-import io.qameta.allure.Story;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+import static org.assertj.core.api.Assertions.assertThatNoException;
+import static org.eclipse.hawkbit.im.authentication.SpPermission.SpringEvalExpressions.CONTROLLER_ROLE;
+import static org.eclipse.hawkbit.im.authentication.SpPermission.SpringEvalExpressions.CONTROLLER_ROLE_ANONYMOUS;
+import static org.eclipse.hawkbit.repository.jpa.configuration.Constants.TX_RT_MAX;
+import static org.eclipse.hawkbit.repository.model.Action.ActionType.DOWNLOAD_ONLY;
+import static org.eclipse.hawkbit.repository.test.util.TestdataFactory.DEFAULT_CONTROLLER_ID;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 @Feature("Component Tests - Repository")
 @Story("Controller Management")
@@ -1455,9 +1452,9 @@ class ControllerManagementTest extends AbstractJpaIntegrationTest {
             @Expect(type = SoftwareModuleCreatedEvent.class, count = 3),
             @Expect(type = DistributionSetUpdatedEvent.class, count = 1), // implicit lock
             @Expect(type = SoftwareModuleUpdatedEvent.class, count = 3), // implicit lock
-            @Expect(type = ActionCreatedEvent.class, count = 1), @Expect(type = TargetUpdatedEvent.class, count = 2),
+            @Expect(type = ActionCreatedEvent.class, count = 1), @Expect(type = TargetUpdatedEvent.class, count = 3),
-            @Expect(type = TargetAttributesRequestedEvent.class, count = 9),
+            @Expect(type = TargetAttributesRequestedEvent.class, count = 10),
-            @Expect(type = ActionUpdatedEvent.class, count = 1),
+            @Expect(type = ActionUpdatedEvent.class, count = 2),
             @Expect(type = TargetAssignDistributionSetEvent.class, count = 1) })
     void quotaExceededExceptionWhenControllerReportsTooManyUpdateActionStatusMessagesForDownloadOnlyAction() {
         final int maxMessages = quotaManagement.getMaxMessagesPerActionStatus();
@@ -1465,20 +1462,32 @@ class ControllerManagementTest extends AbstractJpaIntegrationTest {
         final Long actionId = createAndAssignDsAsDownloadOnly("downloadOnlyDs", DEFAULT_CONTROLLER_ID);
         assertThat(actionId).isNotNull();
 
+        //assert that too many intermediate statuses will throw quota exception
         assertThatExceptionOfType(AssignmentQuotaExceededException.class)
                 .as("No QuotaExceededException thrown for too many DOWNLOADED updateActionStatus updates").isThrownBy(
                         () -> IntStream.range(0, maxMessages).forEach(i -> controllerManagement.addUpdateActionStatus(
                                 entityFactory.actionStatus().create(actionId).status(Status.DOWNLOADED))));
 
-        assertThatExceptionOfType(AssignmentQuotaExceededException.class)
-                .as("No QuotaExceededException thrown for too many ERROR updateActionStatus updates")
-                .isThrownBy(() -> IntStream.range(0, maxMessages).forEach(i -> controllerManagement
-                        .addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.ERROR))));
 
-        assertThatExceptionOfType(AssignmentQuotaExceededException.class)
+        //assert that Final result is accepted even if quota is reached
-                .as("No QuotaExceededException thrown for too many FINISHED updateActionStatus updates")
+        assertThatNoException().isThrownBy(() -> {
-                .isThrownBy(() -> IntStream.range(0, maxMessages).forEach(i -> controllerManagement
+            Action updatedAction = controllerManagement.addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.FINISHED));
-                        .addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.FINISHED))));
+            // check if action really finished
+            assertThat(updatedAction.isActive()).isFalse();
+            // check if final status is updated accordingly
+            assertThat(updatedAction.getStatus()).isEqualTo(Status.FINISHED);
+        });
+
+
+        //assert that additional final result is not accepted
+        assertThatNoException().isThrownBy(() -> {
+            Action updatedAction = controllerManagement
+                    .addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.ERROR));
+            // check if action really finished
+            assertThat(updatedAction.isActive()).isFalse();
+            // check if final status is not changed - e.g. ERROR is not updated because action has already finished
+            assertThat(updatedAction.getStatus()).isEqualTo(Status.FINISHED);
+        });
     }
 
     @Test
@@ -1488,27 +1497,43 @@ class ControllerManagementTest extends AbstractJpaIntegrationTest {
             @Expect(type = SoftwareModuleCreatedEvent.class, count = 3),
             @Expect(type = DistributionSetUpdatedEvent.class, count = 1), // implicit lock
             @Expect(type = SoftwareModuleUpdatedEvent.class, count = 3), // implicit lock
-            @Expect(type = ActionCreatedEvent.class, count = 1), @Expect(type = TargetUpdatedEvent.class, count = 1),
+            @Expect(type = ActionCreatedEvent.class, count = 1),
+            @Expect(type = TargetAttributesRequestedEvent.class, count = 1),
+            @Expect(type = ActionUpdatedEvent.class, count = 1),
+            @Expect(type = TargetUpdatedEvent.class, count = 2),
             @Expect(type = TargetAssignDistributionSetEvent.class, count = 1) })
     void quotaExceededExceptionWhenControllerReportsTooManyUpdateActionStatusMessagesForForced() {
         final int maxMessages = quotaManagement.getMaxMessagesPerActionStatus();
         final Long actionId = createTargetAndAssignDs();
         assertThat(actionId).isNotNull();
 
+        //assert that too many intermediate statuses will throw quota exception
         assertThatExceptionOfType(AssignmentQuotaExceededException.class)
                 .as("No QuotaExceededException thrown for too many DOWNLOADED updateActionStatus updates").isThrownBy(
                         () -> IntStream.range(0, maxMessages).forEach(i -> controllerManagement.addUpdateActionStatus(
                                 entityFactory.actionStatus().create(actionId).status(Status.DOWNLOADED))));
 
-        assertThatExceptionOfType(AssignmentQuotaExceededException.class)
-                .as("No QuotaExceededException thrown for too many ERROR updateActionStatus updates")
-                .isThrownBy(() -> IntStream.range(0, maxMessages).forEach(i -> controllerManagement
-                        .addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.ERROR))));
 
-        assertThatExceptionOfType(AssignmentQuotaExceededException.class)
+        //assert that Final result is accepted even if quota is reached
-                .as("No QuotaExceededException thrown for too many FINISHED updateActionStatus updates")
+        assertThatNoException().isThrownBy(() -> {
-                .isThrownBy(() -> IntStream.range(0, maxMessages).forEach(i -> controllerManagement
+            Action updatedAction = controllerManagement
-                        .addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.FINISHED))));
+                    .addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.FINISHED));
+            // check if action really finished
+            assertThat(updatedAction.isActive()).isFalse();
+            // check if final status is updated accordingly
+            assertThat(updatedAction.getStatus()).isEqualTo(Status.FINISHED);
+        });
+
+
+        //assert that additional final result is not accepted
+        assertThatNoException().isThrownBy(() -> {
+            Action updatedAction = controllerManagement
+                    .addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.ERROR));
+            // check if action really finished
+            assertThat(updatedAction.isActive()).isFalse();
+            // check if final status is not changed - e.g. ERROR is not updated because action has already finished
+            assertThat(updatedAction.getStatus()).isEqualTo(Status.FINISHED);
+        });
     }
 
     @Test