fix(security): add access control annotations to various controllers#312
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)booklore-api/src/**/*.java📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (5)📓 Common learnings📚 Learning: 2026-03-25T19:09:09.638ZApplied to files:
📚 Learning: 2026-03-25T21:02:57.527ZApplied to files:
📚 Learning: 2026-03-27T15:06:25.913ZApplied to files:
📚 Learning: 2026-03-26T02:20:27.862ZApplied to files:
🔇 Additional comments (5)
📝 WalkthroughWalkthroughAdded method-level access controls and authentication checks across controllers and security layers; redacted sensitive values from several auth-related logs; standardized exception handler messages; and added null-checks to book/library access aspects. No public method signatures or request/response shapes were changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-api/src/main/java/org/booklore/controller/BookCoverController.java`:
- Around line 148-152: The endpoint in BookCoverController.getImages exposes a
path with {bookId} but the method signature lacks a `@PathVariable` Long bookId
and is missing the consistent access check; either remove {bookId} from the
`@PostMapping` path or add `@PathVariable` Long bookId and annotate the method with
`@CheckBookAccess`(bookIdParam = "bookId") so access is validated before calling
duckDuckGoCoverService.getCovers(request) (CoverFetchRequest remains the body
parameter); update the method signature accordingly to match other methods like
uploadCoverFromFile/uploadCoverFromUrl.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9792b05-606b-4432-90f0-cf7a72fd1019
📒 Files selected for processing (7)
booklore-api/src/main/java/org/booklore/controller/AppSettingController.javabooklore-api/src/main/java/org/booklore/controller/BookController.javabooklore-api/src/main/java/org/booklore/controller/BookCoverController.javabooklore-api/src/main/java/org/booklore/controller/CbxReaderController.javabooklore-api/src/main/java/org/booklore/controller/KoboSettingsController.javabooklore-api/src/main/java/org/booklore/controller/PdfReaderController.javabooklore-api/src/main/java/org/booklore/controller/TaskController.java
📜 Review details
⏰ 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). (4)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Packaging Smoke Test
🧰 Additional context used
📓 Path-based instructions (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/controller/AppSettingController.javabooklore-api/src/main/java/org/booklore/controller/BookController.javabooklore-api/src/main/java/org/booklore/controller/TaskController.javabooklore-api/src/main/java/org/booklore/controller/BookCoverController.javabooklore-api/src/main/java/org/booklore/controller/CbxReaderController.javabooklore-api/src/main/java/org/booklore/controller/PdfReaderController.javabooklore-api/src/main/java/org/booklore/controller/KoboSettingsController.java
🧠 Learnings (3)
📓 Common learnings
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
📚 Learning: 2026-03-26T02:20:27.862Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 198
File: booklore-api/src/main/java/org/booklore/controller/KoboController.java:100-103
Timestamp: 2026-03-26T02:20:27.862Z
Learning: In `booklore-api/src/main/java/org/booklore/controller/KoboController.java`, the Kobo reader device already makes direct outbound connections to Kobo's CDN/servers for other requests (confirmed by imnotjames via testing). Therefore, returning a `307 TEMPORARY_REDIRECT` to `cdn.kobo.com` for non-`BL-` book cover thumbnails (instead of server-side proxying) does NOT introduce a new privacy or behavioral regression — the Kobo device's IP and User-Agent are already exposed to Kobo through other direct requests.
Applied to files:
booklore-api/src/main/java/org/booklore/controller/BookController.javabooklore-api/src/main/java/org/booklore/controller/BookCoverController.javabooklore-api/src/main/java/org/booklore/controller/CbxReaderController.javabooklore-api/src/main/java/org/booklore/controller/PdfReaderController.java
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
booklore-api/src/main/java/org/booklore/controller/KoboSettingsController.java
🔇 Additional comments (8)
booklore-api/src/main/java/org/booklore/controller/AppSettingController.java (1)
45-52: LGTM!The
@PreAuthorize("@securityUtil.isAdmin()")annotation appropriately restricts application settings modification to administrators, consistent with the existingtestOidcConnectionendpoint's security pattern.booklore-api/src/main/java/org/booklore/controller/KoboSettingsController.java (1)
26-31: LGTM!The
@PreAuthorizeannotation alignsgetSettings()with the existing authorization pattern oncreateOrUpdateToken()andupdateSettings(), ensuring consistent access control across all Kobo settings endpoints.booklore-api/src/main/java/org/booklore/controller/TaskController.java (1)
39-47: LGTM!Adding
@PreAuthorize("@securityUtil.canAccessTaskManager() or@securityUtil.isAdmin()")tostartTask()closes the authorization gap and aligns this endpoint with the other task management endpoints in the controller.booklore-api/src/main/java/org/booklore/controller/CbxReaderController.java (1)
3-3: LGTM!The
@CheckBookAccess(bookIdParam = "bookId")annotations properly secure all three CBX reader endpoints, ensuring users can only access book pages and metadata for books they have library-level access to. The implementation is consistent with the parallel changes inPdfReaderController.Also applies to: 26-31, 36-42, 46-52
booklore-api/src/main/java/org/booklore/controller/PdfReaderController.java (1)
3-3: LGTM!The
@CheckBookAccess(bookIdParam = "bookId")annotations properly secure both PDF reader endpoints, matching the pattern applied toCbxReaderControllerand ensuring consistent access control across all reader controllers.Also applies to: 25-31, 35-41
booklore-api/src/main/java/org/booklore/controller/BookController.java (3)
127-132: LGTM!Adding
@CheckBookAccesstogetComicInfoMetadata()ensures users can only retrieve ComicInfo metadata for books in their assigned libraries, closing an access control gap on this read endpoint.
136-141: LGTM!Adding
@CheckBookAccesstogetFileMetadata()appropriately restricts file metadata extraction to books the user has access to.
163-172: LGTM!The combination of
@PreAuthorize("@securityUtil.canEditMetadata() or@securityUtil.isAdmin()")with the existing@CheckBookAccessprovides proper defense-in-depth: Spring Security's@PreAuthorizevalidates permission to edit metadata first, thenBookAccessAspectverifies the user has access to the specific book. This is appropriate for a content-replacement operation.
booklore-api/src/main/java/org/booklore/controller/BookCoverController.java
Show resolved
Hide resolved
2 participants
Description
Linked Issue: Fixes #
Changes
This pull request enhances the security of several controller endpoints by adding method-level authorization checks using Spring Security annotations. The updates ensure that sensitive operations, such as updating settings, accessing book content, synchronizing with Kobo, and managing tasks, are only accessible to users with the appropriate permissions. Additionally, book-related endpoints now consistently enforce access checks to protect user data.
Authorization and Access Control Enhancements:
@PreAuthorizechecks to restrict the following endpoints to users with specific roles or permissions:AppSettingController.BookCoverControllernow requires metadata edit or admin permissions.BookControllernow requires metadata edit or admin permissions.KoboSettingsControllernow requires Kobo sync or admin permissions.TaskControllernow requires task manager or admin permissions.Book Access Validation:
@CheckBookAccessannotation to ensure users can only access book data they are authorized for in the following endpoints:BookControllerPdfReaderControllerSummary by CodeRabbit