fix(security): gate guest self-confirmed orders behind PENDING_REVIEW (#18) #30
|
|
@ -3,6 +3,7 @@ package se.bilhalsning.entity;
|
||||||
public enum OrderStatus {
|
public enum OrderStatus {
|
||||||
PENDING_PAYMENT("pending_payment"),
|
PENDING_PAYMENT("pending_payment"),
|
||||||
PAID("paid"),
|
PAID("paid"),
|
||||||
|
PENDING_REVIEW("pending_review"),
|
||||||
PROCESSING("processing"),
|
PROCESSING("processing"),
|
||||||
SENT("sent"),
|
SENT("sent"),
|
||||||
DELIVERED("delivered"),
|
DELIVERED("delivered"),
|
||||||
|
|
|
||||||
|
|
@ -53,6 +53,7 @@ public final class AdminOrderStatusRules {
|
||||||
private static List<OrderStatus> allowedTargets(OrderStatus from, Order order) {
|
private static List<OrderStatus> allowedTargets(OrderStatus from, Order order) {
|
||||||
return switch (from) {
|
return switch (from) {
|
||||||
case PENDING_PAYMENT -> List.of(OrderStatus.FAILED);
|
case PENDING_PAYMENT -> List.of(OrderStatus.FAILED);
|
||||||
|
case PENDING_REVIEW -> List.of(OrderStatus.PROCESSING, OrderStatus.CANCELLED, OrderStatus.FAILED);
|
||||||
case PROCESSING -> List.of(OrderStatus.FAILED);
|
case PROCESSING -> List.of(OrderStatus.FAILED);
|
||||||
case SENT -> List.of(OrderStatus.DELIVERED, OrderStatus.FAILED);
|
case SENT -> List.of(OrderStatus.DELIVERED, OrderStatus.FAILED);
|
||||||
case DELIVERED -> List.of(OrderStatus.FAILED);
|
case DELIVERED -> List.of(OrderStatus.FAILED);
|
||||||
|
|
|
||||||
|
|
@ -34,6 +34,9 @@ public class AdminOrderWorkflowService {
|
||||||
|
hermes
commented
💡 Test gap + readability nit. This branch is the load-bearing fix for #18 (re-fires the 'Behandlas' notification only when an admin vouches for the order), and it is not covered by 💡 **Test gap + readability nit.** This branch is the load-bearing fix for #18 (re-fires the 'Behandlas' notification only when an admin vouches for the order), and it is not covered by `AdminOrderWorkflowServiceTest` — the `notifyOrderFailed` branch above is tested, this one isn't. Consider adding `shouldNotifyProcessingWhenAdminAdvancesFromPendingReview`.
Also worth a one-line comment clarifying *why* `PROCESSING` from `PENDING_REVIEW` notifications where `PROCESSING` from `FAILED` does not (because `confirmGuestPayment` deliberately skipped the notify).
|
|||||||
if (newStatus == OrderStatus.FAILED && previousStatus != OrderStatus.FAILED) {
|
if (newStatus == OrderStatus.FAILED && previousStatus != OrderStatus.FAILED) {
|
||||||
orderNotificationService.notifyOrderFailed(saved);
|
orderNotificationService.notifyOrderFailed(saved);
|
||||||
}
|
}
|
||||||
|
if (newStatus == OrderStatus.PROCESSING && previousStatus == OrderStatus.PENDING_REVIEW) {
|
||||||
|
hermes
commented
💡 Test gap + readability nit. This branch is the load-bearing fix for #18 (re-fires the 'Behandlas' notification only when an admin vouches for the order), and it is not covered by 💡 **Test gap + readability nit.** This branch is the load-bearing fix for #18 (re-fires the 'Behandlas' notification only when an admin vouches for the order), and it is not covered by `AdminOrderWorkflowServiceTest` — the `notifyOrderFailed` branch above is tested, this one isn't. Consider adding `shouldNotifyProcessingWhenAdminAdvancesFromPendingReview`.
Also worth a one-line comment clarifying *why* `PROCESSING` from `PENDING_REVIEW` notifications where `PROCESSING` from `FAILED` does not (because `confirmGuestPayment` deliberately skipped the notify).
|
|||||||
|
orderNotificationService.notifyOrderProcessing(saved);
|
||||||
|
hermes
commented
💡 Test gap + readability nit. This branch is the load-bearing fix for #18 (re-fires the 'Behandlas' notification only when an admin vouches for the order), and it is not covered by 💡 **Test gap + readability nit.** This branch is the load-bearing fix for #18 (re-fires the 'Behandlas' notification only when an admin vouches for the order), and it is not covered by `AdminOrderWorkflowServiceTest` — the `notifyOrderFailed` branch above is tested, this one isn't. Consider adding `shouldNotifyProcessingWhenAdminAdvancesFromPendingReview`.
Also worth a one-line comment clarifying *why* `PROCESSING` from `PENDING_REVIEW` notifications where `PROCESSING` from `FAILED` does not (because `confirmGuestPayment` deliberately skipped the notify).
|
|||||||
|
}
|
||||||
|
hermes
commented
💡 Test gap + readability nit. This branch is the load-bearing fix for #18 (re-fires the 'Behandlas' notification only when an admin vouches for the order), and it is not covered by 💡 **Test gap + readability nit.** This branch is the load-bearing fix for #18 (re-fires the 'Behandlas' notification only when an admin vouches for the order), and it is not covered by `AdminOrderWorkflowServiceTest` — the `notifyOrderFailed` branch above is tested, this one isn't. Consider adding `shouldNotifyProcessingWhenAdminAdvancesFromPendingReview`.
Also worth a one-line comment clarifying *why* `PROCESSING` from `PENDING_REVIEW` notifications where `PROCESSING` from `FAILED` does not (because `confirmGuestPayment` deliberately skipped the notify).
|
|||||||
return saved;
|
return saved;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
hermes
commented
💡 Test gap + readability nit. This branch is the load-bearing fix for #18 (re-fires the 'Behandlas' notification only when an admin vouches for the order), and it is not covered by 💡 **Test gap + readability nit.** This branch is the load-bearing fix for #18 (re-fires the 'Behandlas' notification only when an admin vouches for the order), and it is not covered by `AdminOrderWorkflowServiceTest` — the `notifyOrderFailed` branch above is tested, this one isn't. Consider adding `shouldNotifyProcessingWhenAdminAdvancesFromPendingReview`.
Also worth a one-line comment clarifying *why* `PROCESSING` from `PENDING_REVIEW` notifications where `PROCESSING` from `FAILED` does not (because `confirmGuestPayment` deliberately skipped the notify).
hermes
commented
💡 Test gap + readability nit. This branch is the load-bearing fix for #18 (re-fires the 'Behandlas' notification only when an admin vouches for the order), and it is not covered by 💡 **Test gap + readability nit.** This branch is the load-bearing fix for #18 (re-fires the 'Behandlas' notification only when an admin vouches for the order), and it is not covered by `AdminOrderWorkflowServiceTest` — the `notifyOrderFailed` branch above is tested, this one isn't. Consider adding `shouldNotifyProcessingWhenAdminAdvancesFromPendingReview`.
Also worth a one-line comment clarifying *why* `PROCESSING` from `PENDING_REVIEW` notifications where `PROCESSING` from `FAILED` does not (because `confirmGuestPayment` deliberately skipped the notify).
|
|||||||
|
|
@ -64,6 +64,14 @@ public class OrderService {
|
||||||
* Honor-system payment confirmation for guest orders. Mirrors
|
* Honor-system payment confirmation for guest orders. Mirrors
|
||||||
* {@link #confirmPayment(UUID, UUID)} but authenticates via the
|
* {@link #confirmPayment(UUID, UUID)} but authenticates via the
|
||||||
* guest token instead of {@code userId}.
|
* guest token instead of {@code userId}.
|
||||||
|
*
|
||||||
|
* <p>Guest self-confirmed orders enter {@link OrderStatus#PENDING_REVIEW}
|
||||||
|
* rather than {@link OrderStatus#PROCESSING} because the payment is
|
||||||
|
* not verified — the customer simply clicked "I have paid" on the
|
||||||
|
* payment page. An admin must manually review and advance the order
|
||||||
|
* to PROCESSING (which triggers fulfillment notification). This
|
||||||
|
* prevents a guest from marking an order paid without paying and
|
||||||
|
* having it automatically sent to fulfillment.
|
||||||
*/
|
*/
|
||||||
public Order confirmGuestPayment(UUID guestToken) {
|
public Order confirmGuestPayment(UUID guestToken) {
|
||||||
Order order = getOrderByGuestToken(guestToken);
|
Order order = getOrderByGuestToken(guestToken);
|
||||||
|
|
@ -71,10 +79,8 @@ public class OrderService {
|
||||||
throw new InvalidOrderStateException(
|
throw new InvalidOrderStateException(
|
||||||
"Beställningen kan inte ändras i detta tillstånd");
|
"Beställningen kan inte ändras i detta tillstånd");
|
||||||
}
|
}
|
||||||
order.setStatus(OrderStatus.PROCESSING);
|
order.setStatus(OrderStatus.PENDING_REVIEW);
|
||||||
Order saved = orderRepository.save(order);
|
return orderRepository.save(order);
|
||||||
orderNotificationService.notifyOrderProcessing(saved);
|
|
||||||
return saved;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
public List<Order> getOrdersByUserId(UUID userId) {
|
public List<Order> getOrdersByUserId(UUID userId) {
|
||||||
|
|
|
||||||
|
|
@ -30,6 +30,32 @@ class AdminOrderStatusRulesTest {
|
||||||
assertFalse(AdminOrderStatusRules.canRegisterShipment(order));
|
assertFalse(AdminOrderStatusRules.canRegisterShipment(order));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void shouldAllowProcessingCancelledFailedFromPendingReview() {
|
||||||
|
Order order = orderWithStatus(OrderStatus.PENDING_REVIEW);
|
||||||
|
|
||||||
|
assertEquals(
|
||||||
|
java.util.List.of("pending_review", "processing", "cancelled", "failed"),
|
||||||
|
AdminOrderStatusRules.allowedStatusValues(order));
|
||||||
|
assertFalse(AdminOrderStatusRules.canRegisterShipment(order));
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void shouldValidatePendingReviewToProcessingTransition() {
|
||||||
|
Order order = orderWithStatus(OrderStatus.PENDING_REVIEW);
|
||||||
|
|
||||||
|
assertDoesNotThrow(() ->
|
||||||
|
AdminOrderStatusRules.validateTransition(order, OrderStatus.PROCESSING));
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void shouldRejectPendingReviewToSentTransition() {
|
||||||
|
Order order = orderWithStatus(OrderStatus.PENDING_REVIEW);
|
||||||
|
|
||||||
|
assertThrows(se.bilhalsning.exception.InvalidOrderStateException.class,
|
||||||
|
() -> AdminOrderStatusRules.validateTransition(order, OrderStatus.SENT));
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void shouldExposeFailedRecoveryOptionsWhenTrackingExists() {
|
void shouldExposeFailedRecoveryOptionsWhenTrackingExists() {
|
||||||
Order order = orderWithStatus(OrderStatus.FAILED);
|
Order order = orderWithStatus(OrderStatus.FAILED);
|
||||||
|
|
|
||||||
|
|
@ -344,8 +344,8 @@ class OrderServiceTest {
|
||||||
|
|
||||||
Order result = orderService.confirmGuestPayment(token);
|
Order result = orderService.confirmGuestPayment(token);
|
||||||
|
|
||||||
assertEquals(OrderStatus.PROCESSING, result.getStatus());
|
assertEquals(OrderStatus.PENDING_REVIEW, result.getStatus());
|
||||||
verify(orderNotificationService).notifyOrderProcessing(result);
|
verify(orderNotificationService, never()).notifyOrderProcessing(any());
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
|
|
|
||||||
|
|
@ -15,6 +15,8 @@ const statusLabel = computed(() => {
|
||||||
|
hermes
commented
💡 Frontend test gap. 💡 **Frontend test gap.** `GuestOrderPage.spec.ts` asserts labels for `paid`/`sent`/`delivered`/`failed`/`cancelled` but not for the new `pending_review` case (renders `'Betalning mottagen, under granskning'`). The switch `default` keeps existing tests green, but this new label is uncovered — worth adding `it('shows "Betalning mottagen, under granskning" for pending_review status')`.
hermes
commented
💡 Frontend test gap. 💡 **Frontend test gap.** `GuestOrderPage.spec.ts` asserts labels for `paid`/`sent`/`delivered`/`failed`/`cancelled` but not for the new `pending_review` case (renders `'Betalning mottagen, under granskning'`). The switch `default` keeps existing tests green, but this new label is uncovered — worth adding `it('shows "Betalning mottagen, under granskning" for pending_review status')`.
|
|||||||
switch (order.value.status) {
|
switch (order.value.status) {
|
||||||
case 'pending_payment':
|
case 'pending_payment':
|
||||||
return 'Väntar på betalning'
|
return 'Väntar på betalning'
|
||||||
|
case 'pending_review':
|
||||||
|
hermes
commented
💡 Frontend test gap. 💡 **Frontend test gap.** `GuestOrderPage.spec.ts` asserts labels for `paid`/`sent`/`delivered`/`failed`/`cancelled` but not for the new `pending_review` case (renders `'Betalning mottagen, under granskning'`). The switch `default` keeps existing tests green, but this new label is uncovered — worth adding `it('shows "Betalning mottagen, under granskning" for pending_review status')`.
|
|||||||
|
return 'Betalning mottagen, under granskning'
|
||||||
|
hermes
commented
💡 Frontend test gap. 💡 **Frontend test gap.** `GuestOrderPage.spec.ts` asserts labels for `paid`/`sent`/`delivered`/`failed`/`cancelled` but not for the new `pending_review` case (renders `'Betalning mottagen, under granskning'`). The switch `default` keeps existing tests green, but this new label is uncovered — worth adding `it('shows "Betalning mottagen, under granskning" for pending_review status')`.
|
|||||||
case 'paid':
|
case 'paid':
|
||||||
case 'processing':
|
case 'processing':
|
||||||
return 'Behandlas'
|
return 'Behandlas'
|
||||||
|
|
|
||||||
|
hermes
commented
💡 Frontend test gap. 💡 **Frontend test gap.** `GuestOrderPage.spec.ts` asserts labels for `paid`/`sent`/`delivered`/`failed`/`cancelled` but not for the new `pending_review` case (renders `'Betalning mottagen, under granskning'`). The switch `default` keeps existing tests green, but this new label is uncovered — worth adding `it('shows "Betalning mottagen, under granskning" for pending_review status')`.
hermes
commented
💡 Frontend test gap. 💡 **Frontend test gap.** `GuestOrderPage.spec.ts` asserts labels for `paid`/`sent`/`delivered`/`failed`/`cancelled` but not for the new `pending_review` case (renders `'Betalning mottagen, under granskning'`). The switch `default` keeps existing tests green, but this new label is uncovered — worth adding `it('shows "Betalning mottagen, under granskning" for pending_review status')`.
|
|||||||
💡 Test gap + readability nit. This branch is the load-bearing fix for #18 (re-fires the 'Behandlas' notification only when an admin vouches for the order), and it is not covered by
AdminOrderWorkflowServiceTest— thenotifyOrderFailedbranch above is tested, this one isn't. Consider addingshouldNotifyProcessingWhenAdminAdvancesFromPendingReview.Also worth a one-line comment clarifying why
PROCESSINGfromPENDING_REVIEWnotifications wherePROCESSINGfromFAILEDdoes not (becauseconfirmGuestPaymentdeliberately skipped the notify).