-
Notifications
You must be signed in to change notification settings - Fork 97
[그리디] 강동현 Spring JPA (1차) 4, 5, 6 단계 미션 제출합니다. #201
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.
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
base: main
Are you sure you want to change the base?
Changes from 41 commits
3d3151e
0c3e685
8b26e96
378e7c5
cfd15f6
b456b89
97cf233
451beff
f1e3288
0d5c991
f6a7db0
20d7526
1357d07
12171bb
1eb6513
a108bc6
069e91f
f7a7485
b4869c1
97e0106
4b90a36
9636a93
06b14d0
12f68bb
ee61649
589badc
d70f3c3
ee530e2
7554215
8d6b22b
3a20e9f
aacf60c
b935aca
43de312
744396a
be881da
ce83b57
c61db0a
d7e38ec
1c30a05
a44bd1f
739e9db
c05f17e
5b99e63
68ae9f5
c8ef4c0
26a581a
0149b40
94fd21f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,3 +35,6 @@ out/ | |
|
|
||
| ### VS Code ### | ||
| .vscode/ | ||
|
|
||
|
|
||
| application-local.properties | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| package roomescape; | ||
|
|
||
| import org.springframework.beans.factory.annotation.Value; | ||
| import org.springframework.context.annotation.Configuration; | ||
| import org.springframework.web.method.support.HandlerMethodArgumentResolver; | ||
| import org.springframework.web.servlet.config.annotation.InterceptorRegistry; | ||
| import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; | ||
| import roomescape.auth.AdminAuthInterceptor; | ||
| import roomescape.auth.LoginMemberArgumentResolver; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| @Configuration | ||
| public class WebConfig implements WebMvcConfigurer { | ||
| @Value("${roomescape.auth.jwt.secret}") | ||
| private String secretKey; | ||
|
|
||
| @Override | ||
| public void addArgumentResolvers(List<HandlerMethodArgumentResolver> resolvers) { | ||
| resolvers.add(new LoginMemberArgumentResolver(secretKey)); | ||
| } | ||
|
|
||
| @Override | ||
| public void addInterceptors(InterceptorRegistry registry) { | ||
| registry.addInterceptor(new AdminAuthInterceptor(secretKey)) | ||
| .addPathPatterns("/admin", "/admin/**"); | ||
| } | ||
| } | ||
|
|
||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| package roomescape.auth; | ||
|
|
||
| import io.jsonwebtoken.Claims; | ||
| import jakarta.servlet.http.Cookie; | ||
| import jakarta.servlet.http.HttpServletRequest; | ||
| import jakarta.servlet.http.HttpServletResponse; | ||
| import org.springframework.web.servlet.HandlerInterceptor; | ||
| import roomescape.util.JwtUtil; | ||
|
|
||
| public class AdminAuthInterceptor implements HandlerInterceptor { | ||
|
|
||
| private final String secretKey; | ||
|
|
||
| public AdminAuthInterceptor(String secretKey) { | ||
| this.secretKey = secretKey; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) { | ||
| String token = JwtUtil.extractTokenFromCookies(request.getCookies()); | ||
| if (token.isEmpty()) { | ||
| response.setStatus(401); | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| Claims claims = JwtUtil.parseClaims(token, secretKey); | ||
| String role = claims.get("role", String.class); | ||
| if (!"ADMIN".equals(role)) { | ||
| response.setStatus(401); | ||
| return false; | ||
| } | ||
| return true; | ||
| } catch (Exception e) { | ||
| response.setStatus(401); | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+20
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 오류를 잘 걸러주셨어요 :) 현재 토큰이 없는 경우와 권한이 없는 경우의 모든 실패 케이스에서 401을 반환하고 있는 데 상황에 따라 상태코드를 분리해보는 건 어떨까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 또 PageController를 보아하니 동현님께서 응집화에 많이 신경쓰고 계신 거 같아 하나 더 알려 드리자면, 그래서 error 관련 내용을 enum으로 통일해서 관리해보는 건 어떨까 조심스럽게 제안드립니다 😊 ex) @Getter
@AllArgsConstructor
public enum FailMessage {
//400
BAD_REQUEST(HttpStatus.BAD_REQUEST, 40000, "잘못된 요청입니다."),
BAD_REQUEST_REQUEST_BODY_VALID(HttpStatus.BAD_REQUEST, 40001, "잘못된 요청본문입니다."),
BAD_REQUEST_MISSING_PARAM(HttpStatus.BAD_REQUEST, 40002, "필수 파라미터가 없습니다."),
BAD_REQUEST_METHOD_ARGUMENT_TYPE(HttpStatus.BAD_REQUEST, 40003, "메서드 인자타입이 잘못되었습니다."),
BAD_REQUEST_NOT_READABLE(HttpStatus.BAD_REQUEST, 40004, "Json 오류 혹은 요청본문 필드 오류 입니다. ");
private final HttpStatus httpStatus;
private final int code;
private final String message;
}
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 확실히 해당 방식이 유지보수성이 좋아보입니다! 이제 정적인 메세지는 enum으로 하드코딩할 생각을 먼저 하는 게 좋을 것 같습니다! |
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| package roomescape.auth; | ||
|
|
||
| import io.jsonwebtoken.Claims; | ||
| import jakarta.servlet.http.Cookie; | ||
| import jakarta.servlet.http.HttpServletRequest; | ||
| import org.springframework.core.MethodParameter; | ||
| import org.springframework.web.bind.support.WebDataBinderFactory; | ||
| import org.springframework.web.context.request.NativeWebRequest; | ||
| import org.springframework.web.method.support.HandlerMethodArgumentResolver; | ||
| import org.springframework.web.method.support.ModelAndViewContainer; | ||
| import roomescape.member.LoginMember; | ||
| import roomescape.util.JwtUtil; | ||
|
|
||
| public class LoginMemberArgumentResolver implements HandlerMethodArgumentResolver { | ||
|
|
||
| private final String secretKey; | ||
|
|
||
| public LoginMemberArgumentResolver(String secretKey) { | ||
| this.secretKey = secretKey; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean supportsParameter(MethodParameter parameter) { | ||
| return parameter.getParameterType().equals(LoginMember.class); | ||
| } | ||
|
|
||
| @Override | ||
| public Object resolveArgument(MethodParameter parameter, ModelAndViewContainer mavContainer, NativeWebRequest webRequest, WebDataBinderFactory binderFactory) { | ||
| HttpServletRequest request = (HttpServletRequest) webRequest.getNativeRequest(); | ||
| String token = JwtUtil.extractTokenFromCookies(request.getCookies()); | ||
|
|
||
| if (token.isEmpty()) { | ||
| return null; | ||
| } | ||
|
|
||
| Claims claims = JwtUtil.parseClaims(token, secretKey); | ||
|
|
||
| Long id = Long.valueOf(claims.getSubject()); | ||
| String name = claims.get("name", String.class); | ||
| String role = claims.get("role", String.class); | ||
|
|
||
| return new LoginMember(id, name, null, role); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| package roomescape.member; | ||
|
|
||
| public class LoginMember { | ||
| private Long id; | ||
| private String name; | ||
| private String email; | ||
| private String role; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 처음에는 이 객체가 어떤 역할을 하는 지 바로 와닿지 않았는 데, 쓰임을 보아하니 DTO 성격의 객체인 거 같습니다! 보통 DTO의 경우 LoginRequest처럼 클래스명에 Dto를 명시해 주는 경우가 많은데, 지금은 외부 api를 사용하는 프로젝트가 아니라서 필수는 아니지만,
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저도 dto를 붙이는 편이 외부 api를 사용하지 않더라도 엔티티와 구별이 되어 좋다고 생각합니다! |
||
|
|
||
| public LoginMember(Long id, String name, String email, String role) { | ||
| this.id = id; | ||
| this.name = name; | ||
| this.email = email; | ||
| this.role = role; | ||
| } | ||
|
|
||
| public Long getId() { | ||
| return id; | ||
| } | ||
|
|
||
| public String getName() { | ||
| return name; | ||
| } | ||
|
|
||
| public String getEmail() { | ||
| return email; | ||
| } | ||
|
|
||
| public String getRole() { | ||
| return role; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,11 @@ | ||
| package roomescape.member; | ||
|
|
||
| import io.jsonwebtoken.Jwts; | ||
| import io.jsonwebtoken.security.Keys; | ||
| import jakarta.servlet.http.Cookie; | ||
| import jakarta.servlet.http.HttpServletRequest; | ||
| import jakarta.servlet.http.HttpServletResponse; | ||
| import org.springframework.beans.factory.annotation.Value; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.web.bind.annotation.GetMapping; | ||
| import org.springframework.web.bind.annotation.PostMapping; | ||
|
|
@@ -14,6 +17,8 @@ | |
| @RestController | ||
| public class MemberController { | ||
| private MemberService memberService; | ||
| @Value("${roomescape.auth.jwt.secret}") | ||
| private String secretKey; | ||
|
|
||
| public MemberController(MemberService memberService) { | ||
| this.memberService = memberService; | ||
|
|
@@ -25,6 +30,63 @@ public ResponseEntity createMember(@RequestBody MemberRequest memberRequest) { | |
| return ResponseEntity.created(URI.create("/members/" + member.getId())).body(member); | ||
| } | ||
|
|
||
| @PostMapping("/login") | ||
| public ResponseEntity login(@RequestBody MemberRequest memberRequest, HttpServletResponse response) { | ||
| Member member = memberService.login(memberRequest.getEmail(), memberRequest.getPassword()); | ||
|
|
||
| String accessToken = createToken(member); | ||
|
|
||
| Cookie cookie = new Cookie("token", accessToken); | ||
| cookie.setHttpOnly(true); | ||
| cookie.setPath("/"); | ||
| response.addCookie(cookie); | ||
|
|
||
| return ResponseEntity.ok().build(); | ||
| } | ||
|
|
||
| @GetMapping("/login/check") | ||
| public ResponseEntity<MemberResponse> checkLogin(HttpServletRequest request) { | ||
| String token = extractTokenFromCookie(request.getCookies()); | ||
|
|
||
| String name = Jwts.parserBuilder() | ||
| .setSigningKey(Keys.hmacShaKeyFor(secretKey.getBytes())) | ||
| .build() | ||
| .parseClaimsJws(token) | ||
| .getBody() | ||
| .get("name", String.class); | ||
|
|
||
| MemberResponse body = new MemberResponse(null, name, null); | ||
| return ResponseEntity.ok(body); | ||
| } | ||
|
|
||
| public String createToken(Member member) { | ||
| return Jwts.builder() | ||
| .setSubject(member.getId().toString()) | ||
| .claim("name", member.getName()) | ||
| .claim("role", member.getRole()) | ||
| .signWith(Keys.hmacShaKeyFor(secretKey.getBytes())) | ||
| .compact(); | ||
| } | ||
|
|
||
|
|
||
| public String createTokenFromEmailAndPassword(String email, String password) { | ||
| Member member = memberService.login(email, password); | ||
| return createToken(member); | ||
| } | ||
|
|
||
|
|
||
| private String extractTokenFromCookie(Cookie[] cookies) { | ||
| if (cookies == null || cookies.length == 0) { | ||
| return ""; | ||
| } | ||
| for (Cookie cookie : cookies) { | ||
| if ("token".equals(cookie.getName())) { | ||
| return cookie.getValue(); | ||
| } | ||
| } | ||
| return ""; | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (1) 동현님은 컨트롤러의 역할/책임을 어디까지로 보고 계신가요? (2) 지금 컨트롤러에 토큰 생성/파싱/쿠키 추출 로직이 같이 들어가 있는데, 혹시 JwtUtil로 분리하지 않고 컨트롤러에 둔 이유가 있을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (3) 토큰 생성에 토큰 만료시간을 따로 설정 하지 않은 거 같은데요.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (1) 저는 저번 미션의 리뷰 과정에서, 컨트롤러의 역할을 다음과 같이 정의했습니다! next-step/spring-roomescape-playground#533 (comment) -> 저는 컨트롤러의 정의대로, view(http)와 model(domain)을 연결하는 중간지점이라고 생각하여 view에서 RequestDto를 받아 model로 전달하고, 로직이 있다면 양 레이어에서 dto를 받아 필요한 정보만을 사용자에게 제공하는 핸들러 역할만을 수행한다고 생각했습니다. HandlerMapping 만약 코드가 커져 표현에 필요한 DTO를 조립하게 된다면, 컨트롤러 단에서 조립하는 것이 좋은 방법이라고 생각합니다! 그러나 지금 제 코드의 컨트롤러는 비전형적인 요구사항이 추가되지 않는다면 TimeService만 호출하고 TimeResponse 그대로 리턴할 것이라고 생각합니다. 같은 비즈니스 로직을 쓰면서, 서로 다른 DTO로 응답해야 하는 컨트롤러를 지금 단계에서는 예측할 수 없다고 생각하기 때문에, 우선은 지금의 형태 그대로 코드를 유지하고자 합니다! (2) jwtUtil로 메서드를 분리하는 리팩토링을 진행했었는데, 급하게 구현하다보니 생각을 못하고 이전에 하던대로 코딩을 진행했습니다! Jwtutil로 토큰의 생성/파싱/토큰 추출을 관리하도록 리팩토링을 진행했습니다!
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (3) 저번 스터디에서 말씀드린 것처럼 토큰이 한 번 유출되면 만료로 인해 자연스럽게 끊기지 않기 때문에, 공격자가 오랜 기간 해당 사용자 권한으로 접근할 수 있습니다. 토큰을 사용할 때는 만료시간을 정해놓는 건 필수일 것 같습니다! |
||
|
|
||
| @PostMapping("/logout") | ||
| public ResponseEntity logout(HttpServletResponse response) { | ||
| Cookie cookie = new Cookie("token", ""); | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package roomescape.member; | ||
|
|
||
| import org.springframework.data.jpa.repository.JpaRepository; | ||
|
|
||
| import java.util.Optional; | ||
|
|
||
| public interface MemberRepository extends JpaRepository<Member, Long> { | ||
| Optional<Member> findByEmailAndPassword(String email, String password); | ||
| Optional<Member> findByName(String name); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 사용하지 않는 메서드는 지워주는 것이 좋아요!
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이름으로 Member를 조회하는 기능이 필요하다고 착각하여 만들어놨는데 지우는 걸 깜빡했네요..! |
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,14 +4,18 @@ | |
|
|
||
| @Service | ||
| public class MemberService { | ||
| private MemberDao memberDao; | ||
| private MemberRepository memberRepository; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Service 계층은 여러 요청이 동시에 접근할 수 있어서 가변 상태를 줄이는 것도 중요해요.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 좋은 생각입니다! 수정했습니다! |
||
|
|
||
| public MemberService(MemberDao memberDao) { | ||
| this.memberDao = memberDao; | ||
| public MemberService(MemberRepository memberRepository) { | ||
| this.memberRepository = memberRepository; | ||
| } | ||
|
|
||
| public MemberResponse createMember(MemberRequest memberRequest) { | ||
| Member member = memberDao.save(new Member(memberRequest.getName(), memberRequest.getEmail(), memberRequest.getPassword(), "USER")); | ||
| Member member = memberRepository.save(new Member(memberRequest.getName(), memberRequest.getEmail(), memberRequest.getPassword(), "USER")); | ||
| return new MemberResponse(member.getId(), member.getName(), member.getEmail()); | ||
| } | ||
|
|
||
| public Member login(String email, String password) { | ||
| return memberRepository.findByEmailAndPassword(email, password).orElseThrow(); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안 쓰는 import문도 지워주는 것이 좋아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정했습니다!