Merge pull request #7617 from dennishendriksen/fix/7615-rlsDeleteLarge

Fix #7615 Deleting all row-level secured data does not delete all data
molgenis · Aug 1, 2018 · 3585e2d · 3585e2d
2 parents e4e26ac + b75c3a7
commit 3585e2d
Show file tree

Hide file tree

Showing 3 changed files with 63 additions and 40 deletions.
diff --git a/...in/java/org/molgenis/data/security/owned/AbstractRowLevelSecurityRepositoryDecorator.java b/...in/java/org/molgenis/data/security/owned/AbstractRowLevelSecurityRepositoryDecorator.java
@@ -176,12 +176,7 @@ public void delete(E entity)
 	public void delete(Stream<E> entities)
 	{
 		// delete entity before deleting ACL
-		partition(entities.iterator(), BATCH_SIZE).forEachRemaining(entityBatch ->
-		{
-			List<E> filteredEntityBatch = entityBatch.stream().filter(entity -> isActionPermitted(entity, DELETE)).collect(toList());
-			delegate().delete(filteredEntityBatch.stream());
-			filteredEntityBatch.forEach(this::deleteAcl);
-		});
+		partition(entities.iterator(), BATCH_SIZE).forEachRemaining(this::deleteBatch);
 	}
 
 	@Override
@@ -195,7 +190,6 @@ public void deleteById(Object id)
 		}
 	}
 
-	@Transactional
 	@Override
 	public void deleteAll(Stream<Object> ids)
 	{
@@ -211,7 +205,17 @@ public void deleteAll(Stream<Object> ids)
 	@Override
 	public void deleteAll()
 	{
-		delegate().delete(findAllPermitted(new QueryImpl<>(), DELETE));
+		// finding during deletion is tricky business: can't use iterator or findAll here
+		delegate().forEachBatched(this::deleteBatch, BATCH_SIZE);
+	}
+
+	private void deleteBatch(List<E> entities)
+	{
+		List<E> filteredEntities = entities.stream()
+										   .filter(entity -> isActionPermitted(entity, DELETE))
+										   .collect(toList());
+		delegate().delete(filteredEntities.stream());
+		filteredEntities.forEach(this::deleteAcl);
 	}
 
 	@Override

diff --git a/...src/test/java/org/molgenis/data/security/meta/PackageRepositorySecurityDecoratorTest.java b/...src/test/java/org/molgenis/data/security/meta/PackageRepositorySecurityDecoratorTest.java
@@ -11,7 +11,6 @@
 import org.molgenis.data.security.PackagePermission;
 import org.molgenis.data.security.exception.NullParentPackageNotSuException;
 import org.molgenis.data.security.exception.PackagePermissionDeniedException;
-import org.molgenis.data.support.QueryImpl;
 import org.molgenis.security.core.UserPermissionEvaluator;
 import org.springframework.security.acls.model.AlreadyExistsException;
 import org.springframework.security.acls.model.MutableAcl;
@@ -21,9 +20,12 @@
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
+import java.util.List;
+import java.util.function.Consumer;
 import java.util.stream.Stream;
 
 import static java.util.Arrays.asList;
+import static java.util.Collections.singletonList;
 import static java.util.stream.Collectors.toList;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.*;
@@ -233,31 +235,31 @@ public void testDeleteById()
 		verify(delegateRepository).deleteById("1");
 	}
 
+	@SuppressWarnings("unchecked")
 	@Test
 	public void testDeleteAll()
 	{
-		Package package1 = mock(Package.class);
-		Package package2 = mock(Package.class);
-		Package package3 = mock(Package.class);
-		Package package4 = mock(Package.class);
-		when(package3.getId()).thenReturn("3");
-		when(package4.getId()).thenReturn("4");
+		PackageMetadata packageMetadata = mock(PackageMetadata.class);
 
-		when(package1.getParent()).thenReturn(package3);
-		when(package2.getParent()).thenReturn(package4);
+		Package permittedParentPackage = when(mock(Package.class).getId()).thenReturn("permittedParentPackageId")
+																		  .getMock();
 
-		when(delegateRepository.findAll(new QueryImpl<Package>().pageSize(2147483647))).thenReturn(
-				Stream.of(package1, package2));
+		Package permittedPackage = when(mock(Package.class).getId()).thenReturn("permittedPackageId").getMock();
+		when(permittedPackage.getParent()).thenReturn(permittedParentPackage);
+		Package notPermittedPackage = mock(Package.class);
+		doAnswer(invocation ->
+		{
+			Consumer<List<Package>> consumer = invocation.getArgument(0);
+			consumer.accept(asList(permittedPackage, notPermittedPackage));
+			return null;
+		}).when(delegateRepository).forEachBatched(any(), eq(1000));
 
-		doReturn(true).when(userPermissionEvaluator).hasPermission(new PackageIdentity("3"), UPDATE);
-		doReturn(false).when(userPermissionEvaluator).hasPermission(new PackageIdentity("4"), UPDATE);
+		when(userPermissionEvaluator.hasPermission(new PackageIdentity(permittedParentPackage),
+				PackagePermission.UPDATE)).thenReturn(true);
 		repo.deleteAll();
-
-		//TODO: how to verify the deleteAcl method in the "filter" of the stream
-		ArgumentCaptor<Stream<Package>> captor = ArgumentCaptor.forClass(Stream.class);
-		verify(delegateRepository).delete(captor.capture());
-		assertEquals(captor.getValue().collect(toList()), asList(package1));
-
+		ArgumentCaptor<Stream<Package>> entityStreamCaptor = ArgumentCaptor.forClass(Stream.class);
+		verify(delegateRepository).delete(entityStreamCaptor.capture());
+		assertEquals(entityStreamCaptor.getValue().collect(toList()), singletonList(permittedPackage));
 	}
 
 	@Test

diff --git a/...c/test/java/org/molgenis/data/security/owned/RowLevelSecurityRepositoryDecoratorTest.java b/...c/test/java/org/molgenis/data/security/owned/RowLevelSecurityRepositoryDecoratorTest.java
@@ -30,6 +30,7 @@
 import java.util.stream.Stream;
 
 import static com.google.common.collect.Lists.newArrayList;
+import static java.util.Arrays.asList;
 import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
 import static java.util.stream.Collectors.toList;
@@ -223,31 +224,47 @@ public void testDeleteByIdPermissionDenied()
 		verify(mutableAclService, times(0)).deleteAcl(new EntityIdentity(entityTypeId, entityId), true);
 	}
 
+	@SuppressWarnings("unchecked")
 	@Test
 	public void testDeleteAll()
 	{
-		Entity entity = getEntityMock();
-		when(delegateRepository.findAll(new QueryImpl<>().setOffset(0).setPageSize(Integer.MAX_VALUE))).thenAnswer(
-				invocation -> Stream.of(entity));
-		when(userPermissionEvaluator.hasPermission(new EntityIdentity(entity), EntityPermission.DELETE)).thenReturn(
-				true);
-		rowLevelSecurityRepositoryDecorator.deleteAll();
+		EntityType entityType = when(mock(EntityType.class).getId()).thenReturn("entityTypeId").getMock();
+		Entity permittedEntity = when(mock(Entity.class).getIdValue()).thenReturn("permittedEntityId").getMock();
+		when(permittedEntity.getEntityType()).thenReturn(entityType);
+		Entity notPermittedEntity = when(mock(Entity.class).getIdValue()).thenReturn("notPermittedEntityId").getMock();
+		when(notPermittedEntity.getEntityType()).thenReturn(entityType);
+		doAnswer(invocation ->
+		{
+			Consumer<List<Entity>> consumer = invocation.getArgument(0);
+			consumer.accept(asList(permittedEntity, notPermittedEntity));
+			return null;
+		}).when(delegateRepository).forEachBatched(any(), eq(1000));
 
-		@SuppressWarnings("unchecked")
+		when(userPermissionEvaluator.hasPermission(new EntityIdentity(permittedEntity),
+				EntityPermission.DELETE)).thenReturn(true);
+		rowLevelSecurityRepositoryDecorator.deleteAll();
 		ArgumentCaptor<Stream<Entity>> entityStreamCaptor = ArgumentCaptor.forClass(Stream.class);
 		verify(delegateRepository).delete(entityStreamCaptor.capture());
-		assertEquals(entityStreamCaptor.getValue().collect(toList()), singletonList(entity));
+		assertEquals(entityStreamCaptor.getValue().collect(toList()), singletonList(permittedEntity));
 	}
 
+	@SuppressWarnings("unchecked")
 	@Test
 	public void testDeleteAllPermissionDenied()
 	{
-		Entity entity = getEntityMock();
-		when(delegateRepository.findAll(new QueryImpl<>().setOffset(0).setPageSize(Integer.MAX_VALUE))).thenAnswer(
-				invocation -> Stream.of(entity));
-		rowLevelSecurityRepositoryDecorator.deleteAll();
+		EntityType entityType = when(mock(EntityType.class).getId()).thenReturn("entityTypeId").getMock();
+		Entity permittedEntity = when(mock(Entity.class).getIdValue()).thenReturn("permittedEntityId").getMock();
+		when(permittedEntity.getEntityType()).thenReturn(entityType);
+		Entity notPermittedEntity = when(mock(Entity.class).getIdValue()).thenReturn("notPermittedEntityId").getMock();
+		when(notPermittedEntity.getEntityType()).thenReturn(entityType);
+		doAnswer(invocation ->
+		{
+			Consumer<List<Entity>> consumer = invocation.getArgument(0);
+			consumer.accept(asList(permittedEntity, notPermittedEntity));
+			return null;
+		}).when(delegateRepository).forEachBatched(any(), eq(1000));
 
-		@SuppressWarnings("unchecked")
+		rowLevelSecurityRepositoryDecorator.deleteAll();
 		ArgumentCaptor<Stream<Entity>> entityStreamCaptor = ArgumentCaptor.forClass(Stream.class);
 		verify(delegateRepository).delete(entityStreamCaptor.capture());
 		assertEquals(entityStreamCaptor.getValue().collect(toList()), emptyList());