Feature/#156 관리자 권한 설정 #850

amaran-th · 2023-11-27T05:57:35Z

#️⃣ 연관된 이슈

close : #156

📝 작업 내용

관리자 로그인 기능 구현 및 관리자 API 접근 권한 설정, 관리자 로그인 정보(아이디, 패스워드)를 서브모듈에 업데이트

예상 소요 시간 및 실제 소요 시간 (일 / 시간 / 분)

예상 소요 시간 : 11/26
실제 소요 시간 : 11/27

- admin 패키지를 분리 - admin api의 uri 수정 #156

- 아이디와 패스워드가 설정된 값과 일치하면 관리자 토큰 발급 #156

- 헤더에 관리자 토큰이 포함되어있지 않으면 예외를 반환 #156

#156

…023-emmsale into Feature/#156-관리자_권한_설정

hong-sile

조금 큰 작업이니 만큼 리뷰도 좀 있네요.

RC 날릴테니 코멘트 날린 부분에 대한 답을 해주시면 다시 리뷰요청해주세요

hong-sile · 2023-11-27T07:00:04Z

backend/emm-sale/src/documentTest/java/com/emmsale/ActivityApiTest.java

-      fieldWithPath("[].id").type(JsonFieldType.NUMBER).description("activity id"),
-      fieldWithPath("[].activityType").type(JsonFieldType.STRING).description("activity 분류"),
-      fieldWithPath("[].name").type(JsonFieldType.STRING).description("activity 이름")
+        fieldWithPath("[].id").type(JsonFieldType.NUMBER).description("activity id"),


이거 공백이 두칸씩 계속 더 생기는데, google style convention 유지하고 계신거 맞을까요?

확인해봤는데 그대로 사용하고 있는 것 같습니다. 어디서 차이가 생기고 있는건지 모르겠네요....;;;

hong-sile · 2023-11-27T07:02:04Z

backend/emm-sale/src/documentTest/java/com/emmsale/EventApiTest.java

        .file("images", image1.getBytes())
        .file("images", image2.getBytes())
        .file(new MockMultipartFile("request", "", "application/json", contents.getBytes(
            StandardCharsets.UTF_8)));

-    final ResultActions result = mockMvc.perform(builder);
+    final ResultActions result = mockMvc.perform(builder.header("Authorization", accessToken));


이 header에 대한 부분도 위에 MockMultipartHttpServletRequestBuilder에 넣을 수 있지 않을까요??

반영했습니다!

hong-sile · 2023-11-27T07:30:16Z

backend/emm-sale/src/main/java/com/emmsale/admin/login/utils/AdminValidator.java

+
+  public static void validateAuthorization(final Member admin) {
+    if (admin.isNotMe(adminMemberId)) {
+      throw new LoginException(LoginExceptionType.INVALID_ACCESS_TOKEN);


개인적으로 INVALID_ACCESS_TOKEN 보다, 유효하지 않은 Admin이라는 것을 나타내는건 어떨까요?

다른 LoginExceptionType을 만들면 좋을 것 같아요.

다시 보니까 Invalid access token도 ExceptionType으로 있네요 그걸 이용해보는 건 어떨까요?

반영하였습니다!

hong-sile · 2023-11-27T07:31:38Z

backend/emm-sale/src/main/java/com/emmsale/admin/login/utils/AdminValidator.java

+    }
+  }
+
+  @Value("${data.admin_login.member_id}")


혹시 Setter 주입말고, 생성자 주입을 사용하는 건 어떨까요?

static변수와 setter가 섞여있는 구조는 좋지 못한 것 같아요.

필드를 static이 아닌 변수로 두고, 생성자 주입으로 바꿔보죠.

아니면 혹시 이렇게 만든 이유가 있을까요?

처음에 application.yml에 정의한 속성 값을 static 변수에 초기화하려고 하니 런타임 시점에 null 값이 들어가더라구요. 그래서 방법을 찾아봤는데 대부분의 문서에서 setter 주입으로 문제를 해결하는 것을 보고 똑같이 적용했었습니다.

참고했던 게시글

앞서 남겨주신 코멘트를 반영해서 AdminValidator를 AOP로 처리할 수 있게 리팩토링한다면 자연스럽게 사라질 코드로 보이는데, 이 코멘트까지 다음 이슈로 넘겨도 괜찮을까요?

hong-sile · 2023-11-27T07:32:59Z

backend/emm-sale/src/main/java/com/emmsale/admin/login/utils/AdminValidator.java

+
+  private static Long adminMemberId;
+
+  public static void validateAuthorization(final Member admin) {


이 메서드가 현재 모든 method에서 동작하고 있는데,

혹시 별도로 interceptor 같은 걸로 한번에 처리할 수 있지 않을까요? 어차피 엔드포인트는
/admin으로 동일하잖아요

아니면, AOP를 이용해볼 수도 있을 것 같아요.

음.../admin/login이 예외적으로 관리자 권한 인가가 필요하지 않는 엔드포인트라서, 원하는 API에만 검증 로직을 추가해주려면 AOP를 활용해야 할 것 같아요.
근데..제가 어제부터 계속 고민하고 있었는데 생각보다 방법을 찾기가 어렵고 지금 코드에서 수정할 것도 많아보여서 해당 리팩토링 작업은 다음 이슈로 미루어도 괜찮을까요??🥲

네 일단 행사를 추가할 수 있어야 하니 다음 이슈로 만들어두죠. 어쩃든 지금 동작은 하니까요

hong-sile · 2023-11-27T07:33:54Z

backend/emm-sale/src/main/java/com/emmsale/admin/login/application/dto/AdminLoginRequest.java

+@RequiredArgsConstructor
+public class AdminLoginRequest {
+
+  final String id;


필드에 private을 붙이면 좋을 것 같아요

헉...알려주셔서 감사합니다.

hong-sile · 2023-11-27T07:54:08Z

backend/emm-sale/src/test/java/com/emmsale/admin/login/application/AdminLoginServiceTest.java

+  @Autowired
+  private AdminLoginService adminLoginService;
+  @MockBean
+  private JwtTokenProvider tokenProvider;


JwtTokenProvider를 mockBean으로 생성한 이유가 잇을까요?

mockBean을 사용하면 컨텍스트 캐싱이 깨져서 속도가 느려질 수 있어서 최대한 지양하고 있는데, 다른 방법을 시도해볼 수 있지 않을까요?

이 코멘트를 보고 @Mockbean과 컨텍스트 캐싱이라는 키워드로 테스트 성능에 대해 알아봤는데, 저희 프로젝트에서 ServiceIntegrationTestHelper 클래스에 @Mockbean 필드를 모아둔 이유가 있었군요...! 이걸 이제야 알아버렸네요;; 코멘트 감사합니다.
일단 해당 필드는 ServiceIntegrationTestHelper 클래스로 옮겨주었습니다!

근데 이렇게 하니 Mockbean이 아니라 실제 빈을 사용하는 JwtTokenProviderTest에서 테스트가 실패하네요.
그리고 테스트 코드를 다시 보니까, JwtTokenProvider를 굳이 Mocking하지 않아도 충분히 수행할 수 있는 테스트라서 그냥 @MockBean 어노테이션을 사용하지 않도록 수정했습니다!

좋아요. 저도 굳이 mocking하라 필요는 없는 것 같아서 지금 방식이 적당한 것 같습니다.

hong-sile · 2023-11-27T07:55:38Z

backend/emm-sale/src/test/java/com/emmsale/member/MemberFixture.java

        "https://avatars.githubusercontent.com/0/4",
        "아마란스"
    );
    member.updateName("우르");
    return member;
  }

+  public static Member generalMember() {


혹시 별도로 generalMember를 만든 이유가 있을까요?

관리자 권한 검증을 위해선 id를 가지고 있는 멤버가 필요해서 새로 만들어주었습니다!
기존의 memberFixture에 id 값을 주면 실패하는 테스트가 있더라구요.

#156

amaran-th

세세한 코드리뷰 감사합니다! 덕분에 배운 게 많네요...🫠
모두 반영하고 싶었은데 권한 검증 로직을 AOP로 구현하는 작업이 저에겐 생각보다 까다롭더라구요.,,,

amaran-th · 2023-11-27T08:26:53Z

backend/emm-sale/src/documentTest/java/com/emmsale/ActivityApiTest.java

-      fieldWithPath("[].id").type(JsonFieldType.NUMBER).description("activity id"),
-      fieldWithPath("[].activityType").type(JsonFieldType.STRING).description("activity 분류"),
-      fieldWithPath("[].name").type(JsonFieldType.STRING).description("activity 이름")
+        fieldWithPath("[].id").type(JsonFieldType.NUMBER).description("activity id"),


확인해봤는데 그대로 사용하고 있는 것 같습니다. 어디서 차이가 생기고 있는건지 모르겠네요....;;;

amaran-th · 2023-11-27T08:28:18Z

backend/emm-sale/src/main/java/com/emmsale/admin/login/application/dto/AdminLoginRequest.java

+@RequiredArgsConstructor
+public class AdminLoginRequest {
+
+  final String id;


헉...알려주셔서 감사합니다.

amaran-th · 2023-11-27T08:53:08Z

backend/emm-sale/src/documentTest/java/com/emmsale/EventApiTest.java

        .file("images", image1.getBytes())
        .file("images", image2.getBytes())
        .file(new MockMultipartFile("request", "", "application/json", contents.getBytes(
            StandardCharsets.UTF_8)));

-    final ResultActions result = mockMvc.perform(builder);
+    final ResultActions result = mockMvc.perform(builder.header("Authorization", accessToken));


반영했습니다!

amaran-th · 2023-11-27T09:06:30Z

backend/emm-sale/src/test/java/com/emmsale/member/MemberFixture.java

        "https://avatars.githubusercontent.com/0/4",
        "아마란스"
    );
    member.updateName("우르");
    return member;
  }

+  public static Member generalMember() {


관리자 권한 검증을 위해선 id를 가지고 있는 멤버가 필요해서 새로 만들어주었습니다!
기존의 memberFixture에 id 값을 주면 실패하는 테스트가 있더라구요.

amaran-th · 2023-11-27T11:32:32Z

backend/emm-sale/src/test/java/com/emmsale/admin/login/application/AdminLoginServiceTest.java

+  @Autowired
+  private AdminLoginService adminLoginService;
+  @MockBean
+  private JwtTokenProvider tokenProvider;


이 코멘트를 보고 @Mockbean과 컨텍스트 캐싱이라는 키워드로 테스트 성능에 대해 알아봤는데, 저희 프로젝트에서 ServiceIntegrationTestHelper 클래스에 @Mockbean 필드를 모아둔 이유가 있었군요...! 이걸 이제야 알아버렸네요;; 코멘트 감사합니다.
일단 해당 필드는 ServiceIntegrationTestHelper 클래스로 옮겨주었습니다!

amaran-th · 2023-11-27T15:19:00Z

backend/emm-sale/src/main/java/com/emmsale/admin/login/utils/AdminValidator.java

+
+  public static void validateAuthorization(final Member admin) {
+    if (admin.isNotMe(adminMemberId)) {
+      throw new LoginException(LoginExceptionType.INVALID_ACCESS_TOKEN);


반영하였습니다!

amaran-th · 2023-11-28T03:25:47Z

backend/emm-sale/src/test/java/com/emmsale/admin/login/application/AdminLoginServiceTest.java

+  @Autowired
+  private AdminLoginService adminLoginService;
+  @MockBean
+  private JwtTokenProvider tokenProvider;


근데 이렇게 하니 Mockbean이 아니라 실제 빈을 사용하는 JwtTokenProviderTest에서 테스트가 실패하네요.
그리고 테스트 코드를 다시 보니까, JwtTokenProvider를 굳이 Mocking하지 않아도 충분히 수행할 수 있는 테스트라서 그냥 @MockBean 어노테이션을 사용하지 않도록 수정했습니다!

amaran-th · 2023-11-28T07:06:31Z

backend/emm-sale/src/main/java/com/emmsale/admin/login/utils/AdminValidator.java

+
+  private static Long adminMemberId;
+
+  public static void validateAuthorization(final Member admin) {


음.../admin/login이 예외적으로 관리자 권한 인가가 필요하지 않는 엔드포인트라서, 원하는 API에만 검증 로직을 추가해주려면 AOP를 활용해야 할 것 같아요.
근데..제가 어제부터 계속 고민하고 있었는데 생각보다 방법을 찾기가 어렵고 지금 코드에서 수정할 것도 많아보여서 해당 리팩토링 작업은 다음 이슈로 미루어도 괜찮을까요??🥲

amaran-th · 2023-11-28T07:08:36Z

backend/emm-sale/src/main/java/com/emmsale/admin/login/utils/AdminValidator.java

+    }
+  }
+
+  @Value("${data.admin_login.member_id}")


처음에 application.yml에 정의한 속성 값을 static 변수에 초기화하려고 하니 런타임 시점에 null 값이 들어가더라구요. 그래서 방법을 찾아봤는데 대부분의 문서에서 setter 주입으로 문제를 해결하는 것을 보고 똑같이 적용했었습니다.

참고했던 게시글

앞서 남겨주신 코멘트를 반영해서 AdminValidator를 AOP로 처리할 수 있게 리팩토링한다면 자연스럽게 사라질 코드로 보이는데, 이 코멘트까지 다음 이슈로 넘겨도 괜찮을까요?

hong-sile

전체적으로 큰 부분은 잘 반영해준 것 같아요.

일단, 관리자 API가 추가되어야 행사 추가가 가능하니 지금 approve할게요!

amaran-th added 5 commits November 18, 2023 22:41

refactor: admin 패키지 분리

8a95686

- admin 패키지를 분리 - admin api의 uri 수정 #156

feat: 관리자 로그인 구현

d44fb36

- 아이디와 패스워드가 설정된 값과 일치하면 관리자 토큰 발급 #156

feat: 관리자 권한 검증 구현

dc9acef

- 헤더에 관리자 토큰이 포함되어있지 않으면 예외를 반환 #156

feat: dev application.yml 변경사항 업데이트

84b5493

#156

feat: prod application.yml 변경사항 업데이트

2d56921

#156

amaran-th added Backend 백엔드 관련 이슈 환경 설정 코드가 아닌 환경 관련 이슈 High Priority 리뷰 우선순위가 높은 PR labels Nov 27, 2023

amaran-th added this to the 레벨 5 milestone Nov 27, 2023

amaran-th requested a review from hong-sile November 27, 2023 05:57

amaran-th self-assigned this Nov 27, 2023

amaran-th added 2 commits November 27, 2023 15:08

feat: 파일 마지막 줄에 빈 줄 추가

98c5c3d

#156

Merge branch 'backend-main' of https://github.com/woowacourse-teams/2…

42a2a52

…023-emmsale into Feature/#156-관리자_권한_설정

hong-sile requested changes Nov 27, 2023

View reviewed changes

amaran-th added 4 commits November 27, 2023 17:29

refactor: dto 필드에 private 접근 제어자 추가

9d2a118

#156

refactor: @Mockbean 필드를 ServiceIntegrationTestHelper로 이동

2f2de41

#156

refactor: 관리자 토큰 검증 실패 시 발생하는 예외 유형을 INVALID_ADMIN_ACCESS_TOKEN으로 변경

27bb2aa

#156

refactor: JwtTokenProvider를 Mocking하지 않도록 수정

5160b41

#156

amaran-th commented Nov 28, 2023

View reviewed changes

hong-sile approved these changes Nov 28, 2023

View reviewed changes

amaran-th merged commit 2968c47 into backend-main Nov 28, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/#156 관리자 권한 설정 #850

Feature/#156 관리자 권한 설정 #850

amaran-th commented Nov 27, 2023

hong-sile left a comment

hong-sile Nov 27, 2023

amaran-th Nov 27, 2023

hong-sile Nov 27, 2023

amaran-th Nov 27, 2023

hong-sile Nov 27, 2023

hong-sile Nov 27, 2023

amaran-th Nov 27, 2023

hong-sile Nov 27, 2023

amaran-th Nov 28, 2023

hong-sile Nov 27, 2023

amaran-th Nov 28, 2023

hong-sile Nov 28, 2023

hong-sile Nov 27, 2023

amaran-th Nov 27, 2023

hong-sile Nov 27, 2023

hong-sile Nov 27, 2023

amaran-th Nov 27, 2023

amaran-th Nov 28, 2023

hong-sile Nov 28, 2023

hong-sile Nov 27, 2023

amaran-th Nov 27, 2023

amaran-th left a comment

amaran-th Nov 27, 2023

amaran-th Nov 27, 2023

amaran-th Nov 27, 2023

amaran-th Nov 27, 2023

amaran-th Nov 27, 2023

amaran-th Nov 27, 2023

amaran-th Nov 28, 2023

amaran-th Nov 28, 2023

amaran-th Nov 28, 2023

hong-sile left a comment


		private static Long adminMemberId;

		public static void validateAuthorization(final Member admin) {

Feature/#156 관리자 권한 설정 #850

Feature/#156 관리자 권한 설정 #850

Conversation

amaran-th commented Nov 27, 2023

#️⃣ 연관된 이슈

📝 작업 내용

예상 소요 시간 및 실제 소요 시간 (일 / 시간 / 분)

hong-sile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaran-th left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hong-sile left a comment

Choose a reason for hiding this comment