From a350d33cae6a01d9984d652774a935dd6b920ceb Mon Sep 17 00:00:00 2001 From: Bill Smith Date: Mon, 3 Dec 2018 14:29:34 -0700 Subject: [PATCH 01/10] [SHIBUI-1001] Modified resolver initialization code to move shared code into a method. PUT and POST now both behave the same way with the same error message when there is a file not found exception. --- .../MetadataResolversController.java | 34 +++++++++++-------- .../main/resources/i18n/messages.properties | 1 + .../resources/i18n/messages_en.properties | 1 + 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataResolversController.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataResolversController.java index 4593d8bbf..59fed8e93 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataResolversController.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataResolversController.java @@ -119,11 +119,9 @@ public ResponseEntity create(@RequestBody MetadataResolver newResolver) throw MetadataResolver persistedResolver = resolverRepository.save(newResolver); positionOrderContainerService.appendPositionOrderForNew(persistedResolver); - //TODO: currently, the update call might explode, but the save works.. in which case, the UI never gets - // n valid response. This operation is not atomic. Should we return an error here? - if (persistedResolver.getDoInitialization()) { - org.opensaml.saml.metadata.resolver.MetadataResolver openSamlRepresentation = metadataResolverConverterService.convertToOpenSamlRepresentation(persistedResolver); - OpenSamlChainingMetadataResolverUtil.updateChainingMetadataResolver((OpenSamlChainingMetadataResolver) chainingMetadataResolver, openSamlRepresentation); + ResponseEntity initializationError = doResolverInitialization(persistedResolver); + if (initializationError != null) { + return initializationError; } return ResponseEntity.created(getResourceUriFor(persistedResolver)).body(persistedResolver); @@ -151,15 +149,9 @@ public ResponseEntity update(@PathVariable String resourceId, @RequestBody Me MetadataResolver persistedResolver = resolverRepository.save(updatedResolver); - if (persistedResolver.getDoInitialization()) { - org.opensaml.saml.metadata.resolver.MetadataResolver openSamlRepresentation = null; - try { - openSamlRepresentation = metadataResolverConverterService.convertToOpenSamlRepresentation(persistedResolver); - } catch (FileNotFoundException e) { - return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) - .body(new ErrorResponse(HttpStatus.INTERNAL_SERVER_ERROR.toString(), "label.file-doesnt-exist")); - } - OpenSamlChainingMetadataResolverUtil.updateChainingMetadataResolver((OpenSamlChainingMetadataResolver) chainingMetadataResolver, openSamlRepresentation); + ResponseEntity initializationError = doResolverInitialization(persistedResolver); + if (initializationError != null) { + return initializationError; } return ResponseEntity.ok(persistedResolver); @@ -181,4 +173,18 @@ private static URI getResourceUriFor(MetadataResolver resolver) { .build() .toUri(); } + + private ResponseEntity doResolverInitialization(MetadataResolver persistedResolver) throws ComponentInitializationException, ResolverException, IOException { + if (persistedResolver.getDoInitialization()) { + org.opensaml.saml.metadata.resolver.MetadataResolver openSamlRepresentation = null; + try { + openSamlRepresentation = metadataResolverConverterService.convertToOpenSamlRepresentation(persistedResolver); + } catch (FileNotFoundException e) { + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) + .body(new ErrorResponse(HttpStatus.INTERNAL_SERVER_ERROR.toString(), "message.file-doesnt-exist")); + } + OpenSamlChainingMetadataResolverUtil.updateChainingMetadataResolver((OpenSamlChainingMetadataResolver) chainingMetadataResolver, openSamlRepresentation); + } + return null; + } } diff --git a/backend/src/main/resources/i18n/messages.properties b/backend/src/main/resources/i18n/messages.properties index 31c4de9b8..672ae51f4 100644 --- a/backend/src/main/resources/i18n/messages.properties +++ b/backend/src/main/resources/i18n/messages.properties @@ -397,6 +397,7 @@ message.wizard-status=Step { index } of { length } message.entity-id-min-unique=You must add at least one entity id target and they must each be unique. message.required-for-scripts=Required for Scripts message.required-for-regex=Required for Regex +message.file-doesnt-exist=The requested file to be processed does not exist on the server. tooltip.entity-id=Entity ID tooltip.service-provider-name=Service Provider Name (Dashboard Display Only) diff --git a/backend/src/main/resources/i18n/messages_en.properties b/backend/src/main/resources/i18n/messages_en.properties index 01b5c18e2..c5eabc3ee 100644 --- a/backend/src/main/resources/i18n/messages_en.properties +++ b/backend/src/main/resources/i18n/messages_en.properties @@ -403,6 +403,7 @@ message.wizard-status=Step { index } of { length } message.entity-id-min-unique=You must add at least one entity id target and they must each be unique. message.required-for-scripts=Required for Scripts message.required-for-regex=Required for Regex +message.file-doesnt-exist=The requested file to be processed does not exist on the server. tooltip.entity-id=Entity ID tooltip.service-provider-name=Service Provider Name (Dashboard Display Only) From 3f14ef499128a49ecd933722b78b43c3680a344c Mon Sep 17 00:00:00 2001 From: Dmitriy Kopylenko Date: Wed, 5 Dec 2018 08:21:33 -0500 Subject: [PATCH 02/10] SHIBUI-926 --- .../MetadataResolversController.java | 31 +++++++------------ .../support/RestControllersSupport.java | 9 +++++- .../MetadataFileNotFoundException.java | 14 +++++++++ .../MetadataResolverConverterServiceImpl.java | 2 +- 4 files changed, 34 insertions(+), 22 deletions(-) create mode 100644 backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/exceptions/MetadataFileNotFoundException.java diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataResolversController.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataResolversController.java index 59fed8e93..b9a7ab3f0 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataResolversController.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataResolversController.java @@ -1,6 +1,7 @@ package edu.internet2.tier.shibboleth.admin.ui.controller; import com.fasterxml.jackson.databind.exc.InvalidTypeIdException; +import edu.internet2.tier.shibboleth.admin.ui.domain.exceptions.MetadataFileNotFoundException; import edu.internet2.tier.shibboleth.admin.ui.domain.resolvers.MetadataResolver; import edu.internet2.tier.shibboleth.admin.ui.domain.resolvers.MetadataResolverValidationService; import edu.internet2.tier.shibboleth.admin.ui.domain.resolvers.opensaml.OpenSamlChainingMetadataResolver; @@ -105,30 +106,26 @@ public ResponseEntity getOne(@PathVariable String resourceId) { } @PostMapping("/MetadataResolvers") - @Transactional + @Transactional(rollbackFor = Exception.class) public ResponseEntity create(@RequestBody MetadataResolver newResolver) throws IOException, ResolverException, ComponentInitializationException { if (resolverRepository.findByName(newResolver.getName()) != null) { return ResponseEntity.status(HttpStatus.CONFLICT).build(); } ResponseEntity validationErrorResponse = validate(newResolver); - if(validationErrorResponse != null) { + if (validationErrorResponse != null) { return validationErrorResponse; } MetadataResolver persistedResolver = resolverRepository.save(newResolver); positionOrderContainerService.appendPositionOrderForNew(persistedResolver); - - ResponseEntity initializationError = doResolverInitialization(persistedResolver); - if (initializationError != null) { - return initializationError; - } + doResolverInitialization(persistedResolver); return ResponseEntity.created(getResourceUriFor(persistedResolver)).body(persistedResolver); } @PutMapping("/MetadataResolvers/{resourceId}") - @Transactional + @Transactional(rollbackFor = Exception.class) public ResponseEntity update(@PathVariable String resourceId, @RequestBody MetadataResolver updatedResolver) throws IOException, ResolverException, ComponentInitializationException { MetadataResolver existingResolver = resolverRepository.findByResourceId(resourceId); if (existingResolver == null) { @@ -141,18 +138,13 @@ public ResponseEntity update(@PathVariable String resourceId, @RequestBody Me } ResponseEntity validationErrorResponse = validate(updatedResolver); - if(validationErrorResponse != null) { + if (validationErrorResponse != null) { return validationErrorResponse; } updatedResolver.setAudId(existingResolver.getAudId()); - MetadataResolver persistedResolver = resolverRepository.save(updatedResolver); - - ResponseEntity initializationError = doResolverInitialization(persistedResolver); - if (initializationError != null) { - return initializationError; - } + doResolverInitialization(persistedResolver); return ResponseEntity.ok(persistedResolver); } @@ -160,7 +152,7 @@ public ResponseEntity update(@PathVariable String resourceId, @RequestBody Me @SuppressWarnings("Unchecked") private ResponseEntity validate(MetadataResolver metadataResolver) { ValidationResult validationResult = metadataResolverValidationService.validateIfNecessary(metadataResolver); - if(!validationResult.isValid()) { + if (!validationResult.isValid()) { return ResponseEntity.badRequest().body(validationResult.getErrorMessage()); } return null; @@ -174,17 +166,16 @@ private static URI getResourceUriFor(MetadataResolver resolver) { .toUri(); } - private ResponseEntity doResolverInitialization(MetadataResolver persistedResolver) throws ComponentInitializationException, ResolverException, IOException { + private void doResolverInitialization(MetadataResolver persistedResolver) throws + ComponentInitializationException, ResolverException, IOException { if (persistedResolver.getDoInitialization()) { org.opensaml.saml.metadata.resolver.MetadataResolver openSamlRepresentation = null; try { openSamlRepresentation = metadataResolverConverterService.convertToOpenSamlRepresentation(persistedResolver); } catch (FileNotFoundException e) { - return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) - .body(new ErrorResponse(HttpStatus.INTERNAL_SERVER_ERROR.toString(), "message.file-doesnt-exist")); + throw new MetadataFileNotFoundException("message.file-doesnt-exist"); } OpenSamlChainingMetadataResolverUtil.updateChainingMetadataResolver((OpenSamlChainingMetadataResolver) chainingMetadataResolver, openSamlRepresentation); } - return null; } } diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/support/RestControllersSupport.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/support/RestControllersSupport.java index 7dc1a9abe..1adb07d6d 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/support/RestControllersSupport.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/support/RestControllersSupport.java @@ -1,7 +1,7 @@ package edu.internet2.tier.shibboleth.admin.ui.controller.support; -import com.google.common.collect.ImmutableMap; import edu.internet2.tier.shibboleth.admin.ui.controller.ErrorResponse; +import edu.internet2.tier.shibboleth.admin.ui.domain.exceptions.MetadataFileNotFoundException; import edu.internet2.tier.shibboleth.admin.ui.domain.resolvers.MetadataResolver; import edu.internet2.tier.shibboleth.admin.ui.repository.MetadataResolverRepository; import org.springframework.beans.factory.annotation.Autowired; @@ -11,6 +11,7 @@ import org.springframework.web.bind.annotation.RestControllerAdvice; import org.springframework.web.client.HttpClientErrorException; +import static org.springframework.http.HttpStatus.INTERNAL_SERVER_ERROR; import static org.springframework.http.HttpStatus.NOT_FOUND; /** @@ -46,4 +47,10 @@ public final ResponseEntity handleAllOtherExceptions(Exception ex ErrorResponse errorResponse = new ErrorResponse("400", ex.getLocalizedMessage()); return new ResponseEntity<>(errorResponse, HttpStatus.BAD_REQUEST); } + + @ExceptionHandler(MetadataFileNotFoundException.class) + public final ResponseEntity metadataFileNotFoundHandler(MetadataFileNotFoundException ex) { + ErrorResponse errorResponse = new ErrorResponse(INTERNAL_SERVER_ERROR.toString(), ex.getLocalizedMessage()); + return new ResponseEntity<>(errorResponse, INTERNAL_SERVER_ERROR); + } } diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/exceptions/MetadataFileNotFoundException.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/exceptions/MetadataFileNotFoundException.java new file mode 100644 index 000000000..bc3dd1493 --- /dev/null +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/exceptions/MetadataFileNotFoundException.java @@ -0,0 +1,14 @@ +package edu.internet2.tier.shibboleth.admin.ui.domain.exceptions; + +/** + * Indicates that provided metadata file is not found. Thrown during opensaml API initialization code path, if there is + * such. Throwing such exception is useful in @Transactional context for atomic transaction rollbacks, etc. + * + * @author Dmitriy Kopylenko + */ +public class MetadataFileNotFoundException extends RuntimeException { + + public MetadataFileNotFoundException(String message) { + super(message); + } +} diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/MetadataResolverConverterServiceImpl.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/MetadataResolverConverterServiceImpl.java index ccb77164c..8088793a9 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/MetadataResolverConverterServiceImpl.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/MetadataResolverConverterServiceImpl.java @@ -64,7 +64,7 @@ private OpenSamlFilesystemMetadataResolver convertToOpenSamlRepresentation(Files IndexWriter indexWriter = indexWriterService.getIndexWriter(resolver.getResourceId()); File metadataFile = new File(resolver.getMetadataFile()); if (resolver.getDoInitialization() && !metadataFile.exists()) { - throw new FileNotFoundException("No file was found on the fileysystem for provided filename: " + resolver.getMetadataFile()); + throw new FileNotFoundException("No file was found on the file system for provided filename: " + resolver.getMetadataFile()); } OpenSamlFilesystemMetadataResolver openSamlResolver = new OpenSamlFilesystemMetadataResolver(openSamlObjects.getParserPool(), From 0c1304a8d16ed17f21ad5dcad6aa1697d0c8ca68 Mon Sep 17 00:00:00 2001 From: Jodie Muramoto Date: Thu, 6 Dec 2018 10:50:32 -0700 Subject: [PATCH 03/10] SHIBUI-1010: Removed redundant ARIA roles and attributes (if button is semantic, does not need the role of button, etc.); --- .../resources/local-dynamic-metadata-provider.schema.json | 2 +- ui/src/app/app.component.html | 3 +-- .../domain/component/wizard-summary.component.html | 6 +++--- .../metadata/filter/container/edit-filter.component.html | 2 +- .../metadata/filter/container/new-filter.component.html | 2 +- .../app/metadata/manager/container/manager.component.html | 2 +- .../provider/container/provider-wizard.component.html | 5 +++-- .../resolver/container/blank-resolver.component.html | 2 +- .../resolver/container/confirm-copy.component.html | 6 +++--- .../resolver/container/copy-resolver.component.html | 3 +-- .../resolver/container/new-resolver.component.html | 2 +- .../resolver/container/upload-resolver.component.html | 2 +- ui/src/app/shared/autocomplete/autocomplete.component.html | 1 - ui/src/app/shared/component/info-icon.component.html | 2 -- ui/src/app/wizard/component/wizard.component.html | 7 ++----- 15 files changed, 20 insertions(+), 27 deletions(-) diff --git a/backend/src/main/resources/local-dynamic-metadata-provider.schema.json b/backend/src/main/resources/local-dynamic-metadata-provider.schema.json index bb98b8b67..c76434258 100644 --- a/backend/src/main/resources/local-dynamic-metadata-provider.schema.json +++ b/backend/src/main/resources/local-dynamic-metadata-provider.schema.json @@ -9,7 +9,7 @@ "properties": { "name": { "title": "label.metadata-provider-name-display-only", - "description": "tooltip.metadata-provider-name", + "description": "tooltip.metadata-provider-name-display-only", "type": "string", "widget": { "id": "string", diff --git a/ui/src/app/app.component.html b/ui/src/app/app.component.html index f09ae298a..b7adb997b 100644 --- a/ui/src/app/app.component.html +++ b/ui/src/app/app.component.html @@ -14,7 +14,6 @@ - + diff --git a/ui/src/app/metadata/filter/container/edit-filter.component.html b/ui/src/app/metadata/filter/container/edit-filter.component.html index e42137ebf..a7771dbef 100644 --- a/ui/src/app/metadata/filter/container/edit-filter.component.html +++ b/ui/src/app/metadata/filter/container/edit-filter.component.html @@ -1,4 +1,4 @@ -
+
diff --git a/ui/src/app/metadata/filter/container/new-filter.component.html b/ui/src/app/metadata/filter/container/new-filter.component.html index e56dc4634..08633e938 100644 --- a/ui/src/app/metadata/filter/container/new-filter.component.html +++ b/ui/src/app/metadata/filter/container/new-filter.component.html @@ -1,4 +1,4 @@ -
+
diff --git a/ui/src/app/metadata/manager/container/manager.component.html b/ui/src/app/metadata/manager/container/manager.component.html index 2c354d7e3..7803d47e5 100644 --- a/ui/src/app/metadata/manager/container/manager.component.html +++ b/ui/src/app/metadata/manager/container/manager.component.html @@ -1,4 +1,4 @@ -
+
  • Metadata Sources diff --git a/ui/src/app/metadata/provider/container/provider-wizard.component.html b/ui/src/app/metadata/provider/container/provider-wizard.component.html index b1027711b..366bb70d5 100644 --- a/ui/src/app/metadata/provider/container/provider-wizard.component.html +++ b/ui/src/app/metadata/provider/container/provider-wizard.component.html @@ -7,11 +7,12 @@
-
+
+ (onSave)="save()" + role="navigation">
diff --git a/ui/src/app/metadata/resolver/container/blank-resolver.component.html b/ui/src/app/metadata/resolver/container/blank-resolver.component.html index 5229f61f1..70ee5fa19 100644 --- a/ui/src/app/metadata/resolver/container/blank-resolver.component.html +++ b/ui/src/app/metadata/resolver/container/blank-resolver.component.html @@ -9,7 +9,7 @@

  • -
  • -
  • -
  • -
  • -