[Spring MVC (인증)] 정민주 미션 제출합니다. #151

JoungMinJu · 2025-04-23T11:02:48Z

늦게 올려서 죄송합니다 ..!
부족한 부분이 많은 것 같은데 .. 리뷰 해주시면 성실하게 반영하도록 하겠습니다!

seokhwan-an

민주님 spring Mvc(인증) 미션하시느라 고생 많으셨습니다.
코드 리뷰남겼는데 천천히 확인해주세요!

seokhwan-an · 2025-04-24T02:01:17Z

src/main/java/roomescape/common/AuthorizationInterceptor.java

+        Long memberId = loginService.getMemberId(request.getCookies());
+        if (memberId == null) {
+            response.setStatus(401);
+            return false;
+        }


이 부분은 loginService에서 memberId가 null인지 아닌지를 파악해서 예외처리를 하는 것은 어떤가요?
저는 개인적으로 외부에 값을 넘길 때 값이 존재하는지 아닌지를 파악해서 전달하는 편입니다.
null 대신에 optional에 대해 찾아보시고 이용해보면 좋을 것 같습니다.

optional을 사용해보았는데 음 뚜렷한 장점이 뭔지 좀 궁금합니다!!
코드 가독성이 올라간다..정도일까요?!

제가 생각하기에 optional의 장점은 해당 객체가 없는지 있는지를 파악하는데 용이하고 해당 결과가 비워져있을 수 있다는 것을 한눈에 알 수 있다는 것이 장점인 것 같습니다.

자바는 null safe한 언어가 아니기에 넘어온 값이 null이 아닌지 파악하는 작업이 필요한데 모든 부분에 대해서 null인지 아닌지를 체크하는 것은 개발자 입장에서 큰 오버헤드로 작용할 수 있다고 생각하는데요

그래서 Optional로 감싸서 결과를 반환해주면 해당 메소드를 사용하는 입장에서는 해당 응답값이 비워져 있을 수 있다는 힌트를 받을 수 있기에 Optional을 감싼 객체에 대해서만 해당 처리를 하면 된다고 느껴지네요(이 부분은 제가 Optional을 이용하면서 느낀점 입니다.)

seokhwan-an · 2025-04-24T02:04:10Z

src/main/java/roomescape/common/AuthorizationInterceptor.java

+        }
+
+        MemberResponse member = memberService.findById(memberId);
+        if (!member.getRole().equals("ADMIN")) {


이 부분은 member의 상태를 불러와서 외부에서 검증하는 방식이네요! (지금은 검증 로직이 간단하기에 한눈에 의미 파악은 쉽다고 느껴집니다.)
객체의 상태를 불러와서 비교하기보다는 객체에게 메세지를 전달해보는 것은 어떤가요?

예를 들면 member.isAdmin()과 같이 처리하면 보다 한눈에 어떤 의미인지 쉽게 파악할 수 있을 것 같아요!

seokhwan-an · 2025-04-24T02:06:27Z

src/main/java/roomescape/common/AuthorizationInterceptor.java

+@Component
+public class AuthorizationInterceptor implements HandlerInterceptor {


인가처리를 하는 방식은 filter, interceptor, aop 방법이 있을 것 같은데
이 세가지 방법 중 interceptor를 이용하신 이유는 무엇인가요?
각각 어떤 특징이 있는지 파악하면 좋을 것 같아요!

미션 예시에서 인터셉터가 설명되어있어 반영했었습니다!

filter는 모든 http요청을 대상으로, interceptor는 컨트롤러로 가는 요청을 대상으로, aop는 스프링 빈으로 관리되는 메서드 호출을 대상으로 공통적인 처리를 할 수 있다는 특징이 있는데요.

어드민 페이지 진입을 요청한 사용자의 권한을 확인하는 인가 요청의 경우
/admin을 호출하는, 즉 해당 컨트롤러의 요청을 수행하기 직전에 검증하는 것이 옳아서 인터셉터를 인가 과정에 주로 사용하는 것으로 알고 있습니다.

제가 필터와 인터셉터를 이용하면서 느낀 점은 다음과 같습니다.

인터셉터는 spring container에서 관리가 되는 컴포넌트로 controlleradvice 기반의 예외처리가 가능하는 점이 가장 좋았고 필터와 다르게 처리 메소드(preHandle, postHandle, afterCompletion)에는 파라미터로 Object handler를 가지고 있어서 handler를 다룰 수 있다는 특징이 있겠네요

필터의 기존에는 servlet container에서 관리되었다가 지금은 spring container에서 관리되고 있는 것으로 알고 있습니다. 그럼에도 controlleradvice가 적용되지 않아 커스텀한 예외처리하기가 어렵다는 특징이 있지만 filter는 url과 더불에서 http method를 바탕으로 필터 조건을 설정할 수 있다는 것이 가장 큰 장점이자 특징이라 느껴집니다.

seokhwan-an · 2025-04-24T02:09:42Z

src/main/java/roomescape/common/WebConfig.java

+    @Override
+    public void addInterceptors(InterceptorRegistry registry) {
+        registry.addInterceptor(authorizationInterceptor)
+            .order(1)


혹시 순서를 1로 설정하신 이유는 무엇일까요?

혹시 더불어서 인터셉터를 이용하면서 아쉬웠던 점은 없었나요?

order설정은 큰 의미는 없었는데요 ...!
음 어찌됐든 인가와 관련된 인터셉터니까 가장 먼저 실행해서 불필요한 요청은 미리미리 걸러내는게 좋은 선택일 것 같긴합니다!

interceptor 사용시 느꼈던 불편한 점은 딱히 없었습니다.. ! 혹시 석환님은 인터셉터 사용시에 어떤 불편함을 느끼셨을까요?! 궁금해요!

위에 남긴 답변에도 있지만 인터셉터를 이용하면서 아쉬웠던 점은 인터셉터는 url을 통한 설정만 제공하기 때문에 http method 별로 인터셉터 적용을 분리하고 싶은 경우에는 추가적인 작업이 필요하다는 것이 가장 큰 아쉬움으로 다가왔습니다.

seokhwan-an · 2025-04-24T02:11:33Z

src/main/java/roomescape/login/JwtTokenService.java

+@Service
+public class JwtTokenService {


해당 부분에서는 @component를 이용해도 괜찮을 것 같은데 민주님만의 어노테이션을 붙이는 기준이 있으실까요?

음 요건 큰 이유는 없었고 jwt토큰과 관련된 서비스로직을 모아놓은 클래스를 의도하고 만든거라 자연스럽게 @service 어노테이션을 붙이게 되었습니다! 명시적으로 표현해주고싶어서요!

seokhwan-an · 2025-04-24T02:25:15Z

src/main/java/roomescape/login/LoginMemberArgumentResolver.java

+    private MemberService memberService;
+    private LoginService loginService;


해당 부분에 final 키워드를 붙여주는 것이 좋을 것 같습니다

loginMemberArgumentResolver에서 memberService 대신에 memberDao를 의존하는 방향도 있을 것 같은데 memberService을 의존하신 이유는 무엇인가요?

Dao를 서비스에서만 사용하도록 만들고 싶었습니다! 지금 당장은 service에서 추가로 하는게 많지 않지만, 추후에 요건이 추가 됐을 때 그러한 부분을 서비스에 맡기고 싶어서요!

seokhwan-an · 2025-04-24T02:35:02Z

src/main/java/roomescape/login/LoginMemberArgumentResolver.java

+        Long memberId = loginService.getMemberId(request.getCookies());
+
+        MemberResponse memberResponse = memberService.findById(memberId);


이 과정이 interceptor에서 이루어지고 동일하게 argumentResolver에서 이루어지네요.
만약 interceptor에서는 토큰이 만료되지 않아서 통과했는데 해당 부분에서 토큰이 만료되는 문제가 발생할 수 있을 것 같은데 그러면 어떻게 해결해야할까요?

방치하는 것도 하나의 방안이 될 수 있습니다. (뒤에 작업에 큰 문제가 있는 것도 아니고 사용자 입장에서도 로그인이 만료되었구나 정도로 파악될 것 같긴합니다)

근데 이걸 .. 좀 고민을 해봤는데 잘 모르겠습니다 :( . .... 어찌됐든 인가 과정에서 member 역할 검증이 필요하니 findById 메소드를 사용하게 되었고 컨트롤러에 전달할 객체를 바인딩해야하니 findById 메소드를 또 사용하게 된거긴한데

음
이 중복 호출을 막기 위해서 사용할 수 있는 구조가 또 있을까요??

뭔가 고민을 떠넘기는 것 같아보여서 좀 민망하긴한데 진짜 생각이 안나서요 ;__;

저는 interceptor를 통해서 검증이 이루어진 객체 정보를 dto에 저장하고 이를 argumentResolver에서 이용하는 방안을 적용해보면서 불필요하게 토큰을 두번 파싱하는 과정을 줄여본 경험이 있습니다.

이 때 dto를 컴포넌트로 등록을 하게 될 텐데 이를 어떻게 관리하면 좋을지 고민해보면 좋을 것 같습니다. spring의 기본 bean의 타입은 singleton인데 어떤 문제가 발생할 수 있을까요? 그렇다면 어떻게 해결할 수 있을지 고민해보면 좋을 것 같습니다.

seokhwan-an · 2025-04-24T02:35:38Z

src/main/java/roomescape/login/LoginRequest.java

+public class LoginRequest {
+
+    private String email;
+    private String password;
+
+    public String getEmail() {
+        return email;
+    }
+
+    public String getPassword() {
+        return password;
+    }
+


LoginResponse와 다르게 여기는 생성자가 필요 없을까요?

기본생성자로 인해 @RequestBody 할 때 이상 없이 매핑되고 있기 때문에 괜찮을 것 같습니다..!

나중에 역직렬화 (json을 java 객체로 변경하는 작업)이 하는 과정에서 필드가 1개인 경우에는 생성자가 없을 때 jackson에서 예외가 발생할 수 있어서 한번 알아보면 좋을 것 같아서 남긴 리뷰였습니다.

이해를 못했어요!ㅠㅠㅠ
필드가 한 개가 있더라도 기본생성자는 존재하기 때문에 문제 없이 동작하지 않나요??

seokhwan-an · 2025-04-24T02:37:42Z

src/main/java/roomescape/login/LoginService.java

+    private String extractTokenFromCookie(Cookie[] cookies) {
+        for (Cookie cookie : cookies) {
+            if (cookie.getName().equals("token")) {
+                return cookie.getValue();
+            }
+        }
+        return null;
+    }


쿠키들 중에서 인증 쿠키를 찾는 책임이 loginService에 필요한 책임인지 고려해보면 좋을 것 같습니다.
처음부터 해당 쿠키 정보만 전달할 수는 없을까요?

고민을 해봤는데 memberName을 가져오기위한 getMemberName 메소드에서 굳이 "token" 값이 아닌 다른 쿠키 정보를 알아야할 필요가 없을 것 같긴하네용
extractTokenFromCookie 메소드가 private이라 loginService 내에 있는 메소드들만 사용할 수 있다는 것도 좀 이상하긴하구요 . .

컨트롤러에서부터 쿠키를 적절히 처리해서 서비스 호출하는 것이 더 나은 흐름인 것 같아보입니다

맞춰서 수정해놓을게요!

이 부분은 아래 리뷰에 남겼는데 천천히 확인 부탁드립니다!

seokhwan-an · 2025-04-24T02:38:23Z

src/main/java/roomescape/member/MemberDao.java

+    public Member findById(Long id) {
+        return jdbcTemplate.queryForObject(
+            "SELECT id, name, email, role FROM member WHERE id = ?",
+            (rs, rowNum) -> new Member(
+                rs.getLong("id"),
+                rs.getString("name"),
+                rs.getString("email"),
+                rs.getString("role")
+            ),
+            id
+        );
+    }


만약 여기서 member 존재하지 않을 경우는 어떻게 값이 전달될까요?

아 Exception이 발생할 것 같습니다 ..! query 사용해서 값 없으면 null 반환하도록 수정했습니다!

아래에 추가적인 리뷰를 남겼는데 한번 확인 부탁드립니다!

seokhwan-an

민주님 추가적인 리뷰를 남겼는데 확인해주세요
기존의 리뷰에 답변을 남긴 것은 위에서 확인해주셔야 할 것 같습니다!

반영하지 않으신 리뷰도 있는데 반영하지 않았다면 그 이유를 남겨주시면 좋을 것 같습니다!

seokhwan-an · 2025-04-30T00:27:13Z

src/main/java/roomescape/common/AuthorizationInterceptor.java

+        return loginService.getMemberId(request.getCookies())
+            .map(memberId -> {
+                MemberResponse member = memberService.findById(memberId);
+                if (!"ADMIN".equals(member.getRole())) {
+                    response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+                    return false;
+                }
+                return true;
+            })
+            .orElseGet(() -> {
+                response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+                return false;
+            });


해당 로직은 조금 복잡해지는 것 같은데요
지금은 인증과 인가를 한번에 처리하려고 하는 것으로 인해서 복잡성이 높아졌다고 생각합니다.
이전 처럼 각각 분기 문으로 처리하는 방안이 저는 개인적으로 깔끔하다고 생각합니다.

더불어서 인증과 인가에 대한 상태코드에 대해 찾아보시고 반영하면 좋을 것 같네요.

아 테스트코드 통과 요건이 401이어서요!!
헷갈릴만한 부분이 있는 것 같아 일단 HttpServletResponse에 있는거 안쓰고 401이라고 하드코딩 하도록 만들어두었습니다

seokhwan-an · 2025-04-30T00:33:23Z

src/main/java/roomescape/member/MemberDao.java

+        if (members.isEmpty()) {
+            return null;
+        }


여기도 null 대신에 Optional을 반환해 해당 메소드의 사용자에게 힌트를 남겨보는 것은 어떤가요?

seokhwan-an · 2025-04-30T00:33:42Z

src/main/java/roomescape/member/MemberDao.java

+        if (members.isEmpty()) {
+            return null;
+        }


위와 같은 리뷰입니다.

seokhwan-an · 2025-04-30T00:34:42Z

src/main/java/roomescape/member/MemberService.java

+        if (member == null) {
+            throw new IllegalArgumentException("해당 id를 가진 Member 객체를 찾을 수 없습니다.");
+        }
        return new MemberResponse(member.getId(), member.getName(), member.getEmail(), member.getRole());
    }

    public MemberResponse findByEmailAndPassword(String email, String password) {
        Member member = memberDao.findByEmailAndPassword(email, password);
+        if (member == null) {
+            throw new IllegalArgumentException("해당 email와 password를 가진 Member 객체를 찾을 수 없습니다.");
+        }


위의 리뷰가 반영되면 해당 로직도 변경될 수 있을 것 같네요.

seokhwan-an · 2025-04-30T04:54:58Z

src/main/java/roomescape/login/LoginService.java

        Long memberId = jwtTokenService.getMemberId(token);
        return Optional.ofNullable(memberId);
    }

-    private String extractTokenFromCookie(Cookie[] cookies) {
+    public String extractTokenFromCookie(Cookie[] cookies) {


제가 의도했던 것은 쿠키와 관련된 작업들을 모든 컴포넌트를 만드는 것을 의도했던 것인데 이렇게 해도 괜찮을 것 같습니다.(제가 리뷰 드리고 싶었던 것은 과연 cookie를 service layer에서 다루는 것이 좋을까? 아니면 controller 단에서 처리하는 것이 좋을까 하는 것에 대해 고민했으면 해서 남겼던 리뷰였습니다.)

큰 이유는 없는데 저는 컨트롤러 단에 좀 복잡한 처리를 하는 로직은 안두려고 노력하는 편입니다!!

쿠키안에서 토큰을 뽑아내는 작업 또한 서비스레이어에 두게 되면 다른 컨트롤러에서도 서비스 호출을 통해 해당 로직을 사용할 수 있을거라 생각해서 요런 식으로 관리하게 되었습니다 :)

JoungMinJu · 2025-05-01T13:33:50Z

@seokhwan-an 반영 했는데 확인 부탁드리겠습니다!
그리고 또 반영 안한 리뷰가 있을까요??? 첫 번째 리뷰에서 반영안하고 코멘트도 안남긴 리뷰가 뭔지 모르겠어서요!

seokhwan-an · 2025-05-01T23:49:33Z

@JoungMinJu

이 부분은 반영 혹은 답변이 아직 없습니다.

seokhwan-an

민주님 이번 단계 미션을 구현하시느라 고생하셨습니다.
추가적으로 간단하게 리뷰 남겼는데 다음 단계에서 진행하실 때 반영하시면 좋을 것 같습니다!
#151 (comment)
위에도 확인 부탁드립니다.

seokhwan-an · 2025-05-09T05:49:20Z

src/main/java/roomescape/common/AuthorizationInterceptor.java

+
+        Long memberId = optionalMemberId.get();
+        MemberResponse member = memberService.findById(memberId);
+        if (!"ADMIN".equals(member.getRole())) {


member 객체에게 isAdmin()이라는 메세지를 던져보면 좋을 것 같습니다! (굳이 member의 상태를 불러올 필요가 없다고 생각합니다.)

seokhwan-an · 2025-05-09T06:14:51Z

src/main/java/roomescape/member/MemberDao.java

-            return null;
-        }
-        return members.get(0);
+        return members.stream().findFirst();


이렇게 해도 좋은데 전에 이용했던 jdbcTemplate의 queryForObject를 이용하면 코드를 보다 직관적으로 나타낼 수 있을 것입니다.

JoungMinJu added 11 commits April 8, 2025 20:06

step1 : 쿠키 생성 메소드 구현

6d05291

step1 : 쿠키 생성 메소드 구현

933df33

step1 : 로그인 메소드 구현

4614e22

step1 : 로그인 메소드 구현

ee92c0f

step2 : 테스트 메소드 추가

bd8a7ff

step2 : HandlerMethodArgumentResolver 구현

1384536

step2 : HandlerMethodArgumentResolver 수정 (Member 객체 활용하도록)

e7b5690

step2 : Controller 구현

d8f00b8

step3 : 테스트 추가

5c647fb

step3 : common 패키지 추가

ebed158

step3 : 인터셉터 구현 완료

fecab14

seokhwan-an reviewed Apr 24, 2025

View reviewed changes

JoungMinJu added 5 commits April 24, 2025 22:00

step3 : optional 사용하도록 변경

e2070fe

step3 : member에게 어드민 검증 로직 이관

0d74755

step3 : final 키워드 붙이기

92d1eaa

step3 : queryForObject -> query 사용하도록 변경

7ec60fb

step3 : extractToken을 public 접근제어자로 변경

b070720

seokhwan-an reviewed Apr 30, 2025

View reviewed changes

JoungMinJu added 2 commits May 1, 2025 22:21

step3 : 인증과 인가 코드 분리

87c7f25

step3 : Optional 반환하도록 수정

a05e32f

seokhwan-an approved these changes May 9, 2025

View reviewed changes

boorownie merged commit 8c837ad into next-step:joungminju May 9, 2025

		@Component
		public class AuthorizationInterceptor implements HandlerInterceptor {

		private MemberService memberService;
		private LoginService loginService;

		Long memberId = loginService.getMemberId(request.getCookies());

		MemberResponse memberResponse = memberService.findById(memberId);

[Spring MVC (인증)] 정민주 미션 제출합니다. #151

[Spring MVC (인증)] 정민주 미션 제출합니다. #151

Uh oh!

Conversation

JoungMinJu commented Apr 23, 2025

Uh oh!

seokhwan-an left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoungMinJu May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seokhwan-an left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seokhwan-an Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

JoungMinJu May 1, 2025 •

edited

Loading

seokhwan-an left a comment •

edited

Loading

seokhwan-an Apr 30, 2025 •

edited

Loading

seokhwan-an left a comment •

edited

Loading