From d54d02ba7db0b07fdbbbe78291b265acaea5871d Mon Sep 17 00:00:00 2001 From: anmol thapar Date: Tue, 22 Oct 2024 20:17:12 +0100 Subject: [PATCH 1/7] fix: get working version with exceptions --- .../exceptions/FilterChainExceptionHandler.kt | 35 +++++++++++++++++++ .../exceptions/PackitExceptionHandler.kt | 13 +++++-- .../packit/security/WebSecurityConfig.kt | 16 ++++++--- .../packit/security/provider/TokenDecoder.kt | 7 ++-- .../src/main/resources/application.properties | 6 +++- .../src/main/resources/errorBundle.properties | 3 ++ 6 files changed, 69 insertions(+), 11 deletions(-) create mode 100644 api/app/src/main/kotlin/packit/exceptions/FilterChainExceptionHandler.kt 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..b6d93509 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 { diff --git a/api/app/src/main/kotlin/packit/security/WebSecurityConfig.kt b/api/app/src/main/kotlin/packit/security/WebSecurityConfig.kt index eb5d9283..0cbf0c84 100644 --- a/api/app/src/main/kotlin/packit/security/WebSecurityConfig.kt +++ b/api/app/src/main/kotlin/packit/security/WebSecurityConfig.kt @@ -1,9 +1,10 @@ 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 import org.springframework.core.annotation.Order -import org.springframework.http.HttpStatus import org.springframework.security.authentication.AuthenticationManager import org.springframework.security.config.annotation.authentication.builders.AuthenticationManagerBuilder import org.springframework.security.config.annotation.method.configuration.EnableMethodSecurity @@ -11,11 +12,12 @@ import org.springframework.security.config.annotation.web.builders.HttpSecurity import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity 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,17 +59,20 @@ 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() } .handleSecurePaths() .handleOauth2Login() - .exceptionHandling { it.authenticationEntryPoint(HttpStatusEntryPoint(HttpStatus.UNAUTHORIZED)) } + .exceptionHandling() return httpSecurity.build() } @@ -107,10 +112,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/resources/application.properties b/api/app/src/main/resources/application.properties index 96b212f8..efd7b7a0 100644 --- a/api/app/src/main/resources/application.properties +++ b/api/app/src/main/resources/application.properties @@ -14,6 +14,11 @@ 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 +server.error.whitelabel.enabled=false # CUSTOM PACKIT CONFIG #db outpack.server.url=${PACKIT_OUTPACK_SERVER_URL:http://localhost:8000} @@ -31,7 +36,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..3f91ec61 100644 --- a/api/app/src/main/resources/errorBundle.properties +++ b/api/app/src/main/resources/errorBundle.properties @@ -16,6 +16,9 @@ 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 From f001a8cd586fb37c94bbef6ddbc2759724677012 Mon Sep 17 00:00:00 2001 From: anmol thapar Date: Wed, 23 Oct 2024 10:17:44 +0100 Subject: [PATCH 2/7] Refactor error handling in PackitExceptionHandler and TokenAuthenticationFilter --- .../exceptions/PackitExceptionHandler.kt | 5 ++- .../security/TokenAuthenticationFilter.kt | 7 ++-- .../src/main/resources/application.properties | 1 - .../src/main/resources/errorBundle.properties | 1 + .../exceptions/PackitExceptionHandlerTest.kt | 24 ++++++++++++ .../exceptions/PackitExceptionHandlerTest.kt | 37 +++++++++++++++++++ .../security/TokenAuthenticationFilterTest.kt | 4 +- .../security/provider/TokenDecoderTest.kt | 25 ++++--------- 8 files changed, 78 insertions(+), 26 deletions(-) diff --git a/api/app/src/main/kotlin/packit/exceptions/PackitExceptionHandler.kt b/api/app/src/main/kotlin/packit/exceptions/PackitExceptionHandler.kt index b6d93509..06d7b622 100644 --- a/api/app/src/main/kotlin/packit/exceptions/PackitExceptionHandler.kt +++ b/api/app/src/main/kotlin/packit/exceptions/PackitExceptionHandler.kt @@ -98,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/resources/application.properties b/api/app/src/main/resources/application.properties index efd7b7a0..13b1cc25 100644 --- a/api/app/src/main/resources/application.properties +++ b/api/app/src/main/resources/application.properties @@ -18,7 +18,6 @@ management.endpoints.web.exposure.include=health,prometheus spring.mvc.throw-exception-if-no-handler-found=true spring.web.resources.add-mappings=false server.error.include-stacktrace=never -server.error.whitelabel.enabled=false # CUSTOM PACKIT CONFIG #db outpack.server.url=${PACKIT_OUTPACK_SERVER_URL:http://localhost:8000} diff --git a/api/app/src/main/resources/errorBundle.properties b/api/app/src/main/resources/errorBundle.properties index 3f91ec61..bb12f150 100644 --- a/api/app/src/main/resources/errorBundle.properties +++ b/api/app/src/main/resources/errorBundle.properties @@ -22,6 +22,7 @@ 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/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..d1b7b306 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,9 @@ 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) } } From 0b8d438e59fe287f2f701a7b4526b407da88af4a Mon Sep 17 00:00:00 2001 From: anmol thapar Date: Wed, 23 Oct 2024 10:44:36 +0100 Subject: [PATCH 3/7] Refactor error handling in PackitExceptionHandler and TokenAuthenticationFilter --- api/app/src/main/kotlin/packit/security/WebSecurityConfig.kt | 4 +++- .../packit/integration/controllers/RoleControllerTest.kt | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/api/app/src/main/kotlin/packit/security/WebSecurityConfig.kt b/api/app/src/main/kotlin/packit/security/WebSecurityConfig.kt index 0cbf0c84..c89b0d32 100644 --- a/api/app/src/main/kotlin/packit/security/WebSecurityConfig.kt +++ b/api/app/src/main/kotlin/packit/security/WebSecurityConfig.kt @@ -5,6 +5,7 @@ import org.springframework.boot.actuate.autoconfigure.security.servlet.EndpointR import org.springframework.context.annotation.Bean import org.springframework.context.annotation.Configuration import org.springframework.core.annotation.Order +import org.springframework.http.HttpStatus import org.springframework.security.authentication.AuthenticationManager import org.springframework.security.config.annotation.authentication.builders.AuthenticationManagerBuilder import org.springframework.security.config.annotation.method.configuration.EnableMethodSecurity @@ -12,6 +13,7 @@ import org.springframework.security.config.annotation.web.builders.HttpSecurity import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity 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 @@ -72,7 +74,7 @@ class WebSecurityConfig( .formLogin { it.disable() } .handleSecurePaths() .handleOauth2Login() - .exceptionHandling() + .exceptionHandling { it.authenticationEntryPoint(HttpStatusEntryPoint(HttpStatus.UNAUTHORIZED)) } return httpSecurity.build() } 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..8c59e8f9 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 ) From 411cc93d0db36de828e4c2901a6d3f0159d45b05 Mon Sep 17 00:00:00 2001 From: anmol thapar Date: Wed, 23 Oct 2024 11:05:25 +0100 Subject: [PATCH 4/7] lint error --- .../kotlin/packit/unit/security/provider/TokenDecoderTest.kt | 1 - 1 file changed, 1 deletion(-) 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 d1b7b306..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 @@ -99,7 +99,6 @@ class TokenDecoderTest assertEquals("jwtTokenExpired", errorThrown.key) - assertEquals(HttpStatus.UNAUTHORIZED, errorThrown.httpStatus) } } From 0d2899daa24c957f805a508d890488a1a5277cf7 Mon Sep 17 00:00:00 2001 From: anmol thapar Date: Wed, 23 Oct 2024 14:12:32 +0100 Subject: [PATCH 5/7] Refactor RoleService to filter out service users in getSortedRoleDtos --- .../main/kotlin/packit/service/RoleService.kt | 1 + .../packit/unit/service/RoleServiceTest.kt | 30 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/api/app/src/main/kotlin/packit/service/RoleService.kt b/api/app/src/main/kotlin/packit/service/RoleService.kt index 799bdbde..050026a5 100644 --- a/api/app/src/main/kotlin/packit/service/RoleService.kt +++ b/api/app/src/main/kotlin/packit/service/RoleService.kt @@ -111,6 +111,7 @@ class BaseRoleService( override fun getSortedRoleDtos(roles: List): List { return roles.map { role -> + role.users = role.users.filter { !it.isServiceUser() }.toMutableList() val roleDto = role.toDto() roleDto.rolePermissions = roleDto.rolePermissions.sortedByDescending { 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, From 70e99c7f1b2df1a630f73dd0c7b61c88e0e8c21f Mon Sep 17 00:00:00 2001 From: anmol thapar Date: Wed, 23 Oct 2024 14:20:11 +0100 Subject: [PATCH 6/7] refactor to make more clean --- .../src/main/kotlin/packit/service/RoleService.kt | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/api/app/src/main/kotlin/packit/service/RoleService.kt b/api/app/src/main/kotlin/packit/service/RoleService.kt index 050026a5..288d0096 100644 --- a/api/app/src/main/kotlin/packit/service/RoleService.kt +++ b/api/app/src/main/kotlin/packit/service/RoleService.kt @@ -111,14 +111,13 @@ class BaseRoleService( override fun getSortedRoleDtos(roles: List): List { return roles.map { role -> - role.users = role.users.filter { !it.isServiceUser() }.toMutableList() - 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() } } From 192550d4a1408fd2bb349d8af582f9549f2add71 Mon Sep 17 00:00:00 2001 From: anmol thapar Date: Wed, 23 Oct 2024 16:04:41 +0100 Subject: [PATCH 7/7] test fix --- .../kotlin/packit/integration/controllers/RoleControllerTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 8c59e8f9..227356bf 100644 --- a/api/app/src/test/kotlin/packit/integration/controllers/RoleControllerTest.kt +++ b/api/app/src/test/kotlin/packit/integration/controllers/RoleControllerTest.kt @@ -283,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)) }