fix(apub/verify_blocked): check actor instead of id #70
Conversation
WalkthroughThe changes across multiple files involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant A as Actor
participant V as Verify Method
participant B as Blocked List
A->>V: Call verify()
V->>B: Call verify_blocked(self.actor())
B-->>V: Return verification result
V-->>A: Return verification status
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/apub/src/activities/like_or_announce/like_or_announce.rs (2)
52-56: Address TODO comment in theverifymethodThere's a TODO comment in the
verifymethod that should be addressed. It's important to ensure that all necessary verifications are implemented before considering this PR complete.Would you like me to help implement the missing verification logic or create a GitHub issue to track this task?
Line range hint
58-101: Consider refactoring thereceivemethod to reduce code duplicationThe
receivemethod contains similar code blocks for handling "Like" and "Announce" types. To improve maintainability and reduce code duplication, consider refactoring this method.Here's a suggested refactoring approach:
- Create a generic function to handle both "Like" and "Announce" cases.
- Use generics or trait objects to abstract the differences between
ReceivedLikeandReceivedAnnounce.Example pseudocode:
fn handle_activity<T: ActivityType>( actor: &str, object: &str, data: &Data<AppData>, create_apub: fn(&LikeOrAnnounce) -> Result<T, AppError>, ) -> Result<(), AppError> { if T::find() .filter( Condition::all() .add(T::Column::Actor.eq(actor)) .add(T::Column::Object.eq(object)), ) .one(&data.conn) .await? .is_none() { create_apub(&self)? .deref() .clone() .into_active_model() .insert(&data.conn) .await?; } Ok(()) } // In the receive method: match self.kind { LikeOrAnnounceType::LikeType(_) => { handle_activity::<ReceivedLike>(&actor.id, &object.id, data, ApubReceivedLike::from_json)?; }, LikeOrAnnounceType::AnnounceType(_) => { handle_activity::<ReceivedAnnounce>(&actor.id, &object.id, data, ApubReceivedAnnounce::from_json)?; }, }This refactoring would make the code more maintainable and easier to extend in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- crates/apub/src/activities/create_or_update/note.rs (1 hunks)
- crates/apub/src/activities/following/follow.rs (1 hunks)
- crates/apub/src/activities/following/undo_follow.rs (1 hunks)
- crates/apub/src/activities/like_or_announce/like_or_announce.rs (1 hunks)
- crates/apub/src/activities/like_or_announce/undo_like_or_announce.rs (1 hunks)
🔇 Additional comments (5)
crates/apub/src/activities/like_or_announce/undo_like_or_announce.rs (1)
42-42: LGTM! Consider addressing the TODO comment.The change from
self.idtoself.actor()aligns with the PR objective to check the actor instead of the id. This modification likely improves the accuracy of the user blocking verification.However, there are a couple of points to consider:
There's a TODO comment above this line. Are there any plans to address it in this PR or a future one?
It would be good to verify if similar changes have been made consistently across the codebase, especially in other implementations of the
ActivityHandlertrait.To ensure consistency across the codebase, let's check for other occurrences of
verify_blocked:✅ Verification successful
Verified all occurrences of
verify_blockeduseself.actor(). No inconsistencies found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of verify_blocked in the codebase # Expected result: All occurrences should use self.actor() instead of self.id rg --type rust -A 3 'verify_blocked'Length of output: 3948
crates/apub/src/activities/following/undo_follow.rs (1)
49-49: Approve the change and suggest consistency checkThe modification from
self.idtoself.actor()in theverify_blockedfunction call is a logical improvement. It correctly shifts the focus from checking if the activity is blocked to verifying if the actor performing the undo action is blocked.To ensure consistency, let's verify if similar changes have been applied to other activity handlers:
This will help us confirm that the change has been consistently applied across all relevant activity handlers.
✅ Verification successful
Verification Successful: Consistent use of
self.actor()inverify_blockedcallsAll
verify_blockedfunction calls across activity handlers consistently useself.actor(), confirming that the change aligns with PR objectives and maintains uniformity.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of `self.actor()` in `verify_blocked` calls across activity handlers. # Search for `verify_blocked` function calls in Rust files rg --type rust 'verify_blocked\(' -A 3 crates/apub/src/activitiesLength of output: 1585
crates/apub/src/activities/following/follow.rs (1)
64-64: Approve the change and verify consistency across the codebaseThe modification from
self.idtoself.actor()in theverify_blockedfunction call aligns with the PR objective to "check actor instead of id". This change improves the accuracy and security of the blocking mechanism by verifying the actor (follower) instead of the activity's ID.To ensure consistency, let's verify if similar changes have been applied in other files where blocking is checked:
This script will help us identify other locations where
verify_blockedis called and ensure that they also use theactor()method consistently.✅ Verification successful
Change is consistent across the codebase
All instances of
verify_blockedhave been updated to useself.actor()instead ofself.id. This change is consistently applied across the codebase, ensuring uniform behavior in the blocking verification mechanism.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of verify_blocked in the codebase # and ensure they use the actor() method instead of id. # Search for verify_blocked calls echo "Searching for verify_blocked calls:" rg --type rust "verify_blocked\(" -A 3 -B 1 # Search for ActivityHandler implementations echo "\nSearching for ActivityHandler implementations:" rg --type rust "impl ActivityHandler" -A 20Length of output: 13029
crates/apub/src/activities/like_or_announce/like_or_announce.rs (1)
54-54: Verify the impact of usingself.actor()instead ofself.idThe change from
self.idtoself.actor()in theverify_blockedfunction call aligns with the PR objective of checking the actor instead of the id. This modification potentially improves the accuracy of user blocking verification by using the actor's identity rather than the activity's id.However, we should consider the following points:
- Ensure that
self.actor()returns the expected type thatverify_blockedfunction accepts.- Verify that this change is consistent across all implementations of
ActivityHandlerin the codebase.- Consider updating the documentation of the
verify_blockedfunction to reflect this change in usage.To ensure consistency across the codebase, let's run the following script:
This script will help us verify if the change has been applied consistently across all relevant parts of the codebase.
crates/apub/src/activities/create_or_update/note.rs (1)
101-101: Approve change and suggest improvementsThe modification to use
self.actor()instead ofself.idin theverify_blockedcall is a good improvement. It aligns with the PR objective and makes more sense conceptually, as blocking is typically associated with actors rather than individual activities.This change is approved as it improves the blocking verification logic.
To ensure consistency across the codebase, please run the following script to check if similar changes are needed in other files:
Consider the following suggestions to further improve the code:
- Address the TODO comment at the beginning of the
verifymethod. If there are additional verification steps to be implemented, create a separate issue to track this task.- Update the documentation for this method (if it exists) to reflect the change in blocking verification logic.
Summary by CodeRabbit
New Features
Bug Fixes