mirror of
https://github.com/Sea-Haven-Industries/meal-order-manager.git
synced 2026-09-30 22:53:12 +00:00
Fix PR review findings: auth, rounding, and close-form guard
- Remove dead elif branch in submit_order auth (always returned 403) - Catch HTTPError before URLError so expired tokens return 403 not 503 - Wrap SSM get_parameter in try/except for fresh deployments - Add wall-clock guard to close_form handler (Friday >= 11 PM ET) - Add epsilon nudge to JS employeePrice for IEEE 754 boundary match - Switch Flask dev server from round() to Decimal ROUND_HALF_UP - Add tests for HTTPError handling and close_form guard (6 new tests)
This commit is contained in:
parent
fa8f19660d
commit
5c040bf55c
6 changed files with 194 additions and 28 deletions
|
|
@ -1,14 +1,24 @@
|
|||
import json
|
||||
import os
|
||||
from datetime import datetime
|
||||
from zoneinfo import ZoneInfo
|
||||
|
||||
import boto3
|
||||
|
||||
from shared.db import current_week, get_form_status, set_form_status
|
||||
|
||||
_lambda = boto3.client("lambda")
|
||||
EASTERN = ZoneInfo("America/New_York")
|
||||
|
||||
|
||||
def lambda_handler(event, context):
|
||||
now_et = datetime.now(EASTERN)
|
||||
if not (now_et.weekday() == 4 and now_et.hour >= 23):
|
||||
return {
|
||||
"status": "skipped",
|
||||
"reason": "outside close window (must be Friday >= 11 PM ET)",
|
||||
}
|
||||
|
||||
week = event.get("week", current_week())
|
||||
|
||||
status = get_form_status(week)
|
||||
|
|
|
|||
|
|
@ -56,7 +56,13 @@ def _get_google_client_id() -> str:
|
|||
if _google_client_id is None or (now - _google_client_id_ts) > CACHE_TTL_SECONDS:
|
||||
param = os.environ.get("GOOGLE_CLIENT_ID_PARAM", "")
|
||||
if param:
|
||||
_google_client_id = get_parameter(param, decrypt=False) or ""
|
||||
try:
|
||||
_google_client_id = get_parameter(param, decrypt=False) or ""
|
||||
except Exception as exc:
|
||||
logger.warning(
|
||||
"Could not fetch Google client ID from SSM (%s): %s", param, exc
|
||||
)
|
||||
_google_client_id = ""
|
||||
else:
|
||||
_google_client_id = ""
|
||||
_google_client_id_ts = now
|
||||
|
|
@ -92,6 +98,9 @@ def _verify_google_token(token: str) -> tuple[dict | None, str]:
|
|||
logger.warning("Google token domain mismatch: got %s", data.get("hd"))
|
||||
return None, "invalid"
|
||||
return {"name": data.get("name", ""), "email": data.get("email", "")}, "ok"
|
||||
except urllib.error.HTTPError as exc:
|
||||
logger.warning("Google token rejected (HTTP %s): %s", exc.code, exc)
|
||||
return None, "invalid"
|
||||
except (urllib.error.URLError, TimeoutError, OSError) as exc:
|
||||
logger.error("Google token verification service unavailable: %s", exc)
|
||||
return None, "unavailable"
|
||||
|
|
@ -168,19 +177,7 @@ def handle_submit(event):
|
|||
return response(403, {"error": "Invalid or unauthorized Google account"})
|
||||
name = user_info["name"]
|
||||
email = user_info["email"]
|
||||
elif google_token:
|
||||
# Google auth not configured but token provided — verify it anyway
|
||||
user_info, verify_status = _verify_google_token(google_token)
|
||||
if verify_status == "unavailable":
|
||||
return response(
|
||||
503, {"error": "Authentication service temporarily unavailable"}
|
||||
)
|
||||
if user_info is None:
|
||||
return response(403, {"error": "Invalid or unauthorized Google account"})
|
||||
name = user_info["name"]
|
||||
email = user_info["email"]
|
||||
else:
|
||||
# Manual fallback — only when Google auth is not configured
|
||||
name = body.get("employee_name", "").strip()
|
||||
email = body.get("employee_email", "").strip()
|
||||
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ Orders are saved as JSON files in the orders directory, one per employee per wee
|
|||
import json
|
||||
import urllib.request
|
||||
from datetime import datetime
|
||||
from decimal import Decimal, ROUND_HALF_UP
|
||||
from pathlib import Path
|
||||
|
||||
import boto3
|
||||
|
|
@ -122,21 +123,25 @@ def submit_order():
|
|||
return jsonify({"error": "Please select at least one meal"}), 400
|
||||
|
||||
config = load_config()
|
||||
bulk_pct = config.get("bulk_discount_percent", 0)
|
||||
subsidy_pct = config.get("company_subsidy_percent", 0)
|
||||
bulk_mult = 1 - (bulk_pct / 100)
|
||||
subsidy_mult = 1 - (subsidy_pct / 100)
|
||||
TWO_PLACES = Decimal("0.01")
|
||||
bulk_pct = Decimal(str(config.get("bulk_discount_percent", 0)))
|
||||
subsidy_pct = Decimal(str(config.get("company_subsidy_percent", 0)))
|
||||
bulk_mult = Decimal("1") - (bulk_pct / Decimal("100"))
|
||||
subsidy_mult = Decimal("1") - (subsidy_pct / Decimal("100"))
|
||||
|
||||
filtered = [i for i in items if i.get("quantity", 0) > 0]
|
||||
for item in filtered:
|
||||
retail = item.get("retail_price", item.get("price", 0)) or 0
|
||||
qty = item.get("quantity", 0)
|
||||
bulk_price = round(retail * bulk_mult, 2)
|
||||
emp_price = round(bulk_price * subsidy_mult, 2)
|
||||
item["retail_price"] = retail
|
||||
item["bulk_price"] = bulk_price
|
||||
item["price"] = emp_price
|
||||
item["subtotal"] = round(emp_price * qty, 2)
|
||||
retail = Decimal(str(item.get("retail_price", item.get("price", 0)) or 0))
|
||||
qty = Decimal(str(item.get("quantity", 0)))
|
||||
bulk_price = (retail * bulk_mult).quantize(TWO_PLACES, rounding=ROUND_HALF_UP)
|
||||
emp_price = (bulk_price * subsidy_mult).quantize(
|
||||
TWO_PLACES, rounding=ROUND_HALF_UP
|
||||
)
|
||||
subtotal = (emp_price * qty).quantize(TWO_PLACES, rounding=ROUND_HALF_UP)
|
||||
item["retail_price"] = float(retail)
|
||||
item["bulk_price"] = float(bulk_price)
|
||||
item["price"] = float(emp_price)
|
||||
item["subtotal"] = float(subtotal)
|
||||
|
||||
week = current_week()
|
||||
week_dir = ORDERS_DIR / week
|
||||
|
|
@ -150,7 +155,11 @@ def submit_order():
|
|||
)
|
||||
order_file = week_dir / f"{safe_name}.json"
|
||||
|
||||
total = round(sum(i["subtotal"] for i in filtered), 2)
|
||||
total = float(
|
||||
sum(Decimal(str(i["subtotal"])) for i in filtered).quantize(
|
||||
TWO_PLACES, rounding=ROUND_HALF_UP
|
||||
)
|
||||
)
|
||||
|
||||
order = {
|
||||
"employee_name": name,
|
||||
|
|
|
|||
|
|
@ -399,8 +399,8 @@ function escapeHtml(str) {{
|
|||
|
||||
function employeePrice(retail) {{
|
||||
if (!retail) return 0;
|
||||
const bulkPrice = Math.round(retail * (1 - BULK_DISCOUNT / 100) * 100) / 100;
|
||||
const empPrice = Math.round(bulkPrice * (1 - COMPANY_SUBSIDY / 100) * 100) / 100;
|
||||
const bulkPrice = Math.round(retail * (1 - BULK_DISCOUNT / 100) * 100 + 1e-8) / 100;
|
||||
const empPrice = Math.round(bulkPrice * (1 - COMPANY_SUBSIDY / 100) * 100 + 1e-8) / 100;
|
||||
return empPrice;
|
||||
}}
|
||||
{google_auth_js}
|
||||
|
|
|
|||
101
tests/test_close_form.py
Normal file
101
tests/test_close_form.py
Normal file
|
|
@ -0,0 +1,101 @@
|
|||
"""Unit tests for functions/close_form/handler.py — wall-clock guard."""
|
||||
|
||||
import os
|
||||
import sys
|
||||
from datetime import datetime
|
||||
from unittest.mock import patch
|
||||
from zoneinfo import ZoneInfo
|
||||
|
||||
os.environ.setdefault(
|
||||
"AGGREGATE_FUNCTION_ARN",
|
||||
"arn:aws:lambda:us-east-1:000000000000:function:test-aggregate",
|
||||
)
|
||||
|
||||
import importlib.util
|
||||
|
||||
_handler_path = os.path.join(
|
||||
os.path.dirname(__file__), os.pardir, "functions", "close_form", "handler.py"
|
||||
)
|
||||
_spec = importlib.util.spec_from_file_location(
|
||||
"close_form_handler", os.path.abspath(_handler_path)
|
||||
)
|
||||
close_form_handler = importlib.util.module_from_spec(_spec)
|
||||
sys.modules["close_form_handler"] = close_form_handler
|
||||
_spec.loader.exec_module(close_form_handler)
|
||||
|
||||
ET = ZoneInfo("America/New_York")
|
||||
|
||||
|
||||
def _make_datetime(year, month, day, hour, minute=0):
|
||||
return datetime(year, month, day, hour, minute, tzinfo=ET)
|
||||
|
||||
|
||||
class TestCloseFormGuard:
|
||||
@patch("close_form_handler.datetime")
|
||||
def test_skipped_on_thursday(self, mock_dt):
|
||||
"""Thursday 11pm ET -> skipped (not Friday)."""
|
||||
mock_dt.now.return_value = _make_datetime(2026, 5, 14, 23) # Thursday
|
||||
|
||||
result = close_form_handler.lambda_handler({}, None)
|
||||
|
||||
assert result["status"] == "skipped"
|
||||
|
||||
@patch("close_form_handler.datetime")
|
||||
def test_skipped_friday_before_11pm(self, mock_dt):
|
||||
"""Friday 3am ET (UTC cron fires but too early) -> skipped."""
|
||||
mock_dt.now.return_value = _make_datetime(2026, 5, 15, 3) # Friday 3am
|
||||
|
||||
result = close_form_handler.lambda_handler({}, None)
|
||||
|
||||
assert result["status"] == "skipped"
|
||||
|
||||
@patch("close_form_handler.set_form_status")
|
||||
@patch("close_form_handler.get_form_status", return_value="open")
|
||||
@patch("close_form_handler.current_week", return_value="2026-W19")
|
||||
@patch("close_form_handler._lambda")
|
||||
@patch("close_form_handler.datetime")
|
||||
def test_runs_friday_at_11pm(
|
||||
self, mock_dt, mock_lam, mock_week, mock_status, mock_set
|
||||
):
|
||||
"""Friday 11pm ET -> proceeds to close."""
|
||||
mock_dt.now.return_value = _make_datetime(2026, 5, 15, 23) # Friday 11pm
|
||||
|
||||
result = close_form_handler.lambda_handler({}, None)
|
||||
|
||||
assert result["status"] == "closed"
|
||||
mock_set.assert_called_once()
|
||||
|
||||
@patch("close_form_handler.set_form_status")
|
||||
@patch("close_form_handler.get_form_status", return_value="open")
|
||||
@patch("close_form_handler.current_week", return_value="2026-W19")
|
||||
@patch("close_form_handler._lambda")
|
||||
@patch("close_form_handler.datetime")
|
||||
def test_runs_friday_at_1159pm(
|
||||
self, mock_dt, mock_lam, mock_week, mock_status, mock_set
|
||||
):
|
||||
"""Friday 11:59pm ET -> proceeds to close."""
|
||||
mock_dt.now.return_value = _make_datetime(2026, 5, 15, 23, 59)
|
||||
|
||||
result = close_form_handler.lambda_handler({}, None)
|
||||
|
||||
assert result["status"] == "closed"
|
||||
|
||||
@patch("close_form_handler.datetime")
|
||||
def test_skipped_on_wednesday(self, mock_dt):
|
||||
"""Wednesday at any hour -> skipped."""
|
||||
mock_dt.now.return_value = _make_datetime(2026, 5, 13, 23)
|
||||
|
||||
result = close_form_handler.lambda_handler({}, None)
|
||||
|
||||
assert result["status"] == "skipped"
|
||||
|
||||
@patch("close_form_handler.get_form_status", return_value="closed")
|
||||
@patch("close_form_handler.current_week", return_value="2026-W19")
|
||||
@patch("close_form_handler.datetime")
|
||||
def test_already_closed(self, mock_dt, mock_week, mock_status):
|
||||
"""Friday 11pm but already closed -> returns already_closed."""
|
||||
mock_dt.now.return_value = _make_datetime(2026, 5, 15, 23)
|
||||
|
||||
result = close_form_handler.lambda_handler({}, None)
|
||||
|
||||
assert result["status"] == "already_closed"
|
||||
|
|
@ -559,6 +559,55 @@ def test_google_token_service_unavailable(
|
|||
mock_put.assert_not_called()
|
||||
|
||||
|
||||
@patch("submit_order_handler._lambda")
|
||||
@patch("submit_order_handler.put_order")
|
||||
@patch("submit_order_handler.get_form_status", return_value="open")
|
||||
@patch("submit_order_handler.current_week", return_value="2026-W20")
|
||||
@patch(
|
||||
"submit_order_handler.get_settings",
|
||||
return_value={"bulk_discount_percent": 0, "company_subsidy_percent": 0},
|
||||
)
|
||||
@patch("submit_order_handler.get_secret", return_value=TEST_API_KEY)
|
||||
@patch(
|
||||
"submit_order_handler._get_google_client_id", return_value=VALID_GOOGLE_CLIENT_ID
|
||||
)
|
||||
@patch(
|
||||
"submit_order_handler.urllib.request.urlopen",
|
||||
side_effect=urllib.error.HTTPError(
|
||||
"https://oauth2.googleapis.com/tokeninfo",
|
||||
400,
|
||||
"Bad Request",
|
||||
{},
|
||||
None,
|
||||
),
|
||||
)
|
||||
def test_google_token_http_error_returns_403(
|
||||
mock_urlopen,
|
||||
mock_gcid,
|
||||
mock_secret,
|
||||
mock_settings,
|
||||
mock_week,
|
||||
mock_status,
|
||||
mock_put,
|
||||
mock_lam,
|
||||
):
|
||||
"""HTTPError (e.g. 400 for expired token) -> 403, not 503."""
|
||||
from submit_order_handler import lambda_handler
|
||||
|
||||
items = _make_items([(10.00, 1)])
|
||||
event = _submit_event(items, extra_body={"google_id_token": "expired-token"})
|
||||
result = lambda_handler(event, None)
|
||||
status, body = _parse_response(result)
|
||||
|
||||
assert status == 403, (
|
||||
f"Expected 403 for HTTP error (bad token), got {status}: {body}"
|
||||
)
|
||||
assert "Invalid or unauthorized" in body["error"], (
|
||||
f"Expected 'Invalid or unauthorized' in error, got: {body['error']}"
|
||||
)
|
||||
mock_put.assert_not_called()
|
||||
|
||||
|
||||
@patch("submit_order_handler._lambda")
|
||||
@patch("submit_order_handler.put_order")
|
||||
@patch("submit_order_handler.get_form_status", return_value="open")
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue