-
Notifications
You must be signed in to change notification settings - Fork 315
Enable User Agent Extension #3606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a144ef6
13090ce
8359565
c450c61
c8897d3
c0eea2b
26dd6f9
8ad3c69
2184035
af1eeb1
3b51844
ff39552
2394a2f
d673104
90c97c5
d494b66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
using System.Buffers; | ||
using System.Diagnostics; | ||
using System.Text; | ||
using Microsoft.Data.SqlClient.UserAgent; | ||
using Microsoft.Data.SqlClient.Utilities; | ||
|
||
#nullable enable | ||
|
@@ -192,7 +193,14 @@ internal void TdsLogin( | |
|
||
int feOffset = length; | ||
// calculate and reserve the required bytes for the featureEx | ||
length = ApplyFeatureExData(requestedFeatures, recoverySessionData, fedAuthFeatureExtensionData, useFeatureExt, length); | ||
length = ApplyFeatureExData( | ||
requestedFeatures, | ||
recoverySessionData, | ||
fedAuthFeatureExtensionData, | ||
UserAgentInfo.UserAgentCachedJsonPayload.ToArray(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Be careful with .ToArray() - this is likely duplicating the array |
||
useFeatureExt, | ||
length | ||
); | ||
|
||
WriteLoginData(rec, | ||
requestedFeatures, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,8 @@ | |
using Microsoft.Data.SqlClient.DataClassification; | ||
using Microsoft.Data.SqlClient.LocalDb; | ||
using Microsoft.Data.SqlClient.Server; | ||
using Microsoft.Data.SqlClient.UserAgent; | ||
|
||
#if NETFRAMEWORK | ||
using Microsoft.Data.SqlTypes; | ||
#endif | ||
|
@@ -9233,7 +9235,15 @@ private void WriteLoginData(SqlLogin rec, | |
} | ||
} | ||
|
||
ApplyFeatureExData(requestedFeatures, recoverySessionData, fedAuthFeatureExtensionData, useFeatureExt, length, true); | ||
ApplyFeatureExData( | ||
requestedFeatures, | ||
recoverySessionData, | ||
fedAuthFeatureExtensionData, | ||
UserAgentInfo.UserAgentCachedJsonPayload.ToArray(), | ||
useFeatureExt, | ||
length, | ||
true | ||
); | ||
} | ||
catch (Exception e) | ||
{ | ||
|
@@ -9252,6 +9262,7 @@ private void WriteLoginData(SqlLogin rec, | |
private int ApplyFeatureExData(TdsEnums.FeatureExtension requestedFeatures, | ||
SessionData recoverySessionData, | ||
FederatedAuthenticationFeatureExtensionData fedAuthFeatureExtensionData, | ||
byte[] userAgentJsonPayload, | ||
bool useFeatureExt, | ||
int length, | ||
bool write = false) | ||
|
@@ -9306,6 +9317,11 @@ private int ApplyFeatureExData(TdsEnums.FeatureExtension requestedFeatures, | |
length += WriteVectorSupportFeatureRequest(write); | ||
} | ||
|
||
if ((requestedFeatures & TdsEnums.FeatureExtension.UserAgent) != 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to write the User Agent FE first. Move this up to line 9274 and add a comment explaining why it must appear first. |
||
{ | ||
length += WriteUserAgentFeatureRequest(userAgentJsonPayload, write); | ||
} | ||
|
||
length++; // for terminator | ||
if (write) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -828,5 +828,86 @@ public void TestConnWithVectorFeatExtVersionNegotiation(bool expectedConnectionR | |
Assert.Throws<InvalidOperationException>(() => connection.Open()); | ||
} | ||
} | ||
|
||
// Test to verify that the client sends a UserAgent version | ||
// and driver behaves correctly even if server sent an Ack | ||
[Theory] | ||
[InlineData(false)] // We do not force test server to send an Ack | ||
[InlineData(true)] // Server is forced to send an Ack | ||
public void TestConnWithUserAgentFeatureExtension(bool forceAck) | ||
{ | ||
using var server = new TdsServer(); | ||
server.Start(); | ||
|
||
// Configure the server to support UserAgent version 0x01 | ||
server.ServerSupportedUserAgentFeatureExtVersion = 0x01; | ||
|
||
// Opt in to forced ACK for UserAgentSupport (no negotiation) | ||
server.EmitUserAgentFeatureExtAck = forceAck; | ||
|
||
bool loginFound = false; | ||
|
||
// Captured from LOGIN7 as parsed by the test server | ||
byte observedVersion = 0; | ||
byte[] observedJsonBytes = Array.Empty<byte>(); | ||
|
||
// Inspect what the client sends in the LOGIN7 packet | ||
server.OnLogin7Validated = loginToken => | ||
{ | ||
var token = loginToken.FeatureExt | ||
.OfType<TDSLogin7GenericOptionToken>() | ||
.FirstOrDefault(t => t.FeatureID == TDSFeatureID.UserAgentSupport); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should confirm that the User Agent FE is the first FE in the LOGIN7 packet. |
||
if (token == null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be:
The driver is supposed to always send the User Agent FE. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realize that you're checking |
||
{ | ||
return; | ||
} | ||
|
||
var data = token.Data; | ||
if (data == null || data.Length < 2) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above, this must fail the test:
|
||
{ | ||
return; | ||
} | ||
|
||
observedVersion = data[0]; | ||
observedJsonBytes = data.AsSpan(1).ToArray(); | ||
loginFound = true; | ||
}; | ||
|
||
// Connect to the test TDS server. | ||
var connStr = new SqlConnectionStringBuilder | ||
{ | ||
DataSource = $"localhost,{server.EndPoint.Port}", | ||
Encrypt = SqlConnectionEncryptOption.Optional, | ||
}.ConnectionString; | ||
|
||
// TODO: Confirm the server sent an Ack by reading log message from SqlInternalConnectionTds | ||
using var connection = new SqlConnection(connStr); | ||
connection.Open(); | ||
|
||
// Verify the connection itself succeeded | ||
Assert.Equal(ConnectionState.Open, connection.State); | ||
|
||
// Verify client did offer UserAgent | ||
Assert.True(loginFound, "Expected UserAgent extension in LOGIN7"); | ||
Assert.Equal(0x1, observedVersion); | ||
|
||
// Note: Accessing UserAgentInfo via Reflection. | ||
// We cannot use InternalsVisibleTo here because making internals visible to FunctionalTests | ||
// causes the *.TestHarness.cs stubs to clash with the real internal types in SqlClient. | ||
var asm = typeof(SqlConnection).Assembly; | ||
var userAgentInfoType = | ||
asm.GetTypes().FirstOrDefault(t => string.Equals(t.Name, "UserAgentInfo", StringComparison.Ordinal)) ?? | ||
asm.GetTypes().FirstOrDefault(t => t.FullName?.EndsWith(".UserAgentInfo", StringComparison.Ordinal) == true); | ||
|
||
Assert.NotNull(userAgentInfoType); | ||
|
||
// Try to get the property | ||
var prop = userAgentInfoType.GetProperty("UserAgentCachedJsonPayload", | ||
BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic); | ||
Assert.NotNull(prop); | ||
|
||
ReadOnlyMemory<byte> cachedPayload = (ReadOnlyMemory<byte>)prop.GetValue(null)!; | ||
Assert.Equal(cachedPayload.ToArray(), observedJsonBytes.ToArray()); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,11 +50,36 @@ public delegate void OnAuthenticationCompletedDelegate( | |
/// </summary> | ||
public const byte DefaultSupportedVectorFeatureExtVersion = 0x01; | ||
|
||
/// <summary> | ||
/// Property for setting server version for vector feature extension. | ||
/// </summary> | ||
public bool EnableVectorFeatureExt { get; set; } = false; | ||
|
||
/// <summary> | ||
/// Property for setting server version for vector feature extension. | ||
/// </summary> | ||
public byte ServerSupportedVectorFeatureExtVersion { get; set; } = DefaultSupportedVectorFeatureExtVersion; | ||
|
||
/// <summary> | ||
/// Property for setting server version for user agent feature extension. | ||
/// </summary> | ||
public byte ServerSupportedUserAgentFeatureExtVersion { get; set; } = DefaultSupportedUserAgentFeatureExtVersion; | ||
|
||
/// <summary> | ||
/// Client version for vector FeatureExtension. | ||
/// </summary> | ||
private byte _clientSupportedVectorFeatureExtVersion = 0; | ||
|
||
/// <summary> | ||
/// Server will ACK UserAgentSupport in the login response when this property is set to true. | ||
/// </summary> | ||
public bool EmitUserAgentFeatureExtAck { get; set; } = false; | ||
|
||
/// <summary> | ||
/// Default feature extension version supported on the server for user agent. | ||
/// </summary> | ||
public const byte DefaultSupportedUserAgentFeatureExtVersion = 0x01; | ||
|
||
/// <summary> | ||
/// Session counter | ||
/// </summary> | ||
|
@@ -107,16 +132,6 @@ public GenericTdsServer(T arguments, QueryEngine queryEngine) | |
/// </summary> | ||
public int PreLoginCount => _preLoginCount; | ||
|
||
/// <summary> | ||
/// Property for setting server version for vector feature extension. | ||
/// </summary> | ||
public bool EnableVectorFeatureExt { get; set; } = false; | ||
|
||
/// <summary> | ||
/// Property for setting server version for vector feature extension. | ||
/// </summary> | ||
public byte ServerSupportedVectorFeatureExtVersion { get; set; } = DefaultSupportedVectorFeatureExtVersion; | ||
|
||
public OnAuthenticationCompletedDelegate OnAuthenticationResponseCompleted { private get; set; } | ||
|
||
public OnLogin7ValidatedDelegate OnLogin7Validated { private get; set; } | ||
|
@@ -691,6 +706,30 @@ protected virtual TDSMessageCollection OnAuthenticationCompleted(ITDSServerSessi | |
responseMessage.Add(envChange); | ||
} | ||
|
||
// If tests request it, force an ACK for UserAgentSupport with no negotiation | ||
if (EmitUserAgentFeatureExtAck) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please rewrite this large method to split these conditional blocks into individual methods? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should you not be reading session instance value here as done above for other feature ext tokens? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
{ | ||
byte ackVersion = ServerSupportedUserAgentFeatureExtVersion; | ||
|
||
var data = new byte[] { ackVersion }; | ||
var uaAck = new TDSFeatureExtAckGenericOption( | ||
TDSFeatureID.UserAgentSupport, | ||
(uint)data.Length, | ||
data); | ||
|
||
// Reuse an existing FeatureExtAck token if present, otherwise add a new one | ||
var featureExtAckToken = responseMessage.OfType<TDSFeatureExtAckToken>().FirstOrDefault(); | ||
if (featureExtAckToken == null) | ||
{ | ||
featureExtAckToken = new TDSFeatureExtAckToken(uaAck); | ||
responseMessage.Add(featureExtAckToken); | ||
} | ||
else | ||
{ | ||
featureExtAckToken.Options.Add(uaAck); | ||
} | ||
} | ||
|
||
// Create DONE token | ||
TDSDoneToken doneToken = new TDSDoneToken(TDSDoneTokenStatusType.Final); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it still necessary to verify if it competes with another feature?