Fix action delete access control - to require only target update (not delete also) (#2767)
Signed-off-by: Avgustin Marinov <Avgustin.Marinov@bosch.com>
This commit is contained in:
Avgustin Marinov
@@ -10,6 +10,8 @@
|
||||
package org.eclipse.hawkbit.repository.jpa.acm;
|
||||
|
||||
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.assertj.core.api.Assertions.assertThatThrownBy;
|
||||
import static org.eclipse.hawkbit.im.authentication.SpPermission.READ_DISTRIBUTION_SET;
|
||||
import static org.eclipse.hawkbit.im.authentication.SpPermission.READ_TARGET;
|
||||
@@ -56,17 +58,17 @@ class DeploymentManagementTest extends AbstractAccessControllerManagementTest {
|
||||
void verifyActionVisibility() {
|
||||
final String controllerId = target1Type1.getControllerId();
|
||||
verify(
|
||||
assignedId -> {
|
||||
actionId -> {
|
||||
assertThat(deploymentManagement.findActionsAll(UNPAGED)).isEmpty();
|
||||
assertThat(deploymentManagement.findAction(assignedId)).isEmpty();
|
||||
assertThatThrownBy(() -> deploymentManagement.findActionWithDetails(assignedId))
|
||||
assertThat(deploymentManagement.findAction(actionId)).isEmpty();
|
||||
assertThatThrownBy(() -> deploymentManagement.findActionWithDetails(actionId))
|
||||
.isInstanceOf(InsufficientPermissionException.class);
|
||||
assertThat(deploymentManagement.findActions("id==*", UNPAGED)).isEmpty();
|
||||
assertThatThrownBy(() -> deploymentManagement.findActionsByTarget(controllerId, UNPAGED))
|
||||
.isInstanceOf(EntityNotFoundException.class);
|
||||
assertThatThrownBy(() -> deploymentManagement.findActionsByTarget("id==*", controllerId, UNPAGED))
|
||||
.isInstanceOf(EntityNotFoundException.class);
|
||||
assertThatThrownBy(() -> deploymentManagement.findActionStatusByAction(assignedId, UNPAGED))
|
||||
assertThatThrownBy(() -> deploymentManagement.findActionStatusByAction(actionId, UNPAGED))
|
||||
.isInstanceOf(EntityNotFoundException.class);
|
||||
assertThatThrownBy(() -> deploymentManagement.findActiveActionsByTarget(controllerId, UNPAGED))
|
||||
.isInstanceOf(EntityNotFoundException.class);
|
||||
@@ -76,16 +78,16 @@ class DeploymentManagementTest extends AbstractAccessControllerManagementTest {
|
||||
assertThatThrownBy(() -> deploymentManagement.hasPendingCancellations(targetId)).isInstanceOf(
|
||||
EntityNotFoundException.class);
|
||||
},
|
||||
assignedId -> {
|
||||
actionId -> {
|
||||
assertThat(deploymentManagement.findActionsAll(UNPAGED)).hasSize(1).allMatch(this::isActionOfTarget1Type1);
|
||||
assertThat(deploymentManagement.findAction(assignedId)).hasValueSatisfying(this::assertActionOfTarget1Type1);
|
||||
assertThat(deploymentManagement.findActionWithDetails(assignedId)).hasValueSatisfying(this::assertActionOfTarget1Type1);
|
||||
assertThat(deploymentManagement.findAction(actionId)).hasValueSatisfying(this::assertActionOfTarget1Type1);
|
||||
assertThat(deploymentManagement.findActionWithDetails(actionId)).hasValueSatisfying(this::assertActionOfTarget1Type1);
|
||||
assertThat(deploymentManagement.findActions("id==*", UNPAGED)).hasSize(1).allMatch(this::isActionOfTarget1Type1);
|
||||
assertThat(deploymentManagement.findActionsByTarget(controllerId, UNPAGED)).hasSize(1)
|
||||
.allMatch(this::isActionOfTarget1Type1);
|
||||
assertThat(deploymentManagement.findActionsByTarget("id==*", controllerId, UNPAGED))
|
||||
.hasSize(1).allMatch(this::isActionOfTarget1Type1);
|
||||
assertThat(deploymentManagement.findActionStatusByAction(assignedId, UNPAGED))
|
||||
assertThat(deploymentManagement.findActionStatusByAction(actionId, UNPAGED))
|
||||
.hasSize(1).allMatch(actionStatus -> actionStatus.getStatus().equals(Action.Status.RUNNING));
|
||||
assertThat(deploymentManagement.findActiveActionsByTarget(controllerId, UNPAGED))
|
||||
.hasSize(1).allMatch(this::isActionOfTarget1Type1);
|
||||
@@ -96,24 +98,83 @@ class DeploymentManagementTest extends AbstractAccessControllerManagementTest {
|
||||
null);
|
||||
}
|
||||
|
||||
@Test
|
||||
void verifyDeleteActionById() {
|
||||
verify(
|
||||
null,
|
||||
actionId -> assertThatExceptionOfType(InsufficientPermissionException.class)
|
||||
.isThrownBy(() -> deploymentManagement.deleteAction(actionId)),
|
||||
actionId -> assertThatNoException().isThrownBy(() -> deploymentManagement.deleteAction(actionId)));
|
||||
}
|
||||
|
||||
@Test
|
||||
void verifyDeleteActionsById() {
|
||||
verify(
|
||||
null,
|
||||
actionId -> {
|
||||
final List<Long> actionIds = List.of(actionId);
|
||||
assertThatExceptionOfType(InsufficientPermissionException.class)
|
||||
.isThrownBy(() -> deploymentManagement.deleteActionsByIds(actionIds));
|
||||
},
|
||||
actionId -> assertThatNoException().isThrownBy(() -> deploymentManagement.deleteActionsByIds(List.of(actionId))));
|
||||
}
|
||||
|
||||
@Test
|
||||
void verifyDeleteActionByRSQL() {
|
||||
verify(
|
||||
null,
|
||||
actionId -> {
|
||||
assertThatNoException().isThrownBy(() -> deploymentManagement.deleteActionsByRsql("id==" + actionId));
|
||||
// no exception but the action shall not be deleted
|
||||
assertThat(deploymentManagement.findAction(actionId)).isPresent();
|
||||
},
|
||||
actionId -> {
|
||||
assertThatNoException().isThrownBy(() -> deploymentManagement.deleteActionsByRsql("id==" + actionId));
|
||||
// the action shall be deleted
|
||||
assertThat(deploymentManagement.findAction(actionId)).isEmpty();
|
||||
});
|
||||
}
|
||||
|
||||
@Test
|
||||
void verifyDeleteTargetActionsById() {
|
||||
verify(
|
||||
null,
|
||||
actionId -> {
|
||||
final String controllerId = deploymentManagement.findAction(actionId)
|
||||
.map(Action::getTarget).map(Target::getControllerId).orElseThrow();
|
||||
final List<Long> actionIds = List.of(actionId, -1L);
|
||||
assertThatNoException().isThrownBy(() -> deploymentManagement.deleteTargetActionsByIds(controllerId, actionIds));
|
||||
// no exception but the action shall not be deleted
|
||||
assertThat(deploymentManagement.findAction(actionId)).isPresent();
|
||||
},
|
||||
actionId -> {
|
||||
final String controllerId = deploymentManagement.findAction(actionId)
|
||||
.map(Action::getTarget).map(Target::getControllerId).orElseThrow();
|
||||
assertThatNoException().isThrownBy(
|
||||
() -> deploymentManagement.deleteTargetActionsByIds(controllerId, List.of(actionId, -1L)));
|
||||
// the action shall be deleted
|
||||
assertThat(deploymentManagement.findAction(actionId)).isEmpty();
|
||||
});
|
||||
}
|
||||
|
||||
@Test
|
||||
void verifyCancellation() {
|
||||
verify(
|
||||
assignedId -> assertThatThrownBy(() -> deploymentManagement.cancelAction(assignedId))
|
||||
actionId -> assertThatThrownBy(() -> deploymentManagement.cancelAction(actionId))
|
||||
.isInstanceOf(EntityNotFoundException.class),
|
||||
assignedId -> assertThatThrownBy(() -> deploymentManagement.cancelAction(assignedId))
|
||||
actionId -> assertThatThrownBy(() -> deploymentManagement.cancelAction(actionId))
|
||||
.isInstanceOf(InsufficientPermissionException.class),
|
||||
assignedId -> assertThat(deploymentManagement.cancelAction(assignedId).getId()).isEqualTo(assignedId));
|
||||
actionId -> assertThat(deploymentManagement.cancelAction(actionId).getId()).isEqualTo(actionId));
|
||||
}
|
||||
|
||||
@Test
|
||||
void verifyCancellationByDistributionSetId() {
|
||||
verify(
|
||||
assignedId -> {
|
||||
actionId -> {
|
||||
deploymentManagement.cancelActionsForDistributionSet(ActionCancellationType.FORCE, ds1Type1);
|
||||
assertThat(deploymentManagement.findAction(assignedId)).isEmpty();
|
||||
assertThat(deploymentManagement.findAction(actionId)).isEmpty();
|
||||
},
|
||||
assignedId -> assertThat(deploymentManagement.findAction(assignedId))
|
||||
actionId -> assertThat(deploymentManagement.findAction(actionId))
|
||||
.hasValueSatisfying(action -> assertThat(action.getStatus()).isEqualTo(Action.Status.RUNNING)),
|
||||
null);
|
||||
}
|
||||
@@ -121,37 +182,37 @@ class DeploymentManagementTest extends AbstractAccessControllerManagementTest {
|
||||
@Test
|
||||
void verifyForceActionIsNotAllowed() {
|
||||
verify(
|
||||
assignedId -> assertThatThrownBy(() -> deploymentManagement.forceTargetAction(assignedId))
|
||||
actionId -> assertThatThrownBy(() -> deploymentManagement.forceTargetAction(actionId))
|
||||
.isInstanceOf(EntityNotFoundException.class),
|
||||
assignedId -> assertThatThrownBy(() -> deploymentManagement.forceTargetAction(assignedId))
|
||||
actionId -> assertThatThrownBy(() -> deploymentManagement.forceTargetAction(actionId))
|
||||
.isInstanceOf(InsufficientPermissionException.class),
|
||||
assignedId -> assertThat(deploymentManagement.forceTargetAction(assignedId).getActionType())
|
||||
actionId -> assertThat(deploymentManagement.forceTargetAction(actionId).getActionType())
|
||||
.isEqualTo(Action.ActionType.FORCED));
|
||||
}
|
||||
|
||||
private void verify(final Consumer<Long> noRead, final Consumer<Long> noUpdate, final Consumer<Long> readAndUpdate) {
|
||||
final Long assignedId = systemSecurityContext.runAsSystem(() -> {
|
||||
final List<Action> assignedEntity = assignDistributionSet(ds1Type1.getId(), target1Type1.getControllerId()).getAssignedEntity();
|
||||
assertThat(assignedEntity).hasSize(1).allMatch(action -> action.getTarget().getId().equals(target1Type1.getId()));
|
||||
return assignedEntity.get(0);
|
||||
private void verify(final Consumer<Long> noRead, final Consumer<Long> readNoUpdate, final Consumer<Long> readAndUpdate) {
|
||||
final Long actionId = systemSecurityContext.runAsSystem(() -> {
|
||||
final List<Action> actions = assignDistributionSet(ds1Type1.getId(), target1Type1.getControllerId()).getAssignedEntity();
|
||||
assertThat(actions).hasSize(1).allMatch(action -> action.getTarget().getId().equals(target1Type1.getId()));
|
||||
return actions.get(0);
|
||||
}).getId();
|
||||
|
||||
if (noRead != null) {
|
||||
// no read permission
|
||||
runAs(withAuthorities(READ_TARGET + "/type.id==" + targetType2.getId(), UPDATE_TARGET + "/type.id==" + targetType2.getId()),
|
||||
() -> noRead.accept(assignedId));
|
||||
() -> noRead.accept(actionId));
|
||||
}
|
||||
|
||||
if (noUpdate != null) {
|
||||
if (readNoUpdate != null) {
|
||||
// read but no update permission
|
||||
runAs(withAuthorities(READ_TARGET + "/type.id==" + targetType1.getId(), UPDATE_TARGET + "/type.id==" + targetType2.getId()),
|
||||
() -> noUpdate.accept(assignedId));
|
||||
() -> readNoUpdate.accept(actionId));
|
||||
}
|
||||
|
||||
if (readAndUpdate != null) {
|
||||
// read and update permissions
|
||||
runAs(withAuthorities(READ_TARGET + "/type.id==" + targetType1.getId(), UPDATE_TARGET + "/type.id==" + targetType1.getId()),
|
||||
() -> readAndUpdate.accept(assignedId));
|
||||
() -> readAndUpdate.accept(actionId));
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -11,7 +11,6 @@ package org.eclipse.hawkbit.repository.jpa.acm;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.eclipse.hawkbit.im.authentication.SpPermission.CREATE_PREFIX;
|
||||
import static org.eclipse.hawkbit.im.authentication.SpPermission.CREATE_TARGET;
|
||||
import static org.eclipse.hawkbit.im.authentication.SpPermission.DELETE_PREFIX;
|
||||
import static org.eclipse.hawkbit.im.authentication.SpPermission.DELETE_TARGET;
|
||||
import static org.eclipse.hawkbit.im.authentication.SpPermission.DISTRIBUTION_SET;
|
||||
@@ -23,6 +22,7 @@ import static org.eclipse.hawkbit.im.authentication.SpPermission.SOFTWARE_MODULE
|
||||
import static org.eclipse.hawkbit.im.authentication.SpPermission.SOFTWARE_MODULE_TYPE;
|
||||
import static org.eclipse.hawkbit.im.authentication.SpPermission.TARGET_TYPE;
|
||||
import static org.eclipse.hawkbit.im.authentication.SpPermission.UPDATE_PREFIX;
|
||||
import static org.eclipse.hawkbit.im.authentication.SpPermission.UPDATE_TARGET;
|
||||
import static org.eclipse.hawkbit.repository.test.util.SecurityContextSwitch.runAs;
|
||||
import static org.mockito.ArgumentMatchers.any;
|
||||
import static org.mockito.Mockito.mock;
|
||||
@@ -92,17 +92,22 @@ class SystemExecutionTest extends AbstractAccessControllerManagementTest {
|
||||
() -> verifyAccessController(distributionSetTypeAccessController));
|
||||
runAs(withAuthorities(READ_TARGET, DELETE_TARGET + "/type.id==1"),
|
||||
() -> verifyAccessController(targetAccessController));
|
||||
runAs(withAuthorities(UPDATE_PREFIX + TARGET_TYPE + "/id==1"), () -> verifyAccessController(targetTypeAccessController));
|
||||
runAs(withAuthorities(CREATE_TARGET + "/type.id==1"), () -> verifyAccessController(actionAccessController));
|
||||
runAs(withAuthorities(CREATE_PREFIX + TARGET_TYPE + "/id==1"), () -> verifyAccessController(targetTypeAccessController));
|
||||
// Action Access Controller maps CREATE/UPDATE/DELETE to UPDATE - so only UPDATE (or READ scope) is relevant
|
||||
// and update will be called for every of the mapped - so 3 times
|
||||
runAs(withAuthorities(UPDATE_TARGET + "/type.id==1"), () -> verifyAccessController(actionAccessController, 3));
|
||||
}
|
||||
|
||||
@SuppressWarnings({ "rawtypes", "unchecked" })
|
||||
private void verifyAccessController(final AccessController<?> accessController) {
|
||||
verifyAccessController(accessController, 1);
|
||||
}
|
||||
@SuppressWarnings({ "rawtypes", "unchecked" })
|
||||
private void verifyAccessController(final AccessController<?> accessController, final int times) {
|
||||
final Specification mock = mock(Specification.class);
|
||||
for (final Operation operation : Operation.values()) {
|
||||
accessController.appendAccessRules(operation, mock);
|
||||
}
|
||||
verify(mock, times(1)).and(any()); // once for every access controller is scoped only
|
||||
verify(mock, times(times)).and(any()); // once for every access controller is scoped only
|
||||
|
||||
final Specification mockAsSystem = mock(Specification.class);
|
||||
for (Operation operation : Operation.values()) {
|
||||
|
||||
Reference in New Issue
Block a user