[WEB-2718] chore: issue attachment deletion #5903
Conversation
WalkthroughThe changes in this pull request focus on enhancing asset management functionalities across multiple endpoints. Key modifications include the addition of a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 1
🧹 Outside diff range and nitpick comments (5)
apiserver/plane/space/views/asset.py (3)
Line range hint
174-203: Security concern: Missing creator verification in delete method.The delete method currently allows any authenticated user to delete assets, which doesn't align with the PR objective of enabling only issue creators to delete attachments.
Consider adding creator verification like this:
def delete(self, request, anchor, pk): # Get the deploy board deploy_board = DeployBoard.objects.filter( anchor=anchor, entity_name="project" ).first() # Check if the project is published if not deploy_board: return Response( {"error": "Project is not published"}, status=status.HTTP_404_NOT_FOUND, ) # Get the asset asset = FileAsset.objects.get( id=pk, workspace=deploy_board.workspace, project_id=deploy_board.project_id, ) + # Verify if the user is the creator + if asset.created_by != request.user: + return Response( + {"error": "Only the creator can delete this asset"}, + status=status.HTTP_403_FORBIDDEN, + ) # Check deleted assets asset.is_deleted = True asset.deleted_at = timezone.now() # Save the asset asset.save() return Response(status=status.HTTP_204_NO_CONTENT)
Line range hint
19-41: Consider enhancing permission model for better access control.The current permission model uses basic
AllowAnyfor GET andIsAuthenticatedfor other methods. Consider implementing a custom permission class to enforce creator-specific actions.Example custom permission class:
from rest_framework import permissions class IsCreatorOrReadOnly(permissions.BasePermission): def has_object_permission(self, request, view, obj): if request.method in permissions.SAFE_METHODS: return True return obj.created_by == request.userThen update the permission classes:
def get_permissions(self): if self.request.method == "GET": permission_classes = [ AllowAny, ] else: permission_classes = [ - IsAuthenticated, + IsAuthenticated, + IsCreatorOrReadOnly, ] return [permission() for permission in permission_classes]
Based on the gathered information, I can now provide a final response about the asset deletion behavior.
Asset deletion has inconsistent permission checks across different implementations
The codebase shows multiple implementations of asset deletion with varying levels of access control:
- In
space/views/asset.py(the file under review), deletion only checks for deploy board existence without verifying the asset creator- In
app/views/asset/v2.py, deletion properly checks user ownership withuser_id=request.user.id- In
app/views/asset/base.py, deletion verifies creator withcreated_by=request.userThis inconsistency could lead to unauthorized deletions in the space/deploy board context. The implementation should be aligned with the stricter creator-only check pattern used in other asset views.
Recommendations:
- Add creator verification check in
space/views/asset.pydelete method similar to other implementations- Consider implementing a common permission class for asset operations to ensure consistent access control
🔗 Analysis chain
Line range hint
1-286: Verify asset deletion behavior across the codebase.Let's verify how asset deletion is handled in related files to ensure consistent creator-only access control.
Let me gather more specific information about FileAsset model and its deletion behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for asset deletion implementations echo "Searching for asset deletion implementations..." rg -l "def delete.*FileAsset" --type py # Search for asset-related permission checks echo "Searching for asset-related permission checks..." rg "created_by.*request.user" --type py # Search for FileAsset model usage echo "Searching for FileAsset model usage..." ast-grep --pattern 'class $name(models.Model): $$$ created_by = $$$ $$$'Length of output: 6356
Script:
#!/bin/bash # Search for FileAsset model definition echo "Searching for FileAsset model definition..." rg -A 20 "class FileAsset\(" --type py # Search for asset deletion views and permissions echo "Searching for asset deletion views and permissions..." rg -A 10 "def delete.*asset" --type py # Search for asset-related permission classes echo "Searching for asset permission classes..." ast-grep --pattern 'class $name(BasePermission): $$$ def has_permission($$$): $$$'Length of output: 5983
apiserver/plane/app/views/asset/v2.py (2)
Line range hint
89-94: Enhance file type validationThe allowed file types list is duplicated across multiple methods. Consider:
- Moving it to a central configuration
- Expanding it to include more image formats (e.g., gif, svg)
- Adding MIME type validation beyond just checking the extension
# Add to settings.py or constants.py +ALLOWED_IMAGE_TYPES = [ + "image/jpeg", + "image/png", + "image/webp", + "image/jpg", + "image/gif", + "image/svg+xml" +] # In the view methods, replace the duplicated lists with: -allowed_types = ["image/jpeg", "image/png", "image/webp", "image/jpg"] +allowed_types = settings.ALLOWED_IMAGE_TYPESAlso applies to: 380-385, 644-649
Line range hint
86-94: Improve error handling and validation messagesThe error handling could be enhanced for better user experience:
- Provide more specific error messages including the actual vs allowed file size
- Add validation for empty files
- Maintain consistent error response structure
if type not in allowed_types: return Response( { - "error": "Invalid file type. Only JPEG and PNG files are allowed.", + "error": f"Invalid file type '{type}'. Allowed types: {', '.join(allowed_types)}", "status": False, }, status=status.HTTP_400_BAD_REQUEST, ) +if size <= 0: + return Response( + { + "error": f"File size must be greater than 0 bytes", + "status": False, + }, + status=status.HTTP_400_BAD_REQUEST, + ) if size > settings.FILE_SIZE_LIMIT: return Response( { + "error": f"File size {size} exceeds limit of {settings.FILE_SIZE_LIMIT} bytes", + "status": False, }, status=status.HTTP_400_BAD_REQUEST, )Also applies to: 377-385, 641-649
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- apiserver/plane/app/views/asset/v2.py (3 hunks)
- apiserver/plane/app/views/issue/attachment.py (1 hunks)
- apiserver/plane/space/views/asset.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
apiserver/plane/space/views/asset.py (1)
172-172: LGTM: Tracking user for asset updates.The addition of
created_byparameter improves user action tracking for asset updates.apiserver/plane/app/views/asset/v2.py (1)
212-212: 🛠️ Refactor suggestionConsider using
last_modified_byinstead ofcreated_byfor updatesThe changes track who modified the asset by setting
created_byduring updates. However, this might be misleading ascreated_bytypically indicates the original creator. Consider using a separatelast_modified_byfield to maintain this history without overwriting the original creator information.Let's verify if this pattern is consistent across the codebase:
Also applies to: 462-462, 693-693
| @@ -257,5 +257,5 @@ def patch(self, request, slug, project_id, issue_id, pk): | |||
| # Get the storage metadata | |||
| if not issue_attachment.storage_metadata: | |||
| get_asset_object_metadata.delay(str(issue_attachment.id)) | |||
| issue_attachment.save() | |||
| issue_attachment.save(created_by=request.user) | |||
There was a problem hiding this comment.
Fix improper use of 'created_by' parameter in 'save' method
The save method does not accept a created_by parameter unless it is explicitly overridden in the FileAsset model. To set the created_by field, assign it directly before calling save().
Apply this diff to correct the code:
- issue_attachment.save(created_by=request.user)
+ issue_attachment.created_by = request.user
+ issue_attachment.save()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| issue_attachment.save(created_by=request.user) | |
| issue_attachment.created_by = request.user | |
| issue_attachment.save() |
pushya22
changed the title
chore:
Issue Link: WEB-2718
Summary by CodeRabbit
New Features
Bug Fixes
Documentation