Add AAD support #746
Add AAD support #746
Conversation
This comment has been minimized.
This comment has been minimized.
|
I think we are leaning away from including those values in the Connection String. |
Library/AuthHandler.ts
Outdated
| */ | ||
| public async addAuthorizationHeader(requestOptions: http.RequestOptions | https.RequestOptions): Promise<void> { | ||
| const token = await this._getToken({}); | ||
| requestOptions.headers[azureCore.Constants.HeaderConstants.AUTHORIZATION] = `Bearer ${token}`; |
There was a problem hiding this comment.
Does the header value need to be encoded?
There was a problem hiding this comment.
This is the way other SDKs are adding the header, code is not ready on Breeze side so this may change in the future
Library/AuthHandler.ts
Outdated
| // after that point, we retry the refresh of the token only if the token refresher is ready. | ||
| let token = this._tokenCache.getCachedToken(); | ||
| if (!token && this._tokenRefresher.isReady()) { | ||
| token = await this._tokenRefresher.refresh(options); |
There was a problem hiding this comment.
What happens if this fails, does the setCachedToken() handle null / undefined or invalid token value?
There was a problem hiding this comment.
The token could be null or undefined, will add the check before adding the header
Library/AuthHandler.ts
Outdated
| // after that point, we retry the refresh of the token only if the token refresher is ready. | ||
| let token = this._tokenCache.getCachedToken(); | ||
| if (!token && this._tokenRefresher.isReady()) { | ||
| token = await this._tokenRefresher.refresh(options); |
Library/Sender.ts
Outdated
| @@ -146,6 +165,8 @@ class Sender { | |||
| } | |||
| // store to disk in case of burst throttling | |||
| } else if ( | |||
| res.statusCode === 401 || // Unauthorized | |||
| res.statusCode === 403 || // Forbidden | |||
There was a problem hiding this comment.
So there is no retry when we hit any AAD auth error? Is that what other langs are doing? I thought we were going to retry!
There was a problem hiding this comment.
This specific code is the retry logic, envelopes would be put into disk, are you expecting something else?
There was a problem hiding this comment.
Yes, just looked at Python PR. Leighton is retrying if we hit 401. @lzchen can you comment?
Removing CS options
Adding config in readme
| const applicationInsightsResource = "https://monitor.azure.com"; | ||
|
|
||
|
|
||
| class AuthorizationHandler { |
Library/Sender.ts
Outdated
| // Add bearer token | ||
| await authHandler.addAuthorizationHeader(options); | ||
| } | ||
| catch (authError) { |
There was a problem hiding this comment.
I believe we want to do different things (retry, drop, etc.) based off the type of exception that is thrown when getting the token. Python throws these types of exceptions (I believe Java is similar). For CredentialUnavailableError we probably do not want to retry. For ClientAuthenticationError we should investigate which cases make sense to retry.
There was a problem hiding this comment.
If we do not retry telemetry will be lost on the spot in that case, even if customer configuration is wrong we should try to store the telemetry in disk, what about AAD outages, maybe CredentialUnavailableError is triggered in that case?
|
I think an example file of passing in a TokenCredential would be good. |
Updating token refreshing to use policy instead
No description provided.