Skip to content

feat(admin): 관리자 컨트롤러에 권한 검증 추가 #221

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.

Sign up for GitHub

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

Closed
seonghobae wants to merge 1 commit into from
Closed

feat(admin): 관리자 컨트롤러에 권한 검증 추가 #221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions commit_message.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
🛡️ Sentinel: [CRITICAL] 파일 업로드 경로 조작(Path Traversal) 취약점 수정
feat(admin): 관리자 컨트롤러에 권한 검증 추가

MultipartFile.getOriginalFilename()을 신뢰하여 발생할 수 있는 경로 조작 취약점을 수정했습니다.
StringUtils.cleanPath()를 사용하여 경로를 정규화하고 순수한 파일명만 추출하여 악의적인 페이로드(예: ../../../etc/passwd.hwp)로부터 시스템을 보호합니다.
`AdminController`의 엔드포인트에 인증 및 권한(`ADMIN_READ`, `ADMIN_WRITE`) 검증 로직을 추가하여 무단 접근을 방지하도록 개선하였습니다.
- `TenantPermissions`에 `ADMIN_READ` 및 `ADMIN_WRITE` 권한 상수 추가
- `AdminController` 생성자에 `TenantAccessService` 주입
- 각 엔드포인트에 `TenantAccessService.require()` 호출 추가
- `AdminControllerTest`에 인증 서비스 Mock 및 검증 로직 추가
10 changes: 10 additions & 0 deletions src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ public final class TenantPermissions {
*/
public static final String ANALYTICS_READ = "analytics:read";

/**
* Permission required to read admin data.
*/
public static final String ADMIN_READ = "admin:read";

/**
* Permission required to write admin data.
*/
public static final String ADMIN_WRITE = "admin:write";

private TenantPermissions() {
}
}
19 changes: 15 additions & 4 deletions src/main/java/com/clearfolio/viewer/controller/AdminController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.server.ResponseStatusException;
import org.springframework.http.HttpHeaders;
import org.springframework.web.bind.annotation.RequestHeader;
import com.clearfolio.viewer.auth.TenantAccessService;
import com.clearfolio.viewer.auth.TenantPermissions;
import com.clearfolio.viewer.auth.TenantContext;

import com.clearfolio.viewer.api.AdminJobListResponse;
import com.clearfolio.viewer.model.ConversionJob;
Expand All @@ -25,14 +30,17 @@
@RestController
public class AdminController {

private final TenantAccessService tenantAccessService;

private final DocumentConversionService conversionService;

/**
* Creates a controller for admin operations.
*
* @param conversionService conversion service
*/
Comment on lines 37 to 41
public AdminController(DocumentConversionService conversionService) {
public AdminController(DocumentConversionService conversionService, TenantAccessService tenantAccessService) {
this.tenantAccessService = tenantAccessService;
this.conversionService = conversionService;
}

Expand All @@ -43,7 +51,8 @@ public AdminController(DocumentConversionService conversionService) {
* @return list of conversion jobs
*/
@GetMapping("/api/v1/admin/convert/jobs")
public AdminJobListResponse getAllJobs(@RequestParam(required = false) Boolean deadLettered) {
public AdminJobListResponse getAllJobs(@RequestParam(required = false) Boolean deadLettered, @RequestHeader HttpHeaders headers) {
tenantAccessService.require(headers, TenantPermissions.ADMIN_READ);
Iterable<ConversionJob> allJobs = conversionService.getAllJobs();

if (deadLettered == null) {
Expand All @@ -66,7 +75,8 @@ public AdminJobListResponse getAllJobs(@RequestParam(required = false) Boolean d
* @return no content on success
*/
@DeleteMapping("/api/v1/admin/convert/jobs/{jobId}")
public ResponseEntity<Void> deleteJob(@PathVariable UUID jobId) {
public ResponseEntity<Void> deleteJob(@PathVariable UUID jobId, @RequestHeader HttpHeaders headers) {
tenantAccessService.require(headers, TenantPermissions.ADMIN_WRITE);
conversionService.deleteJob(jobId);
return ResponseEntity.noContent().build();
}
Expand All @@ -78,7 +88,8 @@ public ResponseEntity<Void> deleteJob(@PathVariable UUID jobId) {
* @return accepted response on success
*/
@PostMapping("/api/v1/admin/convert/jobs/{jobId}/retry")
public ResponseEntity<Void> retryDeadLettered(@PathVariable UUID jobId) {
public ResponseEntity<Void> retryDeadLettered(@PathVariable UUID jobId, @RequestHeader HttpHeaders headers) {
tenantAccessService.require(headers, TenantPermissions.ADMIN_WRITE);
RetryDeadLetterResult result = conversionService.retryDeadLettered(jobId, "admin");
Comment on lines +92 to 93
if (result == RetryDeadLetterResult.NOT_FOUND) {
throw new ResponseStatusException(HttpStatus.NOT_FOUND, "job not found");
Expand Down
25 changes: 24 additions & 1 deletion src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,18 @@

import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;

import java.util.Arrays;
import java.util.UUID;

import org.junit.jupiter.api.BeforeEach;
import org.springframework.http.HttpHeaders;
import com.clearfolio.viewer.auth.TenantAccessService;
Comment on lines 11 to +13
import com.clearfolio.viewer.auth.TenantPermissions;
import com.clearfolio.viewer.auth.TenantContext;
import java.util.Set;
import org.junit.jupiter.api.Test;
import org.springframework.test.web.reactive.server.WebTestClient;

Expand All @@ -17,13 +24,15 @@
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();
Expand All @@ -34,6 +43,8 @@ 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));
TenantContext ctx = new TenantContext("admin", "admin", Set.of(TenantPermissions.ADMIN_READ));
when(tenantAccessService.require(any(), eq(TenantPermissions.ADMIN_READ))).thenReturn(ctx);

webTestClient.get()
.uri("/api/v1/admin/convert/jobs")
Expand All @@ -52,6 +63,8 @@ void getAllJobsFiltersByDeadLetteredTrue() {
ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "b.pdf", "application/pdf", "hash-b", 100L);

when(conversionService.getAllJobs()).thenReturn(Arrays.asList(job1, job2));
TenantContext ctx = new TenantContext("admin", "admin", Set.of(TenantPermissions.ADMIN_READ));
when(tenantAccessService.require(any(), eq(TenantPermissions.ADMIN_READ))).thenReturn(ctx);

webTestClient.get()
.uri("/api/v1/admin/convert/jobs?deadLettered=true")
Expand All @@ -69,6 +82,8 @@ void getAllJobsFiltersByDeadLetteredFalse() {
ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "b.pdf", "application/pdf", "hash-b", 100L);

when(conversionService.getAllJobs()).thenReturn(Arrays.asList(job1, job2));
TenantContext ctx = new TenantContext("admin", "admin", Set.of(TenantPermissions.ADMIN_READ));
when(tenantAccessService.require(any(), eq(TenantPermissions.ADMIN_READ))).thenReturn(ctx);

webTestClient.get()
.uri("/api/v1/admin/convert/jobs?deadLettered=false")
Expand All @@ -82,6 +97,8 @@ void getAllJobsFiltersByDeadLetteredFalse() {
@Test
void deleteJobReturnsNoContent() {
UUID jobId = UUID.randomUUID();
TenantContext ctx = new TenantContext("admin", "admin", Set.of(TenantPermissions.ADMIN_WRITE));
when(tenantAccessService.require(any(), eq(TenantPermissions.ADMIN_WRITE))).thenReturn(ctx);

webTestClient.delete()
.uri("/api/v1/admin/convert/jobs/" + jobId)
Expand All @@ -92,6 +109,8 @@ void deleteJobReturnsNoContent() {
@Test
void retryDeadLetteredReturnsAcceptedWhenAccepted() {
UUID jobId = UUID.randomUUID();
TenantContext ctx = new TenantContext("admin", "admin", Set.of(TenantPermissions.ADMIN_WRITE));
when(tenantAccessService.require(any(), eq(TenantPermissions.ADMIN_WRITE))).thenReturn(ctx);
when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.ACCEPTED);

webTestClient.post()
Expand All @@ -103,6 +122,8 @@ void retryDeadLetteredReturnsAcceptedWhenAccepted() {
@Test
void retryDeadLetteredReturnsNotFoundWhenNotFound() {
UUID jobId = UUID.randomUUID();
TenantContext ctx = new TenantContext("admin", "admin", Set.of(TenantPermissions.ADMIN_WRITE));
when(tenantAccessService.require(any(), eq(TenantPermissions.ADMIN_WRITE))).thenReturn(ctx);
when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.NOT_FOUND);

webTestClient.post()
Expand All @@ -114,6 +135,8 @@ void retryDeadLetteredReturnsNotFoundWhenNotFound() {
@Test
void retryDeadLetteredReturnsConflictWhenNotEligible() {
UUID jobId = UUID.randomUUID();
TenantContext ctx = new TenantContext("admin", "admin", Set.of(TenantPermissions.ADMIN_WRITE));
when(tenantAccessService.require(any(), eq(TenantPermissions.ADMIN_WRITE))).thenReturn(ctx);
when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.NOT_ELIGIBLE);

webTestClient.post()
Expand Down
Loading