feat(security): add @AuditSecurityEvent AOP aspect for security logging#191
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an AOP-based security audit logging mechanism so controller endpoints can emit consistent “SECURITY_EVENT” logs for authentication-related actions (login/register/logout/refresh/password change), addressing Issue #187.
Changes:
- Introduces
@AuditSecurityEvent+SecurityEventTypeto mark auditable security operations. - Implements
SecurityAuditAspectwith success/failure logging, username extraction, and proxy IP support. - Applies the annotation to
UserControllerauth endpoints and adds unit + integration tests for logging.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/main/java/vaultWeb/security/aspects/SecurityAuditAspect.java | New aspect that emits audit logs on annotated method success/failure. |
| backend/src/main/java/vaultWeb/security/annotations/AuditSecurityEvent.java | New annotation to mark auditable endpoints with an event type. |
| backend/src/main/java/vaultWeb/security/annotations/SecurityEventType.java | New enum defining supported security audit event types. |
| backend/src/main/java/vaultWeb/controllers/UserController.java | Adds @AuditSecurityEvent to key auth endpoints (register/login/refresh/logout/change-password). |
| backend/src/test/java/vaultWeb/security/aspects/SecurityAuditAspectTest.java | Unit tests validating log formatting, username/IP extraction, and failure logging. |
| backend/src/test/java/vaultWeb/integration/SecurityAuditIntegrationTest.java | Integration tests asserting audit logs are emitted for key auth flows. |
Comments suppressed due to low confidence (1)
backend/src/main/java/vaultWeb/controllers/UserController.java:117
- With
@AuditSecurityEvent(SecurityEventType.TOKEN_REFRESH)applied, this method returns 401 on missing refresh token without throwing. The audit aspect’s current@AfterReturningadvice will log this asstatus=SUCCESS, which will create misleading audit records for refresh failures. Consider throwing an Unauthorized-type exception for this branch or updating the aspect to inspect the returned ResponseEntity status and log FAILURE for non-2xx responses.
@AuditSecurityEvent(SecurityEventType.TOKEN_REFRESH)
@ApiRateLimit(capacity = 5, refillTokens = 5, refillDurationMinutes = 1, useIpAddress = true)
@PostMapping("/refresh")
public ResponseEntity<?> refresh(
@CookieValue(name = "refresh_token", required = false) String refreshToken,
HttpServletResponse response) {
if (refreshToken == null) {
return ResponseEntity.status(401).build();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@matthewhou19 Please check copilots comments and if the comments are correct and admissible, please adjust the part. If the comments are not appropiate please resolve the conversations |
Implements issue Vault-Web#187 - adds structured audit logging for security events. - Add SecurityEventType enum (LOGIN, LOGOUT, REGISTER, TOKEN_REFRESH, PASSWORD_CHANGE) - Add @AuditSecurityEvent annotation for marking security-auditable methods - Add SecurityAuditAspect with @AfterReturning/@AfterThrowing advice - Log: event type, username, IP address, timestamp, status, error (on failure) - Support X-Forwarded-For header for proxy environments - Extract username from JWT (authenticated) or UserDto (login/register) - Apply annotation to UserController endpoints (login, logout, register, refresh, changePassword) - Add 6 unit tests and 4 integration tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
7366d9a to
3ecdb6f
Compare
@AfterReturning was always logging status=SUCCESS regardless of the actual HTTP response. Methods like refresh() return 401 without throwing, which was incorrectly logged as SUCCESS. Now captures the return value and checks ResponseEntity status code to log FAILURE for non-2xx responses. Also fixes unit test for X-Forwarded-For to match the aspect's actual behavior of using remoteAddr only. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Linked issue
Closes #
Implements issue #187 - adds structured audit logging for security events.
How to test
Notes / Risk