From 637b395982665ecea5a87cf47783ea5394c73777 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Fri, 31 Jul 2026 11:49:09 -0400 Subject: [PATCH] fix(auth): address review follow-ups Fail closed on whitespace-only Google configuration and centralize shared authentication behavior. --- README.md | 1 + functions/submit_order/handler.py | 30 ++++++++++++++---------------- src/server/generate_form.py | 2 +- tests/test_generate_form.py | 29 +++++++++++++++++++++++++++-- 4 files changed, 43 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index fd3e801..46d001d 100644 --- a/README.md +++ b/README.md @@ -60,6 +60,7 @@ Stack name: `meal-order-manager` (us-east-1) - **Lambda** — 7 functions: submit-order, admin-authorizer, close-form, aggregate-orders, slack-notifier, sync-roster, email-report - **EventBridge** — scheduled rules (dual EST/EDT) for close, reminders, payroll email - **Secrets Manager** — Slack bot token + - Migration note: the existing `meal-order-manager/form-api-key` secret remains until this change is deployed and verified, then must be deleted during post-deploy cleanup. - **SES** — payroll deduction emails - **CloudWatch Alarms** — coverage across the stack, all notifying the shared `site-alerts` SNS topic (see Monitoring) diff --git a/functions/submit_order/handler.py b/functions/submit_order/handler.py index 2704122..d6c6276 100644 --- a/functions/submit_order/handler.py +++ b/functions/submit_order/handler.py @@ -65,6 +65,10 @@ def _get_discount_settings() -> tuple[Decimal, Decimal]: return _settings +def _get_admin_emails() -> set[str]: + return {email.lower() for email in get_settings().get("admin_emails", [])} + + def _google_auth_configured() -> bool: return bool(os.environ.get("GOOGLE_CLIENT_ID_PARAM", "")) @@ -147,6 +151,10 @@ def _verify_google_token(token: str) -> tuple[dict | None, str]: return None, "invalid" +def _authentication_service_unavailable() -> dict: + return response(503, {"error": "Authentication service temporarily unavailable"}) + + def _verify_admin(event) -> tuple[dict | None, dict | None]: """Verify Google auth and admin access. Returns (user_info, error_response).""" if not _google_auth_configured(): @@ -163,14 +171,11 @@ def _verify_admin(event) -> tuple[dict | None, dict | None]: user_info, status = _verify_google_token(token) if status == "unavailable": - return None, response( - 503, {"error": "Authentication service temporarily unavailable"} - ) + return None, _authentication_service_unavailable() if user_info is None: return None, response(403, {"error": "Invalid or unauthorized Google account"}) - admin_emails = {e.lower() for e in get_settings().get("admin_emails", [])} - if user_info["email"].lower() not in admin_emails: + if user_info["email"].lower() not in _get_admin_emails(): return None, response(403, {"error": "Admin access required"}) return user_info, None @@ -422,21 +427,15 @@ def handle_submit(event): client_id = _get_google_client_id() except Exception as exc: logger.error("SSM fetch failed for Google client ID: %s", exc) - return response( - 503, {"error": "Authentication service temporarily unavailable"} - ) + return _authentication_service_unavailable() if not client_id: logger.error("Google client ID is unavailable") - return response( - 503, {"error": "Authentication service temporarily unavailable"} - ) + return _authentication_service_unavailable() if not google_token: return response(403, {"error": "Google authentication is required"}) user_info, verify_status = _verify_google_token(google_token) if verify_status == "unavailable": - return response( - 503, {"error": "Authentication service temporarily unavailable"} - ) + return _authentication_service_unavailable() if user_info is None: return response(403, {"error": "Invalid or unauthorized Google account"}) name = user_info["name"] @@ -453,8 +452,7 @@ def handle_submit(event): week = current_week() status = get_form_status(week) - admin_emails = {e.lower() for e in get_settings().get("admin_emails", [])} - is_admin_user = email.lower() in admin_emails + is_admin_user = email.lower() in _get_admin_emails() if status == "closed" and not is_admin_user: return response(410, {"error": "Orders are closed for this week"}) if status == "not_found": diff --git a/src/server/generate_form.py b/src/server/generate_form.py index d74c283..26819d2 100644 --- a/src/server/generate_form.py +++ b/src/server/generate_form.py @@ -159,7 +159,7 @@ def main(): ssm = boto3.client("ssm") resp = ssm.get_parameter(Name="/meal-order-manager/google-client-id") - google_client_id = resp["Parameter"]["Value"] + google_client_id = resp["Parameter"]["Value"].strip() except Exception as exc: parser.error( "Google auth is required in cloud mode, but the Google client ID " diff --git a/tests/test_generate_form.py b/tests/test_generate_form.py index 06d0ffa..fe6e2b2 100644 --- a/tests/test_generate_form.py +++ b/tests/test_generate_form.py @@ -7,6 +7,7 @@ import json import re import sys from pathlib import Path +from types import SimpleNamespace import pytest @@ -16,7 +17,9 @@ FIXTURES = Path(__file__).resolve().parent / "fixtures" sys.path.insert(0, str(SERVER_DIR)) -from generate_form import generate_form # noqa: E402 +import generate_form as generate_form_module # noqa: E402 + +generate_form = generate_form_module.generate_form EXPECTED_CONFIG_KEYS = { "authMode", @@ -128,6 +131,24 @@ class TestGenerateFormStructural: google_client_id=client_id, ) + def test_cloud_cli_rejects_whitespace_ssm_client_id(self, monkeypatch, capsys): + menu, config = _load_fixtures() + ssm = SimpleNamespace(get_parameter=lambda **_: {"Parameter": {"Value": " "}}) + monkeypatch.setitem(sys.modules, "boto3", SimpleNamespace(client=lambda _: ssm)) + monkeypatch.setattr(generate_form_module, "load_config", lambda: config) + monkeypatch.setattr(generate_form_module, "latest_menu", lambda: menu) + monkeypatch.setattr( + sys, + "argv", + ["generate_form.py", "--api-url", "https://api.example.com"], + ) + + with pytest.raises(SystemExit) as exc_info: + generate_form_module.main() + + assert exc_info.value.code == 2 + assert "Google auth is required in cloud mode" in capsys.readouterr().err + def test_generated_form_contains_no_api_key_handling(self): html = _render(google=True) assert "apiKey" not in _extract_config(html) @@ -138,11 +159,15 @@ class TestGenerateFormStructural: template = (REPO_ROOT / "template.yaml").read_text() workflow = (REPO_ROOT / ".github" / "workflows" / "weekly-menu.yml").read_text() readme = (REPO_ROOT / "README.md").read_text() - for text in (template, workflow, readme): + for text in (template, workflow): assert "FORM_APIKEY" not in text assert "form-api-key" not in text assert "x-api-key" not in text assert "--api-key" not in text + assert ( + "`meal-order-manager/form-api-key` secret remains until this change " + "is deployed and verified" + ) in readme def test_submit_route_has_explicit_throttling(self): template = (REPO_ROOT / "template.yaml").read_text()