fix(ui): disallow content labeling when contributor is not signed on #2345
Conversation
jo-elimu
requested review from
alexander-kuruvilla,
eymaal and
jpatel3
and removed request for
a team
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2345 +/- ##
============================================
+ Coverage 16.72% 16.84% +0.11%
- Complexity 461 465 +4
============================================
Files 264 264
Lines 7826 7848 +22
Branches 897 903 +6
============================================
+ Hits 1309 1322 +13
- Misses 6443 6450 +7
- Partials 74 76 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughDependency tree updated with a version bump and new model dependency. Three controller endpoints for Emoji, Image, and Video editing now require a session Contributor; when absent, they set HTTP 403 and return "error". Method signatures updated to include HttpServletResponse. No public API exports added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant C as EditController (Emoji/Image/Video)
participant S as HttpSession
participant D as Domain Logic
U->>C: POST /{id}/content-label (add/remove)
C->>S: getAttribute("contributor")
alt Contributor missing
C-->>U: HTTP 403, body: "error"
else Contributor present
C->>D: add/remove label, create ContributionEvent
D-->>C: result
C-->>U: HTTP 200, body: "success"
end
note over C: Methods now accept HttpServletResponse and set status explicitly
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
🧹 Nitpick comments (4)
src/main/java/ai/elimu/web/content/multimedia/video/VideoEditController.java (2)
186-201: 403 guard on missing Contributor — correct; consider ResponseEntity for clarity.Current approach sets status on the response and returns "error". Optional: switch to ResponseEntity for explicit status/body and consistency across controllers.
Apply:
- @ResponseBody - public String handleAddContentLabelRequest( - HttpServletRequest request, - HttpServletResponse response, - HttpSession session, - @PathVariable Long id) { + public ResponseEntity<String> handleAddContentLabelRequest( + HttpServletRequest request, + HttpSession session, + @PathVariable Long id) { @@ - if (contributor == null) { - response.setStatus(HttpStatus.FORBIDDEN.value()); - return "error"; - } + if (contributor == null) { + return ResponseEntity.status(HttpStatus.FORBIDDEN).body("error"); + }Note: remove @responsebody when returning ResponseEntity (Spring handles it).
272-284: 403 Responses Now Trigger Client-Side Failure Callbacks – Consider Surfacing a User-Facing ErrorOur .fail handlers in the JSPs (e.g.
src/main/webapp/WEB-INF/jsp/content/multimedia/video/edit.jsparound lines 163–168 and 190–195) will indeed be invoked for HTTP 403, since jQuery treats any non-2xx as failure . However, those callbacks currently only log to the console:jqXHR.fail(function() { console.info('letterId ajax error'); });To improve UX, you may want to:
- Detect the 403 status inside your
.failand show a message, for example:.fail(function(jqXHR) { if (jqXHR.status === 403) { alert('You do not have permission to modify labels.'); } console.error('Label update error', jqXHR); });- Apply the same enhancement for all
.failcallbacks in:
emoji/edit.jsp(lines 120–126, 150–153)image/edit.jsp(lines 197–203, 224–230, 277–283, 304–310, 360–363, 384–390)video/edit.jspas noted aboveThis will ensure that a 403 from your new guard in
VideoEditController.handleRemoveContentLabelRequestis not only prevented from adding/removing UI chips, but also clearly communicated to the user.src/main/java/ai/elimu/web/content/multimedia/image/ImageEditController.java (1)
256-268: 403 guard added for add-content-label — correct; prefer ResponseEntity for symmetry.Matches PR goal. Optional clean-up: return ResponseEntity and drop HttpServletResponse parameter for less imperative response handling.
Example:
- @ResponseBody - public String handleAddContentLabelRequest( - HttpServletRequest request, - HttpServletResponse response, - HttpSession session, - @PathVariable Long id) { + public ResponseEntity<String> handleAddContentLabelRequest( + HttpServletRequest request, + HttpSession session, + @PathVariable Long id) { @@ - if (contributor == null) { - response.setStatus(HttpStatus.FORBIDDEN.value()); - return "error"; - } + if (contributor == null) { + return ResponseEntity.status(HttpStatus.FORBIDDEN).body("error"); + }src/main/java/ai/elimu/web/content/emoji/EmojiEditController.java (1)
111-126: Add-content-label: 403 on missing Contributor — correct; optionally use ResponseEntity.Example:
- @ResponseBody - public String handleAddContentLabelRequest( - HttpServletRequest request, - HttpServletResponse response, - HttpSession session, - @PathVariable Long id) { + public ResponseEntity<String> handleAddContentLabelRequest( + HttpServletRequest request, + HttpSession session, + @PathVariable Long id) { @@ - if (contributor == null) { - response.setStatus(HttpStatus.FORBIDDEN.value()); - return "error"; - } + if (contributor == null) { + return ResponseEntity.status(HttpStatus.FORBIDDEN).body("error"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
pom-dependency-tree.txt(1 hunks)src/main/java/ai/elimu/web/content/emoji/EmojiEditController.java(6 hunks)src/main/java/ai/elimu/web/content/multimedia/image/ImageEditController.java(4 hunks)src/main/java/ai/elimu/web/content/multimedia/video/VideoEditController.java(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-03T08:01:30.217Z
Learnt from: jo-elimu
PR: elimu-ai/webapp#0
File: :0-0
Timestamp: 2025-05-03T08:01:30.217Z
Learning: The elimu-ai/webapp repository has a dependency on ai.elimu:model (version 2.0.97) that provides shared model classes and enums. The Language enum should be imported from ai.elimu.model.v2.enums.Language instead of creating local duplicates.
Applied to files:
pom-dependency-tree.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test_rest
- GitHub Check: build (windows-latest, 17)
- GitHub Check: build (windows-latest, 21)
- GitHub Check: build (macos-latest, 21)
- GitHub Check: build (ubuntu-latest, 17)
- GitHub Check: build (macos-latest, 17)
- GitHub Check: build (ubuntu-latest, 21)
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: test_rest
🔇 Additional comments (10)
pom-dependency-tree.txt (1)
1-4: Model dependency bump verified — no duplicates or conflicts foundAll checks passed:
- The dependency tree includes
ai.elimu:model:jar:model-2.0.124:compile, confirming the bump to 2.0.124.- No local
enum Languagedeclarations were found; all imports reference the sharedai.elimu.model.v2.enums.Language.- Only one
gsondependency (version 2.13.1) appears in the tree, so there are no transitive version conflicts.No further action needed.
src/main/java/ai/elimu/web/content/multimedia/video/VideoEditController.java (2)
27-27: New imports for HttpServletResponse/HttpStatus are appropriate.Also applies to: 40-40
216-216: Switched to local contributor variable — good consistency and readability.Also applies to: 237-237, 258-258
src/main/java/ai/elimu/web/content/multimedia/image/ImageEditController.java (3)
29-29: New imports for HttpServletResponse/HttpStatus are appropriate.Also applies to: 48-48
347-352: 403 guard for remove-content-label — OK; verify client behavior on 403.Use the same script provided in VideoEditController to ensure frontend handles 403 on remove as well.
92-92: Nice: Language import uses ai.elimu.model.v2.enums.Language per prior guidance.src/main/java/ai/elimu/web/content/emoji/EmojiEditController.java (4)
15-15: New imports for HttpServletResponse/HttpStatus are appropriate.Also applies to: 27-27
141-141: Using the local contributor when creating EmojiContributionEvent — good.
156-167: Remove-content-label: 403 on missing Contributor — correct; ensure frontend expects 403.Use the repository-wide JS scan shared earlier to confirm error handling on 403 for emoji labeling flows.
188-188: Using the local contributor when logging remove events — good.
jo-elimu
deleted the
2344-disallow-content-labelling-when-the-contributor-is-not-signed-on
branch
#2344
Issue Number
Purpose
Technical Details
Testing Instructions
Screenshots
Format Checks
Note
Files in PRs are automatically checked for format violations with
mvn spotless:check.If this PR contains files with format violations, run
mvn spotless:applyto fix them.