Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -334,10 +334,20 @@ void logSecurityEvent(AuditSecurityEventType eventType, String username, String
* Retrieves the most recent N security events from the same session (for related activity).
*
* @param sessionId the session ID to filter by
* @param limit the maximum number of events to return
* @param page defines the particular page in a paginated view
* @param size defines how many logs want in one go
* @return a list of {@link AuditSecurityEvent} ordered by eventTime descending
*/
@Authorized(AuditLogConstants.VIEW_SECURITY_AUDIT_LOGS)
List<AuditSecurityEvent> getRelatedSecurityEvents(String sessionId, int limit);
List<AuditSecurityEvent> getRelatedSecurityEvents(String sessionId, int page, int size);

/**
* Counts the related security audit events with session ID filter.
*
* @param sessionId the session ID to filter by
* @return the number of logs found
*/
@Authorized(AuditLogConstants.VIEW_SECURITY_AUDIT_LOGS)
long countRelatedSecurityEvents(String sessionId);

}
Original file line number Diff line number Diff line change
Expand Up @@ -714,14 +714,30 @@ public AuditSecurityEvent getSecurityEventById(Integer eventId) {
* Retrieves the most recent N security events from the same session (for related activity).
*
* @param sessionId the session ID to filter by
* @param limit the maximum number of events to return
* @param page defines the particular page in a paginated view
* @param size defines how many logs want in one go
* @return a list of {@link AuditSecurityEvent} ordered by eventTime descending
*/
public List<AuditSecurityEvent> getRelatedSecurityEvents(String sessionId, int limit) {
public List<AuditSecurityEvent> getRelatedSecurityEvents(String sessionId, int page, int size) {
Query<AuditSecurityEvent> query = sessionFactory.getCurrentSession().createQuery(
"from AuditSecurityEvent e where e.sessionId = :sessionId order by e.eventTime desc", AuditSecurityEvent.class);
query.setParameter("sessionId", sessionId);
query.setMaxResults(limit);
query.setFirstResult(page * size);
query.setMaxResults(size);
return query.getResultList();
}

/**
* Returns the count for all related security events for the given session ID
*
* @param sessionId the session ID to filter by
* @return the count of logs found
*/
public long countRelatedSecurityEvents(String sessionId) {
Query<Long> query = sessionFactory.getCurrentSession()
.createQuery("select count(e) from AuditSecurityEvent e where e.sessionId = :sessionId", Long.class);
query.setParameter("sessionId", sessionId);
Long result = query.uniqueResult();
return result != null ? result : 0L;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* This Source Code Form is subject to the terms of the Mozilla Public License,
* v. 2.0. If a copy of the MPL was not distributed with this file, You can
* obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under
* the terms of the Healthcare Disclaimer located at http://openmrs.org/license.
*
* Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS
* graphic logo is a trademark of OpenMRS Inc.
*/
package org.openmrs.module.auditlogweb.api.dto;

import lombok.Builder;
import lombok.Data;
import org.openmrs.module.auditlogweb.api.utils.AuditSecurityEventType;

import java.util.Date;

@Data
@Builder
public class SecurityAuditLogDTO {

private Integer id;

private AuditSecurityEventType eventType;

private String username;

private String userUuid;

private Date eventTime;

private String ipAddress;

private String userAgent;

private String sessionId;

private String details;

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* This Source Code Form is subject to the terms of the Mozilla Public License,
* v. 2.0. If a copy of the MPL was not distributed with this file, You can
* obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under
* the terms of the Healthcare Disclaimer located at http://openmrs.org/license.
*
* Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS
* graphic logo is a trademark of OpenMRS Inc.
*/
package org.openmrs.module.auditlogweb.api.dto;

import lombok.Builder;
import lombok.Data;

import java.util.List;

@Data
@Builder
public class SecurityLogResponseDTO {

private long totalLogs;

private int currentLogs;

private int totalPages;

private int currentPage;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currentPage is already available in the pagination context. Is it necessary to send it back to the client?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that would be redundant if the client already sets the page parameter in the request then we are going to return same on the response too, but if client not sets page parameter then in that case it would be sightly useful in the response the client will have clear picture of from which page this logs belongs too. So it guess it's not that unnecessary, otherwise we can remove it.


private List<SecurityAuditLogDTO> securityAuditLogs;

}
Original file line number Diff line number Diff line change
Expand Up @@ -387,8 +387,13 @@ public AuditSecurityEvent getSecurityEventById(Integer eventId) {
}

@Override
public List<AuditSecurityEvent> getRelatedSecurityEvents(String sessionId, int limit) {
return auditDao.getRelatedSecurityEvents(sessionId, limit);
public List<AuditSecurityEvent> getRelatedSecurityEvents(String sessionId, int page, int size) {
return auditDao.getRelatedSecurityEvents(sessionId, page, size);
}

@Override
public long countRelatedSecurityEvents(String sessionId) {
return auditDao.countRelatedSecurityEvents(sessionId);
}

private Object fetchPreviousRevision(AuditEntity<?> entity, Object currentEntity) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -580,30 +580,45 @@ void shouldReturnRelatedSecurityEvents_WhenSessionIdProvided() {

when(session.createQuery(anyString(), eq(AuditSecurityEvent.class))).thenReturn(securityEventQuery);
when(securityEventQuery.setParameter("sessionId", sessionId)).thenReturn(securityEventQuery);
when(securityEventQuery.setMaxResults(5)).thenReturn(securityEventQuery);
when(securityEventQuery.setFirstResult(30)).thenReturn(securityEventQuery);
when(securityEventQuery.setMaxResults(2)).thenReturn(securityEventQuery);
when(securityEventQuery.getResultList()).thenReturn(Arrays.asList(e1, e2));

List<AuditSecurityEvent> result = auditDao.getRelatedSecurityEvents(sessionId, 5);
List<AuditSecurityEvent> result = auditDao.getRelatedSecurityEvents(sessionId, 15, 2);

assertNotNull(result);
assertThat(result, hasSize(2));
verify(securityEventQuery).setParameter("sessionId", sessionId);
verify(securityEventQuery).setMaxResults(5);
verify(securityEventQuery).setFirstResult(30);
verify(securityEventQuery).setMaxResults(2);
}

@Test
void shouldReturnEmptyList_WhenNoRelatedSecurityEventsFound() {
when(session.createQuery(anyString(), eq(AuditSecurityEvent.class))).thenReturn(securityEventQuery);
when(securityEventQuery.setParameter(anyString(), anyString())).thenReturn(securityEventQuery);
when(securityEventQuery.setFirstResult(0)).thenReturn(securityEventQuery);
when(securityEventQuery.setMaxResults(anyInt())).thenReturn(securityEventQuery);
when(securityEventQuery.getResultList()).thenReturn(Collections.emptyList());

List<AuditSecurityEvent> result = auditDao.getRelatedSecurityEvents("sess-ghost", 10);
List<AuditSecurityEvent> result = auditDao.getRelatedSecurityEvents("sess-ghost", 0, 10);

assertNotNull(result);
assertThat(result, empty());
}

@Test
void shouldReturnRelatedSecurityEventsCount() {
when(session.createQuery(anyString(), eq(Long.class))).thenReturn(countQuery);
when(countQuery.setParameter(eq("sessionId"), eq("session-123"))).thenReturn(countQuery);
when(countQuery.uniqueResult()).thenReturn(10L);

long count = auditDao.countRelatedSecurityEvents("session-123");

assertThat(count, is(10L));
verify(countQuery).setParameter("sessionId", "session-123");
}

@Test
void shouldBindUnknownEventType_WhenGivenSecurityEventIsInvalid() {
when(session.createQuery(anyString(), eq(AuditSecurityEvent.class))).thenReturn(securityEventQuery);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -397,17 +397,25 @@ void shouldReturnRelatedSecurityEvents() {
AuditSecurityEvent relatedSecurityEvent2 = AuditSecurityEvent.builder()
.eventType(AuditSecurityEventType.LOGIN_SUCCESS).eventTime(new Date()).sessionId("session-123").build();

when(auditDao.getRelatedSecurityEvents("session-123", 2))
when(auditDao.getRelatedSecurityEvents("session-123", 0, 2))
.thenReturn(Arrays.asList(relatedSecurityEvent1, relatedSecurityEvent2));

List<AuditSecurityEvent> result = auditService.getRelatedSecurityEvents("session-123", 2);
List<AuditSecurityEvent> result = auditService.getRelatedSecurityEvents("session-123", 0, 2);

assertNotNull(result);
assertEquals(2, result.size());
assertEquals(relatedSecurityEvent1, result.get(0));
assertEquals(relatedSecurityEvent2, result.get(1));
}

@Test
void shouldCountRelatedSecurityEvents() {
when(auditDao.countRelatedSecurityEvents("session-123")).thenReturn(10L);

long count = auditService.countRelatedSecurityEvents("session-123");
assertEquals(10L, count);
}

public static class TestEntity {

private Integer id;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public ReadAuditLogResponseDTO fetchReadAudits(@RequestParam(value = "logId", re

}

@GetMapping("/releatedAudits")
@GetMapping("/relatedAudits")
public ReadAuditLogResponseDTO fetchRelatedAudits(@RequestParam(value = "sessionId") String sessionId,
@RequestParam(value = "page", defaultValue = "0") int page,
@RequestParam(value = "size", defaultValue = "15") int size) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
/*
* This Source Code Form is subject to the terms of the Mozilla Public License,
* v. 2.0. If a copy of the MPL was not distributed with this file, You can
* obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under
* the terms of the Healthcare Disclaimer located at http://openmrs.org/license.
*
* Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS
* graphic logo is a trademark of OpenMRS Inc.
*/
package org.openmrs.module.auditlogweb.rest;

import lombok.RequiredArgsConstructor;
import org.openmrs.module.auditlogweb.AuditSecurityEvent;
import org.openmrs.module.auditlogweb.api.AuditService;
import org.openmrs.module.auditlogweb.api.dto.SecurityAuditLogDTO;
import org.openmrs.module.auditlogweb.api.dto.SecurityLogResponseDTO;
import org.openmrs.module.auditlogweb.api.utils.AuditSecurityEventType;
import org.openmrs.module.auditlogweb.api.utils.UtilClass;
import org.openmrs.module.webservices.rest.web.RestConstants;
import org.springframework.http.HttpStatus;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.server.ResponseStatusException;

import java.util.ArrayList;
import java.util.Collections;
import java.util.Date;
import java.util.List;

@RestController
@RequestMapping("/rest/" + RestConstants.VERSION_1 + "/securityauditlogs")
@RequiredArgsConstructor
public class SecurityAuditRestController {

private final AuditService auditService;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, added it


@GetMapping
public SecurityLogResponseDTO fetchSecurityAudits(@RequestParam(value = "logId", required = false) Integer logId,
@RequestParam(value = "eventType", required = false) String eventType,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, checking this early now

@RequestParam(value = "username", required = false) String username,
@RequestParam(value = "startDate", required = false) String startDate,
@RequestParam(value = "endDate", required = false) String endDate,
@RequestParam(value = "page", defaultValue = "0") int page,
@RequestParam(value = "size", defaultValue = "15") int size) {

if (logId != null && logId <= 0) {
throw new IllegalArgumentException("Please provide a valid log ID");
}

if (eventType != null && !eventType.trim().isEmpty()) {
AuditSecurityEventType parsed = AuditSecurityEventType.fromName(eventType);
if (parsed == null || parsed == AuditSecurityEventType.UNKNOWN) {
throw new IllegalArgumentException("Invalid eventType " + eventType);
}
}

if (logId != null) {
AuditSecurityEvent securityEvent = auditService.getSecurityEventById(logId);
if (securityEvent == null) {
throw new ResponseStatusException(HttpStatus.NOT_FOUND, "No log found for this logId");
}
List<SecurityAuditLogDTO> securityAuditLogsDTO = mapToDTOs(Collections.singletonList(securityEvent));
return SecurityLogResponseDTO.builder().totalLogs(1).currentLogs(1).securityAuditLogs(securityAuditLogsDTO)
.totalPages(1).currentPage(0).build();
}

if (page < 0) {
page = 0;
}
if (size <= 0) {
size = 15;
}
Comment on lines +69 to +74

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to add validation directly to the request parameters, such as @Min(0) for page and @Positive for size, instead of handling these validations manually?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually using these annotations completely flips the situation means here if page is negative then we are setting it to 0 by default and if size <= 0 then take as 15 by default but these annotations will just validate these values and if not verifies then throw the exception or simple the error body instead of the default results we returning currently.

Second these annotations will probably not the best case if we need to throw the bad request body because first we need to register the the MethodValidationPostProcessor bean and add @Validator on controller class and then most hurdle will come on testing this because we using core 2.7.x which using spring 5.3.x so to test that we need to manually create the proxy for test case because MockMvc standaloneSetup not supports proxy for this spring version and manually creating is more mess instead if our goal is to validate or throw exception we can manually throw using the IllegalArgumentException.


Date start = UtilClass.parseDate(startDate, false);
Date end = UtilClass.parseDate(endDate, true);

List<AuditSecurityEvent> securityEvents = auditService.getSecurityEvents(eventType, username, start, end, page,
size);
long totalCount = auditService.countSecurityEvents(eventType, username, start, end);
int totalPages = UtilClass.computeTotalPages(totalCount, size);

List<SecurityAuditLogDTO> securityAuditLogsDTO = mapToDTOs(securityEvents);

return SecurityLogResponseDTO.builder().totalLogs(totalCount).currentLogs(securityAuditLogsDTO.size())
.totalPages(totalPages).currentPage(page).securityAuditLogs(securityAuditLogsDTO).build();
}

@GetMapping("/relatedAudits")
public SecurityLogResponseDTO fetchRelatedAudits(@RequestParam(value = "sessionId") String sessionId,
@RequestParam(value = "page", defaultValue = "0") int page,
@RequestParam(value = "size", defaultValue = "15") int size) {

if (sessionId == null || sessionId.isEmpty()) {
throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Invalid session id");
}

if (page < 0) {
page = 0;
}
if (size <= 0) {
size = 15;
}

List<AuditSecurityEvent> allRelated = auditService.getRelatedSecurityEvents(sessionId, page, size);
long totalCount = auditService.countRelatedSecurityEvents(sessionId);
int totalPages = UtilClass.computeTotalPages(totalCount, size);

List<SecurityAuditLogDTO> securityAuditLogsDTO = mapToDTOs(allRelated);

return SecurityLogResponseDTO.builder().totalLogs(totalCount).currentLogs(securityAuditLogsDTO.size())
.totalPages(totalPages).currentPage(page).securityAuditLogs(securityAuditLogsDTO).build();
}

private SecurityAuditLogDTO mapToDTO(AuditSecurityEvent event) {
if (event == null) {
return null;
}
return SecurityAuditLogDTO.builder().id(event.getId()).eventType(event.getEventType()).username(event.getUsername())
.userUuid(event.getUserUuid()).eventTime(event.getEventTime()).ipAddress(event.getIpAddress())
.userAgent(event.getUserAgent()).sessionId(event.getSessionId()).details(event.getDetails()).build();
}

private List<SecurityAuditLogDTO> mapToDTOs(List<AuditSecurityEvent> events) {
if (events == null) {
return Collections.emptyList();
}
List<SecurityAuditLogDTO> dtos = new ArrayList<>(events.size());
for (AuditSecurityEvent event : events) {
dtos.add(mapToDTO(event));
}
return dtos;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
package org.openmrs.module.auditlogweb.rest.exceptions;

import org.hibernate.ObjectNotFoundException;
import org.openmrs.api.APIAuthenticationException;
import org.openmrs.api.context.Context;
import org.openmrs.module.auditlogweb.api.exception.AuditLogUnavailableException;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
Expand Down Expand Up @@ -84,6 +86,14 @@ public ResponseEntity<Map<String, String>> handleAuditLogUnavailable(AuditLogUna
return buildResponseEntity("Audit Log Unavailable", ex.getMessage(), HttpStatus.SERVICE_UNAVAILABLE);
}

@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);
}
Comment on lines +89 to +95

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
@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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, fixed !


@ExceptionHandler(Exception.class)
public ResponseEntity<Map<String, String>> handleGeneralError(Exception ex) {
return buildResponseEntity("Internal Server Error", "An unexpected error occurred",
Expand Down
Loading