diff --git a/backend/src/main/groovy/edu/internet2/tier/shibboleth/admin/ui/jsonschema/RelyingPartyOverridesJsonSchemaValidatingControllerAdvice.groovy b/backend/src/main/groovy/edu/internet2/tier/shibboleth/admin/ui/jsonschema/RelyingPartyOverridesJsonSchemaValidatingControllerAdvice.groovy index 934d7b1a7..e77097af6 100644 --- a/backend/src/main/groovy/edu/internet2/tier/shibboleth/admin/ui/jsonschema/RelyingPartyOverridesJsonSchemaValidatingControllerAdvice.groovy +++ b/backend/src/main/groovy/edu/internet2/tier/shibboleth/admin/ui/jsonschema/RelyingPartyOverridesJsonSchemaValidatingControllerAdvice.groovy @@ -1,5 +1,6 @@ package edu.internet2.tier.shibboleth.admin.ui.jsonschema +import edu.internet2.tier.shibboleth.admin.ui.controller.ErrorResponse import edu.internet2.tier.shibboleth.admin.ui.domain.frontend.EntityDescriptorRepresentation import mjson.Json import org.springframework.beans.factory.annotation.Autowired @@ -10,7 +11,6 @@ import org.springframework.http.ResponseEntity import org.springframework.http.converter.HttpMessageConverter import org.springframework.web.bind.annotation.ControllerAdvice import org.springframework.web.bind.annotation.ExceptionHandler -import org.springframework.web.context.request.WebRequest import org.springframework.web.servlet.mvc.method.annotation.RequestBodyAdviceAdapter import javax.annotation.PostConstruct @@ -56,8 +56,8 @@ class RelyingPartyOverridesJsonSchemaValidatingControllerAdvice extends RequestB } @ExceptionHandler(JsonSchemaValidationFailedException) - final ResponseEntity handleUserNotFoundException(JsonSchemaValidationFailedException ex, WebRequest request) { - new ResponseEntity<>([errors: ex.errors], HttpStatus.BAD_REQUEST) + final ResponseEntity handleJsonSchemaValidationFailedException(JsonSchemaValidationFailedException ex) { + ResponseEntity.status(HttpStatus.BAD_REQUEST).body(new ErrorResponse("400", String.join('\n', ex.errors))) } @PostConstruct diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/EntityDescriptorController.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/EntityDescriptorController.java index afde9d244..c3e76983e 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/EntityDescriptorController.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/EntityDescriptorController.java @@ -160,7 +160,7 @@ private ResponseEntity existingEntityDescriptorCheck(String entityId) { return ResponseEntity .status(HttpStatus.CONFLICT) .headers(headers) - .body(String.format("The entity descriptor with entity id [%s] already exists.", entityId)); + .body(new ErrorResponse(String.valueOf(HttpStatus.CONFLICT.value()), String.format("The entity descriptor with entity id [%s] already exists.", entityId))); } //No existing entity descriptor, which is an OK condition indicated by returning a null conflict response return null; 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..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,32 +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); - - //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); - } + 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) { @@ -143,24 +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); - - 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); - } + doResolverInitialization(persistedResolver); return ResponseEntity.ok(persistedResolver); } @@ -168,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; @@ -181,4 +165,17 @@ private static URI getResourceUriFor(MetadataResolver resolver) { .build() .toUri(); } + + 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) { + throw new MetadataFileNotFoundException("message.file-doesnt-exist"); + } + OpenSamlChainingMetadataResolverUtil.updateChainingMetadataResolver((OpenSamlChainingMetadataResolver) chainingMetadataResolver, openSamlRepresentation); + } + } } 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 6a035d9e2..e210adc94 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.hibernate.exception.ConstraintViolationException; @@ -12,6 +12,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.BAD_REQUEST; import static org.springframework.http.HttpStatus.NOT_FOUND; @@ -53,4 +54,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(), diff --git a/backend/src/main/resources/i18n/messages.properties b/backend/src/main/resources/i18n/messages.properties index e425206a9..25e82fa86 100644 --- a/backend/src/main/resources/i18n/messages.properties +++ b/backend/src/main/resources/i18n/messages.properties @@ -398,6 +398,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. message.database-constraint=There was a database constraint problem processing the request. Check the request to ensure that fields that must be unique are truly unique. tooltip.entity-id=Entity ID diff --git a/backend/src/main/resources/i18n/messages_en.properties b/backend/src/main/resources/i18n/messages_en.properties index 441077024..7d20536d0 100644 --- a/backend/src/main/resources/i18n/messages_en.properties +++ b/backend/src/main/resources/i18n/messages_en.properties @@ -404,6 +404,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. message.database-constraint=There was a database constraint problem processing the request. Check the request to ensure that fields that must be unique are truly unique. tooltip.entity-id=Entity ID diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/EntityDescriptorControllerTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/EntityDescriptorControllerTests.groovy index 122a349ae..b38941b74 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/EntityDescriptorControllerTests.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/EntityDescriptorControllerTests.groovy @@ -505,7 +505,7 @@ class EntityDescriptorControllerTests extends Specification { then: result.andExpect(status().isConflict()) - .andExpect(content().string("The entity descriptor with entity id [http://test.scaldingspoon.org/test1] already exists.")) + .andExpect(content().string("{\"errorCode\":\"409\",\"errorMessage\":\"The entity descriptor with entity id [http://test.scaldingspoon.org/test1] already exists.\"}")) } def "POST /EntityDescriptor handles x-www-form-urlencoded happily"() { diff --git a/ui/src/app/metadata/domain/service/resolver.service.ts b/ui/src/app/metadata/domain/service/resolver.service.ts index f0c9f4b3c..4012f9f23 100644 --- a/ui/src/app/metadata/domain/service/resolver.service.ts +++ b/ui/src/app/metadata/domain/service/resolver.service.ts @@ -45,7 +45,9 @@ export class ResolverService { return this.http.post(`${this.base}${this.endpoint}`, xml, { headers: new HttpHeaders().set('Content-Type', 'application/xml'), params: new HttpParams().set('spName', name) - }); + }).pipe(catchError(error => { + return throwError({ errorCode: error.status, errorMessage: `Unable to upload file ... ${error.error}` }); + })); } createFromUrl(name: string, url: string): Observable { @@ -53,7 +55,9 @@ export class ResolverService { return this.http.post(`${this.base}${this.endpoint}`, body, { headers: new HttpHeaders().set('Content-Type', 'application/x-www-form-urlencoded'), params: new HttpParams().set('spName', name) - }); + }).pipe(catchError(error => { + return throwError({ errorCode: error.status, errorMessage: `Unable to upload file ... ${error.error}` }); + })); } preview(id: string): Observable { diff --git a/ui/src/app/metadata/resolver/effect/collection.effects.ts b/ui/src/app/metadata/resolver/effect/collection.effects.ts index d56110a7e..46d2d0712 100644 --- a/ui/src/app/metadata/resolver/effect/collection.effects.ts +++ b/ui/src/app/metadata/resolver/effect/collection.effects.ts @@ -1,15 +1,21 @@ import { Injectable } from '@angular/core'; import { Effect, Actions, ofType } from '@ngrx/effects'; import { Router } from '@angular/router'; - +import { Store } from '@ngrx/store'; import { of } from 'rxjs'; -import { map, catchError, switchMap, tap } from 'rxjs/operators'; +import { map, catchError, switchMap, tap, withLatestFrom } from 'rxjs/operators'; import * as providerActions from '../action/collection.action'; import * as draftActions from '../action/draft.action'; import { ResolverCollectionActionTypes } from '../action/collection.action'; import { ResolverService } from '../../domain/service/resolver.service'; import { removeNulls } from '../../../shared/util'; +import { AddNotification } from '../../../notification/action/notification.action'; +import { Notification, NotificationType } from '../../../notification/model/notification'; +import { I18nService } from '../../../i18n/service/i18n.service'; +import * as fromRoot from '../../../app.reducer'; +import * as fromI18n from '../../../i18n/reducer'; + /* istanbul ignore next */ @Injectable() @@ -112,11 +118,24 @@ export class ResolverCollectionEffects { ofType(ResolverCollectionActionTypes.ADD_RESOLVER_SUCCESS), map(action => action.payload), map(provider => { - console.log(provider); return new draftActions.RemoveDraftRequest(provider); }) ); + @Effect() + addResolverFailNotification$ = this.actions$.pipe( + ofType(ResolverCollectionActionTypes.ADD_RESOLVER_FAIL), + map(action => action.payload), + withLatestFrom(this.store.select(fromI18n.getMessages)), + map(([error, messages]) => new AddNotification( + new Notification( + NotificationType.Danger, + `${error.errorCode}: ${this.i18nService.translate(error.errorMessage, null, messages)}`, + 8000 + ) + )) + ); + @Effect() uploadResolverRequest$ = this.actions$.pipe( ofType(ResolverCollectionActionTypes.UPLOAD_RESOLVER_REQUEST), @@ -126,7 +145,7 @@ export class ResolverCollectionEffects { .upload(file.name, file.body) .pipe( map(p => new providerActions.AddResolverSuccess(p)), - catchError(() => of(new providerActions.AddResolverFail(file))) + catchError((error) => of(new providerActions.AddResolverFail(error))) ) ) ); @@ -140,7 +159,7 @@ export class ResolverCollectionEffects { .createFromUrl(file.name, file.url) .pipe( map(p => new providerActions.AddResolverSuccess(p)), - catchError(() => of(new providerActions.AddResolverFail(file))) + catchError((error) => of(new providerActions.AddResolverFail(error))) ) ) ); @@ -148,6 +167,8 @@ export class ResolverCollectionEffects { constructor( private descriptorService: ResolverService, private actions$: Actions, - private router: Router + private router: Router, + private store: Store, + private i18nService: I18nService ) { } } /* istanbul ignore next */