Feature/access/logback#87
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 38 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR enhances application security and observability by adding method-level authorization checks across ticket-related endpoints, introducing a comprehensive Logback configuration for structured logging, implementing detailed audit logging with staff identifiers in critical operations, and registering error page routes while improving security configuration ordering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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: 5
🧹 Nitpick comments (2)
src/main/java/org/example/cyberwatch/config/security/TicketSecurityService.java (1)
83-91: Consider null-guardingauthenticationinisAdmin.
canAccess/canAccessByCodealso assumeauthenticationis non-null, but sinceisAdminis now invoked from method-level SpEL on endpoints where Spring Security normally supplies a non-nullAuthentication, this is low-risk. Still, a defensiveif (authentication == null) return false;would align with typicalPermissionEvaluator-style helpers and avoid an NPE if ever invoked outside a filtered context (e.g., tests, internal callers).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/config/security/TicketSecurityService.java` around lines 83 - 91, Add a defensive null check at the start of isAdmin(Authentication authentication): if authentication is null return false; this prevents an NPE when isAdmin is invoked outside a secured filter context (e.g., in tests or internal callers). Locate the isAdmin method in TicketSecurityService and add the null-guard before using authentication.getPrincipal(); no other behavior should change (canAccess and canAccessByCode can remain as-is for now, but follow the same pattern if you want consistency with PermissionEvaluator-style helpers).src/main/resources/Logback spring.xml (1)
14-14: Relative log pathlogs/cyberwatch.logdepends on the process working directory.Using a relative path means logs land in whatever directory the JVM is launched from, which is fragile across environments (IDE run vs.
java -jarvs. containers). Consider parameterizing via a Spring property so it can be overridden per-environment:- <file>logs/cyberwatch.log</file> + <springProperty scope="context" name="LOG_PATH" source="logging.file.path" defaultValue="logs"/> + <file>${LOG_PATH}/cyberwatch.log</file>And update
fileNamePatternon line 19 to use${LOG_PATH}accordingly. Note that<springProperty>requires the file to be namedlogback-spring.xml(see the filename comment above).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/Logback` spring.xml at line 14, Change the hardcoded relative path in the <file> element to a Spring-configurable property and update the rolling <fileNamePattern> to use it: add a <springProperty name="LOG_PATH" source="LOG_PATH" defaultValue="logs"/> via <springProperty>, replace <file> value with ${LOG_PATH}/cyberwatch.log and adjust the <fileNamePattern> to reference ${LOG_PATH} as well; also ensure the config file is named logback-spring.xml so <springProperty> is supported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/cyberwatch/config/WebConfig.java`:
- Around line 19-21: The registered view controllers for "/error/404" and
"/error/500" won't catch actual HTTP error dispatches; add an ErrorPageRegistrar
bean (e.g., errorPageRegistrar()) that calls registry.addErrorPages(new
ErrorPage(HttpStatus.NOT_FOUND, "/error/404")) and registry.addErrorPages(new
ErrorPage(HttpStatus.INTERNAL_SERVER_ERROR, "/error/500")) so error responses
are routed to those paths; import ErrorPage,
ErrorPageRegistrar/ErrorPageRegistry and HttpStatus accordingly.
In
`@src/main/java/org/example/cyberwatch/features/ticket/service/TicketService.java`:
- Line 141: The logs in TicketService (e.g., the line logging "Ticket {} status
avancerad..." and similar lines at the other noted locations) currently include
caller-supplied IDs like performedById/assignedById/uploadedById which are
untrusted; change the API and service boundary so the controller resolves the
acting staff from the authenticated principal and passes a server-side actor
identity into service methods (instead of accepting performedById/uploadedById
from request params), remove those caller-controlled ID parameters from public
request signatures and from these audit-style log statements, and update the
service methods (TicketService methods that perform status
changes/assignments/uploads) to accept only the server-resolved actor identity
before reintroducing actor IDs in logs.
In `@src/main/resources/application.properties`:
- Line 27: The default property logging.level.org.springframework.security=DEBUG
should not be enabled globally; change the default to a less verbose level
(e.g., INFO or WARN) in application.properties and move the DEBUG setting into a
profile-specific file (e.g., application-dev.properties or
application-local.properties) or enable it via a spring profile so DEBUG is only
active when a development profile is active; update any documentation or run
configurations to use that profile when you need verbose security logs and
verify by running with the dev profile that
logging.level.org.springframework.security=DEBUG is applied while the default
environment uses INFO/WARN.
In `@src/main/resources/Logback` spring.xml:
- Around line 34-38: The org.springframework logger in the XML is set to WARN
which overrides the DEBUG setting for org.springframework.security from
application.properties; to preserve Spring Security DEBUG output, add an
explicit logger entry for org.springframework.security with level="DEBUG" in
this XML (e.g., <logger name="org.springframework.security" level="DEBUG"/>), or
alternatively remove the org.springframework.security DEBUG override from
application.properties so there is no conflicting configuration—modify the
logger entries around the existing <logger name="org.springframework"
level="WARN"/> and <logger name="org.example.cyberwatch" level="INFO"/> to
include the explicit child logger if keeping DEBUG.
- Around line 1-45: The Logback config file is misnamed so Spring Boot won't
load it; rename the resource file from "Logback spring.xml" to
"logback-spring.xml" so Spring Boot auto-detects it, ensuring the
<configuration> with appenders "CONSOLE" and "FILE", the <file> element pointing
to logs/cyberwatch.log, the rollingPolicy settings (fileNamePattern,
maxFileSize, maxHistory, totalSizeCap) and the logger entries
(org.springframework, org.hibernate, org.example.cyberwatch, and the <root>
logger) are applied at runtime.
---
Nitpick comments:
In
`@src/main/java/org/example/cyberwatch/config/security/TicketSecurityService.java`:
- Around line 83-91: Add a defensive null check at the start of
isAdmin(Authentication authentication): if authentication is null return false;
this prevents an NPE when isAdmin is invoked outside a secured filter context
(e.g., in tests or internal callers). Locate the isAdmin method in
TicketSecurityService and add the null-guard before using
authentication.getPrincipal(); no other behavior should change (canAccess and
canAccessByCode can remain as-is for now, but follow the same pattern if you
want consistency with PermissionEvaluator-style helpers).
In `@src/main/resources/Logback` spring.xml:
- Line 14: Change the hardcoded relative path in the <file> element to a
Spring-configurable property and update the rolling <fileNamePattern> to use it:
add a <springProperty name="LOG_PATH" source="LOG_PATH" defaultValue="logs"/>
via <springProperty>, replace <file> value with ${LOG_PATH}/cyberwatch.log and
adjust the <fileNamePattern> to reference ${LOG_PATH} as well; also ensure the
config file is named logback-spring.xml so <springProperty> is supported.
🪄 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: 20ef20b2-e086-4193-af68-a58b4766d300
⛔ Files ignored due to path filters (1)
logs/cyberwatch.logis excluded by!**/*.log
📒 Files selected for processing (13)
.gitignoresrc/main/java/org/example/cyberwatch/config/SecurityConfig.javasrc/main/java/org/example/cyberwatch/config/WebConfig.javasrc/main/java/org/example/cyberwatch/config/security/TicketSecurityService.javasrc/main/java/org/example/cyberwatch/exception/GlobalExceptionHandler.javasrc/main/java/org/example/cyberwatch/features/activitylog/controller/ActivityLogController.javasrc/main/java/org/example/cyberwatch/features/activitylog/service/ActivityLogService.javasrc/main/java/org/example/cyberwatch/features/comment/controller/CommentController.javasrc/main/java/org/example/cyberwatch/features/ticket/controller/TicketController.javasrc/main/java/org/example/cyberwatch/features/ticket/service/TicketService.javasrc/main/resources/Logback spring.xmlsrc/main/resources/application.propertiessrc/main/resources/static/js/dashboard.js
💤 Files with no reviewable changes (1)
- src/main/java/org/example/cyberwatch/features/activitylog/service/ActivityLogService.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Security