fix(security): gate guest self-confirmed orders behind PENDING_REVIEW (#18) #30

Open
hermes wants to merge 1 commit from fix/guest-payment-pending-review into master
7 changed files with 45 additions and 6 deletions

View file

@ -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"),

View file

@ -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);

View file

@ -34,6 +34,9 @@ public class AdminOrderWorkflowService {
Review

💡 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).

💡 **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).
Review

💡 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).

💡 **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) {
Review

💡 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).

💡 **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);
Review

💡 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).

💡 **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).
}
Review

💡 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).

💡 **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;
} }

Review

💡 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).

💡 **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).
Review

💡 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).

💡 **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).

View file

@ -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) {

View file

@ -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);

View file

@ -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

View file

@ -15,6 +15,8 @@ const statusLabel = computed(() => {
Review

💡 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').

💡 **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')`.
Review

💡 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').

💡 **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':
Review

💡 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').

💡 **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'
Review

💡 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').

💡 **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'

Review

💡 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').

💡 **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')`.
Review

💡 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').

💡 **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')`.