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
60 changes: 39 additions & 21 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,38 @@ 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)
{
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)
{
// 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,20 +187,11 @@ public static async Task<WwwAuthenticateParameters> CreateFromResourceResponseAs
httpResponseHeaders,
scheme);

try
{
// 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)
// 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)
{
throw ex;
return parameters.Claims;
}

return null;
Expand All @@ -183,8 +201,8 @@ internal static WwwAuthenticateParameters CreateWwwAuthenticateParameters(IDicti
{
WwwAuthenticateParameters wwwAuthenticateParameters = new WwwAuthenticateParameters();
wwwAuthenticateParameters.RawParameters = values;
string value;

string value;
if (values.TryGetValue("authorization_uri", out value))
{
wwwAuthenticateParameters.Authority = value.Replace("/oauth2/authorize", string.Empty);
Expand Down Expand Up @@ -325,7 +343,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 @@ -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
Original file line number Diff line number Diff line change
@@ -1,33 +1,36 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System.Collections.Generic;
using System.Linq;
using System.Net;
using System.Net.Http;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using System.Threading.Tasks;
using Microsoft.Identity.Client;
using System.Collections.Generic;
using Microsoft.Identity.Test.Common.Core.Mocks;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using NSubstitute;

namespace Microsoft.Identity.Test.Unit
{
[TestClass]
public class WwwAuthenticateParametersTests
{
const string ClientIdKey = "client_id";
const string ResourceIdKey = "resource_id";
const string ResourceKey = "resource";
const string GraphGuid = "00000003-0000-0000-c000-000000000000";
const string AuthorizationUriKey = "authorization_uri";
const string AuthorizationKey = "authorization";
const string AuthorityKey = "authority";
const string AuthorizationValue = "https://login.microsoftonline.com/common/oauth2/authorize";
const string Realm = "realm";
const string EncodedClaims = "eyJpZF90b2tlbiI6eyJhdXRoX3RpbWUiOnsiZXNzZW50aWFsIjp0cnVlfSwiYWNyIjp7InZhbHVlcyI6WyJ1cm46bWFjZTppbmNvbW1vbjppYXA6c2lsdmVyIl19fX0=";
const string DecodedClaims = "{\"id_token\":{\"auth_time\":{\"essential\":true},\"acr\":{\"values\":[\"urn:mace:incommon:iap:silver\"]}}}";
const string DecodedClaimsHeader = "{\\\"id_token\\\":{\\\"auth_time\\\":{\\\"essential\\\":true},\\\"acr\\\":{\\\"values\\\":[\\\"urn:mace:incommon:iap:silver\\\"]}}}";
const string SomeClaims = "some_claims";
const string ClaimsKey = "claims";
const string ErrorKey = "error";
private const string ClientIdKey = "client_id";
private const string ResourceIdKey = "resource_id";
private const string ResourceKey = "resource";
private const string GraphGuid = "00000003-0000-0000-c000-000000000000";
private const string AuthorizationUriKey = "authorization_uri";
private const string AuthorizationKey = "authorization";
private const string AuthorityKey = "authority";
private const string AuthorizationValue = "https://login.microsoftonline.com/common/oauth2/authorize";
private const string Realm = "realm";
private const string EncodedClaims = "eyJpZF90b2tlbiI6eyJhdXRoX3RpbWUiOnsiZXNzZW50aWFsIjp0cnVlfSwiYWNyIjp7InZhbHVlcyI6WyJ1cm46bWFjZTppbmNvbW1vbjppYXA6c2lsdmVyIl19fX0=";
private const string DecodedClaims = "{\"id_token\":{\"auth_time\":{\"essential\":true},\"acr\":{\"values\":[\"urn:mace:incommon:iap:silver\"]}}}";
private const string DecodedClaimsHeader = "{\\\"id_token\\\":{\\\"auth_time\\\":{\\\"essential\\\":true},\\\"acr\\\":{\\\"values\\\":[\\\"urn:mace:incommon:iap:silver\\\"]}}}";
private const string SomeClaims = "some_claims";
private const string ClaimsKey = "claims";
private const string ErrorKey = "error";

[TestMethod]
[DataRow("client_id=00000003-0000-0000-c000-000000000000", "authorization_uri=\"https://login.microsoftonline.com/common/oauth2/authorize\"")]
Expand All @@ -39,9 +42,7 @@ public class WwwAuthenticateParametersTests
public void CreateWwwAuthenticateResponse(string resource, string authorizationUri)
{
// Arrange
HttpResponseMessage httpResponse = new HttpResponseMessage((HttpStatusCode)401)
{
};
HttpResponseMessage httpResponse = new HttpResponseMessage(HttpStatusCode.Unauthorized);
httpResponse.Headers.Add("WWW-Authenticate", $"Bearer realm=\"\", {resource}, {authorizationUri}");

// Act
Expand Down Expand Up @@ -99,7 +100,7 @@ public void CreateRawParameters_ClaimsAndErrorReturned(string claims)
var authParams = WwwAuthenticateParameters.CreateFromResponseHeaders(httpResponse.Headers);

// Assert
string errorValue = "insufficient_claims";
const string errorValue = "insufficient_claims";
Assert.IsTrue(authParams.RawParameters.TryGetValue(AuthorizationUriKey, out string authorizationUri));
Assert.AreEqual(AuthorizationValue, authorizationUri);
Assert.AreEqual(AuthorizationValue, authParams[AuthorizationUriKey]);
Expand All @@ -125,25 +126,44 @@ public void CreateRawParameters_ClaimsAndErrorReturned(string claims)
public void CreateWwwAuthenticateParamsFromWwwAuthenticateHeader(string clientId, string authorizationUri)
{
// Arrange
HttpResponseMessage httpResponse = new HttpResponseMessage((HttpStatusCode)401)
{
};
HttpResponseMessage httpResponse = new HttpResponseMessage(HttpStatusCode.Unauthorized);
httpResponse.Headers.Add("WWW-Authenticate", $"Bearer realm=\"\", {clientId}, {authorizationUri}");

var wwwAuthenticateResponse = httpResponse.Headers.WwwAuthenticate.FirstOrDefault().Parameter;
var wwwAuthenticateResponse = httpResponse.Headers.WwwAuthenticate.First().Parameter;

// Act
var authParams = WwwAuthenticateParameters.CreateFromWwwAuthenticateHeaderValue(wwwAuthenticateResponse);

// Assert
Assert.AreEqual(GraphGuid, authParams.Resource);
Assert.AreEqual(TestConstants.AuthorityCommonTenant.TrimEnd('/'), authParams.Authority);
Assert.AreEqual($"{GraphGuid}/.default", authParams.Scopes.FirstOrDefault());
Assert.AreEqual($"{GraphGuid}/.default", authParams.Scopes.First());
Assert.AreEqual(3, authParams.RawParameters.Count);
Assert.IsNull(authParams.Claims);
Assert.IsNull(authParams.Error);
}

[TestMethod]
public async Task CreateFromResourceResponseAsync_HttpClientFactoryAsync()
{
const string resourceUri = "https://example.com/";

var handler = new MockHttpMessageHandler
{
ExpectedMethod = HttpMethod.Get,
ExpectedUrl = resourceUri,
ResponseMessage = new HttpResponseMessage()
};
var httpClient = new HttpClient(handler);

var httpClientFactory = Substitute.For<IMsalHttpClientFactory>();
httpClientFactory.GetHttpClient().Returns(httpClient);

var _ = await WwwAuthenticateParameters.CreateFromResourceResponseAsync(httpClientFactory, resourceUri).ConfigureAwait(false);

httpClientFactory.Received().GetHttpClient();
}

[TestMethod]
public void ExtractClaimChallengeFromHeader()
{
Expand Down Expand Up @@ -183,26 +203,20 @@ public void ExtractClaimChallengeFromHeader_IncorrectError_ReturnNull()
private static HttpResponseMessage CreateClaimsHttpResponse(string claims)
{
HttpResponseMessage httpResponse = new HttpResponseMessage(HttpStatusCode.Unauthorized);
{
};
httpResponse.Headers.Add("WWW-Authenticate", $"Bearer realm=\"\", client_id=\"00000003-0000-0000-c000-000000000000\", authorization_uri=\"https://login.microsoftonline.com/common/oauth2/authorize\", error=\"insufficient_claims\", claims=\"{claims}\"");
return httpResponse;
}

private static HttpResponseMessage CreateErrorHttpResponse()
{
HttpResponseMessage httpResponse = new HttpResponseMessage(HttpStatusCode.Unauthorized)
{
};
HttpResponseMessage httpResponse = new HttpResponseMessage(HttpStatusCode.Unauthorized);
httpResponse.Headers.Add("WWW-Authenticate", $"Bearer realm=\"\", client_id=\"00000003-0000-0000-c000-000000000000\", authorization_uri=\"https://login.microsoftonline.com/common/oauth2/authorize\", error=\"some_error\", claims=\"{DecodedClaimsHeader}\"");
return httpResponse;
}

private static HttpResponseMessage CreateHttpResponseHeaders(string resourceHeaderKey, string authorizationUriHeaderKey)
{
HttpResponseMessage httpResponse = new HttpResponseMessage(HttpStatusCode.Unauthorized)
{
};
HttpResponseMessage httpResponse = new HttpResponseMessage(HttpStatusCode.Unauthorized);
httpResponse.Headers.Add("WWW-Authenticate", $"Bearer realm=\"\", {resourceHeaderKey}=\"{GraphGuid}\", {authorizationUriHeaderKey}=\"{AuthorizationValue}\"");
return httpResponse;
}
Expand Down