Skip to content
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

Refactored WwwAuthenticateParameters #2907

Merged
merged 18 commits into from
Oct 4, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Microsoft.Identity.Client.Http
internal static class HttpClientConfig
{
public const long MaxResponseContentBufferSizeInBytes = 1024 * 1024;
public const int MaxConnections = 50; // default depends on rutnime but it is much smaller
public const int MaxConnections = 50; // default depends on runtime but it is much smaller
public static readonly TimeSpan ConnectionLifeTime = TimeSpan.FromMinutes(1);

public static void ConfigureRequestHeadersAndSize(HttpClient httpClient)
Expand Down
80 changes: 57 additions & 23 deletions src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
Expand All @@ -7,7 +7,9 @@
using System.Net.Http;
using System.Net.Http.Headers;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Identity.Client.PlatformsCommon.Factories;

namespace Microsoft.Identity.Client
{
Expand All @@ -32,7 +34,7 @@ public IEnumerable<string> Scopes
{
get
{
if (_scopes != null)
if (_scopes is object)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not seen this type of check before. I am curious - how does it differ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new feature in C# 7.0, part of added pattern matching. It bypasses any potential operator overloads (for either == and/or !=) and directly emits the intended IL for null/not null comparison.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: While this is definitely interesting, these checks obfuscate the fact that we are checking for null here. The word "is" usually implies that you are checking if an object is of a certain type, not whether or not it is null so this may be confusing to someone who is not aware of this feature and they may end up adding the null check in addition to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's a (relatively) new feature and might be confusing. As any other new feature :)

There are some potential benefits in using it, plus many most popular OSS projects have switched to using it too.

So here's our options:

  1. Continue to use != null and == null
  2. Use is object and is null (my preference)
  3. Use is not null and is null (I personally find the former the least readable)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like option 1 or 3.
Thoughts @bgavrilMS?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very interesting problem. It looks like "obj is null" is something we can do today. But "object is not null" is a C# 9.0 feature, which we don't currently use (but we probably should).

I agree that "a is object" is a bit counter-intuitive.

According to https://stackoverflow.com/questions/40676426/what-is-the-difference-between-x-is-null-and-x-null it looks like the compiler (Roslyn) was actually modified so that it emits the same kind of logic as long as == and != are not overloaded (I believe we haven't overloaded them for any object in MSAL). '

So maybe let's keep on using option 1 for consistency with MSAL, and separately we can look at moving to C# 9 and adding some analyzers for this kind of problem. Any changes should be consistent across the lib.

Copy link
Member

@bgavrilMS bgavrilMS Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue tracking the analyzer work #2921

{
return _scopes;
}
Expand Down Expand Up @@ -134,13 +136,52 @@ public static WwwAuthenticateParameters CreateFromWwwAuthenticateHeaderValue(str
/// Create the authenticate parameters by attempting to call the resource unauthenticated, and analyzing the response.
/// </summary>
/// <param name="resourceUri">URI of the resource.</param>
/// <param name="cancellationToken">The cancellation token to cancel operation.</param>
/// <returns>WWW-Authenticate Parameters extracted from response to the un-authenticated call.</returns>
public static async Task<WwwAuthenticateParameters> CreateFromResourceResponseAsync(string resourceUri)
public static Task<WwwAuthenticateParameters> CreateFromResourceResponseAsync(string resourceUri, CancellationToken cancellationToken = default)
{
var httpClientFactory = PlatformProxyFactory.CreatePlatformProxy(null).CreateDefaultHttpClientFactory();
abatishchev marked this conversation as resolved.
Show resolved Hide resolved
return CreateFromResourceResponseAsync(httpClientFactory, resourceUri, cancellationToken);
}

/// <summary>
/// Create the authenticate parameters by attempting to call the resource unauthenticated, and analyzing the response.
/// </summary>
/// <param name="httpClientFactory">Factory to produce an instance of <see cref="HttpClient"/> to make the request with.</param>
/// <param name="resourceUri">URI of the resource.</param>
/// <param name="cancellationToken">The cancellation token to cancel operation.</param>
/// <returns>WWW-Authenticate Parameters extracted from response to the un-authenticated call.</returns>
public static Task<WwwAuthenticateParameters> CreateFromResourceResponseAsync(IMsalHttpClientFactory httpClientFactory, string resourceUri, CancellationToken cancellationToken = default)
{
if (httpClientFactory is null)
{
throw new ArgumentException($"'{nameof(httpClientFactory)}' cannot be null.", nameof(httpClientFactory));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We typically use ArgumentNullException for these types of checks on the api surface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I noticed that too. Let me explain.

The only already existing check is for resourceId for not null or empty or white space. And it throws just ArgumentException. Some people may argue that if the input is an empty string the code can't throw ArgumentNullException because it's not null. But I personally think that's nitpicking and matters less than readable and consistent code.

In this PR I'm adding two more checks, both are for reference types so I'm just checking for not null.
And the caveat is in the order of parameters: ArgumentException(message, paramName) vs ArgumentNullException(paramName, message). Why the inconsistency is a question to BCL. So in our case the code looks little bit more messy and less readable.

So what I would do is to throw ArgumentNullException in all checks. What you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we do that as well in most places, i.e. throw ArgumentNullException for empty string.

Strings should really not be nullable :), but unfortunately we are not a library that uses the nullable references feature, so we have to live with these small inconsistencies.

}

var httpClient = httpClientFactory.GetHttpClient();
return CreateFromResourceResponseAsync(httpClient, resourceUri, cancellationToken);
}

/// <summary>
/// Create the authenticate parameters by attempting to call the resource unauthenticated, and analyzing the response.
/// </summary>
/// <param name="httpClient">Instance of <see cref="HttpClient"/> to make the request with.</param>
/// <param name="resourceUri">URI of the resource.</param>
/// <param name="cancellationToken">The cancellation token to cancel operation.</param>
/// <returns>WWW-Authenticate Parameters extracted from response to the un-authenticated call.</returns>
public static async Task<WwwAuthenticateParameters> CreateFromResourceResponseAsync(HttpClient httpClient, string resourceUri, CancellationToken cancellationToken = default)
{
if (httpClient is null)
{
throw new ArgumentException($"'{nameof(httpClient)}' cannot be null.", nameof(httpClient));
}
if (string.IsNullOrWhiteSpace(resourceUri))
{
throw new ArgumentException($"'{nameof(resourceUri)}' cannot be null or whitespace.", nameof(resourceUri));
}

// call this endpoint and see what the header says and return that
HttpClient httpClient = new HttpClient();
HttpRequestMessage httpRequestMessage = new HttpRequestMessage(HttpMethod.Get, resourceUri);
HttpResponseMessage httpResponseMessage = await httpClient.SendAsync(httpRequestMessage).ConfigureAwait(false);
HttpResponseMessage httpResponseMessage = await httpClient.GetAsync(resourceUri, cancellationToken).ConfigureAwait(false);
var wwwAuthParam = CreateFromResponseHeaders(httpResponseMessage.Headers);
return wwwAuthParam;
}
Expand All @@ -160,31 +201,24 @@ public static async Task<WwwAuthenticateParameters> CreateFromResourceResponseAs
httpResponseHeaders,
scheme);

try
// read the header and checks if it contains an error with insufficient_claims value.
if (string.Equals(parameters.Error, "insufficient_claims", StringComparison.OrdinalIgnoreCase) &&
parameters.Claims is object)
{
// read the header and checks if it contains an error with insufficient_claims value.
if (null != parameters.Error && "insufficient_claims" == parameters.Error)
{
if (null != parameters.Claims)
{
return parameters.Claims;
}
}
}
catch (Exception ex)
{
throw ex;
return parameters.Claims;
}

return null;
}

internal static WwwAuthenticateParameters CreateWwwAuthenticateParameters(IDictionary<string, string> values)
{
WwwAuthenticateParameters wwwAuthenticateParameters = new WwwAuthenticateParameters();
wwwAuthenticateParameters.RawParameters = values;
string value;
WwwAuthenticateParameters wwwAuthenticateParameters = new WwwAuthenticateParameters
{
RawParameters = values
};

string value;
if (values.TryGetValue("authorization_uri", out value))
{
wwwAuthenticateParameters.Authority = value.Replace("/oauth2/authorize", string.Empty);
Expand Down Expand Up @@ -325,7 +359,7 @@ private static string GetJsonFragment(string inputString)
var decoded = Encoding.UTF8.GetString(decodedBase64Bytes);
return decoded;
}
catch (Exception)
catch
{
return inputString;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using System;
using System.Collections.Generic;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Net.Http;
using System.Threading;
Expand All @@ -26,7 +25,7 @@ internal class MockHttpMessageHandler : HttpMessageHandler
public IList<string> UnexpectedRequestHeaders { get; set; }

public HttpMethod ExpectedMethod { get; set; }

public Exception ExceptionToThrow { get; set; }
public Action<HttpRequestMessage> AdditionalRequestValidation { get; set; }

Expand All @@ -51,11 +50,7 @@ protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage reques
{
Assert.AreEqual(
ExpectedUrl,
uri.AbsoluteUri.Split(
new[]
{
'?'
})[0]);
uri.AbsoluteUri.Split('?')[0]);
}

Assert.AreEqual(ExpectedMethod, request.Method);
Expand All @@ -67,7 +62,7 @@ protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage reques
string.IsNullOrEmpty(uri.Query),
string.Format(
CultureInfo.InvariantCulture,
"provided url ({0}) does not contain query parameters, as expected",
"Provided url ({0}) does not contain query parameters, as expected",
uri.AbsolutePath));
IDictionary<string, string> inputQp = CoreHelpers.ParseKeyValueList(uri.Query.Substring(1), '&', false, null);
Assert.AreEqual(ExpectedQueryParams.Count, inputQp.Count, "Different number of query params`");
Expand All @@ -77,7 +72,7 @@ protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage reques
inputQp.ContainsKey(key),
string.Format(
CultureInfo.InvariantCulture,
"expected QP ({0}) not found in the url ({1})",
"Expected query parameter ({0}) not found in the url ({1})",
key,
uri.AbsolutePath));
Assert.AreEqual(ExpectedQueryParams[key], inputQp[key]);
Expand All @@ -104,8 +99,8 @@ protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage reques
Assert.AreEqual(ExpectedPostData[key], ActualRequestPostData[key]);
}
}
}
}

if (ExpectedRequestHeaders != null )
{
foreach (var kvp in ExpectedRequestHeaders)
Expand All @@ -132,13 +127,5 @@ protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage reques

return new TaskFactory().StartNew(() => ResponseMessage, cancellationToken);
}

private string ReturnValueFromRequestHeader(string telemRequest)
{
IEnumerable<string> telemRequestValue = ActualRequestMessage.Headers.GetValues(telemRequest);
List<string> telemRequestValueAsList = telemRequestValue.ToList();
string value = telemRequestValueAsList[0];
return value;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,31 @@ public async Task CreateWwwAuthenticateResponseFromGraphUrlAsync()
Assert.IsNull(authParams.Claims);
Assert.IsNull(authParams.Error);
}

/// <summary>
/// Makes unauthorized call to Azure Resource Manager REST API https://docs.microsoft.com/en-us/rest/api/resources/subscriptions/get.
/// Expects response 401 Unauthorized. Analyzes the WWW-Authenticate header values.
/// </summary>
/// <param name="hostName">ARM endpoint, e.g. Production or Dogfood</param>
/// <param name="subscriptionId">Well-known subscription ID</param>
/// <param name="authority">AAD endpoint, e.g. Production or PPE</param>
/// <param name="tenantId">Expected Tenant ID</param>
[TestMethod]
[DataRow("management.azure.com", "c1686c51-b717-4fe0-9af3-24a20a41fb0c", "login.windows.net", "72f988bf-86f1-41af-91ab-2d7cd011db47")]
[DataRow("api-dogfood.resources.windows-int.net", "1835ad3d-4585-4c5f-b55a-b0c3cbda1103", "login.windows-ppe.net", "f686d426-8d16-42db-81b7-ab578e110ccd")]
public async Task CreateWwwAuthenticateResponseFromAzureResourceManagerUrlAsync(string hostName, string subscriptionId, string authority, string tenantId)
bgavrilMS marked this conversation as resolved.
Show resolved Hide resolved
{
const string apiVersion = "2020-08-01"; // current latest API version for /subscriptions/get
var url = $"https://{hostName}/subscriptions/{subscriptionId}?api-version={apiVersion}";

var authParams = await WwwAuthenticateParameters.CreateFromResourceResponseAsync(url).ConfigureAwait(false);

Assert.IsNull(authParams.Resource);
Assert.AreEqual($"https://{authority}/{tenantId}", authParams.Authority); // authority consists of AAD endpoint and tenant ID
Assert.IsNull(authParams.Scopes);
bgavrilMS marked this conversation as resolved.
Show resolved Hide resolved
Assert.AreEqual(3, authParams.RawParameters.Count);
Assert.IsNull(authParams.Claims);
Assert.AreEqual("invalid_token", authParams.Error);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<TargetFrameworkNetDesktop461>net472</TargetFrameworkNetDesktop461>
<TargetFrameworkNetCore>netcoreapp3.1</TargetFrameworkNetCore>
<TargetFrameworkNet5Win>net5.0-windows10.0.17763.0</TargetFrameworkNet5Win>

<TargetFrameworks>$(TargetFrameworkNetDesktop461);$(TargetFrameworkNetCore);$(TargetFrameworkNet5Win)</TargetFrameworks>
<IsPackable>false</IsPackable>
</PropertyGroup>
Expand Down
3 changes: 1 addition & 2 deletions tests/Microsoft.Identity.Test.Unit/TestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public static void AssemblyInit(TestContext context)
Trace.WriteLine("Test run started");
}

[AssemblyCleanup()]
[AssemblyCleanup]
public static void AssemblyCleanup()
{
Trace.WriteLine("Test run finished");
Expand Down Expand Up @@ -61,7 +61,6 @@ public virtual void TestCleanup()
testContext: TestContext);
}


private static void EnableFileTracingOnEnvVar()
{
string traceFile = Environment.GetEnvironmentVariable("MsalTracePath");
Expand Down
Loading