Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Step2 리뷰 요청드립니다~ #346

Open
wants to merge 10 commits into
base: sang-eun
Choose a base branch
from
Open

Step2 리뷰 요청드립니다~ #346

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c42932d
[add] form login unit test, acceptance test, 구현
sang-eun Aug 15, 2022
bbd2e96
[add] bearer login acceptance test, 구현
sang-eun Aug 15, 2022
bc8efda
[add] 권한 기능
sang-eun Aug 15, 2022
fde6d3a
[add] 권한 기능 체크 인수테스트
sang-eun Aug 15, 2022
a3ef715
[modify] TokenAuthenticationInterceptor, UsernamePasswordAuthenticati…
sang-eun Aug 15, 2022
c0c4ab3
[modify] BasicAuthenticationFilter, BearerTokenAuthenticationFilter 추…
sang-eun Aug 15, 2022
d7c23e2
[modify] auth에서 LoginMemberService 의존성 제거
sang-eun Aug 15, 2022
54cdba1
[modify] step1 코드리뷰 반영
sang-eun Aug 15, 2022
bcf2327
Merge branch 'sang-eun' of https://github.com/next-step/atdd-subway-f…
sang-eun Aug 15, 2022
533ccdf
[modify] 코드리뷰 반영
sang-eun Aug 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/main/java/nextstep/auth/authentication/Authenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package nextstep.auth.authentication;

import nextstep.member.application.UserDetailsService;
import nextstep.member.domain.LoginMember;
import nextstep.member.domain.User;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

미션은 auth 패키지 내에 라이브러리를 제외한 비즈니스 패키지들이 들어오지 않아야 완료됩니다 😢
현재 auth 패키지가 member 에 의존하고 있어요

import org.springframework.web.servlet.HandlerInterceptor;

import javax.servlet.http.HttpServletRequest;
Expand All @@ -19,20 +19,20 @@ public Authenticator(UserDetailsService userDetailsService) {
@Override
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws IOException {
AuthenticationToken token = convert(request);
LoginMember member = userDetailsService.loadUserByUsername(token.getPrincipal());
User user = userDetailsService.loadUserByUsername(token.getPrincipal());

checkAuthentication(member, token.getCredentials());
authenticate(member, response);
checkAuthentication(user, token.getCredentials());
authenticate(user, response);

return false;
}

abstract public AuthenticationToken convert(HttpServletRequest request) throws IOException;

abstract public void authenticate(LoginMember member, HttpServletResponse response) throws IOException;
abstract public void authenticate(User user, HttpServletResponse response) throws IOException;

private void checkAuthentication(LoginMember member, String password) {
if (!member.checkPassword(password)) {
private void checkAuthentication(User user, String password) {
if (!user.checkPassword(password)) {
throw new AuthenticationException();
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/main/java/nextstep/auth/authentication/BasicAuthenticationFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import nextstep.auth.context.Authentication;
import nextstep.member.application.UserDetailsService;
import nextstep.member.domain.LoginMember;
import nextstep.member.domain.User;
import org.apache.tomcat.util.codec.binary.Base64;

import javax.servlet.http.HttpServletRequest;
Expand All @@ -18,7 +18,7 @@ public BasicAuthenticationFilter(UserDetailsService userDetailsService) {
@Override
public Authentication convert(HttpServletRequest request) {
AuthenticationToken token = getToken(request);
LoginMember loginMember = userDetailsService.loadUserByUsername(token.getPrincipal());
User loginMember = userDetailsService.loadUserByUsername(token.getPrincipal());

checkAuthentication(token, loginMember);

Expand All @@ -37,8 +37,8 @@ private AuthenticationToken getToken(HttpServletRequest request) {
return new AuthenticationToken(principal, credentials);
}

private void checkAuthentication(AuthenticationToken token, LoginMember loginMember) {
if (!loginMember.checkPassword(token.getCredentials())) {
private void checkAuthentication(AuthenticationToken token, User user) {
if (!user.checkPassword(token.getCredentials())) {
throw new AuthenticationException();
}
}
Expand Down
5 changes: 0 additions & 5 deletions src/main/java/nextstep/auth/authentication/BearerTokenAuthenticationFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,9 @@
package nextstep.auth.authentication;

import nextstep.auth.context.Authentication;
import nextstep.auth.context.SecurityContextHolder;
import nextstep.auth.token.JwtTokenProvider;
import nextstep.member.domain.LoginMember;
import org.springframework.web.servlet.HandlerInterceptor;

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.util.List;

public class BearerTokenAuthenticationFilter extends Authorizator {
private final JwtTokenProvider jwtTokenProvider;
Expand Down
7 changes: 5 additions & 2 deletions ...a/nextstep/member/domain/LoginMember.java → ...step/auth/authentication/LoginMember.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
package nextstep.member.domain;
package nextstep.auth.authentication;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoginMember 를 옮기시지 않아요 됩니다 😢



import nextstep.member.domain.Member;
import nextstep.member.domain.User;

import java.util.List;

public class LoginMember {
public class LoginMember implements User {
private String email;
private String password;
private List<String> authorities;
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/nextstep/auth/authentication/UsernamePasswordAuthenticationFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import nextstep.auth.context.Authentication;
import nextstep.auth.context.SecurityContextHolder;
import nextstep.member.application.UserDetailsService;
import nextstep.member.domain.LoginMember;
import nextstep.member.domain.User;

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
Expand All @@ -21,7 +21,7 @@ public AuthenticationToken convert(HttpServletRequest request) {
}

@Override
public void authenticate(LoginMember member, HttpServletResponse response) throws IOException {
public void authenticate(User member, HttpServletResponse response) throws IOException {
Authentication authentication = new Authentication(member.getEmail(), member.getAuthorities());
SecurityContextHolder.getContext().setAuthentication(authentication);
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/nextstep/auth/authorization/AuthenticationPrincipalArgumentResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package nextstep.auth.authorization;

import nextstep.auth.authentication.LoginMember;
import nextstep.auth.context.Authentication;
import nextstep.auth.context.SecurityContextHolder;
import nextstep.member.domain.LoginMember;
import org.springframework.core.MethodParameter;
import org.springframework.web.bind.support.WebDataBinderFactory;
import org.springframework.web.context.request.NativeWebRequest;
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/nextstep/auth/token/TokenAuthenticationInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import nextstep.auth.authentication.AuthenticationToken;
import nextstep.auth.authentication.Authenticator;
import nextstep.member.application.UserDetailsService;
import nextstep.member.domain.LoginMember;
import nextstep.member.domain.User;
import org.springframework.http.MediaType;

import javax.servlet.http.HttpServletRequest;
Expand All @@ -14,6 +14,7 @@

public class TokenAuthenticationInterceptor extends Authenticator {
private final JwtTokenProvider jwtTokenProvider;
private final ObjectMapper objectMapper = new ObjectMapper();

public TokenAuthenticationInterceptor(UserDetailsService userDetailsService, JwtTokenProvider jwtTokenProvider) {
super(userDetailsService);
Expand All @@ -32,11 +33,11 @@ public AuthenticationToken convert(HttpServletRequest request) throws IOExceptio
}

@Override
public void authenticate(LoginMember member, HttpServletResponse response) throws IOException {
public void authenticate(User member, HttpServletResponse response) throws IOException {
String token = jwtTokenProvider.createToken(member.getEmail(), member.getAuthorities());
TokenResponse tokenResponse = new TokenResponse(token);

String responseToClient = new ObjectMapper().writeValueAsString(tokenResponse);
String responseToClient = objectMapper.writeValueAsString(tokenResponse);
response.setStatus(HttpServletResponse.SC_OK);
response.setContentType(MediaType.APPLICATION_JSON_VALUE);
response.getOutputStream().print(responseToClient);
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/nextstep/member/application/LoginMemberService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package nextstep.member.application;

import nextstep.member.domain.LoginMember;
import nextstep.auth.authentication.LoginMember;
import nextstep.member.domain.Member;
import nextstep.member.domain.MemberRepository;
import org.springframework.stereotype.Service;
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/nextstep/member/application/UserDetailsService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package nextstep.member.application;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 인터페이스가 member 패키지에 있는게 맞을까요? 🤔

Copy link
Author

@sang-eun sang-eun Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

만들면서 배우는 클린아키텍쳐를 보면 서비스의 인터페이스와 그 구현체를 같이 두기에 그게 익숙해서 member에 두었네요 ㅎㅎ
UserDetailService를 auth에서만 사용하지 않을 수도 있기에 구현체와 같이 두는게 한눈에 더 들어올 것 같습니다!

물론 인터페이스와 구현체의 구분은 폴더로라도 하면 좋은데 급하게 하느라... 🙏

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 책에서의 구현의도는 단순히 인터페이스와 구현체를 같은 패키지에 두는 것은 아닙니다 😅
클린 아키텍처에서 로버트 C.마틴은 아래와 같은 원을 그려서 설명하는데요.
조금 단순화해서 말씀드리면 원 외부의 객체만이 원 내부의 객체에 의존해야 한다는 것을 주장합니다.
말씀하신 만들면서 배우는 클린아키텍처는 DIP를 통해 의존성 격리하기 위해 인터페이스를 두고, 해당 인터페이스를 내부의 객체가 구현하고, 외부의 객체는 그 인터페이스를 사용함으로써 외부의 객체가 내부의 실제 구현객체를 알지 못하게 되는 역할을 합니다.
지금 미션에서 요구하는 것도 사실은 DIP 를 통한 의존성 격리입니다 😄

참고 영상을 통해 공부해보시면 조금 더 이해가 쉬우실 것 같아요 🔥

1_0u-ekVHFu7Om7Z-VTwFHvg

Copy link
Author

@sang-eun sang-eun Aug 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 말씀하신대로 원 외부의 객체만이 원 내부의 객체를 참조해야 한다는 것은 동의합니다.

그런데 현재 구조로는 의존성 격리가 충분히 되지 않았다는 말씀일까요? 현재도 member 과 auth간에 usecase를 통해 통신이 이루어지고 있기에, dip를 통해 구현체는 직접적으로 참조가 이뤄지지 않고 있어서 usecase를 어디에 둘 것인가는 부차적인 문제라고 생각했습니다. (그래서 그중 하나로 만들면서 배우는 클린아키텍쳐 내용을 예시로 든 것이고요. 첨부해주신 영상에서도 import문이 존재하기만 해도 의존관계가 있다고 하네요!)
미션이 그런 의미라면 수정하는건 문제없습니다만 궁금해서 질문드립니다 ㅎㅎ

Copy link

@pch8388 pch8388 Aug 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

클린아키텍처를 언급한건 상은님이 얘기하신 부분에 대해 추가적인 언급을 드리다보니 그렇게 되었네요 😅
클린아키텍처의 내용이 단순히 인터페이스와 구현체를 같은 패키지에 두라는 의미는 아니라고 생각해서 첨언을 드려봤습니다

현재 미션에 대해 이야기하면 auth 와 member 의 성질에 대해 고민해봐야하는 데요.
auth 는 인증/인가에 대한 책임을 가지는 비즈니스 규칙과는 조금 동떨어진 공통의 성질을 지닌 패키지라고 생각됩니다
반면에 member는 이 애플리케이션만의 비즈니스 규칙을 담을수도 있는 사용자에 대한 패키지라고 볼 수 있을 것 같아요
이에 따라 auth -> member 로의 의존성이 생기지 않도록 관리하는 것이 대부분의 상황에서 유리할 거라고 생각합니다.
또한 현재는 MemberController 가 @AuthenticationPrincipal를 사용하고, LoginMember가 User를 구현하기 때문에 패키지 간의 순환참조가 생겼습니다.
이를 끊어내기 위해 DIP를 사용하라고 말씀 드린 것이었습니다. 😅

정리하자면, 미션의 의도는 DIP를 통해 (다른 방법이 있으면 쓰셔도 됩니다) 패키지간 순환참조를 끊어내자 입니다.
영상에서 보신 것과 같이 auth 나 member 둘 중의 한곳에서는 다른 패키지의 import 가 없도록 수정해보시길 바랍니다.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예시를 들어주신 만들면서 배우는 클린 아키텍처에서도 application 패키지의 인터페이스를 adapter 에서 구현 하는 형태로 개발을 하고 있습니다. 의존성 격리를 위해 DIP를 사용하면 개념상으로는 이것 저것 이야기 할 수 있겠지만, 결국엔 내부의 객체들이 외부에 의존하지 않도록 만드는 여러가지 방법들이라고 생각합니다 😄

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MemberController 가 @AuthenticationPrincipal를 사용하고, LoginMember가 User를 구현하기 때문에 패키지 간의 순환참조가 생겼습니다.

여기에 참조가 있다는건 생각못했네요! 넵 수정하겠습니다

예시를 들어주신 만들면서 배우는 클린 아키텍처에서도 application 패키지의 인터페이스를 adapter 에서 구현 하는 형태로 개발을 하고 있습니다

넵 처음 댓글에서 말씀드렸듯이 물론 인터페이스와 구현체의 구분은 폴더로라도 하면 좋은데 급하게 하느라... 🙏 😂 앞으로 참고하겠습니다. 지금까지 긴 설명 감사드립니다!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 토론하며 재미있었습니다
참고 의 예제의 의존관계를 직접 그림으로 그려보시면 더 좋은 인사이트를 얻으 실 수 있을것 같아요 👍


import nextstep.member.domain.LoginMember;
import nextstep.member.domain.User;

public interface UserDetailsService {

LoginMember loadUserByUsername(String email);
User loadUserByUsername(String email);
}
11 changes: 11 additions & 0 deletions src/main/java/nextstep/member/domain/User.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package nextstep.member.domain;


import java.util.List;

public interface User {
String getEmail();
List<String> getAuthorities();

boolean checkPassword(String password);
}
2 changes: 1 addition & 1 deletion src/main/java/nextstep/member/ui/MemberController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
package nextstep.member.ui;

import nextstep.auth.authentication.LoginMember;
import nextstep.auth.authorization.AuthenticationPrincipal;
import nextstep.auth.secured.Secured;
import nextstep.member.application.MemberService;
import nextstep.member.application.dto.MemberRequest;
import nextstep.member.application.dto.MemberResponse;
import nextstep.member.domain.LoginMember;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.*;

Expand Down
2 changes: 1 addition & 1 deletion src/test/java/nextstep/subway/unit/BasicAuthenticationFilterMockTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package nextstep.subway.unit;

import nextstep.auth.authentication.BasicAuthenticationFilter;
import nextstep.auth.authentication.LoginMember;
import nextstep.auth.context.Authentication;
import nextstep.member.application.UserDetailsService;
import nextstep.member.domain.LoginMember;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
Expand Down
9 changes: 6 additions & 3 deletions src/test/java/nextstep/subway/unit/TokenAuthenticationInterceptorMockTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,18 @@

import com.fasterxml.jackson.databind.ObjectMapper;
import nextstep.auth.authentication.AuthenticationToken;
import nextstep.auth.authentication.LoginMember;
import nextstep.auth.token.JwtTokenProvider;
import nextstep.auth.token.TokenAuthenticationInterceptor;
import nextstep.auth.token.TokenRequest;
import nextstep.auth.token.TokenResponse;
import nextstep.member.application.UserDetailsService;
import nextstep.member.domain.LoginMember;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.http.HttpStatus;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.mock.web.MockHttpServletResponse;

Expand Down Expand Up @@ -64,10 +65,12 @@ void authenticate() throws IOException {
void preHandle() throws IOException {
MockHttpServletResponse response = new MockHttpServletResponse();
given(userDetailsService.loadUserByUsername(any())).willReturn(new LoginMember(EMAIL, PASSWORD, List.of()));

boolean result = interceptor.preHandle(createMockRequest(), response, null);
given(jwtTokenProvider.createToken(any(), any())).willReturn(JWT_TOKEN);
boolean result = interceptor.preHandle(createMockRequest(), response, new Object());

assertThat(result).isFalse();
assertThat(response.getStatus()).isEqualTo(HttpStatus.OK.value());
assertThat(response.getContentAsString()).isEqualTo(mapper.writeValueAsString(new TokenResponse(JWT_TOKEN)));
}

private MockHttpServletRequest createMockRequest() throws IOException {
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/nextstep/subway/unit/UsernamePasswordAuthenticationFilterMockTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
package nextstep.subway.unit;

import nextstep.auth.authentication.AuthenticationToken;
import nextstep.auth.authentication.LoginMember;
import nextstep.auth.authentication.UsernamePasswordAuthenticationFilter;
import nextstep.auth.context.Authentication;
import nextstep.auth.context.SecurityContextHolder;
import nextstep.member.application.UserDetailsService;
import nextstep.member.domain.LoginMember;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
Expand Down