Merge pull request #135 from mrc-ide/mrc-5901-401-insteadof-404

Mrc-5901 Fix Excepion handling packit

api/app/src/main/kotlin/packit/exceptions/FilterChainExceptionHandler.kt

@@ -0,0 +1,35 @@
+package packit.exceptions
+
+import jakarta.servlet.FilterChain
+import jakarta.servlet.ServletException
+import jakarta.servlet.http.HttpServletRequest
+import jakarta.servlet.http.HttpServletResponse
+import org.slf4j.LoggerFactory
+import org.springframework.stereotype.Component
+import org.springframework.web.filter.OncePerRequestFilter
+import org.springframework.web.servlet.HandlerExceptionResolver
+import java.io.IOException
+
+@Component
+class FilterChainExceptionHandler(
+    private val handlerExceptionResolver: HandlerExceptionResolver
+) : OncePerRequestFilter()
+{
+    companion object
+    {
+        private val log = LoggerFactory.getLogger(FilterChainExceptionHandler::class.java)
+    }
+
+    @Throws(ServletException::class, IOException::class)
+    override fun doFilterInternal(request: HttpServletRequest, response: HttpServletResponse, filterChain: FilterChain)
+    {
+        try
+        {
+            filterChain.doFilter(request, response)
+        } catch (e: Exception)
+        {
+            log.error("Spring Security Filter Chain Exception:", e)
+            handlerExceptionResolver.resolveException(request, response, null, e)
+        }
+    }
+}

api/app/src/main/kotlin/packit/exceptions/PackitExceptionHandler.kt

@@ -6,9 +6,10 @@ import org.springframework.http.converter.HttpMessageNotReadableException
 import org.springframework.security.access.AccessDeniedException
 import org.springframework.security.authentication.InternalAuthenticationServiceException
 import org.springframework.security.core.AuthenticationException
+import org.springframework.web.HttpRequestMethodNotSupportedException
 import org.springframework.web.bind.MethodArgumentNotValidException
-import org.springframework.web.bind.annotation.ControllerAdvice
 import org.springframework.web.bind.annotation.ExceptionHandler
+import org.springframework.web.bind.annotation.RestControllerAdvice
 import org.springframework.web.client.HttpClientErrorException
 import org.springframework.web.client.HttpServerErrorException
 import org.springframework.web.client.HttpStatusCodeException
@@ -17,16 +18,24 @@ import packit.model.ErrorDetail
 import packit.service.GenericClientException
 import java.util.*
 
-@ControllerAdvice
+@RestControllerAdvice
 class PackitExceptionHandler
 {
+
     @ExceptionHandler(NoHandlerFoundException::class)
     fun handleNoHandlerFoundException(e: Exception): Any
     {
         return ErrorDetail(HttpStatus.NOT_FOUND, e.message ?: "")
             .toResponseEntity()
     }
 
+    @ExceptionHandler(HttpRequestMethodNotSupportedException::class)
+    fun handleHttpRequestMethodNotSupportedException(e: Exception): Any
+    {
+        return ErrorDetail(HttpStatus.METHOD_NOT_ALLOWED, e.message ?: "")
+            .toResponseEntity()
+    }
+
     @ExceptionHandler(HttpMessageNotReadableException::class)
     fun handleHttpMessageNotReadableException(e: Exception): Any
     {
@@ -89,8 +98,9 @@ class PackitExceptionHandler
     ): ResponseEntity<String>
     {
         val resourceBundle = getBundle()
-
-        return ErrorDetail(error.httpStatus, resourceBundle.getString(error.key))
+        val errorDetail =
+            if (resourceBundle.containsKey(error.key)) resourceBundle.getString(error.key) else error.key
+        return ErrorDetail(error.httpStatus, errorDetail)
             .toResponseEntity()
     }
 

api/app/src/main/kotlin/packit/security/TokenAuthenticationFilter.kt

@@ -3,6 +3,7 @@ package packit.security
 import jakarta.servlet.FilterChain
 import jakarta.servlet.http.HttpServletRequest
 import jakarta.servlet.http.HttpServletResponse
+import org.springframework.http.HttpStatus
 import org.springframework.security.core.context.SecurityContextHolder
 import org.springframework.stereotype.Component
 import org.springframework.util.StringUtils
@@ -11,7 +12,7 @@ import packit.exceptions.PackitException
 import packit.security.profile.TokenToPrincipal
 import packit.security.profile.UserPrincipalAuthenticationToken
 import packit.security.provider.JwtDecoder
-import java.util.Optional
+import java.util.*
 
 @Component
 class TokenAuthenticationFilter(
@@ -23,6 +24,7 @@ class TokenAuthenticationFilter(
     {
         private const val BearerTokenSubString = 7
     }
+
     override fun doFilterInternal(
         request: HttpServletRequest,
         response: HttpServletResponse,
@@ -43,15 +45,14 @@ class TokenAuthenticationFilter(
     fun extractToken(request: HttpServletRequest): Optional<String>
     {
         val token = request.getHeader("Authorization")
-
         if (StringUtils.hasText(token))
         {
             if (token.startsWith("Bearer "))
             {
                 return Optional.of(token.substring(BearerTokenSubString))
             } else
             {
-                throw PackitException("Invalid authorization type. Please use a Bearer token")
+                throw PackitException("invalidAuthType", HttpStatus.UNAUTHORIZED)
             }
         }
         return Optional.empty()

api/app/src/main/kotlin/packit/security/WebSecurityConfig.kt

@@ -1,4 +1,6 @@
 package packit.security
+
+import jakarta.servlet.DispatcherType
 import org.springframework.boot.actuate.autoconfigure.security.servlet.EndpointRequest
 import org.springframework.context.annotation.Bean
 import org.springframework.context.annotation.Configuration
@@ -13,9 +15,11 @@ import org.springframework.security.config.http.SessionCreationPolicy
 import org.springframework.security.web.SecurityFilterChain
 import org.springframework.security.web.authentication.HttpStatusEntryPoint
 import org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter
+import org.springframework.security.web.authentication.logout.LogoutFilter
 import org.springframework.web.cors.CorsConfiguration
 import org.springframework.web.cors.CorsConfigurationSource
 import packit.AppConfig
+import packit.exceptions.FilterChainExceptionHandler
 import packit.exceptions.PackitExceptionHandler
 import packit.security.oauth2.OAuth2FailureHandler
 import packit.security.oauth2.OAuth2SuccessHandler
@@ -57,11 +61,14 @@ class WebSecurityConfig(
     fun securityFilterChain(
         httpSecurity: HttpSecurity,
         tokenAuthenticationFilter: TokenAuthenticationFilter,
+        filterChainExceptionHandler: FilterChainExceptionHandler
+
     ): SecurityFilterChain
     {
         httpSecurity
             .cors { it.configurationSource(getCorsConfigurationSource()) }
             .csrf { it.disable() }
+            .addFilterBefore(filterChainExceptionHandler, LogoutFilter::class.java)
             .addFilterBefore(tokenAuthenticationFilter, UsernamePasswordAuthenticationFilter::class.java)
             .sessionManagement { it.sessionCreationPolicy(SessionCreationPolicy.STATELESS) }
             .formLogin { it.disable() }
@@ -107,10 +114,11 @@ class WebSecurityConfig(
         if (config.authEnabled)
         {
             this.securityMatcher("/**")
-                .authorizeHttpRequests { authorizeRequests ->
-                    authorizeRequests
+                .authorizeHttpRequests {
+                    it
                         .requestMatchers("/").permitAll()
                         .requestMatchers("/auth/**", "/oauth2/**").permitAll()
+                        .dispatcherTypeMatchers(DispatcherType.FORWARD, DispatcherType.ERROR).permitAll()
                         .anyRequest().authenticated()
                 }
         } else

api/app/src/main/kotlin/packit/security/provider/TokenDecoder.kt

@@ -6,6 +6,7 @@ import com.auth0.jwt.exceptions.JWTVerificationException
 import com.auth0.jwt.exceptions.SignatureVerificationException
 import com.auth0.jwt.exceptions.TokenExpiredException
 import com.auth0.jwt.interfaces.DecodedJWT
+import org.springframework.http.HttpStatus
 import org.springframework.stereotype.Component
 import packit.AppConfig
 import packit.exceptions.PackitException
@@ -27,13 +28,13 @@ class TokenDecoder(val config: AppConfig) : JwtDecoder
                 .verify(token)
         } catch (e: SignatureVerificationException)
         {
-            throw PackitException("signature failed: ${e.message}")
+            throw PackitException("jwtSignatureVerificationFailed", HttpStatus.UNAUTHORIZED)
         } catch (e: TokenExpiredException)
         {
-            throw PackitException("expired token: ${e.message}")
+            throw PackitException("jwtTokenExpired", HttpStatus.UNAUTHORIZED)
         } catch (e: JWTVerificationException)
         {
-            throw PackitException("verification failed: ${e.message}")
+            throw PackitException("jwtVerificationFailed", HttpStatus.UNAUTHORIZED)
         }
     }
 }

api/app/src/main/kotlin/packit/service/RoleService.kt

@@ -111,13 +111,13 @@ class BaseRoleService(
     override fun getSortedRoleDtos(roles: List<Role>): List<RoleDto>
     {
         return roles.map { role ->
-            val roleDto = role.toDto()
-            roleDto.rolePermissions =
-                roleDto.rolePermissions.sortedByDescending {
+            role.apply {
+                users = users.filterNot { it.isServiceUser() }.sortedBy { it.username }.toMutableList()
+                rolePermissions = role.rolePermissions.sortedByDescending {
                     it.tag == null && it.packet == null && it.packetGroup == null
-                }
-            roleDto.users = roleDto.users.sortedBy { it.username }
-            roleDto
+                }.toMutableList()
+            }
+            role.toDto()
         }
     }
 

api/app/src/main/resources/application.properties

@@ -14,6 +14,10 @@ spring.security.oauth2.client.registration.github.redirect-uri=${PACKIT_API_ROOT
 management.server.port=${PACKIT_MANAGEMENT_PORT:8081}
 management.endpoints.web.base-path=/
 management.endpoints.web.exposure.include=health,prometheus
+#errors
+spring.mvc.throw-exception-if-no-handler-found=true
+spring.web.resources.add-mappings=false
+server.error.include-stacktrace=never
 # CUSTOM PACKIT CONFIG
 #db
 outpack.server.url=${PACKIT_OUTPACK_SERVER_URL:http://localhost:8000}
@@ -31,7 +35,6 @@ auth.githubAPIOrg=${PACKIT_AUTH_GITHUB_ORG:mrc-ide}
 auth.githubAPITeam=${PACKIT_AUTH_GITHUB_TEAM:packit}
 # CORS
 cors.allowedOrigins=${PACKIT_CORS_ALLOWED_ORIGINS:http://localhost*,https://localhost*}
-
 # Comma-separated list of roles that are assigned to new users. The roles aren't
 # automatically created - an admin needs to create them manually for this setting
 # to have any effect.

api/app/src/main/resources/errorBundle.properties

@@ -16,9 +16,13 @@ githubTokenInsufficientPermissions=The supplied GitHub token does not have suffi
 githubTokenUnexpectedError=Unexpected error when checking user credentials with GitHub.
 githubUserRestrictedAccess=User is not in allowed organization or team. Please contact your administrator.
 jwtLoginDisabled=JWT login is disabled
+jwtSignatureVerificationFailed=JWT signature verification failed
+jwtTokenExpired=JWT token has expired
+jwtVerificationFailed=JWT verification failed
 externalJwtTokenInvalid=The supplied token is invalid
 httpClientError=Http Client error occurred
 invalidPassword=Invalid password
+invalidAuthType=Invalid authentication type
 insufficientPrivileges=You do not have sufficient privileges for attempted action
 invalidPermissionsProvided=Invalid permissions provided
 invalidRolesProvided=Invalid roles provided

api/app/src/test/kotlin/packit/integration/controllers/RoleControllerTest.kt

@@ -133,8 +133,9 @@ class RoleControllerTest : IntegrationTest()
         roleRepository.save(Role(name = "testRole"))
 
         val result =
-            restTemplate.postForEntity(
+            restTemplate.exchange(
                 "/role/testRole",
+                HttpMethod.DELETE,
                 getTokenizedHttpEntity(data = createTestRoleBody),
                 String::class.java
             )
@@ -282,7 +283,7 @@ class RoleControllerTest : IntegrationTest()
         val foundAdminRole = roles.find { it.name == adminRole.name }!!
         assertEquals(foundAdminRole.name, adminRole.name)
         assertEquals(foundAdminRole.rolePermissions.size, adminRole.rolePermissions.size)
-        assertEquals(foundAdminRole.users.size, adminRole.users.size)
+        assertEquals(foundAdminRole.users.size, adminRole.users.size - 1) // service user not included
         assertFalse(roles.contains(userRole))
     }
 

api/app/src/test/kotlin/packit/integration/exceptions/PackitExceptionHandlerTest.kt

@@ -52,4 +52,28 @@ class PackitExceptionHandlerTest : IntegrationTest()
 
         assertEquals(result.statusCode, HttpStatus.UNAUTHORIZED)
     }
+
+    @Test
+    fun `throw 404 error when not found endpoint is called`()
+    {
+        val result: ResponseEntity<String> = restTemplate.exchange(
+            "/wrong-endpoint",
+            HttpMethod.GET,
+            getTokenizedHttpEntity()
+        )
+
+        assertEquals(HttpStatus.NOT_FOUND, result.statusCode)
+    }
+
+    @Test
+    fun `throw 405 when method not allowed`()
+    {
+        val result: ResponseEntity<String> = restTemplate.exchange(
+            "/auth/login/api",
+            HttpMethod.PUT,
+            getTokenizedHttpEntity()
+        )
+
+        assertEquals(HttpStatus.METHOD_NOT_ALLOWED, result.statusCode)
+    }
 }

api/app/src/test/kotlin/packit/unit/exceptions/PackitExceptionHandlerTest.kt

@@ -1,7 +1,10 @@
 package packit.unit.exceptions
 
+import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper
 import org.junit.jupiter.api.Test
+import org.springframework.http.HttpStatus
 import packit.exceptions.PackitAuthenticationException
+import packit.exceptions.PackitException
 import packit.exceptions.PackitExceptionHandler
 import kotlin.test.assertEquals
 
@@ -18,4 +21,38 @@ class PackitExceptionHandlerTest
             "The supplied GitHub token does not have sufficient permissions to check user credentials."
         )
     }
+
+    @Test
+    fun `returns error detail from bundle if found for PackitException`()
+    {
+        val key = "doesNotExist"
+        val exception = PackitException(key, HttpStatus.NOT_FOUND)
+        val sut = PackitExceptionHandler()
+
+        val result = sut.handlePackitException(exception)
+
+        assertEquals(
+            result.statusCode,
+            HttpStatus.NOT_FOUND
+        )
+        assertEquals(
+            jacksonObjectMapper().readTree(result.body).get("error").get("detail").asText(),
+            "Resource does not exist"
+        )
+    }
+
+    @Test
+    fun `returns error detail from exception key if not found in bundle for PackitException`()
+    {
+        val key = "This key does not exist in error bundle"
+        val exception = PackitException(key)
+        val sut = PackitExceptionHandler()
+
+        val result = sut.handlePackitException(exception)
+
+        assertEquals(
+            jacksonObjectMapper().readTree(result.body).get("error").get("detail").asText(),
+            key
+        )
+    }
 }

api/app/src/test/kotlin/packit/unit/security/TokenAuthenticationFilterTest.kt

@@ -38,8 +38,8 @@ class TokenAuthenticationFilterTest
         val thrownError = assertThrows<PackitException> { sut.extractToken(mockHttpRequest) }
 
         assertEquals(
-            "PackitException with key Invalid authorization type. Please use a Bearer token",
-            thrownError.message
+            "invalidAuthType",
+            thrownError.key
         )
     }