Release every lock on every exit path

scripts/e2e_smoke.py

@@ -29,7 +29,7 @@
     "monitors": [], "extensions": [], "meta": {"semver": "3.0.0", "vm": "0.2.0", "agent": "smoke"},
 }
 
-USERS = {"sam": "u_sam", "alice": "u_alice", "bob": "u_bob", "dee": "u_dee"}
+USERS = {"sam": "u_sam", "alice": "u_alice", "bob": "u_bob", "dee": "u_dee", "mist": "u_mist"}
 FULL_STORAGE = {"u_dee"}
 GONE_USERS = {"gone_user"}
 BUSY_USERS = {"busy_user"}
@@ -40,7 +40,7 @@
 SCOPED_SESSIONS = {"sam_editor": ("sam", "editor")}
 VOLATILE_NAMES = {"token", "viewKey", "expires", "created", "edited", "firstSeen", "historyBytes"}
 UNORDERED = {"largest"}
-VOLATILE_FIELDS = {("notifications", "id"), ("largest", "bytes"), ("", "mistwarp")}
+VOLATILE_FIELDS = {("notifications", "id"), ("largest", "bytes"), ("", "mistwarp"), ("comment", "id"), ("item", "id"), ("items", "id"), ("platform_data", "commentId")}
 SAVE_PATH = re.compile(r"^/v1/projects/([^/]+)(/upload)?$")
 HEX = re.compile(r"^[0-9a-f]{32,64}$")
 ISO = re.compile(r"^\d{4}-\d{2}-\d{2}T")
@@ -229,6 +229,13 @@ def profile(self, username):
         with open(path) as handle:
             return json.load(handle)
 
+    def extension_policy(self):
+        path = os.path.join(self.dir, "data", "extensions.json")
+        if not os.path.exists(path):
+            return {}
+        with open(path) as handle:
+            return json.load(handle)
+
     def read_project(self, project_id):
         with open(os.path.join(self.dir, "data", "projects", project_id + ".json")) as handle:
             return json.load(handle)
@@ -264,6 +271,7 @@ def blob_inventory(self):
 EMBEDDED_HEX = re.compile(r"[0-9a-f]{32,64}")
 CONTENT_NAME = re.compile(r"^(assets/)?[0-9a-f]{32}\.[a-z0-9]+$|^project-json/[0-9a-f]{64}\.json\.gz$")
 ACCESS_KEY = re.compile(r"([?&]k=)[A-Za-z0-9]+")
+BACKPACK_FOLDER = re.compile(r"(/blobs/backpack/)[A-Za-z0-9]{32}/")
 EMBEDDED_TIME = re.compile(r"(?<![0-9])1[0-9]{12}(?![0-9])")
 
 
@@ -608,6 +616,22 @@ def head_and_edited():
     complete_part(step, path, "sam", tree_token, "complete staged merge tree")
     upload("upload pull merge", {"pullId": pull_index, "expectedHead": head, "mergeTreeToken": tree_token})
     step("merged pull", "GET", path + "/pulls/" + pull_index, "sam")
+    status, comment = step("comment on pull", "POST", path + "/pulls/" + pull_index + "/comments", "sam", {"content": "Thanks for this"})
+    step("comment on pull without content", "POST", path + "/pulls/" + pull_index + "/comments", "sam", {})
+    step("pull after a comment", "GET", path + "/pulls/" + pull_index, "sam")
+    comment_id = str(((comment.get("comment") or {}).get("id", "")) if isinstance(comment, dict) else "")
+    step("delete pull comment", "DELETE", path + "/pulls/" + pull_index + "/comments/" + comment_id, "sam")
+    step("delete missing pull comment", "DELETE", path + "/pulls/" + pull_index + "/comments/nope", "sam")
+    step("pull after deleting the comment", "GET", path + "/pulls/" + pull_index, "sam")
+    head, edited = head_and_edited()
+    step("credit commit co-author", "PATCH", path + "/commits/" + head, "sam", {"coAuthors": ["alice"]})
+    step("credit unknown commit", "PATCH", path + "/commits/" + "1" * 40, "sam", {"coAuthors": ["alice"]})
+    step("credit commit as stranger", "PATCH", path + "/commits/" + head, "bob", {"coAuthors": ["alice"]})
+    step("commit co-authors", "GET", path + "/commits/" + head + "/co-authors")
+    step("rename branch", "PATCH", path + "/branches", "sam", {"branch": "feature", "name": "feature-two"})
+    step("rename branch as stranger", "PATCH", path + "/branches", "bob", {"branch": "feature-two", "name": "x"})
+    step("delete branch", "DELETE", path + "/branches", "sam", {"branch": "feature-two"})
+    step("delete missing branch", "DELETE", path + "/branches", "sam", {"branch": "nope"})
     upload("upload merged pull again", {"pullId": pull_index}, {"project": plain, "mergeTree": pull_tree})
 
     head, edited = head_and_edited()
@@ -778,6 +802,59 @@ def timed(name, send):
     }})
 
 
+def locks_flow(server, step, steps, ids):
+    status, created = step("locks create", "POST", "/v1/projects", "sam", {"title": "Locks smoke"})
+    ids[str(created.get("id", "")) if isinstance(created, dict) else ""] = "<locks>"
+    path = "/v1/projects/<locks>"
+    shared = "/v1/projects/<history>"
+    step("react invalid", "POST", shared + "/react", "alice", {"type": "wow"})
+    step("react heart", "POST", shared + "/react", "alice", {"type": "heart"})
+    step("react broken heart", "POST", shared + "/react", "alice", {"type": "brokenheart"})
+    step("react missing project", "POST", "/v1/projects/nope/react", "alice", {"type": "heart"})
+    step("react on a private project", "POST", path + "/react", "alice", {"type": "heart"})
+    asset_name, asset_content = svg_asset("staged asset")
+    for label, name, content in (("staged asset", asset_name, asset_content), ("mismatched asset", svg_asset("never sent")[0], b"<svg/>")):
+        status, started = step("stage " + label + " start", "POST", path + "/upload-parts", "sam", {"kind": "asset", "name": name, "size": len(content), "chunks": 1})
+        token = started.get("token", "") if isinstance(started, dict) else ""
+        step("stage " + label + " chunk", "POST", path + "/upload-parts/%s/chunk" % token, "sam", form=({"index": "0"}, {"chunk": content}))
+        complete_part(step, path, "sam", token, "complete " + label)
+    step("check staged asset", "POST", path + "/assets/check", "sam", {"assets": [asset_name]})
+
+    for name, fields, files in (
+        ("backpack unsupported type", {"type": "sound", "mime": "text/plain"}, {"body": b"x"}),
+        ("backpack missing body", {"type": "costume", "mime": "image/svg+xml"}, {}),
+        ("backpack add", {"type": "costume", "mime": "image/svg+xml", "name": "Cat"}, {"body": b"<svg xmlns=\"http://www.w3.org/2000/svg\"/>"}),
+        ("backpack add with thumbnail", {"type": "costume", "mime": "image/svg+xml", "name": "Dog", "thumbnailMime": "image/png"}, {"body": b"<svg/>", "thumbnail": b"\x89PNG backpack"}),
+    ):
+        step(name, "POST", "/v1/me/backpack", "sam", form=(fields, files))
+    status, listed = step("backpack list", "GET", "/v1/me/backpack", "sam")
+    items = [item.get("id", "") for item in (listed.get("items") or [])] if isinstance(listed, dict) else []
+    first = items[0] if items else "none"
+    step("backpack rename", "PATCH", "/v1/me/backpack/" + first, "sam", {"name": "Renamed"})
+    step("backpack rename missing", "PATCH", "/v1/me/backpack/nope", "sam", {"name": "Renamed"})
+    step("backpack rename without name", "PATCH", "/v1/me/backpack/" + first, "sam", {})
+    step("backpack delete", "DELETE", "/v1/me/backpack/" + first, "sam")
+    step("backpack delete missing", "DELETE", "/v1/me/backpack/" + first, "sam")
+    step("backpack bulk delete", "POST", "/v1/me/backpack/delete", "sam", {"ids": items[1:] + ["nope"]})
+    step("backpack bulk delete empty", "POST", "/v1/me/backpack/delete", "sam", {"ids": []})
+    step("backpack after deletes", "GET", "/v1/me/backpack", "sam")
+    step("quota after backpack", "GET", "/v1/me/quota", "sam")
+
+    records = server.extension_records()
+    known_hash = sorted(records)[0][:-len(".json")] if records else "0" * 64
+    step("admin extension policy refused", "POST", "/v1/admin/extensions/policy", "sam", {"hash": known_hash, "status": "trusted"})
+    step("admin extension policy invalid", "POST", "/v1/admin/extensions/policy", "mist", {"hash": "nope", "status": "trusted"})
+    step("admin extension policy unknown", "POST", "/v1/admin/extensions/policy", "mist", {"hash": "f" * 64, "status": "trusted"})
+    step("admin extension policy trusted", "POST", "/v1/admin/extensions/policy", "mist", {"hash": known_hash, "status": "trusted"})
+    step("admin extension url blocked", "POST", "/v1/admin/extensions/url-policy", "mist", {"url": "https://example.com/blocked.js", "blocked": True})
+    step("admin extension url blocked again", "POST", "/v1/admin/extensions/url-policy", "mist", {"url": "https://example.com/blocked.js", "blocked": True})
+    step("admin extension url bad", "POST", "/v1/admin/extensions/url-policy", "mist", {"url": "ftp://example.com/x.js", "blocked": True})
+    steps.append({"step": "extension policy after blocking", "status": 0, "body": {"blockedUrls": server.extension_policy().get("blockedUrls")}})
+    step("admin extension url unblocked", "POST", "/v1/admin/extensions/url-policy", "mist", {"url": "https://example.com/blocked.js", "blocked": False})
+    steps.append({"step": "extension policy after unblocking", "status": 0, "body": {"blockedUrls": server.extension_policy().get("blockedUrls")}})
+    steps.append({"step": "extension records after policy changes", "status": 0, "body": server.extension_records()})
+
+
 def volatile_key(key):
     return key in VOLATILE_NAMES
 
@@ -807,7 +884,7 @@ def upload():
 
 def normalize(value, ids, key="", parent=""):
     if isinstance(value, dict):
-        return {k: normalize(v, ids, k, key) for k, v in sorted(value.items())}
+        return {("<hex>" if HEX.match(k) else k): normalize(v, ids, k, key) for k, v in sorted(value.items())}
     if isinstance(value, list):
         items = [normalize(v, ids, key, parent) for v in value]
         return sorted(items, key=json.dumps) if key in UNORDERED else items
@@ -819,7 +896,7 @@ def normalize(value, ids, key="", parent=""):
             value = value.replace(real, alias)
         if volatile_key(key) or HEX.match(value) or ISO.match(value):
             return "<volatile>"
-        value = ACCESS_KEY.sub(r"\1<key>", value)
+        value = BACKPACK_FOLDER.sub(r"\1<folder>/", ACCESS_KEY.sub(r"\1<key>", value))
         if CONTENT_NAME.match(value):
             return value
         return EMBEDDED_TIME.sub("<time>", EMBEDDED_HEX.sub("<hex>", value))
@@ -900,6 +977,7 @@ def step(name, method, path, who=None, body=None, upload=None, headers=(), form=
         update_flow(server, step, ids)
         concurrent_flow(server, step, steps, ids)
         slow_rotur_flow(server, step, steps, ids)
+        locks_flow(server, step, steps, ids)
         time.sleep(1.5)
         for real, alias in list(ids.items()):
             if alias in ("<history>", "<history-remix>", "<update>"):

src/admin/admin-users.osl

@@ -235,7 +235,17 @@ def handleSetAdminNote(*serve.Context c) (
     c.badRequest("note is too long")
     return
   )
+  boolean saved = saveAdminNoteLocked(c, key, text)
+  if !saved (
+    c.internalError("could not save the note")
+    return
+  )
+  c.json(200, {ok: true, note: adminNoteFor(key)})
+)
+
+def saveAdminNoteLocked(*serve.Context c, string key, string text) boolean (
   sync.lock("admin-notes")
+  defer sync.unlock("admin-notes")
   object data = loadAdminNotes()
   object notes = data.notes.assertElse(object, {})
   if text == "" (
@@ -245,12 +255,7 @@ def handleSetAdminNote(*serve.Context c) (
   )
   data.notes = notes
   boolean saved = writeFile(adminNotesFile(), data)
-  sync.unlock("admin-notes")
-  if !saved (
-    c.internalError("could not save the note")
-    return
-  )
-  c.json(200, {ok: true, note: adminNoteFor(key)})
+  return saved
 )
 
 def handleListUsers(*serve.Context c) (

src/admin/admin.osl

@@ -72,9 +72,9 @@ def bannedOwners() object (
   )
   // One rebuild at a time; the rest wait for it and use its result.
   sync.lock("banned-owners")
+  defer sync.unlock("banned-owners")
   if bansCache.has("owners") (
     object built = bansCache.get("owners").assert(object)
-    sync.unlock("banned-owners")
     return built
   )
   object bans = cachedBans().bans.assertElse(object, {})
@@ -90,7 +90,6 @@ def bannedOwners() object (
   )
   object banned = bannedOwnerSet(bans, idsByKey)
   bansCache.set("owners", banned)
-  sync.unlock("banned-owners")
   return banned
 )
 
@@ -173,19 +172,16 @@ def liftRoturBan(string username) boolean (
   return res.ok or res.status == 404
 )
 
-// The bans file is changed between these two, after any calls to Rotur, and
-// re-read so nothing written meanwhile is lost.
-def lockBans() object (
-  sync.lock("bans")
+def bansForUpdate() object (
   object data = loadBans()
   data.unbans = (data.unbans ?? {}).assertElse(object, {})
   return data
 )
 
-def saveAndUnlockBans(object data) boolean (
-  boolean saved = saveBans(data)
-  sync.unlock("bans")
-  return saved
+def bansSnapshotLocked() object (
+  sync.lock("bans")
+  defer sync.unlock("bans")
+  return bansForUpdate()
 )
 
 def banUser(string username, string reason, string actor) boolean (
@@ -194,12 +190,14 @@ def banUser(string username, string reason, string actor) boolean (
     return false
   )
   string rotur = pushRoturBan(username, reason)
-  object data = lockBans()
+  sync.lock("bans")
+  defer sync.unlock("bans")
+  object data = bansForUpdate()
   object bans = data.bans
   object unbans = data.unbans
   bans[key] = {reason: reason, by: actor, created: timestamp, rotur: rotur}
   unbans.delete(key)
-  return saveAndUnlockBans(data)
+  return saveBans(data)
 )
 
 def unbanUser(string username) boolean (
@@ -210,21 +208,22 @@ def unbanUser(string username) boolean (
   // Remembered until a sync has seen Rotur without it, so the sync keeps
   // trying if Rotur couldn't be told, and never brings the ban back.
   boolean lifted = liftRoturBan(username)
-  object data = lockBans()
+  sync.lock("bans")
+  defer sync.unlock("bans")
+  object data = bansForUpdate()
   object bans = data.bans
   object unbans = data.unbans
   bans.delete(key)
   unbans[key] = {at: timestamp, lifted: lifted}
-  return saveAndUnlockBans(data)
+  return saveBans(data)
 )
 
 // Keeps MistWarp's bans and the Rotur App's in step, every few minutes: bans
 // made or lifted on rotur.dev/me/developer reach MistWarp, and bans and unbans
 // Rotur couldn't be told about yet are sent again.
 def syncRoturBans() (
   number started = timestamp
-  object snapshot = lockBans()
-  sync.unlock("bans")
+  object snapshot = bansSnapshotLocked()
   object snapshotBans = snapshot.bans.assertElse(object, {})
   object snapshotUnbans = snapshot.unbans
   // Unbans first, so the list read next already reflects them.
@@ -266,7 +265,9 @@ def syncRoturBans() (
     )
   )
 
-  object data = lockBans()
+  sync.lock("bans")
+  defer sync.unlock("bans")
+  object data = bansForUpdate()
   object bans = data.bans
   object unbans = data.unbans
   boolean changed = false
@@ -344,7 +345,6 @@ def syncRoturBans() (
   if changed (
     saveBans(data)
   )
-  sync.unlock("bans")
 )
 
 def isSuperAdmin(string username) boolean (
@@ -938,19 +938,7 @@ def handleReportAction(*serve.Context c) (
     )
   )
 
-  sync.lock("reports-storage")
-  array currentReports = loadReports()
-  for i currentReports.len (
-    object currentReport = currentReports[i].assert(object)
-    if currentReport.id.toStr() == id (
-      currentReport.resolved = true
-      currentReport.resolvedBy = contextString(c, "username")
-      currentReport.action = action
-      currentReport.resolvedAt = timestamp
-    )
-  )
-  boolean reportsSaved = saveReports(currentReports)
-  sync.unlock("reports-storage")
+  boolean reportsSaved = markReportActionLocked(id, contextString(c, "username"), action)
   if !reportsSaved (
     c.internalError("could not update report")
     return
@@ -962,3 +950,19 @@ def handleReportAction(*serve.Context c) (
   addNotification(found.reporter.toStr(), contextString(c, "username"), "report_update", {action: action, reportType: found.type})
   c.json(200, {ok: true})
 )
+
+def markReportActionLocked(string id, string resolvedBy, string action) boolean (
+  sync.lock("reports-storage")
+  defer sync.unlock("reports-storage")
+  array currentReports = loadReports()
+  for i currentReports.len (
+    object currentReport = currentReports[i].assert(object)
+    if currentReport.id.toStr() == id (
+      currentReport.resolved = true
+      currentReport.resolvedBy = resolvedBy
+      currentReport.action = action
+      currentReport.resolvedAt = timestamp
+    )
+  )
+  return saveReports(currentReports)
+)

src/admin/errors.osl

@@ -273,13 +273,17 @@ def handleRecordSiteError(*serve.Context c) (
     created: timestamp,
     resolved: false
   })
-  sync.lock("site-errors")
-  void errorDocs.insertOne(entry)
-  sync.unlock("site-errors")
+  insertSiteErrorLocked(entry)
   void sweepSiteErrors()
   c.json(201, {ok: true})
 )
 
+def insertSiteErrorLocked(object entry) (
+  sync.lock("site-errors")
+  defer sync.unlock("site-errors")
+  void errorDocs.insertOne(entry)
+)
+
 def handleAdminGetErrors(*serve.Context c) (
   string show = (c.query("show") ?? "").toLower().strip()
   array items = errorDocs.sort("created", "desc").limit(200).all()
@@ -317,31 +321,39 @@ def handleAdminResolveSiteError(*serve.Context c) (
   if body.contains("resolved") (
     resolved = body.resolved == true
   )
-  sync.lock("site-errors")
-  object entry = (errorDocs.findById(id) ?? {})
-  if entry.len == 0 (
-    sync.unlock("site-errors")
-    c.notFound("error not found")
+  object res = resolveSiteErrorLocked(c, id, resolved)
+  if res.found != true (
     return
   )
-  entry.resolved = resolved
-  boolean saved = putDocument(errorDocs, id, entry)
-  sync.unlock("site-errors")
+  boolean saved = res.saved == true
   if saved (
     c.json(200, {ok: true})
     return
   )
   c.internalError("could not update error")
 )
 
+def resolveSiteErrorLocked(*serve.Context c, string id, boolean resolved) object (
+  sync.lock("site-errors")
+  defer sync.unlock("site-errors")
+  object entry = (errorDocs.findById(id) ?? {})
+  if entry.len == 0 (
+    c.notFound("error not found")
+    return {found: false}
+  )
+  entry.resolved = resolved
+  boolean saved = putDocument(errorDocs, id, entry)
+  return {found: true, saved: saved}
+)
+
 def deleteAllSiteErrors() number (
   sync.lock("site-errors")
+  defer sync.unlock("site-errors")
   array entries = errorDocs.all()
   for i entries.len (
     object entry = entries[i].assert(object)
     void errorDocs.deleteOne({_id: entry["_id"]})
   )
-  sync.unlock("site-errors")
   return entries.len
 )