From 963b23f94078dee3ad2d3ab9f6ca6474f42873a6 Mon Sep 17 00:00:00 2001 From: chasegawa Date: Tue, 17 Aug 2021 18:25:51 -0700 Subject: [PATCH] SHIBUI-2024 Added validation for filters urls --- .../controller/MetadataFiltersController.java | 234 ++++++++++-------- .../DynamicHttpMetadataResolverValidator.java | 8 +- ...leBackedHttpMetadataResolverValidator.java | 7 +- .../ui/security/service/GroupServiceImpl.java | 2 +- .../ui/security/service/IGroupService.java | 2 +- .../JPAEntityDescriptorServiceImpl.java | 4 +- ...taFiltersControllerIntegrationTests.groovy | 8 +- 7 files changed, 149 insertions(+), 116 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 33d4458b5..7f48faa1a 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 @@ -1,14 +1,12 @@ package edu.internet2.tier.shibboleth.admin.ui.controller; import edu.internet2.tier.shibboleth.admin.ui.domain.filters.EntityAttributesFilter; -import edu.internet2.tier.shibboleth.admin.ui.domain.filters.EntityRoleWhiteListFilter; import edu.internet2.tier.shibboleth.admin.ui.domain.filters.MetadataFilter; -import edu.internet2.tier.shibboleth.admin.ui.domain.filters.NameIdFormatFilter; -import edu.internet2.tier.shibboleth.admin.ui.domain.filters.RequiredValidUntilFilter; -import edu.internet2.tier.shibboleth.admin.ui.domain.filters.SignatureValidationFilter; import edu.internet2.tier.shibboleth.admin.ui.domain.resolvers.MetadataResolver; import edu.internet2.tier.shibboleth.admin.ui.repository.FilterRepository; import edu.internet2.tier.shibboleth.admin.ui.repository.MetadataResolverRepository; +import edu.internet2.tier.shibboleth.admin.ui.security.service.IGroupService; +import edu.internet2.tier.shibboleth.admin.ui.security.service.UserService; import edu.internet2.tier.shibboleth.admin.ui.service.MetadataResolverService; import org.springframework.beans.factory.annotation.Autowired; @@ -37,52 +35,39 @@ import java.util.stream.Stream; import static java.util.stream.Collectors.toList; +import static org.springframework.http.HttpStatus.BAD_REQUEST; import static org.springframework.http.HttpStatus.NOT_FOUND; @RestController @RequestMapping("/api/MetadataResolvers/{metadataResolverId}") public class MetadataFiltersController { + private static final Supplier HTTP_400_BAD_REQUEST_EXCEPTION = () -> new HttpClientErrorException(BAD_REQUEST); + private static final Supplier HTTP_404_CLIENT_ERROR_EXCEPTION = () -> new HttpClientErrorException(NOT_FOUND); @Autowired - private MetadataResolverRepository repository; - - @Autowired - private MetadataResolverService metadataResolverService; + org.opensaml.saml.metadata.resolver.MetadataResolver chainingMetadataResolver; @Autowired private FilterRepository filterRepository; @Autowired - org.opensaml.saml.metadata.resolver.MetadataResolver chainingMetadataResolver; - - private static final Supplier HTTP_404_CLIENT_ERROR_EXCEPTION = () -> new HttpClientErrorException(NOT_FOUND); + private IGroupService groupService; - @ExceptionHandler - public ResponseEntity notFoundHandler(HttpClientErrorException ex) { - if (ex.getStatusCode() == NOT_FOUND) { - return ResponseEntity.notFound().build(); - } - throw ex; - } + @Autowired + private MetadataResolverService metadataResolverService; - @GetMapping("/Filters") - @Transactional(readOnly = true) - public ResponseEntity getAll(@PathVariable String metadataResolverId) { - MetadataResolver resolver = findResolverOrThrowHttp404(metadataResolverId); - return ResponseEntity.ok(resolver.getMetadataFilters()); - } + @Autowired + private MetadataResolverRepository repository; - @GetMapping("/Filters/{resourceId}") - @Transactional(readOnly = true) - public ResponseEntity getOne(@PathVariable String metadataResolverId, @PathVariable String resourceId) { - MetadataResolver resolver = findResolverOrThrowHttp404(metadataResolverId); - return ResponseEntity.ok(findFilterOrThrowHttp404(resourceId)); - } + @Autowired + private UserService userService; @PostMapping("/Filters") @Transactional public ResponseEntity create(@PathVariable String metadataResolverId, @RequestBody MetadataFilter createdFilter) { MetadataResolver metadataResolver = findResolverOrThrowHttp404(metadataResolverId); + validateFilterOrThrowHttp400(createdFilter); + metadataResolver.addFilter(createdFilter); MetadataResolver persistedMr = repository.save(metadataResolver); @@ -92,57 +77,14 @@ public ResponseEntity create(@PathVariable String metadataResolverId, @Reques MetadataFilter persistedFilter = newlyPersistedFilter(persistedMr.getMetadataFilters().stream(), createdFilter.getResourceId()); return ResponseEntity - .created(getResourceUriFor(persistedMr, createdFilter.getResourceId())) - .body(persistedFilter); - } - - @PutMapping("/Filters/{resourceId}") - @Transactional - public ResponseEntity update(@PathVariable String metadataResolverId, - @PathVariable String resourceId, - @RequestBody MetadataFilter updatedFilter) { - - MetadataResolver metadataResolver = findResolverOrThrowHttp404(metadataResolverId); - - //Now we operate directly on the filter attached to MetadataResolver, - //Instead of fetching filter separately, to accommodate correct envers versioning with uni-directional one-to-many - Optional filterTobeUpdatedOptional = metadataResolver.getMetadataFilters() - .stream() - .filter(it -> it.getResourceId().equals(resourceId)) - .findFirst(); - if (!filterTobeUpdatedOptional.isPresent()) { - return ResponseEntity.notFound().build(); - } - MetadataFilter filterTobeUpdated = filterTobeUpdatedOptional.get(); - if (!resourceId.equals(updatedFilter.getResourceId())) { - return new ResponseEntity(HttpStatus.CONFLICT); - } - - // Verify we're the only one attempting to update the filter - if (updatedFilter.getVersion() != filterTobeUpdated.getVersion()) { - return new ResponseEntity(HttpStatus.CONFLICT); - } - - filterTobeUpdated.setName(updatedFilter.getName()); - filterTobeUpdated.setFilterEnabled(updatedFilter.isFilterEnabled()); - updatedFilter.updateConcreteFilterTypeData(filterTobeUpdated); - - MetadataFilter persistedFilter = filterRepository.save(filterTobeUpdated); - - //To support envers versioning from MetadataResolver side - metadataResolver.markAsModified(); - repository.save(metadataResolver); - - // TODO: do we need to reload filters here? - reloadFiltersAndHandleScriptException(metadataResolver.getResourceId()); - - return ResponseEntity.ok().body(persistedFilter); + .created(getResourceUriFor(persistedMr, createdFilter.getResourceId())) + .body(persistedFilter); } @DeleteMapping("/Filters/{resourceId}") @Transactional public ResponseEntity delete(@PathVariable String metadataResolverId, - @PathVariable String resourceId) { + @PathVariable String resourceId) { MetadataResolver resolver = findResolverOrThrowHttp404(metadataResolverId); MetadataFilter filterToDelete = findFilterOrThrowHttp404(resourceId); @@ -168,6 +110,66 @@ public ResponseEntity delete(@PathVariable String metadataResolverId, return ResponseEntity.noContent().build(); } + private MetadataFilter findFilterOrThrowHttp404(String filterResourceId) { + MetadataFilter filter = filterRepository.findByResourceId(filterResourceId); + if (filter == null) { + throw HTTP_404_CLIENT_ERROR_EXCEPTION.get(); + } + return filter; + } + + private MetadataResolver findResolverOrThrowHttp404(String resolverResourceId) { + MetadataResolver resolver = repository.findByResourceId(resolverResourceId); + if (resolver == null) { + throw HTTP_404_CLIENT_ERROR_EXCEPTION.get(); + } + return resolver; + } + + @GetMapping("/Filters") + @Transactional(readOnly = true) + public ResponseEntity getAll(@PathVariable String metadataResolverId) { + MetadataResolver resolver = findResolverOrThrowHttp404(metadataResolverId); + return ResponseEntity.ok(resolver.getMetadataFilters()); + } + + @GetMapping("/Filters/{resourceId}") + @Transactional(readOnly = true) + public ResponseEntity getOne(@PathVariable String metadataResolverId, @PathVariable String resourceId) { + MetadataResolver resolver = findResolverOrThrowHttp404(metadataResolverId); + return ResponseEntity.ok(findFilterOrThrowHttp404(resourceId)); + } + + private static URI getResourceUriFor(MetadataResolver mr, String filterResourceId) { + return ServletUriComponentsBuilder + .fromCurrentServletMapping().path("/api/MetadataResolvers/") + .pathSegment(mr.getResourceId()) + .pathSegment("Filters") + .pathSegment(filterResourceId) + .build() + .toUri(); + } + + private MetadataFilter newlyPersistedFilter(Stream filters, final String filterResourceId) { + MetadataFilter persistedFilter = filters + .filter(f -> f.getResourceId().equals(filterResourceId)) + .collect(toList()).get(0); + + return persistedFilter; + } + + @ExceptionHandler + public ResponseEntity notFoundHandler(HttpClientErrorException ex) { + switch (ex.getStatusCode()) { + case NOT_FOUND: + return ResponseEntity.notFound().build(); + case BAD_REQUEST: + return ResponseEntity.badRequest().build(); + default: + throw ex; + } + } + private void reloadFiltersAndHandleScriptException(String resolverResourceId) { try { metadataResolverService.reloadFilters(resolverResourceId); @@ -182,37 +184,65 @@ private void reloadFiltersAndHandleScriptException(String resolverResourceId) { } } - private MetadataResolver findResolverOrThrowHttp404(String resolverResourceId) { - MetadataResolver resolver = repository.findByResourceId(resolverResourceId); - if (resolver == null) { - throw HTTP_404_CLIENT_ERROR_EXCEPTION.get(); + @PutMapping("/Filters/{resourceId}") + @Transactional + public ResponseEntity update(@PathVariable String metadataResolverId, + @PathVariable String resourceId, + @RequestBody MetadataFilter updatedFilter) { + + MetadataResolver metadataResolver = findResolverOrThrowHttp404(metadataResolverId); + + //Now we operate directly on the filter attached to MetadataResolver, + //Instead of fetching filter separately, to accommodate correct envers versioning with uni-directional one-to-many + Optional filterTobeUpdatedOptional = metadataResolver.getMetadataFilters() + .stream() + .filter(it -> it.getResourceId().equals(resourceId)) + .findFirst(); + if (!filterTobeUpdatedOptional.isPresent()) { + return ResponseEntity.notFound().build(); + } + MetadataFilter filterTobeUpdated = filterTobeUpdatedOptional.get(); + if (!resourceId.equals(updatedFilter.getResourceId())) { + return new ResponseEntity(HttpStatus.CONFLICT); } - return resolver; - } - private MetadataFilter findFilterOrThrowHttp404(String filterResourceId) { - MetadataFilter filter = filterRepository.findByResourceId(filterResourceId); - if (filter == null) { - throw HTTP_404_CLIENT_ERROR_EXCEPTION.get(); + // Verify we're the only one attempting to update the filter + if (updatedFilter.getVersion() != filterTobeUpdated.getVersion()) { + return new ResponseEntity(HttpStatus.CONFLICT); } - return filter; - } - private MetadataFilter newlyPersistedFilter(Stream filters, final String filterResourceId) { - MetadataFilter persistedFilter = filters - .filter(f -> f.getResourceId().equals(filterResourceId)) - .collect(toList()).get(0); + // perform validation if necessary on the entity ids (if the filter is the right configuration to need such a check) + validateFilterOrThrowHttp400(updatedFilter); - return persistedFilter; + filterTobeUpdated.setName(updatedFilter.getName()); + filterTobeUpdated.setFilterEnabled(updatedFilter.isFilterEnabled()); + updatedFilter.updateConcreteFilterTypeData(filterTobeUpdated); + + MetadataFilter persistedFilter = filterRepository.save(filterTobeUpdated); + + //To support envers versioning from MetadataResolver side + metadataResolver.markAsModified(); + repository.save(metadataResolver); + + // TODO: do we need to reload filters here? + reloadFiltersAndHandleScriptException(metadataResolver.getResourceId()); + + return ResponseEntity.ok().body(persistedFilter); } - private static URI getResourceUriFor(MetadataResolver mr, String filterResourceId) { - return ServletUriComponentsBuilder - .fromCurrentServletMapping().path("/api/MetadataResolvers/") - .pathSegment(mr.getResourceId()) - .pathSegment("Filters") - .pathSegment(filterResourceId) - .build() - .toUri(); + /** + * IF the filter is of type "EntityAttributes" AND the target is "ENTITY" THEN check each of the values (which are entityIds) + */ + private void validateFilterOrThrowHttp400(MetadataFilter createdFilter) { + if ("EntityAttributes".equals(createdFilter.getType())) { + EntityAttributesFilter filter = (EntityAttributesFilter) createdFilter; + if ("ENTITY".equals(filter.getEntityAttributesFilterTarget().getEntityAttributesFilterTargetType())) { + for (String entityId : filter.getEntityAttributesFilterTarget().getValue()) { + if (!groupService.doesStringMatchGroupPattern(userService.getCurrentUser().getGroupId(), entityId)) { + throw HTTP_400_BAD_REQUEST_EXCEPTION.get(); + } + } + } + } } -} +} \ No newline at end of file diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/resolvers/validator/DynamicHttpMetadataResolverValidator.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/resolvers/validator/DynamicHttpMetadataResolverValidator.java index a161ec2fa..2b14ded1e 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/resolvers/validator/DynamicHttpMetadataResolverValidator.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/resolvers/validator/DynamicHttpMetadataResolverValidator.java @@ -7,9 +7,11 @@ import org.springframework.beans.factory.annotation.Autowired; public class DynamicHttpMetadataResolverValidator implements IMetadataResolverValidator { - @Autowired IGroupService groupService; + @Autowired + private IGroupService groupService; - @Autowired UserService userService; + @Autowired + private UserService userService; public DynamicHttpMetadataResolverValidator(IGroupService groupService, UserService userService) { this.groupService = groupService; @@ -22,7 +24,7 @@ public DynamicHttpMetadataResolverValidator(IGroupService groupService, UserServ DynamicHttpMetadataResolver dynamicResolver = (DynamicHttpMetadataResolver) resolver; if ("MetadataQueryProtocol".equals(dynamicResolver.getMetadataRequestURLConstructionScheme().getType())) { String url = dynamicResolver.getMetadataRequestURLConstructionScheme().getContent(); - if (!groupService.doesUrlMatchGroupPattern(userService.getCurrentUser().getGroupId(), url)) { + if (!groupService.doesStringMatchGroupPattern(userService.getCurrentUser().getGroupId(), url)) { return new ValidationResult("Metadata Query Protocol URL not acceptable for user's group"); } } diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/resolvers/validator/FileBackedHttpMetadataResolverValidator.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/resolvers/validator/FileBackedHttpMetadataResolverValidator.java index 19d165859..e387e9bea 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/resolvers/validator/FileBackedHttpMetadataResolverValidator.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/resolvers/validator/FileBackedHttpMetadataResolverValidator.java @@ -2,17 +2,16 @@ import edu.internet2.tier.shibboleth.admin.ui.domain.resolvers.FileBackedHttpMetadataResolver; import edu.internet2.tier.shibboleth.admin.ui.domain.resolvers.MetadataResolver; -import edu.internet2.tier.shibboleth.admin.ui.domain.resolvers.ResourceBackedMetadataResolver; import edu.internet2.tier.shibboleth.admin.ui.security.service.IGroupService; import edu.internet2.tier.shibboleth.admin.ui.security.service.UserService; import org.springframework.beans.factory.annotation.Autowired; public class FileBackedHttpMetadataResolverValidator implements IMetadataResolverValidator { @Autowired - IGroupService groupService; + private IGroupService groupService; @Autowired - UserService userService; + private UserService userService; public FileBackedHttpMetadataResolverValidator(IGroupService groupService, UserService userService) { this.groupService = groupService; @@ -24,7 +23,7 @@ public FileBackedHttpMetadataResolverValidator(IGroupService groupService, UserS @Override public ValidationResult validate(MetadataResolver resolver) { FileBackedHttpMetadataResolver fbhmResolver = (FileBackedHttpMetadataResolver) resolver; String url = fbhmResolver.getMetadataURL(); - if (!groupService.doesUrlMatchGroupPattern(userService.getCurrentUser().getGroupId(), url)) { + if (!groupService.doesStringMatchGroupPattern(userService.getCurrentUser().getGroupId(), url)) { return new ValidationResult("Metadata URL not acceptable for user's group"); } return new ValidationResult(); diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceImpl.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceImpl.java index bcca4c088..4749a5401 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceImpl.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceImpl.java @@ -61,7 +61,7 @@ public void deleteDefinition(String resourceId) throws EntityNotFoundException, * Designed usage is that this would be a URL or an entity Id (which is a URI that does not have to follow the URL conventions) */ @Override - public boolean doesUrlMatchGroupPattern(String groupId, String uri) { + public boolean doesStringMatchGroupPattern(String groupId, String uri) { Group group = find(groupId); return Pattern.matches(group.getValidationRegex(), uri); } diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/IGroupService.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/IGroupService.java index d95415127..d6e44e5ec 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/IGroupService.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/IGroupService.java @@ -23,5 +23,5 @@ public interface IGroupService { Group updateGroup(Group g) throws EntityNotFoundException, InvalidGroupRegexException; - boolean doesUrlMatchGroupPattern(String groupId, String uri); + boolean doesStringMatchGroupPattern(String groupId, String uri); } \ No newline at end of file diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/JPAEntityDescriptorServiceImpl.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/JPAEntityDescriptorServiceImpl.java index 84c956d93..f6a979e2d 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/JPAEntityDescriptorServiceImpl.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/JPAEntityDescriptorServiceImpl.java @@ -399,14 +399,14 @@ public void updateDescriptorFromRepresentation(org.opensaml.saml.saml2.metadata. private void validateEntityIdAndACSUrls(EntityDescriptorRepresentation edRep) throws InvalidUrlMatchException { // Check the entity id first - if (!groupService.doesUrlMatchGroupPattern(edRep.getIdOfOwner(), edRep.getEntityId())) { + if (!groupService.doesStringMatchGroupPattern(edRep.getIdOfOwner(), edRep.getEntityId())) { throw new InvalidUrlMatchException("EntityId is not a pattern match to the group"); } // Check the ACS locations if (edRep.getAssertionConsumerServices() != null && edRep.getAssertionConsumerServices().size() > 0) { for (AssertionConsumerServiceRepresentation acs : edRep.getAssertionConsumerServices()) { - if (!groupService.doesUrlMatchGroupPattern(edRep.getIdOfOwner(), acs.getLocationUrl())) { + if (!groupService.doesStringMatchGroupPattern(edRep.getIdOfOwner(), acs.getLocationUrl())) { throw new InvalidUrlMatchException( "ACS location [ " + acs.getLocationUrl() + " ] is not a pattern match to the group"); } 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 e223e0012..41ed176ad 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 @@ -20,6 +20,7 @@ import org.springframework.boot.test.context.SpringBootTest import org.springframework.boot.test.context.TestConfiguration import org.springframework.boot.test.web.client.TestRestTemplate import org.springframework.context.annotation.Bean +import org.springframework.context.annotation.Profile import org.springframework.http.HttpEntity import org.springframework.http.HttpHeaders import org.springframework.test.context.ActiveProfiles @@ -32,7 +33,7 @@ import static org.springframework.http.HttpMethod.PUT * @author Dmitriy Kopylenko */ @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) -@ActiveProfiles("no-auth") +@ActiveProfiles(["no-auth", "mfci-test"]) class MetadataFiltersControllerIntegrationTests extends Specification { @Autowired @@ -249,7 +250,8 @@ class MetadataFiltersControllerIntegrationTests extends Specification { } @TestConfiguration - static class Config { + @Profile("mfci-test") + static class LocalConfig { @Bean MetadataResolver metadataResolver() { new OpenSamlChainingMetadataResolver().with { @@ -259,4 +261,4 @@ class MetadataFiltersControllerIntegrationTests extends Specification { } } } -} +} \ No newline at end of file