[Feature] 관리자 기능 추가 및 보안 개선#66
Open
kimjuneon wants to merge 5 commits into
Open
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 6 minutes and 59 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough관리자 UI에서 멤버십 활성화 사유를 제출하면 컨트롤러가 IP/권한을 검증해 서비스의 activateMembership을 호출하고, 런타임 정책을 반영해 지갑 멤버십을 활성화하고 감사 로그와 확장된 지갑 응답을 반환합니다. 변경사항관리자 멤버십 활성화 기능
Sequence DiagramsequenceDiagram
participant AdminUI
participant AdminWalletController
participant AdminWalletService
participant RuntimePolicyService
participant TingWallet
participant AuditLog
AdminUI->>AdminWalletController: POST /users/{userId}/wallet/membership (reason)
AdminWalletController->>AdminWalletService: activateMembership(admin, userId, request, ip)
AdminWalletService->>RuntimePolicyService: snapshot().getBenefit().getMembership()
AdminWalletService->>TingWallet: activateMembership(now, policy)
AdminWalletService->>AuditLog: record(MEMBERSHIP_ACTIVATE, admin, userId, ip, reason)
AdminWalletService->>AdminWalletController: AdminWalletResponse(with membership fields)
AdminWalletController->>AdminUI: 200 OK (response)
예상 코드 리뷰 노력🎯 4 (Complex) | ⏱️ ~45 minutes 시 🐰
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🧹 Nitpick comments (2)
manabom/src/main/java/mannabom_server/manabom/application/admin/dto/request/AdminActivateMembershipRequest.java (1)
10-11: ⚡ Quick win
reason길이 상한을 DTO 계약에 같이 명시해주세요.현재는 공백/빈값만 막고 있어 매우 긴 문자열이 그대로 감사 로그/저장 계층까지 전달됩니다.
@Size(max = ...)를 추가해 입력 계약을 고정하는 편이 안전합니다.제안 diff
import jakarta.validation.constraints.NotBlank; +import jakarta.validation.constraints.Size; @@ - `@NotBlank`(message = "reason은 필수입니다.") + `@NotBlank`(message = "reason은 필수입니다.") + `@Size`(max = 255, message = "reason은 255자 이하여야 합니다.") private String reason;🤖 Prompt for 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. In `@manabom/src/main/java/mannabom_server/manabom/application/admin/dto/request/AdminActivateMembershipRequest.java` around lines 10 - 11, The DTO field 'reason' in AdminActivateMembershipRequest currently only has `@NotBlank` allowing arbitrarily long input; add a length constraint (e.g., annotate the field with `@Size`(max = 255) and a matching message like "reason은 최대 255자입니다.") to fix the DTO contract, import javax.validation.constraints.Size, and adjust any tests or validation messages that assert on this DTO to reflect the new max length.manabom/src/main/resources/static/admin/app.js (1)
832-834: ⚡ Quick win
kvHtml의 신뢰 경계를 함수 계약으로 명확히 해주세요.
valueHtml을 그대로 주입하는 구조라 향후 호출부에서 사용자 입력이 들어오면 XSS 회귀 지점이 됩니다. 함수명을kvTrustedHtml처럼 의도를 드러내거나, 주석으로 “escape 보장된 HTML만 허용” 계약을 명시하는 쪽이 안전합니다.🤖 Prompt for 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. In `@manabom/src/main/resources/static/admin/app.js` around lines 832 - 834, The kvHtml function currently injects valueHtml without escaping, creating a future XSS regression; either change the function name to kvTrustedHtml to signal it requires already-escaped/trusted HTML or update kvHtml to accept untrusted input and escape it (use escapeHtml) before concatenation, and add a short JSDoc comment above the function documenting the contract (e.g., "`@param` {string} valueHtml - must be trusted HTML" if renaming, or "`@param` {string} value - untrusted text will be escaped" if changing behavior) so callers know the safety expectations; reference kvHtml (or kvTrustedHtml if renaming) when making the change.
🤖 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
`@manabom/src/main/java/mannabom_server/manabom/presentation/admin/controller/AdminWalletController.java`:
- Around line 40-48: The activateMembership endpoint in AdminWalletController
currently passes a clientIp derived from request headers that can be spoofed;
update the clientIp(HttpServletRequest) logic (or the call site in
activateMembership) to default to request.getRemoteAddr() and only parse
X-Forwarded-For when a trusted-proxy flag or allowlist is configured and
validated; ensure you reject or ignore untrusted X-Forwarded-For values,
document/use a config flag (e.g., trustProxy) and check it before trusting
header-derived IPs so AdminWalletService.activateMembership receives a
non-spoofable audit IP.
In `@manabom/src/main/resources/static/admin/app.js`:
- Around line 425-443: The activateMembership function allows duplicate POSTs on
rapid clicks; add an in-flight guard or disable the membership button while the
request is pending and re-enable it in finally to prevent duplicate calls.
Before sending the POST in activateMembership, validate the reason from
getReasonValue("membershipReason") is non-empty (and show a client-side
error/return if empty), set a local flag like isActivatingMembership or disable
the UI control (e.g., the membership button referenced near
membershipReasonSelect/membershipReasonOther), perform the await request, and in
a finally block clear the flag or re-enable the button and still call
loadUserDetail/loadUsers on success; keep existing error handling via
showUserActionResult for failures.
---
Nitpick comments:
In
`@manabom/src/main/java/mannabom_server/manabom/application/admin/dto/request/AdminActivateMembershipRequest.java`:
- Around line 10-11: The DTO field 'reason' in AdminActivateMembershipRequest
currently only has `@NotBlank` allowing arbitrarily long input; add a length
constraint (e.g., annotate the field with `@Size`(max = 255) and a matching
message like "reason은 최대 255자입니다.") to fix the DTO contract, import
javax.validation.constraints.Size, and adjust any tests or validation messages
that assert on this DTO to reflect the new max length.
In `@manabom/src/main/resources/static/admin/app.js`:
- Around line 832-834: The kvHtml function currently injects valueHtml without
escaping, creating a future XSS regression; either change the function name to
kvTrustedHtml to signal it requires already-escaped/trusted HTML or update
kvHtml to accept untrusted input and escape it (use escapeHtml) before
concatenation, and add a short JSDoc comment above the function documenting the
contract (e.g., "`@param` {string} valueHtml - must be trusted HTML" if renaming,
or "`@param` {string} value - untrusted text will be escaped" if changing
behavior) so callers know the safety expectations; reference kvHtml (or
kvTrustedHtml if renaming) when making the change.
🪄 Autofix (Beta)
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
Run ID: 3c7e6fe2-a5e3-4b24-8d74-2017dff969d2
📒 Files selected for processing (11)
manabom/src/main/java/mannabom_server/manabom/application/admin/dto/request/AdminActivateMembershipRequest.javamanabom/src/main/java/mannabom_server/manabom/application/admin/dto/response/AdminWalletResponse.javamanabom/src/main/java/mannabom_server/manabom/application/admin/service/AdminWalletService.javamanabom/src/main/java/mannabom_server/manabom/application/userInfo/service/UserInfoService.javamanabom/src/main/java/mannabom_server/manabom/domain/admin/enums/AdminAuditActionType.javamanabom/src/main/java/mannabom_server/manabom/infrastructure/security/admin/AdminJwtUtil.javamanabom/src/main/java/mannabom_server/manabom/presentation/admin/controller/AdminWalletController.javamanabom/src/main/java/mannabom_server/manabom/presentation/userInfo/controller/UserInfoController.javamanabom/src/main/resources/application-prod.ymlmanabom/src/main/resources/application.ymlmanabom/src/main/resources/static/admin/app.js
💤 Files with no reviewable changes (2)
- manabom/src/main/java/mannabom_server/manabom/presentation/userInfo/controller/UserInfoController.java
- manabom/src/main/java/mannabom_server/manabom/application/userInfo/service/UserInfoService.java
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
manabom/src/main/resources/static/admin/app.js (2)
343-347:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win권한 없는 관리자에게도 멤버십 활성화 UI가 노출됩니다.
백엔드는
AdminWalletService.activateMembership에서SUPER_ADMIN/FINANCE만 허용하는데, 여기서는 모든 관리자에게 버튼이 보입니다. 권한이 없는 역할은 클릭 후 403만 보게 되니state.admin.roles기준으로 이 섹션을 숨기거나 비활성화해 주세요.수정 예시
function renderUserDetail(user) { + const canActivateMembership = (state.admin?.roles || []).some((role) => + role === "SUPER_ADMIN" || role === "FINANCE" + ); $("selectedUserLabel").textContent = `userId ${user.userId}`; $("userActionResult").textContent = ""; $("userDetail").className = "detail-body"; $("userDetail").innerHTML = ` @@ - <section class="action-box"> - <p class="section-title">멤버십 활성화</p> - ${reasonControl("membershipReason", "멤버십 활성화 사유", ["결제 확인", "결제 보정", "이벤트 지급", "기타"])} - <button id="membershipActivateButton" class="secondary">멤버십 활성화</button> - </section> + ${canActivateMembership ? ` + <section class="action-box"> + <p class="section-title">멤버십 활성화</p> + ${reasonControl("membershipReason", "멤버십 활성화 사유", ["결제 확인", "결제 보정", "이벤트 지급", "기타"])} + <button id="membershipActivateButton" class="secondary">멤버십 활성화</button> + </section> + ` : ""}🤖 Prompt for 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. In `@manabom/src/main/resources/static/admin/app.js` around lines 343 - 347, The membership activation UI is shown to all admins but backend AdminWalletService.activateMembership allows only SUPER_ADMIN/FINANCE; update the front-end to check state.admin.roles and either hide the whole <section> or disable the membershipActivateButton and reasonControl unless the current admin has role SUPER_ADMIN or FINANCE. Locate the section that renders membershipActivateButton and reasonControl, add a role check against state.admin.roles (allowing "SUPER_ADMIN" or "FINANCE") and conditionally render or set the button to disabled with a tooltip/aria-disabled to prevent unauthorized clicks.
438-450:⚠️ Potential issue | 🟠 Major | ⚡ Quick win쓰기 성공과 후속 새로고침 실패를 같은 실패로 처리하지 마세요.
Line 439의
POST가 성공한 뒤loadUserDetail또는loadUsers만 실패해도 catch로 떨어져"멤버십 활성화 실패"가 표시됩니다. 실제 서버 반영과 감사 로그는 이미 끝난 상태라, 운영자가 재시도하면서 중복 활성화/중복 감사 로그를 만들 수 있습니다. 쓰기 성공 여부와 화면 재동기화 실패를 분리해서 처리하는 게 맞습니다.수정 예시
try { await request(`/api/admin/users/${state.selectedUserId}/wallet/membership`, { method: "POST", body: { reason } }); $("membershipReasonSelect").value = "결제 확인"; $("membershipReasonOther").value = ""; $("membershipReasonOther").classList.add("hidden"); - await loadUserDetail(state.selectedUserId); - await loadUsers(); showUserActionResult("멤버십이 활성화되었습니다."); + try { + await loadUserDetail(state.selectedUserId); + await loadUsers(); + } catch (refreshError) { + showUserActionResult("멤버십은 활성화됐지만 화면 동기화에 실패했습니다. 다시 조회해 주세요.", true); + } } catch (error) { showUserActionResult(`멤버십 활성화 실패: ${error.message}`, true); } finally {🤖 Prompt for 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. In `@manabom/src/main/resources/static/admin/app.js` around lines 438 - 450, The POST request for activating membership (request(`/api/admin/users/${state.selectedUserId}/wallet/membership`)) should be treated separately from UI refresh errors: after the await request call succeeds, immediately call showUserActionResult("멤버십이 활성화되었습니다.") (or similar success messaging) to reflect the committed write, then perform loadUserDetail(state.selectedUserId) and loadUsers() inside their own try/catch; if those refresh calls fail, catch and log/display a non-fatal warning (e.g., "멤버십은 활성화되었으나 화면 동기화에 실패했습니다.") rather than treating it as a full activation failure, and avoid retrying the POST to prevent duplicate activations or audit entries. Ensure you reference the same symbols: request, state.selectedUserId, loadUserDetail, loadUsers, and showUserActionResult when applying this change.
🤖 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
`@manabom/src/main/java/mannabom_server/manabom/presentation/admin/controller/AdminWalletController.java`:
- Around line 65-69: The current logic in AdminWalletController that returns
forwarded.split(",")[0].trim() can propagate empty or garbage hops; change the
handling of the "X-Forwarded-For" header in the method where forwarded and
remoteAddr are used so you iterate over forwarded.split(",") tokens, trim each
token and select the first non-blank and non-"unknown" (case-insensitive) token
as the client IP, and if none found return remoteAddr; update the code paths
that call AdminWalletService.activateMembership(...) and any audit logging to
use this validated IP.
---
Outside diff comments:
In `@manabom/src/main/resources/static/admin/app.js`:
- Around line 343-347: The membership activation UI is shown to all admins but
backend AdminWalletService.activateMembership allows only SUPER_ADMIN/FINANCE;
update the front-end to check state.admin.roles and either hide the whole
<section> or disable the membershipActivateButton and reasonControl unless the
current admin has role SUPER_ADMIN or FINANCE. Locate the section that renders
membershipActivateButton and reasonControl, add a role check against
state.admin.roles (allowing "SUPER_ADMIN" or "FINANCE") and conditionally render
or set the button to disabled with a tooltip/aria-disabled to prevent
unauthorized clicks.
- Around line 438-450: The POST request for activating membership
(request(`/api/admin/users/${state.selectedUserId}/wallet/membership`)) should
be treated separately from UI refresh errors: after the await request call
succeeds, immediately call showUserActionResult("멤버십이 활성화되었습니다.") (or similar
success messaging) to reflect the committed write, then perform
loadUserDetail(state.selectedUserId) and loadUsers() inside their own try/catch;
if those refresh calls fail, catch and log/display a non-fatal warning (e.g.,
"멤버십은 활성화되었으나 화면 동기화에 실패했습니다.") rather than treating it as a full activation
failure, and avoid retrying the POST to prevent duplicate activations or audit
entries. Ensure you reference the same symbols: request, state.selectedUserId,
loadUserDetail, loadUsers, and showUserActionResult when applying this change.
🪄 Autofix (Beta)
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
Run ID: 95601d14-19ba-4ddf-8137-50bb434fb2e1
📒 Files selected for processing (4)
manabom/src/main/java/mannabom_server/manabom/presentation/admin/controller/AdminWalletController.javamanabom/src/main/resources/application-prod.ymlmanabom/src/main/resources/application.ymlmanabom/src/main/resources/static/admin/app.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
변경 사항
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores