Step1 #330

nara1030 · 2022-08-09T05:58:46Z

안녕하세요 리뷰어님
리뷰이 엄홍준이라고 합니다.
조부님 상을 당해 늦게 PR 드립니다.
강의를 몇 번 반복해 들었는데 이해가 잘 가지 않아 뼈대 코드와 다른 분의 코드를 참고하여 작성하였는데 몇 가지 질문 드리고 싶습니다(기록 용도로 남겨 놓는 것도 있으니 혹 답변이 어려우시더라도 괜찮습니다).
감사합니다.

토큰이 유효하지 않은 경우 에러 응답이 오는지 확인하는 것이 이번 요구사항 중 하나이고, AuthAcceptanceTest 클래스의 myInfoWithBearerAuthUsingInvalidToken 메소드를 통해 검증하려고 하였습니다. 이때 Interceptor의 예외 역시 ControllerAdvice로 처리할 수 있다고 생각하여 ControllerExceptionHandler에 다음을 추가했습니다.

@ExceptionHandler(AuthenticationException.class)
public ResponseEntity<Void> handleAuthenticationException(AuthenticationException e) {
	return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
}

그리고 BearerTokenAuthenticationFilter 클래스의 preHandle 메소드를 다음과 같이 작성하였습니다.

String authCredentials = AuthorizationExtractor.extract(request, AuthorizationType.BEARER);
AuthenticationToken token = new AuthenticationToken(authCredentials, authCredentials);

if (!jwtTokenProvider.validateToken(token.getPrincipal())) {
	throw new AuthenticationException();
}

String principal = jwtTokenProvider.getPrincipal(token.getPrincipal());
List<String> roles = jwtTokenProvider.getRoles(token.getPrincipal());

Authentication authentication = new Authentication(principal, roles);

SecurityContextHolder.getContext().setAuthentication(authentication);

return true;

즉, try-catch문 없이 작성하여 AuthenticationException 발생 시 다음과 같이 검증 가능했습니다.

@DisplayName("Bearer Auth With Invalid Token")
@Test
void myInfoWithBearerAuthUsingInvalidToken() {
	String invalidAccessToken = 로그인_되어_있음(EMAIL, PASSWORD) + 1;
	ExtractableResponse<Response> response = 베어러_인증으로_내_회원_정보_조회_요청(invalidAccessToken);
	assertThat(response.statusCode()).isEqualTo(HttpStatus.UNAUTHORIZED.value());
}

그런데 이 경우 myInfoWithBasicAuth 및 myInfoWithSession 테스트가 실패하였기에 BearerTokenAuthenticationFilter의 preHandle 메소드 내부를 try-catch문으로 감싸주었습니다. 헌데, 개념이 부족해서인지 왜 다른 인터셉터의 변경이 다른 인증 테스트에 영향을 미치는지 잘 모르겠습니다. 왜 try-catch로 감싸지 않으면 타 테스트에 영향을 미치는 걸까요..?

AuthConfig 클래스의 addInterceptors 메소드를 보면 복수의 인터셉터들이 순서대로 등록되어 있는데요. 여기서 첫 번째 등록된 SecurityContextPersistenceFilter만 필수고 이후의 것들은 선택사항이 맞나요? 즉 폼 기반 인증은 UsernamePasswordAuthenticationFilter, 베이직 인증은 BasicAuthenticationFilter, 베어러 인증은 BearerTokenAuthenticationFilter... TokenAuthenticationInterceptor는 어떤 인증과 관련이 있는 걸까요..?

인터셉터마다 true만을 리턴(BasicAuthenticationFilter)하기도 하고, true/false를 리턴(UsernamePasswordAuthenticationFilter)하기도, false만을 리턴(TokenAuthenticationInterceptor)하기도 하는데 이해가 잘 가지 않습니다..

testrace

안녕하세요 홍준님 😃
리뷰를 맡게 된 송용주입니다

뼈대 코드를 이해하시기 쉽지 않으셨을 텐데 잘 구현해 주셨네요 👍

2단계로 넘어가기 전에 1단계를 조금 더 다듬고 가셨으면 합니다
질문 주셨던 부분들에 대해 도움이 될만한 코멘트를 남겨두었으니 확인해 주시고 리뷰 요청 부탁드립니다.

궁금한 내용이 있으시면 편하게 DM 주세요!

testrace · 2022-08-09T13:02:27Z

src/main/java/nextstep/auth/authentication/BearerTokenAuthenticationFilter.java

+        try {
+            String authCredentials = AuthorizationExtractor.extract(request, AuthorizationType.BEARER);
+            AuthenticationToken token = new AuthenticationToken(authCredentials, authCredentials);
+
+            if (!jwtTokenProvider.validateToken(token.getPrincipal())) {
+                throw new AuthenticationException();
+            }
+
+            String principal = jwtTokenProvider.getPrincipal(token.getPrincipal());
+            List<String> roles = jwtTokenProvider.getRoles(token.getPrincipal());
+
+            Authentication authentication = new Authentication(principal, roles);
+
+            SecurityContextHolder.getContext().setAuthentication(authentication);
+
+            return true;
+        } catch (Exception e) {
+            return true;
+        }


질문 주셨던 부분이네요
try-catch를 하지 않으면 Bearer 인증 요청이 아닌 모든 경우에 예외가 발생할텐데요,
HandlerInterceptor에서 발생한 예외는 DispatcherServlet에서 예외를 처리하다보니 다른 테스트에도 영향이 생길 수 있습니다
HandlerInterceptor는 리턴 값(true/false)에 따라 다음에 어떤 작업을 수행하는지 찾아보시면 좋을 것 같습니다

testrace · 2022-08-09T13:02:43Z

src/main/java/nextstep/auth/authentication/UsernamePasswordAuthenticationFilter.java

+    public static final String USERNAME_FIELD = "username";
+    public static final String PASSWORD_FIELD = "password";


매직 리터럴 상수화 👍
클래스 내부에서만 사용된다면 접근 제어를 private 으로 변경하면 어떨까요?

testrace · 2022-08-09T13:02:48Z

src/main/java/nextstep/auth/authentication/UsernamePasswordAuthenticationFilter.java

+            String username = paramMap.get(USERNAME_FIELD)[0];
+            String password = paramMap.get(PASSWORD_FIELD)[0];
+
+            AuthenticationToken token = new AuthenticationToken(username, password);


AuthenticationToken을 생성할 필요가 있을까요?

testrace · 2022-08-09T13:03:07Z

src/main/java/nextstep/auth/authentication/UsernamePasswordAuthenticationFilter.java

+            if (loginMember == null) {
+                throw new AuthenticationException();
+            }
+
+            if (!loginMember.checkPassword(token.getCredentials())) {
+                throw new AuthenticationException();
+            }


loginMember 검증 👍

testrace · 2022-08-09T13:05:11Z

src/main/java/nextstep/subway/ui/ControllerExceptionHandler.java

+    @ExceptionHandler(AuthenticationException.class)
+    public ResponseEntity<Void> handleAuthenticationException(AuthenticationException e) {
+        return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
+    }


질문 주셨던 내용에 포함되는 부분이네요
이 메서드를 삭제해도 모든 테스트가 통과를 하고 있어요
ExceptionHandler가 어떤 상황에서 수행되는지 찾아 보시면 좋을 것 같습니다

testrace · 2022-08-09T13:05:16Z

src/main/java/nextstep/subway/ui/LineController.java

@@ -19,6 +20,7 @@ public LineController(LineService lineService) {
        this.lineService = lineService;
    }

+    @Secured("ROLE_ADMIN")


Secured 👍

testrace · 2022-08-09T13:05:26Z

src/test/java/nextstep/subway/acceptance/AcceptanceTest.java

+        dataLoader.loadData();
+
+        관리자 = MemberSteps.로그인_되어_있음(EMAIL, PASSWORD);


dataLoder를 활용해주셨네요 💯

testrace · 2022-08-09T13:05:33Z

src/test/java/nextstep/subway/acceptance/AcceptanceTestSteps.java

+    static RequestSpecification given(String token) {
+        return RestAssured
+                .given().log().all()
+                .auth().oauth2(token);
+    }


메서드명에 인증 관련 내용이 명시되면 가독성에 더 도움이 될 것 같아요 😃

testrace · 2022-08-09T13:07:40Z

src/test/java/nextstep/subway/acceptance/AuthAcceptanceTest.java

+    @DisplayName("Bearer Auth With Invalid Token")
+    @Test
+    void myInfoWithBearerAuthUsingInvalidToken() {
+        String accessToken = 로그인_되어_있음(EMAIL, PASSWORD) + 1;
+
+        ExtractableResponse<Response> response = 베어러_인증으로_내_회원_정보_조회_요청(accessToken);
+        System.out.println("====");
+        System.out.println(response.jsonPath().getString("."));
+//        assertThat(response.statusCode()).isEqualTo(HttpStatus.UNAUTHORIZED.value());
+        assertThat(response.jsonPath().getString("path")).isEqualTo("/members/me");
+    }


질문 주셨던 부분이네요!
베어러_인증으로_내_회원_정보_조회_요청은 인증과 조회 2가지의 요청을 보내고 있습니다
응답결과 response는 인증과 요청 중 어느 응답정보를 가지고 있을까요?
잘못된 토큰임을 검증하는데 2가지의 요청이 필요할까요?

testrace · 2022-08-09T13:09:33Z

src/test/java/nextstep/subway/acceptance/LineAcceptanceTest.java

@@ -24,7 +19,7 @@ class LineAcceptanceTest extends AcceptanceTest {
    @Test
    void createLine() {
        // when
-        ExtractableResponse<Response> response = 지하철_노선_생성_요청("2호선", "green");
+        ExtractableResponse<Response> response = 지하철_노선_생성_요청(관리자, "2호선", "green");


관리 권한이 없는 예외 상황 테스트도 추가할 수 있을 것 같아요 😃

nara1030 added 6 commits August 8, 2022 14:18

feat/test] Basic Auth

4f8da13

feat/test] Form 기반 로그인

891ae8e

feat/test] Bearer Auth

312452c

feat/test] 지하철역 API 권한 적용

5bfed96

feat/test] 지하철 노선/구간 API 권한 적용

957ee75

퇴근...

e6b3b11

testrace requested changes Aug 9, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step1 #330

Step1 #330

nara1030 commented Aug 9, 2022

testrace left a comment

testrace Aug 9, 2022

testrace Aug 9, 2022

testrace Aug 9, 2022

testrace Aug 9, 2022

testrace Aug 9, 2022

testrace Aug 9, 2022

testrace Aug 9, 2022

testrace Aug 9, 2022

testrace Aug 9, 2022

testrace Aug 9, 2022

		public static final String USERNAME_FIELD = "username";
		public static final String PASSWORD_FIELD = "password";

		dataLoader.loadData();

		관리자 = MemberSteps.로그인_되어_있음(EMAIL, PASSWORD);

Step1 #330

Are you sure you want to change the base?

Step1 #330

Conversation

nara1030 commented Aug 9, 2022

testrace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment