Skip to content

Commit ddc22a7

Browse files
paulmedynskipaulmedynski
committed
- Fixed auth provider error handling to mimic the old code.
1 parent f80f3e7 commit ddc22a7

File tree

3 files changed

+64
-51
lines changed

3 files changed

+64
-51
lines changed

‎src/Microsoft.Data.SqlClient.Extensions/Azure/src/ActiveDirectoryAuthenticationProvider.cs‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,6 @@ public sealed class ActiveDirectoryAuthenticationProvider : SqlAuthenticationPro
4141
// https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/retry-after#simple-retry-for-errors-with-http-error-codes-500-600
4242
private const int MsalRetryStatusCode = 429;
4343

44-
// For unknown MSAL error codes, we will suggest that the action be retried
45-
// after this period, in milliseconds.
46-
private const int UnknownErrorRetryPeriod = 100;
47-
4844
/// <include file='../doc/ActiveDirectoryAuthenticationProvider.xml' path='docs/members[@name="ActiveDirectoryAuthenticationProvider"]/ctor/*'/>
4945
public ActiveDirectoryAuthenticationProvider()
5046
: this(DefaultDeviceFlowCallback)
@@ -390,14 +386,15 @@ previousPw is byte[] previousPwBytes &&
390386
}
391387

392388
// Check for an unknown error, which we will treat as implicitly
393-
// retryable on a doubling-backoff period.
389+
// retryable, but without a suggested period.
394390
if (ex.ErrorCode == MsalError.UnknownError)
395391
{
396392
throw new Extensions.Azure.AuthenticationException(
397393
parameters.AuthenticationMethod,
398394
ex.ErrorCode,
399395
true,
400-
UnknownErrorRetryPeriod,
396+
// Don't suggest a retry period.
397+
0,
401398
ex.Message,
402399
ex);
403400
}

‎src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs‎

Lines changed: 32 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -2489,8 +2489,6 @@ internal bool TryGetFedAuthTokenLocked(SqlFedAuthInfo fedAuthInfo, DbConnectionP
24892489
/// <returns>SqlFedAuthToken</returns>
24902490
internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
24912491
{
2492-
Debug.Assert(fedAuthInfo != null, "fedAuthInfo should not be null.");
2493-
24942492
// Number of milliseconds to sleep for the initial back off, if a
24952493
// retry period is not specified by the provider.
24962494
const int defaultRetryPeriod = 100;
@@ -2509,7 +2507,7 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
25092507

25102508
// We will perform retries if the provider indicates an error that
25112509
// is retryable.
2512-
for (int retryAttempt = 0; retryAttempt < maxRetryAttempts; ++retryAttempt)
2510+
for (int retryAttempt = 0; retryAttempt <= maxRetryAttempts; ++retryAttempt)
25132511
{
25142512
try
25152513
{
@@ -2566,9 +2564,9 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
25662564
_activeDirectoryAuthTimeoutRetryHelper.CachedToken = _fedAuthToken;
25672565
}
25682566
break;
2569-
#pragma warning disable 0618 // Type or member is obsolete
2567+
#pragma warning disable 0618 // Type or member is obsolete
25702568
case SqlAuthenticationMethod.ActiveDirectoryPassword:
2571-
#pragma warning restore 0618 // Type or member is obsolete
2569+
#pragma warning restore 0618 // Type or member is obsolete
25722570
case SqlAuthenticationMethod.ActiveDirectoryServicePrincipal:
25732571
if (_activeDirectoryAuthTimeoutRetryHelper.State == ActiveDirectoryAuthenticationTimeoutRetryState.Retrying)
25742572
{
@@ -2633,17 +2631,15 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
26332631
}
26342632
catch (SqlAuthenticationProviderException ex)
26352633
{
2636-
// We use a doubling backoff if the provider didn't provide
2637-
// a retry period.
2638-
int retryPeriod = defaultRetryPeriod * (2 ^ retryAttempt);
2639-
2640-
// Should we retry, and were we given a useful retry period?
2641-
if (ex.ShouldRetry && ex.RetryPeriod > 0)
2634+
// Is the error fatal or retryable?
2635+
if (!ex.ShouldRetry)
26422636
{
2643-
// Yes, so use it.
2644-
retryPeriod = ex.RetryPeriod;
2637+
// It's fatal, so translate into a SqlException.
2638+
throw ADP.CreateSqlException(ex, ConnectionOptions, this, username);
26452639
}
26462640

2641+
// We should retry.
2642+
26472643
// Could we retry if we wanted to?
26482644
if (_timeout.IsExpired || _timeout.MillisecondsRemaining <= 0)
26492645
{
@@ -2652,26 +2648,37 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
26522648
throw SQL.ActiveDirectoryTokenRetrievingTimeout(Enum.GetName(typeof(SqlAuthenticationMethod), ConnectionOptions.Authentication), ex.FailureCode, ex);
26532649
}
26542650

2655-
// We will retry, so log.
2651+
// We use a doubling backoff if the provider didn't provide
2652+
// a retry period.
2653+
int retryPeriod =
2654+
ex.RetryPeriod > 0
2655+
? ex.RetryPeriod
2656+
: defaultRetryPeriod * (2 ^ retryAttempt);
2657+
26562658
SqlClientEventSource.Log.TryAdvancedTraceEvent("<sc.SqlInternalConnectionTds.GetFedAuthToken|ADV> {0}, Retry Attempt: {1}, sleeping {2}[Milliseconds]", ObjectID, retryAttempt, retryPeriod);
26572659
SqlClientEventSource.Log.TryAdvancedTraceEvent("<sc.SqlInternalConnectionTds.GetFedAuthToken|ADV> {0}, Retry Attempt: {1}, remaining {2}[Milliseconds]", ObjectID, retryAttempt, _timeout.MillisecondsRemaining);
26582660

26592661
// Sleep for the desired period.
26602662
Thread.Sleep(retryPeriod);
2663+
2664+
// Fall through to retry...
26612665
}
26622666
}
26632667

2664-
// if (_fedAuthToken is null)
2665-
// {
2666-
// // TODO: This is a logic error - eliminate it.
2667-
// throw SqlException.CreateException(
2668-
// new SqlErrorCollection().Add(
2669-
// new SqlError(
2670-
// 0, (byte)0, (byte)0, "unknown",
2671-
// "Logic error; fedAuthToken is null",
2672-
// null, 0)),
2673-
// "unknown", this, null, null);
2674-
// }
2668+
// Nullable context has exposed that _fedAuthToken may be null here,
2669+
// which the existing code didn't handle.
2670+
if (_fedAuthToken is null)
2671+
{
2672+
// We failed to acquire a token, so use a default one.
2673+
//
2674+
// TODO: The old code actually allowed the AccessToken byte
2675+
// array to be null, and then had Debug.Assert()s to verify it
2676+
// wasn't null. We never test in debug, so those were never
2677+
// firing, and we were happily using a _fedAuthToken with a null
2678+
// AccessToken. Now that SqlFedAuthToken doesn't allow a null
2679+
// AccessToken, we just create an empty one instead.
2680+
_fedAuthToken = new SqlFedAuthToken([], 0);
2681+
}
26752682

26762683
// Store the newly generated token in _newDbConnectionPoolAuthenticationContext, only if using pooling.
26772684
if (_dbConnectionPool != null)

‎src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs‎

Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2678,17 +2678,15 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
26782678
}
26792679
catch (SqlAuthenticationProviderException ex)
26802680
{
2681-
// We use a doubling backoff if the provider didn't provide
2682-
// a retry period.
2683-
int retryPeriod = defaultRetryPeriod * (2 ^ retryAttempt);
2684-
2685-
// Should we retry, and were we given a useful retry period?
2686-
if (ex.ShouldRetry && ex.RetryPeriod > 0)
2681+
// Is the error fatal or retryable?
2682+
if (!ex.ShouldRetry)
26872683
{
2688-
// Yes, so use it.
2689-
retryPeriod = ex.RetryPeriod;
2684+
// It's fatal, so translate into a SqlException.
2685+
throw ADP.CreateSqlException(ex, ConnectionOptions, this, username);
26902686
}
26912687

2688+
// We should retry.
2689+
26922690
// Could we retry if we wanted to?
26932691
if (_timeout.IsExpired || _timeout.MillisecondsRemaining <= 0)
26942692
{
@@ -2697,26 +2695,37 @@ internal SqlFedAuthToken GetFedAuthToken(SqlFedAuthInfo fedAuthInfo)
26972695
throw SQL.ActiveDirectoryTokenRetrievingTimeout(Enum.GetName(typeof(SqlAuthenticationMethod), ConnectionOptions.Authentication), ex.FailureCode, ex);
26982696
}
26992697

2700-
// We will retry, so log.
2698+
// We use a doubling backoff if the provider didn't provide
2699+
// a retry period.
2700+
int retryPeriod =
2701+
ex.RetryPeriod > 0
2702+
? ex.RetryPeriod
2703+
: defaultRetryPeriod * (2 ^ retryAttempt);
2704+
27012705
SqlClientEventSource.Log.TryAdvancedTraceEvent("<sc.SqlInternalConnectionTds.GetFedAuthToken|ADV> {0}, Retry Attempt: {1}, sleeping {2}[Milliseconds]", ObjectID, retryAttempt, retryPeriod);
27022706
SqlClientEventSource.Log.TryAdvancedTraceEvent("<sc.SqlInternalConnectionTds.GetFedAuthToken|ADV> {0}, Retry Attempt: {1}, remaining {2}[Milliseconds]", ObjectID, retryAttempt, _timeout.MillisecondsRemaining);
27032707

27042708
// Sleep for the desired period.
27052709
Thread.Sleep(retryPeriod);
2710+
2711+
// Fall through to retry...
27062712
}
27072713
}
27082714

2709-
// if (_fedAuthToken is null)
2710-
// {
2711-
// // TODO: This is a logic error - eliminate it.
2712-
// throw SqlException.CreateException(
2713-
// new SqlErrorCollection().Add(
2714-
// new SqlError(
2715-
// 0, (byte)0, (byte)0, "unknown",
2716-
// "Logic error; fedAuthToken is null",
2717-
// null, 0)),
2718-
// "unknown", this, null, null);
2719-
// }
2715+
// Nullable context has exposed that _fedAuthToken may be null here,
2716+
// which the existing code didn't handle.
2717+
if (_fedAuthToken is null)
2718+
{
2719+
// We failed to acquire a token, so use a default one.
2720+
//
2721+
// TODO: The old code actually allowed the AccessToken byte
2722+
// array to be null, and then had Debug.Assert()s to verify it
2723+
// wasn't null. We never test in debug, so those were never
2724+
// firing, and we were happily using a _fedAuthToken with a null
2725+
// AccessToken. Now that SqlFedAuthToken doesn't allow a null
2726+
// AccessToken, we just create an empty one instead.
2727+
_fedAuthToken = new SqlFedAuthToken([], 0);
2728+
}
27202729

27212730
// Store the newly generated token in _newDbConnectionPoolAuthenticationContext, only if using pooling.
27222731
if (_dbConnectionPool != null)

0 commit comments

Comments
 (0)