Skip to content

S28 3951 Allow SuperUsers to perform more edit actions #1178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

S28 3951 Allow SuperUsers to perform more edit actions #1178

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2c6016a
Fix share id tests
GeorgeBarnesUK Aug 11, 2025
aaaed35
Merge branch 'master' into S28-3951/allow-admins-extra-api-actions-2
GeorgeBarnesUK Aug 11, 2025
4904845
Fix after master pull
GeorgeBarnesUK Aug 11, 2025
dd06f87
Fix typo
GeorgeBarnesUK Aug 11, 2025
93ffaec
Add missing from stash
GeorgeBarnesUK Aug 11, 2025
5132d4e
Merge branch 'master' of https://github.com/hmcts/pre-api into S28-39…
GeorgeBarnesUK Sep 22, 2025
0adf055
Style&Test fixes
GeorgeBarnesUK Sep 22, 2025
b8bfc4f
Remove redundant tests in BookingController
GeorgeBarnesUK Sep 24, 2025
ab69cb8
Merge branch 'master' of https://github.com/hmcts/pre-api into S28-39…
GeorgeBarnesUK Sep 24, 2025
85a7280
checkstyle cleanup
GeorgeBarnesUK Sep 24, 2025
ecbd49e
Functional Test perms fix
GeorgeBarnesUK Sep 24, 2025
2934384
Checkstyle
GeorgeBarnesUK Sep 24, 2025
3a9781a
Merge branch 'master' of https://github.com/hmcts/pre-api into S28-39…
GeorgeBarnesUK Sep 26, 2025
8c0e491
Merge branch 'master' into S28-3951/allow-admins-extra-api-actions-2
GeorgeBarnesUK Sep 29, 2025
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
1 change: 0 additions & 1 deletion src/main/java/uk/gov/hmcts/reform/preapi/controllers/BookingController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,6 @@ public ResponseEntity<Void> undeleteBooking(@PathVariable UUID bookingId) {
return ok().build();
}


private void validateRequestWithBody(UUID bookingId, CreateBookingDTO createBookingDTO) {
if (!bookingId.equals(createBookingDTO.getId())) {
throw new PathPayloadMismatchException("bookingId", "bookingDTO.id");
Expand Down
2 changes: 0 additions & 2 deletions src/main/java/uk/gov/hmcts/reform/preapi/dto/CreateBookingDTO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import jakarta.validation.constraints.NotNull;
import lombok.Data;
import lombok.NoArgsConstructor;
import uk.gov.hmcts.reform.preapi.dto.validators.BookingScheduledForNotPastOrNotChangedConstraint;
import uk.gov.hmcts.reform.preapi.dto.validators.ParticipantTypeConstraint;
import uk.gov.hmcts.reform.preapi.entities.Booking;

Expand All @@ -20,7 +19,6 @@
@NoArgsConstructor
@Schema(description = "CreateBookingDTO")
@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class)
@BookingScheduledForNotPastOrNotChangedConstraint(message = "scheduled_for is required and must not be before today")
public class CreateBookingDTO {
@Schema(description = "CreateBookingId")
@NotNull(message = "id is required")
Expand Down
20 changes: 0 additions & 20 deletions src/main/java/uk/gov/hmcts/reform/preapi/dto/validators/NotPastDateConstraint.java
View file
Open in desktop

This file was deleted.

27 changes: 0 additions & 27 deletions src/main/java/uk/gov/hmcts/reform/preapi/dto/validators/NotPastDateValidator.java
View file
Open in desktop

This file was deleted.

3 changes: 2 additions & 1 deletion src/main/java/uk/gov/hmcts/reform/preapi/security/AuthorisationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,8 @@ public boolean hasUpsertAccess(UserAuthentication authentication, CreateRecordin

public boolean hasUpsertAccess(UserAuthentication authentication, CreateShareBookingDTO dto) {
return authentication.getUserId().equals(dto.getSharedByUser())
&& (authentication.isAdmin() || hasBookingAccess(authentication, dto.getBookingId()));
&& (authentication.isAdmin() || hasBookingAccess(authentication, dto.getBookingId()))
|| authentication.hasRole("ROLE_SUPER_USER");
}

public boolean hasUpsertAccess(UserAuthentication authentication, CreateCaseDTO dto) {
Expand Down
14 changes: 13 additions & 1 deletion src/main/java/uk/gov/hmcts/reform/preapi/services/BookingService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import uk.gov.hmcts.reform.preapi.enums.CaseState;
import uk.gov.hmcts.reform.preapi.enums.RecordingStatus;
import uk.gov.hmcts.reform.preapi.enums.UpsertResult;
import uk.gov.hmcts.reform.preapi.exception.BadRequestException;
import uk.gov.hmcts.reform.preapi.exception.NotFoundException;
import uk.gov.hmcts.reform.preapi.exception.ResourceInDeletedStateException;
import uk.gov.hmcts.reform.preapi.exception.ResourceInWrongStateException;
Expand All @@ -30,6 +31,8 @@
import java.sql.Timestamp;
import java.time.Instant;
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.ZoneId;
import java.time.temporal.ChronoUnit;
import java.util.List;
import java.util.Optional;
Expand Down Expand Up @@ -126,6 +129,15 @@ public Page<BookingDTO> searchBy(
@Transactional
@PreAuthorize("@authorisationService.hasUpsertAccess(authentication, #createBookingDTO)")
public UpsertResult upsert(CreateBookingDTO createBookingDTO) {
var auth = ((UserAuthentication) SecurityContextHolder.getContext().getAuthentication());

var localDateField = LocalDateTime.ofInstant(createBookingDTO.getScheduledFor().toInstant(), ZoneId.of("Europe/London")).toLocalDate();
var today = LocalDate.now();

if (localDateField.isBefore(today)
Copy link
Contributor

@lydiaralphgov lydiaralphgov Oct 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think this adds a restriction to the API that wasn't there before, i.e. it adds a guard to make sure bookings are in the future. That's fine, but we need to create a separate ticket for this piece of code and put it in front of Jacob to make sure it's been signed off by service as its a behavioural change. edit: sorry I should have read the whole PR first!

I agree we need the Super User exception to allow us to recover cases.

&& !auth.hasRole("ROLE_SUPER_USER")) {
throw new BadRequestException("Scheduled date must not be in the past");
}

if (bookingAlreadyDeleted(createBookingDTO.getId())) {
throw new ResourceInDeletedStateException("BookingDTO", createBookingDTO.getId().toString());
Expand All @@ -137,7 +149,7 @@ public UpsertResult upsert(CreateBookingDTO createBookingDTO) {
var caseEntity = caseRepository.findByIdAndDeletedAtIsNull(createBookingDTO.getCaseId())
.orElseThrow(() -> new NotFoundException("Case: " + createBookingDTO.getCaseId()));

if (caseEntity.getState() != CaseState.OPEN) {
if (caseEntity.getState() != CaseState.OPEN && !auth.hasRole("ROLE_SUPER_USER")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree that we should have this exception, as I think we need to avoid modifying closed cases altogether

throw new ResourceInWrongStateException(
"Booking",
createBookingDTO.getId(),
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/uk/gov/hmcts/reform/preapi/services/CaseService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,12 +119,15 @@ public UpsertResult upsert(CreateCaseDTO createCaseDTO) {
var isCaseClosureCancellation = false;
var isCasePendingClosure = false;

var auth = ((UserAuthentication) SecurityContextHolder.getContext().getAuthentication());

if (isUpdate) {
if (foundCase.get().isDeleted()) {
throw new ResourceInDeletedStateException("CaseDTO", createCaseDTO.getId().toString());
}
if (foundCase.get().getState() != CaseState.OPEN
&& foundCase.get().getState() == createCaseDTO.getState()
&& !auth.hasRole("ROLE_SUPER_USER")
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto - I don't like this superpower

) {
throw new ResourceInWrongStateException(
"Resource Case("
Expand Down
112 changes: 56 additions & 56 deletions src/test/java/uk/gov/hmcts/reform/preapi/dto/validators/NotPastDateValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,56 +1,56 @@
package uk.gov.hmcts.reform.preapi.dto.validators;

import jakarta.validation.ConstraintValidatorContext;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

import java.sql.Timestamp;
import java.time.ZoneId;
import java.time.ZonedDateTime;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class NotPastDateValidatorTest {
private NotPastDateValidator validator;
private ConstraintValidatorContext constraintValidatorContext;

@BeforeEach
void setUp() {
validator = new NotPastDateValidator();
constraintValidatorContext = null;
}

@DisplayName("Should return false when value is null")
@Test
void isValidWithDateNull() {
assertFalse(validator.isValid(null, constraintValidatorContext));
}

@DisplayName("Should return false when value is in the past")
@Test
void isValidWithDateInPast() {
ZonedDateTime pastDateTime = ZonedDateTime.now(ZoneId.of("Europe/London"))
.minusDays(1);
Timestamp pastTimestamp = Timestamp.from(pastDateTime.toInstant());
assertFalse(validator.isValid(pastTimestamp, constraintValidatorContext));
}

@DisplayName("Should return true when value is not in the past")
@Test
void isValidWithDateToday() {
ZonedDateTime currentDateTime = ZonedDateTime.now(ZoneId.of("Europe/London"));
Timestamp currentTimestamp = Timestamp.from(currentDateTime.toInstant());
assertTrue(validator.isValid(currentTimestamp, constraintValidatorContext));
}

@DisplayName("Should return true when value is in the future")
@Test
void givenFutureDate_ReturnsTrue() {
ZonedDateTime futureDateTime = ZonedDateTime.now(ZoneId.of("Europe/London"))
.plusDays(1);
Timestamp futureTimestamp = Timestamp.from(futureDateTime.toInstant());
assertTrue(validator.isValid(futureTimestamp, constraintValidatorContext));
}
}
//package uk.gov.hmcts.reform.preapi.dto.validators;
//
//import jakarta.validation.ConstraintValidatorContext;
//import org.junit.jupiter.api.BeforeEach;
//import org.junit.jupiter.api.DisplayName;
//import org.junit.jupiter.api.Test;
//
//import java.sql.Timestamp;
//import java.time.ZoneId;
//import java.time.ZonedDateTime;
//
//import static org.junit.jupiter.api.Assertions.assertFalse;
//import static org.junit.jupiter.api.Assertions.assertTrue;
//
//public class NotPastDateValidatorTest {
// private NotPastDateValidator validator;
// private ConstraintValidatorContext constraintValidatorContext;
//
// @BeforeEach
// void setUp() {
// validator = new NotPastDateValidator();
// constraintValidatorContext = null;
// }
//
// @DisplayName("Should return false when value is null")
// @Test
// void isValidWithDateNull() {
// assertFalse(validator.isValid(null, constraintValidatorContext));
// }
//
// @DisplayName("Should return false when value is in the past")
// @Test
// void isValidWithDateInPast() {
// ZonedDateTime pastDateTime = ZonedDateTime.now(ZoneId.of("Europe/London"))
// .minusDays(1);
// Timestamp pastTimestamp = Timestamp.from(pastDateTime.toInstant());
// assertFalse(validator.isValid(pastTimestamp, constraintValidatorContext));
// }
//
// @DisplayName("Should return true when value is not in the past")
// @Test
// void isValidWithDateToday() {
// ZonedDateTime currentDateTime = ZonedDateTime.now(ZoneId.of("Europe/London"));
// Timestamp currentTimestamp = Timestamp.from(currentDateTime.toInstant());
// assertTrue(validator.isValid(currentTimestamp, constraintValidatorContext));
// }
//
// @DisplayName("Should return true when value is in the future")
// @Test
// void givenFutureDate_ReturnsTrue() {
// ZonedDateTime futureDateTime = ZonedDateTime.now(ZoneId.of("Europe/London"))
// .plusDays(1);
// Timestamp futureTimestamp = Timestamp.from(futureDateTime.toInstant());
// assertTrue(validator.isValid(futureTimestamp, constraintValidatorContext));
// }
//}
15 changes: 15 additions & 0 deletions src/test/java/uk/gov/hmcts/reform/preapi/security/AuthorisationServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,21 @@ void hasUpsertAccessUserIsNotSharing() {
assertFalse(authorisationService.hasUpsertAccess(authenticationUser, dto));
}

@DisplayName("Should grant upsert access when the authenticated super user is not the one sharing the booking,")
@Test
void hasUpsertAccessSuperUserIsNotSharing() {
var dto = new CreateShareBookingDTO();

dto.setSharedByUser(UUID.randomUUID());
dto.setBookingId(UUID.randomUUID());

when(authenticationUser.getUserId()).thenReturn(UUID.randomUUID());
when(authenticationUser.isAdmin()).thenReturn(false);
when(authenticationUser.hasRole("ROLE_SUPER_USER")).thenReturn(true);

assertTrue(authorisationService.hasUpsertAccess(authenticationUser, dto));
}

@DisplayName("Should grant upsert access when the authenticated user is an admin")
@Test
void hasUpsertAccessUserIsAdmin() {
Expand Down
Loading