Azure Active Directory Authentication using Access Token #30342
Conversation
| { | ||
| CheckAndThrowOnInvalidCombinationOfConnectionStringAndSqlCredential(connectionOptions); | ||
| } | ||
| else if (_accessToken != null) |
There was a problem hiding this comment.
Looks like this could just be:
else // _accessToken != null
| { | ||
| result = null; | ||
| } | ||
| return result; |
There was a problem hiding this comment.
Nit: kind of a strange construction... seems like it could just be:
return InnerConnection.ShouldHidePassword && connectionOptions != null && !connectionOptions.PersistSecurityInfo ?
null :
_accessToken;
| } | ||
|
|
||
| _accessToken = value; | ||
| // Need to call ConnectionString_Set to do proper pool group check |
There was a problem hiding this comment.
Does ConnectionString_Set throw? If it does, is it weird that we would have already then stored the value into _accessToken?
| internal bool _fedAuthRequired; | ||
| internal bool _federatedAuthenticationRequested; | ||
| internal bool _federatedAuthenticationAcknowledged; | ||
| internal byte[] _accessTokenInBytes; |
There was a problem hiding this comment.
Can any of these be made readonly?
There was a problem hiding this comment.
We can't make them readonly. They are being set somewhere else in the code and would cause this issue:
A readonly field cannot be used as a ref or out value
| _physicalStateObj.WriteByteArray(fedAuthFeatureData.accessToken, fedAuthFeatureData.accessToken.Length, 0); | ||
| break; | ||
| default: | ||
| Debug.Assert(false, "Unrecognized FedAuthLibrary type for feature extension request"); |
|
|
||
| switch (_fedAuthFeatureExtensionData.Value.libraryType) | ||
| { | ||
| case TdsEnums.FedAuthLibrary.ADAL: // For later support |
There was a problem hiding this comment.
Is the server, ever expected to send TdsEnums.FedAuthLibrary.ADAL for AccessToken?
| // only add lengths of password and username if not using SSPI | ||
| if (!rec.useSSPI) | ||
| // only add lengths of password and username if not using SSPI or requesting federated authentication info | ||
| if (!rec.useSSPI && !(_connHandler._federatedAuthenticationRequested)) |
There was a problem hiding this comment.
Nit: unnecessary parentheses around _connHandler._federatedAuthenticationRequested
There was a problem hiding this comment.
@afsanehr The changes look good over all. How was this tested? How were the access tokens generated and passed to the server?
| SqlConnectionStringBuilder _builder; | ||
|
|
||
| [Fact] | ||
| public void InvalidCombinationOfAccessToken() |
There was a problem hiding this comment.
This test can be parameterized by passing different connection strings, perhaps by using [Theory] and [InlineData] attribute.
|
@dotnet-bot test NETFX x86 Release Build |
Azure Active Directory Authentication using Access Token Commit migrated from dotnet/corefx@a74bf42
Fixes #13660 This pull request will enable authentication using only Access Token.