Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Azure parameters as kwargs for storage.url #1071

Merged
merged 1 commit into from
Oct 16, 2021

Conversation

fao89
Copy link
Contributor

@fao89 fao89 commented Oct 12, 2021

It enables using kwargs, such as content_type, content_disposition, ..., when generating a shared access signature for a blob
https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/storage/azure-storage-blob/azure/storage/blob/_shared_access_signature.py#L482-L574

@ipanova
Copy link

ipanova commented Oct 12, 2021

@jschneier any chance this can be accepted and released any time soon?Thank you.

name = self._get_valid_path(name)
params = parameters.copy() if parameters else {}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to copy, if it is used read only.

Suggested change
params = parameters.copy() if parameters else {}
params = parameters or {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there the dict is augmented before using it. Here it is not.

@fao89 fao89 force-pushed the azure_params_url branch 2 times, most recently from 644f6ad to 493adb7 Compare October 12, 2021 14:16
@fao89 fao89 requested a review from jschneier October 12, 2021 14:17
@jschneier
Copy link
Owner

How come we only want to run this if expire?

@fao89
Copy link
Contributor Author

fao89 commented Oct 12, 2021

How come we only want to run this if expire?

Good question!
I think so, and renaming parameters to expire_params would make more sense. It would make it easier to document/understand. What do you think?

@ipanova
Copy link

ipanova commented Oct 12, 2021

How come we only want to run this if expire?

reading trough the docs expire is set to None by default which means it never expires. Any reason why it is not possible create token_sas when expire is not set?

@ipanova
Copy link

ipanova commented Oct 12, 2021

I see, it boils down to public and private access https://django-storages.readthedocs.io/en/latest/backends/azure.html#private-vs-public-access; tldr in private access there is always an expiration set, and in public access i guess one does not need any credentials.

@jschneier
Copy link
Owner

I think the name parameters is fine. In the initial message things like content_disposition are mentioned for which public/private isn't a meaningful distinction; that is the source of my questions.

@ipanova
Copy link

ipanova commented Oct 14, 2021

I think the name parameters is fine. In the initial message things like content_disposition are mentioned for which public/private isn't a meaningful distinction; that is the source of my questions.

Take this with a grain of salt, I am not expert in this area at all, but here is my understanding - expiry is used only with private containers as mentioned in docs, where as a result a SAS token is created and is being used as credentials when calling into from_blob_url

return BlobClient.from_blob_url(container_blob_url, credential=credential).url

https://docs.microsoft.com/en-us/python/api/azure-storage-blob/azure.storage.blob.blobclient?view=azure-python#examples

https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/storage/azure-storage-blob/azure/storage/blob/_shared_access_signature.py#L558

To hopefully answer your question - apparently there is a limitation and it is not possible to send any headers in a public container since they are passed in the credentials which are used in the SAS token which is used only in private containers.

@jschneier
Copy link
Owner

That is quite the limitation! Thanks for the info. In that case we should add a disclaimer to the docs, and include the urldocumentation as well.

@fao89
Copy link
Contributor Author

fao89 commented Oct 14, 2021

That is quite the limitation! Thanks for the info. In that case we should add a disclaimer to the docs, and include the urldocumentation as well.

I tried to add some docs, let me know how can I improve it!

Here are the links that I used:

I didn't added the second one, because it is too big


The difference between public and private URLs is that private includes the SAS token.
With private URLs you can override certain properties stored for the blob by specifying
query parameters as part of the shared access signature.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add These properties include the cache-control, content-type, content-encoding, content-language, and content-disposition properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I shared the link of the official docs below, so people could see this note:
image

but you are right, I'll add the properties here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants