Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough요청자 Google Drive 접근을 요청자 OAuth로 먼저 검증한 뒤 서비스 계정 쓰기 권한 부여를 시도하도록 권한 흐름을 변경하고, Drive 클라이언트 빌드·검증·재검증 로직을 헬퍼로 분리하며 관련 테스트를 보강했습니다. (기능 호출 위치 변경 외 기존 임포트 흐름은 유지) Changes
Sequence Diagram(s)sequenceDiagram
participant Integrator as ClubSheetIntegratedService
participant PermSvc as GoogleSheetPermissionService
participant DriveUser as User Drive API
participant DriveSA as ServiceAccount Drive API
Integrator->>PermSvc: validateRequesterAccessAndTryGrantServiceAccountWriterAccess(requesterId, spreadsheetId)
PermSvc->>DriveUser: files.get(spreadsheetId).setFields("id").setSupportsAllDrives(true).execute()
alt 요청자 접근 허용
PermSvc->>DriveUser: permissions.list(spreadsheetId).setSupportsAllDrives(true).execute()
alt 서비스계정에 권한 존재
DriveUser-->>PermSvc: permissions list -> includes service account
PermSvc-->>Integrator: 검증 완료 (권한 확인)
else 권한 없음
PermSvc->>DriveSA: permissions.create/update(...).setSupportsAllDrives(true).execute()
DriveSA-->>PermSvc: 권한 생성/업데이트 응답
PermSvc->>DriveSA: files.get(spreadsheetId).setSupportsAllDrives(true).execute() (재검증)
alt 서비스계정 접근 확인
DriveSA-->>PermSvc: 파일 존재 응답
PermSvc-->>Integrator: 권한 부여 완료 (재검증 성공)
else 재검증 실패
DriveSA-->>PermSvc: 예외/접근없음
PermSvc-->>Integrator: 예외 던짐 (권한 부여 실패)
end
end
else 요청자 인증/권한 실패
DriveUser-->>PermSvc: 401/403/NOT_FOUND 등 오류
PermSvc-->>Integrator: 예외 던짐 (요청자 인증/권한 오류)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
JanooGwan
changed the title
There was a problem hiding this comment.
Pull request overview
Integrated 시트 등록 흐름에서 요청자(사용자) Google Drive OAuth 기준으로 실제 스프레드시트 접근 가능 여부를 선검증하여, 서비스 계정만 접근 가능해도 등록이 성공하던 문제를 방지하려는 PR입니다.
Changes:
GoogleSheetPermissionService에 요청자 Drive 접근 검증(files().get) + 이후 서비스 계정 권한 부여 시도를 묶은 신규 메서드 추가ClubSheetIntegratedService의 integrated 시작 지점에서 신규 검증 메서드를 호출하도록 변경- 요청자 접근 불가 시
FORBIDDEN_GOOGLE_SHEET_ACCESS로 실패하는 테스트 및 성공 시나리오 테스트 보강
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.java | 요청자 계정의 시트 접근 검증 후 서비스 계정 writer 권한 부여를 시도하는 흐름 추가 |
| src/main/java/gg/agit/konect/domain/club/service/ClubSheetIntegratedService.java | integrated 등록 시작 시점에 요청자 접근 검증 로직을 적용 |
| src/test/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionServiceTest.java | 요청자 접근 불가(403) 및 접근 가능 시나리오 테스트 추가/보강 |
| src/test/java/gg/agit/konect/domain/club/service/ClubSheetIntegratedServiceTest.java | 신규 메서드 호출로 테스트 갱신 및 접근 불가 시 후속 작업 중단 테스트 추가 |
| if (GoogleSheetApiExceptionHelper.isAccessDenied(e) | ||
| || GoogleSheetApiExceptionHelper.isAuthFailure(e) |
There was a problem hiding this comment.
validateRequesterSpreadsheetAccess()에서 GoogleSheetApiExceptionHelper.isAuthFailure(e) (401 Unauthorized)까지 accessDenied(FORBIDDEN_GOOGLE_SHEET_ACCESS)로 변환하고 있습니다. 인증 실패(토큰 만료/무효)는 권한 부족과 의미가 달라 클라이언트에 오해를 줄 수 있으니, isAuthFailure는 INVALID_GOOGLE_DRIVE_AUTH 또는 FAILED_INIT_GOOGLE_DRIVE 등 별도 에러로 매핑하도록 분기하는 것이 안전합니다.
| if (GoogleSheetApiExceptionHelper.isAccessDenied(e) | |
| || GoogleSheetApiExceptionHelper.isAuthFailure(e) | |
| if (GoogleSheetApiExceptionHelper.isAuthFailure(e)) { | |
| log.warn( | |
| "Google Drive OAuth auth failure while validating spreadsheet access. requesterId={}, " | |
| + "spreadsheetId={}, cause={}", | |
| requesterId, | |
| spreadsheetId, | |
| GoogleSheetApiExceptionHelper.extractDetail(e) | |
| ); | |
| throw GoogleSheetApiExceptionHelper.invalidGoogleDriveAuth(e); | |
| } | |
| if (GoogleSheetApiExceptionHelper.isAccessDenied(e) |
| @DisplayName("Drive OAuth가 미연결된 경우 검증을 건너뛰고 기존 흐름으로 가져오기를 계속 시도한다") | ||
| void analyzeAndImportPreMembersContinuesWhenDriveOAuthIsNotConnected() { |
There was a problem hiding this comment.
이 테스트는 googleSheetPermissionService가 Mock이라 실제로 “Drive OAuth 미연결” 시나리오를 구성/검증하지 못하고, 단지 permissionService가 예외 없이 반환하면 흐름이 계속된다는 것만 확인합니다. DisplayName/메서드명을 더 일반적인 표현으로 바꾸거나, OAuth 미연결 케이스는 GoogleSheetPermissionService 단위 테스트에서 검증하는 편이 테스트 의도를 명확히 합니다.
| @DisplayName("Drive OAuth가 미연결된 경우 검증을 건너뛰고 기존 흐름으로 가져오기를 계속 시도한다") | |
| void analyzeAndImportPreMembersContinuesWhenDriveOAuthIsNotConnected() { | |
| @DisplayName("구글 시트 권한 검증 후에도 기존 흐름대로 사전 회원 가져오기를 수행한다") | |
| void analyzeAndImportPreMembersContinuesAfterPermissionValidation() { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.java`:
- Around line 135-137: Add .setSupportsAllDrives(true) to all Drive API calls
that may operate on shared drives to ensure shared-drive spreadsheets are
handled: update userDriveService.files().get(spreadsheetId) call,
driveService.permissions().list(fileId) call,
userDriveService.permissions().create(fileId, permission) call, and
userDriveService.permissions().update(fileId, existingPermission.getId(),
updatedPermission) so each request includes .setSupportsAllDrives(true) before
.execute() (in GoogleSheetPermissionService where these symbols are used).
In
`@src/test/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionServiceTest.java`:
- Around line 257-302: Add two unit tests in GoogleSheetPermissionServiceTest
covering the unconnected-OAuth branches of
validateRequesterAccessAndTryGrantServiceAccountWriterAccess: one where
userOAuthAccountRepository.findByUserIdAndProvider(...) returns
Optional.empty(), and one where it returns a userOAuthAccount whose
getGoogleDriveRefreshToken() returns blank; in both tests call
validateRequesterAccessAndTryGrantServiceAccountWriterAccess(REQUESTER_ID,
FILE_ID) and assert it does not throw and that google drive interactions are
skipped by verifying files() and permissions.* methods are never invoked
(verify(files, never()).get(...); verify(permissions, never()).create(...),
etc.); if your service uses
googleSheetsConfig.buildUserDriveService(REFRESH_TOKEN) ensure you stub or avoid
calling it in the blank-token case so the behavior matches the contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06215671-dcae-4db6-a8d5-2f3de6fb6fcb
📒 Files selected for processing (4)
src/main/java/gg/agit/konect/domain/club/service/ClubSheetIntegratedService.javasrc/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.javasrc/test/java/gg/agit/konect/domain/club/service/ClubSheetIntegratedServiceTest.javasrc/test/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionServiceTest.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
src/main/java/**/*.java: 아래 원칙으로 리뷰 코멘트를 작성한다.
- 코멘트는 반드시 한국어로 작성한다.
- 반드시 수정이 필요한 항목만 코멘트로 남기고, 단순 취향 차이는 지적하지 않는다.
- 각 코멘트 첫 줄에 심각도를
[LEVEL: high|medium|low]형식으로 반드시 표기한다.- 심각도 기준: high=운영 장애 가능, medium=품질 저하, low=개선 권고.
- 각 코멘트는 "문제 -> 영향 -> 제안" 순서로 3문장 이내로 간결하게 작성한다.
- 가능하면 재현 조건 및 실패 시나리오도 포함한다.
- 제안은 현재 코드베이스(Spring Boot + JPA + Flyway) 패턴과 일치해야 한다.
- 보안, 트랜잭션 경계, 예외 처리, N+1, 성능 회귀 가능성을 우선 점검한다.
- 가독성: 변수/메서드 이름이 의도를 바로 드러내는지, 중첩과 메서드 길이가 과도하지 않은지 점검한다.
- 단순화: 불필요한 추상화, 중복 로직, 과한 방어 코드가 있으면 더 단순한 대안을 제시한다.
- 확장성: 새 요구사항 추가 시 변경 범위가 최소화되는 구조인지(하드코딩 분기/값 여부 포함) 점검한다.
Files:
src/main/java/gg/agit/konect/domain/club/service/ClubSheetIntegratedService.javasrc/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.java
**/*
⚙️ CodeRabbit configuration file
**/*: 공통 리뷰 톤 가이드:
- 모든 코멘트는 첫 줄에
[LEVEL: ...]태그를 포함한다.- 과장된 표현 없이 사실 기반으로 작성한다.
- 한 코멘트에는 하나의 이슈만 다룬다.
- 코드 예시가 필요하면 최소 수정 예시를 제시한다.
- 가독성/단순화/확장성 이슈를 발견하면 우선순위를 높여 코멘트한다.
Files:
src/main/java/gg/agit/konect/domain/club/service/ClubSheetIntegratedService.javasrc/test/java/gg/agit/konect/domain/club/service/ClubSheetIntegratedServiceTest.javasrc/test/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionServiceTest.javasrc/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.java
| File file = userDriveService.files().get(spreadsheetId) | ||
| .setFields("id") | ||
| .execute(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
service_file="$(fd -i 'GoogleSheetPermissionService.java' src | head -n1)"
helper_file="$(fd -i 'GoogleDrivePermissionHelper.java' src | head -n1)"
# 신규 접근 검증 경로에서 supportsAllDrives 사용 여부 확인
rg -n -C2 'files\(\)\.get|setSupportsAllDrives' "$service_file"
# 권한 조회/부여 helper 경로에서도 동일 플래그 사용 여부 확인
if [[ -n "${helper_file:-}" ]]; then
rg -n -C2 'permissions\(\)\.(list|create|update)|setSupportsAllDrives' "$helper_file"
fiRepository: BCSDLab/KONECT_BACK_END
Length of output: 909
공유 드라이브 스프레드시트 접근 검증에 setSupportsAllDrives(true) 누락
[LEVEL: high] Line 135-137의 files().get(spreadsheetId) 호출과 이후 권한 조회/부여 API들이 Google Drive API의 공유 드라이브 지원 플래그를 설정하지 않아, 공유 드라이브에 있는 스프레드시트는 요청자가 실제 접근 권한을 가져도 검증이 실패할 수 있습니다. 이로 인해 동아리 문서가 공유 드라이브에 위치할 경우 정상적인 등록 요청도 차단되는 장애가 발생할 수 있습니다. Line 135-137의 files().get(), 96의 permissions().list(), 143의 permissions().create(), 167의 permissions().update() 모두에 .setSupportsAllDrives(true)를 추가하여 shared drive 접근성을 보장하세요.
관련 코드
Line 135-137:
File file = userDriveService.files().get(spreadsheetId)
.setFields("id")
.execute();
Line 96:
Drive.Permissions.List request = driveService.permissions().list(fileId)
.setFields(PERMISSION_FIELDS);
Line 143-145:
userDriveService.permissions().create(fileId, permission)
.setSendNotificationEmail(false)
.execute();
Line 167-168:
userDriveService.permissions().update(fileId, existingPermission.getId(), updatedPermission)
.execute();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.java`
around lines 135 - 137, Add .setSupportsAllDrives(true) to all Drive API calls
that may operate on shared drives to ensure shared-drive spreadsheets are
handled: update userDriveService.files().get(spreadsheetId) call,
driveService.permissions().list(fileId) call,
userDriveService.permissions().create(fileId, permission) call, and
userDriveService.permissions().update(fileId, existingPermission.getId(),
updatedPermission) so each request includes .setSupportsAllDrives(true) before
.execute() (in GoogleSheetPermissionService where these symbols are used).
src/test/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionServiceTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.java`:
- Around line 49-51: 현재 GoogleSheetPermissionService에서 buildUserDriveService ->
validateRequesterSpreadsheetAccess -> tryGrantServiceAccountWriterAccess 순으로
호출하지만 tryGrantServiceAccountWriterAccess의 실패(false)를 무시해 권한 부여 실패가 정상 흐름으로
처리됩니다; fix: 변경할 수 있는 두 가지 중 하나를 선택하세요 — (1) tryGrantServiceAccountWriterAccess를
boolean에서 예외를 던지도록 변경하거나 (2) boolean 반환을 유지하되 호출부(해당 코드 블록)에서 반환값을 검사해 false이면
즉시 처리 중단(명시적 예외 throw)하도록 수정하고, 추가로 예외경로에서 서비스 계정 접근 재검증 로직을 넣어 불일치 원인을 명확히 하세요;
관련 식별자: tryGrantServiceAccountWriterAccess(...),
validateRequesterSpreadsheetAccess(...), buildUserDriveService(...), 그리고 이 실패가
downstream에 영향을 주는 ClubSheetIntegratedService.analyzeAndImportPreMembers()를 고려해
호출자가 중단 여부를 결정하도록 구현하세요.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53bf3f7a-e338-46c4-b866-68caec463617
📒 Files selected for processing (4)
src/main/java/gg/agit/konect/domain/club/service/GoogleDrivePermissionHelper.javasrc/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.javasrc/test/java/gg/agit/konect/domain/club/service/ClubSheetIntegratedServiceTest.javasrc/test/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionServiceTest.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
src/main/java/**/*.java: 아래 원칙으로 리뷰 코멘트를 작성한다.
- 코멘트는 반드시 한국어로 작성한다.
- 반드시 수정이 필요한 항목만 코멘트로 남기고, 단순 취향 차이는 지적하지 않는다.
- 각 코멘트 첫 줄에 심각도를
[LEVEL: high|medium|low]형식으로 반드시 표기한다.- 심각도 기준: high=운영 장애 가능, medium=품질 저하, low=개선 권고.
- 각 코멘트는 "문제 -> 영향 -> 제안" 순서로 3문장 이내로 간결하게 작성한다.
- 가능하면 재현 조건 및 실패 시나리오도 포함한다.
- 제안은 현재 코드베이스(Spring Boot + JPA + Flyway) 패턴과 일치해야 한다.
- 보안, 트랜잭션 경계, 예외 처리, N+1, 성능 회귀 가능성을 우선 점검한다.
- 가독성: 변수/메서드 이름이 의도를 바로 드러내는지, 중첩과 메서드 길이가 과도하지 않은지 점검한다.
- 단순화: 불필요한 추상화, 중복 로직, 과한 방어 코드가 있으면 더 단순한 대안을 제시한다.
- 확장성: 새 요구사항 추가 시 변경 범위가 최소화되는 구조인지(하드코딩 분기/값 여부 포함) 점검한다.
Files:
src/main/java/gg/agit/konect/domain/club/service/GoogleDrivePermissionHelper.javasrc/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.java
**/*
⚙️ CodeRabbit configuration file
**/*: 공통 리뷰 톤 가이드:
- 모든 코멘트는 첫 줄에
[LEVEL: ...]태그를 포함한다.- 과장된 표현 없이 사실 기반으로 작성한다.
- 한 코멘트에는 하나의 이슈만 다룬다.
- 코드 예시가 필요하면 최소 수정 예시를 제시한다.
- 가독성/단순화/확장성 이슈를 발견하면 우선순위를 높여 코멘트한다.
Files:
src/main/java/gg/agit/konect/domain/club/service/GoogleDrivePermissionHelper.javasrc/test/java/gg/agit/konect/domain/club/service/ClubSheetIntegratedServiceTest.javasrc/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.javasrc/test/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionServiceTest.java
src/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.java:63
- 통합 흐름이
validateRequesterAccessAndTryGrantServiceAccountWriterAccess로 이동하면서tryGrantServiceAccountWriterAccess(requesterId, spreadsheetId)는 현재 main 코드에서 호출처가 없어 보입니다. 외부 사용 의도가 없다면 접근자를 package-private로 낮추거나(테스트는 동일 패키지) 메서드 역할을 정리해 혼동을 줄이는 편이 유지보수에 유리합니다.
public boolean tryGrantServiceAccountWriterAccess(Integer requesterId, String spreadsheetId) {
String refreshToken = userOAuthAccountRepository
.findByUserIdAndProvider(requesterId, Provider.GOOGLE)
.map(account -> account.getGoogleDriveRefreshToken())
.filter(StringUtils::hasText)
.orElse(null);
| String refreshToken = userOAuthAccountRepository | ||
| .findByUserIdAndProvider(requesterId, Provider.GOOGLE) | ||
| .map(account -> account.getGoogleDriveRefreshToken()) | ||
| .filter(StringUtils::hasText) | ||
| .orElse(null); |
There was a problem hiding this comment.
validateRequesterAccessAndTryGrantServiceAccountWriterAccess()와 tryGrantServiceAccountWriterAccess()에서 refresh token 조회/필터링 로직이 동일하게 중복되어 있습니다. 한쪽만 수정되면 동작이 쉽게 불일치할 수 있으니, refresh token 해석 로직을 별도 private 메서드로 추출해 재사용하도록 정리하는 편이 안전합니다.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.java`:
- Around line 41-47: The current logic in GoogleSheetPermissionService that
returns early when refreshToken == null (skipping Drive access validation)
allows integrated sheet registration without verifying requester permissions;
change this to enforce failure or an alternate verification: in the method
containing the refreshToken check (GoogleSheetPermissionService), replace the
early return with either (A) throw a validation/authorization exception when
attempting an integrated sheet registration and no user OAuth refreshToken is
present, or (B) implement an alternative validation path that, when refreshToken
is null, uses the service account to explicitly call Drive API to confirm the
requesterId is an editor/owner of the sheet (or requires the requester to prove
ownership) before accepting the sheet; ensure the change references the
refreshToken branch and the requesterId check so registrations cannot bypass
access validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a0453844-01c4-45aa-b513-5c0fe99e550b
📒 Files selected for processing (2)
src/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.javasrc/test/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionServiceTest.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
src/main/java/**/*.java: 아래 원칙으로 리뷰 코멘트를 작성한다.
- 코멘트는 반드시 한국어로 작성한다.
- 반드시 수정이 필요한 항목만 코멘트로 남기고, 단순 취향 차이는 지적하지 않는다.
- 각 코멘트 첫 줄에 심각도를
[LEVEL: high|medium|low]형식으로 반드시 표기한다.- 심각도 기준: high=운영 장애 가능, medium=품질 저하, low=개선 권고.
- 각 코멘트는 "문제 -> 영향 -> 제안" 순서로 3문장 이내로 간결하게 작성한다.
- 가능하면 재현 조건 및 실패 시나리오도 포함한다.
- 제안은 현재 코드베이스(Spring Boot + JPA + Flyway) 패턴과 일치해야 한다.
- 보안, 트랜잭션 경계, 예외 처리, N+1, 성능 회귀 가능성을 우선 점검한다.
- 가독성: 변수/메서드 이름이 의도를 바로 드러내는지, 중첩과 메서드 길이가 과도하지 않은지 점검한다.
- 단순화: 불필요한 추상화, 중복 로직, 과한 방어 코드가 있으면 더 단순한 대안을 제시한다.
- 확장성: 새 요구사항 추가 시 변경 범위가 최소화되는 구조인지(하드코딩 분기/값 여부 포함) 점검한다.
Files:
src/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.java
**/*
⚙️ CodeRabbit configuration file
**/*: 공통 리뷰 톤 가이드:
- 모든 코멘트는 첫 줄에
[LEVEL: ...]태그를 포함한다.- 과장된 표현 없이 사실 기반으로 작성한다.
- 한 코멘트에는 하나의 이슈만 다룬다.
- 코드 예시가 필요하면 최소 수정 예시를 제시한다.
- 가독성/단순화/확장성 이슈를 발견하면 우선순위를 높여 코멘트한다.
Files:
src/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.javasrc/test/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionServiceTest.java
src/main/java/gg/agit/konect/domain/club/service/GoogleSheetPermissionService.java
Outdated
Show resolved
Hide resolved
| googleSheetPermissionService.validateRequesterAccessAndTryGrantServiceAccountWriterAccess( | ||
| requesterId, | ||
| spreadsheetId | ||
| ); |
There was a problem hiding this comment.
ClubSheetIntegratedService의 주석(“OAuth 미연결이면 … 건너뛴다”) 및 PR 설명(Drive OAuth 미연결 사용자 흐름 유지)과 달리, 현재는 validateRequesterAccessAndTryGrantServiceAccountWriterAccess()를 무조건 호출해서 OAuth 계정/refresh token이 없으면 NOT_FOUND_GOOGLE_DRIVE_AUTH 예외로 흐름이 중단됩니다(기존에는 tryGrant...가 false 반환 후 계속 진행). OAuth 미연결 시에는 기존처럼 검증/권한부여를 스킵하고 다음 단계로 진행하도록 분기(예: refresh token 존재 시에만 validate 메서드 호출, 또는 validate 메서드 내부에서 token 없으면 no-op) 중 하나로 정합성을 맞춰주세요.
| googleSheetPermissionService.validateRequesterAccessAndTryGrantServiceAccountWriterAccess( | |
| requesterId, | |
| spreadsheetId | |
| ); | |
| try { | |
| googleSheetPermissionService.validateRequesterAccessAndTryGrantServiceAccountWriterAccess( | |
| requesterId, | |
| spreadsheetId | |
| ); | |
| } catch (RuntimeException e) { | |
| // OAuth 미연결(예: NOT_FOUND_GOOGLE_DRIVE_AUTH)인 경우에는 검증/권한부여를 스킵하고 | |
| // 기존 동작대로 이후 흐름(시트 분석 및 임포트)을 계속 진행한다. | |
| String message = e.getMessage(); | |
| if (message == null || !message.contains("NOT_FOUND_GOOGLE_DRIVE_AUTH")) { | |
| // 다른 예외는 기존과 동일하게 전파한다. | |
| throw e; | |
| } | |
| } |
| public void validateRequesterAccessAndTryGrantServiceAccountWriterAccess( | ||
| Integer requesterId, | ||
| String spreadsheetId | ||
| ) { | ||
| String refreshToken = requireRefreshToken(requesterId); | ||
| Drive userDriveService = buildUserDriveService(refreshToken, requesterId); | ||
| validateRequesterSpreadsheetAccess(userDriveService, requesterId, spreadsheetId); | ||
| boolean granted = tryGrantServiceAccountWriterAccess(userDriveService, requesterId, spreadsheetId); | ||
| if (!granted) { | ||
| requireServiceAccountSpreadsheetAccess(spreadsheetId, requesterId); | ||
| } |
There was a problem hiding this comment.
validateRequesterAccessAndTryGrantServiceAccountWriterAccess()가 requireRefreshToken()을 사용해 OAuth 미연결/blank token을 곧바로 NOT_FOUND_GOOGLE_DRIVE_AUTH로 예외 처리하고 있습니다. 그런데 동일 PR에서 “OAuth 미연결이면 기존 동작 유지(스킵)”를 명시했고, ClubSheetIntegratedService 주석도 스킵을 기대하고 있어 실제 런타임 동작이 설명과 불일치합니다. 이 메서드를 (1) refresh token이 없으면 조용히 return 하도록 변경하거나, (2) ‘반드시 OAuth 연결이 필요’한 API에서만 호출되도록 호출부를 조정하는 등 의도에 맞게 계약을 명확히 해주세요.
| @Test | ||
| @DisplayName("Drive OAuth 계정이 없으면 요청자 접근 검증을 거부한다") | ||
| void validateRequesterAccessAndTryGrantServiceAccountWriterAccessThrowsWhenOAuthAccountIsMissing() | ||
| throws IOException, GeneralSecurityException { | ||
| given(userOAuthAccountRepository.findByUserIdAndProvider(REQUESTER_ID, Provider.GOOGLE)) | ||
| .willReturn(Optional.empty()); | ||
|
|
||
| assertThatThrownBy(() -> | ||
| googleSheetPermissionService.validateRequesterAccessAndTryGrantServiceAccountWriterAccess( | ||
| REQUESTER_ID, | ||
| FILE_ID | ||
| ) | ||
| ) | ||
| .isInstanceOf(CustomException.class) | ||
| .extracting("errorCode") | ||
| .isEqualTo(ApiResponseCode.NOT_FOUND_GOOGLE_DRIVE_AUTH); | ||
| verify(userDriveService, never()).files(); | ||
| verify(userDriveService, never()).permissions(); | ||
| verify(googleSheetsConfig, never()).buildUserDriveService(any()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Drive OAuth refresh token이 비어 있으면 요청자 접근 검증을 거부한다") | ||
| void validateRequesterAccessAndTryGrantServiceAccountWriterAccessThrowsWhenRefreshTokenIsBlank() | ||
| throws IOException, GeneralSecurityException { | ||
| given(userOAuthAccountRepository.findByUserIdAndProvider(REQUESTER_ID, Provider.GOOGLE)) | ||
| .willReturn(Optional.of(userOAuthAccount)); | ||
| given(userOAuthAccount.getGoogleDriveRefreshToken()).willReturn(" "); | ||
|
|
||
| assertThatThrownBy(() -> | ||
| googleSheetPermissionService.validateRequesterAccessAndTryGrantServiceAccountWriterAccess( | ||
| REQUESTER_ID, | ||
| FILE_ID | ||
| ) | ||
| ) | ||
| .isInstanceOf(CustomException.class) | ||
| .extracting("errorCode") | ||
| .isEqualTo(ApiResponseCode.NOT_FOUND_GOOGLE_DRIVE_AUTH); | ||
| verify(userDriveService, never()).files(); | ||
| verify(userDriveService, never()).permissions(); | ||
| verify(googleSheetsConfig, never()).buildUserDriveService(any()); | ||
| } |
There was a problem hiding this comment.
신규 테스트가 OAuth 계정/refresh token이 없을 때 validateRequesterAccessAndTryGrantServiceAccountWriterAccess()가 NOT_FOUND_GOOGLE_DRIVE_AUTH를 던지는 것을 요구하고 있는데, PR 설명에는 “Drive OAuth 미연결 사용자의 기존 흐름은 유지”라고 되어 있어 의도와 상충됩니다. 실제 제품 요구가 ‘미연결이면 스킵’이라면, 해당 테스트는 예외를 기대하기보다 no-op(후속 로직 진행) 또는 ClubSheetIntegratedService에서 validate 호출 자체를 생략하는 시나리오를 검증하도록 수정하는 편이 정합적입니다.
| // then | ||
| InOrder inOrder = inOrder( | ||
| clubPermissionValidator, | ||
| googleSheetPermissionService, | ||
| sheetHeaderMapper, | ||
| clubMemberSheetService, | ||
| sheetImportService | ||
| ); | ||
| inOrder.verify(clubPermissionValidator).validateManagerAccess(clubId, requesterId); | ||
| inOrder.verify(googleSheetPermissionService) | ||
| .tryGrantServiceAccountWriterAccess(requesterId, spreadsheetId); | ||
| .validateRequesterAccessAndTryGrantServiceAccountWriterAccess(requesterId, spreadsheetId); | ||
| inOrder.verify(sheetHeaderMapper).analyzeAllSheets(spreadsheetId); |
There was a problem hiding this comment.
ClubSheetIntegratedService에서 권한 검증 메서드 호출로 흐름이 바뀌었는데, ClubSheetIntegratedServiceTest에는 ‘Drive OAuth 미연결 사용자도 기존처럼 분석/가져오기가 계속 진행되는지’에 대한 케이스가 없습니다. OAuth 미연결(또는 refresh token blank) 시 validate 메서드가 no-op으로 설계된다면, 이 동작이 유지되는지 테스트를 추가해 회귀를 방지하는 것이 좋습니다.
| @Test | ||
| @DisplayName("Drive OAuth 계정이 없으면 요청자 접근 검증을 거부한다") | ||
| void validateRequesterAccessAndTryGrantServiceAccountWriterAccessThrowsWhenOAuthAccountIsMissing() | ||
| throws IOException, GeneralSecurityException { | ||
| given(userOAuthAccountRepository.findByUserIdAndProvider(REQUESTER_ID, Provider.GOOGLE)) | ||
| .willReturn(Optional.empty()); | ||
|
|
||
| assertThatThrownBy(() -> | ||
| googleSheetPermissionService.validateRequesterAccessAndTryGrantServiceAccountWriterAccess( | ||
| REQUESTER_ID, | ||
| FILE_ID | ||
| ) | ||
| ) | ||
| .isInstanceOf(CustomException.class) | ||
| .extracting("errorCode") | ||
| .isEqualTo(ApiResponseCode.NOT_FOUND_GOOGLE_DRIVE_AUTH); | ||
| verify(userDriveService, never()).files(); | ||
| verify(userDriveService, never()).permissions(); | ||
| verify(googleSheetsConfig, never()).buildUserDriveService(any()); |
There was a problem hiding this comment.
테스트가 validateRequesterAccessAndTryGrantServiceAccountWriterAccess()의 OAuth 미연결 시나리오를 NOT_FOUND_GOOGLE_DRIVE_AUTH 예외로 ‘거부’하는 동작으로 고정하고 있습니다. PR 설명에는 OAuth 미연결 사용자는 기존 흐름을 유지한다고 되어 있어, 의도가 스킵인지/거부인지가 불명확합니다. 의도가 스킵이라면 해당 테스트 기대값과 서비스 로직이 함께 조정되어야 합니다.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/gg/agit/konect/domain/club/service/ClubSheetIntegratedServiceTest.java (1)
100-154: 🧹 Nitpick | 🔵 Trivial[LEVEL: minor]
중복 테스트 케이스 검토 필요
analyzeAndImportPreMembersContinuesAfterPermissionValidation테스트가analyzeAndImportPreMembersSuccess테스트와 거의 동일합니다.기존에는
tryGrantServiceAccountWriterAccess가 boolean을 반환하여true(성공)/false(continue) 경로를 구분했을 것으로 보이지만, 현재validateRequesterAccessAndTryGrantServiceAccountWriterAccess는 void 메서드로 변경되어:
- 예외 없이 정상 반환 → 후속 작업 진행 (두 테스트 모두 이 경로)
- 예외 발생 → 중단 (별도 예외 테스트에서 커버)
두 테스트 간 유의미한 차이가 없어 하나로 통합하거나, 의도된 차이점이 있다면 테스트 설정에 반영이 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/gg/agit/konect/domain/club/service/ClubSheetIntegratedServiceTest.java` around lines 100 - 154, The two tests analyzeAndImportPreMembersContinuesAfterPermissionValidation and analyzeAndImportPreMembersSuccess are effectively identical because googleSheetPermissionService.validateRequesterAccessAndTryGrantServiceAccountWriterAccess is now void; either merge them into a single test that verifies the normal (no-exception) flow, or make the second test exercise a distinct behavior (e.g., simulate the permission method throwing an exception and assert exception handling) by configuring the mock for validateRequesterAccessAndTryGrantServiceAccountWriterAccess accordingly; update or remove the redundant test so only one verifies the successful path (keeping the inOrder verifications and assertions around analyzeAndImportPreMembers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/test/java/gg/agit/konect/domain/club/service/ClubSheetIntegratedServiceTest.java`:
- Around line 100-154: The two tests
analyzeAndImportPreMembersContinuesAfterPermissionValidation and
analyzeAndImportPreMembersSuccess are effectively identical because
googleSheetPermissionService.validateRequesterAccessAndTryGrantServiceAccountWriterAccess
is now void; either merge them into a single test that verifies the normal
(no-exception) flow, or make the second test exercise a distinct behavior (e.g.,
simulate the permission method throwing an exception and assert exception
handling) by configuring the mock for
validateRequesterAccessAndTryGrantServiceAccountWriterAccess accordingly; update
or remove the redundant test so only one verifies the successful path (keeping
the inOrder verifications and assertions around analyzeAndImportPreMembers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ec665a9-dc5d-49ef-aeb1-3286cc0a1507
📒 Files selected for processing (1)
src/test/java/gg/agit/konect/domain/club/service/ClubSheetIntegratedServiceTest.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: 공통 리뷰 톤 가이드:
- 모든 코멘트는 첫 줄에
[LEVEL: ...]태그를 포함한다.- 과장된 표현 없이 사실 기반으로 작성한다.
- 한 코멘트에는 하나의 이슈만 다룬다.
- 코드 예시가 필요하면 최소 수정 예시를 제시한다.
- 가독성/단순화/확장성 이슈를 발견하면 우선순위를 높여 코멘트한다.
Files:
src/test/java/gg/agit/konect/domain/club/service/ClubSheetIntegratedServiceTest.java
🔇 Additional comments (5)
src/test/java/gg/agit/konect/domain/club/service/ClubSheetIntegratedServiceTest.java (5)
6-6: [LEVEL: approve]void 메서드 스터빙을 위한
willThrowimport 추가가 적절합니다.BDDMockito에서 void 메서드에 예외를 설정할 때
willThrow(...).given(...)패턴이 필요하며, 이를 위한 import입니다.
82-83: [LEVEL: approve]새로운 메서드명으로 검증 업데이트가 프로덕션 코드 변경과 일치합니다.
tryGrantServiceAccountWriterAccess→validateRequesterAccessAndTryGrantServiceAccountWriterAccess변경이ClubSheetIntegratedService의 실제 호출과 동기화되었습니다.
167-177: [LEVEL: approve]void 메서드 스터빙 패턴과 예외 검증이 적절합니다.
willThrow(...).given(...)패턴: void 메서드에 대한 BDDMockito 표준 방식.isSameAs(expected): 동일한 예외 인스턴스 검증으로 정확한 예외 전파 확인verifyNoInteractions(...): 예외 발생 시 후속 서비스가 호출되지 않음을 보장
180-202: [LEVEL: approve]요청자 시트 접근 권한 검증 실패 시나리오 테스트 추가
PR 목표인 "요청자 계정에 시트 접근 권한이 없을 경우
FORBIDDEN_GOOGLE_SHEET_ACCESS로 실패" 시나리오가 적절히 커버되었습니다.verifyNoInteractions로 후속 작업이 진행되지 않음을 검증합니다.
204-226: [LEVEL: approve]Drive OAuth 미연결 시나리오 테스트가 적절하게 검증됩니다. OAuth 미연결 시
NOT_FOUND_GOOGLE_DRIVE_AUTH예외가 발생하고 후속 작업(sheetHeaderMapper,clubMemberSheetService,sheetImportService)이 진행되지 않음을 확인합니다.
| // OAuth 미연결이면 기존 동작대로 검증을 건너뛴다. | ||
| // 다만 Drive OAuth가 연결된 경우에는 요청자 계정의 실제 시트 접근 권한을 먼저 검증한다. | ||
| googleSheetPermissionService.validateRequesterAccessAndTryGrantServiceAccountWriterAccess( | ||
| requesterId, | ||
| spreadsheetId | ||
| ); |
There was a problem hiding this comment.
ClubSheetIntegratedService의 주석이 현재 동작과 불일치합니다. 이제 analyzeAndImportPreMembers()는 OAuth 미연결 시 검증을 ‘건너뛰는’ 것이 아니라 validateRequesterAccess...에서 NOT_FOUND_GOOGLE_DRIVE_AUTH 예외로 즉시 실패합니다. 주석을 실제 정책(미연결 시 실패/재연결 요구)로 수정하거나, 정말로 우회 허용이 목적이라면 호출 로직을 변경해야 합니다.
🔍 개요
🚀 주요 변경 내용
GoogleSheetPermissionService에 요청자 Drive OAuth 기준 시트 접근 권한을 먼저 검증하는 로직을 추가했습니다.NOT_FOUND_GOOGLE_DRIVE_AUTH로 즉시 실패하도록 변경했습니다.supportsAllDrives설정을 추가했습니다.💬 참고 사항
tryGrantServiceAccountWriterAccess()메서드는 다른 호출 가능성을 고려해 유지하고 있으며, integrated 등록은 더 엄격한validateRequesterAccessAndTryGrantServiceAccountWriterAccess()흐름을 사용합니다.✅ Checklist (완료 조건)