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.

Sign up for GitHub

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

Jump to bottom

Refactored WwwAuthenticateParameters #2907

Merged
merged 18 commits into from
Oct 4, 2021
Merged

Refactored WwwAuthenticateParameters #2907

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
45632d8
Refactored WwwAuthenticateParameters
abatishchev Sep 28, 2021
7081af9
Update WwwAuthenticateParameters.cs
abatishchev Sep 28, 2021
520b92b
Fixes
abatishchev Sep 28, 2021
be08f01
Unit tests
abatishchev Sep 28, 2021
d202fa8
Integration tests
abatishchev Sep 28, 2021
db14929
401
abatishchev Sep 28, 2021
52067f0
Changed subscription guids
abatishchev Sep 29, 2021
1b81ee1
Annotated test
abatishchev Sep 29, 2021
dccfd49
GetAsync
abatishchev Sep 29, 2021
79550f8
overload 1
abatishchev Sep 29, 2021
2ca6bf2
overload 2
abatishchev Sep 29, 2021
639403b
PlatformProxyFactory
abatishchev Sep 29, 2021
b27c449
Added tests
abatishchev Sep 29, 2021
e72fe5e
typo
abatishchev Sep 29, 2021
e36ca40
added null checks
abatishchev Sep 30, 2021
1782f3d
Added tests
abatishchev Sep 30, 2021
5393d35
Update WwwAuthenticateParametersTests.cs
abatishchev Sep 30, 2021
a3d7f15
Minor comments
bgavrilMS Oct 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 12 additions & 19 deletions src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs
View file
Open in desktop
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,6 +7,7 @@
using System.Net.Http;
using System.Net.Http.Headers;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace Microsoft.Identity.Client
Expand All @@ -32,7 +33,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 •
edited
Loading

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 +135,14 @@ 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 async Task<WwwAuthenticateParameters> CreateFromResourceResponseAsync(string resourceUri, CancellationToken cancellationToken = default)
abatishchev marked this conversation as resolved.
Show resolved Hide resolved
{
// 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.SendAsync(httpRequestMessage, cancellationToken).ConfigureAwait(false);
var wwwAuthParam = CreateFromResponseHeaders(httpResponseMessage.Headers);
return wwwAuthParam;
}
Expand All @@ -160,20 +162,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 (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;
Expand All @@ -183,8 +176,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 +318,7 @@ private static string GetJsonFragment(string inputString)
var decoded = Encoding.UTF8.GetString(decodedBase64Bytes);
return decoded;
}
catch (Exception)
catch
{
return inputString;
}
Expand Down
26 changes: 26 additions & 0 deletions ...dentity.Test.Integration.netfx/HeadlessTests/WwwAuthenticateParametersIntegrationTests.cs
View file
Open in desktop
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>
[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")]
[DataTestMethod]
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);
}
}
}
60 changes: 25 additions & 35 deletions tests/Microsoft.Identity.Test.Unit/WwwAuthenticateParametersTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,33 @@
// 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 Microsoft.Identity.Client;
using System.Collections.Generic;
using Microsoft.VisualStudio.TestTools.UnitTesting;

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 +39,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 +97,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,20 +123,18 @@ 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);
Expand Down Expand Up @@ -183,26 +179,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