From edbc2cf208822296be768daede85dac1cea43257 Mon Sep 17 00:00:00 2001 From: Dmitriy Kopylenko Date: Thu, 21 Nov 2019 10:28:57 -0500 Subject: [PATCH] SHIBUI-1249 --- .../controller/MetadataFiltersController.java | 48 ++++++++++++------- .../filters/EntityAttributesFilterTarget.java | 2 + ...taFiltersControllerIntegrationTests.groovy | 37 +++++++++++++- .../MetadataResolverRepositoryTests.groovy | 38 ++++++++++++++- 4 files changed, 105 insertions(+), 20 deletions(-) diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersController.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersController.java index b640f6362..c2a6f4c07 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersController.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersController.java @@ -15,6 +15,7 @@ import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.transaction.annotation.Transactional; +import org.springframework.transaction.interceptor.TransactionAspectSupport; import org.springframework.web.bind.annotation.DeleteMapping; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.GetMapping; @@ -27,6 +28,7 @@ import org.springframework.web.client.HttpClientErrorException; import org.springframework.web.servlet.support.ServletUriComponentsBuilder; +import javax.script.ScriptException; import java.net.URI; import java.util.ArrayList; import java.util.List; @@ -57,7 +59,7 @@ public class MetadataFiltersController { @ExceptionHandler public ResponseEntity notFoundHandler(HttpClientErrorException ex) { - if(ex.getStatusCode() == NOT_FOUND) { + if (ex.getStatusCode() == NOT_FOUND) { return ResponseEntity.notFound().build(); } throw ex; @@ -78,13 +80,14 @@ public ResponseEntity getOne(@PathVariable String metadataResolverId, @PathVa } @PostMapping("/Filters") + @Transactional public ResponseEntity create(@PathVariable String metadataResolverId, @RequestBody MetadataFilter createdFilter) { MetadataResolver metadataResolver = findResolverOrThrowHttp404(metadataResolverId); metadataResolver.addFilter(createdFilter); MetadataResolver persistedMr = repository.save(metadataResolver); // we reload the filters here after save - metadataResolverService.reloadFilters(persistedMr.getResourceId()); + reloadFiltersAndHandleScriptException(persistedMr.getResourceId()); MetadataFilter persistedFilter = newlyPersistedFilter(persistedMr.getMetadataFilters().stream(), createdFilter.getResourceId()); @@ -106,7 +109,7 @@ public ResponseEntity update(@PathVariable String metadataResolverId, .stream() .filter(it -> it.getResourceId().equals(resourceId)) .findFirst(); - if(!filterTobeUpdatedOptional.isPresent()) { + if (!filterTobeUpdatedOptional.isPresent()) { return ResponseEntity.notFound().build(); } MetadataFilter filterTobeUpdated = filterTobeUpdatedOptional.get(); @@ -129,8 +132,8 @@ public ResponseEntity update(@PathVariable String metadataResolverId, metadataResolver.markAsModified(); repository.save(metadataResolver); - // TODO: this is wrong - metadataResolverService.reloadFilters(metadataResolver.getResourceId()); + // TODO: do we need to reload filters here? + reloadFiltersAndHandleScriptException(metadataResolver.getResourceId()); return ResponseEntity.ok().body(persistedFilter); } @@ -149,7 +152,7 @@ public ResponseEntity delete(@PathVariable String metadataResolverId, //change that we need to make in the entire code base List updatedFilters = new ArrayList<>(resolver.getMetadataFilters()); boolean removed = updatedFilters.removeIf(f -> f.getResourceId().equals(resourceId)); - if(!removed) { + if (!removed) { throw HTTP_404_CLIENT_ERROR_EXCEPTION.get(); } resolver.setMetadataFilters(updatedFilters); @@ -164,9 +167,23 @@ public ResponseEntity delete(@PathVariable String metadataResolverId, return ResponseEntity.noContent().build(); } + private void reloadFiltersAndHandleScriptException(String resolverResourceId) { + try { + metadataResolverService.reloadFilters(resolverResourceId); + } catch (Throwable ex) { + //explicitly mark transaction for rollback when we get ScriptException as we call reloadFilters + //after persistence call. Then re-throw the exception + //to let RestControllerSupport advice return proper 400 error message + if (ex instanceof ScriptException) { + TransactionAspectSupport.currentTransactionStatus().setRollbackOnly(); + throw ex; + } + } + } + private MetadataResolver findResolverOrThrowHttp404(String resolverResourceId) { MetadataResolver resolver = repository.findByResourceId(resolverResourceId); - if(resolver == null) { + if (resolver == null) { throw HTTP_404_CLIENT_ERROR_EXCEPTION.get(); } return resolver; @@ -174,7 +191,7 @@ private MetadataResolver findResolverOrThrowHttp404(String resolverResourceId) { private MetadataFilter findFilterOrThrowHttp404(String filterResourceId) { MetadataFilter filter = filterRepository.findByResourceId(filterResourceId); - if(filter == null) { + if (filter == null) { throw HTTP_404_CLIENT_ERROR_EXCEPTION.get(); } return filter; @@ -189,28 +206,25 @@ private MetadataFilter newlyPersistedFilter(Stream filters, fina } /** - * * Add else if instanceof block here for each concrete filter types we add in the future */ private void updateConcreteFilterTypeData(MetadataFilter filterToBeUpdated, MetadataFilter filterWithUpdatedData) { //TODO: Could we maybe use Dozer here before things get out of control? https://dozermapper.github.io // Mapper mapper = new net.sf.dozer.Mapper(); // or autowire one // mapper.map(fromFilter, toFilter); - if(filterWithUpdatedData instanceof EntityAttributesFilter) { + if (filterWithUpdatedData instanceof EntityAttributesFilter) { EntityAttributesFilter toFilter = EntityAttributesFilter.class.cast(filterToBeUpdated); EntityAttributesFilter fromFilter = EntityAttributesFilter.class.cast(filterWithUpdatedData); toFilter.setEntityAttributesFilterTarget(fromFilter.getEntityAttributesFilterTarget()); toFilter.setRelyingPartyOverrides(fromFilter.getRelyingPartyOverrides()); toFilter.setAttributeRelease(fromFilter.getAttributeRelease()); - } - else if(filterWithUpdatedData instanceof EntityRoleWhiteListFilter) { + } else if (filterWithUpdatedData instanceof EntityRoleWhiteListFilter) { EntityRoleWhiteListFilter toFilter = EntityRoleWhiteListFilter.class.cast(filterToBeUpdated); EntityRoleWhiteListFilter fromFilter = EntityRoleWhiteListFilter.class.cast(filterWithUpdatedData); toFilter.setRemoveEmptyEntitiesDescriptors(fromFilter.getRemoveEmptyEntitiesDescriptors()); toFilter.setRemoveRolelessEntityDescriptors(fromFilter.getRemoveRolelessEntityDescriptors()); toFilter.setRetainedRoles(fromFilter.getRetainedRoles()); - } - else if (filterWithUpdatedData instanceof SignatureValidationFilter) { + } else if (filterWithUpdatedData instanceof SignatureValidationFilter) { SignatureValidationFilter toFilter = SignatureValidationFilter.class.cast(filterToBeUpdated); SignatureValidationFilter fromFilter = SignatureValidationFilter.class.cast(filterWithUpdatedData); toFilter.setRequireSignedRoot(fromFilter.getRequireSignedRoot()); @@ -220,13 +234,11 @@ else if (filterWithUpdatedData instanceof SignatureValidationFilter) { toFilter.setDynamicTrustedNamesStrategyRef(fromFilter.getDynamicTrustedNamesStrategyRef()); toFilter.setTrustEngineRef(fromFilter.getTrustEngineRef()); toFilter.setPublicKey(fromFilter.getPublicKey()); - } - else if(filterWithUpdatedData instanceof RequiredValidUntilFilter) { + } else if (filterWithUpdatedData instanceof RequiredValidUntilFilter) { RequiredValidUntilFilter toFilter = RequiredValidUntilFilter.class.cast(filterToBeUpdated); RequiredValidUntilFilter fromFilter = RequiredValidUntilFilter.class.cast(filterWithUpdatedData); toFilter.setMaxValidityInterval(fromFilter.getMaxValidityInterval()); - } - else if (filterWithUpdatedData instanceof NameIdFormatFilter) { + } else if (filterWithUpdatedData instanceof NameIdFormatFilter) { NameIdFormatFilter toFilter = NameIdFormatFilter.class.cast(filterToBeUpdated); NameIdFormatFilter fromFilter = NameIdFormatFilter.class.cast(filterWithUpdatedData); toFilter.setRemoveExistingFormats(fromFilter.getRemoveExistingFormats()); diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/filters/EntityAttributesFilterTarget.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/filters/EntityAttributesFilterTarget.java index ffff2ca30..8a9c2a7fb 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/filters/EntityAttributesFilterTarget.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/filters/EntityAttributesFilterTarget.java @@ -8,6 +8,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.persistence.Column; import javax.persistence.ElementCollection; import javax.persistence.Entity; import javax.persistence.FetchType; @@ -31,6 +32,7 @@ public enum EntityAttributesFilterTargetType { @ElementCollection @OrderColumn + @Column(length = 4000) private List value; public EntityAttributesFilterTargetType getEntityAttributesFilterTargetType() { diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersControllerIntegrationTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersControllerIntegrationTests.groovy index 098e7ac2b..a07353b9c 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersControllerIntegrationTests.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersControllerIntegrationTests.groovy @@ -4,6 +4,8 @@ import com.fasterxml.jackson.databind.ObjectMapper import com.fasterxml.jackson.databind.SerializationFeature import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule import edu.internet2.tier.shibboleth.admin.ui.configuration.CustomPropertiesConfiguration +import edu.internet2.tier.shibboleth.admin.ui.domain.filters.EntityAttributesFilter +import edu.internet2.tier.shibboleth.admin.ui.domain.filters.EntityAttributesFilterTarget import edu.internet2.tier.shibboleth.admin.ui.domain.resolvers.opensaml.OpenSamlChainingMetadataResolver import edu.internet2.tier.shibboleth.admin.ui.repository.MetadataResolverRepository import edu.internet2.tier.shibboleth.admin.ui.service.MetadataResolverConverterService @@ -23,6 +25,7 @@ import org.springframework.http.HttpHeaders import org.springframework.test.context.ActiveProfiles import spock.lang.Specification +import static edu.internet2.tier.shibboleth.admin.ui.domain.filters.EntityAttributesFilterTarget.EntityAttributesFilterTargetType.CONDITION_SCRIPT import static org.springframework.http.HttpMethod.PUT /** @@ -191,7 +194,7 @@ class MetadataFiltersControllerIntegrationTests extends Specification { def "POST new Filter updates resolver's modifiedDate - SHIBUI-1500"() { given: 'MetadataResolver with attached entity attributes is available in data store' def resolver = generator.buildRandomMetadataResolverOfType('FileBacked') - def filter = generator.entityAttributesFilter() + def filter = generator.entityAttributesFilter() def resolverResourceId = resolver.resourceId metadataResolverRepository.save(resolver) MetadataResolver openSamlRepresentation = metadataResolverConverterService.convertToOpenSamlRepresentation(resolver) @@ -209,6 +212,38 @@ class MetadataFiltersControllerIntegrationTests extends Specification { originalModifiedDate < afterFilterAddedModifiedDate } + def "EntityAttributesFilter with invalid script does not result in persisting that filter"() { + def resolver = generator.buildRandomMetadataResolverOfType('FileBacked') + def resolverResourceId = resolver.resourceId + metadataResolverRepository.save(resolver) + MetadataResolver openSamlRepresentation = metadataResolverConverterService.convertToOpenSamlRepresentation(resolver) + OpenSamlChainingMetadataResolverUtil.updateChainingMetadataResolver((OpenSamlChainingMetadataResolver) chainingMetadataResolver, openSamlRepresentation) + def filter = new EntityAttributesFilter().with { + it.name = 'SHIBUI-1249' + it.resourceId = 'SHIBUI-1249' + it.entityAttributesFilterTarget = new EntityAttributesFilterTarget().with { + it.entityAttributesFilterTargetType = CONDITION_SCRIPT + it.singleValue = """ + echo('invalid; + """ + it + } + it + } + + when: + def result = restTemplate.postForEntity("$BASE_URI/$resolverResourceId/Filters", filter, String) + + then: + result.statusCodeValue == 400 + + when: + result = this.restTemplate.getForEntity("$BASE_URI/$resolverResourceId", Map) + + then: + result.body.metadataFilters.size == 0 + } + private HttpEntity createRequestHttpEntityFor(Closure jsonBodySupplier) { new HttpEntity(jsonBodySupplier(), ['Content-Type': 'application/json'] as HttpHeaders) } diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/repository/MetadataResolverRepositoryTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/repository/MetadataResolverRepositoryTests.groovy index cec7fe8c5..b3f102167 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/repository/MetadataResolverRepositoryTests.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/repository/MetadataResolverRepositoryTests.groovy @@ -25,6 +25,8 @@ import spock.lang.Specification import javax.persistence.EntityManager +import static edu.internet2.tier.shibboleth.admin.ui.domain.filters.EntityAttributesFilterTarget.EntityAttributesFilterTargetType.CONDITION_SCRIPT + /** * Testing persistence of the MetadataResolver models */ @@ -198,7 +200,42 @@ class MetadataResolverRepositoryTests extends Specification { basicPersistenceOfResolverIsCorrectFor { it instanceof LocalDynamicMetadataResolver } } + def "persisting entity attributes filter target with script of more than 255 characters"() { + given: + def mdr = new MetadataResolver().with { + it.name = "SHIBUI-1588" + it + } + def filter = new EntityAttributesFilter().with { + it.name = 'SHIBUI-1588' + it.resourceId = 'SHIBUI-1588' + it.entityAttributesFilterTarget = new EntityAttributesFilterTarget().with { + it.entityAttributesFilterTargetType = CONDITION_SCRIPT + it.singleValue = """ + /* + Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut + labore et dolore magna aliqua. Cras fermentum odio eu feugiat pretium nibh ipsum. Sed augue lacus viverra vitae. + Fermentum et sollicitudin ac orci. Platea dictumst vestibulum rhoncus est pellentesque elit ullamcorper dignissim. + Rhoncus urna neque viverra justo nec ultrices dui sapien. Tortor id aliquet lectus proin nibh nisl condimentum id venenatis. + Massa id neque aliquam vestibulum morbi blandit cursus risus. Metus aliquam eleifend mi in nulla posuere sollicitudin. + Arcu ac tortor dignissim convallis aenean. Et tortor consequat id porta nibh venenatis cras. + Netus et malesuada fames ac turpis egestas. Bibendum arcu vitae elementum curabitur. + Volutpat consequat mauris nunc congue nisi vitae suscipit. + */ + """ + it + } + it + } + mdr.addFilter(filter) + when: + metadataResolverRepository.save(mdr) + entityManager.flush() + + then: + noExceptionThrown() + } private void basicPersistenceOfResolverIsCorrectFor(Closure resolverTypeCheck) { assert metadataResolverRepository.findAll().size() > 0 @@ -227,5 +264,4 @@ class MetadataResolverRepositoryTests extends Specification { } resolver } - }