Repository: getmoto/moto
Pull request #6295 (2023-05-07 15:45:48)

fix S3 PutObjectRetention and PutObjectLegalHold not using versionId
As reported in #6294, `PutObjectRetention` and `PutObjectLegalHold` would not take into account the `versionId` as it was not accessing it the same way as `PutObjectTagging` for example. I've pulled the retrieval of the `versionId` outside of the `if` blocks, so that all operations needing it could access it.

I've ran the tests against AWS as well and it confirms the behaviour the user was expecting. 

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/getmoto/moto/pull/6295?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
> Merging [#6295](https://app.codecov.io/gh/getmoto/moto/pull/6295?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (cedc1d1) into [master](https://app.codecov.io/gh/getmoto/moto/commit/763b582cf23bd916200388d7a9071e9b48c49e14?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (763b582) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##           master    #6295      +/-   ##
==========================================
- Coverage   96.33%   96.33%   -0.01%     
==========================================
  Files         792      792              
  Lines       76835    76833       -2     
==========================================
- Hits        74016    74014       -2     
  Misses       2819     2819              
```

| Flag | Coverage Δ | |
|---|---|---|
| servertests | `37.00% <66.66%> (+0.01%)` | :arrow_up: |
| unittests | `96.27% <100.00%> (-0.01%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://app.codecov.io/gh/getmoto/moto/pull/6295?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [moto/s3/responses.py](https://app.codecov.io/gh/getmoto/moto/pull/6295?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-bW90by9zMy9yZXNwb25zZXMucHk=) | `95.65% <100.00%> (-0.01%)` | :arrow_down: |



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3eefc5167..b3b79418b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,41 @@ Moto Changelog
 ==============
 
 
+4.1.9
+-----
+Docker Digest for 4.1.9: _sha256:121a51dcf0e42e53a601803bdeb25324d18b2d45f08a79923c65034f4f14448a_
+
+    New Services:
+        * Glue:
+            * batch_get_triggers
+            * create_trigger
+            * delete_trigger
+            * get_trigger
+            * get_triggers
+            * list_triggers
+            * start_trigger
+            * stop_trigger
+
+        * Glue:
+            * create_contact
+            * create_contact_list
+            * delete_contact
+            * delete_contact_list
+            * get_contact
+            * get_contact_list
+            * list_contact_lists
+            * list_contacts
+            * send_email
+
+    Miscellaneous:
+        * Kinesis: Improve calculations on stream limits
+        * EC2: Improve logic behind describe_image_attribute()
+        * S3: Various improvements to the logic behind copy_object()
+        * Scheduler: update_schedule() now supports the GroupName-parameter
+        * SNS: Improve and enhance validation of numeric parameters
+        * SNS: MessageDeduplicationId is now forwarded to SQS queues (Fixes a bug in 4.1.7)
+
+
 4.1.8
 -----
 Docker Digest for 4.1.8: _sha256:e83d868df71b193d625d9fb031282f6632c6c80d0314cfca6780f9a3f37d1f61_
diff --git a/IMPLEMENTATION_COVERAGE.md b/IMPLEMENTATION_COVERAGE.md
index 483e84d0c..8b22716bc 100644
--- a/IMPLEMENTATION_COVERAGE.md
+++ b/IMPLEMENTATION_COVERAGE.md
@@ -2036,7 +2036,7 @@
 - [X] describe_iam_instance_profile_associations
 - [ ] describe_id_format
 - [ ] describe_identity_id_format
-- [ ] describe_image_attribute
+- [X] describe_image_attribute
 - [X] describe_images
 - [ ] describe_import_image_tasks
 - [ ] describe_import_snapshot_tasks
@@ -4614,7 +4614,7 @@
 
 ## opensearch
 <details>
-<summary>17% implemented</summary>
+<summary>16% implemented</summary>
 
 - [ ] accept_inbound_connection
 - [X] add_tags
@@ -4634,6 +4634,8 @@
 - [ ] describe_domain_auto_tunes
 - [ ] describe_domain_change_progress
 - [X] describe_domain_config
+- [ ] describe_domain_health
+- [ ] describe_domain_nodes
 - [ ] describe_domains
 - [ ] describe_dry_run_progress
 - [ ] describe_inbound_connections
@@ -5028,7 +5030,7 @@
 
 ## quicksight
 <details>
-<summary>10% implemented</summary>
+<summary>9% implemented</summary>
 
 - [ ] cancel_ingestion
 - [ ] create_account_customization
@@ -5049,6 +5051,9 @@
 - [ ] create_template_alias
 - [ ] create_theme
 - [ ] create_theme_alias
+- [ ] create_topic
+- [ ] create_topic_refresh_schedule
+- [ ] create_vpc_connection
 - [ ] delete_account_customization
 - [ ] delete_account_subscription
 - [ ] delete_analysis
@@ -5067,8 +5072,11 @@
 - [ ] delete_template_alias
 - [ ] delete_theme
 - [ ] delete_theme_alias
+- [ ] delete_topic
+- [ ] delete_topic_refresh_schedule
 - [X] delete_user
 - [ ] delete_user_by_principal_id
+- [ ] delete_vpc_connection
 - [ ] describe_account_customization
 - [ ] describe_account_settings
 - [ ] describe_account_subscription
@@ -5100,7 +5108,12 @@
 - [ ] describe_theme
 - [ ] describe_theme_alias
 - [ ] describe_theme_permissions
+- [ ] describe_topic
+- [ ] describe_topic_permissions
+- [ ] describe_topic_refresh
+- [ ] describe_topic_refresh_schedule
 - [X] describe_user
+- [ ] describe_vpc_connection
 - [ ] generate_embed_url_for_anonymous_user
 - [ ] generate_embed_url_for_registered_user
 - [ ] get_dashboard_embed_url
@@ -5126,8 +5139,11 @@
 - [ ] list_theme_aliases
 - [ ] list_theme_versions
 - [ ] list_themes
+- [ ] list_topic_refresh_schedules
+- [ ] list_topics
 - [ ] list_user_groups
 - [X] list_users
+- [ ] list_vpc_connections
 - [ ] put_data_set_refresh_properties
 - [X] register_user
 - [ ] restore_analysis
@@ -5163,7 +5179,11 @@
 - [ ] update_theme
 - [ ] update_theme_alias
 - [ ] update_theme_permissions
+- [ ] update_topic
+- [ ] update_topic_permissions
+- [ ] update_topic_refresh_schedule
 - [ ] update_user
+- [ ] update_vpc_connection
 </details>
 
 ## ram
diff --git a/docs/docs/services/ec2.rst b/docs/docs/services/ec2.rst
index 9c0916363..40068783a 100644
--- a/docs/docs/services/ec2.rst
+++ b/docs/docs/services/ec2.rst
@@ -294,7 +294,7 @@ ec2
 - [X] describe_iam_instance_profile_associations
 - [ ] describe_id_format
 - [ ] describe_identity_id_format
-- [ ] describe_image_attribute
+- [X] describe_image_attribute
 - [X] describe_images
 - [ ] describe_import_image_tasks
 - [ ] describe_import_snapshot_tasks
diff --git a/docs/docs/services/opensearch.rst b/docs/docs/services/opensearch.rst
index df4f94819..a6728af5d 100644
--- a/docs/docs/services/opensearch.rst
+++ b/docs/docs/services/opensearch.rst
@@ -45,6 +45,8 @@ opensearch
 - [ ] describe_domain_auto_tunes
 - [ ] describe_domain_change_progress
 - [X] describe_domain_config
+- [ ] describe_domain_health
+- [ ] describe_domain_nodes
 - [ ] describe_domains
 - [ ] describe_dry_run_progress
 - [ ] describe_inbound_connections
diff --git a/docs/docs/services/quicksight.rst b/docs/docs/services/quicksight.rst
index a76ec70a6..c0e64b57b 100644
--- a/docs/docs/services/quicksight.rst
+++ b/docs/docs/services/quicksight.rst
@@ -46,6 +46,9 @@ quicksight
 - [ ] create_template_alias
 - [ ] create_theme
 - [ ] create_theme_alias
+- [ ] create_topic
+- [ ] create_topic_refresh_schedule
+- [ ] create_vpc_connection
 - [ ] delete_account_customization
 - [ ] delete_account_subscription
 - [ ] delete_analysis
@@ -64,8 +67,11 @@ quicksight
 - [ ] delete_template_alias
 - [ ] delete_theme
 - [ ] delete_theme_alias
+- [ ] delete_topic
+- [ ] delete_topic_refresh_schedule
 - [X] delete_user
 - [ ] delete_user_by_principal_id
+- [ ] delete_vpc_connection
 - [ ] describe_account_customization
 - [ ] describe_account_settings
 - [ ] describe_account_subscription
@@ -97,7 +103,12 @@ quicksight
 - [ ] describe_theme
 - [ ] describe_theme_alias
 - [ ] describe_theme_permissions
+- [ ] describe_topic
+- [ ] describe_topic_permissions
+- [ ] describe_topic_refresh
+- [ ] describe_topic_refresh_schedule
 - [X] describe_user
+- [ ] describe_vpc_connection
 - [ ] generate_embed_url_for_anonymous_user
 - [ ] generate_embed_url_for_registered_user
 - [ ] get_dashboard_embed_url
@@ -131,12 +142,15 @@ quicksight
 - [ ] list_theme_aliases
 - [ ] list_theme_versions
 - [ ] list_themes
+- [ ] list_topic_refresh_schedules
+- [ ] list_topics
 - [ ] list_user_groups
 - [X] list_users
   
         The NextToken and MaxResults parameters are not yet implemented
         
 
+- [ ] list_vpc_connections
 - [ ] put_data_set_refresh_properties
 - [X] register_user
   
@@ -177,5 +191,9 @@ quicksight
 - [ ] update_theme
 - [ ] update_theme_alias
 - [ ] update_theme_permissions
+- [ ] update_topic
+- [ ] update_topic_permissions
+- [ ] update_topic_refresh_schedule
 - [ ] update_user
+- [ ] update_vpc_connection
 
diff --git a/moto/__init__.py b/moto/__init__.py
index 6932f633d..36df330d0 100644
--- a/moto/__init__.py
+++ b/moto/__init__.py
@@ -196,7 +196,7 @@ mock_all = MockAll
 # logging.getLogger('boto').setLevel(logging.CRITICAL)
 
 __title__ = "moto"
-__version__ = "4.1.9.dev"
+__version__ = "4.1.10.dev"
 
 
 try:
diff --git a/moto/s3/models.py b/moto/s3/models.py
index e6365dabf..930d76256 100644
--- a/moto/s3/models.py
+++ b/moto/s3/models.py
@@ -1888,9 +1888,13 @@ class S3Backend(BaseBackend, CloudWatchMetricProvider):
         return new_key
 
     def put_object_acl(
-        self, bucket_name: str, key_name: str, acl: Optional[FakeAcl]
+        self,
+        bucket_name: str,
+        key_name: str,
+        acl: Optional[FakeAcl],
+        key_is_clean: bool = False,
     ) -> None:
-        key = self.get_object(bucket_name, key_name)
+        key = self.get_object(bucket_name, key_name, key_is_clean=key_is_clean)
         # TODO: Support the XML-based ACL format
         if key is not None:
             key.set_acl(acl)
diff --git a/moto/s3/responses.py b/moto/s3/responses.py
index 10270de8f..3b40c8701 100644
--- a/moto/s3/responses.py
+++ b/moto/s3/responses.py
@@ -1522,10 +1522,14 @@ class S3Response(BaseResponse):
             acl = self.backend.get_bucket(bucket_name).acl
         tagging = self._tagging_from_headers(request.headers)
 
+        if "versionId" in query:
+            version_id = query["versionId"][0]
+        else:
+            version_id = None
+
         if "retention" in query:
             if not lock_enabled:
                 raise LockNotEnabled
-            version_id = query.get("VersionId")
             retention = self._mode_until_from_body()
             self.backend.put_object_retention(
                 bucket_name, key_name, version_id=version_id, retention=retention
@@ -1535,7 +1539,6 @@ class S3Response(BaseResponse):
         if "legal-hold" in query:
             if not lock_enabled:
                 raise LockNotEnabled
-            version_id = query.get("VersionId")
             legal_hold_status = self._legal_hold_status_from_xml(body)
             self.backend.put_object_legal_hold(
                 bucket_name, key_name, version_id, legal_hold_status
@@ -1547,10 +1550,6 @@ class S3Response(BaseResponse):
             return 200, response_headers, ""
 
         if "tagging" in query:
-            if "versionId" in query:
-                version_id = query["versionId"][0]
-            else:
-                version_id = None
             key_to_tag = self.backend.get_object(
                 bucket_name, key_name, version_id=version_id
             )
@@ -2173,7 +2172,12 @@ class S3Response(BaseResponse):
             )
             key.set_metadata(multipart.metadata)
             self.backend.set_key_tags(key, multipart.tags)
-            self.backend.put_object_acl(bucket_name, key.name, multipart.acl)
+            self.backend.put_object_acl(
+                bucket_name=bucket_name,
+                key_name=key.name,
+                acl=multipart.acl,
+                key_is_clean=True,
+            )
 
             template = self.response_template(S3_MULTIPART_COMPLETE_RESPONSE)
             headers: Dict[str, Any] = {}


Test patch:
diff --git a/tests/test_s3/test_s3_lock.py b/tests/test_s3/test_s3_lock.py
index bb562fdfb..2a02fd512 100644
--- a/tests/test_s3/test_s3_lock.py
+++ b/tests/test_s3/test_s3_lock.py
@@ -1,9 +1,10 @@
 import time
 import boto3
 import datetime
-import botocore
+import pytest
 from moto import mock_s3
 from botocore.config import Config
+from botocore.client import ClientError
 from moto.s3.responses import DEFAULT_REGION_NAME
 import sure  # noqa # pylint: disable=unused-import
 
@@ -34,7 +35,7 @@ def test_locked_object():
     try:
         s3.delete_object(Bucket=bucket_name, Key=key_name, VersionId=version_id)
         deleted = True
-    except botocore.client.ClientError as e:
+    except ClientError as e:
         e.response["Error"]["Code"].should.equal("AccessDenied")
 
     deleted.should.equal(False)
@@ -64,7 +65,7 @@ def test_fail_locked_object():
             ObjectLockMode="COMPLIANCE",
             ObjectLockRetainUntilDate=until,
         )
-    except botocore.client.ClientError as e:
+    except ClientError as e:
         e.response["Error"]["Code"].should.equal("InvalidRequest")
         failed = True
 
@@ -99,7 +100,7 @@ def test_put_object_lock():
     try:
         s3.delete_object(Bucket=bucket_name, Key=key_name, VersionId=version_id)
         deleted = True
-    except botocore.client.ClientError as e:
+    except ClientError as e:
         e.response["Error"]["Code"].should.equal("AccessDenied")
 
     deleted.should.equal(False)
@@ -135,7 +136,7 @@ def test_put_object_legal_hold():
     try:
         s3.delete_object(Bucket=bucket_name, Key=key_name, VersionId=version_id)
         deleted = True
-    except botocore.client.ClientError as e:
+    except ClientError as e:
         e.response["Error"]["Code"].should.equal("AccessDenied")
 
     deleted.should.equal(False)
@@ -181,7 +182,7 @@ def test_put_default_lock():
     try:
         s3.delete_object(Bucket=bucket_name, Key=key_name, VersionId=version_id)
         deleted = True
-    except botocore.client.ClientError as e:
+    except ClientError as e:
         e.response["Error"]["Code"].should.equal("AccessDenied")
 
     deleted.should.equal(False)
@@ -194,3 +195,111 @@ def test_put_default_lock():
     response["ObjectLockConfiguration"]["Rule"]["DefaultRetention"][
         "Days"
     ].should.equal(days)
+
+
+@mock_s3
+def test_put_object_legal_hold_with_versions():
+    s3 = boto3.client("s3", config=Config(region_name=DEFAULT_REGION_NAME))
+
+    bucket_name = "put-legal-bucket"
+    key_name = "file.txt"
+
+    s3.create_bucket(Bucket=bucket_name, ObjectLockEnabledForBucket=True)
+
+    put_obj_1 = s3.put_object(Bucket=bucket_name, Body=b"test", Key=key_name)
+    version_id_1 = put_obj_1["VersionId"]
+    # lock the object with the version, locking the version 1
+    s3.put_object_legal_hold(
+        Bucket=bucket_name,
+        Key=key_name,
+        VersionId=version_id_1,
+        LegalHold={"Status": "ON"},
+    )
+
+    # put an object on the same key, effectively creating a version 2 of the object
+    put_obj_2 = s3.put_object(Bucket=bucket_name, Body=b"test", Key=key_name)
+    version_id_2 = put_obj_2["VersionId"]
+    # also lock the version 2 of the object
+    s3.put_object_legal_hold(
+        Bucket=bucket_name,
+        Key=key_name,
+        VersionId=version_id_2,
+        LegalHold={"Status": "ON"},
+    )
+
+    # assert that the version 1 is locked
+    head_obj_1 = s3.head_object(
+        Bucket=bucket_name, Key=key_name, VersionId=version_id_1
+    )
+    assert head_obj_1["ObjectLockLegalHoldStatus"] == "ON"
+
+    # remove the lock from the version 1 of the object
+    s3.put_object_legal_hold(
+        Bucket=bucket_name,
+        Key=key_name,
+        VersionId=version_id_1,
+        LegalHold={"Status": "OFF"},
+    )
+
+    # assert that you can now delete the version 1 of the object
+    s3.delete_object(Bucket=bucket_name, Key=key_name, VersionId=version_id_1)
+
+    with pytest.raises(ClientError) as e:
+        s3.head_object(Bucket=bucket_name, Key=key_name, VersionId=version_id_1)
+    assert e.value.response["Error"]["Code"] == "404"
+
+    # cleaning
+    s3.put_object_legal_hold(
+        Bucket=bucket_name,
+        Key=key_name,
+        VersionId=version_id_2,
+        LegalHold={"Status": "OFF"},
+    )
+    s3.delete_object(Bucket=bucket_name, Key=key_name, VersionId=version_id_2)
+    s3.delete_bucket(Bucket=bucket_name)
+
+
+@mock_s3
+def test_put_object_lock_with_versions():
+    s3 = boto3.client("s3", config=Config(region_name=DEFAULT_REGION_NAME))
+
+    bucket_name = "put-lock-bucket-test"
+    key_name = "file.txt"
+    seconds_lock = 2
+
+    s3.create_bucket(Bucket=bucket_name, ObjectLockEnabledForBucket=True)
+
+    put_obj_1 = s3.put_object(Bucket=bucket_name, Body=b"test", Key=key_name)
+    version_id_1 = put_obj_1["VersionId"]
+    put_obj_2 = s3.put_object(Bucket=bucket_name, Body=b"test", Key=key_name)
+    version_id_2 = put_obj_2["VersionId"]
+
+    until = datetime.datetime.utcnow() + datetime.timedelta(seconds=seconds_lock)
+
+    s3.put_object_retention(
+        Bucket=bucket_name,
+        Key=key_name,
+        VersionId=version_id_1,
+        Retention={"Mode": "COMPLIANCE", "RetainUntilDate": until},
+    )
+
+    # assert that you can delete the locked version 1 of the object
+    deleted = False
+    try:
+        s3.delete_object(Bucket=bucket_name, Key=key_name, VersionId=version_id_1)
+        deleted = True
+    except ClientError as e:
+        e.response["Error"]["Code"].should.equal("AccessDenied")
+
+    deleted.should.equal(False)
+
+    # assert that you can delete the version 2 of the object, not concerned by the lock
+    s3.delete_object(Bucket=bucket_name, Key=key_name, VersionId=version_id_2)
+    with pytest.raises(ClientError) as e:
+        s3.head_object(Bucket=bucket_name, Key=key_name, VersionId=version_id_2)
+    assert e.value.response["Error"]["Code"] == "404"
+
+    # cleaning
+    time.sleep(seconds_lock)
+    s3.delete_object(Bucket=bucket_name, Key=key_name, VersionId=version_id_1)
+    s3.delete_bucket(Bucket=bucket_name)
diff --git a/tests/test_s3/test_s3_multipart.py b/tests/test_s3/test_s3_multipart.py
index b152c88ba..fb511e76a 100644
--- a/tests/test_s3/test_s3_multipart.py
+++ b/tests/test_s3/test_s3_multipart.py
@@ -107,34 +107,35 @@ def test_multipart_upload_too_small():
     )
 
 
+@pytest.mark.parametrize("key", ["the-key", "the%20key"])
 @mock_s3
 @reduced_min_part_size
-def test_multipart_upload():
+def test_multipart_upload(key: str):
     s3 = boto3.resource("s3", region_name=DEFAULT_REGION_NAME)
     client = boto3.client("s3", region_name=DEFAULT_REGION_NAME)
     s3.create_bucket(Bucket="foobar")
 
     part1 = b"0" * REDUCED_PART_SIZE
     part2 = b"1"
-    mp = client.create_multipart_upload(Bucket="foobar", Key="the-key")
+    mp = client.create_multipart_upload(Bucket="foobar", Key=key)
     up1 = client.upload_part(
         Body=BytesIO(part1),
         PartNumber=1,
         Bucket="foobar",
-        Key="the-key",
+        Key=key,
         UploadId=mp["UploadId"],
     )
     up2 = client.upload_part(
         Body=BytesIO(part2),
         PartNumber=2,
         Bucket="foobar",
-        Key="the-key",
+        Key=key,
         UploadId=mp["UploadId"],
     )
 
     client.complete_multipart_upload(
         Bucket="foobar",
-        Key="the-key",
+        Key=key,
         MultipartUpload={
             "Parts": [
                 {"ETag": up1["ETag"], "PartNumber": 1},
@@ -144,7 +145,7 @@ def test_multipart_upload():
         UploadId=mp["UploadId"],
     )
     # we should get both parts as the key contents
-    response = client.get_object(Bucket="foobar", Key="the-key")
+    response = client.get_object(Bucket="foobar", Key=key)
     response["Body"].read().should.equal(part1 + part2)