BingAds / BingAds-dotNet-SDK

Other
26 stars 41 forks source link

proactively fetch new access token, if expired #83

Closed AAATechGuy closed 4 years ago

AAATechGuy commented 4 years ago

I strongly feel these:

  1. We should not maintain logic to get access token from refresh_token. BingAds should just expect AccessToken, and let MSAL.NET (maintained by AAD team) handle token acquiring logic. In addition to OAuth 2.0 code grant flow, there are other ways to fetch access token. At the moment, I feel we are restricting our SDK users to a particular type of method, rather than leveraging full potential of AAD infra.
  2. Even if we don’t use MSAL.NET I feel the flow could be optimized - instead of waiting for calls to fail with 109, we could proactively refresh the token before expiry. This should be a simple fix - in below code, value for needToRefreshToken requires to be determined proactively.
                try
                {
                    var response = await method(client, request).ConfigureAwait(false);

                    return response;
                }
                catch (FaultException ex)
                {
                    // If needToRefreshToken == true, it means one token refresh has alreay been attempted, and we shouldn't do it again
                    if (needToRefreshToken == false && RefreshOAuthTokensAutomatically && IsExpiredTokenException(ex))
                    {
                        needToRefreshToken = true;
                    }
                    else
                    {
                        ((IClientChannel)client).Abort();

                        throw;
                    }
                }
AAATechGuy commented 4 years ago

This is fixed via https://github.com/BingAds/BingAds-dotNet-SDK/blob/da6b1b23bca8b3c17fae34b59cbe90fcfc9d7735/BingAdsApiSDK/OAuthTokens.cs#L88