Deleting a marker must not AccessDenied when retention set

[irq0]

S3 test test_object_lock_delete_object_with_retention_and_marker fails while deleting a previously created delete marker

    @pytest.mark.fails_on_dbstore
    def test_object_lock_delete_object_with_retention_and_marker():
        bucket_name = get_new_bucket_name()
        client = get_client()
        client.create_bucket(Bucket=bucket_name, ObjectLockEnabledForBucket=True)
        key = 'file1'

        response = client.put_object(Bucket=bucket_name, Body='abc', Key=key)
        retention = {'Mode':'GOVERNANCE', 'RetainUntilDate':datetime.datetime(2030,1,1,tzinfo=pytz.UTC)}
        client.put_object_retention(Bucket=bucket_name, Key=key, Retention=retention)
        del_response = client.delete_object(Bucket=bucket_name, Key=key)
        e = assert_raises(ClientError, client.delete_object, Bucket=bucket_name, Key=key, VersionId=response['VersionId'])
        status, error_code = _get_status_and_error_code(e.response)
        assert status == 403
        assert error_code == 'AccessDenied'

>       client.delete_object(Bucket=bucket_name, Key=key, VersionId=del_response['VersionId'])

del_response is the delete marker created by delete_object without a version id before.

Deleting the delete marker should work, but raises a AccessDenied

[giubacc]

IMO the test_object_lock_delete_object_with_retention_and_marker test is incorrect.

This block:

    del_response = client.delete_object(Bucket=bucket_name, Key=key)
    e = assert_raises(ClientError, client.delete_object, Bucket=bucket_name, Key=key, VersionId=response['VersionId'])

What is the purpose of the first deletion? del_response is not even checked.

Then this block makes no sense:

    client.delete_object(Bucket=bucket_name, Key=key, VersionId=del_response['VersionId'])
    e = assert_raises(ClientError, client.delete_object, Bucket=bucket_name, Key=key, VersionId=response['VersionId'])
    status, error_code = _get_status_and_error_code(e.response)
    assert status == 403
    assert error_code == 'AccessDenied'

I think the test wanted to assert the raise of the exception when deleting the specific VersionID, but if so, why calling the exact same call just before the protected assert_raises function?

Indeed the test is (correctly!) raising an unchecked exception in the expected place:

>       client.delete_object(Bucket=bucket_name, Key=key, VersionId=del_response['VersionId'])

s3tests_boto3/functional/test_s3.py:11871: 

E           botocore.exceptions.ClientError: An error occurred (AccessDenied) when calling the DeleteObject operation: forbidden by object lock

To me, this test is invalid.

[giubacc]

@jecluis need another pair of eyes to eval what to do here, IMO this should be closed.

[jecluis]

IMO the test_object_lock_delete_object_with_retention_and_marker test is incorrect.

I haven't read the test, but from your description I'm not so sure that's the case.

AFAIU, the test is supposed to be deleting a delete marker. That means deleting an object that exists, which introduces a delete marker, and then deleting the latest version id of said object, hence the deleting the delete marker.

This block:

    del_response = client.delete_object(Bucket=bucket_name, Key=key)
    e = assert_raises(ClientError, client.delete_object, Bucket=bucket_name, Key=key, VersionId=response['VersionId'])

What is the purpose of the first deletion? del_response is not even checked.

It may not be checked, but is used later on. I suspect this is deleting the object, hence creating a delete marker.

Then this block makes no sense:

    client.delete_object(Bucket=bucket_name, Key=key, VersionId=del_response['VersionId'])
    e = assert_raises(ClientError, client.delete_object, Bucket=bucket_name, Key=key, VersionId=response['VersionId'])
    status, error_code = _get_status_and_error_code(e.response)
    assert status == 403
    assert error_code == 'AccessDenied'

It looks to me that it is attempting to delete the delete marker. See how it is deleting the object with a specified version id, which is the same as returned before by the first object deletion. I presume this is the version id of the delete marker that was introduced.

Only after that, the test attempts to delete the actual initial object version, and only then expects an Access Denied.

To me it seems like what the test expects is to be able to delete the delete marker even under object locking, but expects not being able to delete the actual object version (not the delete marker).

I think the only way to assess what is the right thing here is to resort to the documentation and understand whether a delete marker under object locking can be deleted. I suspect it should, although it might require special privileges (like admin, which we should have because we are running these tests with default admin credentials).

I think the test wanted to assert the raise of the exception when deleting the specific VersionID, but if so, why calling the exact same call just before the protected assert_raises function?

Indeed the test is (correctly!) raising an unchecked exception in the expected place:

>       client.delete_object(Bucket=bucket_name, Key=key, VersionId=del_response['VersionId'])

s3tests_boto3/functional/test_s3.py:11871: 

E           botocore.exceptions.ClientError: An error occurred (AccessDenied) when calling the DeleteObject operation: forbidden by object lock

To me, this test is invalid.

As I said above, I don't think that's what the test is trying to do and validate. The operation failing in this case is expected to succeed (i.e., deleting the delete marker), and only the one that comes after (i.e., deleting the object version) is expected to return Access Denied.

[giubacc]

ah yes, I could have completely misunderstood the test; will recheck this, thanks!

[giubacc]

@jecluis the test is correct, we have a bug in the rgw/sfs that makes not properly recognize a deletemarker when handling an object deletion.