Skip to content

Feature/jwt practice #2

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Feature/jwt practice #2

wants to merge 13 commits into from

Conversation

chs1234
Copy link
Contributor

@chs1234 chs1234 commented Aug 20, 2023

JWT를 이용해서 구현한 내용입니다.

[Summary]

  • 기존 Entity 구조 변경
  • DTO 클래스 생성
  • Exception Handler 및 각종 설정파일의 변화
  • DB는 MySQL대신 H2 console(메모리디비) 붙여서 테스트 했습니다. (+postman)

* DTO 생성
* Entity 구조 변경
* Exception 핸들러 및 설정파일 변경
* 메모리DB H2 Console 이용
@chs1234 chs1234 requested review from dongKos and wapago August 20, 2023 12:35
Copy link

@dongKos dongKos left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 코멘트 확인 부탁드립니다 :)

.authorizeHttpRequests((requests) -> requests
.requestMatchers(PathRequest.toH2Console()).permitAll()
.requestMatchers(
new AntPathRequestMatcher("/rest/api/v1/auth"),
Copy link

Choose a reason for hiding this comment

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

보통 rest는 붙이지 않고 api/v1이렇게만 합니당

Copy link
Contributor Author

Choose a reason for hiding this comment

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

수정해서 올리겠습니다~!

}

@PostMapping
public ResponseEntity<TokenDto> authenticate(@Valid @RequestBody LoginDto loginDto) {
Copy link

Choose a reason for hiding this comment

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

principal details 관련 객체를 제거하고 로그인 관련 요청을 controller에서 메뉴얼하게 처리하기 위해서 만든걸로 이해하면될까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

맞습니다! 그것을 의도했습니다~!

@Builder
@AllArgsConstructor
@NoArgsConstructor
public class AuthorityDto {
Copy link

Choose a reason for hiding this comment

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

요건 java의 record class를 써도 좋겠네요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Record Class 반영할게요!

private String providerId;

@CreationTimestamp
private LocalDateTime createDate;
Copy link

Choose a reason for hiding this comment

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

요건 왜 빼셨을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

와장창 다 빼버렸네요..ㅎㅎ 다시 반영하겠음!

name = "user_authority",
joinColumns = {@JoinColumn(name = "user_id", referencedColumnName = "user_id")},
inverseJoinColumns = {@JoinColumn(name = "authority_name", referencedColumnName = "authority_name")})
private Set<Authority> authorities;
Copy link

Choose a reason for hiding this comment

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

권한을 코드레벨이 아니라 DB레벨로 뺀 이유가 있을까요?

Jwts.parserBuilder().setSigningKey(key).build().parseClaimsJws(token);
return true;
} catch (SecurityException | MalformedJwtException e) {
logger.error("잘못된 JWT 서명입니다.");
Copy link

Choose a reason for hiding this comment

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

토큰 검증에서 에러가 발생하지 않아서 클라에서는 응답은 정상으로 받지만 리소스를 받지는 못할 것 같은데 그러면 각 에러케이스별로 어떤 액션을 해야할지 모르지 않을까요?

this.passwordEncoder = passwordEncoder;
}

public UserDto signUp(UserDto userDto) {
Copy link

Choose a reason for hiding this comment

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

Transactional을 붙여야 할 것 같네요


@Override
@Transactional
public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException {
Copy link

Choose a reason for hiding this comment

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

오잉 요게 있으면 /login 요청은 여기로 올텐데 위에 AuthController는 JWT용으로 따로 빼신건가요? 로그인 프로세스가 어떻게 되는거징..


public UserDto signUp(UserDto userDto) {
if (userRepository.findByUsername(userDto.getUsername()).orElse(null) != null)
throw new DuplicateMemberException("이미 가입되어 있는 유저");
Copy link

Choose a reason for hiding this comment

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

도메인 exception 자체가 의미를 담고 있는것 같은데 굳이 error message를 넘겨줘야 할까요?

public UserDto getMyUserWithAuthorities() {
return UserDto.from(
SecurityUtil.getCurrentUsername()
.flatMap(userRepository::findByUsername)
Copy link

Choose a reason for hiding this comment

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

여긴 왜 flatMap을 사용하셨나요 ? .get() 해도 될 것 같은데

@wapago
Copy link

wapago commented Oct 24, 2023

늦은 코멘트 미안합니당.. 수고 많으셨어요:)

Copy link

@wapago wapago left a comment

Choose a reason for hiding this comment

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

LGTM d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants