From b235b5a84502f320490748f0bfc5bbf4d2ed3a96 Mon Sep 17 00:00:00 2001 From: william Date: Fri, 22 Jul 2022 08:46:16 -0400 Subject: [PATCH] #30 Random group token UUIDs to prevent critical security problem. --- .../filters/JwtAuthenticationFilter.kt | 4 +- .../dtos/account/GroupTokenDto.kt | 2 + .../logic/account/GroupTokenLogic.kt | 26 +++--- .../model/account/GroupToken.kt | 3 + .../repository/AccountRepository.kt | 10 ++- .../service/account/GroupTokenService.kt | 11 ++- .../account/DefaultGroupTokenLogicTest.kt | 80 +++++++++++++++++-- 7 files changed, 109 insertions(+), 27 deletions(-) diff --git a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/config/security/filters/JwtAuthenticationFilter.kt b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/config/security/filters/JwtAuthenticationFilter.kt index 08fe21c..0653354 100644 --- a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/config/security/filters/JwtAuthenticationFilter.kt +++ b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/config/security/filters/JwtAuthenticationFilter.kt @@ -18,7 +18,7 @@ import javax.servlet.http.HttpServletResponse abstract class JwtAuthenticationFilter( filterProcessesUrl: String, private val securityProperties: CreSecurityProperties, - protected val jwtLogic: JwtLogic + private val jwtLogic: JwtLogic ) : AbstractAuthenticationProcessingFilter( AntPathRequestMatcher(filterProcessesUrl, HttpMethod.POST.toString()) @@ -73,4 +73,4 @@ abstract class JwtAuthenticationFilter( private const val AUTHORIZATION_COOKIE_SAME_SITE = true private const val AUTHORIZATION_COOKIE_PATH = Constants.ControllerPaths.BASE_PATH } -} \ No newline at end of file +} diff --git a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/dtos/account/GroupTokenDto.kt b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/dtos/account/GroupTokenDto.kt index 6277fdc..f507e8c 100644 --- a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/dtos/account/GroupTokenDto.kt +++ b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/dtos/account/GroupTokenDto.kt @@ -10,6 +10,8 @@ data class GroupTokenDto( val enabled: Boolean, + val isDeleted: Boolean, + val group: GroupDto ) diff --git a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/logic/account/GroupTokenLogic.kt b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/logic/account/GroupTokenLogic.kt index a3b96d9..b956a1a 100644 --- a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/logic/account/GroupTokenLogic.kt +++ b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/logic/account/GroupTokenLogic.kt @@ -32,8 +32,7 @@ class DefaultGroupTokenLogic( private val groupLogic: GroupLogic, private val jwtLogic: JwtLogic, private val enabledTokensCache: HashSet = hashSetOf() // In constructor for unit testing -) : - GroupTokenLogic { +) : GroupTokenLogic { private val typeName = Constants.ModelNames.GROUP_TOKEN private val typeNameLowerCase = typeName.lowercase() @@ -49,8 +48,7 @@ class DefaultGroupTokenLogic( override fun getById(id: UUID) = service.getById(id) ?: throw notFoundException(value = id) override fun getIdForRequest(request: HttpServletRequest): UUID? { - val groupTokenCookie = getGroupTokenCookie(request) - ?: return null + val groupTokenCookie = getGroupTokenCookie(request) ?: return null val jwt = parseBearer(groupTokenCookie.value) return jwtLogic.parseGroupTokenIdJwt(jwt) @@ -59,10 +57,9 @@ class DefaultGroupTokenLogic( override fun save(dto: GroupTokenSaveDto): GroupTokenDto { throwIfNameAlreadyExists(dto.name) - // We don't need to check for collision, because UUIDs with different names will be different - val id = generateUUIDForName(dto.name) + val id = generateRandomUUID() val token = GroupTokenDto( - id, dto.name, true, groupLogic.getById(dto.groupId) + id, dto.name, enabled = true, isDeleted = false, group = groupLogic.getById(dto.groupId) ) val savedToken = service.save(token) @@ -84,15 +81,26 @@ class DefaultGroupTokenLogic( } override fun deleteById(id: String) { + val token = getById(id).copy(enabled = false, isDeleted = true) + + service.save(token) enabledTokensCache.remove(id) - service.deleteById(UUID.fromString(id)) } private fun setEnabled(id: String, enabled: Boolean) = with(getById(id)) { service.save(this.copy(enabled = enabled)) } - private fun generateUUIDForName(name: String) = UUID.nameUUIDFromBytes(name.toByteArray()) + private fun generateRandomUUID(): UUID { + var uuid = UUID.randomUUID() + + // The UUID specification doesn't guarantee to prevent collisions + while (service.existsById(uuid)) { + uuid = UUID.randomUUID() + } + + return uuid + } private fun throwIfNameAlreadyExists(name: String) { if (service.existsByName(name)) { diff --git a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/model/account/GroupToken.kt b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/model/account/GroupToken.kt index 0820672..7111906 100644 --- a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/model/account/GroupToken.kt +++ b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/model/account/GroupToken.kt @@ -20,6 +20,9 @@ data class GroupToken( @Column(name = "is_valid") val isValid: Boolean, + @Column(name = "deleted") + val isDeleted: Boolean, + @ManyToOne @JoinColumn(name = "group_id") val group: Group diff --git a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/repository/AccountRepository.kt b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/repository/AccountRepository.kt index e530d45..dee225e 100644 --- a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/repository/AccountRepository.kt +++ b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/repository/AccountRepository.kt @@ -39,6 +39,12 @@ interface GroupRepository : JpaRepository { @Repository interface GroupTokenRepository : JpaRepository { - /** Checks if a token with the given [name] exists. */ - fun existsByName(name: String): Boolean + /** Checks if a token that is not deleted with the given [name] exists. */ + fun existsByNameAndIsDeletedIsFalse(name: String): Boolean + + /** Finds all group tokens that are not deleted. */ + fun findAllByIsDeletedIsFalse(): Collection + + /** Finds the group token with the given [id] if it is not deleted. */ + fun findByIdAndIsDeletedIsFalse(id: UUID): GroupToken? } diff --git a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/account/GroupTokenService.kt b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/account/GroupTokenService.kt index 3cfb7c9..d780b22 100644 --- a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/account/GroupTokenService.kt +++ b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/account/GroupTokenService.kt @@ -4,7 +4,6 @@ import dev.fyloz.colorrecipesexplorer.config.annotations.ServiceComponent import dev.fyloz.colorrecipesexplorer.dtos.account.GroupTokenDto import dev.fyloz.colorrecipesexplorer.model.account.GroupToken import dev.fyloz.colorrecipesexplorer.repository.GroupTokenRepository -import org.springframework.data.repository.findByIdOrNull import java.util.UUID interface GroupTokenService { @@ -23,12 +22,12 @@ interface GroupTokenService { class DefaultGroupTokenService(private val repository: GroupTokenRepository, private val groupService: GroupService) : GroupTokenService { override fun existsById(id: UUID) = repository.existsById(id) - override fun existsByName(name: String) = repository.existsByName(name) + override fun existsByName(name: String) = repository.existsByNameAndIsDeletedIsFalse(name) - override fun getAll() = repository.findAll().map(::toDto) + override fun getAll() = repository.findAllByIsDeletedIsFalse().map(::toDto) override fun getById(id: UUID): GroupTokenDto? { - val entity = repository.findByIdOrNull(id) + val entity = repository.findByIdAndIsDeletedIsFalse(id) return if (entity != null) toDto(entity) else null } @@ -41,8 +40,8 @@ class DefaultGroupTokenService(private val repository: GroupTokenRepository, pri override fun deleteById(id: UUID) = repository.deleteById(id) override fun toDto(entity: GroupToken) = - GroupTokenDto(entity.id, entity.name, entity.isValid, groupService.toDto(entity.group)) + GroupTokenDto(entity.id, entity.name, entity.isValid, entity.isDeleted, groupService.toDto(entity.group)) override fun toEntity(dto: GroupTokenDto) = - GroupToken(dto.id, dto.name, dto.enabled, groupService.toEntity(dto.group)) + GroupToken(dto.id, dto.name, dto.enabled, dto.isDeleted, groupService.toEntity(dto.group)) } diff --git a/src/test/kotlin/dev/fyloz/colorrecipesexplorer/logic/account/DefaultGroupTokenLogicTest.kt b/src/test/kotlin/dev/fyloz/colorrecipesexplorer/logic/account/DefaultGroupTokenLogicTest.kt index 25f8182..03b66e0 100644 --- a/src/test/kotlin/dev/fyloz/colorrecipesexplorer/logic/account/DefaultGroupTokenLogicTest.kt +++ b/src/test/kotlin/dev/fyloz/colorrecipesexplorer/logic/account/DefaultGroupTokenLogicTest.kt @@ -1,5 +1,6 @@ package dev.fyloz.colorrecipesexplorer.logic.account +import dev.fyloz.colorrecipesexplorer.Constants import dev.fyloz.colorrecipesexplorer.dtos.account.GroupDto import dev.fyloz.colorrecipesexplorer.dtos.account.GroupTokenDto import dev.fyloz.colorrecipesexplorer.dtos.account.GroupTokenSaveDto @@ -10,22 +11,27 @@ import io.mockk.* import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.Test import org.junit.jupiter.api.assertThrows +import org.springframework.web.util.WebUtils import java.util.* +import javax.servlet.http.Cookie +import javax.servlet.http.HttpServletRequest import kotlin.test.* class DefaultGroupTokenLogicTest { private val groupTokenServiceMock = mockk() private val groupLogicMock = mockk() + private val jwtLogicMock = mockk() private val enabledTokenCache = hashSetOf() - private val groupTokenLogic = spyk(DefaultGroupTokenLogic(groupTokenServiceMock, groupLogicMock, enabledTokenCache)) + private val groupTokenLogic = + spyk(DefaultGroupTokenLogic(groupTokenServiceMock, groupLogicMock, jwtLogicMock, enabledTokenCache)) private val groupTokenName = "Unit test token" private val groupTokenId = UUID.nameUUIDFromBytes(groupTokenName.toByteArray()) private val groupTokenIdStr = groupTokenId.toString() private val group = GroupDto(1L, "Unit test group", listOf(), listOf()) - private val groupToken = GroupTokenDto(groupTokenId, groupTokenName, true, group) + private val groupToken = GroupTokenDto(groupTokenId, groupTokenName, true, false, group) private val groupTokenSaveDto = GroupTokenSaveDto(groupTokenName, group.id) @AfterEach @@ -106,15 +112,31 @@ class DefaultGroupTokenLogicTest { assertThrows { groupTokenLogic.getById(groupTokenId) } } + @Test + fun getIdForRequest_normalBehavior_returnsGroupTokenIdFromRequest() { + // Arrange + val request = mockk() + val cookie = mockk { + every { value } returns "Bearer$groupTokenIdStr" + } + + mockkStatic(WebUtils::class) { + every { WebUtils.getCookie(any(), Constants.CookieNames.GROUP_TOKEN) } returns cookie + } + } + @Test fun save_normalBehavior_callsSaveInService() { // Arrange every { groupTokenServiceMock.existsByName(any()) } returns false + every { groupTokenServiceMock.existsById(any()) } returns false every { groupTokenServiceMock.save(any()) } returns groupToken every { groupLogicMock.getById(any()) } returns group // Act - groupTokenLogic.save(groupTokenSaveDto) + withMockRandomUUID { + groupTokenLogic.save(groupTokenSaveDto) + } // Assert verify { @@ -122,15 +144,41 @@ class DefaultGroupTokenLogicTest { } } + @Test + fun save_idAlreadyExists_generatesNewId() { + // Arrange + every { groupTokenServiceMock.existsByName(any()) } returns false + every { groupTokenServiceMock.existsById(any()) } returnsMany listOf(true, false) + every { groupTokenServiceMock.save(any()) } returns groupToken + every { groupLogicMock.getById(any()) } returns group + + val anotherGroupTokenId = UUID.nameUUIDFromBytes("Another unit test token".toByteArray()) + + // Act + withMockRandomUUID(listOf(groupTokenId, anotherGroupTokenId)) { + groupTokenLogic.save(groupTokenSaveDto) + } + + // Assert + verify { + groupTokenServiceMock.save(match { + it.id == anotherGroupTokenId + }) + } + } + @Test fun save_normalBehavior_addsIdToEnabledTokensCache() { // Arrange every { groupTokenServiceMock.existsByName(any()) } returns false + every { groupTokenServiceMock.existsById(any()) } returns false every { groupTokenServiceMock.save(any()) } returns groupToken every { groupLogicMock.getById(any()) } returns group // Act - groupTokenLogic.save(groupTokenSaveDto) + withMockRandomUUID { + groupTokenLogic.save(groupTokenSaveDto) + } // Assert assertContains(enabledTokenCache, groupTokenIdStr) @@ -207,23 +255,27 @@ class DefaultGroupTokenLogicTest { } @Test - fun deleteById_normalBehavior_callsService() { + fun deleteById_normalBehavior_savesDeletedTokenInService() { // Arrange - every { groupTokenServiceMock.deleteById(any()) } just runs + every { groupTokenLogic.getById(any()) } answers { groupToken } + every { groupTokenServiceMock.save(any()) } answers { firstArg() } // Act groupTokenLogic.deleteById(groupTokenIdStr) // Assert verify { - groupTokenServiceMock.deleteById(groupTokenId) + groupTokenServiceMock.save(match { + it.id == groupTokenId && it.isDeleted + }) } } @Test fun deleteById_normalBehavior_removesIdFromEnabledTokensCache() { // Arrange - every { groupTokenServiceMock.deleteById(any()) } just runs + every { groupTokenLogic.getById(any()) } answers { groupToken } + every { groupTokenServiceMock.save(any()) } answers { firstArg() } // Act groupTokenLogic.deleteById(groupTokenIdStr) @@ -231,4 +283,16 @@ class DefaultGroupTokenLogicTest { // Assert assertFalse(enabledTokenCache.contains(groupTokenIdStr)) } + + private fun withMockRandomUUID(uuids: List? = null, block: () -> Unit) { + mockkStatic(UUID::class) { + if (uuids == null) { + every { UUID.randomUUID() } returns groupTokenId + } else { + every { UUID.randomUUID() } returnsMany uuids + } + + block() + } + } }