feat: implement comments on files and folders with @mentions and noti… - #108
Conversation
DenizAltunkapan
left a comment
DenizAltunkapan
left a comment
There was a problem hiding this comment.
The plumbing here is clean: path traversal is validated through validatePath, the DTO has proper constraints, delete access control is right, notifications are scoped per recipient, and the tests pass. But there is a design contradiction at the center of the feature that needs sorting out before this can go in, plus a missing rate limit. Details inline.
Two blockers and a couple of smaller notes below. Happy to re-review once the access model for mentions is resolved.
| } | ||
|
|
||
| /** Validates access of currentUser to owner's workspace. */ | ||
| public void validateAccess(User currentUser, String ownerUsername) { |
There was a problem hiding this comment.
validateAccess only lets the workspace owner through, so the owner is the only user who can ever comment. That makes @mentions of other users incoherent: the mentioned person has no access to this workspace, yet processMentions still sends them a notification carrying the owner's filePath. So a user can push path fragments from their private workspace to any username they name, which is both a small info leak and a spam vector. Either this feature needs a real sharing model to sit on, or mentions must be restricted to users who actually have access to the resource. As written the core behaviour does not hold together.
|
|
||
| /** Adds a new comment on a file or folder. */ | ||
| @PostMapping | ||
| public ResponseEntity<CommentDto> addComment(@Valid @RequestBody CreateCommentRequest request) |
There was a problem hiding this comment.
Commenting and mention notifications are not rate limited. Everywhere else in this codebase (upload, download, scan, share) writes are throttled, and here a single comment can create an unbounded number of notification rows for other users via many @mentions. Please route this through RateLimitFilter under a suitable category.
| comment.setFilePath(request.getFilePath()); | ||
| comment.setAuthorUsername(author.getUsername()); | ||
| comment.setContent(request.getContent()); | ||
| comment.setCreatedAt(Instant.now()); |
There was a problem hiding this comment.
This uses Instant.now() directly, while the other services (for example ResourceShareService) inject a Clock so time is testable and consistent. Please follow the same pattern here.
| recipient -> { | ||
| String message = | ||
| String.format("%s mentioned you in a comment on %s", authorUsername, itemName); | ||
| notificationService.sendNotification( |
There was a problem hiding this comment.
Minor: the mention notification message embeds the item name and path unconditionally. Once the access question above is resolved, double check that the recipient is actually entitled to see this filePath before including it in their notification.
2 participants
This PR adds a collaborative commenting system on top of files and folders within the Cloud Page storage layer. Users can now leave comments, view comment history, delete comments, and mention other workspace users using @username to trigger notifications.
Key Implementations
Database Entities:
Comment: Mapped to the comments table. Stores the workspace owner, target file/folder relative path, comment author, content, and creation timestamp.
Notification: Mapped to the notifications table. Stores notification text, recipient, read status, and related file context.
Backend Services:
CommentService: Manages comments CRUD operations. Validates that target items exist physically and verifies access limits before posting comments. Uses regular expressions to extract mentions, resolving them against existing users to dispatch notifications.
NotificationService: Handles sending notifications and marking them as read (either individually or all at once).
REST Endpoints:
CommentController: Exposes endpoints for creating comments (POST /api/comments), listing comments on an item (GET /api/comments), and deleting a comment (DELETE /api/comments/{id}).
NotificationController: Exposes endpoints for retrieving notifications (GET /api/notifications), marking one as read (POST /api/notifications/{id}/read), and clearing all (POST /api/notifications/read-all).
Tests and Formatting:
Added unit and MockMvc integration tests covering the new service and controller classes.
Formatted all files with Spotless to adhere to standard code styles.