diff --git a/src/adclaw/campaigns/validation.py b/src/adclaw/campaigns/validation.py index 10b4649..7fc1b37 100644 --- a/src/adclaw/campaigns/validation.py +++ b/src/adclaw/campaigns/validation.py @@ -78,11 +78,26 @@ def validate_budget( ), ) - if lifetime is not None and lifetime <= 0: + if lifetime is not None: + if lifetime <= 0: + issues.append( + ValidationIssue( + field="budget.lifetime_budget", + message="Lifetime budget must be greater than zero.", + ), + ) + # Reached only when daily is None (the daily-and-lifetime-together + # case already returned above), i.e. this is a lifetime-only + # budget. create_ad_set only accepts daily_budget, so without this + # a lifetime-only campaign would pass validation and approval and + # only fail once Task 5's sync tries to create the Meta ad set. issues.append( ValidationIssue( field="budget.lifetime_budget", - message="Lifetime budget must be greater than zero.", + message=( + "Lifetime budgets are not supported yet. Set a daily " + "budget instead." + ), ), ) diff --git a/src/adclaw/meta/client.py b/src/adclaw/meta/client.py index b0b5f33..3d3c48b 100644 --- a/src/adclaw/meta/client.py +++ b/src/adclaw/meta/client.py @@ -52,6 +52,11 @@ CREATE_STATUS = "PAUSED" LIST_FIELDS = "id,name,status,effective_status,created_time" +# Path suffixes that create a campaign/ad set/ad. _post refuses any of these +# with a non-PAUSED status as defence in depth, in case a future caller +# bypasses the typed create_* guards and calls _post directly. +_PAUSED_ONLY_SUFFIXES = ("/campaigns", "/adsets", "/ads") + Transport = Callable[[str, str, dict[str, Any]], Awaitable[dict[str, Any]]] @@ -132,7 +137,22 @@ class MetaClient: Graph returns errors with HTTP 200, so both `_get` and `_post` must inspect the response body for an `"error"` key rather than trust the HTTP status code. + + Defence in depth: a POST to a campaign/ad set/ad creation path with + a status other than PAUSED is refused here too, before any + transport call, even if a caller bypasses the typed create_* + guards. `update_object_status` posts to `/{object_id}`, which never + matches these suffixes, so Launch (the only path allowed to send + ACTIVE) is unaffected. """ + if ( + path.endswith(_PAUSED_ONLY_SUFFIXES) + and data.get("status") != CREATE_STATUS + ): + raise ValueError( + "Refusing to create a campaign/ad set/ad with a non-PAUSED " + "status. Use update_object_status to launch.", + ) payload = dict(data) payload["access_token"] = self._token result = await self._transport("POST", f"{GRAPH_BASE_URL}{path}", payload) @@ -201,7 +221,7 @@ class MetaClient: { "name": name, "objective": objective, - "status": status, + "status": CREATE_STATUS, "special_ad_categories": json.dumps(special_ad_categories or []), }, ) @@ -239,7 +259,7 @@ class MetaClient: "targeting": json.dumps(targeting), "optimization_goal": optimization_goal, "billing_event": billing_event, - "status": status, + "status": CREATE_STATUS, }, ) return result["id"] @@ -325,7 +345,7 @@ class MetaClient: "name": name, "adset_id": adset_id, "creative": json.dumps({"creative_id": creative_id}), - "status": status, + "status": CREATE_STATUS, }, ) return result["id"] diff --git a/src/adclaw/meta/objects.py b/src/adclaw/meta/objects.py index 2a1f24f..c41eef6 100644 --- a/src/adclaw/meta/objects.py +++ b/src/adclaw/meta/objects.py @@ -26,10 +26,19 @@ This is a deliberate, documented choice rather than an oversight: `targeting.geo_locations.countries`. - `spec.targeting["genders"]` -> Graph `targeting.genders`, if present. - `spec.advanced["special_ad_categories"]` -> campaign - `special_ad_categories` (defaults to an empty list). + `special_ad_categories` (defaults to an empty list). A bare string is + normalised to a single-element list rather than exploded into + characters, since `advanced` is unvalidated free-form JSON and this is + the compliance field for regulated advertising (housing, credit, + employment, ...). - `spec.advanced["optimization_goal"]` / `["billing_event"]` -> ad set fields of the same name, defaulting to `LEAD_GENERATION` / - `IMPRESSIONS` (MaskanX's default lead-gen objective). + `IMPRESSIONS` (MaskanX's default lead-gen objective). Both are checked + against an allowlist (`ALLOWED_OPTIMIZATION_GOALS` / + `ALLOWED_BILLING_EVENTS`) before being forwarded, since `advanced` is + client-controlled and `billing_event` in particular determines how the + ad account is charged; an unrecognised value raises `ValueError` naming + the offending value and the allowed set rather than reaching Meta. - `spec.advanced["message"]` / `["headline"]` / `["description"]` / `["link"]` -> ad creative `object_story_spec.link_data` fields of the same purpose (headline defaults to the campaign name if unset). @@ -43,6 +52,18 @@ from ..campaigns.models import CampaignSpec DEFAULT_OPTIMIZATION_GOAL = "LEAD_GENERATION" DEFAULT_BILLING_EVENT = "IMPRESSIONS" +# advanced.optimization_goal / advanced.billing_event are client-controlled +# and billing_event determines how the ad account is charged, so both are +# checked against an allowlist before being forwarded to a spending API. +ALLOWED_OPTIMIZATION_GOALS = frozenset({ + "LEAD_GENERATION", "LINK_CLICKS", "IMPRESSIONS", "REACH", + "OFFSITE_CONVERSIONS", "LANDING_PAGE_VIEWS", "THRUPLAY", + "POST_ENGAGEMENT", "QUALITY_CALL", +}) +ALLOWED_BILLING_EVENTS = frozenset({ + "IMPRESSIONS", "LINK_CLICKS", "THRUPLAY", "POST_ENGAGEMENT", +}) + def build_campaign_payload(spec: CampaignSpec) -> dict[str, Any]: """Map a CampaignSpec onto `MetaClient.create_campaign` kwargs. @@ -54,12 +75,18 @@ def build_campaign_payload(spec: CampaignSpec) -> dict[str, Any]: raise ValueError( "CampaignSpec.objective is required to create a Meta campaign.", ) + # A bare string ("HOUSING") must become a single-element list, not be + # exploded into characters by list(); advanced is unvalidated JSON from + # the API and special_ad_categories is the compliance field for + # regulated advertising, so this cannot be left to Meta's opaque error. + raw_categories = spec.advanced.get("special_ad_categories") or [] + categories = ( + [raw_categories] if isinstance(raw_categories, str) else list(raw_categories) + ) return { "name": spec.name, "objective": spec.objective, - "special_ad_categories": list( - spec.advanced.get("special_ad_categories") or [], - ), + "special_ad_categories": categories, } @@ -90,15 +117,28 @@ def build_ad_set_payload(spec: CampaignSpec, campaign_id: str) -> dict[str, Any] if genders: targeting["genders"] = list(genders) + optimization_goal = spec.advanced.get( + "optimization_goal", DEFAULT_OPTIMIZATION_GOAL, + ) + if optimization_goal not in ALLOWED_OPTIMIZATION_GOALS: + raise ValueError( + f"Unsupported optimization_goal {optimization_goal!r}. Allowed: " + f"{sorted(ALLOWED_OPTIMIZATION_GOALS)}", + ) + billing_event = spec.advanced.get("billing_event", DEFAULT_BILLING_EVENT) + if billing_event not in ALLOWED_BILLING_EVENTS: + raise ValueError( + f"Unsupported billing_event {billing_event!r}. Allowed: " + f"{sorted(ALLOWED_BILLING_EVENTS)}", + ) + return { "campaign_id": campaign_id, "name": f"{spec.name} - Ad Set", "daily_budget": daily_budget, "targeting": targeting, - "optimization_goal": spec.advanced.get( - "optimization_goal", DEFAULT_OPTIMIZATION_GOAL, - ), - "billing_event": spec.advanced.get("billing_event", DEFAULT_BILLING_EVENT), + "optimization_goal": optimization_goal, + "billing_event": billing_event, } diff --git a/tests/test_campaign_validation.py b/tests/test_campaign_validation.py index d2b57e6..dc23bb3 100644 --- a/tests/test_campaign_validation.py +++ b/tests/test_campaign_validation.py @@ -30,6 +30,25 @@ def test_daily_and_lifetime_budget_together_is_rejected(): assert [i.field for i in issues] == ["budget"] +def test_lifetime_only_budget_is_rejected_as_unsupported(): + # create_ad_set only accepts daily_budget (objects.build_ad_set_payload + # raises ValueError for a lifetime-only spec), so a lifetime-only + # budget must be caught here rather than passing validation/approval + # and only failing once sync tries to create the Meta ad set. + issues = validate_budget({"lifetime_budget": 50000}, min_daily_budget=9709) + assert [i.field for i in issues] == ["budget.lifetime_budget"] + assert "not supported yet" in issues[0].message + + +def test_lifetime_only_negative_budget_reports_both_issues(): + issues = validate_budget({"lifetime_budget": -5}, min_daily_budget=9709) + fields = [i.field for i in issues] + assert fields == ["budget.lifetime_budget", "budget.lifetime_budget"] + messages = " ".join(i.message for i in issues) + assert "greater than zero" in messages + assert "not supported yet" in messages + + def test_auto_pause_above_daily_budget_is_rejected(): issues = validate_guardrails( {"daily_budget": 10000}, diff --git a/tests/test_meta_writes.py b/tests/test_meta_writes.py index 64ed597..22db843 100644 --- a/tests/test_meta_writes.py +++ b/tests/test_meta_writes.py @@ -289,6 +289,38 @@ async def test_update_object_status_returns_none(): assert result is None +# --------------------------------------------------------------------------- +# _post defence in depth - reachable directly, still refuses non-PAUSED +# creates on campaign/adset/ad paths, before any transport call +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_post_refuses_non_paused_status_on_campaign_create_path(): + transport = FakeTransport([{"id": "c1"}]) + client = MetaClient(access_token="tok", transport=transport) + + with pytest.raises(ValueError): + await client._post( + "/act_1/campaigns", + {"name": "Q3", "objective": "OUTCOME_LEADS", "status": "ACTIVE"}, + ) + assert transport.calls == [] + + +@pytest.mark.asyncio +async def test_post_still_allows_update_object_status_to_send_active(): + """update_object_status posts to /{object_id}, which never matches the + campaign/adset/ad creation suffixes, so the _post guard above must not + block Launch.""" + transport = FakeTransport([{"success": True}]) + client = MetaClient(access_token="tok", transport=transport) + + await client.update_object_status("c1", "ACTIVE") + + assert transport.calls[0]["params"]["status"] == "ACTIVE" + + # --------------------------------------------------------------------------- # list_campaigns / list_ad_sets / list_ads # --------------------------------------------------------------------------- @@ -383,6 +415,25 @@ def test_build_campaign_payload_requires_objective(): build_campaign_payload(spec) +def test_build_campaign_payload_normalises_bare_string_special_ad_category(): + # advanced is unvalidated free-form JSON, so a bare string is a + # plausible client input for this compliance field. It must become a + # single-element list, not be exploded into characters by list(). + spec = _spec(advanced={"special_ad_categories": "HOUSING"}) + + payload = build_campaign_payload(spec) + + assert payload["special_ad_categories"] == ["HOUSING"] + + +def test_build_campaign_payload_passes_through_a_list_unchanged(): + spec = _spec(advanced={"special_ad_categories": ["HOUSING", "EMPLOYMENT"]}) + + payload = build_campaign_payload(spec) + + assert payload["special_ad_categories"] == ["HOUSING", "EMPLOYMENT"] + + def test_build_ad_set_payload_maps_targeting_and_budget(): spec = _spec() @@ -420,6 +471,40 @@ def test_build_ad_set_payload_requires_daily_budget(): build_ad_set_payload(spec, campaign_id="c1") +def test_build_ad_set_payload_accepts_allowlisted_optimization_goal(): + spec = _spec(advanced={"optimization_goal": "LINK_CLICKS"}) + + payload = build_ad_set_payload(spec, campaign_id="c1") + + assert payload["optimization_goal"] == "LINK_CLICKS" + + +def test_build_ad_set_payload_rejects_unknown_optimization_goal(): + spec = _spec(advanced={"optimization_goal": "SOMETHING_MADE_UP"}) + + with pytest.raises(ValueError): + build_ad_set_payload(spec, campaign_id="c1") + + +def test_build_ad_set_payload_accepts_allowlisted_billing_event(): + spec = _spec(advanced={"billing_event": "LINK_CLICKS"}) + + payload = build_ad_set_payload(spec, campaign_id="c1") + + assert payload["billing_event"] == "LINK_CLICKS" + + +def test_build_ad_set_payload_rejects_unknown_billing_event(): + # billing_event determines how the ad account is charged, so an + # unrecognised value from client-controlled `advanced` must never reach + # Meta - a spending-relevant field with no allowlist is the finding + # this guards against. + spec = _spec(advanced={"billing_event": "SOMETHING_MADE_UP"}) + + with pytest.raises(ValueError): + build_ad_set_payload(spec, campaign_id="c1") + + def test_build_ad_set_payload_targeting_can_produce_json_dumpable_dict(): # This is what create_ad_set will json.dumps() before sending - make # sure the mapping never emits anything that would break that step.