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

refactor/#156 api 인가처리 #161

Merged
merged 9 commits into from
Jan 6, 2025
Merged

Conversation

LeeShinHaeng
Copy link
Contributor

@LeeShinHaeng LeeShinHaeng commented Jan 4, 2025

Summary

관리자 API 인가 처리 및 퍼블릭 API permitAll

Tasks

  • 로그인 하지 않은 유저가 접근할 수 있는 엔드포인트 → PUBLIC_ENDPOINTS 에 추가
  • 관리자 유저가 접근하는 엔드포인트 → 현재 클래스가 분리되어 있기 때문에 클래스 레벨에 PreAuthorize 추가

To Reviewer

현재는 로그인 시 ROLE_USER 임의 설정하고 있지만, 관리자로 로그인 시 ADMIN 권한을 주도록 변경해야합니다. (TokenAuthenticationFilter#doFilterInternal -> TokenProvider#getAuthentication 참고)

Screen Shot

일반 사용자로 로그인 후 Post Create
스크린샷 2025-01-06 10 02 26


Admin 사용자 DB에 생성
스크린샷 2025-01-06 10 05 40

Admin 사용자로 로그인 후 Post Create
스크린샷 2025-01-06 10 02 34

@LeeShinHaeng LeeShinHaeng added the 🔨refactor refactoring code label Jan 4, 2025
@LeeShinHaeng LeeShinHaeng requested a review from a team January 4, 2025 00:58
@LeeShinHaeng LeeShinHaeng self-assigned this Jan 4, 2025
@LeeShinHaeng LeeShinHaeng linked an issue Jan 4, 2025 that may be closed by this pull request
1 task
Copy link
Contributor

coderabbitai bot commented Jan 4, 2025

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

워크스루

이 풀 리퀘스트는 여러 관리자 컨트롤러에 @PreAuthorize("hasRole('ROLE_ADMIN')") 어노테이션을 추가하여 API 엔드포인트에 대한 접근 제어를 강화합니다. 동시에 SecurityConfigPUBLIC_ENDPOINTS 배열을 수정하여 일부 엔드포인트에 대한 공개 접근을 허용합니다. 이러한 변경은 애플리케이션의 보안 메커니즘을 개선하는 데 중점을 둡니다.

변경 사항

파일 경로 변경 요약
aics-admin/src/.../AboutAdminController.java @PreAuthorize("hasRole('ROLE_ADMIN')") 어노테이션 추가
aics-admin/src/.../ClubAdminController.java @PreAuthorize("hasRole('ROLE_ADMIN'))` 어노테이션 추가
aics-admin/src/.../CommentAdminController.java @PreAuthorize("hasRole('ROLE_ADMIN')")) 어노테이션 추가
aics-admin/src/.../FileAdminController.java @PreAuthorize("hasRole('ROLE_ADMIN')")) 어노테이션 추가
aics-admin/src/.../LabAdminController.java @PreAuthorize("hasRole('ROLE_ADMIN')")) 어노테이션 추가
aics-admin/src/.../PostAdminController.java @PreAuthorize("hasRole('ROLE_ADMIN')")) 어노테이션 추가
aics-admin/src/.../ProfessorAdminController.java @PreAuthorize("hasRole('ROLE_ADMIN')")) 어노테이션 추가
aics-admin/src/.../UserAdminController.java @PreAuthorize("hasRole('ROLE_ADMIN')")) 어노테이션 추가
aics-common/src/.../SecurityConfig.java PUBLIC_ENDPOINTS 배열에 새로운 엔드포인트 추가

연결된 이슈에 대한 평가

목표 해결 여부 설명
API 인가 처리 [#156]
Spring Security의 @PreAuthorize 사용 [#156]
공개 API 엔드포인트 설정 [#156]

관련 가능성 있는 PR

  • 현재 관련된 PR 없음

추천 리뷰어

  • LeeHanEum

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@minjo-on minjo-on left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다! 😊 현재 USER 권한으로 관리자(Admin 전용) 엔드포인트에 접근할 경우 500 Internal Server Error가 발생하고 있다고 하셨는데요, 이 상황에서는 403 Forbidden을 반환하도록 수정하는 것이 어떨까 제안드립니다.


제안 이유

  1. HTTP 표준 준수: 권한 부족 상황에는 403 Forbidden이 적합하며, 500 에러는 서버 내부 문제를 의미하기 때문에 상황에 맞지 않습니다.
  2. 보안 강화: 500 에러는 서버 내부 오류로 오해를 살 수 있어, 잠재적 공격자에게 불필요한 힌트를 줄 가능성이 있습니다.
  3. 사용자 경험 개선: 클라이언트가 요청 실패 이유를 명확히 알 수 있도록, 권한 부족임을 403 상태 코드로 전달하는 것이 바람직합니다.
  4. 디버깅 효율성: 500 에러는 서버 측 문제로 간주되어 불필요한 디버깅을 유발할 수 있습니다. 반면 403 Forbidden은 권한 문제를 명확히 표현합니다.

권한 부족 상황에서 403 Forbidden이 반환될 수 있도록 Spring Security 설정과 예외 처리 로직을 점검해주시면 좋을 것 같습니다!

@LeeShinHaeng
Copy link
Contributor Author

LeeShinHaeng commented Jan 5, 2025

고생 많으셨습니다! 😊 현재 USER 권한으로 관리자(Admin 전용) 엔드포인트에 접근할 경우 500 Internal Server Error가 발생하고 있다고 하셨는데요, 이 상황에서는 403 Forbidden을 반환하도록 수정하는 것이 어떨까 제안드립니다.

제안 이유

  1. HTTP 표준 준수: 권한 부족 상황에는 403 Forbidden이 적합하며, 500 에러는 서버 내부 문제를 의미하기 때문에 상황에 맞지 않습니다.
  2. 보안 강화: 500 에러는 서버 내부 오류로 오해를 살 수 있어, 잠재적 공격자에게 불필요한 힌트를 줄 가능성이 있습니다.
  3. 사용자 경험 개선: 클라이언트가 요청 실패 이유를 명확히 알 수 있도록, 권한 부족임을 403 상태 코드로 전달하는 것이 바람직합니다.
  4. 디버깅 효율성: 500 에러는 서버 측 문제로 간주되어 불필요한 디버깅을 유발할 수 있습니다. 반면 403 Forbidden은 권한 문제를 명확히 표현합니다.

권한 부족 상황에서 403 Forbidden이 반환될 수 있도록 Spring Security 설정과 예외 처리 로직을 점검해주시면 좋을 것 같습니다!

민준님 말씀대로 403 에러를 반환하도록 수정하는 것이 더 명확하고 우수할것으로 생각됩니다.
좋은 의견 감사합니다 👍
edc07d9 에서 반영 완료했습니다!

@LeeShinHaeng LeeShinHaeng requested a review from a team January 5, 2025 02:55
Copy link
Member

@LeeHanEum LeeHanEum left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다 👍
깔끔한 코드 보기 좋았고, 고민한 흔적도 많이 보이네요.

추가적으로 @LeeShinHaeng 님이 PR에 작성해주신 것처럼 현재는 로그인 시 ROLE_USER 임의 설정하고 있지만, 관리자로 로그인 시 ADMIN 권한을 주도록 변경해야합니다. 이 작업도 진행해주실 수 있을까요? 이번 PR에서 처리하는게 좋아보이네요~

Comment on lines 12 to 19
@RestControllerAdvice
public class AdminAuthorizeExceptionHandler {
@ExceptionHandler(AccessDeniedException.class)
public ResponseEntity<ExceptionResponse> handleAccessDeniedException() {
ExceptionResponse response = ExceptionResponse.from(NOT_ADMIN);
return ResponseEntity.status(FORBIDDEN).body(response);
}
}
Copy link
Member

@LeeHanEum LeeHanEum Jan 5, 2025

Choose a reason for hiding this comment

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

AdminAuthorizeExceptionHandlerGlobalExceptionHandler에 포함시키지 않고 분리한 이유가 있나요?
가능하다면 예외 핸들링 처리는 하나의 클래스에서 수행하는 것이, 예외 처리 로직 전반을 중앙에서 관리하고, 가독성을 향상시키는데 도움이 될 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Admin 접근시에만 사용하는 ExceptionHandler여서 분리했습니다.
하지만 말씀대로 하나의 클래스에서 관리하는게 가독성 측면에서 나을 것 같습니다!
93aea42 에서 반영했습니다

@LeeShinHaeng
Copy link
Contributor Author

수고 많으셨습니다 👍 깔끔한 코드 보기 좋았고, 고민한 흔적도 많이 보이네요.

추가적으로 @LeeShinHaeng 님이 PR에 작성해주신 것처럼 현재는 로그인 시 ROLE_USER 임의 설정하고 있지만, 관리자로 로그인 시 ADMIN 권한을 주도록 변경해야합니다. 이 작업도 진행해주실 수 있을까요? 이번 PR에서 처리하는게 좋아보이네요~

b623440 에서 반영 완료했습니다!

Copy link
Contributor

@minjo-on minjo-on left a comment

Choose a reason for hiding this comment

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

LGTM😁

@LeeShinHaeng LeeShinHaeng merged commit 63e5fc9 into develop Jan 6, 2025
2 of 3 checks passed
@LeeShinHaeng LeeShinHaeng deleted the refactor/#156-api-authorize branch January 6, 2025 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨refactor refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API 인가처리
3 participants