AUDIT-61: Create the REST APIs for the Security Audit Logging - #58
AUDIT-61: Create the REST APIs for the Security Audit Logging#58sudhanshu-raj wants to merge 11 commits into
Conversation
|
hi @ManojLL @wikumChamith , let me know your thoughts |
| List<AuditSecurityEvent> allRelated = auditService.getRelatedSecurityEvents(sessionId, 1000); | ||
| long totalCount = allRelated.size(); | ||
| int totalPages = UtilClass.computeTotalPages(totalCount, size); | ||
|
|
||
| int fromIndex = page * size; | ||
| List<AuditSecurityEvent> pagedList; | ||
| if (fromIndex >= allRelated.size()) { | ||
| pagedList = Collections.emptyList(); | ||
| } else { | ||
| int toIndex = Math.min(fromIndex + size, allRelated.size()); | ||
| pagedList = allRelated.subList(fromIndex, toIndex); | ||
| } |
There was a problem hiding this comment.
Paging this in memory needs to change before merge, for two reasons.
page * size is int arithmetic on an unclamped request parameter, so a large page overflows. Running it against this controller with RestExceptionHandler in place:
GET /rest/v1/securityauditlogs/releatedAudits?sessionId=SESSION-A&page=200000000&size=15
-> 500 {"error":"Internal Server Error","message":"An unexpected error occurred"}
page * size is -1294967296, which slips past the fromIndex >= allRelated.size() guard, and then subList throws IndexOutOfBoundsException. The main endpoint survives the same input because Hibernate rejects a negative setFirstResult as an IllegalArgumentException (400), so this is the only path that 500s.
The other problem is that totalLogs and totalPages are derived from a list the DAO has already truncated. getRelatedSecurityEvents ends in setMaxResults(limit), so with the hardcoded 1000 a session with more events than that reports totalLogs: 1000 and the older rows are unreachable at any page. Nothing throws, so a client paging through just gets a quietly wrong total.
ReadAuditRestController.fetchRelatedAudits already has the shape I would copy: getRelatedReadLogs(sessionId, page, size) alongside countRelatedReadLogs(sessionId). Adding the matching pair to AuditService/AuditDao for security events fixes both problems and gets rid of the 1000 literal at the same time. UtilClass.paginate is not a shortcut here, by the way, it has the same negative-index hole.
There was a problem hiding this comment.
I thought of avoiding inner changes but that seems to be bad idea. anyway fixed it with the same thought.
| public void shouldFetchSecurityAuditsSuccessfullyWithIdFilter() throws Exception { | ||
| AuditSecurityEvent mockEvent = mock(AuditSecurityEvent.class); | ||
| when(mockEvent.getEventType()).thenReturn(AuditSecurityEventType.LOGIN_SUCCESS); | ||
| when(auditService.getSecurityEventById(1)).thenReturn(mockEvent); | ||
|
|
||
| mockMvc.perform(get("/rest/v1/securityauditlogs").param("logId", "1")).andExpect(status().isOk()); | ||
|
|
||
| verify(auditService).getSecurityEventById(1); | ||
| } |
There was a problem hiding this comment.
I'd like a success-path assertion here before this merges. The tests only check the status code, and with mock(AuditSecurityEvent.class) every getter except getEventType() returns null, so the field mapping in mapToDTO is never exercised. I swapped username with userUuid and ipAddress with userAgent in mapToDTO and all seven tests still passed, which means a misattributed IP or user in the audit payload would ship with green CI.
AuditSecurityEvent has a Lombok builder, so no mock is needed:
AuditSecurityEvent event = AuditSecurityEvent.builder().id(7).eventType(AuditSecurityEventType.LOGIN_SUCCESS)
.username("admin").userUuid("user-uuid-1").eventTime(new Date()).ipAddress("10.0.0.7")
.userAgent("curl/8.4").sessionId("SESSION-A").details("{}").build();Asserting the JSON fields off that, plus totalLogs/currentLogs/totalPages/currentPage, would close the gap.
Worth stretching shouldFetchRelatedAuditsSuccessfully too: the paging arithmetic in fetchRelatedAudits is the only genuinely new logic in this PR and the test covers a single-element first page only. A case with more items than fit on a page, and one requesting a page past the end, would have caught the overflow I mentioned on the controller.
There was a problem hiding this comment.
Modified with the more robust test cases.
| @RequiredArgsConstructor | ||
| public class SecurityAuditRestController { | ||
|
|
||
| private final AuditService auditService; |
There was a problem hiding this comment.
This one is worth fixing but I don't think it blocks the PR: a caller who lacks View Security Audit Logs gets a 500 from these endpoints instead of a 403. The service methods are @Authorized(VIEW_SECURITY_AUDIT_LOGS) and the injected proxy runs core's AuthorizationAdvice, so an unprivileged call throws APIAuthenticationException, and RestExceptionHandler has no branch for it. It falls through to handleGeneralError:
500 {"error":"Internal Server Error","message":"An unexpected error occurred"}
I checked that with this controller and that advice in a standalone MockMvc setup. For comparison, webservices.rest's own BaseRestController.apiAuthenticationExceptionHandler answers 403, and nothing else is competing to handle it here since webservices.rest 2.49.0 ships no @ControllerAdvice of its own.
An @ExceptionHandler(APIAuthenticationException.class) returning 403 in RestExceptionHandler would cover this controller and the three that already exist. Permission denials on a security-audit endpoint are the ones most likely to be hit in practice, and 500s there will send people hunting for a server fault that isn't there.
There was a problem hiding this comment.
done, added it
| return SecurityLogResponseDTO.builder().totalLogs(1).currentLogs(1).securityAuditLogs(securityAuditLogsDTO) | ||
| .build(); |
There was a problem hiding this comment.
A single-log lookup reports totalPages: 0 even though it returns a row:
GET /rest/v1/securityauditlogs?logId=7
{"totalLogs":1,"currentLogs":1,"totalPages":0,"currentPage":0,"securityAuditLogs":[{...}]}
A client driving its loop off totalPages would skip the record it just asked for.
| return SecurityLogResponseDTO.builder().totalLogs(1).currentLogs(1).securityAuditLogs(securityAuditLogsDTO) | |
| .build(); | |
| return SecurityLogResponseDTO.builder().totalLogs(1).currentLogs(1).totalPages(1).currentPage(0) | |
| .securityAuditLogs(securityAuditLogsDTO).build(); |
|
|
||
| @GetMapping | ||
| public SecurityLogResponseDTO fetchSecurityAudits(@RequestParam(value = "logId", required = false) Integer logId, | ||
| @RequestParam(value = "eventType", required = false) String eventType, |
There was a problem hiding this comment.
Not a blocker, but a misspelled eventType comes back as an empty result rather than an error. AuditSecurityEventType.fromName maps anything unrecognised to UNKNOWN, and the DAO then filters on that enum value, so ?eventType=LOGIN_SUCESS returns {"totalLogs":0,...} and the caller can't tell a typo from a genuinely empty range. UNKNOWN is a real stored value too, so ?eventType=UNKNOWN and ?eventType=garbage are indistinguishable.
This method already rejects a malformed startDate with a 400, so validating eventType against the enum and throwing IllegalArgumentException listing the accepted values would be consistent and much friendlier to API clients.
There was a problem hiding this comment.
fixed, checking this early now
| .totalPages(totalPages).currentPage(page).securityAuditLogs(securityAuditLogsDTO).build(); | ||
| } | ||
|
|
||
| @GetMapping("/releatedAudits") |
There was a problem hiding this comment.
Not a correctness problem, but releatedAudits is misspelled, and the window to fix it for free closes at the next release. ReadAuditRestController carries the same typo and neither endpoint has shipped: both landed after the auditlogweb-1.0.0 tag and are still on 1.1.0-SNAPSHOT. Renaming both to relatedAudits now costs nothing, whereas after a release it's a permanent part of the URL contract.
…body validation too
| List<AuditSecurityEvent> getRelatedSecurityEvents(String sessionId, int limit); | ||
| List<AuditSecurityEvent> getRelatedSecurityEvents(String sessionId, int size, int page); | ||
|
|
||
| @Authorized(AuditLogConstants.VIEW_READ_AUDIT_LOGS) |
There was a problem hiding this comment.
The privilege on this method is wrong, and it takes the /relatedAudits endpoint down with it: it is gated on VIEW_READ_AUDIT_LOGS, while every other security-audit method in this interface, including getRelatedSecurityEvents two lines up, uses VIEW_SECURITY_AUDIT_LOGS.
If merged as-is, a user granted only "View Security Audit Logs" cannot use GET /rest/v1/securityauditlogs/relatedAudits at all, because fetchRelatedAudits calls both service methods in sequence: the first passes the authorization advice, the second throws APIAuthenticationException. I wrapped the service in core's AuthorizationAdvice the way moduleApplicationContext.xml's serviceInterceptors does, with a user context holding the security privilege but not the read one:
GET /rest/v1/securityauditlogs/relatedAudits?sessionId=SESSION-A
-> 403 {"error":"Unauthorized access", ...}
GET /rest/v1/securityauditlogs
-> 200
config.xml declares the three privileges separately and no role is handed them together, so a security-auditor role is exactly the case that breaks. It goes the other way too: someone with only "View Read Audit Logs" can call this and learn how many security events a session has.
| @Authorized(AuditLogConstants.VIEW_READ_AUDIT_LOGS) | |
| @Authorized(AuditLogConstants.VIEW_SECURITY_AUDIT_LOGS) |
Nothing catches it today because the controller tests mock AuditService, so the advice never runs. ServiceAuthorizationTest is where the two sibling checks live and would be a natural home for this one.
There was a problem hiding this comment.
Ahh I missed it, fixed now
| */ | ||
| @Authorized(AuditLogConstants.VIEW_SECURITY_AUDIT_LOGS) | ||
| List<AuditSecurityEvent> getRelatedSecurityEvents(String sessionId, int limit); | ||
| List<AuditSecurityEvent> getRelatedSecurityEvents(String sessionId, int size, int page); |
There was a problem hiding this comment.
The parameter order here is inverted relative to the Javadoc directly above it, which lists @param page before @param size, and relative to ReadAuditService.getRelatedReadLogs(String sessionId, int page, int size). Because page * size is commutative the offset comes out right either way, so a caller who writes the arguments in the documented order gets no error at all, just a wrongly sized page. Capturing what actually reaches the query:
dao.getRelatedSecurityEvents("S", 0, 15) // meaning page 0, size 15
-> setFirstResult(0) setMaxResults(0) // asks the DB for zero rows
dao.getRelatedSecurityEvents("S", 2, 15) // meaning page 2, size 15
-> setFirstResult(30) setMaxResults(2) // right offset, 2 rows instead of 15
Both current callers pass it correctly so nothing is broken right now, but this is a public service API and the window to change it for free closes at the next release. I would swap it to (String sessionId, int page, int size) across AuditService, AuditServiceImpl and AuditDao so it matches its own Javadoc and the read-audit sibling. Two small things while you are in those files: countRelatedSecurityEvents below is the only method in this interface with no Javadoc, and the DAO spells it countRelatedSecurityEvent, singular, where the sibling is countRelatedReadLogs.
There was a problem hiding this comment.
Fixed, I ordered the doc lines
There was a problem hiding this comment.
Both callers pass this correctly so nothing is broken today, but the two sibling methods still take their int pair in opposite orders: getRelatedSecurityEvents(String sessionId, int size, int page) here, and ReadAuditService.getRelatedReadLogs(String sessionId, int page, int size) next door. Both are (String, int, int), so a call written in the wrong order compiles and quietly pages wrong. Run against a real database with 80 events in one session:
getRelatedSecurityEvents("S", 15, 0) -> 15 rows (size 15, page 0)
getRelatedSecurityEvents("S", 15, 2) -> 15 rows (size 15, page 2)
getRelatedSecurityEvents("S", 0, 15) -> 0 rows (what someone writing "page 0, size 15" gets)
getRelatedSecurityEvents("S", 2, 15) -> 2 rows (what someone writing "page 2, size 15" gets)
I would still swap it to (String sessionId, int page, int size). The signature is already changing in this PR, it was (sessionId, limit) on main, so matching the read-audit sibling costs the same three call sites now and becomes a breaking change once 1.1.0 ships.
The other half of this is that nothing in the build pins the offset. I changed AuditDao.getRelatedSecurityEvents to query.setFirstResult(page) and ran the lot: 196 api tests and 67 omod tests, all green. AuditDaoTest only ever asks for page 0, where page * size and page agree. A case like getRelatedSecurityEvents("s", 15, 2) asserting verify(query).setFirstResult(30) would close it.
There was a problem hiding this comment.
Done, and tweaked the existing test case for this case.
There was a problem hiding this comment.
The offset itself is still unpinned. What landed in AuditDaoTest is when(securityEventQuery.setFirstResult(30)).thenReturn(securityEventQuery), which is a stub rather than an assertion: the DAO discards that return value, and the class initialises its mocks with MockitoAnnotations.openMocks rather than MockitoExtension, so there is no strict-stubs check to trip on an unused stub either. Changing page * size back to page would leave the test green. A verify(securityEventQuery).setFirstResult(30) beside the existing setMaxResults(2) verification closes it.
| throw new IllegalArgumentException("Please provide a valid log ID"); | ||
| } | ||
|
|
||
| if (eventType != null) { |
There was a problem hiding this comment.
This rejects a blank eventType along with a bad one, which I do not think is the intent. Both an empty and a whitespace value come back 400, while the equivalent blank username is treated as no filter:
GET /rest/v1/securityauditlogs?eventType= -> 400 {"error":"Bad Request","message":"Invalid eventType "}
GET /rest/v1/securityauditlogs?username= -> 200 (unfiltered)
fromName returns null for blank input precisely so that blank means "no filter", and bindSecurityEventFilters skips a blank username for the same reason. A UI that always emits its filter parameters will now get a 400 whenever the event-type dropdown is left empty. Skipping the check for a blank value keeps this consistent with how username behaves:
| if (eventType != null) { | |
| if (eventType != null && !eventType.trim().isEmpty()) { |
| @ExceptionHandler(APIAuthenticationException.class) | ||
| public ResponseEntity<Map<String, String>> handleAPIAuthException(APIAuthenticationException ex) { | ||
| return buildResponseEntity("Unauthorized access", ex.getMessage(), HttpStatus.FORBIDDEN); | ||
| } |
There was a problem hiding this comment.
An unauthenticated caller reaches this handler too, and for that case the platform answers 401 rather than 403. In webservices.rest 2.49.0, BaseRestController.apiAuthenticationExceptionHandler branches on Context.isAuthenticated(): logged in but missing the privilege gives 403, not logged in gives 401 plus a WWW-Authenticate: Basic realm=... header. Its AuthorizationFilter deliberately lets credential-less requests straight through ("It will not fail on invalid or missing credentials. We count on the API to throw exceptions if an unauthenticated user tries to do something they are not allowed to do"), so the unauthenticated case genuinely lands here instead of being bounced upstream. As written, every caller gets 403, so a client whose session expired cannot tell "log in again" from "you will never be allowed to see this". My suggestion last round to return 403 was only half the story.
| @ExceptionHandler(APIAuthenticationException.class) | |
| public ResponseEntity<Map<String, String>> handleAPIAuthException(APIAuthenticationException ex) { | |
| return buildResponseEntity("Unauthorized access", ex.getMessage(), HttpStatus.FORBIDDEN); | |
| } | |
| @ExceptionHandler(APIAuthenticationException.class) | |
| public ResponseEntity<Map<String, String>> handleAPIAuthException(APIAuthenticationException ex) { | |
| if (Context.isAuthenticated()) { | |
| return buildResponseEntity("Forbidden", ex.getMessage(), HttpStatus.FORBIDDEN); | |
| } | |
| return buildResponseEntity("Unauthorized", ex.getMessage(), HttpStatus.UNAUTHORIZED); | |
| } |
That needs an org.openmrs.api.context.Context import. I also changed the error labels: the other handlers here use the status reason phrase, and "Unauthorized access" sitting on a 403 reads like a 401. Sending the WWW-Authenticate challenge as well would match BaseRestController exactly, but the status code is the part that matters.
There was a problem hiding this comment.
thanks, fixed !
| mockMvc.perform(get("/rest/v1/securityauditlogs").param("eventType", "LOGIN_SUCESS")) | ||
| .andExpect(status().isBadRequest()).andExpect(jsonPath("$.error", is("Bad Request"))) | ||
| .andExpect(jsonPath("$.message", is("Invalid eventType LOGIN_SUCESS"))); | ||
| } |
There was a problem hiding this comment.
The two behavior changes in the last couple of commits both sit in code this class already reaches, and neither has a test.
A blank eventType now means "no filter" instead of a 400, which is the empty-dropdown case, and nothing here covers it:
mockMvc.perform(get("/rest/v1/securityauditlogs").param("eventType", "")).andExpect(status().isOk());The 401 vs 403 split in RestExceptionHandler has no test anywhere in the repo. All four REST controller tests wire the advice in, but none of them ever throws APIAuthenticationException, so neither branch is exercised. It is reachable from this exact setup, since mockito-inline is already on the omod test classpath:
when(auditService.getSecurityEvents(isNull(), isNull(), isNull(), isNull(), anyInt(), anyInt()))
.thenThrow(new APIAuthenticationException("Privileges required: View Security Audit Logs"));
try (MockedStatic<Context> ctx = mockStatic(Context.class)) {
ctx.when(Context::isAuthenticated).thenReturn(false);
mockMvc.perform(get("/rest/v1/securityauditlogs")).andExpect(status().isUnauthorized())
.andExpect(jsonPath("$.error", is("Unauthorized")));
}I ran both of those against this branch and they pass, so it is mostly a matter of committing them. The behavior is right today so this is not a correctness blocker, but I would land it in the same PR: the wrong privilege constant got as far as it did here precisely because the service is mocked out of every controller test, and ServiceAuthorizationTest still has no case for countRelatedSecurityEvents.
There was a problem hiding this comment.
Thanks for suggesting the code, yeah putting these more test cases will help in test more edge cases. And on ServiceAuthorizationTest is meant to verify the AOP wiring where it would verify the one test per privilege for @Authorized interceptor and verifying one such interceptor for a privilege is enough for it, that's why I don't think covering every method under the same privilege is necessary.
|
|
@claude review |
| size = 15; | ||
| } | ||
|
|
||
| List<AuditSecurityEvent> allRelated = auditService.getRelatedSecurityEvents(sessionId, size, page); |
There was a problem hiding this comment.
This needs fixing before merge: the arguments here were not updated when the signature changed in 6a26eb7. AuditService.getRelatedSecurityEvents now reads (String sessionId, int page, int size), so (sessionId, size, page) feeds the requested size into page and the requested page into size.
If merged as-is, every request that leaves page at its default returns an empty list while still reporting the true total, so GET /rest/v1/securityauditlogs/relatedAudits?sessionId=X never returns a row. The DAO ends up calling setFirstResult(size * 0) and setMaxResults(0), and in Hibernate 5.6.15 (what openmrs-core 2.7.0 resolves to) setMaxResults(0) stores a 0 rather than meaning "unlimited": RowSelection.setMaxRows keeps the zero, and AbstractProducedQuery.doList() opens with if (getMaxResults() == 0) return Collections.EMPTY_LIST;, so the query never reaches the database. For a session holding 25 events that works out to:
{"totalLogs":25,"currentLogs":0,"totalPages":2,"currentPage":0,"securityAuditLogs":[]}
Nothing throws, so callers get a healthy 200 with a plausible total and no rows.
| List<AuditSecurityEvent> allRelated = auditService.getRelatedSecurityEvents(sessionId, size, page); | |
| List<AuditSecurityEvent> allRelated = auditService.getRelatedSecurityEvents(sessionId, page, size); |
shouldFetchRelatedAuditsSuccessfully needs the matching flip to ("session-123", 0, 10). It verifies the mocked service against (10, 0), so it mirrors this call site instead of checking it, and that is why CI stays green. For comparison, ReadAuditRestController.fetchRelatedAudits two files over already passes (sessionId, page, size).
| List<AuditSecurityEvent> relatedEvents = null; | ||
| if (event.getSessionId() != null && !event.getSessionId().isEmpty()) { | ||
| relatedEvents = auditService.getRelatedSecurityEvents(event.getSessionId(), RELATED_EVENTS_LIMIT); | ||
| relatedEvents = auditService.getRelatedSecurityEvents(event.getSessionId(), RELATED_EVENTS_LIMIT, 0); |
There was a problem hiding this comment.
Same inverted pair here, and this one takes down a panel that works on main today. After the swap, RELATED_EVENTS_LIMIT lands in page and the 0 lands in size, so the DAO reaches setMaxResults(0) and Hibernate hands back an empty list without running the query.
If merged as-is, the "RELATED ACTIVITY (SAME SESSION)" table on viewSecurityAudit.jsp disappears for every event, since the JSP guards it with <c:if test="${not empty relatedEvents}">. On main this call is getRelatedSecurityEvents(sessionId, RELATED_EVENTS_LIMIT), which maps to setMaxResults(10) and fills the table.
| relatedEvents = auditService.getRelatedSecurityEvents(event.getSessionId(), RELATED_EVENTS_LIMIT, 0); | |
| relatedEvents = auditService.getRelatedSecurityEvents(event.getSessionId(), 0, RELATED_EVENTS_LIMIT); |
SecurityAuditDetailControllerTest stubs and verifies ("session-test", 10, 0) on lines 106 and 114, so those need the same flip.
Description of what I changed
Created the REST APIs to fetch the security audit logs.
The main API for fetching the logs based on different params, like :
openmrs/ws/rest/v1/securityauditlogsopenmrs/ws/rest/v1/securityauditlogs?logId=21openmrs/ws/rest/v1/securityauditlogs?eventType=SESSION_TIMEOUT&startDate=22/07/2026And the second one for fetching the related logs reference to that session id, like :
openmrs/ws/rest/v1/securityauditlogs/releatedAudits?sessionId=5048796D4BBFA09EAB8DA6D565B40E51Issue I worked on
see https://openmrs.atlassian.net/browse/AUDIT-61
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master