Role based access#3
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces a Spring Boot authentication system with JWT-based security. It adds a Spring Boot application entry point, configures HTTP security with JWT filtering and CORS, defines JWT token utilities, creates a registration request DTO, updates domain entities with ChangesJWT Authentication & Security Configuration
Sequence DiagramsequenceDiagram
participant Client
participant JwtFilter
participant JwtUtils
participant SecurityConfig
participant SecurityContext
participant Backend
Client->>JwtFilter: HTTP Request + Authorization: Bearer <token>
JwtFilter->>JwtFilter: Extract token from header
JwtFilter->>JwtUtils: Validate token (extractUsername, extractRole, isTokenExpired)
JwtUtils-->>JwtFilter: Return username, role, validity status
alt Token Valid
JwtFilter->>SecurityContext: Create UsernamePasswordAuthenticationToken<br/>with ROLE_{role} authority
JwtFilter->>SecurityContext: Set authentication
JwtFilter->>Backend: Continue filter chain
Backend-->>Client: Process authorized request
else Token Invalid/Expired
JwtFilter->>JwtFilter: Log warning, skip auth set
JwtFilter->>Backend: Continue filter chain (unauthenticated)
Backend->>SecurityConfig: Check authorization rules
alt Public Endpoint
Backend-->>Client: Allow access
else Protected Endpoint
Backend-->>Client: 403/401 Unauthorized
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
src/main/java/com/saccoplus/service/impl/AuthServiceImpl.java (1)
11-14: Stub implementation -requestparameter is unused.The
registermethod ignores theRegisterRequestparameter and returns a hardcoded string. When implementing the actual registration logic, note thatRegisterRequestcurrently lacks getters/setters to access field values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/saccoplus/service/impl/AuthServiceImpl.java` around lines 11 - 14, The register method in AuthServiceImpl currently ignores the RegisterRequest parameter and returns a hardcoded string; update AuthServiceImpl.register to consume RegisterRequest (validate inputs, extract username/email/password, call user creation/persistence logic and return an appropriate result or token) and remove the stubbed return; if RegisterRequest lacks accessors, add proper getters/setters or make its fields accessible (e.g., getEmail(), getPassword(), getUsername()) so AuthServiceImpl can read the values, perform validation, hashing, and persistence via your User repository/service..idea/misc.xml (1)
1-12: Exclude IDE-specific files from version control.
.idea/directory contains user-specific IDE settings and should typically be added to.gitignoreto avoid polluting the repository with local configuration changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.idea/misc.xml around lines 1 - 12, The .idea/misc.xml file (containing components like ExternalStorageConfigurationManager and ProjectRootManager) is IDE-specific and should be excluded from version control; update the repository by adding an entry to .gitignore to ignore the .idea/ directory (or at least .idea/misc.xml), remove the tracked file from git history with git rm --cached (or git rm -r --cached .idea) and commit the change so the file is no longer tracked, and then push the updated commit containing the new .gitignore.target/classes/application.yml (1)
1-108: Removetarget/classes/application.ymlfrom version control.This is build output, so edits here are overwritten on the next build and can drift from the real source config. The canonical file should live under
src/main/resources, andtarget/should stay ignored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@target/classes/application.yml` around lines 1 - 108, The committed build artifact target/classes/application.yml should be removed from version control and ignored; delete target/classes/application.yml from the repo (git rm --cached or remove and commit) and add the target/ (or target/classes/) pattern to .gitignore so it isn’t re-added, ensuring the canonical config lives at src/main/resources/application.yml and that file is tracked instead.
🤖 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/saccoplus/config/SecurityConfig.java`:
- Around line 31-61: The security chain in SecurityConfig is missing CORS setup
so preflight requests are blocked; update the HttpSecurity configuration to call
.cors(Customizer.withDefaults()) (e.g., insert .cors(Customizer.withDefaults())
into the http builder before authorizeHttpRequests) and add a
CorsConfigurationSource bean that reads the app.cors.allowed-origins property
from configuration and constructs the CorsConfiguration (allowedOrigins,
allowedMethods, allowedHeaders, allowCredentials) so Spring Security applies
CORS before jwtFilter/authorization; reference SecurityConfig, jwtFilter, and
the CorsConfigurationSource bean when implementing this.
In `@src/main/java/com/saccoplus/dto/request/RegisterRequest.java`:
- Line 8: Change the RegisterRequest.nationalId field from int to String and
update any related getters/setters/constructors or builders in the
RegisterRequest class to use String; also adjust any validation/parsing logic
that assumed numeric input (e.g., length checks) and ensure call sites that call
IndividualUserRepository.existsByNationalId(...) pass the String nationalId
directly (no numeric conversions) so leading zeros and non-numeric characters
are preserved.
- Around line 3-11: The RegisterRequest DTO currently has private fields
(firstName, lastName, phone, nationalId, password) but no accessors, so
deserialization and callers will fail; fix by adding either Lombok annotations
(e.g., `@Data` or `@Getter/`@Setter) to the RegisterRequest class or implement
explicit public getters and setters for each field (getFirstName/setFirstName,
getLastName/setLastName, getPhone/setPhone, getNationalId/setNationalId,
getPassword/setPassword) so Jackson and other code can read/write the
properties.
In `@src/main/java/com/saccoplus/entity/IndividualUser.java`:
- Around line 28-29: The nationalId field in IndividualUser is declared as int
but should be a String to preserve leading zeros and match repository method
existsByNationalId(String); change the field type of nationalId in the
IndividualUser class from int to String, update its `@Column` annotation to
include unique = true and an appropriate length (or nullable as needed), and add
optional validation annotations (e.g., `@Size` or `@Pattern`) to enforce format
constraints; also scan for any usages/constructors/getters/setters referring to
nationalId and update their types/signatures accordingly to String so they align
with the repository method existsByNationalId(String).
In `@src/main/java/com/saccoplus/entity/Wallet.java`:
- Line 17: Replace the primitive double balance in class Wallet with
java.math.BigDecimal to avoid rounding errors: change the field type (balance),
update its getter/setter (getBalance/setBalance) and any constructors or methods
that mutate it (e.g., deposit/withdraw if present) to accept/return BigDecimal,
and annotate the JPA mapping for balance with an explicit precision and scale
(e.g., `@Column`(precision=19, scale=4)) so the database column stores fixed-point
amounts; also update equals/hashCode/toString uses and any code that performs
arithmetic to use BigDecimal methods (add/subtract) instead of double
arithmetic.
In `@src/main/java/com/saccoplus/security/JwtFilter.java`:
- Around line 44-60: JwtFilter currently trusts jwtUtils.extractRole(token) and
builds a SimpleGrantedAuthority("ROLE_" + role) even when role is null/blank;
update JwtFilter to require both a non-null, non-blank role and a non-null
username before creating the SimpleGrantedAuthority and
UsernamePasswordAuthenticationToken and setting the SecurityContextHolder, e.g.,
call jwtUtils.extractRole(token), trim/check isEmpty (or isBlank) and only
proceed to create new SimpleGrantedAuthority("ROLE_" + role) and set
authentication when the role passes validation; if role is missing/blank, do not
set the SecurityContext and handle/log the invalid token case accordingly.
In `@src/main/java/com/saccoplus/security/JwtUtils.java`:
- Around line 18-21: The getSigningKey() method is incorrectly calling
Base64.getEncoder().encode(...) on SECRET_KEY; replace that call with the
correct handling: if SECRET_KEY is a Base64-encoded string, decode it using
Base64.getDecoder().decode(SECRET_KEY) before passing to Keys.hmacShaKeyFor,
otherwise (for a raw secret) pass SECRET_KEY.getBytes(StandardCharsets.UTF_8)
directly; update getSigningKey() to remove Base64.getEncoder().encode and use
the appropriate bytes source for Keys.hmacShaKeyFor.
- Around line 42-44: isTokenValid currently only compares the extracted username
and ignores token expiry; update JwtUtils.isTokenValid to also verify the token
is not expired by calling the existing expiration helper (e.g.,
extractExpiration or isTokenExpired) for the given token and confirming the
expiration is after now before returning true; reference JwtUtils.isTokenValid
and the helper used to extract/validate expiration (extractExpiration /
isTokenExpired) so the method returns true only when username matches AND token
is not expired.
- Line 15: The SECRET_KEY field in JwtUtils is currently an empty hardcoded
string which will cause WeakKeyException at runtime; replace this with a
configuration-injected value (e.g., use `@Value` or constructor injection to read
a jwt.secret property) in the JwtUtils class, validate that the injected secret
is present and meets required length (>= 256 bits for HMAC-SHA) and throw a
clear exception during startup if invalid, and update any places that reference
SECRET_KEY to use the injected value (or its validated byte[] form) instead of
the empty literal.
In `@src/main/java/com/saccoplus/service/AuthService.java`:
- Around line 3-5: The interface AuthService is missing the register method that
AuthServiceImpl overrides; add a matching method declaration to AuthService
(e.g., the register signature used in AuthServiceImpl.register) so the interface
and implementation align; ensure the method name, parameter types and return
type match AuthServiceImpl.register() exactly to resolve the compilation error.
In `@target/classes/application.yml`:
- Around line 4-5: Remove fallback defaults in application.yml so profile and
JWT cannot silently "fail open": stop using ${SPRING_PROFILES_ACTIVE:dev} and
any default for JWT (e.g., ${JWT_SECRET:...}), require callers to set
SPRING_PROFILES_ACTIVE and JWT_SECRET exactly (no colon/default) and ensure
dangerous defaults like spring.jpa.hibernate.ddl-auto: update and debug logging
are not enabled by default in the file; instead set safe production defaults or
omit those keys so environments must explicitly configure ddl-auto and logging
levels. Update references for spring.profiles.active and JWT_SECRET (and any
spring.jpa.hibernate.ddl-auto/logging.level entries) to remove inline defaults
and document that these env vars are mandatory.
---
Nitpick comments:
In @.idea/misc.xml:
- Around line 1-12: The .idea/misc.xml file (containing components like
ExternalStorageConfigurationManager and ProjectRootManager) is IDE-specific and
should be excluded from version control; update the repository by adding an
entry to .gitignore to ignore the .idea/ directory (or at least .idea/misc.xml),
remove the tracked file from git history with git rm --cached (or git rm -r
--cached .idea) and commit the change so the file is no longer tracked, and then
push the updated commit containing the new .gitignore.
In `@src/main/java/com/saccoplus/service/impl/AuthServiceImpl.java`:
- Around line 11-14: The register method in AuthServiceImpl currently ignores
the RegisterRequest parameter and returns a hardcoded string; update
AuthServiceImpl.register to consume RegisterRequest (validate inputs, extract
username/email/password, call user creation/persistence logic and return an
appropriate result or token) and remove the stubbed return; if RegisterRequest
lacks accessors, add proper getters/setters or make its fields accessible (e.g.,
getEmail(), getPassword(), getUsername()) so AuthServiceImpl can read the
values, perform validation, hashing, and persistence via your User
repository/service.
In `@target/classes/application.yml`:
- Around line 1-108: The committed build artifact target/classes/application.yml
should be removed from version control and ignored; delete
target/classes/application.yml from the repo (git rm --cached or remove and
commit) and add the target/ (or target/classes/) pattern to .gitignore so it
isn’t re-added, ensuring the canonical config lives at
src/main/resources/application.yml and that file is tracked instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8af79a7-7e11-47a9-a2d0-746c0e96c851
⛔ Files ignored due to path filters (7)
target/classes/com/saccoplus/dto/request/RegisterRequest.classis excluded by!**/*.classtarget/classes/com/saccoplus/entity/IndividualUser.classis excluded by!**/*.classtarget/classes/com/saccoplus/entity/Role.classis excluded by!**/*.classtarget/classes/com/saccoplus/entity/Wallet.classis excluded by!**/*.classtarget/classes/com/saccoplus/repository/IndividualUserRepository.classis excluded by!**/*.classtarget/classes/com/saccoplus/security/JwtUtils.classis excluded by!**/*.classtarget/classes/com/saccoplus/service/AuthService.classis excluded by!**/*.class
📒 Files selected for processing (14)
.idea/misc.xmlsrc/main/java/com/saccoplus/SaccoPlusApplication.javasrc/main/java/com/saccoplus/config/SecurityConfig.javasrc/main/java/com/saccoplus/dto/request/RegisterRequest.javasrc/main/java/com/saccoplus/entity/IndividualUser.javasrc/main/java/com/saccoplus/entity/Role.javasrc/main/java/com/saccoplus/entity/Wallet.javasrc/main/java/com/saccoplus/repository/IndividualUserRepository.javasrc/main/java/com/saccoplus/security/JwtFilter.javasrc/main/java/com/saccoplus/security/JwtUtils.javasrc/main/java/com/saccoplus/service/AuthService.javasrc/main/java/com/saccoplus/service/impl/AuthServiceImpl.javatarget/classes/application.ymltarget/classes/templates/email/.gitkeep
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/saccoplus/security/JwtFilter.java`:
- Around line 44-45: Wrap the calls to jwtUtils.extractUsername(token) and
jwtUtils.extractRole(token) in a try/catch that catches JJWT parsing exceptions
(e.g. io.jsonwebtoken.JwtException) and IllegalArgumentException so
malformed/expired/tampered tokens do not propagate; on catch, log at debug/info
and simply do not set an Authentication in JwtFilter (allow the filter chain to
continue as unauthenticated) instead of throwing a 500. Ensure the try/catch
surrounds both extractUsername and extractRole (which delegate to
parseClaimsJws) and only set SecurityContext authentication when no exception
occurs and token validation succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33ea61d2-7287-4498-bfa2-db76df3698db
📒 Files selected for processing (1)
src/main/java/com/saccoplus/security/JwtFilter.java
…, RegisterRequest, AuthService, Wallet BigDecimal
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/com/saccoplus/config/SecurityConfig.java (2)
30-31: 💤 Low valueRemove leftover development comment.
The inline comment
// ← ADD THISappears to be a development reminder that should be cleaned up before merging.♻️ Proposed fix
- `@Value`("${app.cors.allowed-origins}") // ← ADD THIS + `@Value`("${app.cors.allowed-origins}") private String allowedOrigins;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/saccoplus/config/SecurityConfig.java` around lines 30 - 31, Remove the leftover inline development comment by deleting the trailing "// ← ADD THIS" on the allowedOrigins field in the SecurityConfig class; ensure the field declaration remains as a clean annotation and private String allowedOrigins; so only the `@Value`("${app.cors.allowed-origins}") annotation and the private String allowedOrigins; line remain.
82-87: ⚡ Quick winConsider supporting multiple comma-separated origins.
List.of(allowedOrigins)treats the entire string as one origin. IfCORS_ORIGINScontains comma-separated values like"http://a.com,http://b.com", this will fail to match any request origin. The current setup works for single-origin deployments, but consider splitting for flexibility.♻️ Proposed fix to split comma-separated origins
+import java.util.Arrays; ... `@Bean` public CorsConfigurationSource corsConfigurationSource() { CorsConfiguration configuration = new CorsConfiguration(); - configuration.setAllowedOrigins(List.of(allowedOrigins)); + configuration.setAllowedOrigins( + Arrays.asList(allowedOrigins.split("\\s*,\\s*")) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/saccoplus/config/SecurityConfig.java` around lines 82 - 87, The corsConfigurationSource method currently calls List.of(allowedOrigins) which treats the entire CORS_ORIGINS string as one origin; update corsConfigurationSource to split the allowedOrigins string on commas (and trim whitespace) and pass the resulting List<String> into configuration.setAllowedOrigins(...) so multiple comma-separated origins (e.g. "http://a.com,http://b.com") are recognized; refer to the corsConfigurationSource method and the allowedOrigins field/variable when making this change.
🤖 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/saccoplus/security/JwtUtils.java`:
- Around line 20-22: The getSigningKey method uses SECRET_KEY.getBytes() which
depends on the platform default charset; change it to use an explicit charset
(e.g., StandardCharsets.UTF_8) so the key bytes are consistent across
JVMs—update the getSigningKey() implementation to convert SECRET_KEY to bytes
using a fixed charset reference and import/use java.nio.charset.StandardCharsets
to avoid platform-dependent signature failures.
---
Nitpick comments:
In `@src/main/java/com/saccoplus/config/SecurityConfig.java`:
- Around line 30-31: Remove the leftover inline development comment by deleting
the trailing "// ← ADD THIS" on the allowedOrigins field in the SecurityConfig
class; ensure the field declaration remains as a clean annotation and private
String allowedOrigins; so only the `@Value`("${app.cors.allowed-origins}")
annotation and the private String allowedOrigins; line remain.
- Around line 82-87: The corsConfigurationSource method currently calls
List.of(allowedOrigins) which treats the entire CORS_ORIGINS string as one
origin; update corsConfigurationSource to split the allowedOrigins string on
commas (and trim whitespace) and pass the resulting List<String> into
configuration.setAllowedOrigins(...) so multiple comma-separated origins (e.g.
"http://a.com,http://b.com") are recognized; refer to the
corsConfigurationSource method and the allowedOrigins field/variable when making
this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69179706-a989-410c-9ee2-720058e3c667
📒 Files selected for processing (7)
.gitignoresrc/main/java/com/saccoplus/config/SecurityConfig.javasrc/main/java/com/saccoplus/dto/request/RegisterRequest.javasrc/main/java/com/saccoplus/entity/IndividualUser.javasrc/main/java/com/saccoplus/entity/Wallet.javasrc/main/java/com/saccoplus/security/JwtUtils.javasrc/main/java/com/saccoplus/service/AuthService.java
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- src/main/java/com/saccoplus/entity/Wallet.java
- src/main/java/com/saccoplus/entity/IndividualUser.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/saccoplus/service/AuthService.java
- src/main/java/com/saccoplus/dto/request/RegisterRequest.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/com/saccoplus/security/JwtFilter.java (1)
68-71: ⚡ Quick winNarrow the
catchclause to avoid silencing programming errors in the filter body.
catch (Exception e)intercepts not only JWT parsing failures (JwtException,IllegalArgumentException) but also anyNullPointerExceptionor other bugs introduced inside thetryblock. These get logged as"Invalid JWT token"and silently swallowed, making them very hard to diagnose.♻️ Proposed refactor
- } catch (Exception e) { - - logger.warn("Invalid JWT token: " + e.getMessage()); + } catch (io.jsonwebtoken.JwtException | IllegalArgumentException e) { + logger.warn("Invalid JWT token: " + e.getMessage()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/saccoplus/security/JwtFilter.java` around lines 68 - 71, The catch-all Exception in JwtFilter is hiding programming errors; narrow it to only JWT/parsing failures by replacing catch (Exception e) with a multi-catch for the JWT-related exceptions (e.g., catch (io.jsonwebtoken.JwtException | IllegalArgumentException e)) so only token parsing errors are logged via logger.warn("Invalid JWT token: " + e.getMessage()), and allow other runtime exceptions (NPEs, etc.) to propagate rather than being swallowed in the JwtFilter try block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/com/saccoplus/security/JwtFilter.java`:
- Around line 68-71: The catch-all Exception in JwtFilter is hiding programming
errors; narrow it to only JWT/parsing failures by replacing catch (Exception e)
with a multi-catch for the JWT-related exceptions (e.g., catch
(io.jsonwebtoken.JwtException | IllegalArgumentException e)) so only token
parsing errors are logged via logger.warn("Invalid JWT token: " +
e.getMessage()), and allow other runtime exceptions (NPEs, etc.) to propagate
rather than being swallowed in the JwtFilter try block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4b1bafe-ed99-4dd2-8cf4-217b234dc6f6
📒 Files selected for processing (1)
src/main/java/com/saccoplus/security/JwtFilter.java
Summary by CodeRabbit
Release Notes