From 114120689afb0c308bc748949262ff6b73af6f3c Mon Sep 17 00:00:00 2001 From: Isto Nikula Date: Sat, 30 May 2020 15:51:09 +0300 Subject: [PATCH 1/3] feat(error-handling): use ResponseEntityExceptionHandler In addition, start moving towards more robust error handling --- TODO.md | 1 + .../main/kotlin/io/realworld/ErrorHandler.kt | 143 ++++++++++++------ .../main/kotlin/io/realworld/errors/Dtos.kt | 14 ++ .../main/kotlin/io/realworld/users/Routes.kt | 10 +- .../test/kotlin/io/realworld/ArticleTests.kt | 10 +- .../src/test/kotlin/io/realworld/TestUtils.kt | 8 + .../src/test/kotlin/io/realworld/UserTests.kt | 2 +- 7 files changed, 134 insertions(+), 54 deletions(-) create mode 100644 realworld-app/web/src/main/kotlin/io/realworld/errors/Dtos.kt diff --git a/TODO.md b/TODO.md index 9298037..7115bdb 100644 --- a/TODO.md +++ b/TODO.md @@ -8,4 +8,5 @@ ## Nice to have * applicative validator +* kotlin logging * audit logging, check https://github.com/logstash/logstash-logback-encoder diff --git a/realworld-app/web/src/main/kotlin/io/realworld/ErrorHandler.kt b/realworld-app/web/src/main/kotlin/io/realworld/ErrorHandler.kt index 5ac3f29..e9aa8d3 100644 --- a/realworld-app/web/src/main/kotlin/io/realworld/ErrorHandler.kt +++ b/realworld-app/web/src/main/kotlin/io/realworld/ErrorHandler.kt @@ -2,51 +2,126 @@ package io.realworld import com.fasterxml.jackson.databind.JsonMappingException import com.fasterxml.jackson.module.kotlin.MissingKotlinParameterException +import io.realworld.errors.ErrorResponseDto +import io.realworld.errors.SingleErrorDto import org.slf4j.Logger import org.slf4j.LoggerFactory import org.springframework.core.NestedExceptionUtils +import org.springframework.core.Ordered +import org.springframework.core.annotation.Order +import org.springframework.http.HttpHeaders import org.springframework.http.HttpStatus import org.springframework.http.ResponseEntity import org.springframework.http.converter.HttpMessageNotReadableException +import org.springframework.validation.FieldError +import org.springframework.validation.ObjectError import org.springframework.web.bind.MethodArgumentNotValidException import org.springframework.web.bind.annotation.ControllerAdvice import org.springframework.web.bind.annotation.ExceptionHandler +import org.springframework.web.context.request.ServletWebRequest +import org.springframework.web.context.request.WebRequest +import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler +import org.springframework.web.util.WebUtils +import java.lang.Exception +import javax.servlet.ServletException @ControllerAdvice -class ErrorHandler { - val log: Logger = LoggerFactory.getLogger(ErrorHandler::class.java) - - @ExceptionHandler(HttpMessageNotReadableException::class) - fun httpMessageNotReadable(ex: HttpMessageNotReadableException): ResponseEntity { - val t = NestedExceptionUtils.getMostSpecificCause(ex) - return when (t) { - is JsonMappingException -> { - handleError(HttpStatus.UNPROCESSABLE_ENTITY, listOf(t.toValidationError())) - } - else -> handleError(HttpStatus.UNPROCESSABLE_ENTITY, listOf(ValidationError( - type = "ValidationError", - path = "body", - message = t.message ?: "" - ))) +class RestExceptionHandler : ResponseEntityExceptionHandler() { + // thrown when an object fails the @Valid validation + override fun handleMethodArgumentNotValid( + ex: MethodArgumentNotValidException, + headers: HttpHeaders, + status: HttpStatus, + request: WebRequest + ): ResponseEntity { + val fieldErrors = ex.bindingResult.fieldErrors.map(FieldError::toErrorDto) + val globalErrors = ex.bindingResult.globalErrors.map(ObjectError::toErrorDto) + + val httpStatus = HttpStatus.UNPROCESSABLE_ENTITY + val servletWebRequest = request as ServletWebRequest + + val responseBody = ErrorResponseDto( + status = httpStatus.value(), + errorCode = "InvalidData", + message = "There are validation errors in the data", + path = servletWebRequest.request.servletPath, + errors = listOf(fieldErrors, globalErrors).flatten() + ) + return handleExceptionInternal(ex, responseBody, headers, httpStatus, request) + } + + // use ErrorResponseDto also for errors detected by Spring + override fun handleExceptionInternal( + ex: Exception, + body: Any?, + headers: HttpHeaders, + status: HttpStatus, + request: WebRequest + ): ResponseEntity { + if (HttpStatus.INTERNAL_SERVER_ERROR.equals(status)) { + request.setAttribute(WebUtils.ERROR_EXCEPTION_ATTRIBUTE, ex, WebRequest.SCOPE_REQUEST) } + + val errorCode = ex::class.simpleName ?: "Undefined" + val servletWebRequest = request as ServletWebRequest + val responseBody = body ?: ErrorResponseDto( + status = status.value(), + errorCode = errorCode, + message = ex.localizedMessage, + path = servletWebRequest.request.servletPath, + errors = listOf() + ) + // TODO log error + return ResponseEntity(responseBody, headers, status) } @ExceptionHandler(ForbiddenException::class) - fun forbidden() = handleError(HttpStatus.FORBIDDEN) + fun forbidden() = ResponseEntity(HttpStatus.FORBIDDEN) @ExceptionHandler(UnauthorizedException::class) - fun unauthorized() = handleError(HttpStatus.UNAUTHORIZED) + fun unauthorized() = ResponseEntity(HttpStatus.UNAUTHORIZED) - @ExceptionHandler(FieldError::class) - fun fieldError(ex: FieldError) = handleError(HttpStatus.UNPROCESSABLE_ENTITY, listOf(ex.toValidationError())) + @ExceptionHandler(MyFieldError::class) + fun fieldError(ex: MyFieldError) = handleError(HttpStatus.UNPROCESSABLE_ENTITY, listOf(ex.toValidationError())) - @ExceptionHandler(MethodArgumentNotValidException::class) - fun methodArgumentNotValid(ex: MethodArgumentNotValidException) = - handleError(HttpStatus.UNPROCESSABLE_ENTITY, ex.bindingResult.fieldErrors.map { - ValidationError(type = it.code ?: "", path = it.field, message = it.defaultMessage ?: "") - }) + // TODO find out why this overrides more specific errors + // @ExceptionHandler(Throwable::class) + fun handleAny(t: Throwable, request: WebRequest): ResponseEntity { + val cause = resolveCause(t) + val msg = "Unexpected error" + + return ResponseEntity( + ErrorResponseDto( + status = HttpStatus.INTERNAL_SERVER_ERROR.value(), + errorCode = "Undefined", + message = msg, + path = (request as ServletWebRequest).request.servletPath, + errors = listOf() + ), + HttpStatus.INTERNAL_SERVER_ERROR + ) + // TODO log error + } + + private fun resolveCause(t: Throwable): Throwable { + var resolved = t + while (resolved is ServletException && resolved.cause != null) { + resolved = resolved.cause!! + } + return resolved + } } +fun FieldError.toErrorDto() = SingleErrorDto( + message = defaultMessage ?: "Undefined error", + metadata = mapOf("path" to field) +) + +fun ObjectError.toErrorDto() = SingleErrorDto( + message = defaultMessage ?: "Undefined error", + metadata = mapOf("path" to objectName) +) + fun handleError( httpStatus: HttpStatus, validationErrors: List = emptyList() @@ -64,25 +139,7 @@ data class ValidationError( val arguments: Map? = emptyMap() ) -fun JsonMappingException.toValidationErrorPath(): String = - path.joinToString(separator = ".", transform = { it -> - val i = if (it.index >= 0) "${it.index}" else "" - "${it.fieldName ?: ""}$i" - }) - -fun JsonMappingException.toValidationError() = ValidationError( - type = "TypeMismatch", - path = toValidationErrorPath(), - message = message ?: "" -) - -fun MissingKotlinParameterException.toValidationError() = ValidationError( - type = "TypeMismatch", - path = toValidationErrorPath(), - message = message ?: "" -) - -class FieldError(val path: String, message: String) : Throwable(message) { +class MyFieldError(val path: String, message: String) : Throwable(message) { fun toValidationError() = ValidationError( type = "FieldError", path = path, diff --git a/realworld-app/web/src/main/kotlin/io/realworld/errors/Dtos.kt b/realworld-app/web/src/main/kotlin/io/realworld/errors/Dtos.kt new file mode 100644 index 0000000..6e7bba7 --- /dev/null +++ b/realworld-app/web/src/main/kotlin/io/realworld/errors/Dtos.kt @@ -0,0 +1,14 @@ +package io.realworld.errors + +import java.time.Instant + +data class SingleErrorDto(val message: String, val metadata: Any? = null) + +data class ErrorResponseDto( + val status: Int, + val errorCode: String, + val message: String, + val path: String, + val timestamp: Instant = Instant.now(), + val errors: List +) diff --git a/realworld-app/web/src/main/kotlin/io/realworld/users/Routes.kt b/realworld-app/web/src/main/kotlin/io/realworld/users/Routes.kt index 0c4e375..5de933e 100644 --- a/realworld-app/web/src/main/kotlin/io/realworld/users/Routes.kt +++ b/realworld-app/web/src/main/kotlin/io/realworld/users/Routes.kt @@ -1,7 +1,7 @@ package io.realworld.users import arrow.core.Option -import io.realworld.FieldError +import io.realworld.MyFieldError import io.realworld.UnauthorizedException import io.realworld.domain.common.Auth import io.realworld.domain.users.CreateUser @@ -71,9 +71,9 @@ class UserController( { when (it) { is UserRegistrationError.EmailAlreadyTaken -> - throw FieldError("email", "already taken") + throw MyFieldError("email", "already taken") is UserRegistrationError.UsernameAlreadyTaken -> - throw FieldError("username", "already taken") + throw MyFieldError("username", "already taken") } }, { ResponseEntity.status(HttpStatus.CREATED).body(UserResponse.fromDomain(it)) } @@ -122,9 +122,9 @@ class UserController( { when (it) { is UserUpdateError.EmailAlreadyTaken -> - throw FieldError("email", "already taken") + throw MyFieldError("email", "already taken") is UserUpdateError.UsernameAlreadyTaken -> - throw FieldError("username", "already taken") + throw MyFieldError("username", "already taken") } }, { ResponseEntity.ok(UserResponse.fromDomain(it)) } diff --git a/realworld-app/web/src/test/kotlin/io/realworld/ArticleTests.kt b/realworld-app/web/src/test/kotlin/io/realworld/ArticleTests.kt index 48a07bc..6e9bd9e 100644 --- a/realworld-app/web/src/test/kotlin/io/realworld/ArticleTests.kt +++ b/realworld-app/web/src/test/kotlin/io/realworld/ArticleTests.kt @@ -270,9 +270,9 @@ class ArticleTests { client.post("/api/articles", it) .then() .statusCode(422) - .body("errors.title.message", Matchers.equalTo("must not be blank")) - .body("errors.description.message", Matchers.equalTo("must not be blank")) - .body("errors.body.message", Matchers.equalTo("must not be blank")) + .verifyValidationError("title", "must not be blank") + .verifyValidationError("description", "must not be blank") + .verifyValidationError("body", "must not be blank") } listOf("title", "description", "body").map { prop -> @@ -281,8 +281,8 @@ class ArticleTests { }.toString().let { client.post("/api/articles", it) .then() - .statusCode(422) - .body("errors.$prop.type", Matchers.equalTo("TypeMismatch")) + .statusCode(400) + .body("errorCode", Matchers.equalTo("HttpMessageNotReadableException")) } } } diff --git a/realworld-app/web/src/test/kotlin/io/realworld/TestUtils.kt b/realworld-app/web/src/test/kotlin/io/realworld/TestUtils.kt index dae8d26..a8eb054 100644 --- a/realworld-app/web/src/test/kotlin/io/realworld/TestUtils.kt +++ b/realworld-app/web/src/test/kotlin/io/realworld/TestUtils.kt @@ -18,6 +18,7 @@ import io.restassured.http.ContentType import io.restassured.module.jsv.JsonSchemaValidator import io.restassured.response.ValidatableResponse import io.restassured.specification.RequestSpecification +import org.hamcrest.Matchers import java.util.UUID fun initSpec(port: Int) = RequestSpecBuilder() @@ -96,3 +97,10 @@ object Schemas { fun ValidatableResponse.verifyResponse(schema: String, statusCode: Int) = statusCode(statusCode) .body(JsonSchemaValidator.matchesJsonSchemaInClasspath(schema)) + +fun ValidatableResponse.verifyValidationError(path: String, message: String) = + body("errors", Matchers.hasItem(Matchers.equalTo(mapOf( + "message" to message, + "metadata" to mapOf("path" to path) + )))) + diff --git a/realworld-app/web/src/test/kotlin/io/realworld/UserTests.kt b/realworld-app/web/src/test/kotlin/io/realworld/UserTests.kt index 26f84ae..d6ce82f 100644 --- a/realworld-app/web/src/test/kotlin/io/realworld/UserTests.kt +++ b/realworld-app/web/src/test/kotlin/io/realworld/UserTests.kt @@ -142,7 +142,7 @@ class UserTests { pathToObject("user").remove("password") }) .then() - .statusCode(422) + .statusCode(400) } @Test From f801c37e13846ccd9b7ce9e2896b138c46463692 Mon Sep 17 00:00:00 2001 From: Isto Nikula Date: Sat, 30 May 2020 17:06:41 +0300 Subject: [PATCH 2/3] feat(error-handling): introduce domain error --- .../main/kotlin/io/realworld/Application.kt | 15 +++- .../main/kotlin/io/realworld/ErrorHandler.kt | 74 +++++++++---------- .../web/src/main/kotlin/io/realworld/Jwt.kt | 15 +--- .../kotlin/io/realworld/articles/Routes.kt | 26 +------ .../io/realworld/errors/RestException.kt | 61 +++++++++++++++ .../main/kotlin/io/realworld/users/Routes.kt | 25 +------ .../test/kotlin/io/realworld/ArticleTests.kt | 3 +- .../src/test/kotlin/io/realworld/TestUtils.kt | 4 + .../src/test/kotlin/io/realworld/UserTests.kt | 7 +- .../resources/json-schemas/resp-error.json | 38 ++++++++++ .../io/realworld/domain/articles/Errors.kt | 36 +++++++++ .../io/realworld/domain/articles/UseCases.kt | 29 -------- .../io/realworld/domain/common/Errors.kt | 16 ++++ .../io/realworld/domain/users/Errors.kt | 22 ++++++ .../io/realworld/domain/users/UseCases.kt | 13 ---- 15 files changed, 238 insertions(+), 146 deletions(-) create mode 100644 realworld-app/web/src/main/kotlin/io/realworld/errors/RestException.kt create mode 100644 realworld-app/web/src/test/resources/json-schemas/resp-error.json create mode 100644 realworld-domain/src/main/kotlin/io/realworld/domain/articles/Errors.kt create mode 100644 realworld-domain/src/main/kotlin/io/realworld/domain/common/Errors.kt create mode 100644 realworld-domain/src/main/kotlin/io/realworld/domain/users/Errors.kt diff --git a/realworld-app/web/src/main/kotlin/io/realworld/Application.kt b/realworld-app/web/src/main/kotlin/io/realworld/Application.kt index 6a61d86..e249722 100644 --- a/realworld-app/web/src/main/kotlin/io/realworld/Application.kt +++ b/realworld-app/web/src/main/kotlin/io/realworld/Application.kt @@ -2,9 +2,11 @@ package io.realworld import arrow.core.getOrElse import io.realworld.domain.common.Auth +import io.realworld.domain.common.DomainError import io.realworld.domain.common.Settings import io.realworld.domain.common.Token import io.realworld.domain.users.User +import io.realworld.errors.RestException import io.realworld.persistence.ArticleRepository import io.realworld.persistence.UserRepository import org.springframework.beans.factory.annotation.Autowired @@ -39,7 +41,9 @@ class Application : WebMvcConfigurer { // TODO check token match override fun invoke(token: Token): User { - return repo.findById(token.id).unsafeRunSync().map { it.user }.getOrElse { throw UnauthorizedException() } + return repo.findById(token.id).unsafeRunSync().map { it.user }.getOrElse { + throw RestException.Unauthorized(AuthError.BadCredentials) + } } } @@ -76,10 +80,13 @@ inline fun userArgumentResolver( webRequest: NativeWebRequest, binderFactory: WebDataBinderFactory? ) = resolveToken(webRequest.authHeader()).fold( - { throw UnauthorizedException() }, + { throw RestException.Unauthorized(it) }, { createUser(it) } ) } -class ForbiddenException : Throwable() -class UnauthorizedException : Throwable() +sealed class AuthError(override val msg: String) : DomainError.Single() { + object InvalidToken : AuthError("Invalid token") + object InvalidAuthorizationHeader : AuthError("Invalid authorization header") + object BadCredentials : AuthError("Bad credentials") +} diff --git a/realworld-app/web/src/main/kotlin/io/realworld/ErrorHandler.kt b/realworld-app/web/src/main/kotlin/io/realworld/ErrorHandler.kt index e9aa8d3..ae7b717 100644 --- a/realworld-app/web/src/main/kotlin/io/realworld/ErrorHandler.kt +++ b/realworld-app/web/src/main/kotlin/io/realworld/ErrorHandler.kt @@ -1,18 +1,12 @@ package io.realworld -import com.fasterxml.jackson.databind.JsonMappingException -import com.fasterxml.jackson.module.kotlin.MissingKotlinParameterException +import io.realworld.domain.common.DomainError import io.realworld.errors.ErrorResponseDto +import io.realworld.errors.RestException import io.realworld.errors.SingleErrorDto -import org.slf4j.Logger -import org.slf4j.LoggerFactory -import org.springframework.core.NestedExceptionUtils -import org.springframework.core.Ordered -import org.springframework.core.annotation.Order import org.springframework.http.HttpHeaders import org.springframework.http.HttpStatus import org.springframework.http.ResponseEntity -import org.springframework.http.converter.HttpMessageNotReadableException import org.springframework.validation.FieldError import org.springframework.validation.ObjectError import org.springframework.web.bind.MethodArgumentNotValidException @@ -22,7 +16,6 @@ import org.springframework.web.context.request.ServletWebRequest import org.springframework.web.context.request.WebRequest import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler import org.springframework.web.util.WebUtils -import java.lang.Exception import javax.servlet.ServletException @ControllerAdvice @@ -75,14 +68,8 @@ class RestExceptionHandler : ResponseEntityExceptionHandler() { return ResponseEntity(responseBody, headers, status) } - @ExceptionHandler(ForbiddenException::class) - fun forbidden() = ResponseEntity(HttpStatus.FORBIDDEN) - - @ExceptionHandler(UnauthorizedException::class) - fun unauthorized() = ResponseEntity(HttpStatus.UNAUTHORIZED) - - @ExceptionHandler(MyFieldError::class) - fun fieldError(ex: MyFieldError) = handleError(HttpStatus.UNPROCESSABLE_ENTITY, listOf(ex.toValidationError())) + @ExceptionHandler(RestException::class) + fun handleRestException(e: RestException, req: WebRequest) = e.error.toErrorResponse(e.status, req) // TODO find out why this overrides more specific errors // @ExceptionHandler(Throwable::class) @@ -122,29 +109,38 @@ fun ObjectError.toErrorDto() = SingleErrorDto( metadata = mapOf("path" to objectName) ) -fun handleError( - httpStatus: HttpStatus, - validationErrors: List = emptyList() -) = when (validationErrors.isEmpty()) { - true -> ResponseEntity(httpStatus) - else -> ResponseEntity( - ValidationErrorResponse(validationErrors.associateBy { it.path }), - httpStatus) +fun DomainError.toErrorResponse( + status: HttpStatus, + request: WebRequest +): ResponseEntity = when (this) { + is DomainError.Single -> toErrorResponse(status, request) + is DomainError.Multi -> toErrorResponse(status, request) } -data class ValidationError( - val type: String, - val path: String, - val message: String, - val arguments: Map? = emptyMap() +fun DomainError.Single.toErrorResponse( + status: HttpStatus, + request: WebRequest +): ResponseEntity = ResponseEntity( + ErrorResponseDto( + status = status.value(), + errorCode = this::class.simpleName ?: "Undefined", + message = msg, + path = (request as ServletWebRequest).request.servletPath, + errors = listOf() + ), + status ) -class MyFieldError(val path: String, message: String) : Throwable(message) { - fun toValidationError() = ValidationError( - type = "FieldError", - path = path, - message = this.message ?: "" - ) -} - -class ValidationErrorResponse(val errors: Map) +fun DomainError.Multi.toErrorResponse( + status: HttpStatus, + request: WebRequest +): ResponseEntity = ResponseEntity( + ErrorResponseDto( + status = status.value(), + errorCode = errorCode, + message = msg, + path = (request as ServletWebRequest).request.servletPath, + errors = errors.all.map { SingleErrorDto(it.msg) } + ), + status +) diff --git a/realworld-app/web/src/main/kotlin/io/realworld/Jwt.kt b/realworld-app/web/src/main/kotlin/io/realworld/Jwt.kt index a35d027..9fdebf4 100644 --- a/realworld-app/web/src/main/kotlin/io/realworld/Jwt.kt +++ b/realworld-app/web/src/main/kotlin/io/realworld/Jwt.kt @@ -3,31 +3,24 @@ package io.realworld import arrow.core.Either import arrow.core.left import arrow.core.right -import io.realworld.JwtError.NoToken -import io.realworld.JwtError.ParseFail import org.springframework.http.HttpHeaders import org.springframework.web.context.request.NativeWebRequest -typealias ResolveToken = (authHeader: String?) -> Either +typealias ResolveToken = (authHeader: String?) -> Either typealias ParseToken = (token: String) -> T -sealed class JwtError { - object ParseFail : JwtError() - object NoToken : JwtError() -} - class JwtTokenResolver(val parseToken: ParseToken) : ResolveToken { - override fun invoke(authHeader: String?): Either { + override fun invoke(authHeader: String?): Either { authHeader?.apply { if (startsWith(TOKEN_PREFIX)) { return try { parseToken(substring(TOKEN_PREFIX.length)).right() } catch (t: Throwable) { - ParseFail.left() + AuthError.InvalidToken.left() } } } - return NoToken.left() + return AuthError.InvalidAuthorizationHeader.left() } companion object { diff --git a/realworld-app/web/src/main/kotlin/io/realworld/articles/Routes.kt b/realworld-app/web/src/main/kotlin/io/realworld/articles/Routes.kt index f0a5b86..41d19d9 100644 --- a/realworld-app/web/src/main/kotlin/io/realworld/articles/Routes.kt +++ b/realworld-app/web/src/main/kotlin/io/realworld/articles/Routes.kt @@ -1,16 +1,12 @@ package io.realworld.articles -import io.realworld.ForbiddenException import io.realworld.JwtTokenResolver import io.realworld.authHeader import io.realworld.domain.articles.Article import io.realworld.domain.articles.ArticleCommentDeleteError import io.realworld.domain.articles.ArticleCommentError -import io.realworld.domain.articles.ArticleDeleteError -import io.realworld.domain.articles.ArticleFavoriteError import io.realworld.domain.articles.ArticleFilter import io.realworld.domain.articles.ArticleUnfavoriteError -import io.realworld.domain.articles.ArticleUpdateError import io.realworld.domain.articles.Comment import io.realworld.domain.articles.CommentArticleCommand import io.realworld.domain.articles.CommentUseCase @@ -43,6 +39,7 @@ import io.realworld.domain.articles.ValidateArticleUpdateService import io.realworld.domain.articles.articleScopedCommentId import io.realworld.domain.common.Auth import io.realworld.domain.users.User +import io.realworld.errors.toRestException import io.realworld.persistence.ArticleRepository import io.realworld.persistence.UserRepository import io.realworld.runReadTx @@ -190,12 +187,7 @@ class ArticleController( }.run { DeleteArticleCommand(slug, user).runUseCase() }.runWriteTx(txManager).fold( - { - when (it) { - is ArticleDeleteError.NotAuthor -> throw ForbiddenException() - is ArticleDeleteError.NotFound -> ResponseEntity.notFound().build() - } - }, + { it.toRestException() }, { ResponseEntity.noContent().build() } ) } @@ -221,12 +213,7 @@ class ArticleController( }.run { UpdateArticleCommand(update.toDomain(), slug, user).runUseCase() }.runWriteTx(txManager).fold( - { - when (it) { - is ArticleUpdateError.NotAuthor -> throw ForbiddenException() - is ArticleUpdateError.NotFound -> ResponseEntity.notFound().build() - } - }, + { it.toRestException() }, { ResponseEntity.ok(ArticleResponse.fromDomain(it)) } ) } @@ -243,12 +230,7 @@ class ArticleController( }.run { FavoriteArticleCommand(slug, user).runUseCase() }.runWriteTx(txManager).fold( - { - when (it) { - is ArticleFavoriteError.Author -> throw ForbiddenException() - is ArticleFavoriteError.NotFound -> ResponseEntity.notFound().build() - } - }, + { it.toRestException() }, { ResponseEntity.ok(ArticleResponse.fromDomain(it)) } ) } diff --git a/realworld-app/web/src/main/kotlin/io/realworld/errors/RestException.kt b/realworld-app/web/src/main/kotlin/io/realworld/errors/RestException.kt new file mode 100644 index 0000000..c49a027 --- /dev/null +++ b/realworld-app/web/src/main/kotlin/io/realworld/errors/RestException.kt @@ -0,0 +1,61 @@ +package io.realworld.errors + +import io.realworld.domain.common.DomainError +import org.springframework.http.HttpStatus +import io.realworld.domain.articles.ArticleDeleteError +import io.realworld.domain.articles.ArticleFavoriteError +import io.realworld.domain.articles.ArticleUpdateError +import io.realworld.domain.users.UserLoginError +import io.realworld.domain.users.UserRegistrationError +import io.realworld.domain.users.UserUpdateError + +sealed class RestException(val status: HttpStatus) : RuntimeException() { + abstract val error: DomainError + data class BadRequest(override val error: DomainError) : RestException(HttpStatus.BAD_REQUEST) + data class Conflict(override val error: DomainError) : RestException(HttpStatus.CONFLICT) + data class Forbidden(override val error: DomainError) : RestException(HttpStatus.FORBIDDEN) + data class NotFound(override val error: DomainError) : RestException(HttpStatus.NOT_FOUND) + data class Unauthorized(override val error: DomainError) : RestException(HttpStatus.UNAUTHORIZED) + data class UnprocessableEntity(override val error: DomainError) : RestException(HttpStatus.UNPROCESSABLE_ENTITY) + data class InternalServerError(override val error: DomainError) : RestException(HttpStatus.INTERNAL_SERVER_ERROR) +} + +fun UserLoginError.toRestException(): Nothing = when (this) { + is UserLoginError.BadCredentials -> + throw RestException.Unauthorized(this) +} + +fun UserRegistrationError.toRestException(): Nothing = when (this) { + is UserRegistrationError.EmailAlreadyTaken -> + throw RestException.Conflict(this) + is UserRegistrationError.UsernameAlreadyTaken -> + throw RestException.Conflict(this) +} + +fun UserUpdateError.toRestException(): Nothing = when (this) { + is UserUpdateError.EmailAlreadyTaken -> + throw RestException.Conflict(this) + is UserUpdateError.UsernameAlreadyTaken -> + throw RestException.Conflict(this) +} + +fun ArticleDeleteError.toRestException(): Nothing = when (this) { + is ArticleDeleteError.NotAuthor -> + throw RestException.Forbidden(this) + is ArticleDeleteError.NotFound -> + throw RestException.NotFound(this) +} + +fun ArticleFavoriteError.toRestException(): Nothing = when (this) { + is ArticleFavoriteError.Author -> + throw RestException.Forbidden(this) + is ArticleFavoriteError.NotFound -> + throw RestException.NotFound(this) +} + +fun ArticleUpdateError.toRestException(): Nothing = when (this) { + is ArticleUpdateError.NotAuthor -> + throw RestException.Forbidden(this) + is ArticleUpdateError.NotFound -> + throw RestException.NotFound(this) +} diff --git a/realworld-app/web/src/main/kotlin/io/realworld/users/Routes.kt b/realworld-app/web/src/main/kotlin/io/realworld/users/Routes.kt index 5de933e..62584ff 100644 --- a/realworld-app/web/src/main/kotlin/io/realworld/users/Routes.kt +++ b/realworld-app/web/src/main/kotlin/io/realworld/users/Routes.kt @@ -1,8 +1,6 @@ package io.realworld.users import arrow.core.Option -import io.realworld.MyFieldError -import io.realworld.UnauthorizedException import io.realworld.domain.common.Auth import io.realworld.domain.users.CreateUser import io.realworld.domain.users.GetUserByEmail @@ -15,13 +13,12 @@ import io.realworld.domain.users.UpdateUserCommand import io.realworld.domain.users.UpdateUserUseCase import io.realworld.domain.users.User import io.realworld.domain.users.UserRegistration -import io.realworld.domain.users.UserRegistrationError import io.realworld.domain.users.UserUpdate -import io.realworld.domain.users.UserUpdateError import io.realworld.domain.users.ValidateUserRegistration import io.realworld.domain.users.ValidateUserService import io.realworld.domain.users.ValidateUserUpdate import io.realworld.domain.users.ValidateUserUpdateService +import io.realworld.errors.toRestException import io.realworld.persistence.UserRepository import io.realworld.runWriteTx import org.springframework.http.HttpStatus @@ -68,14 +65,7 @@ class UserController( password = registration.password )).runUseCase() }.runWriteTx(txManager).fold( - { - when (it) { - is UserRegistrationError.EmailAlreadyTaken -> - throw MyFieldError("email", "already taken") - is UserRegistrationError.UsernameAlreadyTaken -> - throw MyFieldError("username", "already taken") - } - }, + { it.toRestException() }, { ResponseEntity.status(HttpStatus.CREATED).body(UserResponse.fromDomain(it)) } ) } @@ -91,7 +81,7 @@ class UserController( password = login.password ).runUseCase() }.runWriteTx(txManager).fold( - { throw UnauthorizedException() }, + { it.toRestException() }, { ResponseEntity.ok().body(UserResponse.fromDomain(it)) } ) } @@ -119,14 +109,7 @@ class UserController( current = user ).runUseCase() }.runWriteTx(txManager).fold( - { - when (it) { - is UserUpdateError.EmailAlreadyTaken -> - throw MyFieldError("email", "already taken") - is UserUpdateError.UsernameAlreadyTaken -> - throw MyFieldError("username", "already taken") - } - }, + { it.toRestException() }, { ResponseEntity.ok(UserResponse.fromDomain(it)) } ) } diff --git a/realworld-app/web/src/test/kotlin/io/realworld/ArticleTests.kt b/realworld-app/web/src/test/kotlin/io/realworld/ArticleTests.kt index 6e9bd9e..51e0988 100644 --- a/realworld-app/web/src/test/kotlin/io/realworld/ArticleTests.kt +++ b/realworld-app/web/src/test/kotlin/io/realworld/ArticleTests.kt @@ -281,8 +281,7 @@ class ArticleTests { }.toString().let { client.post("/api/articles", it) .then() - .statusCode(400) - .body("errorCode", Matchers.equalTo("HttpMessageNotReadableException")) + .verifyErrorCode("HttpMessageNotReadableException", 400) } } } diff --git a/realworld-app/web/src/test/kotlin/io/realworld/TestUtils.kt b/realworld-app/web/src/test/kotlin/io/realworld/TestUtils.kt index a8eb054..faa280d 100644 --- a/realworld-app/web/src/test/kotlin/io/realworld/TestUtils.kt +++ b/realworld-app/web/src/test/kotlin/io/realworld/TestUtils.kt @@ -104,3 +104,7 @@ fun ValidatableResponse.verifyValidationError(path: String, message: String) = "metadata" to mapOf("path" to path) )))) +fun ValidatableResponse.verifyErrorCode(errorCode: String, statusCode: Int) = this + .statusCode(statusCode) + .body(JsonSchemaValidator.matchesJsonSchemaInClasspath("json-schemas/resp-error.json")) + .body("errorCode", Matchers.equalTo(errorCode)) diff --git a/realworld-app/web/src/test/kotlin/io/realworld/UserTests.kt b/realworld-app/web/src/test/kotlin/io/realworld/UserTests.kt index d6ce82f..c75fe93 100644 --- a/realworld-app/web/src/test/kotlin/io/realworld/UserTests.kt +++ b/realworld-app/web/src/test/kotlin/io/realworld/UserTests.kt @@ -11,7 +11,6 @@ import io.realworld.users.UserResponseDto import io.realworld.users.UserUpdateDto import io.restassured.specification.RequestSpecification import org.assertj.core.api.Assertions.assertThat -import org.hamcrest.Matchers.equalTo import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test @@ -101,8 +100,7 @@ class UserTests { )) ApiClient(spec).post("/api/users", regReq) .then() - .statusCode(422) - .body("errors.username.message", equalTo("already taken")) + .verifyErrorCode("UsernameAlreadyTaken", 409) } @Test @@ -115,8 +113,7 @@ class UserTests { )) ApiClient(spec).post("/api/users", regReq) .then() - .statusCode(422) - .body("errors.email.message", equalTo("already taken")) + .verifyErrorCode("EmailAlreadyTaken", 409) } @Test diff --git a/realworld-app/web/src/test/resources/json-schemas/resp-error.json b/realworld-app/web/src/test/resources/json-schemas/resp-error.json new file mode 100644 index 0000000..8b1244b --- /dev/null +++ b/realworld-app/web/src/test/resources/json-schemas/resp-error.json @@ -0,0 +1,38 @@ +{ + "$schema": "http://json-schema.org/draft-04/schema#", + "type": "object", + "additionalProperties": false, + "required": [ + "status", + "errorCode", + "message", + "path", + "timestamp" + ], + "properties": { + "status": { "type": "number", "minimum": 100, "maximum": 599 }, + "errorCode": { + "type": "string", + "enum": [ + "EmailAlreadyTaken", + "HttpMessageNotReadableException", + "UsernameAlreadyTaken" + ] + }, + "message": { "type": "string" }, + "path": { "type": "string" }, + "timestamp": { "$ref": "defs-common.json#/definitions/datetime" }, + "errors": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ "message", "metadata" ], + "properties": { + "message": { "type": "string" }, + "metadata": { "type": [ "object", "null" ] } + } + } + } + } +} diff --git a/realworld-domain/src/main/kotlin/io/realworld/domain/articles/Errors.kt b/realworld-domain/src/main/kotlin/io/realworld/domain/articles/Errors.kt new file mode 100644 index 0000000..9deac9e --- /dev/null +++ b/realworld-domain/src/main/kotlin/io/realworld/domain/articles/Errors.kt @@ -0,0 +1,36 @@ +package io.realworld.domain.articles + +import io.realworld.domain.common.DomainError + +object ErrorMsg { + const val notFound = "Not found" +} + +sealed class ArticleUpdateError(override val msg: String) : DomainError.Single() { + object NotAuthor : ArticleUpdateError("Not author") + object NotFound : ArticleUpdateError(ErrorMsg.notFound) +} + +sealed class ArticleDeleteError(override val msg: String) : DomainError.Single() { + object NotAuthor : ArticleDeleteError("Not author") + object NotFound : ArticleDeleteError(ErrorMsg.notFound) +} + +sealed class ArticleFavoriteError(override val msg: String) : DomainError.Single() { + object Author : ArticleFavoriteError("Author cannot favorite") + object NotFound : ArticleFavoriteError(ErrorMsg.notFound) +} + +sealed class ArticleUnfavoriteError(override val msg: String) : DomainError.Single() { + object NotFound : ArticleUnfavoriteError(ErrorMsg.notFound) +} + +sealed class ArticleCommentError(override val msg: String) : DomainError.Single() { + object NotFound : ArticleCommentError(ErrorMsg.notFound) +} + +sealed class ArticleCommentDeleteError(override val msg: String) : DomainError.Single() { + object ArticleNotFound : ArticleCommentDeleteError("Article not found") + object CommentNotFound : ArticleCommentDeleteError("Comment not found") + object NotAuthor : ArticleCommentDeleteError("Only author can delete") +} diff --git a/realworld-domain/src/main/kotlin/io/realworld/domain/articles/UseCases.kt b/realworld-domain/src/main/kotlin/io/realworld/domain/articles/UseCases.kt index c5341b4..f876377 100644 --- a/realworld-domain/src/main/kotlin/io/realworld/domain/articles/UseCases.kt +++ b/realworld-domain/src/main/kotlin/io/realworld/domain/articles/UseCases.kt @@ -32,35 +32,6 @@ data class DeleteCommentCommand(val slug: String, val commentId: ArticleScopedCo data class GetCommentsCommand(val slug: String, val user: Option) object GetTagsCommand -sealed class ArticleUpdateError { - object NotAuthor : ArticleUpdateError() - object NotFound : ArticleUpdateError() -} - -sealed class ArticleDeleteError { - object NotAuthor : ArticleDeleteError() - object NotFound : ArticleDeleteError() -} - -sealed class ArticleFavoriteError { - object Author : ArticleFavoriteError() - object NotFound : ArticleFavoriteError() -} - -sealed class ArticleUnfavoriteError { - object NotFound : ArticleUnfavoriteError() -} - -sealed class ArticleCommentError { - object NotFound : ArticleCommentError() -} - -sealed class ArticleCommentDeleteError { - object ArticleNotFound : ArticleCommentDeleteError() - object CommentNotFound : ArticleCommentDeleteError() - object NotAuthor : ArticleCommentDeleteError() -} - interface CreateArticleUseCase { val createUniqueSlug: CreateUniqueSlug val createArticle: CreateArticle diff --git a/realworld-domain/src/main/kotlin/io/realworld/domain/common/Errors.kt b/realworld-domain/src/main/kotlin/io/realworld/domain/common/Errors.kt new file mode 100644 index 0000000..420f481 --- /dev/null +++ b/realworld-domain/src/main/kotlin/io/realworld/domain/common/Errors.kt @@ -0,0 +1,16 @@ +// ktlint-disable filename +package io.realworld.domain.common + +import arrow.core.Nel + +sealed class DomainError { + abstract class Single() : DomainError() { + abstract val msg: String + } + + data class Multi( + val errors: Nel, + val msg: String = "Multiple errors encountered", + val errorCode: String = "MultiError" + ) : DomainError() +} diff --git a/realworld-domain/src/main/kotlin/io/realworld/domain/users/Errors.kt b/realworld-domain/src/main/kotlin/io/realworld/domain/users/Errors.kt new file mode 100644 index 0000000..432bd49 --- /dev/null +++ b/realworld-domain/src/main/kotlin/io/realworld/domain/users/Errors.kt @@ -0,0 +1,22 @@ +package io.realworld.domain.users + +import io.realworld.domain.common.DomainError + +object ErrorMsg { + const val emailAlreadyTaken = "Email already taken" + const val usernameAlreadyTaken = "Username already taken" +} + +sealed class UserLoginError(override val msg: String) : DomainError.Single() { + object BadCredentials : UserLoginError("Bad credentials") +} + +sealed class UserRegistrationError(override val msg: String) : DomainError.Single() { + object EmailAlreadyTaken : UserRegistrationError(ErrorMsg.emailAlreadyTaken) + object UsernameAlreadyTaken : UserRegistrationError(ErrorMsg.usernameAlreadyTaken) +} + +sealed class UserUpdateError(override val msg: String) : DomainError.Single() { + object EmailAlreadyTaken : UserUpdateError(ErrorMsg.emailAlreadyTaken) + object UsernameAlreadyTaken : UserUpdateError(ErrorMsg.usernameAlreadyTaken) +} diff --git a/realworld-domain/src/main/kotlin/io/realworld/domain/users/UseCases.kt b/realworld-domain/src/main/kotlin/io/realworld/domain/users/UseCases.kt index 50a993f..b151ecd 100644 --- a/realworld-domain/src/main/kotlin/io/realworld/domain/users/UseCases.kt +++ b/realworld-domain/src/main/kotlin/io/realworld/domain/users/UseCases.kt @@ -15,19 +15,6 @@ import io.realworld.domain.common.Auth data class RegisterUserCommand(val data: UserRegistration) data class LoginUserCommand(val email: String, val password: String) data class UpdateUserCommand(val data: UserUpdate, val current: User) -sealed class UserLoginError { - object BadCredentials : UserLoginError() -} - -sealed class UserRegistrationError { - object EmailAlreadyTaken : UserRegistrationError() - object UsernameAlreadyTaken : UserRegistrationError() -} - -sealed class UserUpdateError { - object EmailAlreadyTaken : UserUpdateError() - object UsernameAlreadyTaken : UserUpdateError() -} interface RegisterUserUseCase { val createUser: CreateUser From 45baaded73573a5ac900ede21f92eee58e708614 Mon Sep 17 00:00:00 2001 From: Isto Nikula Date: Sun, 31 May 2020 11:29:15 +0300 Subject: [PATCH 3/3] feat(error-handling): more robust auth error handling --- .../main/kotlin/io/realworld/Application.kt | 8 +----- .../web/src/main/kotlin/io/realworld/Jwt.kt | 24 ++++++---------- .../kotlin/io/realworld/domain/common/Auth.kt | 28 +++++++++++++++---- 3 files changed, 32 insertions(+), 28 deletions(-) diff --git a/realworld-app/web/src/main/kotlin/io/realworld/Application.kt b/realworld-app/web/src/main/kotlin/io/realworld/Application.kt index e249722..d89cda6 100644 --- a/realworld-app/web/src/main/kotlin/io/realworld/Application.kt +++ b/realworld-app/web/src/main/kotlin/io/realworld/Application.kt @@ -2,7 +2,7 @@ package io.realworld import arrow.core.getOrElse import io.realworld.domain.common.Auth -import io.realworld.domain.common.DomainError +import io.realworld.domain.common.AuthError import io.realworld.domain.common.Settings import io.realworld.domain.common.Token import io.realworld.domain.users.User @@ -84,9 +84,3 @@ inline fun userArgumentResolver( { createUser(it) } ) } - -sealed class AuthError(override val msg: String) : DomainError.Single() { - object InvalidToken : AuthError("Invalid token") - object InvalidAuthorizationHeader : AuthError("Invalid authorization header") - object BadCredentials : AuthError("Bad credentials") -} diff --git a/realworld-app/web/src/main/kotlin/io/realworld/Jwt.kt b/realworld-app/web/src/main/kotlin/io/realworld/Jwt.kt index 9fdebf4..d546f8e 100644 --- a/realworld-app/web/src/main/kotlin/io/realworld/Jwt.kt +++ b/realworld-app/web/src/main/kotlin/io/realworld/Jwt.kt @@ -1,27 +1,21 @@ package io.realworld import arrow.core.Either -import arrow.core.left -import arrow.core.right +import arrow.core.flatMap +import arrow.core.toOption +import io.realworld.domain.common.AuthError import org.springframework.http.HttpHeaders import org.springframework.web.context.request.NativeWebRequest typealias ResolveToken = (authHeader: String?) -> Either -typealias ParseToken = (token: String) -> T +typealias ParseToken = (token: String) -> Either class JwtTokenResolver(val parseToken: ParseToken) : ResolveToken { - override fun invoke(authHeader: String?): Either { - authHeader?.apply { - if (startsWith(TOKEN_PREFIX)) { - return try { - parseToken(substring(TOKEN_PREFIX.length)).right() - } catch (t: Throwable) { - AuthError.InvalidToken.left() - } - } - } - return AuthError.InvalidAuthorizationHeader.left() - } + override fun invoke(authHeader: String?): Either = + authHeader.toOption() + .filter { it.startsWith(TOKEN_PREFIX) } + .toEither { AuthError.InvalidAuthorizationHeader } + .flatMap { parseToken(it.substring(TOKEN_PREFIX.length)) } companion object { const val TOKEN_PREFIX = "Token " diff --git a/realworld-domain/src/main/kotlin/io/realworld/domain/common/Auth.kt b/realworld-domain/src/main/kotlin/io/realworld/domain/common/Auth.kt index 3a52a02..1a24f68 100644 --- a/realworld-domain/src/main/kotlin/io/realworld/domain/common/Auth.kt +++ b/realworld-domain/src/main/kotlin/io/realworld/domain/common/Auth.kt @@ -1,5 +1,10 @@ package io.realworld.domain.common +import arrow.core.Either +import arrow.core.left +import arrow.core.right +import io.jsonwebtoken.Claims +import io.jsonwebtoken.Jws import io.jsonwebtoken.Jwts import io.jsonwebtoken.SignatureAlgorithm import io.realworld.domain.users.UserId @@ -10,21 +15,32 @@ import java.util.UUID inline class Token(val id: UserId) +sealed class AuthError(override val msg: String) : DomainError.Single() { + data class InvalidToken(val cause: Throwable) : AuthError("Invalid token") + object InvalidAuthorizationHeader : AuthError("Invalid authorization header") + object BadCredentials : AuthError("Bad credentials") +} + class Auth(val settings: Settings.Security) { private val encryptor: PasswordEncryptor = StrongPasswordEncryptor() // TODO set expiration - fun createToken(proto: Token) = Jwts.builder() + fun createToken(proto: Token): String = Jwts.builder() .setSubject(proto.id.value.toString()) .signWith(SignatureAlgorithm.HS512, settings.tokenSecret.toByteArray()) .compact() - fun parse(token: String): Token { - val claims = Jwts.parser().setSigningKey(settings.tokenSecret.toByteArray()).parseClaimsJws(token) - return Token(UUID.fromString(claims.body.subject).userId()) - } + fun parse(token: String): Either = + parseClaims(token).map { Token(UUID.fromString(it.body.subject).userId()) } - fun encryptPassword(plain: String) = encryptor.encryptPassword(plain) + fun encryptPassword(plain: String): String = encryptor.encryptPassword(plain) fun checkPassword(plain: String, encrypted: String) = encryptor.checkPassword(plain, encrypted) + + private fun parseClaims(token: String): Either> = + try { + Jwts.parser().setSigningKey(settings.tokenSecret.toByteArray()).parseClaimsJws(token).right() + } catch (t: Throwable) { + AuthError.InvalidToken(t).left() + } }