Updated SasBuilder to correctly order raw string permissions #13433
Conversation
ghost
added
the
seanmcc-msft
requested review from
amishra-dev,
amnguye,
kasobol-msft and
tg-msft
as code owners
|
@seanmcc-msft And a minor suggestion: Anyway, this might not be a common customer scenario, just a minor suggestion. |
| // Check that each permission is a real SAS permission. | ||
| if (!validPermissionsSet.Contains(permission)) | ||
| { | ||
| throw new ArgumentException($"{permission} is not a valid SAS permission"); |
There was a problem hiding this comment.
This is a nontrivial change. We've always allowed customers to do whatever they want with the raw permissions, but now you're forcing them to be a set of known values. Why not let the service reject unknown values? Why aren't we putting this behind something like an optional bool normalize = false param? Are we doing this consistently across languages?
There was a problem hiding this comment.
The service doesn't return a useful error message if you include an invalid permission - just "Unauthorized". The user has no way of knowing that they did wrong. Also, how do I re-order the permissions if the user provides permissions that don't exist?
Response if you include an invalid SAS permission:
HTTP/1.1 403 Server failed to authenticate the request. Make sure the value of Authorization header is formed correctly including the signature.
Content-Length: 408
Content-Type: application/xml
Server: Microsoft-HTTPAPI/2.0
x-ms-request-id: 55282e2e-d01e-0002-0901-5a08f0000000
x-ms-error-code: AuthenticationFailed
Date: Tue, 14 Jul 2020 17:09:29 GMT
<?xml version="1.0" encoding="utf-8"?><Error><Code>AuthenticationFailed</Code><Message>Server failed to authenticate the request. Make sure the value of Authorization header is formed correctly including the signature.
RequestId:55282e2e-d01e-0002-0901-5a08f0000000
Time:2020-07-14T17:09:30.2593026Z</Message><AuthenticationErrorDetail>Signature fields not well formed.</AuthenticationErrorDetail></Error>
@gapra-msft, @xiafu-msft, do we allow users to pass invalid raw SAS permissions in in Java and Python?
There was a problem hiding this comment.
@tg-msft, my bad, I accidentally edited your comment and then reverted it back.
There was a problem hiding this comment.
Can we get a bug filed against the service to return a better error message for an unknown permission? It should be fine to explain why a SAS permission is invalid per the service API rather than relative to a resource.
And yes, you can't order permissions you don't know about which is why I'm suggesting moving this validation + reordering logic behind an optional normalize flag. We normally don't validate anything on clients so we should make it optional if we're providing this as a convenience. This would also enable @blueww's scenario from above.
+@XiaoningLiu for JS as well.
There was a problem hiding this comment.
Fixed in next iteration.
There was a problem hiding this comment.
@tg-msft Java does not allow to pass raw permissions verbatim - they're always validated and ordered before generating sas. Python seems to accept union of *SasPermission (which validates and orders) and raw string, so user can pass raw string without any validation there.
There was a problem hiding this comment.
So it sounds like Java is the outlier with no option to skip validation and we don't know about JS yet. Is it worth filing an issue for Java? I could see waiting to add this until someone asks because it wouldn't be a breaking change.
There was a problem hiding this comment.
I feel that if I were implementing that from scratch I'd chose Java's approach as it reduces # of ways customer can get it wrong (it takes care of the ordering). So I'd rather change .NET and Python in next major rev. I think we should hold with changing Java until someone explicitly asks for that (not sure why would they want to do that).
| [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)] | ||
| public void SetPermissions(string rawPermissions) { } | ||
| public void SetPermissions(string rawPermissions, bool normalize = false) { } |
There was a problem hiding this comment.
Nit - we mostly use the [EditorBrowsable(Never)] for hiding overloads with optional params. We'd typically solve this case with an extra overload.
I updated
BlobSasBuilder,ShareSasBuilder,DataLakeSasBuilder, andQueueSasBuilderto validate and re-order raw string permissions when theSetPermissions()method is called with a string.