Draft
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors message access permission checks by moving them from middleware to the service layer. The goal is to improve separation of concerns by removing domain logic from the router layer.
Key changes:
- Adds
IsAccessiblemethod to the message service interface and implementation - Removes
CheckMessageAccessPermmiddleware function - Adds access checks directly to message-related handlers
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| service/message/manager.go | Adds interface definition for IsAccessible method |
| service/message/manager_impl.go | Implements message access checking via channel access verification |
| router/v3/router.go | Removes message access permission middleware from route setup |
| router/v3/messages.go | Adds checkMessageAccess helper and access checks to all message handlers |
| router/middlewares/access_control.go | Removes CheckMessageAccessPerm middleware function |
| return false, fmt.Errorf("failed to check channel access: %w", err) | ||
| } | ||
|
|
||
| return accessible, nil |
There was a problem hiding this comment.
[nitpick] The function name IsAccessible is ambiguous as it doesn't specify what type of access is being checked. Consider renaming to IsAccessibleToUser or adding more specific documentation about what access permissions are being validated.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
タイトルの通り
責務の分離を進めた
なぜこの PR を入れたいのか
traQ の middleware では多数の処理が行われている
/router/middlewares/param_retriever.goのように、単に params から構造体を取り出すだけであれば問題がないしかし、アクセス権限の確認などは domain / service に関わる内容であり、これは router 層に置くべきではないと考えた
特に、現状の設計は ConnectRPC での api handler 実装を行う際にも障壁となりうるものであると考えている
最終的には
/router/middlewares/access_control.goをファイルごと削除したい動作確認の手順
テストが通ってるので大丈夫だと思います
PR を出す前の確認事項
見てほしいところ・聞きたいことなど
この方針で refactor を進めても大丈夫かをご確認したいです