Skip to content

fix(security): enforce tenant-scoped admin deletion - #341

Open
seonghobae wants to merge 4 commits into
mainfrom
fix/admin-delete-tenant-boundary-main-20260810
Open

fix(security): enforce tenant-scoped admin deletion#341
seonghobae wants to merge 4 commits into
mainfrom
fix/admin-delete-tenant-boundary-main-20260810

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Objective

Reconcile one high-risk semantic slice from stale descendant #268 directly on current protected main: administrative delete must authenticate tenant authority, require delete permission, use the existing tenant-scoped service mutation, and conceal missing/foreign jobs as the same 404 outcome.

Fresh exact state — 2026-08-10

  • exact source head: 5da7462b0b98cabc429ea922888ebb8b5a0ccf1c;
  • independently resolved protected main: 55d7ae8647208e301f282350f076eeddaba61d11;
  • clean current-base slice: three changed files, three commits, zero commits behind protected main;
  • exact-head CI 31399587866: success;
  • exact-head Security Scan 31399587848: success;
  • exact-head SAST Semgrep 31399587735: success;
  • exact-head fuzz 31399587671: success;
  • no unresolved review threads and no formal reviews at the latest direct refetch;
  • automated checks/statuses are evidence, never qualifying independent approval.

Test-first evidence

RED

Test-only head 110bf80220b2783e73b496b4edfd54d119c17e06 added the focused WebFlux regression before changing production. Exact-head CI 31399317127 failed in Maven test compilation at AdminControllerDeleteTenantBoundaryTest: production exposed only AdminController(DocumentConversionService) and therefore had no tenant-authorization dependency. The exact checkout identity in the failed job was the RED SHA. Security Scan and SAST were already successful on that test-only head.

GREEN

Current AdminController injects TenantAccessService, requires job:delete from request tenant claims before any deletion, delegates only to the existing deleteJob(jobId, TenantContext) service boundary, and maps a missing or cross-tenant result to the same controlled 404. The legacy global deleteJob(jobId) path is no longer reachable from this administrative delete endpoint.

Focused tests prove:

  • missing tenant authority returns 401 before mutation;
  • missing delete permission returns 403 before mutation;
  • missing/foreign jobs return 404 through the tenant-scoped mutation;
  • owned deletion returns 204;
  • the legacy global delete method is never called by the endpoint.

The authoritative exact-head mvn -B --no-transfer-progress verify CI lifecycle, synthetic-merge verification, security scan, Semgrep and fuzz are all GREEN.

Scope boundary

This deliberately does not copy #268's stale ancestry or claim the whole descendant is reconciled. Tenant-scoped admin list/retry, durable deletion receipts/recovery, immutable identity/generation fencing and privacy-safe audit remain separate semantics to preserve or reimplement deliberately. Issue #263's end-to-end accessible deletion/download/recovery journey also remains open.

Merge gate

Move to Ready for ordinary independent review. Keep this exact head unchanged. Merge or auto-merge may complete only after live repository protection still sees all required gates passing, zero valid unresolved findings, and any required qualifying independent non-author approval. Automated evidence is not approval.

Summary by CodeRabbit

  • 버그 수정
    • 관리자 작업 삭제 시 테넌트 인증 및 삭제 권한을 검증합니다.
    • 인증 정보가 없으면 401, 권한이 없으면 403을 반환합니다.
    • 다른 테넌트의 작업이나 존재하지 않는 작업은 404로 처리합니다.
    • 소유한 작업을 성공적으로 삭제하면 204를 반환합니다.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@seonghobae, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 43 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e2b2c6ce-fad2-414c-85a6-89b8de478151

📥 Commits

Reviewing files that changed from the base of the PR and between 5da7462 and 8b8b72e.

📒 Files selected for processing (1)
  • src/test/java/com/clearfolio/viewer/controller/AdminControllerDeleteTenantBoundaryTest.java
📝 Walkthrough

Walkthrough

AdminController의 작업 삭제에 테넌트 인증과 JOB_DELETE 권한 검증을 추가했습니다. 삭제는 검증된 TenantContext로 수행하며, 인증 실패는 401, 권한 부족은 403, 삭제 대상이 없으면 404를 반환합니다.

Changes

테넌트 범위 작업 삭제

Layer / File(s) Summary
삭제 엔드포인트 인증 및 범위 적용
src/main/java/com/clearfolio/viewer/controller/AdminController.java
TenantAccessService를 주입합니다. 요청 헤더에서 테넌트 컨텍스트와 JOB_DELETE 권한을 검증합니다. 검증된 TenantContext로 작업을 삭제하고, 대상이 없으면 404 NOT_FOUND를 반환합니다.
테넌트 경계 테스트 갱신
src/test/java/com/clearfolio/viewer/controller/AdminControllerDeleteTenantBoundaryTest.java, src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java
인증 헤더 누락, 권한 부족, 다른 테넌트 작업, 삭제 대상 없음, 소유 테넌트의 204 응답을 검증합니다. 테스트 mock과 생성자를 새 삭제 계약에 맞게 갱신합니다.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant AdminController
  participant TenantAccessService
  participant DocumentConversionService
  Client->>AdminController: DELETE 요청 및 테넌트 헤더
  AdminController->>TenantAccessService: 테넌트와 JOB_DELETE 권한 검증
  TenantAccessService-->>AdminController: TenantContext
  AdminController->>DocumentConversionService: TenantContext와 jobId로 삭제
  DocumentConversionService-->>AdminController: 삭제 결과
  AdminController-->>Client: 204 또는 404 응답
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 테넌트 범위 기반 관리자 삭제 보안 적용이라는 변경의 핵심을 정확하고 간결하게 설명합니다.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/admin-delete-tenant-boundary-main-20260810

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae
seonghobae marked this pull request as ready for review August 10, 2026 14:46
@seonghobae
seonghobae enabled auto-merge (squash) August 10, 2026 14:47

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@src/test/java/com/clearfolio/viewer/controller/AdminControllerDeleteTenantBoundaryTest.java`:
- Line 44: 인증 거부 경로에서 특정 메서드뿐 아니라 conversionService의 모든 상호작용이 없음을 검증하세요.
AdminControllerDeleteTenantBoundaryTest.java의 44-44 및 57-57 위치에서 기존
verify(conversionService, never()).deleteJob(jobId) 검증을
verifyNoInteractions(conversionService)로 교체하세요.
🪄 Autofix

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 Plus

Run ID: b07a0b1a-75ec-497c-81de-af7d7fa6bd62

📥 Commits

Reviewing files that changed from the base of the PR and between 55d7ae8 and 5da7462.

📒 Files selected for processing (3)
  • src/main/java/com/clearfolio/viewer/controller/AdminController.java
  • src/test/java/com/clearfolio/viewer/controller/AdminControllerDeleteTenantBoundaryTest.java
  • src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant