Primitives/SharedAccessSignatureTokenProvider.cs doesn't handle timeouts properly

[DavidBerg-MSFT]

Actual Behavior

1. GetTokenAsync takes a timeout parameter. The parameter is ignored and the value passed in the constructor (if any) is used instead.
2. Looking at the code for BuildSignature it will ignore the passed targetUri if the SharedAccessSignature field isn’t blank and just return the saved SharedAccessSignature, yet if it uses the other codepath, it uses SharedAccessSignature, indicating that there are times when this routine expects a non-blank SharedAccessSignature, which it will never see because of the preceding logic.

Expected Behavior

1. Timeout parameter should be used if specified.
2. Not sure of the intent behind ignoring targetURI in favor of cached signature, but behavior just seems wrong. Please ensure the code is doing what it's supposed to do (add comments to explain why this is the correct behavior if it is, as it's not obvious from the context).

Versions

• Problem is in current source (as of 6/26/2018)

[nemakam]

@DavidBerg-MSFT Could I know if you are running into any issue?

Looking at the code for BuildSignature it will ignore the passed targetUri if the SharedAccessSignature field isn’t blank and just return the saved SharedAccessSignature, yet if it uses the other codepath, it uses SharedAccessSignature, indicating that there are times when this routine expects a non-blank SharedAccessSignature, which it will never see because of the preceding logic.

The thing is you can construct this TokenProvider either by giving combination of SASKeyName and SASKey, or by generating your own SAS token and passing only the SASToken. This is why you see the code where if sharedAccessSignature is provided by user, we directly use it, else if we end up constructing it ourselves and then use that. If the token is provided by the user, then the token is already expected to have the target URL and the tokenTTL inbuilt.

GetTokenAsync takes a timeout parameter. The parameter is ignored and the value passed in the constructor (if any) is used instead.

There are two values. One is the operationTimeout which dictates what is the time limit for the GetToken operation if it is going beyond the machine boundary, and one is the tokenTTL which dictates how long the token is even valid for. So tokenTTL is taken through the constructor and is used while building a SASToken ourselves using SASKeyName and SASKey. operationTimeout is not required for SharedAccessTokenProvider as GetTokenAsync() is not really an asynchronous call and there is no need for timeout.

operationTimeout is currently not required for any of the authentication token providers that we have exposed in the library. There are few other token providers which needed it which we have decided to not expose in this library as of now.

[DavidBerg-MSFT]

On timeout: That these are two different values is NOT at all obvious either from the code or the documentation. The documentation makes no mention of there being a default 1 hour time to live, so it's a reasonable assumption that if you don't specify time to live in the constructor that it needs to be specified in the method to get the token.

Further, the code uses Timeout and TimeToLive interchangeably (The default for tokenTimeToLive is called DefaultTokenTimeout). So when the parameter to GetTokenAsync is simply called timeout, it's natural to assume that this is just another case of using the terms interchangeably. The documentation here is also hard to parse ("The time span that specifies the timeout value for the message that gets the security token"). And it's further confused because the timeout parameter doesn't actually do anything.

May I suggest:

1. Update the documentation (inline) to clarify that the default timeout is 1 hour.
2. Rename DefaultTokenTimeout to DefaultTokenTimeToLive.
3. Update the documentation (inline) for GetTokenAsync to clarify the use of the timeout parameter. For example, The timeout value for how long it takes to get the security token (not the token time to live). This parameter is here for compatibility, but is not currently used.

On BuildSignature, I see I confused sharedAccessSignatureToken with SharedAccessSignatureToken.SharedAccessSignatureToken. Sorry.