From 86769c19bf306da3cae583e6d30c39d34e603c33 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Wed, 9 Mar 2022 11:19:23 -0500
Subject: [PATCH] Finish abort logic and tests.

---
 src/allmydata/storage/http_server.py    | 16 ++++--
 src/allmydata/test/test_storage_http.py | 68 +++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 5 deletions(-)

diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index 57e2b3e82..7659513e1 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -311,13 +311,19 @@ class HTTPServer(object):
             bucket = self._uploads.get_write_bucket(
                 storage_index, share_number, authorization[Secrets.UPLOAD]
             )
-        except _HTTPError:
-            # TODO 3877 If 404, check if this was already uploaded, in which case return 405
-            # TODO 3877 write tests for 404 cases?
+        except _HTTPError as e:
+            if e.code == http.NOT_FOUND:
+                # It may be we've already uploaded this, in which case error
+                # should be method not allowed (405).
+                try:
+                    self._storage_server.get_buckets(storage_index)[share_number]
+                except KeyError:
+                    pass
+                else:
+                    # Already uploaded, so we can't abort.
+                    raise _HTTPError(http.NOT_ALLOWED)
             raise
 
-        # TODO 3877 test for checking upload secret
-
         # Abort the upload; this should close it which will eventually result
         # in self._uploads.remove_write_bucket() being called.
         bucket.abort()
diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index 46914dbf2..97a81e250 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -891,3 +891,71 @@ class ImmutableHTTPAPITests(SyncTestCase):
                 b"ABC",
             )
         )
+
+    def test_unknown_aborts(self):
+        """
+        Aborting aborts with unknown storage index or share number will 404.
+        """
+        (upload_secret, _, storage_index, _) = self.create_upload({1}, 100)
+
+        for si, num in [(storage_index, 3), (b"x" * 16, 1)]:
+            with assert_fails_with_http_code(self, http.NOT_FOUND):
+                result_of(self.imm_client.abort_upload(si, num, upload_secret))
+
+    def test_unauthorized_abort(self):
+        """
+        An abort with the wrong key will return an unauthorized error, and will
+        not abort the upload.
+        """
+        (upload_secret, _, storage_index, _) = self.create_upload({1}, 100)
+
+        # Failed to abort becaues wrong upload secret:
+        with assert_fails_with_http_code(self, http.UNAUTHORIZED):
+            result_of(
+                self.imm_client.abort_upload(storage_index, 1, upload_secret + b"X")
+            )
+
+        # We can still write to it:
+        result_of(
+            self.imm_client.write_share_chunk(
+                storage_index,
+                1,
+                upload_secret,
+                0,
+                b"ABC",
+            )
+        )
+
+    def test_too_late_abort(self):
+        """
+        An abort of an already-fully-uploaded immutable will result in 405
+        error and will not affect the immutable.
+        """
+        uploaded_data = b"123"
+        (upload_secret, _, storage_index, _) = self.create_upload({0}, 3)
+        result_of(
+            self.imm_client.write_share_chunk(
+                storage_index,
+                0,
+                upload_secret,
+                0,
+                uploaded_data,
+            )
+        )
+
+        # Can't abort, we finished upload:
+        with assert_fails_with_http_code(self, http.NOT_ALLOWED):
+            result_of(self.imm_client.abort_upload(storage_index, 0, upload_secret))
+
+        # Abort didn't prevent reading:
+        self.assertEqual(
+            uploaded_data,
+            result_of(
+                self.imm_client.read_share_chunk(
+                    storage_index,
+                    0,
+                    0,
+                    3,
+                )
+            ),
+        )