과제 Lv3 완료

[Hohomii]

[리팩토링 시도해볼 부분]

• Dto에 이너클래스를 사용하여 코드 간 응집도 높이기
• ResponseDto를 제네릭을 사용해서 변경해보기

---

[질문]

1. 지금 저희가 쓰고 있는 정도의 예외 처리는 표준 예외를 쓰는 것이 더 나을까요?

2. 예외처리 부분에 대한 코드리뷰를 받고 싶습니다! 처음에는 GlobalExceptionHandler 안에 커스텀 익셉션 클래스들을 쭉 썼었는데 중복값이 많아져서 찾아보니 CustomException 클래스를 따로 빼면 된다고 하여 그렇게 했습니다. 근데 커스텀익셉션이 상속 받고 있는 RuntimeException 외의 예외들도 따로 처리해줘야 하는 건지 의문이 들었습니다. 또 ErrorCode enum 클래스의 응답값을 줄 때 responseEntity를 썼는데 거기서 왜 HttpStatus를 .status와 .body에서 두번이나 불러오고 있는지 이해가 잘 안 됩니다. 이전에 예외처리를 아주아주 간단한 try-catch 정도밖에 안 써봐서 어찌어찌 찾아서 해봤는데😓 지금 쓴 이 방식이 괜찮은지, 현업에서는 어떤 식으로 다루고 있는지 궁금합니다.

3. 스프링 시큐리티 적용중입니다. 시큐리티 적용으로 인해 기존의 customException 핸들러가 필요없게 된 부분이 생겼습니다. 그런데 그 중 토큰이 없는 상태에서 글 작성을 시도하면 NullPointerException이 발생합니다. 시큐리티 부분에서 처리해주려고 CustomAccessDeniedHandler와 CustomAuthenticationEntryPoint를 적용했는데도 같은 에러가 발생하는데 원인을 못 찾고 있습니다. 도움 부탁드립니다!

[Hohomii]

3번 해결되었습니다. 해결 내용은 아래에 첨부하겠습니다.

---

토큰을 넣지 않고 게시글 작성하려 하니 nullpointerexception 발생. CustomAccessDeniedHandler와 CustomAuthenticationEntryPoint를 적용했는데도 잡히지 않음. 스택 트레이스를 살펴보니 boardController의 createBoard에서 인자로 받고 있는 useDetails에서 발생하는 에러였음. UseDetailsImpl과 UseDetailsServiceImpl에서 예외 처리를 시도해보았으나 똑같은 에러가 발생하였음.

• 원인 :
  • http.authorizeRequests().antMatchers("/api/board").permitAll() 에 의해 board에 토큰 없이 접근하는 것이 허용됨 ⇒ 시큐리티 인증 영역을 벗어남
  • 인증을 통해 선별이 안 된 채로 UserDetails 로 넘어가게 되고, boardController에서 userDetails에 null값이 들어갔는데 처리해주는 부분이 없으니 에러가 발생.

• 해결 :

  • 1차 해결은 BoardController의 createBoard에서 userDetails가 null값이 들어올 때 예외처리 해줌
  • 2차 근본적인 해결은 WebSecurityConfig에서 페이지 허용하는 부분 http.authorizeRequests().antMatchers("/api/board").permitAll() ← 여기에 url 앞에 HttpMethod.GET을 넣어줌.

  http.authorizeRequests().antMatchers(HttpMethod.GET,"/api/board").permitAll()

  • 이렇게 하면 GET 요청만 허용한다는 뜻. 즉 POST 요청 시에는 시큐리티에 설정해놓은 CustomAccessDeniedHandler와 CustomAuthenticationEntryPoint 가 작동되어 해당 예외 처리가 실행된다.

[slsnrnsep]

1. 일반적으로 표준예외를 활용할 수 있다면 표준예외를 사용하는 것이 좋습니다. NullPotiterException, EntityNotFoundException 등 그러나 예외가 발생했을 때, 별도의 메시지를 추가로 전달하고 싶으시다면, 별도로 같은 이름의 Exception을 구현하여서 한번에 처리를 하는 방법 등이 더 좋습니다.

2.스프링에서는 상속관계에 있을때에, 구체적인 디테일한 정보부터 우선으로 처리하게끔 되어있습니다. 현재 GlobalExceptionHandler에서 RuntimeException을 상속받은 CustomException만 처리를 하도록 되어 있어서 나머지 RuntimeException에 대해서는 처리할 수 없습니다.

    @ExceptionHandler(RunTimeException.class)
    @ExceptionHandler(Exception.class)

따라서 2가지 경우를 처리해주게 된다면 발생하는 모든 예외들은 저기 2가지에서 처리가 가능하며, 더 디테일하게 메세지를 다르게 보내야 하는 경우가 있다면 상속을 활용하여 묶어서 처리해주면 좋을 것 같습니다. 저같은 경우에는 ExceptionRoot라는 CustomeException들의 부모를 만들어서 전역예외처리에서는 ExceptionRoot.class를 처리하는 편입니다.

3.스프링 시큐리티의 주요예외는 2가지가 있습니다. AuthenticationException 와 AccessDeniedException 2가지가 있습니다. 현재 코드에서도

http.exceptionHandling().authenticationEntryPoint(customAuthenticationEntryPoint);
http.exceptionHandling().accessDeniedHandler(customAccessDeniedHandler);

를 통해서 주요예외를 따로 처리해주는 Custom Class를 구현하셨기 떄문에 , 발생하는 에러들은 CustomClass에서 @OverRide된 곳에서 처리가 될 것입니다.

기존에 만들어둔 ExceptionHandler에서 처리를 하고 싶다면, authenticationEntryPoint에서 @ExceptionHandler({ AuthenticationException.class }) 와, HandlerExceptionResolver 를 사용하여 기존에 만들어둔 쪽에서 처리를 하도록 위임하는 방법도 가능합니다. 관련해서 https://velog.io/@dailylifecoding/spring-security-exception-handling-and-request-caching 에 그림이 시큐리티 예외처리에 대해 그림으로 잘나와있는 것 같아서 한번 보시는 것을 추천드립니다!

[slsnrnsep]

추가로 LV3까지의 코드리뷰 공유드립니다! Controller 패키지 우선 지난번보다 Restful해진 것 같아서 코드를 이해하는데에 더 수월했습니다. 그러나 스프링에서 제공하는 ResponseEntity를 사용하여 반환을 하고 계신데, 제네릭을 활용하여 별도의 ReponseDto를 만들어서 관리하면 더 유지보수에도 용이하고 유저에게 필요한 정보들과 code,msg등을 동시에 전달해줄 수 있어보입니다.

@Getter
public class ApiResponse<T> {
    private boolean success;
    private T response;
    private ErrorResponse error;

    public ApiResponse(boolean success) {
        this.success = success;
    }

    public ApiResponse(boolean success, T response, ErrorResponse error) {
        this.success = success;
        this.response = response;
        this.error = error;
    }
}

저같은 경우에는 더많은 에러 정보를 담기위해 ErrorResponse라는 클래스도 별도로 만들었는데, 단순한 String 필드로 message를 선언하셔도 무방합니다.

---

dto 패키지 // 왜 이 어노테이션들이 다 필요한가? 이 dto가 쓰이는 상황이 어떤 상황이기에? @AllArgsConstructor @NoArgsConstructor @Getter @Setter

이런 주석문을 남겨주신 것처럼, 사용하지 않거나 불필요한 어노테이션은 제외하는 편이 좋습니다. 기본적으로 dto에는 @getter와 기본생성자만 들어가는게 default이며 필요한 경우에만 추가해주시는게 좋아보입니다.

---

Entity패키지 board나 comment를 현재는 user와 연관관계를 맺으면서 작성자명 같은 정보를 꺼내시는 것처럼 보입니다. 그러나 앞으로 추가될 entity에도 작성자,수정한사람 등의 정보들이 계속 필요로 할텐데, 그떄마다 연관관계를 맺어서 관리하기 보다는 Jpa의 Auditting 기능을 활용해보는 것은 어떨까요? @createdby 라는 키워드로 검색해보시면, 자동으로 jpa에서 작성자 userID를 DB에 insert될때 심어넣어줄 수 있습니다! 그렇다면 post와 comment에는 누가 작성했는지 편리하게 관리할 수 있으며 작성자명 또한 추적이 가능하답니다.

---

예외처리 관련 enum으로 잘 분리는 하셨습니다. 그러나 code와 message를 따로 분리할 필요없이, enum에 필드값으로 code,msg를 선언하여 하나의 Enum으로 관리하는게 더 좋을 것 같아요. 지금도 CustomException을 구성하여 관리하고 계시지만, 필드값으로 하나의 enum으로 예외처리하면 더 좋을것 같습니다.

---

Service 패키지 한번 깔끔하게 리팩토링 한 흔적이 보이는 것 같아서 좋습니다. 코드가 읽기 쉬우나 관리자 권한을 검사하는 부분을 서비스단에 중복적으로 계속해서 사용하고 계신데, Security기능 중 @secured나 @PreAuthorize 어노테이션 들을 이용하여 관리자가 아니라면 컨트롤러 앞에서 막히도록 가능합니다. 또한 config에 requestMatchers에서도 hasRole() 등을 사용하여 제한할 수 있으므로 서비스단에 중복된 코드를 최대한 줄이시면 더 좋을 것 같네요.

---

전체적으로 지난번 리뷰당시보다 코드의 가독성과 구조가 좋아진 것이 느껴집니다. 많은 고민을 하신게 느껴지며 이제 앞으로는 배포도 하셔야 하니 application.properties 파일은 github에 올리시면 안됩니다. (DB정보나 Jwt SecretKey가 노출되어 있어요) - 해커나 장난치는 사람들이 repo를 검색하여, aws 폭탄요금을 부과하게 한다던가, 시크릿키가 노출되어 관리자가 아님에도 여러 기능들을 수행하도록 하면 안되겠죠? 간단하게는 gitignore에서 처리가 가능하고, 암호화하는 jasypt라는 라이브러리도 있으니 검색한번 해보시면 좋을 것 같아요.

[Hohomii]

자세하게 설명해주시고 더 나아갈 부분까지 꼼꼼히 짚어주셔서 도움이 많이 됩니다! 감사합니다 🧡💛💚