-
Notifications
You must be signed in to change notification settings - Fork 0
π‘οΈ Sentinel: [CRITICAL] Admin APIμ κΆν μ μ΄ μ°ν λ° λ°μ΄ν° μ μΆ μ·¨μ½μ ν¨μΉ #359
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
π‘οΈ Sentinel: [CRITICAL] Admin APIμ κΆν μ μ΄ μ°ν λ° λ°μ΄ν° μ μΆ μ·¨μ½μ ν¨μΉ #359
Changes from all commits
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 |
|---|---|---|
| @@ -1,42 +1,66 @@ | ||
| package com.clearfolio.viewer.controller; | ||
|
|
||
| import static org.mockito.ArgumentMatchers.any; | ||
| import static org.mockito.ArgumentMatchers.anyString; | ||
| import static org.mockito.ArgumentMatchers.eq; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.Optional; | ||
| import java.util.UUID; | ||
|
|
||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.springframework.http.HttpHeaders; | ||
| import org.springframework.test.web.reactive.server.WebTestClient; | ||
|
|
||
| import com.clearfolio.viewer.auth.TenantAccessService; | ||
| import com.clearfolio.viewer.auth.TenantContext; | ||
| import com.clearfolio.viewer.auth.TenantPermissions; | ||
| import com.clearfolio.viewer.model.ConversionJob; | ||
| import com.clearfolio.viewer.service.DocumentConversionService; | ||
| import com.clearfolio.viewer.service.RetryDeadLetterResult; | ||
|
|
||
| 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(); | ||
| } | ||
|
|
||
| private void mockTenantContext(String permission) { | ||
| TenantContext context = new TenantContext("tenant-a", "user-1", null); | ||
| when(tenantAccessService.require(any(HttpHeaders.class), eq(permission))) | ||
| .thenReturn(context); | ||
| } | ||
|
|
||
| @Test | ||
| void getAllJobsReturnsAllJobsWhenNoFilterProvided() { | ||
| 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); | ||
| when(conversionService.getAllJobs()).thenReturn(Arrays.asList(job1, job2)); | ||
| mockTenantContext(TenantPermissions.JOB_READ); | ||
| ConversionJob job1 = new ConversionJob(UUID.randomUUID(), "tenant-a", "user-1", | ||
| "a.pdf", "application/pdf", "hash-a", 100L, 3); | ||
| ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "tenant-a", "user-1", | ||
| "b.pdf", "application/pdf", "hash-b", 100L, 3); | ||
| ConversionJob job3 = new ConversionJob(UUID.randomUUID(), "tenant-b", "user-1", | ||
| "c.pdf", "application/pdf", "hash-c", 100L, 3); | ||
| when(conversionService.getAllJobs()).thenReturn(Arrays.asList(job1, job2, job3)); | ||
|
|
||
| webTestClient.get() | ||
| .uri("/api/v1/admin/convert/jobs") | ||
| .header(TenantContext.TENANT_ID_HEADER, "tenant-a") | ||
| .header(TenantContext.SUBJECT_ID_HEADER, "user-1") | ||
| .header(TenantContext.PERMISSIONS_HEADER, TenantPermissions.JOB_READ) | ||
| .exchange() | ||
| .expectStatus().isOk() | ||
| .expectBody() | ||
|
|
@@ -47,14 +71,20 @@ void getAllJobsReturnsAllJobsWhenNoFilterProvided() { | |
|
|
||
| @Test | ||
| void getAllJobsFiltersByDeadLetteredTrue() { | ||
| ConversionJob job1 = new ConversionJob(UUID.randomUUID(), "a.pdf", "application/pdf", "hash-a", 100L); | ||
| mockTenantContext(TenantPermissions.JOB_READ); | ||
| ConversionJob job1 = new ConversionJob(UUID.randomUUID(), "tenant-a", "user-1", | ||
| "a.pdf", "application/pdf", "hash-a", 100L, 3); | ||
| job1.markDeadLettered("failed"); | ||
| ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "b.pdf", "application/pdf", "hash-b", 100L); | ||
| ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "tenant-a", "user-1", | ||
| "b.pdf", "application/pdf", "hash-b", 100L, 3); | ||
|
|
||
| when(conversionService.getAllJobs()).thenReturn(Arrays.asList(job1, job2)); | ||
|
|
||
| webTestClient.get() | ||
| .uri("/api/v1/admin/convert/jobs?deadLettered=true") | ||
| .header(TenantContext.TENANT_ID_HEADER, "tenant-a") | ||
| .header(TenantContext.SUBJECT_ID_HEADER, "user-1") | ||
| .header(TenantContext.PERMISSIONS_HEADER, TenantPermissions.JOB_READ) | ||
| .exchange() | ||
| .expectStatus().isOk() | ||
| .expectBody() | ||
|
|
@@ -64,14 +94,20 @@ void getAllJobsFiltersByDeadLetteredTrue() { | |
|
|
||
| @Test | ||
| void getAllJobsFiltersByDeadLetteredFalse() { | ||
| ConversionJob job1 = new ConversionJob(UUID.randomUUID(), "a.pdf", "application/pdf", "hash-a", 100L); | ||
| mockTenantContext(TenantPermissions.JOB_READ); | ||
| ConversionJob job1 = new ConversionJob(UUID.randomUUID(), "tenant-a", "user-1", | ||
| "a.pdf", "application/pdf", "hash-a", 100L, 3); | ||
| job1.markDeadLettered("failed"); | ||
| ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "b.pdf", "application/pdf", "hash-b", 100L); | ||
| ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "tenant-a", "user-1", | ||
| "b.pdf", "application/pdf", "hash-b", 100L, 3); | ||
|
|
||
| when(conversionService.getAllJobs()).thenReturn(Arrays.asList(job1, job2)); | ||
|
|
||
| webTestClient.get() | ||
| .uri("/api/v1/admin/convert/jobs?deadLettered=false") | ||
| .header(TenantContext.TENANT_ID_HEADER, "tenant-a") | ||
| .header(TenantContext.SUBJECT_ID_HEADER, "user-1") | ||
| .header(TenantContext.PERMISSIONS_HEADER, TenantPermissions.JOB_READ) | ||
| .exchange() | ||
| .expectStatus().isOk() | ||
| .expectBody() | ||
|
|
@@ -81,44 +117,75 @@ void getAllJobsFiltersByDeadLetteredFalse() { | |
|
|
||
| @Test | ||
| void deleteJobReturnsNoContent() { | ||
| mockTenantContext(TenantPermissions.JOB_DELETE); | ||
| UUID jobId = UUID.randomUUID(); | ||
| ConversionJob job = new ConversionJob(jobId, "tenant-a", "user-1", | ||
| "a.pdf", "application/pdf", "hash", 100L, 3); | ||
| when(conversionService.getJob(jobId)).thenReturn(Optional.of(job)); | ||
|
|
||
| webTestClient.delete() | ||
| .uri("/api/v1/admin/convert/jobs/" + jobId) | ||
| .header(TenantContext.TENANT_ID_HEADER, "tenant-a") | ||
| .header(TenantContext.SUBJECT_ID_HEADER, "user-1") | ||
| .header(TenantContext.PERMISSIONS_HEADER, TenantPermissions.JOB_DELETE) | ||
| .exchange() | ||
| .expectStatus().isNoContent(); | ||
| } | ||
|
|
||
| @Test | ||
| void retryDeadLetteredReturnsAcceptedWhenAccepted() { | ||
| mockTenantContext(TenantPermissions.JOB_RETRY); | ||
| UUID jobId = UUID.randomUUID(); | ||
| when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.ACCEPTED); | ||
| ConversionJob job = new ConversionJob(jobId, "tenant-a", "user-1", | ||
| "a.pdf", "application/pdf", "hash", 100L, 3); | ||
| when(conversionService.getJob(jobId)).thenReturn(Optional.of(job)); | ||
| when(conversionService.retryDeadLettered(eq(jobId), anyString())) | ||
| .thenReturn(RetryDeadLetterResult.ACCEPTED); | ||
|
Comment on lines
135
to
+143
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 μ¬μλ 주체 κ°λͺ ν κ°μ μ νν κ²μ¦νμμμ€. Line 142μ As per coding guidelines, π€ Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| webTestClient.post() | ||
| .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry") | ||
| .header(TenantContext.TENANT_ID_HEADER, "tenant-a") | ||
| .header(TenantContext.SUBJECT_ID_HEADER, "user-1") | ||
| .header(TenantContext.PERMISSIONS_HEADER, TenantPermissions.JOB_RETRY) | ||
| .exchange() | ||
| .expectStatus().isAccepted(); | ||
| } | ||
|
|
||
| @Test | ||
| void retryDeadLetteredReturnsNotFoundWhenNotFound() { | ||
| mockTenantContext(TenantPermissions.JOB_RETRY); | ||
| UUID jobId = UUID.randomUUID(); | ||
| when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.NOT_FOUND); | ||
| ConversionJob job = new ConversionJob(jobId, "tenant-a", "user-1", | ||
| "a.pdf", "application/pdf", "hash", 100L, 3); | ||
| when(conversionService.getJob(jobId)).thenReturn(Optional.of(job)); | ||
| when(conversionService.retryDeadLettered(eq(jobId), anyString())) | ||
| .thenReturn(RetryDeadLetterResult.NOT_FOUND); | ||
|
|
||
| webTestClient.post() | ||
| .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry") | ||
| .header(TenantContext.TENANT_ID_HEADER, "tenant-a") | ||
| .header(TenantContext.SUBJECT_ID_HEADER, "user-1") | ||
| .header(TenantContext.PERMISSIONS_HEADER, TenantPermissions.JOB_RETRY) | ||
| .exchange() | ||
| .expectStatus().isNotFound(); | ||
| } | ||
|
|
||
| @Test | ||
| void retryDeadLetteredReturnsConflictWhenNotEligible() { | ||
| mockTenantContext(TenantPermissions.JOB_RETRY); | ||
| UUID jobId = UUID.randomUUID(); | ||
| when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.NOT_ELIGIBLE); | ||
| ConversionJob job = new ConversionJob(jobId, "tenant-a", "user-1", | ||
| "a.pdf", "application/pdf", "hash", 100L, 3); | ||
| when(conversionService.getJob(jobId)).thenReturn(Optional.of(job)); | ||
| when(conversionService.retryDeadLettered(eq(jobId), anyString())) | ||
| .thenReturn(RetryDeadLetterResult.NOT_ELIGIBLE); | ||
|
|
||
| webTestClient.post() | ||
| .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry") | ||
| .header(TenantContext.TENANT_ID_HEADER, "tenant-a") | ||
| .header(TenantContext.SUBJECT_ID_HEADER, "user-1") | ||
| .header(TenantContext.PERMISSIONS_HEADER, TenantPermissions.JOB_RETRY) | ||
| .exchange() | ||
| .expectStatus().isEqualTo(409); // isConflict() isn't always available depending on spring-test version, so using isEqualTo(409) is safer | ||
| .expectStatus().isEqualTo(409); | ||
| } | ||
| } | ||
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.
π Maintainability & Code Quality | π‘ Minor | β‘ Quick win
ν λνΈ λ²μλ₯Ό JavaDocμ λ°μνμμμ€.
Line 62μ βall conversion jobsβ μ€λͺ μ ꡬνκ³Ό λ€λ¦ λλ€. μ΄ λ©μλλ
context.tenantId()μ μν μμ λ§ λ°νν©λλ€.@returnμ€λͺ λ νμ¬ ν λνΈμ μμ λͺ©λ‘μΌλ‘ λ³κ²½νμμμ€.π€ Prompt for AI Agents