security#15
Merged
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (2)
📝 WalkthroughWalkthroughDataInitializer now injects PasswordEncoder and seeds two users, roles, and user_roles idempotently with encoded passwords. ActivityLogController responses are wrapped in a new ApiResponse. A CustomUserDetailsService loads users and maps roles for Spring Security; SecurityConfig switches to form login and registers PasswordEncoder and the custom UserDetailsService. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SecurityConfig
participant AuthenticationManager
participant CustomUserDetailsService
participant UserRepository
participant PasswordEncoder
Client->>SecurityConfig: Submit login (username, password)
SecurityConfig->>AuthenticationManager: Authenticate request
AuthenticationManager->>CustomUserDetailsService: loadUserByUsername(username)
CustomUserDetailsService->>UserRepository: findByUsername(username)
UserRepository-->>CustomUserDetailsService: User(entity with encoded password & roles)
CustomUserDetailsService->>AuthenticationManager: return UserDetails (username, encodedPassword, authorities)
AuthenticationManager->>PasswordEncoder: matches(rawPassword, encodedPassword)
PasswordEncoder-->>AuthenticationManager: match result
AuthenticationManager-->>SecurityConfig: authentication success/failure
SecurityConfig-->>Client: success (redirect/token) or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![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 `@src/main/java/org/group1/projectbackend/config/DataInitializer.java`:
- Around line 27-38: The INSERTs in DataInitializer using jdbcTemplate.update
for seeding users (the calls that pass userPassword and adminPassword) use ON
CONFLICT (id) DO NOTHING which prevents replacing legacy/plaintext passwords;
change the upsert to ON CONFLICT (id) DO UPDATE SET password =
EXCLUDED.password, updated_at = CURRENT_TIMESTAMP (or similar) so existing
seeded users get their password column migrated to the new BCrypt value; update
both jdbcTemplate.update calls (the ones inserting id=1 and id=2) to use this DO
UPDATE behavior and preserve other columns as needed.
- Around line 22-24: The DataInitializer currently seeds predictable credentials
by calling passwordEncoder.encode on literal strings ("password123" /
"admin123"); remove those hard-coded passwords and replace seeding with a secure
approach: either read the raw passwords from a secure source (environment
variables or a secret manager) before calling passwordEncoder.encode, or
generate strong random passwords at startup and log/store them securely only for
dev/test runs, and ensure seeding is skipped or disabled in non-development
profiles. Update references to userPassword and adminPassword and the code paths
in the DataInitializer class so no hard-coded raw passwords remain in the
repository.
- Around line 27-65: The seed inserts for users/roles/user_roles
(jdbcTemplate.update calls inserting into users, roles, and user_roles using
userPassword and adminPassword) must be executed in a single transaction to
avoid partial state on failure; wrap the block in a transactional boundary by
marking the DataInitializer method that performs these inserts with
`@Transactional` (or execute the inserts via a
TransactionTemplate/PlatformTransactionManager) so all inserts commit or
rollback together, keeping the same jdbcTemplate calls and handling exceptions
as before.
- Around line 55-64: In DataInitializer, update the two jdbcTemplate.update SQL
blocks that insert into user_roles (the multi-line string literals used to
"Koppla ADMIN → ROLE_ADMIN" and the other user_roles insert) to use an explicit
conflict target by replacing "ON CONFLICT DO NOTHING" with "ON CONFLICT
(user_id, role_id) DO NOTHING" so PostgreSQL accepts the composite key conflict
check; keep everything else in the jdbcTemplate.update calls unchanged.
In
`@src/main/java/org/group1/projectbackend/controller/ActivityLogController.java`:
- Around line 25-84: The controller methods (createActivityLog,
getAllActivityLogs, getActivityLogById, getByTicket, getByUser) now return an
enveloped response (status/data/timestamp) which breaks existing
consumers/tests; either revert to the original raw DTO/list responses or
introduce versioning/migration: add new versioned endpoints (e.g., /v2/...
mapped to the envelope response) and keep the existing endpoints returning the
old raw payloads, or update the tests in ActivityLogControllerTest to assert the
new envelope shape; ensure whichever approach you choose is applied consistently
across these controller methods and update or add tests accordingly.
- Line 28: The create path is surfacing 500 because
ActivityLogService.createActivityLog throws RuntimeException for missing
user/ticket; change the service to throw ResourceNotFoundException (the
exception your controller/handler expects) whenever a user or ticket is not
found (replace the RuntimeException throws in
ActivityLogService.createActivityLog and its helper lookups with
ResourceNotFoundException), or alternatively catch the current RuntimeException
in createActivityLog and rethrow a ResourceNotFoundException with the same
message so ActivityLogController.activityLogService.createActivityLog(dto)
results in a 404.
In
`@src/main/java/org/group1/projectbackend/security/CustomUserDetailsService.java`:
- Around line 23-31: The UserDetails built in CustomUserDetailsService (in the
method that converts your User entity to a Spring Security User) does not
propagate the entity's enabled flag, so disabled users can still authenticate;
update the org.springframework.security.core.userdetails.User.builder() chain to
set the disabled flag by calling .disabled(!user.isEnabled()) (or the equivalent
.disabled when user.isEnabled() is false) so account status from the User entity
is enforced during authentication.
🪄 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: ca678efe-90c0-48a7-a2ca-4f605b9bc0b3
📒 Files selected for processing (5)
src/main/java/org/group1/projectbackend/config/DataInitializer.javasrc/main/java/org/group1/projectbackend/controller/ActivityLogController.javasrc/main/java/org/group1/projectbackend/dto/ApiResponse.javasrc/main/java/org/group1/projectbackend/security/CustomUserDetailsService.javasrc/main/java/org/group1/projectbackend/security/SecurityConfig.java
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/group1/projectbackend/controller/ActivityLogController.java`:
- Around line 25-28: The createActivityLog endpoint currently trusts
client-supplied userId in CreateActivityLogDto; change its signature in
ActivityLogController.createActivityLog to accept a java.security.Principal (or
Authentication) instead of reading userId from the request body, then resolve
the actor ID server-side by calling
UserService.findByUsername(principal.getName()).getId() and set that ID on the
CreateActivityLogDto (or construct the ActivityLog entity) before passing to
activityLogService.createActivityLog; remove any reliance on dto.userId and
ensure the controller obtains username from Principal and uses UserService (the
existing UserService.findByUsername) to fetch the authoritative user ID.
In `@src/main/java/org/group1/projectbackend/security/SecurityConfig.java`:
- Around line 22-28: You disabled CSRF globally in the HttpSecurity
configuration (csrf(...).disable()) while using session-based form login and
also left a catch-all anyRequest().permitAll(), which exposes state-changing
endpoints; re-enable CSRF protection (remove csrf.disable()) and instead use
csrf.ignoringRequestMatchers(...) only for truly stateless endpoints, and
tighten authorization rules by removing anyRequest().permitAll() and explicitly
requiring authentication for endpoints that accept POST/PUT/DELETE (e.g.,
protect /tickets and /tickets/{id}/comments) via
authorizeHttpRequests/authorizeRequests so formLogin(), csrf(), and your
controller methods that expect an authenticated Principal remain secure.
🪄 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: 7d0652b5-a31f-4957-9be7-830ae3c28819
📒 Files selected for processing (3)
src/main/java/org/group1/projectbackend/config/DataInitializer.javasrc/main/java/org/group1/projectbackend/controller/ActivityLogController.javasrc/main/java/org/group1/projectbackend/security/SecurityConfig.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/group1/projectbackend/config/DataInitializer.java
Comment on lines
+25
to
+28
| public ResponseEntity<ApiResponse<ActivityLogDto>> createActivityLog( | ||
| @Valid @RequestBody CreateActivityLogDto dto) { | ||
|
|
||
| ActivityLogDto created = activityLogService.createActivityLog(dto); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that activity-log creation still depends on a client-supplied user id.
set -e
fd 'CreateActivityLogDto.java$' src/main/java | xargs -r sed -n '1,200p'
fd 'ActivityLogService.java$' src/main/java | xargs -r sed -n '1,260p'
rg -n -C2 'createActivityLog\(|getUserId\(' src/main/java/org/group1/projectbackendRepository: ithsjava25/project-backend-grupp-1
Length of output: 9512
🏁 Script executed:
# Check if Spring Security is being introduced or configured in this PR
# Look for Security config, Principal usage, or auth dependencies
# Check for Security-related files and configurations
fd -e 'java' src/main/java | xargs rg -l 'SecurityFilterChain|@Configuration|WebSecurityConfigurerAdapter|SecurityContext|Principal' 2>/dev/null | head -20
# Check if Controller accepts Principal parameter
rg -n 'Principal\|@AuthenticationPrincipal' src/main/java/org/group1/projectbackend/controller/ 2>/dev/null
# Check pom.xml or build file for Spring Security dependency
fd 'pom.xml|build.gradle' | xargs cat 2>/dev/null | grep -i 'security\|auth' | head -10Repository: ithsjava25/project-backend-grupp-1
Length of output: 1119
🏁 Script executed:
# Check the SecurityConfig to see if it's fully configured
cat src/main/java/org/group1/projectbackend/security/SecurityConfig.java 2>/dev/null || echo "File not found"
# Double-check the actual ActivityLogController implementation
cat src/main/java/org/group1/projectbackend/controller/ActivityLogController.java 2>/dev/null || echo "File not found"Repository: ithsjava25/project-backend-grupp-1
Length of output: 3867
Derive the activity-log actor from the authenticated user, not the request body.
Spring Security is now configured. The create endpoint should stop accepting client-supplied userId and instead derive it from the authenticated principal. Otherwise callers can forge activity logs for any user, undermining the audit trail.
Replace the @RequestBody CreateActivityLogDto dto with a Principal principal parameter, then resolve the user ID server-side via UserService.findByUsername(principal.getName()).getId().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/group1/projectbackend/controller/ActivityLogController.java`
around lines 25 - 28, The createActivityLog endpoint currently trusts
client-supplied userId in CreateActivityLogDto; change its signature in
ActivityLogController.createActivityLog to accept a java.security.Principal (or
Authentication) instead of reading userId from the request body, then resolve
the actor ID server-side by calling
UserService.findByUsername(principal.getName()).getId() and set that ID on the
CreateActivityLogDto (or construct the ActivityLog entity) before passing to
activityLogService.createActivityLog; remove any reliance on dto.userId and
ensure the controller obtains username from Principal and uses UserService (the
existing UserService.findByUsername) to fetch the authoritative user ID.
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.
Added security
Summary by CodeRabbit
Security Enhancements
API Improvements
Chores