-
Notifications
You must be signed in to change notification settings - Fork 1
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
코드 리뷰~ #4
base: main
Are you sure you want to change the base?
코드 리뷰~ #4
Changes from 1 commit
ba7c924
5e95967
2ff3936
24cbdf4
a3e383e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
package kr.where.backend.admin.dto; | ||
|
||
import io.swagger.v3.oas.annotations.media.Schema; | ||
import lombok.Getter; | ||
import lombok.Setter; | ||
@Getter | ||
@Setter | ||
@Schema(description = "사용자 정보") | ||
public class AdminInfo { | ||
|
||
@Schema(description = "이름", example = "홍길동") | ||
private String name; | ||
|
||
@Schema(description = "비밀번호", example = "****") | ||
private String passwd; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package kr.where.backend.admin.dto; | ||
|
||
import io.swagger.v3.oas.annotations.media.Schema; | ||
import lombok.Getter; | ||
import lombok.Setter; | ||
|
||
@Getter | ||
@Setter | ||
@Schema(description = "JSON 형식의 key:value") | ||
public class KeyValueInfo { | ||
|
||
@Schema(description = "{'key':'value'}", example = "value") | ||
private String key; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
package kr.where.backend.auth.authUser; | ||
|
||
import kr.where.backend.auth.authUser.exception.AuthUserException; | ||
import lombok.AccessLevel; | ||
import lombok.Builder; | ||
import lombok.NoArgsConstructor; | ||
import lombok.Setter; | ||
import org.springframework.security.core.context.SecurityContextHolder; | ||
|
||
@Setter | ||
@NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
public class AuthUser { | ||
private Integer intraId; | ||
private String intraName; | ||
private Long defaultGroupId; | ||
|
||
@Builder | ||
public AuthUser(final Integer intraId, final String intraName, final Long defaultGroupId) { | ||
this.intraId = intraId; | ||
this.intraName = intraName; | ||
this.defaultGroupId = defaultGroupId; | ||
} | ||
|
||
public Integer getIntraId() { | ||
return intraId; | ||
} | ||
|
||
public String getIntraName() { | ||
return intraName; | ||
} | ||
|
||
public Long getDefaultGroupId() { | ||
if (defaultGroupId != null) { | ||
return defaultGroupId; | ||
} | ||
throw new AuthUserException.ForbiddenUserException(); | ||
} | ||
|
||
public void setDefaultGroupId(final Long groupId) { | ||
this.defaultGroupId = groupId; | ||
} | ||
public static AuthUser of() { | ||
final Object principle = SecurityContextHolder.getContext().getAuthentication().getPrincipal(); | ||
if (principle.equals("anonymousUser")) { | ||
throw new AuthUserException.AnonymousUserException(); | ||
} | ||
return (AuthUser) principle; | ||
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. 캐스팅 할 때 중간에 공백은 가독성을 위한걸까요?? 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. 저희 v2의 컨벤션이에요 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. 디코를 뒤져봤지만 잘 찾지 못했는데 혹시 컨벤션이랑 설계 전반 정리 돼있는 공간이 있을까요?? 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. 음 저희는 만나서 코드 작성해서 정해진건 없지만 네이버를 기준으로 합니다 |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
package kr.where.backend.auth.authUser; | ||
|
||
import java.lang.annotation.ElementType; | ||
import java.lang.annotation.Retention; | ||
import java.lang.annotation.RetentionPolicy; | ||
import java.lang.annotation.Target; | ||
import org.springframework.security.core.annotation.AuthenticationPrincipal; | ||
|
||
@Retention(RetentionPolicy.RUNTIME) | ||
@Target(ElementType.PARAMETER) | ||
@AuthenticationPrincipal | ||
public @interface AuthUserInfo { | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
package kr.where.backend.auth.authUser.exception; | ||
|
||
import kr.where.backend.exception.ErrorCode; | ||
import lombok.AllArgsConstructor; | ||
import lombok.Getter; | ||
|
||
@Getter | ||
@AllArgsConstructor | ||
public enum AuthUserErrorCode implements ErrorCode { | ||
FORBIDDEN_USER(1800, "Where42 서비스에 동의한 user가 아닙니다."), | ||
ANONYMOUS_USER(1801, "인가 인증을 받지 않은 user 입니다"); | ||
|
||
private final int errorCode; | ||
private final String errorMessage; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
package kr.where.backend.auth.authUser.exception; | ||
|
||
import kr.where.backend.exception.CustomException; | ||
|
||
public class AuthUserException extends CustomException { | ||
public AuthUserException(final AuthUserErrorCode authUserErrorCode) { | ||
super(authUserErrorCode); | ||
} | ||
|
||
public static class ForbiddenUserException extends AuthUserException { | ||
public ForbiddenUserException() { | ||
super(AuthUserErrorCode.FORBIDDEN_USER); | ||
} | ||
} | ||
|
||
public static class AnonymousUserException extends AuthUserException { | ||
public AnonymousUserException() { | ||
super(AuthUserErrorCode.ANONYMOUS_USER); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
package kr.where.backend.auth.filter; | ||
|
||
public enum JwtConstants { | ||
HEADER_TYPE("Bearer"), | ||
ACCESS("accessToken"), | ||
REFRESH("refreshToken"), | ||
USER_SUBJECTS("User"), | ||
USER_ID("intraId"), | ||
USER_NAME("intraName"), | ||
USER_ROLE("Cadet"), | ||
TOKEN_TYPE("type"), | ||
ROLE_LEVEL("roles"), | ||
REISSUE_URI("/v3/jwt/reissue"); | ||
|
||
private final String value; | ||
|
||
JwtConstants(final String value) { | ||
this.value = value; | ||
} | ||
|
||
public String getValue() { | ||
return this.value; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
package kr.where.backend.auth.filter; | ||
|
||
import static com.fasterxml.jackson.core.JsonEncoding.UTF8; | ||
|
||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
import jakarta.servlet.FilterChain; | ||
import jakarta.servlet.ServletException; | ||
import jakarta.servlet.http.HttpServletRequest; | ||
import jakarta.servlet.http.HttpServletResponse; | ||
import java.io.IOException; | ||
import kr.where.backend.jwt.exception.JwtException; | ||
import kr.where.backend.member.exception.MemberException; | ||
import lombok.RequiredArgsConstructor; | ||
import org.springframework.http.MediaType; | ||
import org.springframework.stereotype.Component; | ||
import org.springframework.web.filter.OncePerRequestFilter; | ||
|
||
@Component | ||
@RequiredArgsConstructor | ||
public class JwtExceptionFilter extends OncePerRequestFilter { | ||
private final ObjectMapper objectMapper; | ||
|
||
@Override | ||
protected void doFilterInternal( | ||
final HttpServletRequest request, | ||
final HttpServletResponse response, | ||
final FilterChain filterChain | ||
) throws ServletException, IOException { | ||
try { | ||
filterChain.doFilter(request, response); | ||
} catch (final JwtException e) { | ||
sendRequest(response, HttpServletResponse.SC_UNAUTHORIZED, e.toString()); | ||
} catch (final MemberException e) { | ||
sendRequest(response, HttpServletResponse.SC_NOT_FOUND, e.toString()); | ||
} | ||
} | ||
|
||
private void sendRequest( | ||
final HttpServletResponse response, | ||
final int errorCode, | ||
final String error | ||
) throws IOException { | ||
response.setStatus(errorCode); | ||
response.setCharacterEncoding(UTF8.getJavaName()); | ||
response.setContentType(MediaType.APPLICATION_JSON_VALUE); | ||
|
||
response.getWriter().write(objectMapper.writeValueAsString(error)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
package kr.where.backend.auth.filter; | ||
|
||
import jakarta.servlet.FilterChain; | ||
import jakarta.servlet.ServletException; | ||
import jakarta.servlet.http.HttpServletRequest; | ||
import jakarta.servlet.http.HttpServletResponse; | ||
import kr.where.backend.jwt.JwtService; | ||
import lombok.RequiredArgsConstructor; | ||
import lombok.extern.slf4j.Slf4j; | ||
import org.springframework.security.core.Authentication; | ||
import org.springframework.security.core.context.SecurityContextHolder; | ||
import org.springframework.stereotype.Component; | ||
import org.springframework.web.filter.OncePerRequestFilter; | ||
|
||
import java.io.IOException; | ||
|
||
@Component | ||
@RequiredArgsConstructor | ||
@Slf4j | ||
public class JwtFilter extends OncePerRequestFilter { | ||
private final JwtService jwtService; | ||
|
||
@Override | ||
protected void doFilterInternal( | ||
final HttpServletRequest request, | ||
final HttpServletResponse response, | ||
final FilterChain filterChain) | ||
throws ServletException, IOException { | ||
|
||
final String token = jwtService.extractToken(request).orElse(null); | ||
if (token != null) { | ||
final Authentication auth = jwtService.getAuthentication(request, token); | ||
SecurityContextHolder.getContext().setAuthentication(auth); | ||
} | ||
filterChain.doFilter(request, response); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
package kr.where.backend.auth.filter.exception; | ||
|
||
import static com.fasterxml.jackson.core.JsonEncoding.UTF8; | ||
|
||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
import jakarta.servlet.http.HttpServletRequest; | ||
import jakarta.servlet.http.HttpServletResponse; | ||
import lombok.RequiredArgsConstructor; | ||
import org.springframework.http.HttpStatus; | ||
import org.springframework.http.MediaType; | ||
import org.springframework.security.access.AccessDeniedException; | ||
import org.springframework.security.web.access.AccessDeniedHandler; | ||
import org.springframework.stereotype.Component; | ||
|
||
import java.io.IOException; | ||
|
||
@Component | ||
@RequiredArgsConstructor | ||
public class CustomAccessDeniedHandler implements AccessDeniedHandler { | ||
private final ObjectMapper objectMapper; | ||
|
||
@Override | ||
public void handle(final HttpServletRequest request, | ||
final HttpServletResponse response, | ||
final AccessDeniedException accessDeniedException) throws IOException { | ||
//필요한 권한이 없이 접근하려 할때 403 | ||
sendErrorResponse(response); | ||
// response.sendError(HttpServletResponse.SC_FORBIDDEN); | ||
} | ||
|
||
private void sendErrorResponse(final HttpServletResponse response) throws IOException{ | ||
response.setStatus(HttpServletResponse.SC_FORBIDDEN); | ||
response.setCharacterEncoding(UTF8.getJavaName()); | ||
response.setContentType(MediaType.APPLICATION_JSON_VALUE); | ||
|
||
response.getWriter().write(objectMapper.writeValueAsString("접근 권한을 확인하세요.")); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
package kr.where.backend.auth.filter.exception; | ||
|
||
import static com.fasterxml.jackson.core.JsonEncoding.UTF8; | ||
|
||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
import jakarta.servlet.ServletException; | ||
import jakarta.servlet.http.HttpServletRequest; | ||
import jakarta.servlet.http.HttpServletResponse; | ||
import java.io.IOException; | ||
import kr.where.backend.exception.CustomException; | ||
import lombok.extern.slf4j.Slf4j; | ||
import org.springframework.beans.factory.annotation.Qualifier; | ||
import org.springframework.http.HttpStatus; | ||
import org.springframework.http.MediaType; | ||
import org.springframework.security.core.AuthenticationException; | ||
import org.springframework.security.web.AuthenticationEntryPoint; | ||
import org.springframework.stereotype.Component; | ||
import org.springframework.web.servlet.HandlerExceptionResolver; | ||
|
||
@Slf4j | ||
@Component | ||
public class JwtAuthenticationEntryPoint implements AuthenticationEntryPoint { | ||
private final HandlerExceptionResolver exceptionResolver; | ||
|
||
public JwtAuthenticationEntryPoint(@Qualifier("handlerExceptionResolver") final HandlerExceptionResolver exceptionResolver) { | ||
this.exceptionResolver = exceptionResolver; | ||
} | ||
@Override | ||
public void commence(final HttpServletRequest request, final HttpServletResponse response, | ||
final AuthenticationException authException) throws IOException, ServletException { | ||
String error = (String) request.getAttribute("exception"); | ||
response.setContentType("application/json;charset=UTF-8"); | ||
response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); | ||
|
||
response.setCharacterEncoding(UTF8.getJavaName()); | ||
response.setContentType(MediaType.APPLICATION_JSON_VALUE); | ||
ObjectMapper objectMapper = new ObjectMapper(); | ||
response.getWriter().write(objectMapper.writeValueAsString(error)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
package kr.where.backend.auth.oauth2login; | ||
|
||
import lombok.RequiredArgsConstructor; | ||
import lombok.extern.slf4j.Slf4j; | ||
import org.springframework.security.core.authority.SimpleGrantedAuthority; | ||
import org.springframework.security.oauth2.client.userinfo.DefaultOAuth2UserService; | ||
import org.springframework.security.oauth2.client.userinfo.OAuth2UserRequest; | ||
import org.springframework.security.oauth2.client.userinfo.OAuth2UserService; | ||
import org.springframework.security.oauth2.core.OAuth2AuthenticationException; | ||
import org.springframework.security.oauth2.core.user.OAuth2User; | ||
import org.springframework.stereotype.Service; | ||
|
||
import java.util.Collections; | ||
import java.util.Map; | ||
|
||
@RequiredArgsConstructor | ||
@Service | ||
@Slf4j | ||
public class CustomOauth2UserService implements OAuth2UserService<OAuth2UserRequest, OAuth2User> { | ||
|
||
@Override | ||
public UserProfile loadUser(final OAuth2UserRequest userRequest) throws OAuth2AuthenticationException { | ||
final OAuth2UserService<OAuth2UserRequest, OAuth2User> oAuth2UserService = new DefaultOAuth2UserService(); | ||
final OAuth2User oAuth2User = oAuth2UserService.loadUser(userRequest); | ||
|
||
final Map<String, Object> attributes = oAuth2User.getAttributes(); | ||
final String registrationId = userRequest.getClientRegistration().getRegistrationId(); | ||
|
||
return new UserProfile( | ||
registrationId, | ||
Collections.singleton(new SimpleGrantedAuthority("ROLE_USER")), | ||
attributes | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
package kr.where.backend.auth.oauth2login; | ||
|
||
import jakarta.servlet.ServletException; | ||
import jakarta.servlet.http.HttpServletRequest; | ||
import jakarta.servlet.http.HttpServletResponse; | ||
import org.springframework.security.core.AuthenticationException; | ||
import org.springframework.security.web.authentication.SimpleUrlAuthenticationFailureHandler; | ||
import org.springframework.stereotype.Component; | ||
import org.springframework.web.util.UriComponentsBuilder; | ||
|
||
import java.io.IOException; | ||
import java.rmi.RemoteException; | ||
|
||
@Component | ||
public class OAuth2FailureHandler extends SimpleUrlAuthenticationFailureHandler { | ||
|
||
@Override | ||
public void onAuthenticationFailure( | ||
HttpServletRequest request, HttpServletResponse response, AuthenticationException exception) | ||
throws IOException, ServletException { | ||
response.sendRedirect("https://where42.kr/login-fail"); | ||
} | ||
} |
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.
defaultGroupId가 널일 경우가 언제인가요?? where42 서비스 동의를 안 한 경우인가요??
그리고 유효할 경우 return 정상, 아닌경우 자연스럽게 예외를 throw하는 방법이 너무 코드가 깔끔하고 좋은 방법이라 생각하는데 이런 플로우를 짜게 되면 일관성 있게 다른 코드들도 이렇게 구현하는게 나을까요??
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.
예를 들어 if 유효하지 않은 경우 {throw}랑 위와 같은 코드랑 고민이 될 때가 있어서 남깁니다.
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.
defaultGroup id가 없는경우는 where42 동의를 하지 않은 경우 입니다
-> 비동의 맴버로 남는다면, defaultId 없이 Member 엔티티만 생성 됩니다
음 마지막 질문이 이해가 잘 안되는데 혹시 어떤 부분일까요?
저희 컨벤션은 if (유효한 경우, 유효한 값을 리턴) 그 외 throw 하는 방식을 선택하고 있습니다.
그리고 만약 자료구조를 순회하거나, Optional<>을 사용할때는 람다식으로 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.
"저희 컨벤션은 if (유효한 경우, 유효한 값을 리턴) 그 외 throw 하는 방식을 선택하고 있습니다." 로 이해 했습니다 ! 딱 제가 원하던 답변을 주셨어요. 감사합니다.
Optional<> 을 좀 많이 사용해보고 람다식도 얼른 연습해보겠습니다.