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

Fix for #2371 #2374

Merged
merged 11 commits into from
Aug 13, 2023
10 changes: 1 addition & 9 deletions src/Microsoft.Identity.Web.OWIN/OwinTokenAcquisitionHost.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
using Microsoft.Extensions.Options;
using Microsoft.Identity.Abstractions;
using Microsoft.Identity.Client;
using Microsoft.IdentityModel.JsonWebTokens;
using Microsoft.IdentityModel.Tokens;

namespace Microsoft.Identity.Web.Hosts
Expand Down Expand Up @@ -68,14 +67,7 @@ public MergedOptions GetOptions(string? authenticationScheme, out string effecti

public SecurityToken? GetTokenUsedToCallWebAPI()
{
object? o = (HttpContext.Current.User.Identity as ClaimsIdentity)?.BootstrapContext;

if (o != null)
{
// TODO: do better as this won't do for JWE and wastes time. The token was already decrypted.
return new JsonWebToken(o as string);
}
return null;
return HttpContext.Current.User.GetBootstrapToken();
}

public ClaimsPrincipal? GetUserFromRequest()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Extensions;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Microsoft.Identity.Abstractions;
using Microsoft.Identity.Client;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System.Security.Claims;
using System.Security.Principal;
using Microsoft.IdentityModel.JsonWebTokens;
using Microsoft.IdentityModel.Tokens;

namespace Microsoft.Identity.Web
{
/// <summary>
/// Extensions to retrive a <see cref="SecurityToken"/> from <see cref="ClaimsPrincipal"/>.
jmprieur marked this conversation as resolved.
Show resolved Hide resolved
/// </summary>
public static class PrincipalExtensionsForSecurityTokens
{
/// <summary>
/// Get the <see cref="SecurityToken"/> used to call a protected web API.
/// </summary>
/// <param name="claimsPrincipal"></param>
/// <returns></returns>
jmprieur marked this conversation as resolved.
Show resolved Hide resolved
public static SecurityToken? GetBootstrapToken(this IPrincipal claimsPrincipal)
{
object? o = (claimsPrincipal?.Identity as ClaimsIdentity)?.BootstrapContext;
if (o is SecurityToken securityToken)
{
return securityToken;
}
if (o is string s)
{
return new JsonWebToken(s);
}
return null;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ public static IServiceCollection AddTokenAcquisition(
{
_ = Throws.IfNull(services);

#if !NETSTANDARD2_0 && !NET462 && !NET472
MZOLN marked this conversation as resolved.
Show resolved Hide resolved
bool forceSdk = !services.Any(s => s.ServiceType == typeof(AspNetCore.Hosting.IWebHostEnvironment));
#endif

if (services.FirstOrDefault(s => s.ImplementationType == typeof(ICredentialsLoader)) == null)
{
services.AddSingleton<ICredentialsLoader, DefaultCertificateLoader>();
Expand Down Expand Up @@ -88,10 +92,17 @@ public static IServiceCollection AddTokenAcquisition(
#if !NETSTANDARD2_0 && !NET462 && !NET472
// ASP.NET Core
services.AddHttpContextAccessor();
if (forceSdk)
{
services.AddSingleton<ITokenAcquisitionHost, DefaultTokenAcquisitionHost>();
}
else
{
services.AddSingleton<ITokenAcquisitionHost, TokenAcquisitionAspnetCoreHost>();
}
services.AddSingleton<ITokenAcquisition, TokenAcquisitionAspNetCore>();
services.AddSingleton<ITokenAcquirerFactory, DefaultTokenAcquirerFactoryImplementation>();

services.AddSingleton<ITokenAcquisitionHost, TokenAcquisitionAspnetCoreHost>();

#else
// .NET FW.
Expand All @@ -107,10 +118,17 @@ public static IServiceCollection AddTokenAcquisition(
// ASP.NET Core
services.AddHttpContextAccessor();

if (forceSdk)
{
services.AddScoped<ITokenAcquisitionHost, DefaultTokenAcquisitionHost>();
}
else
{
services.AddScoped<ITokenAcquisitionHost, TokenAcquisitionAspnetCoreHost>();
}
services.AddScoped<ITokenAcquisition, TokenAcquisitionAspNetCore>();
services.AddScoped<ITokenAcquirerFactory, DefaultTokenAcquirerFactoryImplementation>();

services.AddScoped<ITokenAcquisitionHost, TokenAcquisitionAspnetCoreHost>();
#else
// .NET FW.
services.AddScoped<ITokenAcquisition, TokenAcquisition>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,9 @@
using System.IdentityModel.Tokens.Jwt;
using System.Linq;
using System.Net.Http;
using System.Runtime.CompilerServices;
using System.Security.Claims;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Identity.Abstractions;
using Microsoft.Identity.Client;
Expand Down Expand Up @@ -245,7 +243,8 @@ public async Task<AuthenticationResult> GetAuthenticationResultForUserAsync(
tenantId,
scopes,
tokenAcquisitionOptions,
mergedOptions).ConfigureAwait(false);
mergedOptions,
user).ConfigureAwait(false);

if (authenticationResult != null)
{
Expand Down Expand Up @@ -658,12 +657,13 @@ private IConfidentialClientApplication BuildConfidentialClientApplication(Merged
string? tenantId,
IEnumerable<string> scopes,
TokenAcquisitionOptions? tokenAcquisitionOptions,
MergedOptions mergedOptions)
MergedOptions mergedOptions,
ClaimsPrincipal? userHint)
{
try
{
// In web API, validatedToken will not be null
SecurityToken? validatedToken = _tokenAcquisitionHost.GetTokenUsedToCallWebAPI();
SecurityToken? validatedToken = userHint?.GetBootstrapToken() ?? _tokenAcquisitionHost.GetTokenUsedToCallWebAPI();

// In the case the token is a JWE (encrypted token), we use the decrypted token.
string? tokenUsedToCallTheWebApi = GetActualToken(validatedToken);
Expand Down Expand Up @@ -889,7 +889,7 @@ private Task<AuthenticationResult> GetAuthenticationResultForWebAppWithAccountFr

if (dict != null)
{
builder.WithExtraQueryParameters(dict);
builder.WithExtraQueryParameters(dict);
}
if (tokenAcquisitionOptions.ExtraHeadersParameters != null)
{
Expand Down Expand Up @@ -937,16 +937,16 @@ private Task<AuthenticationResult> GetAuthenticationResultForWebAppWithAccountFr
{
var mergedDict = new Dictionary<string, string>(tokenAcquisitionOptions.ExtraQueryParameters);
if (mergedOptions.ExtraQueryParameters != null)
{
{
foreach (var pair in mergedOptions!.ExtraQueryParameters)
{
if (!mergedDict!.ContainsKey(pair.Key))
mergedDict.Add(pair.Key, pair.Value);
}
}
}
return mergedDict;
}

return (Dictionary<string, string>?)mergedOptions.ExtraQueryParameters;
}

Expand Down
2 changes: 1 addition & 1 deletion tests/DevApps/aspnet-mvc/OwinWebApi/Web.config
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@
</dependentAssembly>
<dependentAssembly>
<assemblyIdentity name="Microsoft.Identity.Client" publicKeyToken="0A613F4DD989E8AE" culture="neutral"/>
<bindingRedirect oldVersion="0.0.0.0-4.54.1.0" newVersion="4.54.1.0"/>
<bindingRedirect oldVersion="0.0.0.0-4.55.0.0" newVersion="4.55.0.0"/>
</dependentAssembly>
<dependentAssembly>
<assemblyIdentity name="Microsoft.Extensions.Primitives" publicKeyToken="ADB9793829DDAE60" culture="neutral"/>
Expand Down
2 changes: 1 addition & 1 deletion tests/DevApps/aspnet-mvc/OwinWebApp/Web.config
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@
</dependentAssembly>
<dependentAssembly>
<assemblyIdentity name="Microsoft.Identity.Client" publicKeyToken="0A613F4DD989E8AE" culture="neutral"/>
<bindingRedirect oldVersion="0.0.0.0-4.54.1.0" newVersion="4.54.1.0"/>
<bindingRedirect oldVersion="0.0.0.0-4.55.0.0" newVersion="4.55.0.0"/>
</dependentAssembly>
<dependentAssembly>
<assemblyIdentity name="Microsoft.Extensions.Primitives" publicKeyToken="ADB9793829DDAE60" culture="neutral"/>
Expand Down
2 changes: 1 addition & 1 deletion tests/DevApps/daemon-app/Daemon-app/Daemon-app.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFrameworks>net462</TargetFrameworks>
<TargetFrameworks>net7.0</TargetFrameworks>
<RootNamespace>Daemon_app</RootNamespace>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"profiles": {
"WSL": {
"commandName": "WSL2",
"distributionName": ""
}
}
}
11 changes: 10 additions & 1 deletion tests/IntegrationTests/TokenAcquirerTests/TokenAcquirer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,16 @@ public TokenAcquirer()
TokenAcquirerFactory.ResetDefaultInstance(); // Test only
}

[Fact]
public void TokenAcquirerFactoryDoesNotUseAspNetCoreHost()
Copy link

Choose a reason for hiding this comment

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

This test project only tests NET7.0 and NET8.0 it will never test netstandard, which is part of the change.
Would it make sense to add netstandard as part of the target to the test project?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No longer needed with the fix

Copy link

Choose a reason for hiding this comment

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

Well we should still test the whole library for netstandard2.0 dont you think? It`s a release target after all.

{
TokenAcquirerFactory tokenAcquirerFactory = TokenAcquirerFactory.GetDefaultInstance();
var serviceProvider = tokenAcquirerFactory.Build();
var service = serviceProvider.GetService<ITokenAcquisitionHost>();
Assert.NotNull(service);
Assert.Equal("Microsoft.Identity.Web.Hosts.DefaultTokenAcquisitionHost", service.GetType().FullName);
}

[IgnoreOnAzureDevopsFact]
//[Theory]
//[InlineData(false)]
Expand All @@ -43,7 +53,6 @@ public async Task AcquireToken_WithMicrosoftIdentityOptions_ClientCredentialsAsy
TokenAcquirerFactory tokenAcquirerFactory = TokenAcquirerFactory.GetDefaultInstance();
IServiceCollection services = tokenAcquirerFactory.Services;


services.Configure<MicrosoftIdentityOptions>(s_optionName, option =>
{
option.Instance = "https://login.microsoftonline.com/";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
using Xunit.Abstractions;
using Microsoft.Identity.Client.Platforms.Features.DesktopOs.Kerberos;
using System.Threading;
using Microsoft.AspNetCore.Builder;

namespace Microsoft.Identity.Web.Test.Integration
{
Expand Down Expand Up @@ -225,7 +226,8 @@ async Task authResult() =>
[Fact]
public async Task GetAccessTokenForApp_WithAnonymousController_Async()
{
var serviceCollection = new ServiceCollection();
// ASP.NET Core builder.
var serviceCollection = WebApplication.CreateBuilder().Services;
var configuration = new ConfigurationBuilder()
.AddInMemoryCollection(new Dictionary<string, string?>
{
Expand All @@ -243,6 +245,7 @@ public async Task GetAccessTokenForApp_WithAnonymousController_Async()
var services = serviceCollection.BuildServiceProvider();

var tokenAcquisition = services.GetRequiredService<ITokenAcquisition>();
var tokenAcquisitionHost = services.GetRequiredService<ITokenAcquisitionHost>();

var token = await tokenAcquisition.GetAccessTokenForAppAsync("https://graph.microsoft.com/.default").ConfigureAwait(false);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
using Microsoft.IdentityModel.JsonWebTokens;
jmprieur marked this conversation as resolved.
Show resolved Hide resolved
using Microsoft.IdentityModel.Tokens;
using System.IdentityModel.Tokens.Jwt;
using System.Security.Claims;
using System.Security.Principal;
using Xunit;

namespace Microsoft.Identity.Web.Tests
{
public class PrincipalExtensionsForSecurityTokensTests
{
[Fact]
public void GetBootstrapToken_Returns_Null_When_ClaimsPrincipal_Is_Null()
{
// Arrange
IPrincipal claimsPrincipal = null;

// Act
var result = claimsPrincipal.GetBootstrapToken();

jmprieur marked this conversation as resolved.
Show resolved Hide resolved
// Assert
Assert.Null(result);
}

[Fact]
public void GetBootstrapToken_Returns_Null_When_BootstrapContext_Is_Null()
{
// Arrange
IPrincipal claimsPrincipal = new GenericPrincipal(new GenericIdentity(""), null);

// Act
var result = claimsPrincipal.GetBootstrapToken();

// Assert
Assert.Null(result);
}

[Fact]
public void GetBootstrapToken_Returns_SecurityToken_When_BootstrapContext_Is_SecurityToken()
{
// Arrange
var securityToken = new JwtSecurityToken();
IPrincipal claimsPrincipal = new ClaimsPrincipal(new ClaimsIdentity(new[] { new Claim(ClaimTypes.Name, "TestUser") })
{
BootstrapContext = securityToken
});

// Act
var result = claimsPrincipal.GetBootstrapToken();

// Assert
Assert.Equal(securityToken, result);
}

[Fact]
public void GetBootstrapToken_Returns_JsonWebToken_When_BootstrapContext_Is_String()
{
// Arrange
const string jwtString = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c";
IPrincipal claimsPrincipal = new ClaimsPrincipal(new ClaimsIdentity(new[] { new Claim(ClaimTypes.Name, "TestUser") })
{
BootstrapContext = jwtString
});

// Act
var result = claimsPrincipal.GetBootstrapToken() as JsonWebToken;

// Assert
Assert.NotNull(result);
Assert.Equal(jwtString, result.EncodedToken);
}
}
}