From a239329a848d28282b4432c2bd7931ebe91a548b Mon Sep 17 00:00:00 2001 From: jennyf19 Date: Mon, 12 Oct 2020 10:22:11 -0700 Subject: [PATCH] make aadissuervalidator a singleton (#675) --- .../AuthorizeForScopesAttribute.cs | 1 + .../Microsoft.Identity.Web.xml | 21 ++++--- .../Resource/AadIssuerValidator.cs | 46 +-------------- ...MicrosoftIdentityIssuerValidatorFactory.cs | 57 +++++++++++++++++++ ...tyWebApiAuthenticationBuilderExtensions.cs | 7 ++- ...tyWebAppAuthenticationBuilderExtensions.cs | 8 ++- .../Resource/AadIssuerValidatorTests.cs | 33 ++++++----- .../WebApiExtensionsTests.cs | 2 + .../WebAppExtensionsTests.cs | 4 ++ 9 files changed, 111 insertions(+), 68 deletions(-) create mode 100644 src/Microsoft.Identity.Web/Resource/MicrosoftIdentityIssuerValidatorFactory.cs diff --git a/src/Microsoft.Identity.Web/AuthorizeForScopesAttribute.cs b/src/Microsoft.Identity.Web/AuthorizeForScopesAttribute.cs index ed706e240..5ce3694df 100644 --- a/src/Microsoft.Identity.Web/AuthorizeForScopesAttribute.cs +++ b/src/Microsoft.Identity.Web/AuthorizeForScopesAttribute.cs @@ -136,6 +136,7 @@ public override void OnException(ExceptionContext context) return null; } } + private static bool IsAjaxRequest(HttpRequest request) { return string.Equals(request.Query[Constants.XRequestedWith], Constants.XmlHttpRequest, StringComparison.Ordinal) || diff --git a/src/Microsoft.Identity.Web/Microsoft.Identity.Web.xml b/src/Microsoft.Identity.Web/Microsoft.Identity.Web.xml index c1c9f48cf..b76503ca2 100644 --- a/src/Microsoft.Identity.Web/Microsoft.Identity.Web.xml +++ b/src/Microsoft.Identity.Web/Microsoft.Identity.Web.xml @@ -1203,14 +1203,6 @@ A list of all Issuers across the various Azure AD instances. - - - Gets an for an authority. - - The authority to create the validator for, e.g. https://login.microsoftonline.com/. - A for the aadAuthority. - if is null or empty. - Validate the issuer for multi-tenant applications of various audiences (Work and School accounts, or Work and School accounts + @@ -1298,6 +1290,19 @@ Events to subscribe to. for chaining. + + + Factory class for creating the IssuerValidator per authority. + + + + + Gets an for an authority. + + The authority to create the validator for, e.g. https://login.microsoftonline.com/. + A for the aadAuthority. + if is null or empty. + Diagnostics used in the OpenID Connect middleware diff --git a/src/Microsoft.Identity.Web/Resource/AadIssuerValidator.cs b/src/Microsoft.Identity.Web/Resource/AadIssuerValidator.cs index a92078584..f886834b0 100644 --- a/src/Microsoft.Identity.Web/Resource/AadIssuerValidator.cs +++ b/src/Microsoft.Identity.Web/Resource/AadIssuerValidator.cs @@ -2,14 +2,10 @@ // Licensed under the MIT License. using System; -using System.Collections.Concurrent; using System.Collections.Generic; using System.Globalization; using System.IdentityModel.Tokens.Jwt; -using System.Linq; -using Microsoft.Identity.Web.InstanceDiscovery; using Microsoft.IdentityModel.JsonWebTokens; -using Microsoft.IdentityModel.Protocols; using Microsoft.IdentityModel.Tokens; namespace Microsoft.Identity.Web.Resource @@ -19,56 +15,16 @@ namespace Microsoft.Identity.Web.Resource /// public class AadIssuerValidator { - // TODO: separate AadIssuerValidator creation logic from the validation logic in order to unit test it - private static readonly IDictionary s_issuerValidators = new ConcurrentDictionary(); - - private static readonly ConfigurationManager s_configManager = new ConfigurationManager(Constants.AzureADIssuerMetadataUrl, new IssuerConfigurationRetriever()); - /// /// A list of all Issuers across the various Azure AD instances. /// private readonly ISet _issuerAliases; - internal /* internal for test */ AadIssuerValidator(IEnumerable aliases) + internal /*internal for tests*/ AadIssuerValidator(IEnumerable aliases) { _issuerAliases = new HashSet(aliases, StringComparer.OrdinalIgnoreCase); } - /// - /// Gets an for an authority. - /// - /// The authority to create the validator for, e.g. https://login.microsoftonline.com/. - /// A for the aadAuthority. - /// if is null or empty. - public static AadIssuerValidator GetIssuerValidator(string aadAuthority) - { - if (string.IsNullOrEmpty(aadAuthority)) - { - throw new ArgumentNullException(nameof(aadAuthority)); - } - - Uri.TryCreate(aadAuthority, UriKind.Absolute, out Uri? authorityUri); - string authorityHost = authorityUri?.Authority ?? new Uri(Constants.FallbackAuthority).Authority; - - if (s_issuerValidators.TryGetValue(authorityHost, out AadIssuerValidator? aadIssuerValidator)) - { - return aadIssuerValidator; - } - - // In the constructor, we hit the Azure AD issuer metadata endpoint and cache the aliases. The data is cached for 24 hrs. - IssuerMetadata issuerMetadata = s_configManager.GetConfigurationAsync().ConfigureAwait(false).GetAwaiter().GetResult(); - - // Add issuer aliases of the chosen authority to the cache - IEnumerable aliases = issuerMetadata.Metadata - .Where(m => m.Aliases.Any(a => string.Equals(a, authorityHost, StringComparison.OrdinalIgnoreCase))) - .SelectMany(m => m.Aliases) - .Append(authorityHost) // For B2C scenarios, the alias will be the authority itself - .Distinct(); - s_issuerValidators[authorityHost] = new AadIssuerValidator(aliases); - - return s_issuerValidators[authorityHost]; - } - /// /// Validate the issuer for multi-tenant applications of various audiences (Work and School accounts, or Work and School accounts + /// Personal accounts). diff --git a/src/Microsoft.Identity.Web/Resource/MicrosoftIdentityIssuerValidatorFactory.cs b/src/Microsoft.Identity.Web/Resource/MicrosoftIdentityIssuerValidatorFactory.cs new file mode 100644 index 000000000..e5374cad3 --- /dev/null +++ b/src/Microsoft.Identity.Web/Resource/MicrosoftIdentityIssuerValidatorFactory.cs @@ -0,0 +1,57 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using Microsoft.Identity.Web.InstanceDiscovery; +using Microsoft.IdentityModel.Protocols; + +namespace Microsoft.Identity.Web.Resource +{ + /// + /// Factory class for creating the IssuerValidator per authority. + /// + internal class MicrosoftIdentityIssuerValidatorFactory + { + private readonly IDictionary _issuerValidators = new ConcurrentDictionary(); + + private readonly ConfigurationManager _configManager = new ConfigurationManager(Constants.AzureADIssuerMetadataUrl, new IssuerConfigurationRetriever()); + + /// + /// Gets an for an authority. + /// + /// The authority to create the validator for, e.g. https://login.microsoftonline.com/. + /// A for the aadAuthority. + /// if is null or empty. + public AadIssuerValidator GetAadIssuerValidator(string aadAuthority) + { + if (string.IsNullOrEmpty(aadAuthority)) + { + throw new ArgumentNullException(nameof(aadAuthority)); + } + + Uri.TryCreate(aadAuthority, UriKind.Absolute, out Uri? authorityUri); + string authorityHost = authorityUri?.Authority ?? new Uri(Constants.FallbackAuthority).Authority; + + if (_issuerValidators.TryGetValue(authorityHost, out AadIssuerValidator? aadIssuerValidator)) + { + return aadIssuerValidator; + } + + // In the constructor, we hit the Azure AD issuer metadata endpoint and cache the aliases. The data is cached for 24 hrs. + IssuerMetadata issuerMetadata = _configManager.GetConfigurationAsync().ConfigureAwait(false).GetAwaiter().GetResult(); + + // Add issuer aliases of the chosen authority to the cache + IEnumerable aliases = issuerMetadata.Metadata + .Where(m => m.Aliases.Any(a => string.Equals(a, authorityHost, StringComparison.OrdinalIgnoreCase))) + .SelectMany(m => m.Aliases) + .Append(authorityHost) // For B2C scenarios, the alias will be the authority itself + .Distinct(); + _issuerValidators[authorityHost] = new AadIssuerValidator(aliases); + + return _issuerValidators[authorityHost]; + } + } +} diff --git a/src/Microsoft.Identity.Web/WebApiExtensions/MicrosoftIdentityWebApiAuthenticationBuilderExtensions.cs b/src/Microsoft.Identity.Web/WebApiExtensions/MicrosoftIdentityWebApiAuthenticationBuilderExtensions.cs index 1d67a6a7b..c5752d6d4 100644 --- a/src/Microsoft.Identity.Web/WebApiExtensions/MicrosoftIdentityWebApiAuthenticationBuilderExtensions.cs +++ b/src/Microsoft.Identity.Web/WebApiExtensions/MicrosoftIdentityWebApiAuthenticationBuilderExtensions.cs @@ -159,6 +159,7 @@ private static void AddMicrosoftIdentityWebApiImplementation( builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton, MicrosoftIdentityOptionsValidation>()); builder.Services.AddHttpContextAccessor(); builder.Services.AddHttpClient(); + builder.Services.TryAddSingleton(); if (subscribeToJwtBearerMiddlewareDiagnosticsEvents) { @@ -194,7 +195,11 @@ private static void AddMicrosoftIdentityWebApiImplementation( { // Instead of using the default validation (validating against a single tenant, as we do in line of business apps), // we inject our own multi-tenant validation logic (which even accepts both v1.0 and v2.0 tokens) - options.TokenValidationParameters.IssuerValidator = AadIssuerValidator.GetIssuerValidator(options.Authority).Validate; + MicrosoftIdentityIssuerValidatorFactory microsoftIdentityIssuerValidatorFactory = + serviceProvider.GetRequiredService(); + + options.TokenValidationParameters.IssuerValidator = + microsoftIdentityIssuerValidatorFactory.GetAadIssuerValidator(options.Authority).Validate; } // If you provide a token decryption certificate, it will be used to decrypt the token diff --git a/src/Microsoft.Identity.Web/WebAppExtensions/MicrosoftIdentityWebAppAuthenticationBuilderExtensions.cs b/src/Microsoft.Identity.Web/WebAppExtensions/MicrosoftIdentityWebAppAuthenticationBuilderExtensions.cs index 16f94eb62..3e11efdf2 100644 --- a/src/Microsoft.Identity.Web/WebAppExtensions/MicrosoftIdentityWebAppAuthenticationBuilderExtensions.cs +++ b/src/Microsoft.Identity.Web/WebAppExtensions/MicrosoftIdentityWebAppAuthenticationBuilderExtensions.cs @@ -229,6 +229,8 @@ private static void AddMicrosoftIdentityWebAppInternal( builder.AddCookie(cookieScheme, configureCookieAuthenticationOptions); + builder.Services.TryAddSingleton(); + if (subscribeToOpenIdConnectMiddlewareDiagnosticsEvents) { builder.Services.AddSingleton(); @@ -267,7 +269,11 @@ private static void AddMicrosoftIdentityWebAppInternal( // If you want to restrict the users that can sign-in to several organizations // Set the tenant value in the appsettings.json file to 'organizations', and add the // issuers you want to accept to options.TokenValidationParameters.ValidIssuers collection - options.TokenValidationParameters.IssuerValidator = AadIssuerValidator.GetIssuerValidator(options.Authority).Validate; + MicrosoftIdentityIssuerValidatorFactory microsoftIdentityIssuerValidatorFactory = + serviceProvider.GetRequiredService(); + + options.TokenValidationParameters.IssuerValidator = + microsoftIdentityIssuerValidatorFactory.GetAadIssuerValidator(options.Authority).Validate; } // Avoids having users being presented the select account dialog when they are already signed-in diff --git a/tests/Microsoft.Identity.Web.Test/Resource/AadIssuerValidatorTests.cs b/tests/Microsoft.Identity.Web.Test/Resource/AadIssuerValidatorTests.cs index abdb1db14..b7d63dc3a 100644 --- a/tests/Microsoft.Identity.Web.Test/Resource/AadIssuerValidatorTests.cs +++ b/tests/Microsoft.Identity.Web.Test/Resource/AadIssuerValidatorTests.cs @@ -16,18 +16,25 @@ namespace Microsoft.Identity.Web.Test.Resource { public class AadIssuerValidatorTests { + private readonly MicrosoftIdentityIssuerValidatorFactory _issuerValidatorFactory; + + public AadIssuerValidatorTests() + { + _issuerValidatorFactory = new MicrosoftIdentityIssuerValidatorFactory(); + } + [Fact] public void GetIssuerValidator_NullOrEmptyAuthority_ThrowsException() { - var exception = Assert.Throws(TestConstants.AadAuthority, () => AadIssuerValidator.GetIssuerValidator(string.Empty)); + var exception = Assert.Throws(TestConstants.AadAuthority, () => _issuerValidatorFactory.GetAadIssuerValidator(string.Empty)); - exception = Assert.Throws(TestConstants.AadAuthority, () => AadIssuerValidator.GetIssuerValidator(null)); + exception = Assert.Throws(TestConstants.AadAuthority, () => _issuerValidatorFactory.GetAadIssuerValidator(null)); } [Fact] public void GetIssuerValidator_InvalidAuthority_ReturnsValidatorBasedOnFallbackAuthority() { - var validator = AadIssuerValidator.GetIssuerValidator(TestConstants.InvalidAuthorityFormat); + var validator = _issuerValidatorFactory.GetAadIssuerValidator(TestConstants.InvalidAuthorityFormat); Assert.NotNull(validator); } @@ -37,7 +44,7 @@ public void GetIssuerValidator_AuthorityInAliases_ReturnsValidator() { var authorityInAliases = TestConstants.AuthorityCommonTenantWithV2; - var validator = AadIssuerValidator.GetIssuerValidator(authorityInAliases); + var validator = _issuerValidatorFactory.GetAadIssuerValidator(authorityInAliases); Assert.NotNull(validator); } @@ -47,7 +54,7 @@ public void GetIssuerValidator_B2cAuthorityNotInAliases_ReturnsValidator() { var authorityNotInAliases = TestConstants.B2CAuthorityWithV2; - var validator = AadIssuerValidator.GetIssuerValidator(authorityNotInAliases); + var validator = _issuerValidatorFactory.GetAadIssuerValidator(authorityNotInAliases); Assert.NotNull(validator); } @@ -57,8 +64,8 @@ public void GetIssuerValidator_CachedAuthority_ReturnsCachedValidator() { var authorityNotInAliases = TestConstants.AuthorityWithTenantSpecifiedWithV2; - var validator1 = AadIssuerValidator.GetIssuerValidator(authorityNotInAliases); - var validator2 = AadIssuerValidator.GetIssuerValidator(authorityNotInAliases); + var validator1 = _issuerValidatorFactory.GetAadIssuerValidator(authorityNotInAliases); + var validator2 = _issuerValidatorFactory.GetAadIssuerValidator(authorityNotInAliases); Assert.Same(validator1, validator2); } @@ -250,7 +257,7 @@ public void Validate_FromB2CAuthority_WithNoTidClaim_ValidateSuccessfully() Claim tfpClaim = new Claim(TestConstants.ClaimNameTfp, TestConstants.B2CSignUpSignInUserFlow); JwtSecurityToken jwtSecurityToken = new JwtSecurityToken(issuer: TestConstants.B2CIssuer, claims: new[] { issClaim, tfpClaim }); - AadIssuerValidator validator = AadIssuerValidator.GetIssuerValidator(TestConstants.B2CAuthorityWithV2); + AadIssuerValidator validator = _issuerValidatorFactory.GetAadIssuerValidator(TestConstants.B2CAuthorityWithV2); validator.Validate( TestConstants.B2CIssuer, @@ -271,7 +278,7 @@ public void Validate_FromB2CAuthority_WithTidClaim_ValidateSuccessfully() Claim tidClaim = new Claim(TestConstants.ClaimNameTid, TestConstants.B2CTenantAsGuid); JwtSecurityToken jwtSecurityToken = new JwtSecurityToken(issuer: TestConstants.B2CIssuer, claims: new[] { issClaim, tfpClaim, tidClaim }); - AadIssuerValidator validator = AadIssuerValidator.GetIssuerValidator(TestConstants.B2CAuthorityWithV2); + AadIssuerValidator validator = _issuerValidatorFactory.GetAadIssuerValidator(TestConstants.B2CAuthorityWithV2); validator.Validate( TestConstants.B2CIssuer, @@ -291,7 +298,7 @@ public void Validate_FromB2CAuthority_InvalidIssuer_Fails() Claim tfpClaim = new Claim(TestConstants.ClaimNameTfp, TestConstants.B2CSignUpSignInUserFlow); JwtSecurityToken jwtSecurityToken = new JwtSecurityToken(issuer: TestConstants.B2CIssuer2, claims: new[] { issClaim, tfpClaim }); - AadIssuerValidator validator = AadIssuerValidator.GetIssuerValidator(TestConstants.B2CAuthorityWithV2); + AadIssuerValidator validator = _issuerValidatorFactory.GetAadIssuerValidator(TestConstants.B2CAuthorityWithV2); Assert.Throws(() => validator.Validate( @@ -313,7 +320,7 @@ public void Validate_FromB2CAuthority_InvalidIssuerTid_Fails() Claim tfpClaim = new Claim(TestConstants.ClaimNameTfp, TestConstants.B2CSignUpSignInUserFlow); JwtSecurityToken jwtSecurityToken = new JwtSecurityToken(issuer: issuerWithInvalidTid, claims: new[] { issClaim, tfpClaim }); - AadIssuerValidator validator = AadIssuerValidator.GetIssuerValidator(TestConstants.B2CAuthorityWithV2); + AadIssuerValidator validator = _issuerValidatorFactory.GetAadIssuerValidator(TestConstants.B2CAuthorityWithV2); Assert.Throws(() => validator.Validate( @@ -334,7 +341,7 @@ public void Validate_FromCustomB2CAuthority_ValidateSuccessfully() Claim tfpClaim = new Claim(TestConstants.ClaimNameTfp, TestConstants.B2CSignUpSignInUserFlow); JwtSecurityToken jwtSecurityToken = new JwtSecurityToken(issuer: TestConstants.B2CCustomDomainIssuer, claims: new[] { issClaim, tfpClaim }); - AadIssuerValidator validator = AadIssuerValidator.GetIssuerValidator(TestConstants.B2CCustomDomainAuthorityWithV2); + AadIssuerValidator validator = _issuerValidatorFactory.GetAadIssuerValidator(TestConstants.B2CCustomDomainAuthorityWithV2); validator.Validate( TestConstants.B2CCustomDomainIssuer, @@ -351,7 +358,7 @@ public void Validate_FromB2CAuthority_WithTfpIssuer_ThrowsException() Claim issClaim = new Claim(TestConstants.ClaimNameIss, TestConstants.B2CIssuerTfp); JwtSecurityToken jwtSecurityToken = new JwtSecurityToken(issuer: TestConstants.B2CIssuerTfp, claims: new[] { issClaim }); - AadIssuerValidator validator = AadIssuerValidator.GetIssuerValidator(TestConstants.B2CAuthorityWithV2); + AadIssuerValidator validator = _issuerValidatorFactory.GetAadIssuerValidator(TestConstants.B2CAuthorityWithV2); var exception = Assert.Throws(() => validator.Validate( diff --git a/tests/Microsoft.Identity.Web.Test/WebApiExtensionsTests.cs b/tests/Microsoft.Identity.Web.Test/WebApiExtensionsTests.cs index 187067a28..88056cbc6 100644 --- a/tests/Microsoft.Identity.Web.Test/WebApiExtensionsTests.cs +++ b/tests/Microsoft.Identity.Web.Test/WebApiExtensionsTests.cs @@ -124,6 +124,7 @@ private void AddMicrosoftIdentityWebApi_TestCommon(IServiceCollection services, Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); Assert.Contains(services, s => s.ServiceType == typeof(IJwtBearerMiddlewareDiagnostics)); + Assert.Equal(ServiceLifetime.Singleton, services.First(s => s.ServiceType == typeof(MicrosoftIdentityIssuerValidatorFactory)).Lifetime); Assert.Equal(ServiceLifetime.Singleton, services.First(s => s.ServiceType == typeof(IJwtBearerMiddlewareDiagnostics)).Lifetime); // JWT options added correctly @@ -376,6 +377,7 @@ private async Task AddMicrosoftIdentityWebApiCallsWebApi_TestCommon(IServiceColl Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); + Assert.Equal(ServiceLifetime.Singleton, services.First(s => s.ServiceType == typeof(MicrosoftIdentityIssuerValidatorFactory)).Lifetime); // Assert token validated event added correctly var jwtOptions = provider.GetRequiredService>().Create(JwtBearerScheme); diff --git a/tests/Microsoft.Identity.Web.Test/WebAppExtensionsTests.cs b/tests/Microsoft.Identity.Web.Test/WebAppExtensionsTests.cs index 999c98e54..dc2c97a8b 100644 --- a/tests/Microsoft.Identity.Web.Test/WebAppExtensionsTests.cs +++ b/tests/Microsoft.Identity.Web.Test/WebAppExtensionsTests.cs @@ -427,6 +427,7 @@ public void AddMicrosoftGraphOptionsTest(bool useMSGraphOptions) Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); + Assert.Equal(ServiceLifetime.Singleton, services.First(s => s.ServiceType == typeof(MicrosoftIdentityIssuerValidatorFactory)).Lifetime); // Assert properties set var msGraphOptions = provider.GetRequiredService>(); @@ -481,6 +482,7 @@ public void AddDownstreamWebApiOptionsTest(bool useDownstreamWebApiOptions) Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); + Assert.Equal(ServiceLifetime.Singleton, services.First(s => s.ServiceType == typeof(MicrosoftIdentityIssuerValidatorFactory)).Lifetime); // Assert properties set var downstreamWebApiOptions = provider.GetRequiredService>(); @@ -506,6 +508,7 @@ private void AddMicrosoftIdentityWebApp_TestCommon(IServiceCollection services, Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); Assert.Contains(services, s => s.ServiceType == typeof(IPostConfigureOptions)); + Assert.Equal(ServiceLifetime.Singleton, services.First(s => s.ServiceType == typeof(MicrosoftIdentityIssuerValidatorFactory)).Lifetime); // Assert properties set var oidcOptions = provider.GetRequiredService>().Create(OidcScheme); @@ -600,6 +603,7 @@ private void AddMicrosoftIdentityWebAppCallsWebApi_TestCommon(IServiceCollection Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); Assert.Contains(services, s => s.ServiceType == typeof(IConfigureOptions)); + Assert.Equal(ServiceLifetime.Singleton, services.First(s => s.ServiceType == typeof(MicrosoftIdentityIssuerValidatorFactory)).Lifetime); // Assert OIDC options added correctly var configuredOidcOptions = provider.GetService>() as ConfigureNamedOptions;