-
Notifications
You must be signed in to change notification settings - Fork 1
fix: 구글 시트 403 응답 처리 및 권한 부여 보강 #478
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
Changes from all commits
79edea7
2f721bc
9b19f74
f91cf65
2c0b4ba
a9c0b31
7ffd52a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| package gg.agit.konect.domain.club.event; | ||
|
|
||
| import java.time.LocalDateTime; | ||
|
|
||
| public record SheetSyncFailedEvent( | ||
| Integer clubId, | ||
| String spreadsheetId, | ||
| boolean accessDenied, | ||
| String reason, | ||
| LocalDateTime occurredAt | ||
| ) { | ||
|
|
||
| public static SheetSyncFailedEvent accessDenied( | ||
| Integer clubId, | ||
| String spreadsheetId, | ||
| String reason | ||
| ) { | ||
| return new SheetSyncFailedEvent(clubId, spreadsheetId, true, reason, LocalDateTime.now()); | ||
| } | ||
|
|
||
| public static SheetSyncFailedEvent unexpected( | ||
| Integer clubId, | ||
| String spreadsheetId, | ||
| String reason | ||
| ) { | ||
| return new SheetSyncFailedEvent(clubId, spreadsheetId, false, reason, LocalDateTime.now()); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |||||||||
| public class ClubSheetIntegratedService { | ||||||||||
|
|
||||||||||
| private final ClubPermissionValidator clubPermissionValidator; | ||||||||||
| private final GoogleSheetPermissionService googleSheetPermissionService; | ||||||||||
| private final SheetHeaderMapper sheetHeaderMapper; | ||||||||||
| private final ClubMemberSheetService clubMemberSheetService; | ||||||||||
| private final SheetImportService sheetImportService; | ||||||||||
|
|
@@ -22,6 +23,9 @@ public SheetImportResponse analyzeAndImportPreMembers( | |||||||||
| clubPermissionValidator.validateManagerAccess(clubId, requesterId); | ||||||||||
|
|
||||||||||
| String spreadsheetId = SpreadsheetUrlParser.extractId(spreadsheetUrl); | ||||||||||
| // Best-effort: OAuth 미연결/권한 부여 실패여도 이미 수동 공유된 시트는 그대로 읽을 수 있다. | ||||||||||
|
||||||||||
| // Best-effort: OAuth 미연결/권한 부여 실패여도 이미 수동 공유된 시트는 그대로 읽을 수 있다. | |
| // Best-effort: 서비스 계정에 쓰기 권한 부여를 시도한다. OAuth 미연결/일반적인 권한 부여 실패가 있어도, | |
| // 이미 수동 공유된 시트는 그대로 읽을 수 있다. 단, Drive 서비스 초기화 실패나 예기치 못한 I/O 오류 등에서는 | |
| // 예외가 전파되어 import 흐름이 중단될 수 있다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LEVEL: medium] tryGrantServiceAccountWriterAccess의 반환값이 무시됩니다.
문제: 메서드가 false를 반환해도 (OAuth 미연결, 권한 부여 실패) 후속 작업을 계속 진행합니다. 영향: 사용자는 "OAuth 미연결" 대신 "스프레드시트 접근 권한 없음"이라는 덜 명확한 오류 메시지를 받게 됩니다.
제안: 의도적인 "best-effort" 설계라면 주석으로 명시하고, 그렇지 않다면 false 반환 시 적절한 예외를 던지도록 수정하세요.
♻️ 명시적 처리 예시
String spreadsheetId = SpreadsheetUrlParser.extractId(spreadsheetUrl);
- googleSheetPermissionService.tryGrantServiceAccountWriterAccess(requesterId, spreadsheetId);
+ // best-effort: 권한 부여 실패 시에도 기존 권한으로 시도
+ googleSheetPermissionService.tryGrantServiceAccountWriterAccess(requesterId, spreadsheetId);또는 실패 시 명확한 에러 반환:
String spreadsheetId = SpreadsheetUrlParser.extractId(spreadsheetUrl);
- googleSheetPermissionService.tryGrantServiceAccountWriterAccess(requesterId, spreadsheetId);
+ boolean granted = googleSheetPermissionService.tryGrantServiceAccountWriterAccess(requesterId, spreadsheetId);
+ if (!granted) {
+ throw CustomException.of(ApiResponseCode.NOT_FOUND_GOOGLE_DRIVE_AUTH);
+ }🤖 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/ClubSheetIntegratedService.java`
around lines 26 - 27, The call to
googleSheetPermissionService.tryGrantServiceAccountWriterAccess(requesterId,
spreadsheetId) in ClubSheetIntegratedService currently ignores its boolean
return value; update the code to handle false explicitly: either document with a
clear comment that this is an intentional best-effort and proceed, or
(preferred) check the returned boolean and throw an appropriate exception (e.g.,
PermissionDenied or a custom ClearSpreadsheetAccessException) when it returns
false so downstream code doesn't continue on failed grant; ensure the check
references tryGrantServiceAccountWriterAccess, requesterId and spreadsheetId so
reviewers can find the exact call to modify.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| package gg.agit.konect.domain.club.service; | ||
|
|
||
| final class GoogleDrivePermissionHelper { | ||
|
|
||
| private static final int ROLE_RANK_NONE = 0; | ||
| private static final int ROLE_RANK_READER = 1; | ||
| private static final int ROLE_RANK_COMMENTER = 2; | ||
| private static final int ROLE_RANK_WRITER = 3; | ||
|
|
||
| private GoogleDrivePermissionHelper() {} | ||
|
|
||
| enum PermissionApplyStatus { | ||
| CREATED, | ||
| UPGRADED, | ||
| UNCHANGED | ||
| } | ||
|
|
||
| static boolean hasRequiredRole(String currentRole, String targetRole) { | ||
| return roleRank(currentRole) >= roleRank(targetRole); | ||
| } | ||
|
|
||
| private static int roleRank(String role) { | ||
| if (role == null) { | ||
| return ROLE_RANK_NONE; | ||
| } | ||
|
|
||
| return switch (role) { | ||
| case "reader" -> ROLE_RANK_READER; | ||
| case "commenter" -> ROLE_RANK_COMMENTER; | ||
| case "writer", "fileOrganizer", "organizer", "owner" -> ROLE_RANK_WRITER; | ||
| default -> ROLE_RANK_NONE; | ||
| }; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| package gg.agit.konect.domain.club.service; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.List; | ||
| import java.util.Set; | ||
|
|
||
| import com.google.api.client.googleapis.json.GoogleJsonError; | ||
| import com.google.api.client.googleapis.json.GoogleJsonResponseException; | ||
| import com.google.api.client.http.HttpResponseException; | ||
|
|
||
| import gg.agit.konect.global.code.ApiResponseCode; | ||
| import gg.agit.konect.global.exception.CustomException; | ||
|
|
||
| public final class GoogleSheetApiExceptionHelper { | ||
|
|
||
| private static final Set<String> ACCESS_DENIED_REASONS = Set.of( | ||
| "accessDenied", | ||
| "forbidden", | ||
| "insufficientFilePermissions", | ||
| "insufficientPermissions", | ||
| "notAuthorized", | ||
| "required" | ||
| ); | ||
| private static final Set<String> AUTH_FAILURE_REASONS = Set.of( | ||
| "authError", | ||
| "invalidCredentials", | ||
| "unauthorized" | ||
| ); | ||
| private static final int HTTP_STATUS_FORBIDDEN = 403; | ||
| private static final int HTTP_STATUS_UNAUTHORIZED = 401; | ||
| private static final int HTTP_STATUS_NOT_FOUND = 404; | ||
|
|
||
| private GoogleSheetApiExceptionHelper() {} | ||
|
|
||
| public static boolean isAccessDenied(IOException exception) { | ||
| if (exception instanceof GoogleJsonResponseException responseException) { | ||
| if (responseException.getStatusCode() != HTTP_STATUS_FORBIDDEN) { | ||
| return false; | ||
| } | ||
| return hasReason(responseException, ACCESS_DENIED_REASONS); | ||
| } | ||
| return getStatusCode(exception) == HTTP_STATUS_FORBIDDEN; | ||
| } | ||
|
|
||
| public static boolean isAuthFailure(IOException exception) { | ||
| if (exception instanceof GoogleJsonResponseException responseException) { | ||
| if (responseException.getStatusCode() != HTTP_STATUS_UNAUTHORIZED) { | ||
| return false; | ||
| } | ||
| return hasReason(responseException, AUTH_FAILURE_REASONS) | ||
| || !hasKnownReasons(responseException); | ||
| } | ||
| return getStatusCode(exception) == HTTP_STATUS_UNAUTHORIZED; | ||
| } | ||
|
|
||
| public static boolean isNotFound(IOException exception) { | ||
| return getStatusCode(exception) == HTTP_STATUS_NOT_FOUND; | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| public static CustomException accessDenied() { | ||
| return CustomException.of(ApiResponseCode.FORBIDDEN_GOOGLE_SHEET_ACCESS); | ||
| } | ||
|
|
||
| private static boolean hasReason( | ||
| GoogleJsonResponseException exception, | ||
| Set<String> expectedReasons | ||
| ) { | ||
| return getReasons(exception).stream() | ||
| .anyMatch(expectedReasons::contains); | ||
| } | ||
|
|
||
| private static boolean hasKnownReasons(GoogleJsonResponseException exception) { | ||
| return !getReasons(exception).isEmpty(); | ||
| } | ||
|
|
||
| private static List<String> getReasons(GoogleJsonResponseException exception) { | ||
| if (exception.getDetails() == null || exception.getDetails().getErrors() == null) { | ||
| return List.of(); | ||
| } | ||
| return exception.getDetails().getErrors().stream() | ||
| .map(GoogleJsonError.ErrorInfo::getReason) | ||
| .filter(reason -> reason != null && !reason.isBlank()) | ||
| .toList(); | ||
| } | ||
|
Comment on lines
+35
to
+84
|
||
|
|
||
| private static int getStatusCode(IOException exception) { | ||
| if (exception instanceof GoogleJsonResponseException responseException) { | ||
| return responseException.getStatusCode(); | ||
| } | ||
| if (exception instanceof HttpResponseException responseException) { | ||
| return responseException.getStatusCode(); | ||
| } | ||
| return -1; | ||
|
Comment on lines
+86
to
+93
|
||
| } | ||
|
Comment on lines
+86
to
+94
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: BCSDLab/KONECT_BACK_END
Length of output: 4066
[LEVEL: medium] SheetSyncFailedEvent를 처리하는 리스너가 없습니다
문제:
SheetSyncExecutor에서SheetSyncFailedEvent를 발행하지만, 이벤트를 처리하는@EventListener또는@TransactionalEventListener가 코드베이스에 존재하지 않습니다.영향: 시트 동기화 실패 시 이벤트가 발행되어도 아무런 처리가 되지 않아, 사용자에게 오류를 알리거나 상태를 기록할 메커니즘이 동작하지 않습니다.
제안: 이 이벤트를 처리하는 리스너를 구현하여 실패 상태를 저장하거나 알림을 발송하세요.
🤖 Prompt for AI Agents