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

Remove public permissions from image store blob in favor of SAS #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rozele
Copy link
Contributor

@rozele rozele commented Sep 12, 2018

We ran into an issue where we didn't initially notice that the OCR images were being stored in a public blob when testing on our own documents. This change would prevent others from making the same mistake.

We ran into an issue where we didn't initially notice that the OCR images were being stored in a public blob when testing on our own documents. This change would prevent others from making the same mistake.
@Careyjmac
Copy link
Collaborator

I am going to test this soon to confirm it works still, but I am likely going to end up merging it if it does as it seems like a very nice change, let me know if anyone has any objections.

@@ -18,6 +18,9 @@ namespace Microsoft.CognitiveSearch.WebApiSkills
{
public static class JfkWebApiSkills
{
// Configure duration of SAS token for image store blobs here
private static readonly TimeSpan s_imageBlobSASDuration = TimeSpan.FromDays(365);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you possibly change this to be effectively forever (i.e. some day in the year 9999) so that in case anyone uses this demo long term they don't become frustrated if it stops working and the only way to fix it is to reindex?

@Careyjmac
Copy link
Collaborator

You'd also want to update the README to remove the note about publicy readable storage containers.

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.

2 participants