From 3d9354782bfc79605b708777f6ad05dc21e65e29 Mon Sep 17 00:00:00 2001 From: Vasil Ilchev Date: Tue, 24 Sep 2024 13:07:50 +0300 Subject: [PATCH] =?UTF-8?q?Quota=20must=20be=20checked=20with=20conditions?= =?UTF-8?q?=20over=20incoming=20status,=20not=20curre=E2=80=A6=20(#1847)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- .../jpa/management/JpaActionManagement.java | 21 +-- .../management/JpaControllerManagement.java | 53 ++++--- .../management/ControllerManagementTest.java | 131 +++++++++++------- 3 files changed, 116 insertions(+), 89 deletions(-) diff --git a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaActionManagement.java b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaActionManagement.java index d38aa4f5d..dda243e02 100644 --- a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaActionManagement.java +++ b/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()) - && (Action.Status.ERROR != actionStatus.getStatus()); + final boolean intermediateStatus = isIntermediateStatus(actionStatus); - final boolean isAllowedByRepositoryConfiguration = !repositoryProperties.isRejectActionStatusForClosedAction() - && isIntermediateFeedback; + final boolean isAllowedByRepositoryConfiguration = intermediateStatus && !repositoryProperties.isRejectActionStatusForClosedAction(); - 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) { - final boolean intermediateStatus = FINISHED != action.getStatus() && Action.Status.ERROR != action.getStatus(); - if (intermediateStatus) {// check for quota only for intermediate statuses + protected void assertActionStatusQuota(final JpaActionStatus newActionStatus, final JpaAction action) { + if (isIntermediateStatus(newActionStatus)) {// 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(); + } } diff --git a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaControllerManagement.java b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaControllerManagement.java index af25ea7c8..d5a51d672 100644 --- a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaControllerManagement.java +++ b/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); diff --git a/hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/ControllerManagementTest.java b/hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/ControllerManagementTest.java index d62519f05..8fab8f76b 100644 --- a/hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/ControllerManagementTest.java +++ b/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 static org.assertj.core.api.Assertions.assertThatExceptionOfType; -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.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 io.qameta.allure.Description; +import io.qameta.allure.Feature; +import io.qameta.allure.Step; +import io.qameta.allure.Story; 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 io.qameta.allure.Feature; -import io.qameta.allure.Step; -import io.qameta.allure.Story; +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 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 = TargetAttributesRequestedEvent.class, count = 9), - @Expect(type = ActionUpdatedEvent.class, count = 1), + @Expect(type = ActionCreatedEvent.class, count = 1), @Expect(type = TargetUpdatedEvent.class, count = 3), + @Expect(type = TargetAttributesRequestedEvent.class, count = 10), + @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) - .as("No QuotaExceededException thrown for too many FINISHED updateActionStatus updates") - .isThrownBy(() -> IntStream.range(0, maxMessages).forEach(i -> controllerManagement - .addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.FINISHED)))); + //assert that Final result is accepted even if quota is reached + assertThatNoException().isThrownBy(() -> { + Action updatedAction = controllerManagement.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) - .as("No QuotaExceededException thrown for too many FINISHED updateActionStatus updates") - .isThrownBy(() -> IntStream.range(0, maxMessages).forEach(i -> controllerManagement - .addUpdateActionStatus(entityFactory.actionStatus().create(actionId).status(Status.FINISHED)))); + //assert that Final result is accepted even if quota is reached + assertThatNoException().isThrownBy(() -> { + Action updatedAction = controllerManagement + .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