Conversation
Uralauah
left a comment
Uralauah
left a comment
There was a problem hiding this comment.
고생하셨습니다! 전반적으로 잘 작성된 코드 같습니다.
몇가지 수정하면 좋을 것 같은 내용이 있어 리뷰 남깁니다!
| Optional<UserSanction> unreadSanctionOpt = sanctionService.findLatestUnreadSanction(user); | ||
| AuthExceptionCode code = user.getUserStatus() == UserStatus.SUSPENDED ? | ||
| AuthExceptionCode.ACCOUNT_SUSPENDED : AuthExceptionCode.ACCOUNT_BANNED; | ||
|
|
||
| UserSanction sanction = unreadSanctionOpt.get(); |
There was a problem hiding this comment.
Optional처럼 Optional로 했다면 값이 null일 가능성이 아예 없는게 아니기 때문에
이렇게 바로 .get()을 하기보다는 다른 로직들 처럼
UserSanction sanction = sanctionService.findLatestUnreadSanction(user)
.orElseThrow(() -> new CustomException(AuthExceptionCode.SANCTION_NOT_FOUND));이런식으로 처리해주는게 안전할 것 같습니다!
There was a problem hiding this comment.
동의합니다! get ( )는 값이 없을 때 호출되면 바로 에러가 발생되기에 지양해야 할 것 같습니다.
| @Transactional | ||
| public void markSanctionAsRead(UserSanction sanction) { | ||
| sanction.markAsRead(); | ||
| } |
There was a problem hiding this comment.
이 코드는 굳이 없어도 되는 코드 같습니다.
읽음 처리하는 로직에서 바로
saction.markAsRead()를 호출하는게 더 좋을 것 같습니다
There was a problem hiding this comment.
네! 그게 더 좋겠네요 감사합니다
| } | ||
|
|
||
| public void incrementWarningCount() { | ||
| this.warningCount = (this.warningCount == null) ? 1 : this.warningCount + 1; |
There was a problem hiding this comment.
User의 warningCount의 기본값을 0으로 지정해서 null 체크 없이 작성하는게 더 괜찮을 것 같습니다!
There was a problem hiding this comment.
네! 그게 더 좋은 방법 이겠네요
| User user = userRepository.findByEmail(signInReq.email()) | ||
| .orElseThrow(() -> new CustomException(AuthExceptionCode.INVALID_USERNAME_AND_PASSWORD)); | ||
|
|
||
| // 1. 제재/차단 상태 확인 (로그인 차단) |
There was a problem hiding this comment.
현재 로그인 로직이
- 제재/차단 상태 확인 → 2. 비밀번호 검증 순서로 처리되고 있는데
이 구조는 보안 측면에서 계정 존재 여부가 노출되는 문제가 생길 수 있습니다.
예를 들어,
해커가 무작위 이메일,비밀번호 조합으로 로그인 시도를 반복하면
- 존재하지 않는 이메일인 경우 →
INVALID_USERNAME_AND_PASSWORD - 존재하는 이메일이고 정지된 계정인 경우 →
ACCOUNT_SUSPENDED
와 같이 서로 다른 응답을 반환하게 됩니다.
이 차이를 통해 공격자는
어떤 이메일이 우리 서비스에 가입되어 있는지 알아낼 수 있으며,
이는 전형적인 계정 열거 취약점이라고 합니다.
따라서, 이메일 + 비밀번호가 올바른지 먼저 검증하고,
이 검증이 통과된 경우에만 제재/차단 상태를 확인하는 방식으로 순서를 재배치하는 것이 좋을 것 같습니다.
이렇게 하면 모든 로그인 실패 시 동일한 에러 응답을 반환할 수 있어 계정 존재 여부를 외부에 노출하지 않는 보다 안전한 인증 구조가 됩니다.
There was a problem hiding this comment.
너무 맞는말이네요 당연한 문젠데 인지하지 못했던 거 같습니다
There was a problem hiding this comment.
수고하셨습니다!! 꼼꼼하게 작성해주신 것이 보여 저도 열심히 읽어보았습니다!!
각 코드에 대한 리뷰는 달아두었고, 전체적인 논의점이 있어 해당 리뷰란에 적어봅니다.
논의가 필요한 지점 - 신고 기록과 제재 기능의 연결
현재 제재 프로세스를 정리하면 다음과 같습니다.
- 유저가 다른 유저를 신고한다.
- 관리자가 신고 목록을 확인한다.
- 관리자가 유저 목록으로 넘어가 신고받은 유저를 닉네임으로 찾는다.
- 해당 유저 상세 화면에서 “경고/정지” 버튼을 눌러 제재를 가한다.
이 흐름에서 몇 가지 문제가 발생합니다.
물론 프론트와 함께 로직 논의를 해야하지만, 개인적으로 해결 방법을 생각해보았습니다.
문제점 1 — 신고 기록과 제재 간의 연결 정보가 사라짐
UserSanction에는 특정 UserReport를 근거로 제재를 기록할 수 있지만,
현재 UI 상에서는 sourceReportId를 전달할 방법이 없습니다.
- 제재 화면에는 신고 ID를 입력하거나 선택하는 UI가 없음
- 신고 상세 페이지 → 제재 화면으로 이동하는 플로우도 없음
- 따라서 현재 구조에서는
sourceReportId가 항상null로 들어오게 됨
즉, 어떤 신고를 근거로 어떤 제재를 했는지 기록이 남지 않는 문제가 발생합니다.
이를 해결하기 위해 다음과 같은 방법을 생각해보았습니다!
해결법 1 — 신고 상세 화면 → “제재하기” 버튼 제공
- 신고 상세에서 바로 제재 페이지로 이동
- 신고 ID를 자동으로 API에 포함
- 관리자는 신고 ID를 알 필요 없음
해결법 2 — 제재 화면에 “신고 ID 선택/입력” UI 추가
- 간단
해결법 3 — sourceReportId 매핑을 추후 기능으로 빼기
- 초간단
문제점 2 — 제재 대상 유저의 UUID를 알 수 없음
제재 API는 유저를 UUID로 식별하지만,
- 신고 목록 UI에는 대상자의 닉네임만 표시
- 유저 목록 UI에도 UUID나 ID가 없음
- 보안상 고유 ID를 화면에 직접 노출하는 것도 바람직하지 않음
결과적으로 관리자는 제재 대상 유저의 UUID를 알 수 없고, 입력할 수도 없습니다.
이 문제는 유저 목록 GET API에서 uuid를 함께 내려주고
프론트가 노출하지 않고 내부적으로만 보관하는 방식으로 해결할 수 있을 것 같습니다.
| Optional<UserSanction> unreadSanctionOpt = sanctionService.findLatestUnreadSanction(user); | ||
| AuthExceptionCode code = user.getUserStatus() == UserStatus.SUSPENDED ? | ||
| AuthExceptionCode.ACCOUNT_SUSPENDED : AuthExceptionCode.ACCOUNT_BANNED; | ||
|
|
||
| UserSanction sanction = unreadSanctionOpt.get(); |
There was a problem hiding this comment.
동의합니다! get ( )는 값이 없을 때 호출되면 바로 에러가 발생되기에 지양해야 할 것 같습니다.
| if (user.getUserStatus() == UserStatus.SUSPENDED || user.getUserStatus() == UserStatus.BANNED) { | ||
| Optional<UserSanction> unreadSanctionOpt = sanctionService.findLatestUnreadSanction(user); | ||
| AuthExceptionCode code = user.getUserStatus() == UserStatus.SUSPENDED ? | ||
| AuthExceptionCode.ACCOUNT_SUSPENDED : AuthExceptionCode.ACCOUNT_BANNED; |
There was a problem hiding this comment.
해당 signIn 함수가 너무 많은 비즈니스 로직을 직접 처리하고 있다고 느껴집니다.
특히 UserStatus에 대한 상태 판단 로직까지 서비스 레이어에서 담당하는 구조는 SRP 관점에서 조금 아쉽습니다.
enum은 단순한 값의 나열뿐 아니라, 각 상태가 어떤 의미를 가지는지 스스로 판단하는 책임도 함께 가져가는 것이 더 적합하다고 생각합니다.
예를 들어 UserStatus 내부에서 로그인 차단 여부를 판단하도록 변경하면 다음처럼 더 깔끔해질 수 있습니다.
public enum UserStatus {
ACTIVE,
SUSPENDED,
BANNED;
public boolean isLoginBlocked() {
return this == SUSPENDED || this == BANNED;
}
}서비스 로직도 다음처럼 훨씬 가독성이 좋아집니다.
UserStatus userStatus = user.getUserStatus();
if (userStatus.isLoginBlocked()) {
...
}또한 UserStatus 내부에서 각 상태에 대응하는 적절한 ExceptionCode까지 반환하도록 만들면 서비스 코드가 더욱 단순해질 것 같습니다.
if (status.isLoginBlocked()) {
throw new CustomException(status.getExceptionCode());
}다만, 현재 구조에서는 UserStatus가 AuthExceptionCode를 참조하게 되면
gotcha-domain-user ↔ gotcha-domain-auth 간의 순환 의존성이 발생하여 적용이 어렵다는 점을 확인했습니다.
그럼에도, 책임을 enum 내부로 이동하는 방향은 좋은 개선이라고 생각합니다!
서비스 레벨의 복잡도를 줄이고, UserStatus의 역할을 명확히 할 수 있을 것 같습니담
There was a problem hiding this comment.
가독성이 훨씬 좋아지겠네요! 좋은 제시 감사합니다
| public Optional<UserSanction> findLatestUnreadSanction(User user) { | ||
| return sanctionRepository.findTopByUserAndIsReadIsFalseOrderByCreatedAtDesc(user); | ||
| } | ||
|
|
||
| public Optional<UserSanction> findLatestUnreadWarning(User user) { | ||
| return sanctionRepository.findTopByUserAndSanctionTypeAndIsReadIsFalseOrderByCreatedAtDesc(user, SanctionType.WARNING); | ||
| } |
There was a problem hiding this comment.
해당 함수는 다음과 같은 오버로딩을 사용하면 더 명확하고 활용 가능성 있게 사용할 수 있을 것 같습니다.
public Optional<UserSanction> findLatestUnread(User user) {
return sanctionRepository.findTopByUserAndIsReadIsFalseOrderByCreatedAtDesc(user);
}
public Optional<UserSanction> findLatestUnread(User user, SanctionType type) {
return sanctionRepository.findTopByUserAndSanctionTypeAndIsReadIsFalseOrderByCreatedAtDesc(user, type);
}There was a problem hiding this comment.
좋은 개선 방안이네요 👍 감사합니다
| // 3. 제재 유형에 따른 로직 처리 | ||
| LocalDateTime expiresAt = null; // 제재 만료 시간 | ||
| SanctionType sanctionType = sanctionReq.getSanctionType(); | ||
| switch(sanctionType){ | ||
| case WARNING: | ||
| targetUser.incrementWarningCount(); | ||
| // 경고는 만료 시간이 없음 | ||
| break; | ||
| case TEMP_BAN: | ||
| // 임시 정지는 현재 시간에서 지정된 기간만큼 더함 | ||
| Long durationDays = sanctionReq.getDurationDays(); | ||
| expiresAt = LocalDateTime.now().plusDays(durationDays); | ||
| targetUser.suspendUser(durationDays); | ||
| break; | ||
| case PERM_BAN: | ||
| // 영구 정지는 만료 시간이 없음 | ||
| targetUser.banUser(); | ||
| break; | ||
| } |
There was a problem hiding this comment.
현재 각 enum 별 처리 전략을 비즈니스 로직에서 관리하니, 하나의 로직이 너무 많은 책임 및 확장성에 어려움이 있는 것 같습니다.
다음과 같이 enum 객체가 각 값에 대한 처리 전략을 가진다면 srp 및 ocp를 고려한 코드가 될 것 같습니다!
public enum SanctionType {
WARNING {
@Override
public void apply(User user, Long days) {
user.incrementWarningCount();
}
},
TEMP_BAN {
@Override
public void apply(User user, Long days) {
user.suspendUser(days);
}
},
PERM_BAN {
@Override
public void apply(User user, Long days) {
user.banUser();
}
};
public abstract void apply(User user, Long days);
}서비스 내 사용 코드
// 3. 제재 유형에 따른 로직 처리
SanctionType sanctionType = sanctionReq.getSanctionType();
sanctionType.apply(targetUser, sanctionReq.getDurationDays());There was a problem hiding this comment.
너무 깔끔한 개선 제안이네요 👍 감사합니다
| // 제재 만료 일시 | ||
| @Column(name = "expiry_date") | ||
| private LocalDateTime expiresAt; |
There was a problem hiding this comment.
현재 로직에서는 해당 필드에 제재 만료 일시가 들어오는데, 사실 해당 정보는 이미 User 필드 내에 업데이트 되어 있고, 로그인 시 User 필드를 통해 확인해주고 있습니다.
해당 필드를 제재 만료 일시가 아닌 제재 기간으로 용도를 변경하는 건 어떤가요?
그럼 해당 엔티티는 관리자가 설정한 제재 기간(ex. 3일)에 대한 정보만 보관하면 됩니다.
그렇게 되면 SanctionService 에서도 만료 시간을 계산해줄 필요가 없고, 해당 만료 시간 계산 로직은 user 엔티티 내에만 존재할 수 있게 됩니다!
There was a problem hiding this comment.
좋은 의견이네요 그렇게 하는 게 좋을 것 같습니다!
| @RestController | ||
| @RequestMapping("/api/v1/admin/sanctions") | ||
| @RequiredArgsConstructor | ||
| public class SanctionController implements SanctionApi { |
There was a problem hiding this comment.
관리자가 유저의 정지/경고 요청 전송을 하기 위해 리스트를 반환하는 getMapping을 추가해야 할 것 같습니다.
또한 현재 피그마에는 정지 취소 기능이 있는데, 해당 기능도 추가해야 할 것 같습니다!
There was a problem hiding this comment.
"유저의 정지/경고 요청 전송을 위해 리스트를 반환하는" 이 무슨 뜻인가요..?? 해당 유저의 정지/경고 내역을 반환하는 리스트라면 필요한 것 같은데, 관리자가 유저에 대한 정지/경고 를 만드는(Post) 요청은 1. UserReport 리스트를 조회한 다음 그 곳에서 Post 요청, 2. 게임 대기방 or 채팅창에서 Post 요청 이라고 생각했습니다
There was a problem hiding this comment.
정지 취소 기능은 필요할 것 같네요! 제가 놓친 부분인 것 같습니다 (__)
There was a problem hiding this comment.
저희 이 피그마 화면에 대한 get 요청이 없다는 의미였습니다!
그러나 제가 2달 전의 회의로 해당 부분을 구현하지 말자는 사실을 까먹고 리뷰를 남겼습니당..ㅎㅎ 형준님 로직이 현재 회의된 로직이 맞습니다.😓
| @NoArgsConstructor | ||
| public class SanctionReq { | ||
|
|
||
| @NotBlank(message = "제재 대상의 ID는 필수입니다.") |
There was a problem hiding this comment.
현재 요청 dto에 제재 대상의 id를 필요로 하고 있는데, 이는 로직 내에선 uuid 이므로 메시지 및 필드를 변경해야 할 것 같습니다!
There was a problem hiding this comment.
네 가독성을 위해서 그렇게 하는게 좋겠네요.!
| @NotNull(message = "제재 유형은 필수입니다.") | ||
| private SanctionType sanctionType; | ||
|
|
||
| private Long sourceReportId; // 제재의 근거가 되는 신고 ID (nullable) |
There was a problem hiding this comment.
이건 리뷰보단 하나의 제안으로 남깁니다!
현재 로직에선 신고 기록을 넘겨도 되고, 넘기지 않아도 되는데 항상 넘기는 것이 맞지 않을까요?
신고 기록이 없는 제재가 가능하다는 건 관리자의 근거없는 차별적 제재가 가능하다는 의미가 되어 위험할 것 같습니다!
There was a problem hiding this comment.
정책 상에서 논의해야 할 부분인 것 같습니다. 제가 판단했을 때는 관리자가 유저에 대한 정지/경고 를 만드는 케이스가
- UserReport를 조회해둔 상태에서 제재
- 채팅창/게임방에서 즉시 제재
로 발생할 수 있다고 생각하였습니다.
There was a problem hiding this comment.
이건 추후 팀원들과 논의해보는 게 좋겠네요!!
| @Column(name = "suspension_end_date") | ||
| private LocalDateTime suspensionEndDate; | ||
|
|
There was a problem hiding this comment.
현재 제재 기간이 끝나면, 해당 유저의 상태를 다시 ACTIVE로 돌려주는 기능이 없는 것 같습니다!
There was a problem hiding this comment.
맞네요 "임시 정지된 유저의 경우" 매 로그인 마다 시간이 지났는지 아닌지를 체크 해주는 로직으로 생각해두고 있었는데 추가한다는걸 깜빡했습니다 (__)
| ResponseEntity<SanctionRes> applySanction( | ||
| @Valid @RequestBody SanctionReq sanctionReq, | ||
| SecurityUserDetails userDetails); | ||
| } |
There was a problem hiding this comment.
스웨거 설명이 불충분한 것 같습니다! ㅠㅠ 저도 테스트를 해보다가 어떤 의미인지 모르겠어서 로직을 직접 보고 유추했습니다.
다음과 같이 설명을 더 보충하는 건 어떤가요?
- targetUserId는 targetUserUuid로 변경
- sanctionType enum에 어떤 값이 있는지 설명
- sourceReportId, durationDays가 의미 설명
There was a problem hiding this comment.
아 거기까지 생각하지 못했습니다 지적하신 대로 설명을 보충하는게 좋겠네요
전에 회의했을 때 이런식으로 얘기를 하긴 했었습니다
|
아하!! 얘기 나눠주셔서 감사합니다 그랬군요!!! 그럼 reportId는 해결 될 것 같습니다!! uuid 문제는 기존 신고 목록 반환 api에서 신고 대상자 Uuid도 함께 넘겨주도록 수정하면 이 문제가 해결 될 거 같슴니다!! 😄 |
Jackson이 getter를 필드로 인식해 "suspensionExpired"가 캐시에 저장되던 문제 해결.
3 participants
feat: 사용자 정지/경고 기능 구현
📝 개요
사용자 정지/경고 기능을 구현하였습니다.
경고의 경우 사용자의 최초 로그인 시 해당 메시지가 전달되도록
정지의 경우 해당 기간동안 로그인에 실패하도록 구현하였습니다.
⚙️ 구현 내용
UserSanction 엔터티를 추가하였습니다. 해당 사용자가 받은 제재 내역을 담고 있으며, 경우에 따라 유저 신고 내역인 UserReport와도 연관관계를 가질 수 있습니다.
UserSanction 테이블에 해당 제재에 대한 알림을 전송했는지를 뜻하는 필드인 Read 필드를 두었습니다. 이를 이용해 사용자 최초 로그인 시(AuthService) 제재 관련 알림(SanctionService)을 TokenDto에 포함하 보내도록 구현했습니다.
새로운 Exception class를 작성했습니다 : UserAccountStatusException.
사용자 정지로 인한 로그인 실패 시에 Sanction에 대한 자세한 정보를 함께 반환해주기 위해 새로운 Exception 클래스를 작상했습니다.
User 엔터티에 해당 유저의 상태(ACTIVE, SUSPENDED, BANNED)를 나타내는 필드와 일시 정지 종료 기간을 나타내는 필드를 추가했습니다.
📎 기타
UserAccountStatusException에 대해 ... CustomException 에다가 Map 필드를 하나 추가해서 더 디테일한 정보들을 넣을 수 있도록 하는게 더 예쁜 구조가 아닐까 하는 생각도 듭니다. (SocketCustomException에는 구현되어 있는 것으로 확인했습니다)🧪 테스트 결과
경고 - 성공
경고 성공 : userreport 와 연결
경고 받은 유저 로그인 시 알림
한 번 읽은 유저 알림 안 가는 거 확인
정지 성공
영구정지 된 유저 로그인 시도
일시 정지 된 유저 로그인 시도
정지 성공 : 영구밴
제재 실패 : 관리자 권한 없음
제재 실패 : 필드 검증 오류
제재 실패 : 사용자 NOT FOUND
제재 실패 : 신고 내역 NOT FOUND