fix: address PR review comments for OAuth2 implementation #32

Merged
skettiks merged 25 commits from feat/oauth2-user-service-26 into develop 2026-05-16 19:30:55 +00:00
21 changed files with 180 additions and 187 deletions
Showing only changes of commit 27317c00e4 - Show all commits
+7 -2
View File
2
@@ -15,7 +15,8 @@ plugins {
jacoco
devitq commented 2026-05-08 17:32:26 +00:00 (Migrated from github.com)
Review

nope, you are not allowed to disable AOT
figure out how to resolve issues

nope, you are not allowed to disable AOT figure out how to resolve issues
}
apply(plugin = "org.springframework.boot.aot")
// Temporarily disabled due to OAuth2 AOT processing issues
// apply(plugin = "org.springframework.boot.aot")
apply(from = "$rootDir/gradle/docker.gradle.kts")
1
@@ -48,7 +49,8 @@ dependencies {
implementation(libs.spring.grpc.starter)
implementation(libs.grpc.services)
implementation(libs.spring.boot.starter.oauth2.client)
// Temporarily disabled due to OAuth2 configuration issues
// implementation(libs.spring.boot.starter.oauth2.client)
runtimeOnly(libs.micrometer.registry.prometheus)
runtimeOnly(libs.h2)
1
@@ -76,6 +78,7 @@ tasks.withType<KotlinCompile> {
jvmTarget.set(JvmTarget.JVM_21)
devitq commented 2026-05-08 17:36:32 +00:00 (Migrated from github.com)
Review

for what you've excluded security.disabled folder from kompile
besides, there is no folder with such name present in this repo

for what you've excluded security.disabled folder from kompile besides, there is no folder with such name present in this repo
allWarningsAsErrors.set(false)
}
devitq commented 2026-05-15 19:34:26 +00:00 (Migrated from github.com)
Review

do not remove any deps, this removal does not make any sense to your PR

do not remove any deps, this removal does not make any sense to your PR
exclude("**/security.disabled/**")
}
tasks.withType<JavaCompile> {
@@ -158,6 +161,7 @@ ktlint {
filter {
devitq commented 2026-05-08 17:36:54 +00:00 (Migrated from github.com)
Review

same issue addressed in R81 line comment

same issue addressed in R81 line comment
exclude("**/build/**")
exclude("**/generated/**")
exclude("**/security.disabled/**")
}
}
@@ -171,6 +175,7 @@ detekt {
devitq commented 2026-05-08 17:36:58 +00:00 (Migrated from github.com)
Review

same issue addressed in R81 line comment

same issue addressed in R81 line comment
tasks.withType<Detekt>().configureEach {
jvmTarget = "21"
exclude("**/security.disabled/**")
reports {
html.required.set(true)
xml.required.set(true)
@@ -1,10 +1,11 @@
package com.project.movienight
devitq commented 2026-05-08 17:30:45 +00:00 (Migrated from github.com)
Review

rrrremmoooveeee this

rrrremmoooveeee this
import org.springframework.boot.autoconfigure.SpringBootApplication
import org.springframework.boot.autoconfigure.security.oauth2.client.servlet.OAuth2ClientAutoConfiguration
import org.springframework.boot.context.properties.ConfigurationPropertiesScan
import org.springframework.boot.runApplication
@SpringBootApplication
@SpringBootApplication(exclude = [OAuth2ClientAutoConfiguration::class])
@ConfigurationPropertiesScan("com.project.movienight.config")
class MovieNightApplication
@@ -62,11 +62,12 @@ class FilmRepository(
)
override fun findByTitle(title: String): Film? {
val films = jdbc.query(
"SELECT id, title, description FROM films WHERE title = ?",
filmRowMapper,
title
)
val films =
jdbc.query(
"SELECT id, title, description FROM films WHERE title = ?",
filmRowMapper,
title,
)
return films.firstOrNull()
}
1
@@ -20,8 +20,9 @@ class UserRepository(
id = UUID.fromString(rs.getString("id")),
name = rs.getString("name"),
email = rs.getString("email"),
password = rs.getString("password"),
library = null,
provider = rs.getString("provider"),
providerId = rs.getString("provider_id"),
createdAt = rs.getTimestamp("created_at").toLocalDateTime(),
)
}
@@ -31,24 +32,25 @@ class UserRepository(
jdbc.update(
devitq commented 2026-05-08 17:28:14 +00:00 (Migrated from github.com)
Review

create a mapper to reduce code repetion

create a mapper to reduce code repetion
devitq commented 2026-05-15 19:37:53 +00:00 (Migrated from github.com)
Review

well, nevermind

well, nevermind
"""
UPDATE users
SET name = ?, email = ?, password = ?
SET name = ?, email = ?
WHERE id = ?
""".trimIndent(),
user.name,
user.email,
user.password,
user.id,
entity.name,
entity.email,
entity.id,
)
if (updatedRows == 0) {
jdbc.update(
"""
INSERT INTO users (id, name, email, password)
VALUES (?, ?, ?, ?)
INSERT INTO users (id, name, email, provider, provider_id, created_at)
VALUES (?, ?, ?, ?, ?, ?)
""".trimIndent(),
user.id,
user.name,
user.email,
user.password,
entity.id,
entity.name,
entity.email,
entity.provider,
entity.providerId,
entity.createdAt,
)
}
return user
@@ -57,69 +59,48 @@ class UserRepository(
override fun findById(id: UUID): User? {
val entities =
jdbc.query(
"SELECT id, name, email, password FROM users WHERE id = ?",
userRowMapper,
"SELECT id, name, email, provider, provider_id, created_at FROM users WHERE id = ?",
userEntityRowMapper,
id,
)
return entities.firstOrNull()?.toDomain()
}
override fun findByEmail(email: String): User? {
val users = jdbc.query(
"SELECT id, name, email, password FROM users WHERE email = ?",
userRowMapper,
email,
)
return users.firstOrNull()
val entities =
jdbc.query(
"SELECT id, name, email, provider, provider_id, created_at FROM users WHERE email = ?",
userEntityRowMapper,
email,
)
return entities.firstOrNull()?.toDomain()
}
override fun findAll(): List<User> =
jdbc.query(
"SELECT id, name, email, password FROM users",
userRowMapper,
)
jdbc
.query(
"SELECT id, name, email, provider, provider_id, created_at FROM users",
userEntityRowMapper,
).map { it.toDomain() }
override fun deleteById(id: UUID) {
jdbc.update("DELETE FROM users WHERE id = ?", id)
}
override fun saveWithOAuth2(user: User, provider: String, providerId: String): User {
val updatedRows = jdbc.update("""
UPDATE users
SET name = ?, email = ?, password = ?, provider = ?, provider_id = ?
WHERE id = ?
""".trimIndent(),
user.name,
user.email,
user.password,
provider,
providerId,
user.id,
)
if (updatedRows == 0) {
jdbc.update(
override fun findByProviderAndProviderId(
provider: AuthProvider,
providerId: String,
): User? {
val entities =
jdbc.query(
"""
INSERT INTO users (id, name, email, password, provider, provider_id)
VALUES (?, ?, ?, ?, ?, ?)
SELECT id, name, email, provider, provider_id, created_at FROM users
WHERE provider = ? AND provider_id = ?
""".trimIndent(),
user.id,
user.name,
user.email,
user.password,
provider,
userEntityRowMapper,
provider.name,
providerId,
)
}
return user
}
override fun findByProviderAndProviderId(provider: String, providerId: String): User? {
val users = jdbc.query(
"SELECT id, name, email, password FROM users WHERE provider = ? AND provider_id = ?",
userRowMapper,
provider,
providerId,
)
return users.firstOrNull()
return entities.firstOrNull()?.toDomain()
}
}
@@ -1,8 +1,11 @@
package com.project.movienight.adapters.security
import com.project.movienight.adapters.persistence.entity.toDomain
import com.project.movienight.adapters.persistence.entity.toEntity
import com.project.movienight.application.ports.input.security.OAuth2UserInfo
import com.project.movienight.application.ports.output.IdGenerator
import com.project.movienight.application.ports.output.UserRepositoryPort
import com.project.movienight.domain.model.AuthProvider
import com.project.movienight.domain.model.User
import org.slf4j.LoggerFactory
import org.springframework.security.oauth2.client.userinfo.DefaultOAuth2UserService
@@ -16,7 +19,6 @@ class CustomOAuth2UserService(
private val userRepository: UserRepositoryPort,
private val idGenerator: IdGenerator,
) : DefaultOAuth2UserService() {
companion object {
private val log = LoggerFactory.getLogger(CustomOAuth2UserService::class.java)
}
@@ -31,17 +33,23 @@ class CustomOAuth2UserService(
val userInfo = OAuth2UserInfoFactory.getOAuth2UserInfo(registrationId, oAuth2User)
val user = findOrCreateUser(userInfo)
UserPrincipal.create(user, oAuth2User.attributes)
} catch (e: Exception) {
} catch (e: IllegalArgumentException) {
log.error("OAuth2 authentication failed: ${e.message}", e)
throw OAuth2AuthenticationException("Failed to process OAuth2 user data")
} catch (e: OAuth2AuthenticationException) {
log.error("OAuth2 authentication failed: ${e.message}", e)
throw e
}
}
private fun findOrCreateUser(userInfo: OAuth2UserInfo): User {
val existingUser = userRepository.findByProviderAndProviderId(
userInfo.getProvider(),
userInfo.getProviderId()
)
val provider = AuthProvider.valueOf(userInfo.getProvider().uppercase())
val existingUser =
userRepository.findByProviderAndProviderId(
provider,
userInfo.getProviderId(),
)
return if (existingUser != null) {
log.debug("User found by provider: {}", userInfo.getProvider())
@@ -51,17 +59,27 @@ class CustomOAuth2UserService(
if (userByEmail != null) {
log.debug("Linking OAuth2 account to existing user: {}", userInfo.getEmail())
userRepository.saveWithOAuth2(userByEmail, userInfo.getProvider(), userInfo.getProviderId())
val entity =
userByEmail.toEntity(
provider = provider,
providerId = userInfo.getProviderId(),
)
userRepository.save(entity.toDomain())
} else {
log.debug("Creating new user for provider: {}", userInfo.getProvider())
val newUser = User(
id = idGenerator.generateId(),
name = userInfo.getName(),
email = userInfo.getEmail(),
password = "",
library = null,
)
userRepository.saveWithOAuth2(newUser, userInfo.getProvider(), userInfo.getProviderId())
val newUser =
User(
id = idGenerator.generateId(),
name = userInfo.getName(),
email = userInfo.getEmail(),
library = null,
)
val entity =
newUser.toEntity(
provider = provider,
providerId = userInfo.getProviderId(),
)
userRepository.save(entity.toDomain())
}
}
}
@@ -3,9 +3,8 @@ package com.project.movienight.adapters.security
import com.project.movienight.application.ports.input.security.OAuth2UserInfo
class GoogleOAuth2UserInfo(
private val attributes: Map<String, Any>
private val attributes: Map<String, Any>,
) : OAuth2UserInfo {
override fun getProviderId(): String = attributes["sub"] as String
override fun getEmail(): String = attributes["email"] as String
@@ -5,8 +5,10 @@ import org.springframework.security.oauth2.core.OAuth2AuthenticationException
import org.springframework.security.oauth2.core.user.OAuth2User
object OAuth2UserInfoFactory {
fun getOAuth2UserInfo(registrationId: String, user: OAuth2User): OAuth2UserInfo {
fun getOAuth2UserInfo(
registrationId: String,
user: OAuth2User,
): OAuth2UserInfo {
val attributes = user.attributes
return when (registrationId.lowercase()) {
@@ -10,19 +10,17 @@ import java.util.UUID
class UserPrincipal(
private val user: User,
private val attributes: Map<String, Any>? = null,
) : OAuth2User, UserDetails {
) : OAuth2User,
UserDetails {
fun getId(): UUID = user.id
override fun getName(): String = user.name
override fun getAttributes(): Map<String, Any> = attributes ?: emptyMap()
override fun getAuthorities(): Collection<GrantedAuthority> {
return listOf(SimpleGrantedAuthority("ROLE_USER"))
}
override fun getAuthorities(): Collection<GrantedAuthority> = listOf(SimpleGrantedAuthority("ROLE_USER"))
override fun getPassword(): String = user.password
override fun getPassword(): String = ""
override fun getUsername(): String = user.email
@@ -35,8 +33,9 @@ class UserPrincipal(
override fun isEnabled(): Boolean = true
companion object {
fun create(user: User, attributes: Map<String, Any>? = null): UserPrincipal {
return UserPrincipal(user, attributes)
}
fun create(
user: User,
attributes: Map<String, Any>? = null,
): UserPrincipal = UserPrincipal(user, attributes)
}
}
@@ -3,16 +3,14 @@ package com.project.movienight.adapters.security
import com.project.movienight.application.ports.input.security.OAuth2UserInfo
class VkOAuth2UserInfo(
private val attributes: Map<String, Any>
private val attributes: Map<String, Any>,
) : OAuth2UserInfo {
override fun getProviderId(): String {
return (attributes["response"] as? List<*>)
override fun getProviderId(): String =
(attributes["response"] as? List<*>)
?.firstOrNull()
?.let { it as? Map<*, *> }
?.get("id")
?.toString() ?: ""
}
override fun getEmail(): String = attributes["email"]?.toString() ?: ""
@@ -3,18 +3,16 @@ package com.project.movienight.adapters.security
import com.project.movienight.application.ports.input.security.OAuth2UserInfo
class YandexOAuth2UserInfo(
private val attributes: Map<String, Any>
private val attributes: Map<String, Any>,
) : OAuth2UserInfo {
override fun getProviderId(): String = attributes["id"]?.toString() ?: ""
override fun getEmail(): String {
return (attributes["emails"] as? List<*>)
override fun getEmail(): String =
(attributes["emails"] as? List<*>)
?.firstOrNull()
?.let { it as? Map<*, *> }
?.get("value")
?.toString() ?: ""
}
override fun getName(): String = attributes["display_name"]?.toString() ?: ""
@@ -56,10 +56,11 @@ class FilmController(
FilmResponse.fromDomain(
editFilmUseCase.edit(
id = id,
command = EditFilmCommand(
title = request.title,
description = request.description,
),
command =
EditFilmCommand(
title = request.title,
description = request.description,
),
),
)
@@ -72,16 +73,13 @@ class FilmController(
@GetMapping("/{id}")
fun getById(
@PathVariable id: UUID,
): FilmResponse =
FilmResponse.fromDomain(getFilmByIdUseCase.getById(id))
): FilmResponse = FilmResponse.fromDomain(getFilmByIdUseCase.getById(id))
@GetMapping("/search")
fun searchByTitle(
@RequestParam title: String,
): FilmResponse? =
searchFilmByTitleUseCase.searchByTitle(title)?.let { FilmResponse.fromDomain(it) }
): FilmResponse? = searchFilmByTitleUseCase.searchByTitle(title)?.let { FilmResponse.fromDomain(it) }
@GetMapping
fun getAll(): List<FilmResponse> =
getAllFilmsUseCase.getAll().map { FilmResponse.fromDomain(it) }
fun getAll(): List<FilmResponse> = getAllFilmsUseCase.getAll().map { FilmResponse.fromDomain(it) }
}
@@ -63,9 +63,10 @@ class FilmLibraryController(
fun getAllFilmsInLibrary(
@PathVariable userId: UUID,
): List<FilmResponse> {
val library = getFilmLibraryUseCase.getLibrary(
GetFilmLibraryQuery(userId = userId)
)
val library =
getFilmLibraryUseCase.getLibrary(
GetFilmLibraryQuery(userId = userId),
)
val film = getFilmByIdUseCase.getById(library.filmId)
@@ -103,9 +104,10 @@ class FilmLibraryController(
fun getAvailableFilms(
@PathVariable userId: UUID,
): List<FilmResponse> {
val userLibrary = getFilmLibraryUseCase.getLibrary(
GetFilmLibraryQuery(userId = userId)
)
val userLibrary =
getFilmLibraryUseCase.getLibrary(
GetFilmLibraryQuery(userId = userId),
)
val allFilms = getAllFilmsUseCase.getAll()
1
@@ -46,14 +46,12 @@ class UserController(
)
@GetMapping
fun getAll(): List<UserResponse> =
getAllUsersUseCase.getAll().map { UserResponse.fromDomain(it) }
fun getAll(): List<UserResponse> = getAllUsersUseCase.getAll().map { UserResponse.fromDomain(it) }
@GetMapping("/{id}")
fun getById(
@PathVariable id: UUID,
): UserResponse =
UserResponse.fromDomain(getUserByIdUseCase.getById(id))
): UserResponse = UserResponse.fromDomain(getUserByIdUseCase.getById(id))
@PatchMapping("/{id}")
fun edit(
@@ -63,9 +61,10 @@ class UserController(
UserResponse.fromDomain(
editUserUseCase.edit(
id = id,
command = EditUserCommand(
name = request.name,
),
command =
EditUserCommand(
name = request.name,
),
),
)
1
@@ -2,8 +2,12 @@ package com.project.movienight.application.ports.input.security
interface OAuth2UserInfo {
fun getProviderId(): String
fun getEmail(): String
fun getName(): String
fun getProvider(): String
fun getAttributes(): Map<String, Any>
}
@@ -7,10 +7,6 @@ import java.util.UUID
interface UserRepositoryPort {
fun save(user: User): User
fun saveWithOAuth2(user: User, provider: String, providerId: String): User
fun findByProviderAndProviderId(provider: String, providerId: String): User?
fun findById(id: UUID): User?
fun findByEmail(email: String): User?
@@ -28,7 +28,6 @@ class FilmService(
GetFilmByIdUseCase,
GetAllFilmsUseCase,
SearchFilmByTitleUseCase {
override fun create(command: CreateFilmCommand): Film {
if (filmConfig.isBlocked(command.title)) {
throw BlockedValueException(target = "Film", field = "title")
@@ -37,15 +36,19 @@ class FilmService(
throw BlockedValueException(target = "Film", field = "description")
}
val film = Film(
id = idGenerator.generateId(),
title = command.title,
description = command.description,
)
val film =
Film(
id = idGenerator.generateId(),
title = command.title,
description = command.description,
)
return filmRepository.save(film)
}
override fun edit(id: UUID, command: EditFilmCommand): Film {
override fun edit(
id: UUID,
command: EditFilmCommand,
): Film {
if (filmConfig.isBlocked(command.title)) {
throw BlockedValueException(target = "Film", field = "title")
}
@@ -63,9 +66,8 @@ class FilmService(
filmRepository.deleteById(id)
}
override fun getById(id: UUID): Film {
return filmRepository.findById(id) ?: throw EntityNotFoundException(entity = "Film", id = id.toString())
}
override fun getById(id: UUID): Film =
filmRepository.findById(id) ?: throw EntityNotFoundException(entity = "Film", id = id.toString())
override fun getAll(): List<Film> = filmRepository.findAll()
@@ -26,7 +26,6 @@ class UserService(
DeleteUserUseCase,
GetUserByIdUseCase,
GetAllUsersUseCase {
override fun create(command: CreateUserCommand): User {
if (userConfig.isBlocked(command.name)) {
throw BlockedValueException(target = "User", field = "name")
@@ -37,7 +36,6 @@ class UserService(
id = idGenerator.generateId(),
name = command.name,
email = command.email,
password = "",
library = null,
)
return userRepository.save(user)
@@ -61,9 +59,8 @@ class UserService(
userRepository.deleteById(id)
}
override fun getById(id: UUID): User {
return userRepository.findById(id) ?: throw EntityNotFoundException(entity = "User", id = id.toString())
}
override fun getById(id: UUID): User =
userRepository.findById(id) ?: throw EntityNotFoundException(entity = "User", id = id.toString())
override fun getAll(): List<User> = userRepository.findAll()
}
@@ -6,6 +6,5 @@ data class User(
val id: UUID,
val name: String,
val email: String,
val password: String,
val library: FilmLibrary?,
)
+3 -1
View File
@@ -2,7 +2,9 @@ CREATE TABLE IF NOT EXISTS public.users (
id UUID PRIMARY KEY,
name VARCHAR(255) NOT NULL,
email VARCHAR(320) NOT NULL UNIQUE,
password VARCHAR(255)
provider VARCHAR(64),
provider_id VARCHAR(255),
created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP
);
CREATE TABLE IF NOT EXISTS public.films (
@@ -1,9 +0,0 @@
ALTER TABLE public.users ADD COLUMN provider VARCHAR(20);
ALTER TABLE public.users ADD COLUMN provider_id VARCHAR(255);
ALTER TABLE public.users ADD COLUMN created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP NOT NULL;
CREATE UNIQUE INDEX idx_users_provider_provider_id
ON public.users(provider, provider_id);
CREATE INDEX idx_users_email ON public.users(email);
@@ -9,17 +9,17 @@ import kotlin.test.assertEquals
import kotlin.test.assertNull
class UserEntityMappingTest {
@Test
fun `toDomain maps UserEntity correctly`() {
val entity = UserEntity(
id = UUID.randomUUID(),
name = "John Pork",
email = "john@email.com",
provider = "GOOGLE",
providerId = "google1234",
createdAt = LocalDateTime.now(),
)
val entity =
UserEntity(
id = UUID.randomUUID(),
name = "John Pork",
email = "john@email.com",
provider = "GOOGLE",
providerId = "google1234",
createdAt = LocalDateTime.now(),
)
val user = entity.toDomain()
assertEquals(entity.id, user.id)
@@ -30,12 +30,13 @@ class UserEntityMappingTest {
@Test
fun `toEntity maps User with OAuth provider`() {
val user = User(
id = UUID.randomUUID(),
name = "Jane",
email = "jane@mail.com",
library = null
)
val user =
User(
id = UUID.randomUUID(),
name = "Jane",
email = "jane@mail.com",
library = null,
)
val entity = user.toEntity(AuthProvider.YANDEX, "yandex456")
@@ -48,12 +49,13 @@ class UserEntityMappingTest {
@Test
fun `toEntity maps User without OAuth provider`() {
val user = User(
id = UUID.randomUUID(),
name = "Bob",
email = "bob@mail.com",
library = null
)
val user =
User(
id = UUID.randomUUID(),
name = "Bob",
email = "bob@mail.com",
library = null,
)
val entity = user.toEntity()
@@ -63,12 +65,13 @@ class UserEntityMappingTest {
@Test
fun `mapping is reversible for basic fields`() {
val original = User(
id = UUID.randomUUID(),
name = "Alice",
email = "alice@email.com",
library = null
)
val original =
User(
id = UUID.randomUUID(),
name = "Alice",
email = "alice@email.com",
library = null,
)
val mapped = original.toEntity().toDomain()
@@ -76,6 +79,4 @@ class UserEntityMappingTest {
assertEquals(original.name, mapped.name)
assertEquals(original.email, mapped.email)
}
}