Feat/spring security setup#193
Merged
Merged
Conversation
- Implements generating JWT tokens with claims for user authentication. - Adds decoding and validation of tokens with HS256 algorithm. - Exposes JwtDecoder as a bean for Spring Security integration.
- Implements a filter to authenticate HTTP requests using JWT tokens. - Extracts and validates tokens from the Authorization header. - Loads user details and sets authentication in SecurityContext.
- Added SecurityFilterChain to define security rules, including JWT-based authentication and CORS configuration. - Introduced AuthenticationManager, JwtDecoder, and DaoAuthenticationProvider beans. - Enabled stateless session handling for RESTful API requests.
…gration - Provides a bridge between Spring Security and the database. - Loads user details by email for JWT-based authentication. - Logs user lookup actions and handles not found scenarios.
- Provides a bridge between Spring Security and the database. - Loads user details by email for JWT-based authentication. - Logs user lookup actions and handles not found scenarios.
annikaholmqvist94
requested review from
TatjanaTrajkovic,
johanbriger and
lindaeskilsson
April 11, 2026 16:27
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 54 seconds. ⌛ 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds JWT-based authentication and wiring: new JwtService, JwtProperties, JwtAuthenticationFilter, CustomUserDetailsService, SecurityConfig updates (stateless, CORS, provider beans), pom dependency for oauth2 resource server, application properties, controller signatures switched to Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Filter as JwtAuthenticationFilter
participant JwtSvc as JwtService
participant UserSvc as CustomUserDetailsService
participant SecurityCtx as SecurityContextHolder
participant Chain as FilterChain
Client->>Filter: HTTP request with Authorization: Bearer <token>
Filter->>JwtSvc: decode(token)
alt token valid
JwtSvc-->>Filter: Jwt (claims with subject/email)
Filter->>UserSvc: loadUserByUsername(email)
UserSvc-->>Filter: UserDetails
Filter->>SecurityCtx: setAuthentication(UsernamePasswordAuthenticationToken)
else token invalid/expired
JwtSvc-->>Filter: JwtException
Filter-->>Filter: log warning (no auth set)
end
Filter->>Chain: continue filter chain
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…ordControllerTest
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/main/java/org/example/vet1177/security/SecurityConfig.java (1)
137-148: Externalize CORS allowed origins for production deployments.The hardcoded localhost origins will block legitimate frontend requests in production. Consider moving these to configuration properties.
💡 Suggested approach
Add to
application.properties:cors.allowed-origins=http://localhost:5173,http://localhost:3000Then inject and use in the config:
+ `@Value`("${cors.allowed-origins}") + private List<String> allowedOrigins; + `@Bean` public CorsConfigurationSource corsConfigurationSource() { CorsConfiguration config = new CorsConfiguration(); - config.setAllowedOrigins(List.of("http://localhost:5173", "http://localhost:3000")); + config.setAllowedOrigins(allowedOrigins);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/security/SecurityConfig.java` around lines 137 - 148, The corsConfigurationSource bean currently hardcodes allowed origins (config.setAllowedOrigins(...)), which breaks production; move origins to a configurable property by adding a comma-separated cors.allowed-origins entry in application.properties (or application.yml), inject it into SecurityConfig (e.g., via `@Value`("${cors.allowed-origins}") or a `@ConfigurationProperties` class) and parse/split into a List<String>, then replace the hardcoded List.of(...) call in corsConfigurationSource to use that injected list so UrlBasedCorsConfigurationSource registers runtime-configured origins for "/api/**".src/main/resources/application.properties (1)
32-34: Ensure the default secret key is never used in production.The fallback
default-dev-secret-key-change-in-production-min-32-chars!!is helpful for local development but poses a security risk if accidentally used in production. Consider adding validation inJwtServiceor a startup check to fail fast if the secret matches the default in non-dev profiles.💡 Example startup validation in JwtService
`@PostConstruct` public void validateConfiguration() { if (jwtProperties.getSecretKey().contains("default-dev-secret")) { log.warn("⚠️ Using default development JWT secret key. " + "Set JWT_SECRET environment variable for production!"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/application.properties` around lines 32 - 34, Add a startup validation in JwtService (e.g., a validateConfiguration or `@PostConstruct` method) that reads jwtProperties.getSecretKey() and checks whether it equals or contains the default-dev-secret fallback string from application.properties; if the active profile is not a development profile (check Environment or active profiles) then log a clear error and fail-fast (throw an exception or exit) to prevent startup with the default secret, otherwise log a warning for dev. Ensure the method references JwtService, jwtProperties, and the validation method name (validateConfiguration) so reviewers can find and verify the change.src/main/java/org/example/vet1177/security/JwtProperties.java (1)
11-22: Consider adding validation annotations.Adding Bean Validation annotations would catch configuration errors at startup rather than at runtime.
💡 Optional validation enhancement
+import jakarta.validation.constraints.NotBlank; +import jakarta.validation.constraints.Positive; +import org.springframework.validation.annotation.Validated; +@Validated `@ConfigurationProperties`(prefix = "jwt") public class JwtProperties { + `@NotBlank`(message = "JWT secret key must be configured") private String secretKey; + `@Positive`(message = "JWT expiration must be positive") private long expirationMs;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/security/JwtProperties.java` around lines 11 - 22, Annotate the JwtProperties class with javax validation by adding `@Validated` to the class and apply constraints: add `@NotBlank` on the secretKey field and `@Positive` (or `@Min`(1)) on expirationMs, keeping the existing getters/setters (class name JwtProperties, fields secretKey and expirationMs); also ensure the project includes spring-boot-starter-validation so configuration properties are validated at startup.
🤖 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/org/example/vet1177/security/CustomUserDetailsService.java`:
- Around line 44-48: The warning currently logs the raw email in
CustomUserDetailsService inside the userRepository.findByEmail(email)
orElseThrow lambda; remove or replace the raw email with a non-PII
representation (e.g., a deterministic hash or masked partial like user@***), and
update the log call to use that value instead of the email before throwing
UsernameNotFoundException; ensure the hashing/masking logic is implemented near
this code path and does not expose the original email in logs or exception
messages.
In `@src/main/java/org/example/vet1177/security/JwtAuthenticationFilter.java`:
- Around line 85-118: The JwtAuthenticationFilter currently only catches
JwtException but loadUserByUsername(email) can throw UsernameNotFoundException
causing a 500; update the filter to also catch UsernameNotFoundException (or
catch Exception around the userDetailsService.loadUserByUsername call) and
handle it the same way as an invalid token: log a warning (including the email
and exception message) and do not set the SecurityContext (leave authentication
null) so Spring returns 401; reference: JwtAuthenticationFilter,
loadUserByUsername, UsernamePasswordAuthenticationToken,
UsernameNotFoundException.
In `@src/main/java/org/example/vet1177/security/JwtService.java`:
- Around line 86-88: The log in JwtService that currently logs user.getEmail
when generating tokens should be changed to avoid PII exposure: update the log
statement in the token generation method (where
jwtEncoder.encode(params).getTokenValue() and log.info(...) appear) to either
remove the email entirely or log a non-PII identifier (e.g., user.getId() or a
generated requestId) instead, ensuring the message retains context but no
personal email is emitted.
- Around line 41-44: The SecretKey derivation in JwtService uses
jwtProperties.getSecretKey().getBytes() which relies on the platform default
charset; change it to explicitly use UTF-8 so the same key bytes are produced
across environments—update the SecretKeySpec creation in JwtService (the
SecretKey key = new SecretKeySpec(...)) to call getBytes(StandardCharsets.UTF_8)
(import or reference StandardCharsets.UTF_8) when converting
jwtProperties.getSecretKey().
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/security/JwtProperties.java`:
- Around line 11-22: Annotate the JwtProperties class with javax validation by
adding `@Validated` to the class and apply constraints: add `@NotBlank` on the
secretKey field and `@Positive` (or `@Min`(1)) on expirationMs, keeping the existing
getters/setters (class name JwtProperties, fields secretKey and expirationMs);
also ensure the project includes spring-boot-starter-validation so configuration
properties are validated at startup.
In `@src/main/java/org/example/vet1177/security/SecurityConfig.java`:
- Around line 137-148: The corsConfigurationSource bean currently hardcodes
allowed origins (config.setAllowedOrigins(...)), which breaks production; move
origins to a configurable property by adding a comma-separated
cors.allowed-origins entry in application.properties (or application.yml),
inject it into SecurityConfig (e.g., via `@Value`("${cors.allowed-origins}") or a
`@ConfigurationProperties` class) and parse/split into a List<String>, then
replace the hardcoded List.of(...) call in corsConfigurationSource to use that
injected list so UrlBasedCorsConfigurationSource registers runtime-configured
origins for "/api/**".
In `@src/main/resources/application.properties`:
- Around line 32-34: Add a startup validation in JwtService (e.g., a
validateConfiguration or `@PostConstruct` method) that reads
jwtProperties.getSecretKey() and checks whether it equals or contains the
default-dev-secret fallback string from application.properties; if the active
profile is not a development profile (check Environment or active profiles) then
log a clear error and fail-fast (throw an exception or exit) to prevent startup
with the default secret, otherwise log a warning for dev. Ensure the method
references JwtService, jwtProperties, and the validation method name
(validateConfiguration) so reviewers can find and verify the 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cd1fdc7-ef5d-46e4-9059-226bc8caa1a2
📒 Files selected for processing (7)
pom.xmlsrc/main/java/org/example/vet1177/security/CustomUserDetailsService.javasrc/main/java/org/example/vet1177/security/JwtAuthenticationFilter.javasrc/main/java/org/example/vet1177/security/JwtProperties.javasrc/main/java/org/example/vet1177/security/JwtService.javasrc/main/java/org/example/vet1177/security/SecurityConfig.javasrc/main/resources/application.properties
Comment on lines
+44
to
+48
| return userRepository.findByEmail(email) | ||
| .orElseThrow(() -> { | ||
| log.warn("User not found email={}", email); | ||
| return new UsernameNotFoundException("Användare hittades inte: " + email); | ||
| }); |
There was a problem hiding this comment.
Avoid logging email addresses in production logs.
Logging email in the warning message may violate privacy requirements (GDPR/CCPA) as emails are PII. Consider logging a hash or partial email, or removing the email from the log entirely.
🛡️ Suggested fix
return userRepository.findByEmail(email)
.orElseThrow(() -> {
- log.warn("User not found email={}", email);
+ log.warn("User not found for provided email");
return new UsernameNotFoundException("Användare hittades inte: " + email);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return userRepository.findByEmail(email) | |
| .orElseThrow(() -> { | |
| log.warn("User not found email={}", email); | |
| return new UsernameNotFoundException("Användare hittades inte: " + email); | |
| }); | |
| return userRepository.findByEmail(email) | |
| .orElseThrow(() -> { | |
| log.warn("User not found for provided email"); | |
| return new UsernameNotFoundException("Användare hittades inte: " + email); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/vet1177/security/CustomUserDetailsService.java`
around lines 44 - 48, The warning currently logs the raw email in
CustomUserDetailsService inside the userRepository.findByEmail(email)
orElseThrow lambda; remove or replace the raw email with a non-PII
representation (e.g., a deterministic hash or masked partial like user@***), and
update the log call to use that value instead of the email before throwing
UsernameNotFoundException; ensure the hashing/masking logic is implemented near
this code path and does not expose the original email in logs or exception
messages.
Comment on lines
+86
to
+88
| String token = jwtEncoder.encode(params).getTokenValue(); | ||
| log.info("Generated JWT for user email={}", user.getEmail()); | ||
| return token; |
There was a problem hiding this comment.
Avoid logging user email in token generation.
Similar to CustomUserDetailsService, logging the email may expose PII. Consider logging a user identifier or omitting this detail.
🛡️ Suggested fix
String token = jwtEncoder.encode(params).getTokenValue();
- log.info("Generated JWT for user email={}", user.getEmail());
+ log.info("Generated JWT for userId={}", user.getId());
return token;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| String token = jwtEncoder.encode(params).getTokenValue(); | |
| log.info("Generated JWT for user email={}", user.getEmail()); | |
| return token; | |
| String token = jwtEncoder.encode(params).getTokenValue(); | |
| log.info("Generated JWT for userId={}", user.getId()); | |
| return token; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/vet1177/security/JwtService.java` around lines 86 -
88, The log in JwtService that currently logs user.getEmail when generating
tokens should be changed to avoid PII exposure: update the log statement in the
token generation method (where jwtEncoder.encode(params).getTokenValue() and
log.info(...) appear) to either remove the email entirely or log a non-PII
identifier (e.g., user.getId() or a generated requestId) instead, ensuring the
message retains context but no personal email is emitted.
…chmentControllerTest - Integrated `UsernamePasswordAuthenticationToken` for authentication in `ActivityLogIntegrationTest`. - Added `JwtService` and `CustomUserDetailsService` mocks in `AttachmentControllerTest`.
- Log a warning when a valid JWT token references a non-existent user.
Contributor
Author
|
closes #61 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/example/vet1177/security/JwtService.java (1)
88-88:⚠️ Potential issue | 🟠 MajorAvoid logging user email during token generation.
Line 88 logs PII (
user.getEmail()). Please remove it or log a non-PII identifier (e.g., userId).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/security/JwtService.java` at line 88, The log statement in JwtService that calls log.info("Generated JWT for user email={}", user.getEmail()) exposes PII; change it to avoid email by logging a non-PII identifier instead (e.g., user.getId() or userId). Locate the log invocation in the generateToken/createToken method inside JwtService and replace the user.getEmail reference with the appropriate non-PII field (user.getId() or another stable identifier) and update the log message text accordingly (e.g., "Generated JWT for userId={}").
🧹 Nitpick comments (1)
src/test/java/org/example/vet1177/integration/activitylog/ActivityLogIntegrationTest.java (1)
139-142: Add a principal/header mismatch regression test.These cases only test matching
userIdheader + authenticated user. Please add one negative test where authenticated principal anduserIddiffer, and assert rejection (403/400). That protects against identity-spoofing regressions during the transition away from header-based identity.Based on learnings:
currentUserIdvia request data was a temporary placeholder until authentication existed, and should move to principal/JWT-derived identity.Also applies to: 171-174, 205-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/vet1177/integration/activitylog/ActivityLogIntegrationTest.java` around lines 139 - 142, Add a negative regression test in ActivityLogIntegrationTest that sends a request with header "userId" set to a different UUID than the authenticated principal's id and uses authentication(new UsernamePasswordAuthenticationToken(...)) with the real principal (e.g., use owner vs a differentUser); call the same endpoint used by the positive tests and assert the response is rejected (expect 403 or 400 as your controller returns) to ensure principal/header mismatch is forbidden. Implement this alongside the existing matching tests (the blocks that set .header("userId", owner.getId().toString()) and authentication(new UsernamePasswordAuthenticationToken(owner,...))) so you mirror the request construction but swap the header to differentUser.getId().toString() and assert the error response. Ensure the new test uses the same setup variables (owner, differentUser) and names the test to indicate "principal_header_mismatch_rejected".
🤖 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/org/example/vet1177/security/JwtService.java`:
- Around line 42-45: Validate the HS256 secret length in the JwtService
constructor before creating the SecretKey: call jwtProperties.getSecretKey(),
convert to bytes (StandardCharsets.UTF_8) and ensure length >= 32, and if not
throw an IllegalStateException (or IllegalArgumentException) with a clear
message so the app fails fast at startup; then proceed to create the
SecretKeySpec as before. Also ensure tests/configs use the correct property name
matching JwtProperties (jwt.secret-key) so the value is actually populated.
---
Duplicate comments:
In `@src/main/java/org/example/vet1177/security/JwtService.java`:
- Line 88: The log statement in JwtService that calls log.info("Generated JWT
for user email={}", user.getEmail()) exposes PII; change it to avoid email by
logging a non-PII identifier instead (e.g., user.getId() or userId). Locate the
log invocation in the generateToken/createToken method inside JwtService and
replace the user.getEmail reference with the appropriate non-PII field
(user.getId() or another stable identifier) and update the log message text
accordingly (e.g., "Generated JWT for userId={}").
---
Nitpick comments:
In
`@src/test/java/org/example/vet1177/integration/activitylog/ActivityLogIntegrationTest.java`:
- Around line 139-142: Add a negative regression test in
ActivityLogIntegrationTest that sends a request with header "userId" set to a
different UUID than the authenticated principal's id and uses authentication(new
UsernamePasswordAuthenticationToken(...)) with the real principal (e.g., use
owner vs a differentUser); call the same endpoint used by the positive tests and
assert the response is rejected (expect 403 or 400 as your controller returns)
to ensure principal/header mismatch is forbidden. Implement this alongside the
existing matching tests (the blocks that set .header("userId",
owner.getId().toString()) and authentication(new
UsernamePasswordAuthenticationToken(owner,...))) so you mirror the request
construction but swap the header to differentUser.getId().toString() and assert
the error response. Ensure the new test uses the same setup variables (owner,
differentUser) and names the test to indicate
"principal_header_mismatch_rejected".
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c048cc49-75aa-4ab6-bc46-748b23e10209
📒 Files selected for processing (5)
pom.xmlsrc/main/java/org/example/vet1177/security/JwtAuthenticationFilter.javasrc/main/java/org/example/vet1177/security/JwtService.javasrc/test/java/org/example/vet1177/controller/AttachmentControllerTest.javasrc/test/java/org/example/vet1177/integration/activitylog/ActivityLogIntegrationTest.java
✅ Files skipped from review due to trivial changes (1)
- pom.xml
- Ensure JWT secret key is at least 32 bytes to comply with HS256 requirements. - Update property name to `jwt.secret-key` for clarity in application-test.properties. Closes #61
johanbriger
approved these changes
Apr 13, 2026
…Controller - Simplify user context handling by using `@AuthenticationPrincipal` instead of `@RequestHeader`. - Remove unused `UserService` dependency. - Improve logging for pet-related endpoints.
- Replace `@RequestHeader` with `@AuthenticationPrincipal` for user context. - Remove unused `UserService` dependency.
- Added `UsernamePasswordAuthenticationToken` for mock authentication. - Imported `SecurityConfig` to configure security for tests. - Removed unused `UserService` mock.
- Removed redundant `userId` headers from test requests. - Deleted test case for missing `userId` header as it is no longer applicable.
- Created `API.md` covering detailed usage of backend API. - Included authentication methods, example requests, and responses. - Documented endpoints for pets, medical records, clinics, and comments. - Provided test data and instructions for local development.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/org/example/vet1177/security/JwtService.java (1)
87-89:⚠️ Potential issue | 🟠 MajorAvoid logging user email during token generation.
Line 88 logs PII (
userId) or remove the identifier entirely.🛡️ Proposed fix
- log.info("Generated JWT for user email={}", user.getEmail()); + log.info("Generated JWT for userId={}", user.getId());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/security/JwtService.java` around lines 87 - 89, The log statement in JwtService that runs immediately after jwtEncoder.encode(params).getTokenValue() currently logs the user's email (PII) — update the log to avoid PII by either removing the identifier or replacing user.getEmail() with a non-PII identifier such as user.getId() (or another internal userId), and ensure the change is applied to the log.info call that follows token creation so the message no longer contains the email.
🧹 Nitpick comments (4)
src/main/java/org/example/vet1177/security/JwtService.java (2)
57-57: Validatejwt.expiration-msis positive.A non-positive value will create instantly expired (or invalid) tokens. Add a startup guard for clearer misconfiguration handling.
🔧 Proposed hardening
- this.expirationMs = jwtProperties.getExpirationMs(); + this.expirationMs = jwtProperties.getExpirationMs(); + if (this.expirationMs <= 0) { + throw new IllegalStateException("jwt.expiration-ms must be > 0"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/security/JwtService.java` at line 57, Validate the jwt expiration configuration in the JwtService constructor: after reading jwtProperties.getExpirationMs() into the expirationMs field, check that expirationMs is > 0 and if not throw an IllegalStateException (or other startup-failing runtime exception) with a clear message mentioning the invalid jwt.expiration-ms value; update the JwtService constructor (where expirationMs is assigned) to perform this guard so misconfiguration fails fast at startup.
38-43: Fail fast with clear error whenjwt.secret-keyis missing/blank.Line 39 dereferences
jwtProperties.getSecretKey()directly. If config is absent, startup fails with a less actionable NPE instead of a clear configuration error.🔧 Proposed hardening
public JwtService(JwtProperties jwtProperties) { - byte[] keyBytes = jwtProperties.getSecretKey().getBytes(StandardCharsets.UTF_8); + String secret = jwtProperties.getSecretKey(); + if (secret == null || secret.isBlank()) { + throw new IllegalStateException("Missing required property: jwt.secret-key"); + } + byte[] keyBytes = secret.getBytes(StandardCharsets.UTF_8); if (keyBytes.length < 32) { throw new IllegalStateException( "JWT secret key must be at least 32 bytes for HS256, got " + keyBytes.length); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/security/JwtService.java` around lines 38 - 43, The JwtService constructor currently dereferences jwtProperties.getSecretKey() and can throw an NPE if the configuration is missing or blank; update the constructor to first validate the secret string (e.g., check for null or empty/blank via Objects.requireNonNull or StringUtils.hasText) and if invalid throw an IllegalStateException with a clear message that the configuration property "jwt.secret-key" is missing or blank; only after that convert to bytes and enforce the existing 32-byte minimum check so the validation flow in JwtService (constructor) is explicit and fails fast with a descriptive error.src/test/java/org/example/vet1177/integration/activitylog/ActivityLogIntegrationTest.java (1)
57-119: Remove commented-out test code.This large block of commented-out code appears to be the old version of
should_return_logs_for_owner_onlybefore the refactoring. Since the new implementation (lines 120-144) already covers this test case, this dead code should be removed to improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/vet1177/integration/activitylog/ActivityLogIntegrationTest.java` around lines 57 - 119, Delete the large commented-out test block for should_return_logs_for_owner_only inside ActivityLogIntegrationTest; remove the entire commented code (the old test setup and assertions) so only the current active test implementation remains, ensuring no leftover commented test code is kept in the class.API.md (1)
122-122: Inconsistent API path prefix.The endpoint
GET /pets/owner/{ownerId}(and other/petsendpoints in Section 3.3, 7) lacks the/apiprefix that other endpoints use (e.g.,/api/clinics,/api/medical-records). Verify this matches the actual controller mapping.Looking at the relevant code snippet from
PetController.java:`@RequestMapping`("/pets")This appears intentional, but the inconsistency between
/petsand/api/...endpoints may confuse frontend developers. Consider documenting why some endpoints use/apiprefix and others don't, or aligning them for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@API.md` at line 122, The API path for GET /pets/owner/{ownerId} is inconsistent with other endpoints because PetController is mapped with `@RequestMapping`("/pets"); fix this by either updating the controller mapping to `@RequestMapping`("/api/pets") (and adjust any other PetController routes) so all endpoints use the /api prefix, or update API.md to reflect the actual /pets paths and add a brief note explaining why PetController uses a different base path; reference the PetController class and the GET /pets/owner/{ownerId} route when making the change so frontend developers see the authoritative mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@API.md`:
- Line 585: Fix the Swedish grammar on the line starting with "**Allt annat ar
likadant**" by changing "ar" to the correct "är" and by using the colon form for
the inflected abbreviation "URLer" -> "URL:er" (so the phrase becomes "**Allt
annat är likadant** — samma URL:er, samma request bodies, samma svar. Bara
headern andras.").
In `@src/test/resources/application-test.properties`:
- Around line 2-3: Normalize the file to UTF-8 encoding and fix the mojibake in
the comment strings: replace '# TEST-profil ? k�rs av `@ActiveProfiles`("test")'
with '# TEST-profil körs av `@ActiveProfiles`("test")' and replace '# Anv�nds av
integrationstester med `@SpringBootTest`' with '# Används av integrationstester
med `@SpringBootTest`'; apply the same fix to the other malformed comments (the
ones referenced at lines 16, 23 and 34) by replacing corrupted characters with
the correct Swedish characters (å, ä, ö) so all comments are valid UTF-8 and
readable.
---
Duplicate comments:
In `@src/main/java/org/example/vet1177/security/JwtService.java`:
- Around line 87-89: The log statement in JwtService that runs immediately after
jwtEncoder.encode(params).getTokenValue() currently logs the user's email (PII)
— update the log to avoid PII by either removing the identifier or replacing
user.getEmail() with a non-PII identifier such as user.getId() (or another
internal userId), and ensure the change is applied to the log.info call that
follows token creation so the message no longer contains the email.
---
Nitpick comments:
In `@API.md`:
- Line 122: The API path for GET /pets/owner/{ownerId} is inconsistent with
other endpoints because PetController is mapped with `@RequestMapping`("/pets");
fix this by either updating the controller mapping to
`@RequestMapping`("/api/pets") (and adjust any other PetController routes) so all
endpoints use the /api prefix, or update API.md to reflect the actual /pets
paths and add a brief note explaining why PetController uses a different base
path; reference the PetController class and the GET /pets/owner/{ownerId} route
when making the change so frontend developers see the authoritative mapping.
In `@src/main/java/org/example/vet1177/security/JwtService.java`:
- Line 57: Validate the jwt expiration configuration in the JwtService
constructor: after reading jwtProperties.getExpirationMs() into the expirationMs
field, check that expirationMs is > 0 and if not throw an IllegalStateException
(or other startup-failing runtime exception) with a clear message mentioning the
invalid jwt.expiration-ms value; update the JwtService constructor (where
expirationMs is assigned) to perform this guard so misconfiguration fails fast
at startup.
- Around line 38-43: The JwtService constructor currently dereferences
jwtProperties.getSecretKey() and can throw an NPE if the configuration is
missing or blank; update the constructor to first validate the secret string
(e.g., check for null or empty/blank via Objects.requireNonNull or
StringUtils.hasText) and if invalid throw an IllegalStateException with a clear
message that the configuration property "jwt.secret-key" is missing or blank;
only after that convert to bytes and enforce the existing 32-byte minimum check
so the validation flow in JwtService (constructor) is explicit and fails fast
with a descriptive error.
In
`@src/test/java/org/example/vet1177/integration/activitylog/ActivityLogIntegrationTest.java`:
- Around line 57-119: Delete the large commented-out test block for
should_return_logs_for_owner_only inside ActivityLogIntegrationTest; remove the
entire commented code (the old test setup and assertions) so only the current
active test implementation remains, ensuring no leftover commented test code is
kept in the class.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 365fdf79-0e4e-4345-98bd-fa42b356eaba
📒 Files selected for processing (7)
API.mdsrc/main/java/org/example/vet1177/controller/ActivityLogController.javasrc/main/java/org/example/vet1177/controller/PetController.javasrc/main/java/org/example/vet1177/security/JwtService.javasrc/test/java/org/example/vet1177/controller/ActivityLogControllerTest.javasrc/test/java/org/example/vet1177/integration/activitylog/ActivityLogIntegrationTest.javasrc/test/resources/application-test.properties
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/example/vet1177/controller/ActivityLogControllerTest.java
| } | ||
| ``` | ||
|
|
||
| **Allt annat ar likadant** — samma URLer, samma request bodies, samma svar. Bara headern andras. |
There was a problem hiding this comment.
Minor Swedish grammar correction.
Per the static analysis hint, Swedish abbreviations should use a colon when inflected.
📝 Suggested fix
-**Allt annat ar likadant** — samma URLer, samma request bodies, samma svar. Bara headern andras.
+**Allt annat är likadant** — samma URL:er, samma request bodies, samma svar. Bara headern ändras.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **Allt annat ar likadant** — samma URLer, samma request bodies, samma svar. Bara headern andras. | |
| **Allt annat är likadant** — samma URL:er, samma request bodies, samma svar. Bara headern ändras. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~585-~585: Förkortningar som böjs binds ihop med ett kolon: "URL:er"
Context: ...``` Allt annat ar likadant — samma URLer, samma request bodies, samma svar. Bara...
(Forkortning)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@API.md` at line 585, Fix the Swedish grammar on the line starting with
"**Allt annat ar likadant**" by changing "ar" to the correct "är" and by using
the colon form for the inflected abbreviation "URLer" -> "URL:er" (so the phrase
becomes "**Allt annat är likadant** — samma URL:er, samma request bodies, samma
svar. Bara headern andras.").
Comment on lines
+2
to
+3
| # TEST-profil ? k�rs av @ActiveProfiles("test") | ||
| # Anv�nds av integrationstester med @SpringBootTest |
There was a problem hiding this comment.
Fix corrupted comment encoding (mojibake).
Comments on Line 2, Line 3, Line 16, Line 23, and Line 34 contain malformed characters, which hurts readability. Please normalize this file to UTF-8 and rewrite those comment strings.
Also applies to: 16-16, 23-23, 34-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/resources/application-test.properties` around lines 2 - 3, Normalize
the file to UTF-8 encoding and fix the mojibake in the comment strings: replace
'# TEST-profil ? k�rs av `@ActiveProfiles`("test")' with '# TEST-profil körs av
`@ActiveProfiles`("test")' and replace '# Anv�nds av integrationstester med
`@SpringBootTest`' with '# Används av integrationstester med `@SpringBootTest`';
apply the same fix to the other malformed comments (the ones referenced at lines
16, 23 and 34) by replacing corrupted characters with the correct Swedish
characters (å, ä, ö) so all comments are valid UTF-8 and readable.
- Removed redundant and commented-out lines in `application-test.properties`. - Ensured properties have clear and concise descriptions for better readability.
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
2 participants
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
Security
Documentation
Tests
Chores