Fix issues #54 and #53

skills · jkcso · Jan 5, 2024 · Dec 31, 2023 · Dec 31, 2023 · 155548cd1843a25635d04757bb6a5e417a7085fc
commit 155548cd1843a25635d04757bb6a5e417a7085fc
diff --git a/Season-1/Level-1/hack.py b/Season-1/Level-1/hack.py
@@ -3,21 +3,35 @@
 
 class TestOnlineStore(unittest.TestCase):
 
-    # Tricks the system and walks away with 1 television, despite valid payment & reimbursement
-    def test_4(self):
+    # Tricks the system and walks away with 1 television, despite valid payment & reimbursement.
+    def test_6(self):
         tv_item = c.Item(type='product', description='tv', amount=1000.00, quantity=1)
         payment = c.Item(type='payment', description='invoice_4', amount=1e19, quantity=1)
         payback = c.Item(type='payment', description='payback_4', amount=-1e19, quantity=1)
         order_4 = c.Order(id='4', items=[payment, tv_item, payback])
         self.assertEqual(c.validorder(order_4), 'Order ID: 4 - Payment imbalance: $-1000.00')
 
-    # Valid payments that should add up correctly, but do not
-    def test_5(self):
+    # Valid payments that should add up correctly, but do not.
+    def test_7(self):
         small_item = c.Item(type='product', description='accessory', amount=3.3, quantity=1)
         payment_1 = c.Item(type='payment', description='invoice_5_1', amount=1.1, quantity=1)
         payment_2 = c.Item(type='payment', description='invoice_5_2', amount=2.2, quantity=1)
         order_5 = c.Order(id='5', items=[small_item, payment_1, payment_2])
         self.assertEqual(c.validorder(order_5), 'Order ID: 5 - Full payment received!')
 
+    # The total amount of an order must be limited. Order validation shouldn't depend on ordering of items.
+    def test_8(self):
+        num_items = 12
+        items = [c.Item(type='product', description='tv', amount=99999, quantity=num_items)]
+        for i in range(num_items):
+            items.append(c.Item(type='payment', description='invoice_' + str(i), amount=99999, quantity=1))
+        order_1 = c.Order(id='1', items=items)
+        self.assertEqual(c.validorder(order_1), 'Total amount exceeded')
+
+        # Put payments before products
+        items = items[1:] + [items[0]]
+        order_2 = c.Order(id='2', items=items)
+        self.assertEqual(c.validorder(order_2), 'Total amount exceeded')
+
 if __name__ == '__main__':
     unittest.main()
diff --git a/Season-1/Level-1/solution.py b/Season-1/Level-1/solution.py
@@ -9,23 +9,25 @@
 MAX_TOTAL = 1e6 # maximum total amount accepted for an order
 
 def validorder(order):
-    net = Decimal('0')
+    payments = Decimal('0')
+    expenses = Decimal('0')
 
     for item in order.items:
         if item.type == 'payment':
-            # sets a reasonable min & max value for the invoice amounts
-            if item.amount > -1*MAX_ITEM_AMOUNT and item.amount < MAX_ITEM_AMOUNT:
-                net += Decimal(str(item.amount))
+            # Sets a reasonable min & max value for the invoice amounts
+            if -MAX_ITEM_AMOUNT <= item.amount <= MAX_ITEM_AMOUNT:
+                payments += Decimal(str(item.amount))
         elif item.type == 'product':
-            if item.quantity > 0 and item.quantity <= MAX_QUANTITY and item.amount > 0 and item.amount <= MAX_ITEM_AMOUNT:
-                net -= Decimal(str(item.amount)) * item.quantity
-            if net > MAX_TOTAL or net < -1*MAX_TOTAL:
-                return "Total amount exceeded"
+            if type(item.quantity) is int and 0 < item.quantity <= MAX_QUANTITY and 0 < item.amount <= MAX_ITEM_AMOUNT:
+                expenses += Decimal(str(item.amount)) * item.quantity
         else:
             return "Invalid item type: %s" % item.type
+
+    if abs(payments) > MAX_TOTAL or expenses > MAX_TOTAL:
+        return "Total amount exceeded"
 
-    if net != 0:
-        return "Order ID: %s - Payment imbalance: $%0.2f" % (order.id, net)
+    if payments != expenses:
+        return "Order ID: %s - Payment imbalance: $%0.2f" % (order.id, payments - expenses)
     else:
         return "Order ID: %s - Full payment received!" % order.id
 
@@ -67,6 +69,9 @@ def validorder(order):
 # >>> Decimal('0.3')
 # Decimal('0.3')
 
+# Input validation should be expanded to also check data types besides testing allowed range of values. 
+# This specific bug, caused by using a non-integer quantity, is due to insufficient attention to requirements engineering. 
+# In some systems it is OK to buy half of something, but here it was assumed to be a positive integer.
 
 # Contribute new levels to the game in 3 simple steps!
 # Read our Contribution Guideline at github.com/skills/secure-code-game/blob/main/CONTRIBUTING.md
diff --git a/Season-1/Level-1/tests.py b/Season-1/Level-1/tests.py
@@ -24,5 +24,20 @@ def test_3(self):
         order_3 = c.Order(id='3', items=[payment, tv_item, payback])
         self.assertEqual(c.validorder(order_3), 'Order ID: 3 - Payment imbalance: $-1000.00')
 
+    # Example 4 - invalid input should not blow up the system
+    def test_4(self):
+        tv = c.Item(type='product', description='tv', amount=1000, quantity=1.5)
+        order_1 = c.Order(id='1', items=[tv])
+        try:
+            c.validorder(order_1)
+        except:
+            self.fail("Unhandled exception occured in the target system!")
+
+    # Example 5 - successfully detects an invalid item type
+    def test_5(self):
+        service = c.Item(type='service', description='shippment of goods', amount=100, quantity=1)
+        order_1 = c.Order(id='1', items=[service])
+        self.assertEqual(c.validorder(order_1), 'Invalid item type: service')
+
 if __name__ == '__main__':
     unittest.main()