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.

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

[chance0523-issue32] jwt 구현 #35

Merged
merged 6 commits into from
Sep 6, 2021
Merged

Conversation

chance0523
Copy link
Contributor

Issue: #32

작업 내용

  • 무지성 jwt 구현입니다.

생성/변경 로직

  • jwt 추가

개인 코멘트

  • 무지성으로 실버나인님 + 오리님 예제코드 짬뽕으로 구현했습니다.
  • 그야말로 무지성이라 이해는 제대로 하지는 못했습니다...

@chance0523 chance0523 added the chance 찬스 label Aug 22, 2021
@chance0523 chance0523 self-assigned this Aug 22, 2021
Copy link
Member

@jinyoungchoi95 jinyoungchoi95 left a comment

Choose a reason for hiding this comment

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

예제 코드라 사실 제가 리뷰할 다른 부분이 바로 보이지는 않네요 🤣
그래도 기간내에 최대한 적용하셔서 보기 좋은 것 같습니다 🥇

추가적으로 controller의 response json에 대한 테스트나, 추가하지 못하셨던 다양한 실패 케이스에 대해서 고려보시면 좋을 것 같아요 :)

String header = request.getHeader(HttpHeaders.AUTHORIZATION);

String[] headers = header.split(" ");
if (headers.length == 2 && headers[0].equals("Token") && StringUtils.hasText(headers[1])) {
Copy link
Member

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.

넵~!

Copy link
Member

@hoony-lab hoony-lab left a comment

Choose a reason for hiding this comment

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

테스트까지 완료하셔서 초록색을 보여주세요 !
마지막까지 ✅

@@ -33,12 +33,15 @@

private final String image;

public static UserResponse createResponse(User user) {
private final String accessToken;
Copy link
Member

Choose a reason for hiding this comment

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

@JsonProperty("token")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아 token이 api 스펙이었네요
수정했습니다 👍

Comment on lines +39 to +41
//@Formatter:off
@WebMvcTest({UserController.class, SecurityConfig.class, JwtAuthenticationEntryPoint.class, JwtAccessDeniedHandler.class})
//@Formatter:on
Copy link
Member

Choose a reason for hiding this comment

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

What is @Formatter ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

//@Formatter:off 얘와
//@Formatter:on 얘 사이에 있는 코드는
포맷팅이 되지 않습니다! (자동정렬 안 먹음)

* @author JeongJoon Seo
*/
@Component
public class JwtAccessDeniedHandler implements AccessDeniedHandler {
Copy link
Member

Choose a reason for hiding this comment

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

👍

* @author JeongJoon Seo
*/
@Component
public class JwtAuthenticationEntryPoint implements AuthenticationEntryPoint {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@chance0523 chance0523 changed the base branch from chance0523-issue11 to chance0523 August 27, 2021 14:09
@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2021

Codecov Report

Merging #35 (89b7dd8) into chance0523 (1b3505f) will decrease coverage by 5.55%.
The diff coverage is 90.56%.

Impacted file tree graph

@@               Coverage Diff                @@
##             chance0523      #35      +/-   ##
================================================
- Coverage        100.00%   94.44%   -5.56%     
- Complexity           17       28      +11     
================================================
  Files                10       16       +6     
  Lines                41       90      +49     
  Branches              0        1       +1     
================================================
+ Hits                 41       85      +44     
- Misses                0        4       +4     
- Partials              0        1       +1     
Impacted Files Coverage Δ
...va/com/study/realworld/core/jwt/TokenProvider.java 73.33% <73.33%> (ø)
...n/java/com/study/realworld/core/jwt/JwtFilter.java 91.66% <91.66%> (ø)
...ealworld/core/configuration/JwtSecurityConfig.java 100.00% <100.00%> (ø)
...y/realworld/core/configuration/SecurityConfig.java 100.00% <100.00%> (ø)
...udy/realworld/core/jwt/JwtAccessDeniedHandler.java 100.00% <100.00%> (ø)
...om/study/realworld/core/jwt/JwtAuthentication.java 100.00% <100.00%> (ø)
...ealworld/core/jwt/JwtAuthenticationEntryPoint.java 100.00% <100.00%> (ø)
...dy/realworld/user/presentation/UserController.java 100.00% <100.00%> (ø)
...ealworld/user/presentation/model/UserResponse.java 100.00% <100.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b3505f...89b7dd8. Read the comment docs.

/**
* @author Jeongjoon Seo
*/
@Configuration
@EnableGlobalMethodSecurity(prePostEnabled = true)
Copy link
Member

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.

이거 뭔가 인프런 강의 따라 치다가 들어간 것 같은데요...
https://ncucu.me/136
위 블로그를 참고해보면

- Spring Security는 Web기반의 Security 외에 Method Security 기능을 제공하는데
Method Security는 우리가 기존에 사용했던 SecurityConfig 설정이 적용되지 않는다.
- Method Security용 설정이 따로 필요한데 이때 사용하는것이 **@EnableGlobalMethodSecurity**이다.
- `prePostEnabled`은 @PreAuthorize, @PostAuthorize을 사용할 수 있게 해줍니다 (여기서는 사용하지 않았습니다)

이정도?로 정리할 수 있겠네요. 아마 지금은 EnableGlobalMethodSecurity의 기능을 쓰고 있지 않는 것 같아서 없어도 되는 코드 같기도 하네요 (어려운 Spring Security.. 정확히 설명을 못 드리겠습니다 ㅠ_ㅠ)

Copy link

@kwj1270 kwj1270 left a comment

Choose a reason for hiding this comment

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

후니님께서 리뷰를 너무 잘해주셔서 크게 드릴건 없는 것 같네요 수고하셨습니다.

String header = request.getHeader(HttpHeaders.AUTHORIZATION);

String[] headers = header.split(" ");
if (headers.length == 2 && headers[0].equals("Token") && StringUtils.hasText(headers[1])) {
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.

아 그렇겠네요 감사합니다 👍

Copy link
Member

@hoony-lab hoony-lab left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@chance0523 chance0523 merged commit 2412aa5 into chance0523 Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chance 찬스
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants