[azure] Allow creation of signed URLs for upload

[CorentinGoodays]

Allow creation of signed URLs for upload in AzureStorage (See #1413)

[jschneier]

Thanks for the PR, looks okay. Two comments:

1. Is there a library version minimum for the from_string api?
2. Can you please add or update a test case that covers this change

[CorentinGoodays]

Thank you for reviewing my PR and for your feedback.

According to the release notes, the BlobSasPermissions.from_string method was introduced in version 12.0.0b4:

New features:
...
AccountSasPermissions, BlobSasPermissions, ContainerSasPermissions now have method from_string which takes parameters as a string.

This version was released a few weeks before version 12.0.0, so from my understanding, there is no need to change the requirements of django-storages.

Regarding the test case, I believe the modification I'm proposing is already covered by this existing test:
https://github.com/jschneier/django-storages/blob/master/tests/test_azure.py#L157

This test currently only checks the functionality with read permissions, but there is no functional difference between a signed URL with read, write, or any other permissions. The only difference resides in the query string of the resulting URL. There's is a sp parameter which contains the permission string (like r, or w etc), and the sig parameter which contains the actual signature. Since the query string is not included in the mock implementation, I am not certain what additional aspects could be tested.

[jschneier]

Sounds good on point (1).

For point (2), we should update the test or tweak it such that we confirm this is covered both for the default case and at least for the write case. I want the test to break if someone accidentally reverts parts of your patch.

[CorentinGoodays]

I updated the test to cover both the default case and the write case.

Initially, I was hoping I could do this:

            generate_blob_sas_mocked.assert_called_once_with(
                self.account_name,
                self.container_name,
                "some blob",
                account_key=self.account_key,
                user_delegation_key=None,
-               permission=mock.ANY,
+               permission=BlobSasPermission(read=True),
                expiry=fixed_time + timedelta(seconds=100),
            )
...
            generate_blob_sas_mocked.assert_called_with(
                self.account_name,
                self.container_name,
                "some blob",
                account_key=self.account_key,
                user_delegation_key=None,
-               permission=mock.ANY,
+               permission=BlobSasPermission(write=True),
                expiry=fixed_time + timedelta(seconds=100),
            )

But sadly Microsoft has not implemented the __eq__ method for the BlobSasPermissions class, so the check is not working because it's comparing two different instances :/

I resorted to this instead:

            called_args, called_kwargs = generate_blob_sas_mocked.call_args
            self.assertEqual(str(called_kwargs["permission"]), "r")
...
            called_args, called_kwargs = generate_blob_sas_mocked.call_args
            self.assertEqual(str(called_kwargs["permission"]), "w")

Are you OK with this approach?

Would you prefer to have a separate test case for the write permission?

[jschneier]

Ah that’s too bad. Is there a property on the class we can check?

[CorentinGoodays]

There's a _str field whose value is based on the boolean values (read, write, etc) passed to the constructor. That's actually what I'm checking by converting the permission to a string here:

self.assertEqual(str(called_kwargs["permission"]), "w")

An alternative could be to dynamically add an __eq__ method to the BlobSasPermissions class, like this:

# Dynamically add __eq__ method to BlobSasPermissions
def blob_sas_permissions_eq(self, other):
    if not isinstance(other, BlobSasPermissions):
        return NotImplemented
    return self._str == other._str

BlobSasPermissions.__eq__ = blob_sas_permissions_eq

Then this code would work as expected:

            generate_blob_sas_mocked.assert_called_with(
                self.account_name,
                self.container_name,
                "some blob",
                account_key=self.account_key,
                user_delegation_key=None,
                permission=BlobSasPermission(write=True),
                expiry=fixed_time + timedelta(seconds=100),
            )

[jschneier]

Thanks for updating, looks great!

[jschneier]

Can you just fix the lint error? Good to merge after that.

[jschneier]

Also if you wouldn't mind adding to the CHANGELOG file, that'd be appreciated!

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 6cdc419..0b0085f 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -15,11 +15,13 @@ Azure
 -----
 
 - Fix ``collectstatic --clear`` (`#1403`_)
+- Add ``mode`` kwarg to ``.url()`` to support creation of signed URLs for upload (`#1414`_)
 
 .. _#1399: https://github.com/jschneier/django-storages/pull/1399
 .. _#1381: https://github.com/jschneier/django-storages/pull/1381
 .. _#1402: https://github.com/jschneier/django-storages/pull/1402
 .. _#1403: https://github.com/jschneier/django-storages/pull/1403
+.. _#1414: https://github.com/jschneier/django-storages/pull/1414

[CorentinGoodays]

All right, I updated the changelog and addressed the lint error (though I couldn't reproduce it using tox, only when running Black 23.3.0 manually).

I'm not clear whether you're satisfied with this way of doing:

called_args, called_kwargs = generate_blob_sas_mocked.call_args
self.assertEqual(str(called_kwargs["permission"]), "w")

or if you would prefer adding the __eq__ method to BlobSasPermissions like this?

# Dynamically add __eq__ method to BlobSasPermissions
def blob_sas_permissions_eq(self, other):
    if not isinstance(other, BlobSasPermissions):
        return NotImplemented
    return self._str == other._str

BlobSasPermissions.__eq__ = blob_sas_permissions_eq

...
            generate_blob_sas_mocked.assert_called_with(
                self.account_name,
                self.container_name,
                "some blob",
                account_key=self.account_key,
                user_delegation_key=None,
                permission=BlobSasPermission(write=True),
                expiry=fixed_time + timedelta(seconds=100),
            )

I think the second approach is nice as it makes the test case more readable. Additionally, if Microsoft ever implements the __eq__ method, we can easily remove our custom implementation.

Please let me know which approach you prefer.