diff --git a/pkg/models/models.go b/pkg/models/models.go index a6d89f1..a158885 100644 --- a/pkg/models/models.go +++ b/pkg/models/models.go @@ -597,6 +597,16 @@ type ServiceDeviceInfo struct { DiscoveryMethod string `json:"discovery_method,omitempty"` AccountID string `json:"account_id,omitempty"` Components []ServiceComponent `json:"components,omitempty" xml:"-"` + // CreatedOn is the ISO8601 timestamp the device was first + // registered against the account. Preserved across renames so + // AfterTouch's PUT response matches real Bose's "first paired + // in 2017" semantics rather than rewriting `now()` on every + // update. Empty for never-persisted records. + CreatedOn string `json:"created_on,omitempty" xml:"-"` + // UpdatedOn is the ISO8601 timestamp of the most recent change + // to the device record (rename, IP refresh, …). Refreshed by + // every SaveDeviceInfo write that mutates a known device. + UpdatedOn string `json:"updated_on,omitempty" xml:"-"` } // ServiceComponent represents a hardware or software component of a device. diff --git a/pkg/service/datastore/datastore.go b/pkg/service/datastore/datastore.go index 2752828..1ee93c9 100644 --- a/pkg/service/datastore/datastore.go +++ b/pkg/service/datastore/datastore.go @@ -565,6 +565,8 @@ func (ds *DataStore) getDeviceInfoNoLock(account, device string) (*models.Servic MacAddress string `xml:"macAddress"` } `xml:"networkInfo"` DiscoveryMethod string `xml:"discoveryMethod"` + CreatedOn string `xml:"createdOn,omitempty"` + UpdatedOn string `xml:"updatedOn,omitempty"` } if err := xml.Unmarshal(data, &info); err != nil { @@ -577,6 +579,8 @@ func (ds *DataStore) getDeviceInfoNoLock(account, device string) (*models.Servic ProductCode: fmt.Sprintf("%s %s", info.Type, info.ModuleType), Name: info.Name, DiscoveryMethod: info.DiscoveryMethod, + CreatedOn: info.CreatedOn, + UpdatedOn: info.UpdatedOn, } for _, comp := range info.Components { @@ -789,6 +793,8 @@ func (ds *DataStore) parseDeviceInfoFile(path string) (*models.ServiceDeviceInfo MacAddress string `xml:"macAddress"` } `xml:"networkInfo"` DiscoveryMethod string `xml:"discoveryMethod"` + CreatedOn string `xml:"createdOn,omitempty"` + UpdatedOn string `xml:"updatedOn,omitempty"` } if err := xml.Unmarshal(data, &info); err != nil { @@ -1192,6 +1198,8 @@ func (ds *DataStore) SaveDeviceInfo(account, device string, info *models.Service Components []componentXML `xml:"components>component"` NetworkInfo []NetworkInfoXML `xml:"networkInfo"` DiscoveryMethod string `xml:"discoveryMethod,omitempty"` + CreatedOn string `xml:"createdOn,omitempty"` + UpdatedOn string `xml:"updatedOn,omitempty"` } // Parsing product code back to type and moduleType (best effort) @@ -1203,6 +1211,8 @@ func (ds *DataStore) SaveDeviceInfo(account, device string, info *models.Service Type: devType, ModuleType: moduleType, DiscoveryMethod: info.DiscoveryMethod, + CreatedOn: info.CreatedOn, + UpdatedOn: info.UpdatedOn, } if ix.DiscoveryMethod == "" { @@ -1266,6 +1276,21 @@ func (ds *DataStore) mergeWithExistingDeviceInfo(account, device string, info *m if info.DiscoveryMethod == "" { info.DiscoveryMethod = existing.DiscoveryMethod } + + // CreatedOn is set once at first persistence and never re-derived + // from inbound data — preserve unconditionally so the + // "first-paired" timestamp survives renames, IP refreshes, etc. + // UpdatedOn is the opposite: every write that reaches here is by + // definition an update, so callers that want it refreshed must + // set it explicitly. If they didn't, fall back to the existing + // value (better than a regression to empty). + if existing.CreatedOn != "" { + info.CreatedOn = existing.CreatedOn + } + + if info.UpdatedOn == "" { + info.UpdatedOn = existing.UpdatedOn + } } func (ds *DataStore) parseProductCode(productCode string) (string, string) { diff --git a/pkg/service/handlers/handlers_marge.go b/pkg/service/handlers/handlers_marge.go index b3d2da4..e6927a6 100644 --- a/pkg/service/handlers/handlers_marge.go +++ b/pkg/service/handlers/handlers_marge.go @@ -588,7 +588,7 @@ func (s *Server) HandleMargeAddDevice(w http.ResponseWriter, r *http.Request) { return } - deviceID, data, err := marge.AddDeviceToAccount(s.ds, account, body) + deviceID, data, err := marge.AddDeviceToAccount(s.ds, account, body, r.RemoteAddr) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -637,7 +637,7 @@ func (s *Server) HandleMargeUpdateDevice(w http.ResponseWriter, r *http.Request) return } - bodyDeviceID, data, err := marge.AddDeviceToAccount(s.ds, account, body) + bodyDeviceID, data, err := marge.AddDeviceToAccount(s.ds, account, body, r.RemoteAddr) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return diff --git a/pkg/service/handlers/issue285_regression_test.go b/pkg/service/handlers/issue285_regression_test.go index 63c538a..7330a6b 100644 --- a/pkg/service/handlers/issue285_regression_test.go +++ b/pkg/service/handlers/issue285_regression_test.go @@ -65,20 +65,26 @@ func TestIssue285_RenamePutAcceptedAndPersisted(t *testing.T) { _ = ds.Initialize() const ( - accountID = "3981561" - deviceID = "884AEAEEBD27" - oldName = "Wohnzimmer" - newName = "Wohnzimmer SB" + accountID = "3981561" + deviceID = "884AEAEEBD27" + oldName = "Wohnzimmer" + newName = "Wohnzimmer SB" + preExistingIP = "192.168.0.109" + preExistingPaired = "2017-02-07T11:13:03.000+00:00" ) - // 1. Seed datastore with the device under its original name — - // modelling a pre-existing paired device the user is now - // renaming. + // 1. Seed datastore with the device under its original name and + // a known pre-existing first-paired timestamp. The pre-existing + // data models a long-paired device the user is now renaming — + // CreatedOn must survive the PUT (real Bose preserves it + // across renames; see parity capture at + // data/parity_mismatches/1771797308__streaming_account_3230304_device_A81B6A536A98.json). if err := ds.SaveDeviceInfo(accountID, deviceID, &models.ServiceDeviceInfo{ DeviceID: deviceID, AccountID: accountID, Name: oldName, - IPAddress: "192.168.0.109", + IPAddress: preExistingIP, + CreatedOn: preExistingPaired, }); err != nil { t.Fatalf("seed datastore: %v", err) } @@ -146,10 +152,35 @@ func TestIssue285_RenamePutAcceptedAndPersisted(t *testing.T) { t.Errorf("response still carries old name %q; body:\n%s", oldName, respBody) } + // Parity assertion: the pre-existing first-paired CreatedOn + // must survive the rename. This is the load-bearing fix versus + // the prior behaviour that rewrote `now()` on every PUT, and + // matches what real Bose's pre-shutdown 200 OK responses + // carried (see the parity capture referenced above). + if !bytes.Contains(respBody, []byte(``+preExistingPaired+``)) { + t.Errorf("response did not preserve pre-existing CreatedOn %q; body:\n%s", preExistingPaired, respBody) + } + + // Parity assertion: the pre-existing IP address must survive + // the rename. The request body doesn't carry an ``, + // so the datastore merge has to inject what was already on + // disk rather than writing back empty. + if !bytes.Contains(respBody, []byte(``+preExistingIP+``)) { + t.Errorf("response did not preserve pre-existing IPAddress %q; body:\n%s", preExistingIP, respBody) + } + + // Parity assertion: UpdatedOn refreshes. Don't pin the exact + // value — it's "now()" — but assert it's present and + // non-empty. + if !bytes.Contains(respBody, []byte(``)) || + bytes.Contains(respBody, []byte(``)) { + t.Errorf("response missing or empty ; body:\n%s", respBody) + } + // 4. Persistence assertion: the datastore now reflects the new - // name. This is what the Bose App reads back on its next - // /streaming/account/.../full poll, which is what closes the - // visible rename loop. + // name AND keeps the original CreatedOn. This is what the + // Bose App reads back on its next /streaming/account/.../full + // poll, which is what closes the visible rename loop. persisted, err := ds.GetDeviceInfo(accountID, deviceID) if err != nil { t.Fatalf("read persisted device info: %v", err) @@ -158,6 +189,109 @@ func TestIssue285_RenamePutAcceptedAndPersisted(t *testing.T) { if persisted.Name != newName { t.Errorf("persisted Name = %q, want %q", persisted.Name, newName) } + + if persisted.CreatedOn != preExistingPaired { + t.Errorf("persisted CreatedOn = %q, want %q (preserved across rename)", persisted.CreatedOn, preExistingPaired) + } + + if persisted.IPAddress != preExistingIP { + t.Errorf("persisted IPAddress = %q, want %q (preserved across rename)", persisted.IPAddress, preExistingIP) + } + + if persisted.UpdatedOn == "" { + t.Errorf("persisted UpdatedOn is empty; want a fresh timestamp from the rename") + } +} + +// TestIssue285_NewDeviceGetsRemoteAddrAndFreshTimestamps covers the +// "first-time registration" path on a PUT (which can happen if the +// speaker emits a rename before AfterTouch has ever heard of it). +// With no pre-existing datastore record: +// +// - CreatedOn must be a fresh timestamp (no record to preserve). +// - IPAddress must come from r.RemoteAddr (the inbound connection) +// since the request body doesn't carry one. +// - UpdatedOn must be the same fresh timestamp. +// +// Pairs with the parity-preservation assertions in the main test: +// existing records win, but new records seed sensibly instead of +// landing with empty CreatedOn / IPAddress. +func TestIssue285_NewDeviceGetsRemoteAddrAndFreshTimestamps(t *testing.T) { + tempDir, err := os.MkdirTemp("", "issue285-new-") + if err != nil { + t.Fatalf("mkdir temp: %v", err) + } + defer os.RemoveAll(tempDir) + + ds := datastore.NewDataStore(tempDir) + _ = ds.Initialize() + + const ( + accountID = "1111111" + deviceID = "A81B6A536A98" + newName = "Sound Machinechen" + ) + + r, _ := setupRouter("http://localhost:8001", ds) + ts := httptest.NewServer(r) + + t.Cleanup(ts.Close) + + body := []byte(`` + + `` + newName + `` + deviceID + ``) + + req, err := http.NewRequest(http.MethodPut, + ts.URL+"/streaming/account/"+accountID+"/device/"+deviceID, + bytes.NewReader(body)) + if err != nil { + t.Fatalf("build request: %v", err) + } + + req.Header.Set("Content-Type", "application/xml") + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("PUT: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + respBody, _ := io.ReadAll(resp.Body) + t.Fatalf("PUT status = %d, want 200; body:\n%s", resp.StatusCode, respBody) + } + + respBody, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read response: %v", err) + } + + // CreatedOn present and non-empty (will be "now()" since no + // prior record existed). + if !bytes.Contains(respBody, []byte(``)) || + bytes.Contains(respBody, []byte(``)) { + t.Errorf("first-registration response missing CreatedOn; body:\n%s", respBody) + } + + // IPAddress should be the httptest connection's remote host + // (127.0.0.1) since the body didn't carry one and there was + // no existing record to preserve from. + if !bytes.Contains(respBody, []byte(`127.0.0.1`)) { + t.Errorf("first-registration response missing IPAddress from RemoteAddr; body:\n%s", respBody) + } + + // Persistence: CreatedOn and IPAddress on disk too. + persisted, err := ds.GetDeviceInfo(accountID, deviceID) + if err != nil { + t.Fatalf("read persisted device info: %v", err) + } + + if persisted.CreatedOn == "" { + t.Errorf("persisted CreatedOn is empty for new device; want a fresh timestamp") + } + + if persisted.IPAddress != "127.0.0.1" { + t.Errorf("persisted IPAddress = %q, want %q (from RemoteAddr)", persisted.IPAddress, "127.0.0.1") + } } // TestIssue285_RenamePutRejectsMismatchedDeviceID pins the safety diff --git a/pkg/service/marge/marge.go b/pkg/service/marge/marge.go index cf7e1d9..4feedb9 100644 --- a/pkg/service/marge/marge.go +++ b/pkg/service/marge/marge.go @@ -7,6 +7,7 @@ import ( "encoding/xml" "fmt" "log" + "net" "os" "sort" "strconv" @@ -1816,8 +1817,26 @@ func formatRecentResponse(recentObj *models.ServiceRecent, matchingSrc *models.C return append([]byte(header+"\n"), data...) } -// AddDeviceToAccount adds a new device to the specified account. -func AddDeviceToAccount(ds *datastore.DataStore, account string, sourceXML []byte) (string, []byte, error) { +// AddDeviceToAccount upserts a device record for the given account. +// Called by both the device-create (POST) and device-rename (PUT) +// handlers — the persistence layer doesn't distinguish; only the +// response status differs. +// +// remoteAddr is the speaker's address as seen by the HTTP server +// (r.RemoteAddr, "host:port"). When the request body doesn't carry +// an `` and the datastore has no IP for this device yet, +// we fall back to remoteAddr's host portion. An empty remoteAddr +// is treated as "no fallback available" — never errors. +// +// Timestamps: +// - CreatedOn is preserved from any existing datastore record so a +// rename doesn't reset the "first paired in 2017" semantics real +// Bose emits. New devices get CreatedOn = now() at first save. +// - UpdatedOn is set to now() on every call. +// +// Returns the persisted deviceID and the marge XML response shape +// (``). +func AddDeviceToAccount(ds *datastore.DataStore, account string, sourceXML []byte, remoteAddr string) (string, []byte, error) { var newDeviceElem struct { DeviceID string `xml:"deviceid,attr"` Name string `xml:"name"` @@ -1827,28 +1846,68 @@ func AddDeviceToAccount(ds *datastore.DataStore, account string, sourceXML []byt return "", nil, err } + now := FormatTime(time.Now()) + + // Build the info to save. Empty fields are filled in by the + // datastore's mergeWithExistingDeviceInfo (which preserves IP, + // MAC, CreatedOn, etc.) before the write — so the precedence + // here is "explicit > merged > remoteAddr fallback". info := &models.ServiceDeviceInfo{ DeviceID: newDeviceElem.DeviceID, Name: newDeviceElem.Name, MacAddress: newDeviceElem.MACAddress, - // Other fields will be filled by discovery later or default + UpdatedOn: now, + } + + existing, _ := ds.GetDeviceInfo(account, newDeviceElem.DeviceID) + + // CreatedOn: preserve from existing record for renames; set + // now() only on first registration (no prior record OR the + // record has no CreatedOn — older AfterTouch installs may + // have records without one). + if existing != nil && existing.CreatedOn != "" { + info.CreatedOn = existing.CreatedOn + } else { + info.CreatedOn = now + } + + // IPAddress: prefer existing record's IP (the speaker may be + // hitting us through a different network path right now, e.g. + // SSH port-forward, and the persisted IP is the one other + // flows like DNS hints care about). Fall back to the inbound + // connection's remote address only when there's no existing + // IP to preserve. Invalid remoteAddr leaves info.IPAddress + // empty, which the merge then handles. + if existing == nil || existing.IPAddress == "" { + if remoteAddr != "" { + if host, _, splitErr := net.SplitHostPort(remoteAddr); splitErr == nil { + info.IPAddress = host + } + } } if err := ds.SaveDeviceInfo(account, newDeviceElem.DeviceID, info); err != nil { return "", nil, err } - createdOn := FormatTime(time.Now()) - res := fmt.Sprintf(``, EscapeXML(newDeviceElem.DeviceID)) - res += fmt.Sprintf(`%s`, EscapeXML(createdOn)) - res += `` - res += fmt.Sprintf(`%s`, EscapeXML(newDeviceElem.Name)) - res += fmt.Sprintf(`%s`, EscapeXML(createdOn)) + // Re-read the persisted record so the response XML reflects + // the merged state (preserved CreatedOn, preserved IP if the + // new info had none and the existing record did, etc.). + persisted, err := ds.GetDeviceInfo(account, newDeviceElem.DeviceID) + if err != nil { + return "", nil, fmt.Errorf("re-read persisted device info: %w", err) + } + + res := fmt.Sprintf(``, EscapeXML(persisted.DeviceID)) + res += fmt.Sprintf(`%s`, EscapeXML(persisted.CreatedOn)) + res += fmt.Sprintf(`%s`, EscapeXML(persisted.IPAddress)) + res += fmt.Sprintf(`%s`, EscapeXML(persisted.Name)) + res += fmt.Sprintf(`%s`, EscapeXML(persisted.UpdatedOn)) res += `` header := constants.XMLHeader - return newDeviceElem.DeviceID, append([]byte(header), []byte(res)...), nil + return persisted.DeviceID, append([]byte(header), []byte(res)...), nil } // RemoveDeviceFromAccount removes a device from the specified account.