Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Google OAuth2 sign-in: new POST /api/v1/auth/google endpoint, OAuthRequest DTO, User fields provider/googleId/pictureUrl, Google ID token decoding via a JwtDecoder, SHA-256 refresh-token hashing, OpenAPI docs, config/dependency changes, interface updates, and unit tests for Google sign-in flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as AuthController
participant Service as AuthService
participant Decoder as JwtDecoder
participant Repo as UserRepository
participant DB as Database
Client->>Controller: POST /api/v1/auth/google (idToken)
Controller->>Service: googleSignIn(idToken)
Service->>Decoder: decode(idToken)
Decoder-->>Service: Jwt (claims)
Service->>Service: validate issuer & audience
Service->>Repo: findByGoogleId(googleId)
alt user found
Service->>Service: update pictureUrl if needed
else not found
Service->>Repo: findByEmail(email)
alt email matches existing
Service->>Repo: link googleId, update pictureUrl
else
Service->>Repo: save(new user with provider/googleId/pictureUrl)
end
end
Service->>Service: generate access & refresh tokens (SHA-256 hashed refresh)
Service-->>Controller: JwtResponse
Controller-->>Client: ApiResponse<JwtResponse> (200)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/smf/api-docs.yaml`:
- Around line 219-226: The OpenAPI schema for OAuthRequest currently marks
idToken as required but allows an empty string; update the OAuthRequest object
so the idToken property includes minLength: 1 (e.g., add minLength: 1 under
idToken) to mirror the server DTO's `@NotBlank` constraint and ensure generated
clients enforce non-empty tokens.
- Around line 901-920: The operation for the Google sign-in endpoint
(/api/v1/auth/google, operationId googleSignIn) is currently inheriting the
top-level bearerAuth requirement; make this operation explicitly public by
adding security: [] at the operation level so codegen/Swagger treats it as
unauthenticated. Update the POST operation object for googleSignIn to include a
top-level security: [] entry directly under the operation (alongside tags,
summary, requestBody, responses) to override the global security requirement.
In `@src/main/java/com/smf/Application.java`:
- Around line 10-12: The static initializer in Application unconditionally calls
Dotenv.configure().load(), which throws when no .env exists; change the
initializer to call Dotenv.configure().ignoreIfMissing().load() so the .env file
is optional while still allowing environment variables from the host—update the
static block in the Application class where Dotenv.configure().load() is
invoked.
In `@src/main/java/com/smf/service/auth/AuthService.java`:
- Around line 83-84: The code now assumes persisted refreshTokenHash values are
SHA-256 hex strings (using sha256Hash) which will break older bcrypt-stored
tokens; update AuthService to perform migration-safe matching by adding a helper
(e.g., matchesRefreshToken(String refreshToken, String storedHash)) that returns
false for null, checks if storedHash startsWith("$2") and uses
passwordEncoder.matches(refreshToken, storedHash) for bcrypt, otherwise compares
sha256Hash(refreshToken) to storedHash; replace direct usages of String hash =
sha256Hash(refreshToken); and any equality checks in refresh() and logout() (and
the other spots noted) to call matchesRefreshToken(...) so both bcrypt and
sha256 hashes are supported.
- Around line 171-176: googleSignIn currently constructs a new NimbusJwtDecoder
on every call via googleJwtDecoder() inside the `@Transactional` method, causing
network JWKS fetches to block the DB transaction; fix by either moving the token
verification out of the transactional boundary (call JwtDecoder.decode(...)
before entering googleSignIn's `@Transactional` section) or by making the
JwtDecoder a cached class-level field (initialize a single
JwtDecoder/NimbusJwtDecoder once and reuse it) so googleJwtDecoder() returns the
shared instance and leverages its internal JWKS cache; update references to
googleJwtDecoder(), JwtDecoder, and googleSignIn() accordingly.
- Around line 183-222: Add issuer, email_verified and null/blank checks before
using JWT claims: after validating audience, verify jwt.getIssuer() equals
"https://accounts.google.com" or "accounts.google.com", ensure
Boolean.TRUE.equals(jwt.getClaimAsBoolean("email_verified")), and check
jwt.getSubject() (googleId) and jwt.getClaimAsString("email") are
non-null/non-blank; if any check fails throw AppError(HttpStatus.UNAUTHORIZED,
"..."). Update the flow around jwt, googleId, email and the userRepo calls
(findByGoogleId, findByEmail) so linking/creation only happens after these
validations.
In `@src/main/resources/application.properties`:
- Around line 32-35: Replace the hardcoded web client ID property
(google.client-id-web) with an environment-backed value and remove unsafe dummy
defaults for android/ios so all three client IDs are loaded consistently from
environment variables; specifically change google.client-id-web to reference
${GOOGLE_CLIENT_ID_WEB} (like google.client-id-android uses
${GOOGLE_CLIENT_ID_ANDROID}) and remove the fallback defaults from
google.client-id-android and google.client-id-ios (or replace them with
empty/no-default forms) so missing env vars fail visibly, and add/update a
.env.example documenting GOOGLE_CLIENT_ID_WEB, GOOGLE_CLIENT_ID_ANDROID, and
GOOGLE_CLIENT_ID_IOS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2fcd6947-913d-47f8-ade8-d759b6b3dff8
📒 Files selected for processing (12)
.gitignoredocs/smf/api-docs.yamlpom.xmlsrc/main/java/com/smf/Application.javasrc/main/java/com/smf/controller/AuthController.javasrc/main/java/com/smf/dto/auth/OAuthRequest.javasrc/main/java/com/smf/model/User.javasrc/main/java/com/smf/repo/UserRepository.javasrc/main/java/com/smf/service/auth/AuthService.javasrc/main/java/com/smf/service/auth/IAuthService.javasrc/main/resources/application.propertiessrc/test/java/com/smf/service/auth/AuthServiceTest.java
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/java/com/smf/service/auth/AuthService.java (1)
85-86:⚠️ Potential issue | 🟠 MajorKeep refresh-token matching backward compatible during the hash migration.
If this release is not meant to log out every existing session,
refresh()andlogout()still assume every persistedrefreshTokenHashis a SHA-256 hex string. Any pre-deploy bcrypt-backed token will start failing here until the user logs in again.Also applies to: 142-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/smf/service/auth/AuthService.java` around lines 85 - 86, refresh() and logout() currently assume persisted refreshTokenHash is a SHA-256 hex string; to preserve backward compatibility with bcrypt-backed tokens, detect the stored hash type (e.g., startsWith("$2"), "$2a", "$2b", "$2y") and if it’s a bcrypt hash use BCrypt.checkpw(refreshToken, refreshTokenHash) (or equivalent BCrypt.matches) to verify, otherwise fall back to computing sha256Hash(refreshToken) and comparing to refreshTokenHash; update both refresh() and logout() to use this dual-check logic and keep sha256Hash(...) as the fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/smf/service/auth/AuthService.java`:
- Around line 225-243: In AuthService's Google sign-in flow (the findByGoogleId
-> findByEmail branch) don't unconditionally call u.setGoogleId(googleId);
instead, when you get a User from userRepo.findByEmail(email) check
u.getGoogleId(): if it's null (or equals the incoming googleId) then set
u.setGoogleId(googleId) and u.setProvider("GOOGLE") as currently done; if it
exists and differs, do not relink — return an error or create a new User record
(i.e. treat as conflict) to avoid reassigning an account tied to another Google
subject; update the code around findByEmail(...) and setGoogleId calls in
AuthService accordingly.
- Around line 175-183: In googleSignIn, jwt.getIssuer() can be null leading to
an NPE when calling .toString(); add a null guard before calling .toString()
(e.g., check jwt.getIssuer() != null) and handle the null case by throwing the
same AppError(HttpStatus.UNAUTHORIZED, "Invalid Google ID token") or another
appropriate error; update the use sites in googleSignIn where
jwt.getIssuer().toString() is used so you first read issuer = jwt.getIssuer()
and validate non-null before converting to string or proceeding.
In `@src/test/java/com/smf/service/auth/AuthServiceTest.java`:
- Around line 326-340: buildGoogleJwt in AuthServiceTest always sets issuer to
"https://accounts.google.com" and email_verified to true, preventing easy
construction of negative JWT cases; modify buildGoogleJwt(String subject, String
email, String name, String picture, String clientId) to accept optional
parameters (or overloads) for issuer and emailVerified (e.g., add parameters
issuer and Boolean emailVerified or create an overloaded
buildGoogleJwtWithClaims) and use those values when calling .issuer(...) and
.claim("email_verified", ...), keeping existing default behavior by providing
the original values when the new params are null so existing tests remain
unchanged and negative-path tests can pass custom invalid claims.
---
Duplicate comments:
In `@src/main/java/com/smf/service/auth/AuthService.java`:
- Around line 85-86: refresh() and logout() currently assume persisted
refreshTokenHash is a SHA-256 hex string; to preserve backward compatibility
with bcrypt-backed tokens, detect the stored hash type (e.g., startsWith("$2"),
"$2a", "$2b", "$2y") and if it’s a bcrypt hash use BCrypt.checkpw(refreshToken,
refreshTokenHash) (or equivalent BCrypt.matches) to verify, otherwise fall back
to computing sha256Hash(refreshToken) and comparing to refreshTokenHash; update
both refresh() and logout() to use this dual-check logic and keep
sha256Hash(...) as the fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fb0ae2a6-3c14-41d4-9059-a45ca6069995
📒 Files selected for processing (2)
src/main/java/com/smf/service/auth/AuthService.javasrc/test/java/com/smf/service/auth/AuthServiceTest.java
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/main/java/com/smf/service/auth/AuthService.java (2)
199-201:⚠️ Potential issue | 🟡 MinorGuard issuer nullability before calling
.toString().
jwt.getIssuer().toString()can throw an NPE whenissis missing, returning 500 instead of 401.💡 Proposed fix
- String issuer = jwt.getIssuer().toString(); + String issuer = jwt.getIssuer() != null ? jwt.getIssuer().toString() : null; if (!"https://accounts.google.com".equals(issuer) && !"accounts.google.com".equals(issuer)) { throw new AppError(HttpStatus.UNAUTHORIZED, "Invalid Google ID token issuer"); }In Spring Security's Jwt API, can Jwt.getIssuer() be null when the `iss` claim is absent?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/smf/service/auth/AuthService.java` around lines 199 - 201, The code calls jwt.getIssuer().toString() without guarding for null, which can throw NPE when the iss claim is absent; update the AuthService validation to first retrieve the issuerClaim (e.g., var issuerClaim = jwt.getIssuer()), check for null (or use Optional) and treat a null/missing issuer as unauthorized (return 401), and only call toString() or compare when issuerClaim != null (for example compare issuerClaim.toString() against "https://accounts.google.com" and "accounts.google.com" or use Objects.equals to avoid NPE).
85-89:⚠️ Potential issue | 🟠 MajorAdd migration-safe refresh-token hash matching.
This now assumes all persisted hashes are SHA-256. Pre-existing bcrypt-backed refresh tokens will fail refresh/logout immediately after deploy unless forced logout is intentional.
💡 Proposed fix
+ private boolean matchesRefreshToken(String refreshToken, String storedHash) { + if (storedHash == null) return false; + if (storedHash.startsWith("$2")) { + return passwordEncoder.matches(refreshToken, storedHash); + } + return sha256Hash(refreshToken).equals(storedHash); + }- || !sha256Hash(refreshToken).equals(user.getRefreshTokenHash())) { + || !matchesRefreshToken(refreshToken, user.getRefreshTokenHash())) {- if (user.getRefreshTokenHash() == null - || !sha256Hash(refreshToken).equals(user.getRefreshTokenHash())) { + if (!matchesRefreshToken(refreshToken, user.getRefreshTokenHash())) {Also applies to: 142-143, 164-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/smf/service/auth/AuthService.java` around lines 85 - 89, The code unconditionally uses sha256Hash when storing and comparing refresh tokens (see sha256Hash and user.setRefreshTokenHash) which will break existing bcrypt-stored tokens; update the refresh-token logic to be migration-safe by: when validating a presented refresh token, detect the stored hash format (e.g., bcrypt hash prefix like "$2") and if bcrypt, verify with bcrypt; if verification succeeds, optionally re-hash and replace user.setRefreshTokenHash with the new SHA-256 value (or keep bcrypt) and persist user.setRefreshTokenId/user.setRefreshTokenExpiry as before; apply this detection-and-fallback pattern in all places where sha256Hash is used for storing/verifying (including the other occurrences referenced near lines 142-143 and 164-165).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/smf/controller/GlobalExceptionHandler.java`:
- Around line 77-80: The handler method
handleMethodNotAllowed(HttpRequestMethodNotSupportedException ex) returns 405
but uses the message "Endpoint not found"; update the ApiResponse message to
accurately reflect a method-not-allowed error (e.g., "HTTP method not allowed"
or "Method not allowed: <method>") and optionally include ex.getMethod() or
ex.getSupportedMethods() in the message to provide actionable details; modify
the ApiResponse construction in GlobalExceptionHandler to use this corrected
message while keeping the HttpStatus.METHOD_NOT_ALLOWED response.
In `@src/main/java/com/smf/service/auth/AuthService.java`:
- Around line 237-240: googleSignIn and register create new User instances
without any roles, which leaves User.roles empty and yields zero authorities in
AppUserDetails.buildUserDetails; update both googleSignIn(...) and register(...)
to assign a default role (e.g., "ROLE_USER") when constructing or immediately
after creating a new User so they mirror UserService.createUser() behavior,
ensuring newUser.getRoles() contains at least the default role before returning
or passing to buildUserDetails().
In `@src/main/java/com/smf/service/user/UserService.java`:
- Around line 137-141: In UserService where you build UserResponse (the block
creating new UserResponse and calling response.setRoles(...)), make the roles
mapping null-safe by guarding against user.getRoles() being null: if null, set
an empty Set; otherwise stream user.getRoles(), map each Role r to
r.getRoleName(), and collect to a Set. Update the response.setRoles(...) call to
use this null-safe value so legacy or partially-initialized users don't cause a
NullPointerException.
- Around line 36-45: The createUser flow in UserService allows creating LOCAL
accounts with null/blank passwords; update createUser to validate that when
request.getProvider() is null or equals the LOCAL provider (e.g., Provider.LOCAL
or string "LOCAL") the request.getPassword() must be non-null and non-blank and
otherwise throw a suitable exception (e.g., IllegalArgumentException or your
BadRequestException); only call passwordEncoder.encode(request.getPassword())
after that validation and proceed to construct User with the encoded password,
leaving the existing provider.setProvider(request.getProvider()) logic
unchanged.
---
Duplicate comments:
In `@src/main/java/com/smf/service/auth/AuthService.java`:
- Around line 199-201: The code calls jwt.getIssuer().toString() without
guarding for null, which can throw NPE when the iss claim is absent; update the
AuthService validation to first retrieve the issuerClaim (e.g., var issuerClaim
= jwt.getIssuer()), check for null (or use Optional) and treat a null/missing
issuer as unauthorized (return 401), and only call toString() or compare when
issuerClaim != null (for example compare issuerClaim.toString() against
"https://accounts.google.com" and "accounts.google.com" or use Objects.equals to
avoid NPE).
- Around line 85-89: The code unconditionally uses sha256Hash when storing and
comparing refresh tokens (see sha256Hash and user.setRefreshTokenHash) which
will break existing bcrypt-stored tokens; update the refresh-token logic to be
migration-safe by: when validating a presented refresh token, detect the stored
hash format (e.g., bcrypt hash prefix like "$2") and if bcrypt, verify with
bcrypt; if verification succeeds, optionally re-hash and replace
user.setRefreshTokenHash with the new SHA-256 value (or keep bcrypt) and persist
user.setRefreshTokenId/user.setRefreshTokenExpiry as before; apply this
detection-and-fallback pattern in all places where sha256Hash is used for
storing/verifying (including the other occurrences referenced near lines 142-143
and 164-165).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d81d15f4-0818-4312-9fa8-f70c3913d660
📒 Files selected for processing (6)
src/main/java/com/smf/controller/GlobalExceptionHandler.javasrc/main/java/com/smf/controller/HttpRequestMethodNotAllowedException.javasrc/main/java/com/smf/dto/user/UserRequest.javasrc/main/java/com/smf/dto/user/UserResponse.javasrc/main/java/com/smf/service/auth/AuthService.javasrc/main/java/com/smf/service/user/UserService.java
💤 Files with no reviewable changes (1)
- src/main/java/com/smf/controller/HttpRequestMethodNotAllowedException.java
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/main/java/com/smf/service/user/UserService.java (2)
144-144:⚠️ Potential issue | 🟡 MinorMake role mapping null-safe in response conversion.
Line 144 can throw if
user.getRoles()is null (legacy/partially initialized rows).💡 Proposed fix
- response.setRoles(user.getRoles().stream().map(r -> r.getRoleName()).collect(java.util.stream.Collectors.toSet())); + response.setRoles( + user.getRoles() == null + ? Set.of() + : user.getRoles().stream() + .map(Role::getRoleName) + .collect(java.util.stream.Collectors.toSet()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/smf/service/user/UserService.java` at line 144, The call in UserService converting roles into the response via response.setRoles(user.getRoles().stream().map(r -> r.getRoleName()).collect(...)) is not null-safe and will NPE when user.getRoles() is null; update the conversion to guard against null (e.g., treat null as empty collection) before streaming so response.setRoles(...) always receives a non-null set; locate the mapping in UserService (response.setRoles(...), user.getRoles(), r.getRoleName()) and replace it with a null-safe variant that returns Collections.emptySet() when roles are null.
43-45:⚠️ Potential issue | 🔴 CriticalAvoid encoding null passwords in OAuth/non-LOCAL create flows.
Line 43 encodes
request.getPassword()unconditionally. For non-LOCAL providers, this can fail when password is absent and block OIDC account creation.💡 Proposed fix
- String encodedPassword = passwordEncoder.encode(request.getPassword()); + String rawPassword = request.getPassword(); + String encodedPassword = + (rawPassword != null && !rawPassword.isBlank()) + ? passwordEncoder.encode(rawPassword) + : null;To verify impact, check encoder config and OAuth create-user call paths (expect to find non-LOCAL user creation paths where password is not set):
#!/bin/bash set -euo pipefail echo "== PasswordEncoder definitions ==" rg -n --type=java -C3 'PasswordEncoder|BCryptPasswordEncoder|Argon2PasswordEncoder|Pbkdf2PasswordEncoder' src/main/java || true echo echo "== createUser call sites ==" rg -n --type=java -C4 '\bcreateUser\s*\(' src/main/java || true echo echo "== OAuth/UserRequest construction (password/provider/google fields) ==" rg -n --type=java -C4 'new UserRequest|setPassword\s*\(|setProvider\s*\(|setGoogleId\s*\(' src/main/java || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/smf/service/user/UserService.java` around lines 43 - 45, The code unconditionally calls passwordEncoder.encode(request.getPassword()) which fails for OAuth/non-LOCAL flows where password is absent; update the logic in UserService (the method that constructs the User) to only call passwordEncoder.encode(...) when request.getPassword() is non-null/non-blank and the provider indicates a LOCAL sign-up (e.g., check request.getPassword() != null && !request.getPassword().isBlank() && request.getProvider() == Provider.LOCAL or equivalent); otherwise set encodedPassword to null (or leave it unset) and pass that into the User constructor so OIDC/OAuth create flows do not attempt to encode a missing password. Ensure any downstream code that expects a password can handle a null/empty password.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/com/smf/service/user/UserService.java`:
- Line 144: The call in UserService converting roles into the response via
response.setRoles(user.getRoles().stream().map(r ->
r.getRoleName()).collect(...)) is not null-safe and will NPE when
user.getRoles() is null; update the conversion to guard against null (e.g.,
treat null as empty collection) before streaming so response.setRoles(...)
always receives a non-null set; locate the mapping in UserService
(response.setRoles(...), user.getRoles(), r.getRoleName()) and replace it with a
null-safe variant that returns Collections.emptySet() when roles are null.
- Around line 43-45: The code unconditionally calls
passwordEncoder.encode(request.getPassword()) which fails for OAuth/non-LOCAL
flows where password is absent; update the logic in UserService (the method that
constructs the User) to only call passwordEncoder.encode(...) when
request.getPassword() is non-null/non-blank and the provider indicates a LOCAL
sign-up (e.g., check request.getPassword() != null &&
!request.getPassword().isBlank() && request.getProvider() == Provider.LOCAL or
equivalent); otherwise set encodedPassword to null (or leave it unset) and pass
that into the User constructor so OIDC/OAuth create flows do not attempt to
encode a missing password. Ensure any downstream code that expects a password
can handle a null/empty password.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 71acbafa-d914-4496-b686-6eb132873bfd
📒 Files selected for processing (1)
src/main/java/com/smf/service/user/UserService.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Bug Fixes