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

Fix SAS token generation for directory-scoped access #1277

Merged

Conversation

dmweis
Copy link
Contributor

@dmweis dmweis commented May 11, 2023

Directory scoped SAS tokens require signedDirectoryDepth field to work. This adds it as an optional element.

Docs say "The value of the sdd field must be a non-negative integer." so I went with a u32 assuming no one would need a higher directory depth than that. Is that a sensible assumption?

I also added some tests but they aren't particularly inspired so I am happy to change/remove them

Links:
About directory scoped access: https://learn.microsoft.com/en-us/rest/api/storageservices/create-service-sas#service-sas-support-for-directory-scoped-access
Directory depth field: https://learn.microsoft.com/en-us/rest/api/storageservices/create-service-sas#specify-the-directory-depth

Copy link
Contributor

@demoray demoray left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! This looks positive.

My only requested change is to use usize rather than u32, in most of the counting results (think Iterator::count or String::len return usize.

sdk/storage/src/shared_access_signature/service_sas.rs Outdated Show resolved Hide resolved
sdk/storage/src/shared_access_signature/service_sas.rs Outdated Show resolved Hide resolved
sdk/storage/src/shared_access_signature/service_sas.rs Outdated Show resolved Hide resolved
Comment on lines 219 to 221
assert!(signed_token.contains("sr=b"));
// this assert will be a bad idea if we ever add a new field that ends with "sdd"
assert!(!signed_token.contains("sdd="));
Copy link
Contributor

Choose a reason for hiding this comment

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

As this example uses a constant expire date of 1970-01-08, the signed token result should always be the same. What are your thoughts for just having a const for the expected result?

Otherwise, to address your comment, this could also naively split on &, as the SasToken in the example should not result in a & except as a delim.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

I added both better splitting and an assert against a hardcoded string. This way you get more clear failures if things break but have a wider test too.

If people add more tests later or other things break the hardcoded tests they can remove one or the other

@dmweis dmweis requested a review from demoray May 12, 2023 01:32
@dmweis
Copy link
Contributor Author

dmweis commented May 14, 2023

Thanks for the approve @demoray !
I assume that since it's approved it's just going to be merged later? Or is there something else I need to do?

@demoray
Copy link
Contributor

demoray commented May 15, 2023

Thanks for the approve @demoray ! I assume that since it's approved it's just going to be merged later? Or is there something else I need to do?

You didn't need to do anything else. I needed CI to validate the changes, which has been done now.

@demoray demoray merged commit dcf7738 into Azure:main May 15, 2023
8 checks passed
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.

None yet

2 participants