diff --git a/api/app/src/main/kotlin/packit/exceptions/FilterChainExceptionHandler.kt b/api/app/src/main/kotlin/packit/exceptions/FilterChainExceptionHandler.kt new file mode 100644 index 00000000..ae7a660a --- /dev/null +++ b/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) + } + } +} diff --git a/api/app/src/main/kotlin/packit/exceptions/PackitExceptionHandler.kt b/api/app/src/main/kotlin/packit/exceptions/PackitExceptionHandler.kt index 9dfd4476..06d7b622 100644 --- a/api/app/src/main/kotlin/packit/exceptions/PackitExceptionHandler.kt +++ b/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,9 +18,10 @@ import packit.model.ErrorDetail import packit.service.GenericClientException import java.util.* -@ControllerAdvice +@RestControllerAdvice class PackitExceptionHandler { + @ExceptionHandler(NoHandlerFoundException::class) fun handleNoHandlerFoundException(e: Exception): Any { @@ -27,6 +29,13 @@ class PackitExceptionHandler .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 { 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() } diff --git a/api/app/src/main/kotlin/packit/security/TokenAuthenticationFilter.kt b/api/app/src/main/kotlin/packit/security/TokenAuthenticationFilter.kt index d97668a5..c9d22b40 100644 --- a/api/app/src/main/kotlin/packit/security/TokenAuthenticationFilter.kt +++ b/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,7 +45,6 @@ class TokenAuthenticationFilter( fun extractToken(request: HttpServletRequest): Optional { val token = request.getHeader("Authorization") - if (StringUtils.hasText(token)) { if (token.startsWith("Bearer ")) @@ -51,7 +52,7 @@ class TokenAuthenticationFilter( 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() diff --git a/api/app/src/main/kotlin/packit/security/WebSecurityConfig.kt b/api/app/src/main/kotlin/packit/security/WebSecurityConfig.kt index eb5d9283..c89b0d32 100644 --- a/api/app/src/main/kotlin/packit/security/WebSecurityConfig.kt +++ b/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 diff --git a/api/app/src/main/kotlin/packit/security/provider/TokenDecoder.kt b/api/app/src/main/kotlin/packit/security/provider/TokenDecoder.kt index c1412213..26ef210f 100644 --- a/api/app/src/main/kotlin/packit/security/provider/TokenDecoder.kt +++ b/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) } } } diff --git a/api/app/src/main/kotlin/packit/service/RoleService.kt b/api/app/src/main/kotlin/packit/service/RoleService.kt index 799bdbde..288d0096 100644 --- a/api/app/src/main/kotlin/packit/service/RoleService.kt +++ b/api/app/src/main/kotlin/packit/service/RoleService.kt @@ -111,13 +111,13 @@ class BaseRoleService( override fun getSortedRoleDtos(roles: List): List { 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() } } diff --git a/api/app/src/main/resources/application.properties b/api/app/src/main/resources/application.properties index 96b212f8..13b1cc25 100644 --- a/api/app/src/main/resources/application.properties +++ b/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. diff --git a/api/app/src/main/resources/errorBundle.properties b/api/app/src/main/resources/errorBundle.properties index 63366bd8..bb12f150 100644 --- a/api/app/src/main/resources/errorBundle.properties +++ b/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 diff --git a/api/app/src/test/kotlin/packit/integration/controllers/RoleControllerTest.kt b/api/app/src/test/kotlin/packit/integration/controllers/RoleControllerTest.kt index 086acf20..227356bf 100644 --- a/api/app/src/test/kotlin/packit/integration/controllers/RoleControllerTest.kt +++ b/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)) } diff --git a/api/app/src/test/kotlin/packit/integration/exceptions/PackitExceptionHandlerTest.kt b/api/app/src/test/kotlin/packit/integration/exceptions/PackitExceptionHandlerTest.kt index f2aa77f3..1cda3ff7 100644 --- a/api/app/src/test/kotlin/packit/integration/exceptions/PackitExceptionHandlerTest.kt +++ b/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 = restTemplate.exchange( + "/wrong-endpoint", + HttpMethod.GET, + getTokenizedHttpEntity() + ) + + assertEquals(HttpStatus.NOT_FOUND, result.statusCode) + } + + @Test + fun `throw 405 when method not allowed`() + { + val result: ResponseEntity = restTemplate.exchange( + "/auth/login/api", + HttpMethod.PUT, + getTokenizedHttpEntity() + ) + + assertEquals(HttpStatus.METHOD_NOT_ALLOWED, result.statusCode) + } } diff --git a/api/app/src/test/kotlin/packit/unit/exceptions/PackitExceptionHandlerTest.kt b/api/app/src/test/kotlin/packit/unit/exceptions/PackitExceptionHandlerTest.kt index 89910582..52cd2f61 100644 --- a/api/app/src/test/kotlin/packit/unit/exceptions/PackitExceptionHandlerTest.kt +++ b/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 + ) + } } diff --git a/api/app/src/test/kotlin/packit/unit/security/TokenAuthenticationFilterTest.kt b/api/app/src/test/kotlin/packit/unit/security/TokenAuthenticationFilterTest.kt index 27bbcef6..9cec9ecd 100644 --- a/api/app/src/test/kotlin/packit/unit/security/TokenAuthenticationFilterTest.kt +++ b/api/app/src/test/kotlin/packit/unit/security/TokenAuthenticationFilterTest.kt @@ -38,8 +38,8 @@ class TokenAuthenticationFilterTest val thrownError = assertThrows { sut.extractToken(mockHttpRequest) } assertEquals( - "PackitException with key Invalid authorization type. Please use a Bearer token", - thrownError.message + "invalidAuthType", + thrownError.key ) } diff --git a/api/app/src/test/kotlin/packit/unit/security/provider/TokenDecoderTest.kt b/api/app/src/test/kotlin/packit/unit/security/provider/TokenDecoderTest.kt index 1e688f3d..465ae2bf 100644 --- a/api/app/src/test/kotlin/packit/unit/security/provider/TokenDecoderTest.kt +++ b/api/app/src/test/kotlin/packit/unit/security/provider/TokenDecoderTest.kt @@ -12,7 +12,6 @@ import packit.security.profile.UserPrincipal import packit.security.provider.TokenDecoder import packit.security.provider.TokenProvider import java.time.Duration -import java.util.* import kotlin.test.assertEquals class TokenDecoderTest @@ -65,13 +64,9 @@ class TokenDecoderTest val errorThrown = assertThrows { tokenDecoder.decode(jwtToken) } - assertEquals( - "PackitException with key verification failed: " + - "The token was expected to have 3 parts, but got 2.", - errorThrown.message - ) + assertEquals("jwtVerificationFailed", errorThrown.key) - assertEquals(HttpStatus.INTERNAL_SERVER_ERROR, errorThrown.httpStatus) + assertEquals(HttpStatus.UNAUTHORIZED, errorThrown.httpStatus) } @Test @@ -85,13 +80,9 @@ class TokenDecoderTest val errorThrown = assertThrows { tokenDecoder.decode(jwtToken) } - assertEquals( - "PackitException with key signature failed: The Token's Signature resulted " + - "invalid when verified using the Algorithm: HmacSHA256", - errorThrown.message - ) + assertEquals("jwtSignatureVerificationFailed", errorThrown.key) - assertEquals(HttpStatus.INTERNAL_SERVER_ERROR, errorThrown.httpStatus) + assertEquals(HttpStatus.UNAUTHORIZED, errorThrown.httpStatus) } @Test @@ -106,11 +97,8 @@ class TokenDecoderTest val errorThrown = assertThrows { tokenDecoder.decode(jwtToken) } - assertEquals( - "PackitException with key expired token: The Token has expired on 2023-09-22T13:52:38Z.", - errorThrown.message - ) + assertEquals("jwtTokenExpired", errorThrown.key) - assertEquals(HttpStatus.INTERNAL_SERVER_ERROR, errorThrown.httpStatus) + assertEquals(HttpStatus.UNAUTHORIZED, errorThrown.httpStatus) } } diff --git a/api/app/src/test/kotlin/packit/unit/service/RoleServiceTest.kt b/api/app/src/test/kotlin/packit/unit/service/RoleServiceTest.kt index 4292505f..7bab5473 100644 --- a/api/app/src/test/kotlin/packit/unit/service/RoleServiceTest.kt +++ b/api/app/src/test/kotlin/packit/unit/service/RoleServiceTest.kt @@ -519,6 +519,36 @@ class RoleServiceTest assertEquals("c user", result[0].users[2].username) } + @Test + fun `getSortedRoleDtos filters out service users`() + { + val role1 = Role(name = "role1", id = 1).apply { + users = mutableListOf( + User( + username = "c user", + id = UUID.randomUUID(), + disabled = false, + displayName = "user1", + userSource = "github" + ), + User( + username = "service user", + id = UUID.randomUUID(), + disabled = false, + displayName = "service user", + userSource = "service" + ), + ) + } + + val roles = listOf(role1) + + val result = roleService.getSortedRoleDtos(roles) + + assertEquals(1, result[0].users.size) + assert(result[0].users.none { it.username == "service user" }) + } + private fun createRoleWithPermission( roleName: String, permissionName: String,