From dcb39f29230a70d332dbd3aadc660ac5fe6de82e Mon Sep 17 00:00:00 2001 From: Marcos Lopez Gonzalez Date: Thu, 29 Apr 2021 13:49:12 +0200 Subject: [PATCH 1/4] added tests for PUTs with key mismatches --- .../gbif/registry/ws/it/NetworkEntityIT.java | 22 +++++++++++++++---- .../collections/BaseCollectionEntityIT.java | 15 +++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/registry-integration-tests/src/test/java/org/gbif/registry/ws/it/NetworkEntityIT.java b/registry-integration-tests/src/test/java/org/gbif/registry/ws/it/NetworkEntityIT.java index 4287936311..d8943af3e5 100644 --- a/registry-integration-tests/src/test/java/org/gbif/registry/ws/it/NetworkEntityIT.java +++ b/registry-integration-tests/src/test/java/org/gbif/registry/ws/it/NetworkEntityIT.java @@ -33,6 +33,7 @@ import org.gbif.registry.database.TestCaseDatabaseInitializer; import org.gbif.registry.search.test.EsManageServer; import org.gbif.registry.test.TestDataFactory; +import org.gbif.registry.ws.client.NetworkEntityClient; import org.gbif.registry.ws.it.fixtures.TestConstants; import org.gbif.ws.client.filter.SimplePrincipalProvider; import org.gbif.ws.security.KeyStore; @@ -62,11 +63,11 @@ import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.test.context.ContextConfiguration; import com.google.common.base.Preconditions; import com.google.common.base.Throwables; import com.google.common.collect.Lists; -import org.springframework.test.context.ContextConfiguration; import static org.gbif.registry.ws.it.LenientAssert.assertLenientEquals; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -79,7 +80,11 @@ * A generic test for all network entities that implement all interfaces required by the * BaseNetworkEntityResource. */ -@ContextConfiguration(initializers = {BaseItTest.ContextInitializer.class, BaseItTest.EsContainerContextInitializer.class}) +@ContextConfiguration( + initializers = { + BaseItTest.ContextInitializer.class, + BaseItTest.EsContainerContextInitializer.class + }) public abstract class NetworkEntityIT< T extends NetworkEntity & Contactable & Taggable & MachineTaggable & Commentable & Endpointable @@ -91,8 +96,7 @@ public abstract class NetworkEntityIT< private final TestDataFactory testDataFactory; - @Autowired - private DataSource dataSource; + @Autowired private DataSource dataSource; @RegisterExtension public TestCaseDatabaseInitializer databaseRule = new TestCaseDatabaseInitializer(); @@ -673,6 +677,16 @@ public void testIdentifierSearch(ServiceType serviceType) { assertEquals(0, res.getResults().size()); } + @Test + public void updateEntityKeyMismatchTest() { + ServiceType serviceType = ServiceType.CLIENT; + NetworkEntityClient crudClient = (NetworkEntityClient) client; + T entity = create(newEntity(serviceType), serviceType, 1); + + assertThrows( + IllegalArgumentException.class, () -> crudClient.updateResource(UUID.randomUUID(), entity)); + } + /** @return a new example instance */ protected abstract T newEntity(ServiceType serviceType); diff --git a/registry-integration-tests/src/test/java/org/gbif/registry/ws/it/collections/BaseCollectionEntityIT.java b/registry-integration-tests/src/test/java/org/gbif/registry/ws/it/collections/BaseCollectionEntityIT.java index 4a136c6306..3f6f05dca1 100644 --- a/registry-integration-tests/src/test/java/org/gbif/registry/ws/it/collections/BaseCollectionEntityIT.java +++ b/registry-integration-tests/src/test/java/org/gbif/registry/ws/it/collections/BaseCollectionEntityIT.java @@ -35,6 +35,7 @@ import org.gbif.registry.database.TestCaseDatabaseInitializer; import org.gbif.registry.identity.service.IdentityService; import org.gbif.registry.search.test.EsManageServer; +import org.gbif.registry.ws.client.collections.CrudClient; import org.gbif.registry.ws.it.BaseItTest; import org.gbif.ws.NotFoundException; import org.gbif.ws.client.filter.SimplePrincipalProvider; @@ -46,6 +47,7 @@ import javax.validation.ValidationException; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.EnumSource; @@ -189,6 +191,19 @@ public void updateInvalidEntityTest(ServiceType serviceType) { assertThrows(ValidationException.class, () -> service.update(newEntity)); } + @Test + public void updateEntityKeyMismatchTest() { + CrudClient crudClient = (CrudClient) client; + + T entity = newEntity(); + UUID key = crudClient.create(entity); + T entityCreated = crudClient.get(key); + + assertThrows( + IllegalArgumentException.class, + () -> crudClient.updateResource(UUID.randomUUID(), entityCreated)); + } + @ParameterizedTest @EnumSource(ServiceType.class) public void getMissingEntity(ServiceType serviceType) { From a812b1c95d5498995c792c872e49511e262ad7a8 Mon Sep 17 00:00:00 2001 From: Marcos Lopez Gonzalez Date: Thu, 29 Apr 2021 14:26:53 +0200 Subject: [PATCH 2/4] Fixed ITs --- .../registry/ws/resources/BaseNetworkEntityResource.java | 3 ++- .../collections/ExtendedCollectionEntityResource.java | 5 +++-- .../registry/ws/resources/collections/PersonResource.java | 3 ++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/registry-ws/src/main/java/org/gbif/registry/ws/resources/BaseNetworkEntityResource.java b/registry-ws/src/main/java/org/gbif/registry/ws/resources/BaseNetworkEntityResource.java index 60125d2ccc..50849359b5 100644 --- a/registry-ws/src/main/java/org/gbif/registry/ws/resources/BaseNetworkEntityResource.java +++ b/registry-ws/src/main/java/org/gbif/registry/ws/resources/BaseNetworkEntityResource.java @@ -235,12 +235,13 @@ public PagingResponse listByIdentifier(String identifier, Pageable page) { @Validated({PostPersist.class, Default.class}) @Trim @Transactional - @Secured({ADMIN_ROLE, EDITOR_ROLE, IPT_ROLE}) public void update(@PathVariable("key") UUID key, @Valid @RequestBody @Trim T entity) { checkArgument(key.equals(entity.getKey())); update(entity); } + @Secured({ADMIN_ROLE, EDITOR_ROLE, IPT_ROLE}) + @Transactional @Override public void update(T entity) { Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); diff --git a/registry-ws/src/main/java/org/gbif/registry/ws/resources/collections/ExtendedCollectionEntityResource.java b/registry-ws/src/main/java/org/gbif/registry/ws/resources/collections/ExtendedCollectionEntityResource.java index 79bc2d93c6..811046a16e 100644 --- a/registry-ws/src/main/java/org/gbif/registry/ws/resources/collections/ExtendedCollectionEntityResource.java +++ b/registry-ws/src/main/java/org/gbif/registry/ws/resources/collections/ExtendedCollectionEntityResource.java @@ -174,14 +174,15 @@ public UUID create(@RequestBody @Trim T entity) { @PutMapping(value = "{key}", consumes = MediaType.APPLICATION_JSON_VALUE) @Validated({PostPersist.class, Default.class}) - @Transactional @Trim - @Secured({GRSCICOLL_ADMIN_ROLE, GRSCICOLL_EDITOR_ROLE}) + @Transactional public void update(@PathVariable("key") UUID key, @Valid @RequestBody @Trim T entity) { checkArgument(key.equals(entity.getKey())); update(entity); } + @Secured({GRSCICOLL_ADMIN_ROLE, GRSCICOLL_EDITOR_ROLE}) + @Transactional @Override public void update(T entity) { preUpdate(entity); diff --git a/registry-ws/src/main/java/org/gbif/registry/ws/resources/collections/PersonResource.java b/registry-ws/src/main/java/org/gbif/registry/ws/resources/collections/PersonResource.java index fdeb83e8ca..cfb35dbb8b 100644 --- a/registry-ws/src/main/java/org/gbif/registry/ws/resources/collections/PersonResource.java +++ b/registry-ws/src/main/java/org/gbif/registry/ws/resources/collections/PersonResource.java @@ -160,12 +160,13 @@ public UUID create(@RequestBody @Trim Person person) { @Validated({PostPersist.class, Default.class}) @Trim @Transactional - @Secured({GRSCICOLL_ADMIN_ROLE, GRSCICOLL_EDITOR_ROLE}) public void update(@PathVariable("key") UUID key, @Valid @RequestBody @Trim Person entity) { checkArgument(key.equals(entity.getKey())); update(entity); } + @Secured({GRSCICOLL_ADMIN_ROLE, GRSCICOLL_EDITOR_ROLE}) + @Transactional @Override public void update(Person person) { preUpdate(person); From 7c5859b8faefce4b46bdf17c988aed60fc9e9b84 Mon Sep 17 00:00:00 2001 From: fmendezh Date: Thu, 29 Apr 2021 14:54:50 +0200 Subject: [PATCH 3/4] indexing changes in network and organizations DatasetMapper deleted networks were being added to the list of networks --- .../gbif/registry/search/DatasetIndexUpdateListener.java | 8 +++++--- .../gbif/registry/persistence/mapper/DatasetMapper.xml | 7 ++++--- .../org/gbif/registry/ws/resources/NetworkResource.java | 6 ++++-- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/registry-events/src/main/java/org/gbif/registry/search/DatasetIndexUpdateListener.java b/registry-events/src/main/java/org/gbif/registry/search/DatasetIndexUpdateListener.java index 5cd72d39fd..b4faf12a12 100644 --- a/registry-events/src/main/java/org/gbif/registry/search/DatasetIndexUpdateListener.java +++ b/registry-events/src/main/java/org/gbif/registry/search/DatasetIndexUpdateListener.java @@ -19,7 +19,6 @@ import org.gbif.api.model.registry.Installation; import org.gbif.api.model.registry.Network; import org.gbif.api.model.registry.Organization; -import org.gbif.api.model.registry.Tag; import org.gbif.api.service.registry.DatasetService; import org.gbif.registry.events.ChangedComponentEvent; import org.gbif.registry.events.CreateEvent; @@ -100,14 +99,17 @@ public final void updated(UpdateEvent event) { public final void deleted(DeleteEvent event) { if (event.getObjectClass().equals(Dataset.class)) { indexService.delete((Dataset) event.getOldObject()); + } else if (event.getObjectClass().equals(Organization.class)) { + indexService.index((Organization) event.getOldObject()); + } else if (event.getObjectClass().equals(Network.class)) { + indexService.index((Network) event.getOldObject()); } } @Subscribe public final void updatedComponent(ChangedComponentEvent event) { // only fire in case of tagged datasets - if (event.getTargetClass().equals(Dataset.class) - && event.getComponentClass().equals(Tag.class)) { + if (event.getTargetClass().equals(Dataset.class)) { // we only put tagged datasets onto the queue for this event type! UUID key = event.getTargetEntityKey(); try { diff --git a/registry-persistence/src/main/resources/org/gbif/registry/persistence/mapper/DatasetMapper.xml b/registry-persistence/src/main/resources/org/gbif/registry/persistence/mapper/DatasetMapper.xml index 62fbfd6606..adb594667b 100644 --- a/registry-persistence/src/main/resources/org/gbif/registry/persistence/mapper/DatasetMapper.xml +++ b/registry-persistence/src/main/resources/org/gbif/registry/persistence/mapper/DatasetMapper.xml @@ -443,9 +443,10 @@ diff --git a/registry-ws/src/main/java/org/gbif/registry/ws/resources/NetworkResource.java b/registry-ws/src/main/java/org/gbif/registry/ws/resources/NetworkResource.java index db75e45758..2a6a82267f 100644 --- a/registry-ws/src/main/java/org/gbif/registry/ws/resources/NetworkResource.java +++ b/registry-ws/src/main/java/org/gbif/registry/ws/resources/NetworkResource.java @@ -63,6 +63,7 @@ public class NetworkResource extends BaseNetworkEntityResource implemen private final DatasetMapper datasetMapper; private final NetworkMapper networkMapper; private final OrganizationMapper organizationMapper; + private final EventManager eventManager; public NetworkResource( MapperServiceLocator mapperServiceLocator, @@ -74,6 +75,7 @@ public NetworkResource( Network.class, eventManager, withMyBatis); + this.eventManager = eventManager; this.datasetMapper = mapperServiceLocator.getDatasetMapper(); this.networkMapper = mapperServiceLocator.getNetworkMapper(); this.organizationMapper = mapperServiceLocator.getOrganizationMapper(); @@ -125,7 +127,7 @@ public PagingResponse listConstituents( @Override public void addConstituent(@PathVariable("key") UUID networkKey, @PathVariable UUID datasetKey) { networkMapper.addDatasetConstituent(networkKey, datasetKey); - ChangedComponentEvent.newInstance(datasetKey, Dataset.class, Network.class); + eventManager.post(ChangedComponentEvent.newInstance(datasetKey, Dataset.class, Network.class)); } @DeleteMapping("{key}/constituents/{datasetKey}") @@ -134,7 +136,7 @@ public void addConstituent(@PathVariable("key") UUID networkKey, @PathVariable U public void removeConstituent( @PathVariable("key") UUID networkKey, @PathVariable UUID datasetKey) { networkMapper.deleteDatasetConstituent(networkKey, datasetKey); - ChangedComponentEvent.newInstance(datasetKey, Dataset.class, Network.class); + eventManager.post(ChangedComponentEvent.newInstance(datasetKey, Dataset.class, Network.class)); } @GetMapping("suggest") From c24ad11a1119647cb223a0044ef536d3a3b05eca Mon Sep 17 00:00:00 2001 From: Marcos Lopez Gonzalez Date: Thu, 29 Apr 2021 15:28:04 +0200 Subject: [PATCH 4/4] Fixed ITs --- .../ws/it/OrganizationCreationIT.java | 22 ++++++++++++++----- .../resources/BaseNetworkEntityResource.java | 2 +- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/registry-integration-tests/src/test/java/org/gbif/registry/ws/it/OrganizationCreationIT.java b/registry-integration-tests/src/test/java/org/gbif/registry/ws/it/OrganizationCreationIT.java index 1f4a3989e6..05e85126e9 100644 --- a/registry-integration-tests/src/test/java/org/gbif/registry/ws/it/OrganizationCreationIT.java +++ b/registry-integration-tests/src/test/java/org/gbif/registry/ws/it/OrganizationCreationIT.java @@ -142,9 +142,7 @@ private void setupPrincipal(String name, Enum... roles) { } } - /** - * It is not in the scope of this test to test the email bits. - */ + /** It is not in the scope of this test to test the email bits. */ @ParameterizedTest @EnumSource(ServiceType.class) public void testEndorsements(ServiceType serviceType) { @@ -345,14 +343,28 @@ public void testSetEndorsementsByNonAdmin(ServiceType serviceType) { // reset principal - use USER role setupPrincipal(TEST_USER, USER); + OrganizationService userService = getService(serviceType, organizationResource, userOrganizationClient); Organization createdOrganization = userService.get(organization.getKey()); createdOrganization.setEndorsementApproved(true); - // make sure an app can not change the endorsementApproved directly - assertThrows(AccessDeniedException.class, () -> userService.update(createdOrganization)); + if (serviceType == ServiceType.RESOURCE) { + // we use the resource class directly to use the update method of the API, which is the one that is secured + OrganizationResource userServiceResource = + (OrganizationResource) + getService(serviceType, organizationResource, userOrganizationClient); + + // make sure an app can not change the endorsementApproved directly + assertThrows( + AccessDeniedException.class, + () -> userServiceResource.update(createdOrganization.getKey(), createdOrganization)); + } else if (serviceType == ServiceType.CLIENT) { + // the client always uses the secured method + // make sure an app can not change the endorsementApproved directly + assertThrows(AccessDeniedException.class, () -> userService.update(createdOrganization)); + } } @Test diff --git a/registry-ws/src/main/java/org/gbif/registry/ws/resources/BaseNetworkEntityResource.java b/registry-ws/src/main/java/org/gbif/registry/ws/resources/BaseNetworkEntityResource.java index 50849359b5..4c205a6f65 100644 --- a/registry-ws/src/main/java/org/gbif/registry/ws/resources/BaseNetworkEntityResource.java +++ b/registry-ws/src/main/java/org/gbif/registry/ws/resources/BaseNetworkEntityResource.java @@ -232,6 +232,7 @@ public PagingResponse listByIdentifier(String identifier, Pageable page) { * @param entity entity that extends NetworkEntity */ @PutMapping(value = "{key}", consumes = MediaType.APPLICATION_JSON_VALUE) + @Secured({ADMIN_ROLE, EDITOR_ROLE, IPT_ROLE}) @Validated({PostPersist.class, Default.class}) @Trim @Transactional @@ -240,7 +241,6 @@ public void update(@PathVariable("key") UUID key, @Valid @RequestBody @Trim T en update(entity); } - @Secured({ADMIN_ROLE, EDITOR_ROLE, IPT_ROLE}) @Transactional @Override public void update(T entity) {