-
Notifications
You must be signed in to change notification settings - Fork 51
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
Report different user-agent strings for ASP.NET and ASP.NET Core #49
Report different user-agent strings for ASP.NET and ASP.NET Core #49
Conversation
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.
Can we add some tests to assert that the user agent string is being built as we expect it?
@@ -47,7 +47,7 @@ private static AuthenticationBuilder AddCodeFlow(AuthenticationBuilder builder, | |||
oidcOptions.GetClaimsFromUserInfoEndpoint = options.GetClaimsFromUserInfoEndpoint; | |||
oidcOptions.SaveTokens = true; | |||
oidcOptions.UseTokenLifetime = false; | |||
oidcOptions.BackchannelHttpHandler = new UserAgentHandler(); | |||
oidcOptions.BackchannelHttpHandler = new UserAgentHandler("aspnet-core", typeof(OktaAuthenticationOptionsExtensions).Assembly.GetName().Version); |
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.
nit: Use okta-aspnetcore
@@ -82,7 +82,7 @@ private static void AddJwtBearerAuthentication(IAppBuilder app, OktaWebApiOption | |||
private static void AddOpenIdConnectAuthentication(IAppBuilder app, OktaMvcOptions options) | |||
{ | |||
var issuer = UrlHelper.CreateIssuerUrl(options.OktaDomain, options.AuthorizationServerId); | |||
var httpClient = new HttpClient(new UserAgentHandler()); | |||
var httpClient = new HttpClient(new UserAgentHandler("aspnet", typeof(OktaMiddlewareExtensions).Assembly.GetName().Version)); |
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.
nit: Use okta-aspnet
} | ||
|
||
protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) | ||
{ | ||
request.Headers.Add("user-agent", UserAgentBuilder.GetUserAgent()); | ||
request.Headers.Add("user-agent", _userAgentBuilder.GetUserAgent()); |
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.
Let's use lazy initialization so we only have to call GetUserAgent()
once, not on every request. Should be able to just wrap that call in a Lazy<string>
.
- Add unit test for UserAgentHandler
{ | ||
protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) | ||
{ | ||
return Task.Factory.StartNew(() => new HttpResponseMessage(HttpStatusCode.OK), cancellationToken); |
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.
Typically you can use Task.FromResult
to return a Task<T>
LGTM! If you can use |
httpRequestMessage.Headers.UserAgent.ToString() | ||
.IndexOf( | ||
ProductInfoHeaderValue.Parse($"{frameworkName}/{version.Major}.{version.Minor}.{version.Build}") | ||
.ToString(), StringComparison.InvariantCultureIgnoreCase).Should().BeGreaterOrEqualTo(0); |
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.
Just curious, does this work with StringComparison.OrdinalIgnoreCase
too, or only InvariantCulture
?
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.
No, it does work with OrdinalIgnoreCase
as well.
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.
LGTM
Resolves #48