feat(loyalty): production readiness round 2 — 12 security, integrity & correctness fixes
Some checks failed
Some checks failed
Security: - Fix TOCTOU race conditions: move balance/limit checks after row lock in redeem_points, add_stamp, redeem_stamps - Add PIN ownership verification to update/delete/unlock store routes - Gate adjust_points endpoint to merchant_owner role only Data integrity: - Track total_points_voided in void_points - Add order_reference idempotency guard in earn_points Correctness: - Fix LoyaltyProgramAlreadyExistsException to use merchant_id parameter - Add StorefrontProgramResponse excluding wallet IDs from public API - Add bounds (±100000) to PointsAdjustRequest.points_delta Audit & config: - Add CARD_REACTIVATED transaction type with audit record - Improve admin audit logging with actor identity and old values - Use merchant-specific PIN lockout settings with global fallback - Guard MerchantLoyaltySettings creation with get_or_create pattern Tests: 27 new tests (265 total) covering all 12 items — unit and integration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -290,3 +290,44 @@ class TestGetStoreTransactions:
|
||||
)
|
||||
assert total >= 3
|
||||
assert len(transactions) == 2
|
||||
|
||||
|
||||
# ============================================================================
|
||||
# Item 9: Reactivate card creates audit transaction
|
||||
# ============================================================================
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@pytest.mark.loyalty
|
||||
class TestReactivateCardAudit:
|
||||
"""Tests for reactivate_card creating CARD_REACTIVATED transaction."""
|
||||
|
||||
def setup_method(self):
|
||||
self.service = CardService()
|
||||
|
||||
def test_reactivate_creates_transaction(self, db, test_loyalty_card):
|
||||
"""Reactivating a card creates a CARD_REACTIVATED transaction."""
|
||||
# Deactivate first
|
||||
test_loyalty_card.is_active = False
|
||||
db.commit()
|
||||
|
||||
self.service.reactivate_card(db, test_loyalty_card.id)
|
||||
|
||||
tx = (
|
||||
db.query(LoyaltyTransaction)
|
||||
.filter(
|
||||
LoyaltyTransaction.card_id == test_loyalty_card.id,
|
||||
LoyaltyTransaction.transaction_type == TransactionType.CARD_REACTIVATED.value,
|
||||
)
|
||||
.first()
|
||||
)
|
||||
assert tx is not None
|
||||
assert tx.merchant_id == test_loyalty_card.merchant_id
|
||||
|
||||
def test_reactivate_sets_card_active(self, db, test_loyalty_card):
|
||||
"""Reactivating a card sets is_active to True."""
|
||||
test_loyalty_card.is_active = False
|
||||
db.commit()
|
||||
|
||||
card = self.service.reactivate_card(db, test_loyalty_card.id)
|
||||
assert card.is_active is True
|
||||
|
||||
@@ -233,3 +233,52 @@ class TestVerifyPin:
|
||||
# Should find the active PIN
|
||||
result = self.service.verify_pin(db, program.id, "2222", store_id=store.id)
|
||||
assert result.name == "Active"
|
||||
|
||||
|
||||
# ============================================================================
|
||||
# Item 11: Merchant-specific PIN lockout settings
|
||||
# ============================================================================
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@pytest.mark.loyalty
|
||||
class TestVerifyPinMerchantSettings:
|
||||
"""Tests for verify_pin using merchant-specific lockout settings."""
|
||||
|
||||
def setup_method(self):
|
||||
self.service = PinService()
|
||||
|
||||
def test_uses_merchant_lockout_attempts(self, db, pin_setup):
|
||||
"""Failed attempts use merchant settings, not global config."""
|
||||
from app.modules.loyalty.models import MerchantLoyaltySettings
|
||||
|
||||
program = pin_setup["program"]
|
||||
store = pin_setup["store"]
|
||||
merchant = pin_setup["merchant"]
|
||||
|
||||
# Create merchant settings with low lockout threshold
|
||||
settings = MerchantLoyaltySettings(
|
||||
merchant_id=merchant.id,
|
||||
staff_pin_lockout_attempts=3,
|
||||
staff_pin_lockout_minutes=60,
|
||||
)
|
||||
db.add(settings)
|
||||
db.commit()
|
||||
|
||||
self.service.create_pin(db, program.id, store.id, PinCreate(name="Test", pin="1234"))
|
||||
|
||||
# Fail 3 times (merchant setting), should lock
|
||||
for _ in range(3):
|
||||
try:
|
||||
self.service.verify_pin(db, program.id, "9999", store_id=store.id)
|
||||
except (InvalidStaffPinException, StaffPinLockedException):
|
||||
pass
|
||||
|
||||
# Next attempt should be locked
|
||||
with pytest.raises((InvalidStaffPinException, StaffPinLockedException)):
|
||||
self.service.verify_pin(db, program.id, "9999", store_id=store.id)
|
||||
|
||||
# Verify the PIN is actually locked
|
||||
pins = self.service.list_pins(db, program.id, store_id=store.id, is_active=True)
|
||||
locked_pins = [p for p in pins if p.is_locked]
|
||||
assert len(locked_pins) >= 1
|
||||
|
||||
@@ -372,3 +372,169 @@ class TestAdjustPoints:
|
||||
|
||||
assert result["success"] is True
|
||||
assert result["points_balance"] == 0
|
||||
|
||||
|
||||
# ============================================================================
|
||||
# Item 1: TOCTOU Race Condition (redeem_points checks after lock)
|
||||
# ============================================================================
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@pytest.mark.loyalty
|
||||
class TestRedeemPointsTOCTOU:
|
||||
"""Verify balance check happens after row lock."""
|
||||
|
||||
def setup_method(self):
|
||||
self.service = PointsService()
|
||||
|
||||
def test_redeem_insufficient_after_lock(self, db, points_setup):
|
||||
"""Balance check uses locked row state, not stale read."""
|
||||
card = points_setup["card"]
|
||||
store = points_setup["store"]
|
||||
|
||||
# Set balance to exactly the reward cost
|
||||
card.points_balance = 100
|
||||
db.commit()
|
||||
|
||||
# Should succeed — balance is exactly enough
|
||||
result = self.service.redeem_points(
|
||||
db, store_id=store.id, card_id=card.id, reward_id="r1",
|
||||
)
|
||||
assert result["success"] is True
|
||||
assert result["points_balance"] == 0
|
||||
|
||||
# Second redemption should fail (balance is now 0 after lock)
|
||||
with pytest.raises(InsufficientPointsException):
|
||||
self.service.redeem_points(
|
||||
db, store_id=store.id, card_id=card.id, reward_id="r1",
|
||||
)
|
||||
|
||||
|
||||
# ============================================================================
|
||||
# Item 4: void_points updates total_points_voided
|
||||
# ============================================================================
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@pytest.mark.loyalty
|
||||
class TestVoidPointsTracking:
|
||||
"""Verify void_points increments total_points_voided."""
|
||||
|
||||
def setup_method(self):
|
||||
self.service = PointsService()
|
||||
|
||||
def test_void_updates_total_points_voided(self, db, points_setup):
|
||||
"""total_points_voided is incremented on void."""
|
||||
card = points_setup["card"]
|
||||
store = points_setup["store"]
|
||||
|
||||
initial_voided = card.total_points_voided
|
||||
|
||||
self.service.void_points(
|
||||
db, store_id=store.id, card_id=card.id, points_to_void=50,
|
||||
)
|
||||
|
||||
db.refresh(card)
|
||||
assert card.total_points_voided == initial_voided + 50
|
||||
|
||||
def test_void_caps_voided_at_balance(self, db, points_setup):
|
||||
"""total_points_voided only counts actually voided amount (capped at balance)."""
|
||||
card = points_setup["card"]
|
||||
store = points_setup["store"]
|
||||
|
||||
# Set balance to 30, try to void 100
|
||||
card.points_balance = 30
|
||||
card.total_points_voided = 0
|
||||
db.commit()
|
||||
|
||||
self.service.void_points(
|
||||
db, store_id=store.id, card_id=card.id, points_to_void=100,
|
||||
)
|
||||
|
||||
db.refresh(card)
|
||||
assert card.total_points_voided == 30 # Only 30 was available
|
||||
assert card.points_balance == 0
|
||||
|
||||
|
||||
# ============================================================================
|
||||
# Item 5: Duplicate order_reference guard
|
||||
# ============================================================================
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@pytest.mark.loyalty
|
||||
class TestEarnPointsIdempotency:
|
||||
"""Verify duplicate order_reference returns existing result."""
|
||||
|
||||
def setup_method(self):
|
||||
self.service = PointsService()
|
||||
|
||||
def test_duplicate_order_reference_returns_existing(self, db, points_setup):
|
||||
"""Same order_reference returns existing result without double-earning."""
|
||||
card = points_setup["card"]
|
||||
store = points_setup["store"]
|
||||
|
||||
result1 = self.service.earn_points(
|
||||
db,
|
||||
store_id=store.id,
|
||||
card_id=card.id,
|
||||
purchase_amount_cents=1000,
|
||||
order_reference="ORDER-DUP-001",
|
||||
)
|
||||
balance_after_first = result1["points_balance"]
|
||||
|
||||
result2 = self.service.earn_points(
|
||||
db,
|
||||
store_id=store.id,
|
||||
card_id=card.id,
|
||||
purchase_amount_cents=1000,
|
||||
order_reference="ORDER-DUP-001",
|
||||
)
|
||||
|
||||
# Second call should return same points_earned, balance unchanged
|
||||
assert result2["points_earned"] == result1["points_earned"]
|
||||
assert result2["points_balance"] == balance_after_first
|
||||
assert result2["message"] == "Points already earned for this order"
|
||||
|
||||
def test_different_order_references_earn_separately(self, db, points_setup):
|
||||
"""Different order_references earn points independently."""
|
||||
card = points_setup["card"]
|
||||
store = points_setup["store"]
|
||||
|
||||
result1 = self.service.earn_points(
|
||||
db,
|
||||
store_id=store.id,
|
||||
card_id=card.id,
|
||||
purchase_amount_cents=1000,
|
||||
order_reference="ORDER-A",
|
||||
)
|
||||
result2 = self.service.earn_points(
|
||||
db,
|
||||
store_id=store.id,
|
||||
card_id=card.id,
|
||||
purchase_amount_cents=1000,
|
||||
order_reference="ORDER-B",
|
||||
)
|
||||
|
||||
assert result2["points_balance"] > result1["points_balance"]
|
||||
assert result2["message"] == "Points earned successfully"
|
||||
|
||||
def test_no_order_reference_allows_multiple_earns(self, db, points_setup):
|
||||
"""Without order_reference, multiple earns are allowed."""
|
||||
card = points_setup["card"]
|
||||
store = points_setup["store"]
|
||||
|
||||
result1 = self.service.earn_points(
|
||||
db,
|
||||
store_id=store.id,
|
||||
card_id=card.id,
|
||||
purchase_amount_cents=1000,
|
||||
)
|
||||
result2 = self.service.earn_points(
|
||||
db,
|
||||
store_id=store.id,
|
||||
card_id=card.id,
|
||||
purchase_amount_cents=1000,
|
||||
)
|
||||
|
||||
assert result2["points_balance"] > result1["points_balance"]
|
||||
|
||||
@@ -236,6 +236,38 @@ class TestCreateProgram:
|
||||
settings = self.service.get_merchant_settings(db, ps_merchant.id)
|
||||
assert settings is not None
|
||||
|
||||
def test_create_program_with_existing_settings_no_error(self, db, ps_merchant):
|
||||
"""Creating a program when MerchantLoyaltySettings already exists succeeds (Item 12)."""
|
||||
from app.modules.loyalty.models import MerchantLoyaltySettings
|
||||
|
||||
# Pre-create settings
|
||||
settings = MerchantLoyaltySettings(merchant_id=ps_merchant.id)
|
||||
db.add(settings)
|
||||
db.commit()
|
||||
|
||||
# Should not raise IntegrityError
|
||||
data = ProgramCreate(loyalty_type="points")
|
||||
program = self.service.create_program(db, ps_merchant.id, data)
|
||||
assert program is not None
|
||||
|
||||
# Should still have exactly one settings row
|
||||
count = (
|
||||
db.query(MerchantLoyaltySettings)
|
||||
.filter(MerchantLoyaltySettings.merchant_id == ps_merchant.id)
|
||||
.count()
|
||||
)
|
||||
assert count == 1
|
||||
|
||||
def test_duplicate_program_exception_uses_merchant_id(self, db, ps_program, ps_merchant):
|
||||
"""LoyaltyProgramAlreadyExistsException message references merchant, not store (Item 6)."""
|
||||
data = ProgramCreate(loyalty_type="points")
|
||||
with pytest.raises(LoyaltyProgramAlreadyExistsException) as exc_info:
|
||||
self.service.create_program(db, ps_merchant.id, data)
|
||||
|
||||
assert "Merchant" in str(exc_info.value.message)
|
||||
assert str(ps_merchant.id) in str(exc_info.value.message)
|
||||
assert exc_info.value.details["merchant_id"] == ps_merchant.id
|
||||
|
||||
|
||||
# ============================================================================
|
||||
# Update Operations
|
||||
|
||||
@@ -3,11 +3,14 @@
|
||||
from datetime import UTC, datetime
|
||||
|
||||
import pytest
|
||||
from pydantic import ValidationError
|
||||
|
||||
from app.modules.loyalty.schemas.card import (
|
||||
CardEnrollRequest,
|
||||
TransactionResponse,
|
||||
)
|
||||
from app.modules.loyalty.schemas.points import PointsAdjustRequest
|
||||
from app.modules.loyalty.schemas.program import StorefrontProgramResponse
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@@ -73,3 +76,104 @@ class TestTransactionResponse:
|
||||
created_at=now,
|
||||
)
|
||||
assert tx.customer_name == "John Doe"
|
||||
|
||||
|
||||
# ============================================================================
|
||||
# Item 8: PointsAdjustRequest bounds
|
||||
# ============================================================================
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@pytest.mark.loyalty
|
||||
class TestPointsAdjustRequestBounds:
|
||||
"""Tests for PointsAdjustRequest.points_delta bounds."""
|
||||
|
||||
def test_valid_positive_delta(self):
|
||||
"""Positive delta within bounds is valid."""
|
||||
req = PointsAdjustRequest(points_delta=100, reason="Bonus for customer")
|
||||
assert req.points_delta == 100
|
||||
|
||||
def test_valid_negative_delta(self):
|
||||
"""Negative delta within bounds is valid."""
|
||||
req = PointsAdjustRequest(points_delta=-500, reason="Correction needed")
|
||||
assert req.points_delta == -500
|
||||
|
||||
def test_max_boundary(self):
|
||||
"""Delta at max boundary (100000) is valid."""
|
||||
req = PointsAdjustRequest(points_delta=100000, reason="Max allowed delta")
|
||||
assert req.points_delta == 100000
|
||||
|
||||
def test_min_boundary(self):
|
||||
"""Delta at min boundary (-100000) is valid."""
|
||||
req = PointsAdjustRequest(points_delta=-100000, reason="Min allowed delta")
|
||||
assert req.points_delta == -100000
|
||||
|
||||
def test_exceeds_max_rejected(self):
|
||||
"""Delta exceeding 100000 is rejected."""
|
||||
with pytest.raises(ValidationError):
|
||||
PointsAdjustRequest(points_delta=100001, reason="Too many points")
|
||||
|
||||
def test_exceeds_min_rejected(self):
|
||||
"""Delta below -100000 is rejected."""
|
||||
with pytest.raises(ValidationError):
|
||||
PointsAdjustRequest(points_delta=-100001, reason="Too many negative")
|
||||
|
||||
|
||||
# ============================================================================
|
||||
# Item 7: StorefrontProgramResponse excludes wallet fields
|
||||
# ============================================================================
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@pytest.mark.loyalty
|
||||
class TestStorefrontProgramResponse:
|
||||
"""Tests for StorefrontProgramResponse excluding wallet IDs."""
|
||||
|
||||
def test_wallet_fields_excluded_from_serialization(self):
|
||||
"""Wallet integration IDs are excluded from JSON output."""
|
||||
now = datetime.now(UTC)
|
||||
response = StorefrontProgramResponse(
|
||||
id=1,
|
||||
merchant_id=1,
|
||||
loyalty_type="points",
|
||||
stamps_target=10,
|
||||
stamps_reward_description="Free item",
|
||||
points_per_euro=10,
|
||||
cooldown_minutes=15,
|
||||
max_daily_stamps=5,
|
||||
require_staff_pin=True,
|
||||
card_color="#4F46E5",
|
||||
is_active=True,
|
||||
created_at=now,
|
||||
updated_at=now,
|
||||
)
|
||||
|
||||
data = response.model_dump()
|
||||
assert "google_issuer_id" not in data
|
||||
assert "google_class_id" not in data
|
||||
assert "apple_pass_type_id" not in data
|
||||
|
||||
def test_non_wallet_fields_present(self):
|
||||
"""Non-wallet fields are still present."""
|
||||
now = datetime.now(UTC)
|
||||
response = StorefrontProgramResponse(
|
||||
id=1,
|
||||
merchant_id=1,
|
||||
loyalty_type="points",
|
||||
stamps_target=10,
|
||||
stamps_reward_description="Free item",
|
||||
points_per_euro=10,
|
||||
cooldown_minutes=15,
|
||||
max_daily_stamps=5,
|
||||
require_staff_pin=True,
|
||||
card_color="#4F46E5",
|
||||
is_active=True,
|
||||
created_at=now,
|
||||
updated_at=now,
|
||||
)
|
||||
|
||||
data = response.model_dump()
|
||||
assert data["id"] == 1
|
||||
assert data["loyalty_type"] == "points"
|
||||
assert data["points_per_euro"] == 10
|
||||
assert data["card_color"] == "#4F46E5"
|
||||
|
||||
@@ -285,3 +285,69 @@ class TestVoidStamps:
|
||||
|
||||
assert void_result["success"] is True
|
||||
assert void_result["stamp_count"] == 0
|
||||
|
||||
|
||||
# ============================================================================
|
||||
# Item 1: TOCTOU Race Condition (stamp checks after lock)
|
||||
# ============================================================================
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@pytest.mark.loyalty
|
||||
class TestStampTOCTOU:
|
||||
"""Verify cooldown/daily-limit checks happen after row lock."""
|
||||
|
||||
def setup_method(self):
|
||||
self.service = StampService()
|
||||
|
||||
def test_cooldown_checked_after_lock(self, db, stamp_setup):
|
||||
"""Cooldown check uses locked row state."""
|
||||
card = stamp_setup["card"]
|
||||
store = stamp_setup["store"]
|
||||
program = stamp_setup["program"]
|
||||
|
||||
# Enable cooldown
|
||||
program.cooldown_minutes = 60
|
||||
db.commit()
|
||||
|
||||
# First stamp succeeds
|
||||
self.service.add_stamp(db, store_id=store.id, card_id=card.id)
|
||||
|
||||
# Second stamp should fail (cooldown active after lock)
|
||||
with pytest.raises(StampCooldownException):
|
||||
self.service.add_stamp(db, store_id=store.id, card_id=card.id)
|
||||
|
||||
def test_daily_limit_checked_after_lock(self, db, stamp_setup):
|
||||
"""Daily limit check uses locked row state."""
|
||||
card = stamp_setup["card"]
|
||||
store = stamp_setup["store"]
|
||||
program = stamp_setup["program"]
|
||||
|
||||
program.max_daily_stamps = 1
|
||||
db.commit()
|
||||
|
||||
# First stamp succeeds
|
||||
self.service.add_stamp(db, store_id=store.id, card_id=card.id)
|
||||
|
||||
# Second stamp should fail (daily limit after lock)
|
||||
with pytest.raises(DailyStampLimitException):
|
||||
self.service.add_stamp(db, store_id=store.id, card_id=card.id)
|
||||
|
||||
def test_redeem_stamps_insufficient_after_lock(self, db, stamp_setup):
|
||||
"""Stamp count check uses locked row state."""
|
||||
card = stamp_setup["card"]
|
||||
store = stamp_setup["store"]
|
||||
program = stamp_setup["program"]
|
||||
|
||||
# Give exact stamp target
|
||||
card.stamp_count = program.stamps_target
|
||||
card.total_stamps_earned = program.stamps_target
|
||||
db.commit()
|
||||
|
||||
# First redeem succeeds
|
||||
result = self.service.redeem_stamps(db, store_id=store.id, card_id=card.id)
|
||||
assert result["success"] is True
|
||||
|
||||
# Second redeem should fail (0 stamps after lock)
|
||||
with pytest.raises(InsufficientStampsException):
|
||||
self.service.redeem_stamps(db, store_id=store.id, card_id=card.id)
|
||||
|
||||
Reference in New Issue
Block a user