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
15 changed files with 76 additions and 56 deletions
Showing only changes of commit a10737a7f7 - 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
3
@@ -71,6 +71,16 @@ class UserRepository(
return entities.firstOrNull()?.toDomain()
}
override fun findByEmail(email: String): User? {
val entities =
jdbc.query(
"SELECT id, name, email, password, provider, provider_id, created_at FROM users WHERE email = ?",
userEntityRowMapper,
email,
)
return entities.firstOrNull()?.toDomain()
}
override fun findAll(): List<User> =
jdbc
.query(
@@ -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() ?: ""
@@ -68,6 +68,7 @@ class FilmLibraryController(
getFilmLibraryUseCase.getLibrary(
GetFilmLibraryQuery(userId = userId),
)
val film = getFilmByIdUseCase.getById(library.filmId)
return listOf(FilmResponse.fromDomain(film))
}
@@ -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?
@@ -36,7 +36,6 @@ class UserService(
id = idGenerator.generateId(),
name = command.name,
email = command.email,
password = "",
library = null,
)
return userRepository.save(user)
@@ -6,6 +6,5 @@ data class User(
val id: UUID,
val name: String,
val email: String,
val password: String,
val library: FilmLibrary?,
)
@@ -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);