diff --git a/openfut-adapter-fifa17/src/fut/squad.rs b/openfut-adapter-fifa17/src/fut/squad.rs index ce0abae..1b6e78f 100644 --- a/openfut-adapter-fifa17/src/fut/squad.rs +++ b/openfut-adapter-fifa17/src/fut/squad.rs @@ -200,6 +200,67 @@ pub fn parse_squad_put(body: &[u8]) -> Result { serde_json::from_slice(body).map_err(|e| SquadError::Parse(e.to_string())) } +/// A role-only squad update: the client changed the captain and/or the +/// kick-taker assignments without touching the squad itself. +#[derive(Debug, Clone, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct Fifa17SquadRolePatch { + #[serde(default)] + pub id: i64, + /// Opaque 33-int array, verbatim. Differs from the replacement's `custom` + /// in the captures (the role screen writes per-slot values into it), so it + /// MUST be carried through rather than preserved from the stored copy. + #[serde(default)] + pub custom: Option, + #[serde(default)] + pub captain: Option, + #[serde(default)] + pub kicktakers: Vec, +} + +/// Which mutation a `PUT …/squad/` body actually expresses. +/// +/// FIFA 17 sends two different operations down one path, so the BODY SHAPE is +/// the operation discriminator. Across 73 captured squad PUTs spanning five +/// captures there are exactly two shapes: +/// +/// * 68x with `players` — a full replacement, also carrying `squadName`, +/// `formation`, `squadType`, `manager`, `chemistry`/`rating`/`starRating`, +/// and (redundantly) `captain`/`kicktakers`. +/// * 5x without `players` — `{id, custom, captain, kicktakers}` only, emitted +/// by the captain/kick-taker screen. +/// +/// `players` is therefore the discriminator: its PRESENCE means "this body +/// describes the whole squad". Its ABSENCE means the squad was not part of the +/// edit at all and must be left alone — which is NOT the same as an empty +/// `players` array, and that distinction is the whole point. Serde's +/// `#[serde(default)]` collapses both to an empty vec, so key presence is +/// tested on the raw JSON before deserialising. +/// +/// An explicit `"players": []` still classifies as a replacement, so the +/// empty-replacement guard in Core keeps seeing it. +#[derive(Debug, Clone)] +pub enum SquadMutation { + /// Full replacement of the squad's slots and metadata. + Replace(Box), + /// Role-only patch: captain and/or kick-takers, nothing else. + PatchRoles(Fifa17SquadRolePatch), +} + +/// Classify a squad PUT body. See [`SquadMutation`] for the discriminator and +/// the capture evidence behind it. +pub fn classify_squad_put(body: &[u8]) -> Result { + let raw: serde_json::Value = + serde_json::from_slice(body).map_err(|e| SquadError::Parse(e.to_string()))?; + let has_players = raw.as_object().is_some_and(|o| o.contains_key("players")); + if has_players { + return parse_squad_put(body).map(|p| SquadMutation::Replace(Box::new(p))); + } + serde_json::from_slice(body) + .map(SquadMutation::PatchRoles) + .map_err(|e| SquadError::Parse(e.to_string())) +} + /// Resolve a parsed save into a **canonical** [`ProposedSquad`]: drop empty /// (`id == 0`) slots, reverse-map each occupied slot's wire id to a Core /// `owned_card_id`, flag the captain, derive the bench split from the fixed diff --git a/openfut-adapter-fifa17/src/fut/squad_ext.rs b/openfut-adapter-fifa17/src/fut/squad_ext.rs index d15f402..724d245 100644 --- a/openfut-adapter-fifa17/src/fut/squad_ext.rs +++ b/openfut-adapter-fifa17/src/fut/squad_ext.rs @@ -61,7 +61,7 @@ pub struct KicktakerRef { } /// FIFA 17 Squad Extension, version 1. Serialized to the opaque payload Core stores. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct Fifa17SquadExtensionV1 { /// Opaque 33-int array as a JSON-encoded string, verbatim. Never decoded. #[serde(default)] diff --git a/openfut-utas-host/src/lib.rs b/openfut-utas-host/src/lib.rs index b2064e6..895f542 100644 --- a/openfut-utas-host/src/lib.rs +++ b/openfut-utas-host/src/lib.rs @@ -76,9 +76,13 @@ use openfut_adapter_fifa17::fut::owned_query::{ use openfut_adapter_fifa17::fut::pack_content::GeneratedCandidate; use openfut_adapter_fifa17::fut::sbc as fifa17_sbc; use openfut_adapter_fifa17::fut::season_wire; -use openfut_adapter_fifa17::fut::squad::{parse_squad_put, save_ack, SquadWireResolver}; +use openfut_adapter_fifa17::fut::squad::{ + classify_squad_put, save_ack, Fifa17SquadPut, Fifa17SquadRolePatch, SquadMutation, + SquadWireResolver, +}; use openfut_adapter_fifa17::fut::squad_ext::{ - build_squad_write, Fifa17SquadExtensionV1, SquadBuildError, EXT_NAMESPACE, EXT_SCHEMA_VERSION, + build_squad_write, Fifa17SquadExtensionV1, KicktakerRef, SquadBuildError, WireItemRef, + EXT_NAMESPACE, EXT_SCHEMA_VERSION, }; use openfut_adapter_fifa17::fut::squad_projection::{ project_squad, squad_actives, squad_list, user_mass_info_squad, ProjectionSlot, SquadExtInput, @@ -810,6 +814,21 @@ pub struct CoreReplaceRequest { pub ext_payload: String, } +/// A role-only squad patch: captain plus the opaque extension, nothing else. +/// +/// Deliberately has no `slots`, `name`, `formation` or manager field — the +/// absence is structural, so this request cannot express a squad replacement +/// even by mistake. +#[derive(Debug, Clone)] +pub struct CoreRolePatchRequest { + /// Owned instance to flag as captain. `None` leaves the captain unchanged; + /// it is never a request to clear it. + pub captain_owned_card_id: Option, + pub ext_namespace: String, + pub ext_schema_version: i64, + pub ext_payload: String, +} + /// Client-reported shadow evaluation carried through to Core (never Core's /// authoritative evaluation). #[derive(Debug, Clone, Default)] @@ -878,6 +897,18 @@ pub trait CoreAccess: Send + Sync { /// Replace the active squad's canonical slots + opaque extension atomically. fn replace_squad(&self, req: &CoreReplaceRequest) -> Result; + /// Patch ONLY the active squad's role assignments (captain) + opaque + /// extension. Never touches player assignments, the manager, or actives. + /// + /// Separate from [`Self::replace_squad`] because they are different + /// operations: a role-only client update carries no slot array, and sending + /// it as a replacement presents zero slots to Core's empty-replacement + /// guard. Default is `Status(501)` so a transport that has not implemented + /// it fails loudly instead of silently degrading into a replacement. + fn patch_squad_roles(&self, _req: &CoreRolePatchRequest) -> Result<(), CoreError> { + Err(CoreError::Status(501)) + } + /// The owned instance id assigned as the active squad's **manager**, or /// `None` (`GET /club/manager`). Default: `None` — a transport without the /// endpoint simply projects no manager (non-fatal, like an absent @@ -1050,6 +1081,29 @@ impl CoreAccess for HttpCoreClient { }) } + fn patch_squad_roles(&self, req: &CoreRolePatchRequest) -> Result<(), CoreError> { + let url = format!("{}/squad/roles", self.base_url); + let resp = self + .client + .put(&url) + .header("X-OpenFUT-Game", &self.game) + .json(&json!({ + "captain_owned_card_id": req.captain_owned_card_id, + "extension": { + "namespace": req.ext_namespace, + "schema_version": req.ext_schema_version, + "payload": req.ext_payload, + }, + })) + .send() + .map_err(|e| CoreError::Http(e.to_string()))?; + let status = resp.status().as_u16(); + if !(200..300).contains(&status) { + return Err(CoreError::Status(status)); + } + Ok(()) + } + fn get_squad_manager(&self) -> Result, CoreError> { let url = format!("{}/club/manager", self.base_url); let resp = self @@ -2844,26 +2898,35 @@ fn error_response(status: u16, code: &str) -> WireResponse { } } -/// `PUT …/squad/` — parse the full replacement, reverse-resolve every wire id, -/// AUTHORIZE every resolved item against the active club, then commit the -/// canonical squad + FIFA extension to Core in one transaction. On any failure -/// it returns an error and NEVER falls back to Python (no double mutation). +/// `PUT …/squad/` — dispatch on which mutation the body actually expresses. +/// +/// FIFA 17 sends two operations down this one path and distinguishes them only +/// by body shape (see [`SquadMutation`]). Classifying FIRST is the whole fix: a +/// role-only update carries no `players`, and routing it into the replacement +/// path presents zero slots to Core's empty-replacement guard, which correctly +/// refuses it — silently losing the user's captain/kick-taker change. pub fn handle_put_squad(body: &[u8], deps: &SquadDeps<'_>) -> (WireResponse, SquadLog) { - let put = match parse_squad_put(body) { - Ok(p) => p, - Err(e) => { - return ( - error_response(400, "parse_error"), - SquadLog { - outcome: "parse_error", - detail: e.to_string(), - }, - ) - } - }; + match classify_squad_put(body) { + Ok(SquadMutation::Replace(put)) => handle_squad_replace(&put, deps), + Ok(SquadMutation::PatchRoles(patch)) => handle_squad_role_patch(&patch, deps), + Err(e) => ( + error_response(400, "parse_error"), + SquadLog { + outcome: "parse_error", + detail: e.to_string(), + }, + ), + } +} + +/// The full-replacement path: reverse-resolve every wire id, AUTHORIZE every +/// resolved item against the active club, then commit the canonical squad + +/// FIFA extension to Core in one transaction. On any failure it returns an +/// error and NEVER falls back to Python (no double mutation). +fn handle_squad_replace(put: &Fifa17SquadPut, deps: &SquadDeps<'_>) -> (WireResponse, SquadLog) { // Reverse-resolve wire→owned and shape canonical + extension. Refuses on an // unresolved wire id or the same owned item placed twice. - let build = match build_squad_write(&put, deps.resolver) { + let build = match build_squad_write(put, deps.resolver) { Ok(b) => b, Err(SquadBuildError::UnresolvedWireIds(ids)) => { return ( @@ -3004,6 +3067,153 @@ pub fn handle_put_squad(body: &[u8], deps: &SquadDeps<'_>) -> (WireResponse, Squ ) } +/// `PUT …/squad/` carrying NO `players` — the role-only patch path. +/// +/// Observed real-client shape: `{id, custom, captain, kicktakers[5]}`, emitted +/// by the captain/kick-taker screen. Omission is the contract here: the absent +/// `players`, `manager` and actives mean UNCHANGED, never cleared. That is +/// enforced structurally — [`CoreRolePatchRequest`] has no field able to express +/// any of them, and Core's patch issues no statement that can touch them. +/// +/// The extension is MERGED, not overwritten: the patch body carries only +/// `custom` and `kicktakers`, so kit numbers, squad type and the client-reported +/// evaluation are preserved from the stored copy. Overwriting would silently +/// drop every kit number in the squad. +fn handle_squad_role_patch( + patch: &Fifa17SquadRolePatch, + deps: &SquadDeps<'_>, +) -> (WireResponse, SquadLog) { + // Start from the stored extension so omitted FIFA-only state survives. A + // stale extension is still the right base: its kit numbers belong to the + // same squad, and this patch re-anchors the fingerprint on commit. + let read = match deps.core.read_squad_ext(EXT_NAMESPACE) { + Ok(r) => r, + Err(e) => { + return ( + error_response(502, "core_error"), + SquadLog { + outcome: "core_error", + detail: e.to_string(), + }, + ) + } + }; + let mut ext = match &read.ext { + CoreExtState::Fresh { + schema_version, + payload, + } + | CoreExtState::Stale { + schema_version, + payload, + } => Fifa17SquadExtensionV1::from_payload(*schema_version, payload).unwrap_or_default(), + CoreExtState::Missing => Fifa17SquadExtensionV1::default(), + }; + + // Carry the patch's own fields through. `custom` genuinely differs between + // the two shapes -- the role screen writes per-slot values into it -- so it + // is taken from the patch, not preserved. + if patch.custom.is_some() { + ext.custom = patch.custom.clone(); + } + ext.kicktakers = patch + .kicktakers + .iter() + .map(|k| KicktakerRef { + index: k.index, + item: WireItemRef { + id: k.id, + dream: k.dream, + }, + }) + .collect(); + + // Resolve + AUTHORIZE the captain before any write. Identity resolution is + // not authorization: a globally-valid wire id belonging to another profile + // must not become this club's captain. + let mut captain_owned_card_id = None; + if let Some(wire) = patch.captain.filter(|c| *c != 0) { + match deps.resolver.owned_id_for_wire(wire) { + Some(owned) => { + let owned_set: std::collections::HashSet = match deps.core.all_owned() { + Ok(v) => v.into_iter().map(|i| i.owned_card_id).collect(), + Err(e) => { + return ( + error_response(502, "core_error"), + SquadLog { + outcome: "core_error", + detail: e.to_string(), + }, + ) + } + }; + if !owned_set.contains(&owned) { + return ( + error_response(403, "not_owned"), + SquadLog { + outcome: "unauthorized_captain", + detail: owned, + }, + ); + } + captain_owned_card_id = Some(owned); + } + None => { + // Refuse rather than silently patch the kicktakers alone: the + // user asked for a captain and would otherwise be told it + // succeeded while the captain never moved. + return ( + error_response(400, "unresolved_captain"), + SquadLog { + outcome: "unresolved_captain", + detail: wire.to_string(), + }, + ); + } + } + } + + let req = CoreRolePatchRequest { + captain_owned_card_id, + ext_namespace: EXT_NAMESPACE.to_string(), + ext_schema_version: EXT_SCHEMA_VERSION, + ext_payload: ext.to_payload(), + }; + if let Err(e) = deps.core.patch_squad_roles(&req) { + // A Core 400 means the REQUEST was invalid (e.g. a captain not fielded + // in this squad). Reporting that as 502 would blame the server for a + // client error and hide it behind "upstream unavailable". + let (status, code) = match e { + CoreError::Status(400) => (400, "invalid_role_patch"), + _ => (502, "core_error"), + }; + return ( + error_response(status, code), + SquadLog { + outcome: if status == 400 { + "invalid_role_patch" + } else { + "core_error" + }, + detail: e.to_string(), + }, + ); + } + eprintln!( + "utas-host owner=RUST route=squad-roles captain={:?} kicktakers={} \ + (players/manager/actives untouched)", + req.captain_owned_card_id, + ext.kicktakers.len() + ); + ( + json_response(&save_ack(patch.id)), + SquadLog { + outcome: "ok", + detail: String::new(), + }, + ) +} + /// `GET …/squad/list` — served from Core + the ONE projector. Stale/Missing are /// integrity failures for the migrated dev profile: logged prominently, degraded /// to an empty list, NEVER served from Python and NEVER projected from stale ext. diff --git a/openfut-utas-host/tests/host_test.rs b/openfut-utas-host/tests/host_test.rs index 51ae98a..079fd2e 100644 --- a/openfut-utas-host/tests/host_test.rs +++ b/openfut-utas-host/tests/host_test.rs @@ -16,8 +16,9 @@ use openfut_utas_host::account_store::AccountStore; use openfut_utas_host::{ classify, handle_club, handle_put_squad, handle_squad_active, handle_squad_list, handle_user_mass_info, read_request, ClubDeps, CoreAccess, CoreError, CoreExtState, - CoreKitAssignments, CorePage, CoreReplaceRequest, CoreReplaceResult, CoreSquadRead, - CoreSquadSlot, Fifa17IdentityResolver, HttpCoreClient, PassClient, Route, Server, SquadDeps, + CoreKitAssignments, CorePage, CoreReplaceRequest, CoreReplaceResult, CoreRolePatchRequest, + CoreSquadRead, CoreSquadSlot, Fifa17IdentityResolver, HttpCoreClient, PassClient, Route, + Server, SquadDeps, }; use parking_lot::Mutex; use serde_json::Value; @@ -51,6 +52,10 @@ struct FakeCore { /// full squad replacement writes it (or clears it with `None`). manager: Mutex>, replaced: Mutex>, + /// Recorded role-only patches: (captain_owned_card_id, ext_payload). Kept + /// separate from `replaced` so a test can assert a patch NEVER went through + /// the replacement path. + role_patches: Mutex, String)>>, panic_if_called: bool, return_err: bool, } @@ -95,6 +100,9 @@ impl FakeCore { fn last(&self) -> Vec<(String, String)> { self.last_params.lock().clone() } + fn role_patches(&self) -> Vec<(Option, String)> { + self.role_patches.lock().clone() + } fn manager(&self) -> Option { self.manager.lock().clone() } @@ -132,6 +140,45 @@ impl CoreAccess for FakeCore { self.squad.lock().clone().ok_or(CoreError::Status(404)) } + fn patch_squad_roles(&self, req: &CoreRolePatchRequest) -> Result<(), CoreError> { + assert!( + !self.panic_if_called, + "Core must NOT be called on this path" + ); + if self.return_err { + return Err(CoreError::Status(500)); + } + // Mirror Core: the captain must already be fielded, otherwise 400. + if let Some(captain) = &req.captain_owned_card_id { + let fielded = self + .squad + .lock() + .as_ref() + .map(|s| s.slots.iter().any(|sl| &sl.owned_card_id == captain)) + .unwrap_or(false); + if !fielded { + return Err(CoreError::Status(400)); + } + } + self.role_patches + .lock() + .push((req.captain_owned_card_id.clone(), req.ext_payload.clone())); + // Apply to the stored squad WITHOUT touching slots or the manager, so a + // read-after-patch shows exactly what Core would show. + if let Some(sq) = self.squad.lock().as_mut() { + if let Some(captain) = &req.captain_owned_card_id { + for slot in sq.slots.iter_mut() { + slot.is_captain = &slot.owned_card_id == captain; + } + } + sq.ext = CoreExtState::Fresh { + schema_version: req.ext_schema_version, + payload: req.ext_payload.clone(), + }; + } + Ok(()) + } + fn replace_squad(&self, req: &CoreReplaceRequest) -> Result { assert!( !self.panic_if_called, @@ -1213,6 +1260,141 @@ fn put_full_replacement_commits_canonical_and_extension_and_acks_id0() { assert_eq!(r.chemistry, Some(52), "client-reported shadow carried"); } +/// The REAL captured partial body must succeed and take the PATCH path, not the +/// replacement path. +/// +/// Captured 2026-08-25 00:42:42 from the retail client's captain/kick-taker +/// screen (`offline-seasons-squadexp-20260825T0018Z.pcap`). It carries no +/// `players`, so the old code handed Core a replacement with zero slots; the +/// empty-replacement guard refused it (400) and the host reported 502, losing +/// the user's change. The body shape is the operation discriminator. +#[test] +fn put_partial_captain_and_kicktakers_patches_roles_without_replacing() { + let items = vec![gk(), st()]; + let (resolver, w) = resolver_with_wires(&items, ASSETS); + let core = FakeCore::new(items.clone(), 2); + let ent = entities(); + let deps = SquadDeps { + core: &core, + resolver: &resolver, + entities: &ent, + squad_actives: false, + }; + // Establish a squad first, so there is something to patch. + let full = put_body( + "f442", + w["oc-a"], + &[(0, w["oc-a"], 1), (1, w["oc-b"], 9)], + "[1,2,3]", + Some(w["oc-b"]), + ); + let (resp, log) = handle_put_squad(&full, &deps); + assert_eq!(resp.status, 200, "{log:?}"); + assert_eq!(core.replaced().len(), 1); + + // The captured partial shape: no players, no manager, no formation. + let partial = format!( + r#"{{"id":0,"custom":"[0,8,16,16,8,134220032]","captain":{},"kicktakers":[ + {{"index":0,"id":{},"dream":false}},{{"index":1,"id":{},"dream":false}}, + {{"index":2,"id":{},"dream":false}},{{"index":3,"id":{},"dream":false}}, + {{"index":4,"id":{},"dream":false}}]}}"#, + w["oc-b"], w["oc-a"], w["oc-a"], w["oc-b"], w["oc-b"], w["oc-a"] + ); + let (resp, log) = handle_put_squad(partial.as_bytes(), &deps); + assert_eq!(resp.status, 200, "the partial shape must succeed: {log:?}"); + assert_eq!(resp.body, br#"{"id":0}"#, "same ack as a full save"); + + // It went through the PATCH path, never the replacement path. + assert_eq!( + core.replaced().len(), + 1, + "a role patch must NOT reach replace_squad — that is what tripped the guard" + ); + let patches = core.role_patches(); + assert_eq!(patches.len(), 1); + assert_eq!( + patches[0].0.as_deref(), + Some("oc-b"), + "captain resolved to the Core owned id, never the wire id" + ); + // Omitted state is UNCHANGED, not cleared. + assert_eq!( + core.manager().as_deref(), + Some("oc-b"), + "a role patch must not touch the manager" + ); + // The patch's opaque custom is carried, and kit numbers from the earlier + // full save survive the merge rather than being overwritten away. + assert!(patches[0].1.contains("134220032"), "patch custom carried"); + assert!( + patches[0].1.contains("kit_numbers"), + "kit numbers preserved from the stored extension: {}", + patches[0].1 + ); +} + +/// An explicit `"players": []` is still a REPLACEMENT and must still be refused +/// by Core's guard — the patch path must not become a way to smuggle a +/// destructive write past it. +#[test] +fn put_explicit_empty_players_is_still_a_replacement_not_a_patch() { + let items = vec![gk(), st()]; + let (resolver, _w) = resolver_with_wires(&items, ASSETS); + let core = FakeCore::new(items.clone(), 2); + let ent = entities(); + let deps = SquadDeps { + core: &core, + resolver: &resolver, + entities: &ent, + squad_actives: false, + }; + let body = br#"{"id":0,"formation":"f442","custom":"[]","players":[],"manager":[]}"#; + let (resp, log) = handle_put_squad(body, &deps); + // Reaches the replacement path (Core decides), and NEVER the patch path. + assert_eq!( + core.role_patches().len(), + 0, + "an explicit empty players array must not be treated as a role patch: {log:?}" + ); + assert!( + resp.status == 200 || resp.status >= 400, + "handled by the replacement path" + ); + assert_eq!( + core.replaced().len(), + 1, + "it went to replace_squad, where the empty-replacement guard lives" + ); +} + +/// A captain the resolver cannot map must refuse the whole patch, not silently +/// apply the kick-takers and report success. +#[test] +fn put_partial_with_unresolvable_captain_refuses_the_whole_patch() { + let items = vec![gk(), st()]; + let (resolver, w) = resolver_with_wires(&items, ASSETS); + let core = FakeCore::new(items.clone(), 2); + let ent = entities(); + let deps = SquadDeps { + core: &core, + resolver: &resolver, + entities: &ent, + squad_actives: false, + }; + let full = put_body("f442", w["oc-a"], &[(0, w["oc-a"], 1)], "[1]", None); + assert_eq!(handle_put_squad(&full, &deps).0.status, 200); + + let partial = br#"{"id":0,"custom":"[9]","captain":999999999,"kicktakers":[]}"#; + let (resp, log) = handle_put_squad(partial, &deps); + assert_eq!(resp.status, 400, "unresolvable captain is a client error"); + assert_eq!(log.outcome, "unresolved_captain"); + assert_eq!( + core.role_patches().len(), + 0, + "nothing may be written when the captain cannot be resolved" + ); +} + /// The squad's manager is an ownership-backed assignment, not an opaque wire /// echo: a save assigns the owned instance behind the ref. A later save that /// carries NO manager ref does NOT clear it.