Conversation
…th and without authorization
WalkthroughIntroduced a private helper to centralize OAuth2 user ID extraction (checks Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
✨ 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: 1
🧹 Nitpick comments (2)
backend/src/test/java/org/fungover/zipp/controller/SecuredReportControllerTest.java (1)
43-58: Tighten success assertion and align OAuth2 attributes with ID extraction
- The PR description mentions expecting a created response; if
POST /api/reportsreturns201, consider assertingstatus().isCreated()instead of the broaderis2xxSuccessful()so the test locks in the intended contract.- Your mocked OAuth2 principal only explicitly sets
name,role. SinceUserIdentityService.extractOAuth2UserIdlooks forsub,id, oruser_id, it’s safer to ensure one of those attributes is present in this mock (e.g.,user.put("sub", "test-user-id");) so the test mirrors real authentication data and avoids unexpectedIllegalStateExceptionif that service is used in the report flow.- Both tests duplicate the
Reportcreation andObjectMapper+JavaTimeModulesetup; you could factor these into helper methods or a@BeforeEachto keep the tests DRY (optional).backend/src/main/java/org/fungover/zipp/service/UserIdentityService.java (1)
20-65: OAuth2 ID extraction is clear; consider a security-specific exception typeThe new
getUserId/extractOAuth2UserIdlogic is straightforward and covers common OAuth2 ID attributes (sub,id,user_id). The main behavioral change is throwingIllegalStateExceptionwhen no ID can be derived (or when authentication is null). Unless you already translate these exceptions, this is likely to bubble up as a 500 response rather than a 401/403.Consider using a Spring Security–specific exception (e.g.,
AuthenticationCredentialsNotFoundExceptionor another appropriateAuthenticationException/AccessDeniedException) or handling this case closer to the web layer so clients receive an auth-related status instead of a generic server error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/pom.xml(1 hunks)backend/src/main/java/org/fungover/zipp/service/UserIdentityService.java(2 hunks)backend/src/test/java/org/fungover/zipp/controller/SecuredReportControllerTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PR Build
- GitHub Check: Jenkins
- GitHub Check: Build, Test & Analyze
🔇 Additional comments (1)
backend/pom.xml (1)
120-123: Jackson JavaTime module dependency fits the new test usageAdding
jackson-datatype-jsr310is consistent with your use ofJavaTimeModuleforInstantserialization and will help ensure runtime JSON handling of Java time types works as expected. No issues from a build/config perspective.
backend/src/test/java/org/fungover/zipp/controller/SecuredReportControllerTest.java
Show resolved
Hide resolved
# Conflicts: # backend/pom.xml # backend/src/main/java/org/fungover/zipp/service/UserIdentityService.java
|
Jenkins Build #4 Summary (for PR #84)
Details:
Error Logs (truncated): |
|
Jenkins Build #5 Summary (for PR #84)
Details:
Error Logs (truncated): |
…d run: - ./mvnw verify - ./mvnw spotless:apply - ./mvnw spotless:check - ./mvnw checkstyle:check # Conflicts: # backend/pom.xml # backend/src/main/java/org/fungover/zipp/service/UserIdentityService.java
…gover/zipp into test/authenticated-endpoints # Conflicts: # backend/src/main/java/org/fungover/zipp/service/UserIdentityService.java # backend/src/test/java/org/fungover/zipp/controller/SecuredReportControllerTest.java
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/main/java/org/fungover/zipp/service/UserIdentityService.java(2 hunks)backend/src/test/java/org/fungover/zipp/controller/SecuredReportControllerTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/java/org/fungover/zipp/service/UserIdentityService.java
🧰 Additional context used
🧬 Code graph analysis (1)
backend/src/test/java/org/fungover/zipp/controller/SecuredReportControllerTest.java (1)
backend/src/test/java/org/fungover/zipp/authtest/AuthenticationIntegrationTest.java (1)
SpringBootTest(24-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PR Build
- GitHub Check: Jenkins
- GitHub Check: Build, Test & Analyze
🔇 Additional comments (2)
backend/src/test/java/org/fungover/zipp/controller/SecuredReportControllerTest.java (2)
1-40: LGTM!The imports and class setup are appropriate for integration testing with MockMvc, OAuth2, and Testcontainers.
57-72: LGTM!The setup correctly prepares the test user, configures Jackson to handle
Instantserialization, and mocks Kafka to prevent actual message publishing.
backend/src/test/java/org/fungover/zipp/controller/SecuredReportControllerTest.java
Show resolved
Hide resolved
backend/src/test/java/org/fungover/zipp/controller/SecuredReportControllerTest.java
Show resolved
Hide resolved
Jorlindstrom
left a comment
Jorlindstrom
left a comment
There was a problem hiding this comment.
The implementation shows a solid understanding of Spring Security, OAuth2 authentication, and integration testing with MockMvc. The code is generally well structured, readable, and defensive, especially in how authentication and principal extraction are handled. The test coverage demonstrates that both authenticated and unauthenticated flows have been considered, which is an important strength from a security perspective.
That said, there are a few architectural and maintainability concerns worth addressing before this is merged.
SecuredReportControllerTest
Lines ~32–35
@SpringBootTest
@AutoConfigureMockMvc
This test loads the full application context even though the test scope is limited to controller + security behavior.
Action: Consider replacing with @WebMvcTest(ReportController.class), Mock required collaborators instead of starting the full context
Keep @SpringBootTest only if database + Kafka wiring is intentionally under test
Lines ~44–50
@MockitoBean
private KafkaTemplate<String, ReportResponse> kafkaTemplate;
@MockitoBean
private KafkaAdmin kafkaAdmin;
Kafka infrastructure is mocked, but the test still implicitly depends on messaging configuration being present.
Action: Verify whether Kafka is actually required for this controller test
If not, exclude Kafka auto-configuration or move Kafka behavior into a separate integration test
Lines ~58–66
@beforeeach
void setup() {
user = new User();
user.setName("Test User");
...
}
The test manually constructs a User entity with multiple fields.
Action: Introduce a test fixture or builder (e.g. TestUserFactory.createUser()), Avoid coupling tests to entity internals that may change
Lines ~76–83
Report firstReport = new Report(
user,
"Candy paper",
ReportType.DEBRIS,
...
);
The test constructs a domain object with fields (status, timestamps) that are not directly relevant to authorization.
Action: Use a minimal request DTO instead of the domain entity
Focus the test on authorization behavior, not entity completeness
Lines ~87–89
.andExpect(status().is3xxRedirection());
The test asserts only that the user is redirected.
Action: Also assert the redirect target if possible (e.g. /login), This makes the intent of the test clearer
Lines ~94–104
.with(oauth2Login().attributes(u -> {
u.put("name", "Test User");
u.put("email", "test@gmail.com");
u.put("sub", user.getProviderId());
}))
OAuth2 attributes are defined inline.
Action: Extract OAuth2 user creation into a helper method, This avoids duplication and improves readability across tests
UserIdentityService
Lines ~24–26
public String getUserId(Authentication authentication)
This method mixes identity extraction and authentication validation.
Action: Extract identity resolution into a separate private or dedicated class, Keep this method focused on delegation rather than logic branching
Lines ~37–45
if (principal instanceof OAuth2User oAuth2User) {
return extractOAuth2UserId(oAuth2User);
}
OAuth2 user handling is correct, but the method returns a raw String without context.
Action: Document whether this ID is guaranteed to be a provider ID, Consider returning a value object instead of a raw string
Lines ~55–72
public User getCurrentUser(Authentication authentication)
This method handles: authentication validation, identity extraction, repository lookups, error handling
Action:
Split into: resolveProviderId(Authentication) and loadUserByProviderId(String)
This improves testability and readability
Lines ~61–68
String email = oauth.getAttribute("email");
if (email != null && !email.isBlank()) {
return userRepository.findByEmail(email)
User lookup prioritizes email over provider ID.
Action (important): Use provider ID as the primary lookup key, Treat email as metadata only
This prevents account duplication and login inconsistencies
Lines ~96–99
throw new IllegalStateException(
"Unsupported principal type: " + principal.getClass().getName()
);
The error message is clear, but the exception type is very generic.
Action: Replace with a more specific exception (e.g., AuthenticationCredentialsNotFoundException)
This improves error handling and security clarity
Lines ~109–128
private String extractOAuth2UserId(OAuth2User oAuth2User)
The method correctly checks multiple attribute names.
Action: Consider externalizing supported attribute names into a constant list, This makes future extensions simpler and safer
|
Thanks for the feedback @Jorlindstrom !
Haven't gotten it to work without Kafka. It's mocked with @MockitoBean so it doesn't actually run, but Spring Boot needs the dependency since the controller uses it.
We are using the
This is an integration test for the security/authentication flow, not a unit test. We need the actual security config, database, and OAuth2 setup running. @WebMvcTest would mock too much and we wouldn't actually be testing authentication. About the User field in the DTO |
viktornoskire
left a comment
viktornoskire
left a comment
There was a problem hiding this comment.
From what I can see the tests work as intended. The code is clean and easy to read. Good job in working with so many different things but still keeping it clean. I'd say this is ready to be merged into main.
LolloGro
left a comment
LolloGro
left a comment
There was a problem hiding this comment.
The test is valide, the code is easy to read and its clear what the test tests. Good work!
Emilyempa
dismissed
Jorlindstrom’s stale review
@alfred gave good answer in this PR about why
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* Add test for redirect if not logged in and test for logged in POSTs to REST API * Write a simple SecuredReportControllerTest to test report creation with and without authorization * Run the maven commands --------- Co-authored-by: Alfred Brännare <alfred-brannare@hotmail.com>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Tarupotter
left a comment
Tarupotter
left a comment
There was a problem hiding this comment.
the tests are clear and structured, and I am really impressed by your dedication and contribution to this project :))
7 participants
Description
A test exists that attempts to call POST /api/reports without authentication and expects a 3xx.
A test exists that calls POST /api/reports with valid authentication and expects a successful response (201 Created or appropriate status).
Made by
@Emilyempa
@alfredbrannare
@Klumpish
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.