diff --git a/server/internal/api/handlers_cameras.go b/server/internal/api/handlers_cameras.go index dcd92ff..84471a9 100644 --- a/server/internal/api/handlers_cameras.go +++ b/server/internal/api/handlers_cameras.go @@ -21,7 +21,13 @@ const snapshotTTL = 5 * time.Minute func (s *Server) handleCameras(w http.ResponseWriter, r *http.Request) { p := PrincipalFrom(r.Context()) - siteID := trim(r.URL.Query().Get("site_id")) + // siteParam, not Query().Get("site_id"): every other filtered endpoint + // takes both spellings, and this one took only the longer. `?site=chennai` + // was therefore not a filter but an unknown parameter, silently ignored - + // so a caller asking for one shop's cameras was handed the whole tenant's. + // Measured: it made a script skip creating cameras for three shops because + // another shop's already existed, and deleted a camera from the wrong shop. + siteID := siteParam(r) if siteID != "" { var ok bool if siteID, ok = s.resolveSiteFilter(w, r, siteID); !ok { diff --git a/server/internal/api/sitefilter_test.go b/server/internal/api/sitefilter_test.go new file mode 100644 index 0000000..800f4d6 --- /dev/null +++ b/server/internal/api/sitefilter_test.go @@ -0,0 +1,61 @@ +package api + +import ( + "encoding/json" + "net/http" + "testing" +) + +// Every endpoint that narrows by shop must accept BOTH spellings, because an +// unknown query parameter is silently ignored - so the wrong one is not an +// error, it is the whole estate returned as though it were one shop. That is a +// wrong answer nobody would question, and it has already caused a camera to be +// deleted from the wrong shop. +func TestEveryShopFilterAcceptsBothSpellings(t *testing.T) { + s, fs := newServer(t) + seedUser(fs) + fs.sites = []SiteHealth{ + {SiteID: siteA, Slug: "chennai", Name: "TeNext Coimbatore"}, + {SiteID: "site-other", Slug: "other-shop", Name: "Other Shop"}, + } + fs.cameras = []Camera{ + {ID: "c1", SiteID: siteA, CameraID: "entrance"}, + {ID: "c2", SiteID: "site-other", CameraID: "backdoor"}, + } + sess := login(t, s, "manager@acme.com", "correct horse battery") + + for _, path := range []string{ + "/api/cameras?site=chennai", + "/api/cameras?site_id=" + siteA, + } { + rec := do(t, s, "GET", path, sess.Token, nil) + if rec.Code != http.StatusOK { + t.Fatalf("%s: got %d: %s", path, rec.Code, rec.Body.String()) + } + var got []Camera + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("%s: %v", path, err) + } + if len(got) != 1 || got[0].CameraID != "entrance" { + t.Errorf("%s returned %d cameras %v - a shop filter that does not filter hands back the whole tenant", + path, len(got), names(got)) + } + } + + // And with no filter at all, the tenant's cameras - which is the only case + // that should ever return more than one shop's. + rec := do(t, s, "GET", "/api/cameras", sess.Token, nil) + var all []Camera + _ = json.Unmarshal(rec.Body.Bytes(), &all) + if len(all) != 2 { + t.Errorf("unfiltered list returned %d, want 2", len(all)) + } +} + +func names(cams []Camera) []string { + out := make([]string, 0, len(cams)) + for _, c := range cams { + out = append(out, c.CameraID) + } + return out +}