Repository navigation
π‘οΈ Sentinel: [security improvement] νκ²½ λ³μ ν΄λ°± λ§€ν μ κ±° #479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
36ac667
f655376
4eead5f
735dafc
8772377
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,19 +14,34 @@ | |
| import com.clearfolio.viewer.service.DocumentConversionService; | ||
| import com.clearfolio.viewer.service.RetryDeadLetterResult; | ||
|
|
||
| import com.clearfolio.viewer.auth.TenantAccessService; | ||
| import com.clearfolio.viewer.auth.TenantContext; | ||
| import org.springframework.http.HttpHeaders; | ||
| import static org.mockito.ArgumentMatchers.any; | ||
| import static org.mockito.ArgumentMatchers.eq; | ||
|
|
||
| import com.clearfolio.viewer.model.ConversionJobStatus; | ||
| class AdminControllerTest { | ||
|
|
||
| private DocumentConversionService conversionService; | ||
| private TenantAccessService tenantAccessService; | ||
| private WebTestClient webTestClient; | ||
| private AdminController controller; | ||
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| conversionService = mock(DocumentConversionService.class); | ||
| controller = new AdminController(conversionService); | ||
| tenantAccessService = mock(TenantAccessService.class); | ||
| controller = new AdminController(conversionService, tenantAccessService); | ||
| webTestClient = WebTestClient.bindToController(controller) | ||
| .controllerAdvice(new ApiExceptionHandler()) | ||
| .build(); | ||
|
|
||
| TenantContext mockContext = mock(TenantContext.class); | ||
| when(mockContext.tenantId()).thenReturn("buyer-demo"); | ||
| when(mockContext.subjectId()).thenReturn("subject-1"); | ||
| org.mockito.Mockito.doCallRealMethod().when(tenantAccessService).requireSameTenant(any(), any()); | ||
| when(tenantAccessService.require(any(HttpHeaders.class), any(String.class))).thenReturn(mockContext); | ||
|
Comment on lines
+17
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π Security & Privacy | π Major | β‘ Quick win μ ν λνΈ λ³΄μ κ³μ½μ μ§μ κ²μ¦ν΄μΌ ν©λλ€.
μ±κ³΅ μ¬μλ ν
μ€νΈλ As per coding guidelines, βEvery production change must maintain 100% JaCoCo line and branch coverage for Also applies to: 110-111 π€ Prompt for AI AgentsSource: Coding guidelines |
||
| } | ||
|
|
||
| @Test | ||
|
|
@@ -79,9 +94,30 @@ void getAllJobsFiltersByDeadLetteredFalse() { | |
| .jsonPath("$.jobs[0].fileName").isEqualTo("b.pdf"); | ||
| } | ||
|
|
||
|
|
||
| @Test | ||
| void getAllJobsSkipsJobsNotBelongingToTenant() { | ||
| ConversionJob job1 = new ConversionJob(UUID.randomUUID(), "a.pdf", "application/pdf", "hash-a", 100L); | ||
| ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "b.pdf", "application/pdf", "hash-b", 100L); | ||
| // job1 belongs to buyer-demo by default. | ||
| // We can't change job2's tenant easily without reflection or constructor if we didn't add it. | ||
| // Wait, ConversionJob has a constructor with tenantId! | ||
| ConversionJob job3 = new ConversionJob(UUID.randomUUID(), "other-tenant", "subject-1", "c.pdf", "application/pdf", "hash-c", 100L, 0); | ||
|
|
||
| when(conversionService.getAllJobs()).thenReturn(Arrays.asList(job1, job2, job3)); | ||
|
|
||
| webTestClient.get() | ||
| .uri("/api/v1/admin/convert/jobs") | ||
| .exchange() | ||
| .expectStatus().isOk() | ||
| .expectBody() | ||
| .jsonPath("$.jobs.length()").isEqualTo(2); | ||
| } | ||
| @Test | ||
| void deleteJobReturnsNoContent() { | ||
| UUID jobId = UUID.randomUUID(); | ||
| ConversionJob mockJob = new ConversionJob(jobId, "a.pdf", "application/pdf", "hash-a", 100L); | ||
| when(conversionService.getAllJobs()).thenReturn(Arrays.asList(mockJob)); | ||
|
|
||
| webTestClient.delete() | ||
| .uri("/api/v1/admin/convert/jobs/" + jobId) | ||
|
|
@@ -92,7 +128,8 @@ void deleteJobReturnsNoContent() { | |
| @Test | ||
| void retryDeadLetteredReturnsAcceptedWhenAccepted() { | ||
| UUID jobId = UUID.randomUUID(); | ||
| when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.ACCEPTED); | ||
| when(conversionService.getAllJobs()).thenReturn(Arrays.asList(new ConversionJob(jobId, "a", "application/pdf", "hash", 100L))); | ||
| when(conversionService.retryDeadLettered(eq(jobId), any(String.class))).thenReturn(RetryDeadLetterResult.ACCEPTED); | ||
|
|
||
| webTestClient.post() | ||
| .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry") | ||
|
|
@@ -103,7 +140,8 @@ void retryDeadLetteredReturnsAcceptedWhenAccepted() { | |
| @Test | ||
| void retryDeadLetteredReturnsNotFoundWhenNotFound() { | ||
| UUID jobId = UUID.randomUUID(); | ||
| when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.NOT_FOUND); | ||
| when(conversionService.getAllJobs()).thenReturn(Arrays.asList(new ConversionJob(jobId, "a", "application/pdf", "hash", 100L))); | ||
| when(conversionService.retryDeadLettered(eq(jobId), any(String.class))).thenReturn(RetryDeadLetterResult.NOT_FOUND); | ||
|
|
||
| webTestClient.post() | ||
| .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry") | ||
|
|
@@ -114,11 +152,71 @@ void retryDeadLetteredReturnsNotFoundWhenNotFound() { | |
| @Test | ||
| void retryDeadLetteredReturnsConflictWhenNotEligible() { | ||
| UUID jobId = UUID.randomUUID(); | ||
| when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.NOT_ELIGIBLE); | ||
| when(conversionService.getAllJobs()).thenReturn(Arrays.asList(new ConversionJob(jobId, "a", "application/pdf", "hash", 100L))); | ||
| when(conversionService.retryDeadLettered(eq(jobId), any(String.class))).thenReturn(RetryDeadLetterResult.NOT_ELIGIBLE); | ||
|
|
||
| webTestClient.post() | ||
| .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry") | ||
| .exchange() | ||
| .expectStatus().isEqualTo(409); // isConflict() isn't always available depending on spring-test version, so using isEqualTo(409) is safer | ||
| .expectStatus().isEqualTo(409); | ||
| } | ||
|
|
||
| @Test | ||
| void deleteJobThrowsWhenJobNotFound() { | ||
| UUID jobId = UUID.randomUUID(); | ||
| when(conversionService.getAllJobs()).thenReturn(Arrays.asList()); | ||
|
|
||
| webTestClient.delete() | ||
| .uri("/api/v1/admin/convert/jobs/" + jobId) | ||
| .exchange() | ||
| .expectStatus().isNotFound(); | ||
| } | ||
|
|
||
| @Test | ||
| void retryThrowsWhenJobNotFound() { | ||
| UUID jobId = UUID.randomUUID(); | ||
| when(conversionService.getAllJobs()).thenReturn(Arrays.asList()); | ||
|
|
||
| webTestClient.post() | ||
| .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry") | ||
| .exchange() | ||
| .expectStatus().isNotFound(); | ||
| } | ||
|
|
||
| @Test | ||
| void deleteJobThrowsWhenTenantMismatch() { | ||
| UUID jobId = UUID.randomUUID(); | ||
| ConversionJob mockJob = new ConversionJob(jobId, "a.pdf", "application/pdf", "hash-a", 100L); | ||
| when(conversionService.getAllJobs()).thenReturn(Arrays.asList(mockJob)); | ||
|
|
||
| // Use a different mock context that does not match "buyer-demo" | ||
| TenantContext mockContext = mock(TenantContext.class); | ||
| when(mockContext.tenantId()).thenReturn("other-tenant"); | ||
| when(tenantAccessService.require(any(HttpHeaders.class), any(String.class))).thenReturn(mockContext); | ||
|
|
||
| webTestClient.delete() | ||
| .uri("/api/v1/admin/convert/jobs/" + jobId) | ||
| .exchange() | ||
| .expectStatus().isNotFound(); | ||
| } | ||
|
|
||
| @Test | ||
| void retryThrowsWhenTenantMismatch() { | ||
| UUID jobId = UUID.randomUUID(); | ||
| ConversionJob mockJob = new ConversionJob(jobId, "a.pdf", "application/pdf", "hash-a", 100L); | ||
| when(conversionService.getAllJobs()).thenReturn(Arrays.asList(mockJob)); | ||
|
|
||
| TenantContext mockContext = mock(TenantContext.class); | ||
| when(mockContext.tenantId()).thenReturn("other-tenant"); | ||
| when(mockContext.subjectId()).thenReturn("subject-1"); | ||
| when(tenantAccessService.require(any(HttpHeaders.class), any(String.class))).thenReturn(mockContext); | ||
|
|
||
| webTestClient.post() | ||
| .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry") | ||
| .exchange() | ||
| .expectStatus().isNotFound(); | ||
| } | ||
|
|
||
|
|
||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π Security & Privacy | π Major | ποΈ Heavy lift
π§© Analysis chain
π Script executed:
Repository: ContextualWisdomLab/clearfolio
Length of output: 24760
π Script executed:
Repository: ContextualWisdomLab/clearfolio
Length of output: 50386
π Script executed:
Repository: ContextualWisdomLab/clearfolio
Length of output: 9158
π Script executed:
Repository: ContextualWisdomLab/clearfolio
Length of output: 50389
λ μν¬λ¦Ώμ
@Valueλ°μΈλ©μμ KV μ‘°νλ‘ μ΄μ νμμμ€.nullκ°μ SpringμsystemEnvironmentPropertySourceλ₯Ό μ°¨λ¨νμ§ μμ΅λλ€.ArtifactLinkServiceμTenantAccessServiceλ μ¬μ ν@Valueλ‘ λ μν¬λ¦Ώμ μ½μΌλ―λ‘ OS νκ²½ λ³μλ‘ μ£Όμ ν μ μμ΅λλ€. λ μν¬λ¦Ώμ KV/credential registry APIμμ μ§μ μ‘°ννκ³ , μν¬λ¦Ώ νκ²½ λ³μλ§ μ€μ νλ©΄ λΆν μ΄ μ€ν¨νλ©° KV κ°μ΄ μμΌλ©΄ μ±κ³΅νλ ν μ€νΈλ₯Ό μΆκ°νμμμ€.π€ Prompt for AI Agents
Source: Coding guidelines