[BE] feat: 리소스 접근 권한 검증 aop 구현 #1089

nayonsoso · 2025-02-12T08:35:34Z

resolves [BE] 리소스에 대한 권한 체크를 aop로 진행한다. #1085

🚀 어떤 기능을 구현했나요 ?

리소스 접근 권한 검증 aop를 구현했습니다.

🔥 어떻게 해결했나요 ?

시행착오가 조금 있었는데요😅
처음에는 '권한'이라는 것이 비지니스 로직이라고 생각해서 서비스 코드에 aop 를 두었어요.
그런데 테드가 구현한 resolver 를 보고, 권한 검증도 컨트롤러에서 하는게 좋겠다 싶어서 추가 커밋으로 코드를 바꿨습니다.

📝 어떤 부분에 집중해서 리뷰해야 할까요?

네이밍 고민되는게 있는데, RequireReviewAccess vs RequireReviewAuthorization 중에 뭐가 더 좋나요?

📚 참고 자료, 할 말

github-actions · 2025-02-12T08:37:05Z

Test Results

196 tests +23 193 ✅ +23 5s ⏱️ ±0s
73 suites +10 3 💤 ± 0
73 files +10 0 ❌ ± 0

Results for commit de1ff1c. ± Comparison against base commit 19c7845.

♻️ This comment has been updated with latest results.

Kimprodp

아직 꼼꼼하게 다 확인은 못했는데요,

리졸버 구현과 중복되는 로직이 존재하는 것 같아서 우선 RC 먼저 남길게요!

리졸버에서 세션을 확인하고, dto를 반환해서 컨트롤러 메서드 인자로 넘겨주고 있습니다.
근데 aop에서도 세션을 확인하는 동일한 로직을 하는 것으로 보여요. (아마 리졸버 구현 전이라 이렇게 구현했을 거라 생각해요.)

따라서 리졸버 구현이 단순하니 먼저 머지하고, JoinPoint의 LoginMember 또는 GuestReviewGroup을 직접 사용하도록 변경하면 어떨까요?

존재하는 세션을 확인한다. (회원 세션 or 비회원 세션)
존재하는 세션과 요청된 자원 id의 관계를 확인하여 검증

위 내용에 동의하면 작업하는 동안 더 세부적으로 보고있겠습니다~

nayonsoso · 2025-02-13T15:55:11Z

저는 “세션에 저장된 정보를 가져오기 위한 resolver”와
“세션에 저장된 정보로부터 권한을 체크하기 위한 aop”가 분명히 다른 목적을 가지고 있다고 생각해요.
그래서 단일 책임 원칙을 더 중요시하고 싶어요..!

resolver가 만들어준 LoginMember 또는 GuestReviewGroup를 직접 사용하는 aop라면, 아래와 같이 구현될거예요.

@PostMapping("/v2/highlight")
@RequireReviewGroupAccess(target = "#request.reviewGroupId()”)
public ResponseEntity<Void> highlight(
        @Valid @RequestBody HighlightsRequest request,
        @LoginMemberSession(required = false) LoginMember loginMember,
        @GuestReviewGroupSession(required = false) GuestReviewGroup guestReviewGroup
) {
    highlightService.editHighlight(request);
    return ResponseEntity.ok().build();
}

aop 가 resolver에 종석적이라서, aop 에게 값을 전달하기 위한 용도의 인자가 생겼어요. 🙀
실제로 로직에 사용되는게 아니라 그저 “aop 에 값을 전달하기 위한” 코드가 존재하는건 좋지 않다 생각해요.

이들이 둘 다 세션에 접근하긴 하지만, aop와 resolver 를 같이 쓰는 곳이 없어요.
연산이 중복해서 일어나는 것도 아니라, 비효율적이지 않다고 생각합니다.

@PostMapping("/v2/highlight")
@RequireReviewGroupAccess(target = "#request.reviewGroupId()”)
public ResponseEntity<Void> highlight(
        @Valid @RequestBody HighlightsRequest request
) {
    highlightService.editHighlight(request);
    return ResponseEntity.ok().build();
}

테드는 첫번째 코드가 더 읽기 좋나요, 두번째 코드가 읽기 좋나요?
저는 두번째가 명확해서 보기 좋아보입니다... 🥹

Kimprodp · 2025-02-13T16:19:09Z

사전에 작업 이해한 내용이 조금 달랐던 거 같네요..
저는 리졸버를 통해 받아온 걸로 인증하자고 이야기한걸로 기억해서 그런 방식으로 구현하는 줄 알았어요.

그럼 산초가 말한 것처럼 리졸버는 기존처럼 변환만 하고 aop에서 모든 인증을 다 하는 게 깔끔하네요~
코드 다시 확인하고 리뷰 남길게요!

Kimprodp

산초우 aop 구현 잘했네요 👍👍
다만 몇 가지 포인트에서 문제 될 수 있는 부분이나 더 좋은 의견과 함께 리뷰 남깁니다!

Kimprodp · 2025-02-14T01:33:47Z

backend/src/main/java/reviewme/global/authorization/ReviewAuthorizationAspect.java

+    private final ReviewRepository reviewRepository;
+    private final ReviewGroupRepository reviewGroupRepository;
+
+    @Transactional


Aspect 내부에서 트랜잭션을 사용하는 이유가 무엇인가요?

이 안에서 트랜잭션을 시작하면, 비즈니스 로직이 수행되는 서비스 계층의 트랜잭션 경계와 중첩 되어서 예상치 못한 작동이 일어날 수 있다고 생각하는데요. 또한 컨트롤러 메서드가 트랜잭션 안에서 실행되어야 하는 이유도 모르겠어요.

DB조회하는 동작이라 습관적으로 붙였던 것 같습니다😞
없애는게 맞겠네요!

3ab68dc

Kimprodp · 2025-02-14T01:54:15Z

backend/src/main/java/reviewme/global/authorization/ReviewAuthorizationAspect.java

+    private boolean isMemberAuthorized(Review review, HttpSession session) {
+        GitHubMember gitHubMember = sessionManager.getGitHubMember(session);
+        if (gitHubMember == null) {
+            return false;
+        }
+
+        boolean isReviewGroupCreator = reviewGroupRepository.findAllByMemberId(gitHubMember.getMemberId())
+                .stream()
+                .map(ReviewGroup::getId)
+                .anyMatch(id -> id == review.getReviewGroupId());
+        boolean isReviewAuthor = review.getMemberId() != null && review.getMemberId() == gitHubMember.getMemberId();


회원이 등록한 모든 그룹을 찾고 반복하면서 리뷰의 그룹 아이디와 일치하는 그룹을 찾고 있어서 비효율적으로 보여요.
리뷰에 있는 그룹 아이디로 해당 그룹만 찾고 그룹에 있는 회원 아이디와 gitHubMember.getMemberId()만 비교하면 간단할 거 같다는 생각인데, 혹시 이렇게 한 이유가 있을까요?

or reviewGroupRepository.existsByMemberIdAndReviewGroupId

혹시 이렇게 한 이유가 있을까요?

특별한 이유는 없고, 위 박식만 떠올라서 그렇게 했습니다😅
테드가 제안한 방식이 더 효율적이겠네요.

bb63961

Kimprodp · 2025-02-14T02:01:49Z

backend/src/main/java/reviewme/global/authorization/ReviewAuthorizationAspect.java

+        }
+
+        ReviewGroup reviewGroup = reviewGroupRepository.findByReviewRequestCode(reviewRequestCode)
+                .orElseThrow(() -> new ReviewGroupNotFoundByReviewRequestCodeException(reviewRequestCode));


이 예외를 여기서 던지는 것이 맞을지 생각이 듭니다요..

예외 메세지가 클라이언트에게 전달이 될텐데, 클라이언트 입장에서는 세션을 통해 던져진 ReviewRequestCode라서, 사실 세션을 사용한지도 모를텐데 뜬금없는 메세지를 받게 될 것 같아요. 그리고 세션에 저장된 코드로 도메인 객체를 찾을 수 없다는 건 NotFound가 아닌 서버 에러로 봐야 한다고 생각합니다!

그렇네요, ReviewRequestCode 때문이라는 메세지를 담은
"ReviewGroupNotFoundByReviewRequestCodeException"는 변경해야겠네요😅

9412d32

Kimprodp · 2025-02-14T02:15:01Z

backend/src/main/java/reviewme/global/authorization/ReviewGroupAuthorizationAspect.java

+    private final SessionManager sessionManager;
+    private final ReviewGroupRepository reviewGroupRepository;
+
+    @Transactional


여기 트랜잭션도 위에서 했던 의견과 동일합니다!

cloudflare-workers-and-pages · 2025-02-15T11:41:14Z

Deploying 2024-review-me-release with Cloudflare Pages

Latest commit:	`de1ff1c`
Status:	✅ Deploy successful!
Preview URL:	https://c6652008.2024-review-me-release.pages.dev
Branch Preview URL:	https://be-feat-1085-resource-author.2024-review-me-release.pages.dev

View logs

cloudflare-workers-and-pages · 2025-02-15T11:42:34Z

Deploying 2024-review-me-develop with Cloudflare Pages

Latest commit:	`de1ff1c`
Status:	✅ Deploy successful!
Preview URL:	https://1c986b6f.2024-review-me-develop.pages.dev
Branch Preview URL:	https://be-feat-1085-resource-author.2024-review-me-develop.pages.dev

View logs

nayonsoso · 2025-02-15T11:46:06Z

✍️ 추가된 부분

리뷰 남겨준 부분 반영했습니다.
권한 관련 예외를 Unauthorized → Forbidden 로 변경했습니다.
권한 관련 클래스들 security 패키지 하위로 이동했습니다.

- 세션과 권한의 책임으로 바뀌었으므로 삭제한다.

- 더 중요한 검증이 먼저 오도록

- 서비스 코드에 aop 를 붙이지 않고, 컨트롤러에 붙임에 따라서 테스트 설정을 원복한다.

- 세션에 저장된 정보에 해당하는게 DB에 없다는 의미 전달

- global.authorization -> security.aspects

nayonsoso added the ✨ BE label Feb 12, 2025

nayonsoso self-assigned this Feb 12, 2025

nayonsoso requested review from donghoony, Kimprodp and skylar1220 as code owners February 12, 2025 08:35

nayonsoso changed the title ~~feat: 리소스 접근 권한 검증 aop 구현~~ [BE] feat: 리소스 접근 권한 검증 aop 구현 Feb 12, 2025

Kimprodp requested changes Feb 13, 2025

View reviewed changes

Kimprodp requested changes Feb 14, 2025

View reviewed changes

nayonsoso force-pushed the be/feat/1085-resource-authorization-aop branch from 58ed576 to d8f2ca4 Compare February 15, 2025 11:41

nayonsoso added 15 commits February 15, 2025 23:54

feat: 리소스 접근 관한 관리 어노테이션 생성

745e5db

feat: 리소스 접근 관한에 사용될 util 코드 생성

9fd2cb5

feat: 리뷰 그룹 권한 aop 구현

79ea4e8

feat: 리뷰 권한 aop 구현

15f91ff

feat: 리뷰그룹 접근 권한 aop 구현

5c671b9

test: 권한 aop 테스트

0e4c5ac

test: 서비스 코드의 테스트에 aop가 동작하지 않도록 설정 변경

61f5260

refactor: 하이라이트 수정 시, ReviewGroupSession 을 사용하지 않도록

720db84

refactor: 단일리뷰 조회 시, ReviewGroupSession 을 사용하지 않도록

a849300

refactor: 리뷰 요약 조회 시, ReviewGroupSession 을 사용하지 않도록

190ca47

refactor: 리뷰 목록 조회 시, ReviewGroupSession 을 사용하지 않도록

9e753a0

refactor: 리뷰 모아보기 시, ReviewGroupSession 을 사용하지 않도록

b06827a

test: 사용하지 않는 테스트 삭제

71fb966

- 세션과 권한의 책임으로 바뀌었으므로 삭제한다.

refactor: 권한 검증 순서 변경

d11211e

- 더 중요한 검증이 먼저 오도록

test: 관련된 테스트 코드끼리 분류

5748b3c

nayonsoso added 10 commits February 15, 2025 23:55

test: 테스트 설정 수정

3b8983f

- 서비스 코드에 aop 를 붙이지 않고, 컨트롤러에 붙임에 따라서 테스트 설정을 원복한다.

refactor: 서비스가 아닌 컨트롤러에서 권한 검증하도록

73ad384

refactor: aspect에 사용된 트랜잭션 제거

7a87f90

refactor: 회원이 만든 리뷰인지 확인하는 로직 수정

1cc9bf8

refactor: 사용되는 예외 변경

d83de97

- 세션에 저장된 정보에 해당하는게 DB에 없다는 의미 전달

refactor: 권한 관련 예외와 함수 이름 변경

3f94d5a

refactor: forbidden 예외 핸들러 추가

8cfdbf3

refactor: 패키지 변경

120be95

- global.authorization -> security.aspects

fix: import 추가

f3ccc13

test: 변경된 픽스쳐 적용

de1ff1c

nayonsoso force-pushed the be/feat/1085-resource-authorization-aop branch from 281a992 to de1ff1c Compare February 15, 2025 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] feat: 리소스 접근 권한 검증 aop 구현 #1089

[BE] feat: 리소스 접근 권한 검증 aop 구현 #1089

nayonsoso commented Feb 12, 2025 •

edited

Loading

github-actions bot commented Feb 12, 2025 •

edited

Loading

Kimprodp left a comment

nayonsoso commented Feb 13, 2025 •

edited

Loading

Kimprodp commented Feb 13, 2025

Kimprodp left a comment

Kimprodp Feb 14, 2025

nayonsoso Feb 15, 2025 •

edited

Loading

Kimprodp Feb 14, 2025

nayonsoso Feb 15, 2025 •

edited

Loading

Kimprodp Feb 14, 2025 •

edited

Loading

nayonsoso Feb 15, 2025 •

edited

Loading

Kimprodp Feb 14, 2025

cloudflare-workers-and-pages bot commented Feb 15, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 15, 2025 •

edited

Loading

nayonsoso commented Feb 15, 2025

[BE] feat: 리소스 접근 권한 검증 aop 구현 #1089

Are you sure you want to change the base?

[BE] feat: 리소스 접근 권한 검증 aop 구현 #1089

Conversation

nayonsoso commented Feb 12, 2025 • edited Loading

🚀 어떤 기능을 구현했나요 ?

🔥 어떻게 해결했나요 ?

📝 어떤 부분에 집중해서 리뷰해야 할까요?

📚 참고 자료, 할 말

github-actions bot commented Feb 12, 2025 • edited Loading

Test Results

Kimprodp left a comment

Choose a reason for hiding this comment

nayonsoso commented Feb 13, 2025 • edited Loading

Kimprodp commented Feb 13, 2025

Kimprodp left a comment

Choose a reason for hiding this comment

Kimprodp Feb 14, 2025

Choose a reason for hiding this comment

nayonsoso Feb 15, 2025 • edited Loading

Choose a reason for hiding this comment

Kimprodp Feb 14, 2025

Choose a reason for hiding this comment

nayonsoso Feb 15, 2025 • edited Loading

Choose a reason for hiding this comment

Kimprodp Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

nayonsoso Feb 15, 2025 • edited Loading

Choose a reason for hiding this comment

Kimprodp Feb 14, 2025

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Feb 15, 2025 • edited Loading

Deploying 2024-review-me-release with Cloudflare Pages

cloudflare-workers-and-pages bot commented Feb 15, 2025 • edited Loading

Deploying 2024-review-me-develop with Cloudflare Pages

nayonsoso commented Feb 15, 2025

✍️ 추가된 부분

nayonsoso commented Feb 12, 2025 •

edited

Loading

github-actions bot commented Feb 12, 2025 •

edited

Loading

nayonsoso commented Feb 13, 2025 •

edited

Loading

nayonsoso Feb 15, 2025 •

edited

Loading

nayonsoso Feb 15, 2025 •

edited

Loading

Kimprodp Feb 14, 2025 •

edited

Loading

nayonsoso Feb 15, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 15, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 15, 2025 •

edited

Loading