Fix detection of a bad UPDATE in response to PUT (#3233)

Because of a bad test for the UPDATE's success, file updates were committed
Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1384817

bin/require.pip

@@ -1,2 +1,3 @@
 flake8==3.3.0
 GitPython==2.1.3
+requests

server/src/server.js

@@ -649,6 +649,11 @@ app.put("/data/:id/:domain", function(req, res) {
     }
     return inserted;
   }).then((commands) => {
+    if (!commands) {
+      mozlog.warn("invalid-put-update", {msg: "Attempt to PUT to existing shot by non-owner", ip: req.ip});
+      simpleResponse(res, 'No shot updated', 403);
+      return;
+    }
     commands = commands || [];
     simpleResponse(res, JSON.stringify({updates: commands.filter((x) => !!x)}), 200);
   }).catch((err) => {

server/src/servershot.js

@@ -227,13 +227,16 @@ class Shot extends AbstractShot {
         WHERE id = $5 AND deviceid = $6`,
         [JSON.stringify(json), this.url, this.title, searchable.version, this.id, this.ownerId].concat(searchable.args)
       );
-      return promise.then((rowCount) => {
-        if (!rowCount) {
-          throw new Error("No row updated");
+      return promise.then((result) => {
+        if (!result.rowCount) {
+          clipRewrites.revertShotUrls();
+          return false;
         }
-        return clipRewrites.commit(client);
-      }).then(() => {
-        return oks;
+        return clipRewrites.commit(client).then(() => {
+          return true;
+        });
+      }).then((updated) => {
+        return updated ? oks : null;
       });
     });
   }

test/server/clientlib.py

@@ -0,0 +1,131 @@
+import os
+import re
+import requests
+from urlparse import urljoin
+import json
+import uuid
+import random
+import time
+
+example_images = {}
+execfile(os.path.normpath(os.path.join(__file__, "../../../bin/load_test_exercise_images.py")), example_images)
+example_images = example_images["example_images"]
+
+
+class ScreenshotsClient(object):
+
+    def __init__(self, backend="http://localhost:10080"):
+        self.backend = backend
+        self.deviceInfo = make_device_info()
+        self.deviceId = make_uuid()
+        self.secret = make_uuid()
+        self.session = requests.Session()
+
+    def login(self):
+        resp = self.session.post(
+            urljoin(self.backend, "/api/login"),
+            data=dict(deviceId=self.deviceId, secret=self.secret, deviceInfo=json.dumps(self.deviceInfo)))
+        if resp.status_code == 404:
+            resp = self.session.post(
+                urljoin(self.backend, "/api/register"),
+                data=dict(deviceId=self.deviceId, secret=self.secret, deviceInfo=json.dumps(self.deviceInfo)))
+        resp.raise_for_status()
+
+    def delete_account(self):
+        resp = self.session.post(
+            urljoin(self.backend, "/leave-screenshots/leave"),
+            json={})
+        resp.raise_for_status()
+
+    def create_shot(self, shot_id=None, **example_args):
+        if not shot_id:
+            shot_id = make_random_id() + "/test.com"
+        shot_url = urljoin(self.backend, shot_id)
+        shot_data = urljoin(self.backend, "data/" + shot_id)
+        shot_json = make_example_shot(self.deviceId, **example_args)
+        resp = self.session.put(
+            shot_data,
+            json=shot_json,
+        )
+        resp.raise_for_status()
+        print("status", resp.status_code)
+        return shot_url
+
+    def read_shot(self, url):
+        # FIXME: should get at least the clip image subresource itself
+        resp = self.session.get(url)
+        resp.raise_for_status()
+        page = resp.text
+        clip_match = re.search(r'<img id="clipImage"[^>]*src="([^"]+)"', page)
+        clip_url = clip_content = None
+        if clip_match:
+            clip_url = clip_match.group(1)
+            clip_content = self.session.get(clip_url).content
+        return {"page": page, "clip_url": clip_url, "clip_content": clip_content}
+
+    def read_my_shots(self):
+        resp = self.session.get(urljoin(self.backend, "/shots"))
+        resp.raise_for_status()
+
+    def search_shots(self, q):
+        resp = self.session.get(urljoin(self.backend, "/shots"), params={"q": q})
+        resp.raise_for_status()
+
+
+def make_example_shot(deviceId, **overrides):
+    image = random.choice(example_images)
+    text = []
+    for i in range(10):
+        text.append(random.choice(text_strings))
+    text = " ".join(text)
+    return dict(
+        deviceId=deviceId,
+        url="http://test.com/?" + make_uuid(),
+        docTitle=overrides.get("docTitle", "Load test page"),
+        createdDate=int(time.time() * 1000),
+        favicon=None,
+        siteName="test site",
+        clips={
+            make_uuid(): dict(
+                createdDate=int(time.time() * 1000),
+                sortOrder=100,
+                image=dict(
+                    url=image["url"],
+                    captureType="selection",
+                    text=text,
+                    location=dict(
+                        top=100,
+                        left=100,
+                        bottom=100 + image["height"],
+                        right=100 + image["width"],
+                    ),
+                    dimensions=dict(
+                        x=image["width"],
+                        y=image["height"],
+                    ),
+                ),
+            ),
+        },
+    )
+
+
+text_strings = """
+Example strings like apple orange banana some stuff like whatever and whoever
+and bucket blanket funky etc keyboard screen house window tree leaf leaves
+feather feathers
+""".split()
+
+
+def make_device_info():
+    return dict(
+        addonVersion='0.1.2014test',
+        platform='test',
+    )
+
+
+def make_uuid():
+    return str(uuid.uuid1()).replace("-", "")
+
+
+def make_random_id():
+    return make_uuid()[:16]

test/server/test_shot_put_auth.py

@@ -0,0 +1,38 @@
+#!../../.venv/bin/python
+
+import urlparse
+from clientlib import ScreenshotsClient
+import random
+import requests
+
+# Hack to make this predictable:
+random.seed(0)
+
+
+def test_put_auth():
+    first_user = ScreenshotsClient()
+    second_user = ScreenshotsClient()
+    first_user.login()
+    second_user.login()
+    shot_url = first_user.create_shot(docTitle="A_TEST_SITE_1")
+    shot_id = urlparse.urlsplit(shot_url).path.strip("/")
+    shot_page = first_user.read_shot(shot_url)
+    print(first_user.read_shot(shot_url)["clip_url"], shot_page["clip_url"])
+    assert first_user.read_shot(shot_url)["clip_content"] == shot_page["clip_content"]
+    assert "A_TEST_SITE_1" in shot_page["page"]
+    try:
+        second_user.create_shot(shot_id=shot_id, docTitle="A_TEST_SITE_2")
+    except requests.HTTPError, e:
+        if e.response.status_code != 403:
+            raise
+    else:
+        assert False, "Second attempt to upload should have failed"
+    second_shot_page = first_user.read_shot(shot_url)
+    assert "A_TEST_SITE_1" in second_shot_page["page"]
+    assert "A_TEST_SITE_2" not in second_shot_page["page"]
+    assert shot_page["clip_url"] == second_shot_page["clip_url"]
+    assert shot_page["clip_content"] == second_shot_page["clip_content"]
+
+
+if __name__ == "__main__":
+    test_put_auth()