Skip to content

Commit

Permalink
Fix L2 logging. Fix L2 empty read. (#2339)
Browse files Browse the repository at this point in the history 
* Fix L2 logging. Fix L2 empty read.

* Add test.

* Add fille to netFx TFM.

* Fix test.

* Minor fixes, PR feedback.
  • Loading branch information
@pmaytak
pmaytak authored Jul 28, 2023
1 parent 68bf457 commit ba21960
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 44 deletions.
6 changes: 4 additions & 2 deletions src/Microsoft.Identity.Web.TokenCache/Distributed/MsalDistributedTokenCacheAdapter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,8 @@ await L2OperationWithRetryOnFailureAsync(
// the parent class
distributedCacheEntryOptions,
cacheSerializerHints?.CancellationToken ?? CancellationToken.None),
cacheKey).Measure().ConfigureAwait(false);
cacheKey,
bytes!).Measure().ConfigureAwait(false);
}
else
{
Expand All @@ -279,7 +280,8 @@ await L2OperationWithRetryOnFailureAsync(
bytes!,
distributedCacheEntryOptions,
cacheSerializerHints?.CancellationToken ?? CancellationToken.None),
cacheKey).Measure().ConfigureAwait(false));
cacheKey,
bytes!).Measure().ConfigureAwait(false));
}
}

Expand Down
15 changes: 8 additions & 7 deletions src/Microsoft.Identity.Web.TokenCache/MsalAbstractTokenCacheProvider.cs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Security.Cryptography;
using System.Threading.Tasks;
using Microsoft.AspNetCore.DataProtection;
Expand Down Expand Up @@ -109,13 +108,12 @@ private async Task OnBeforeAccessAsync(TokenCacheNotificationArgs args)
if (!string.IsNullOrEmpty(GetSuggestedCacheKey(args)))
{
byte[]? tokenCacheBytes = await ReadCacheBytesAsync(GetSuggestedCacheKey(args), CreateHintsFromArgs(args)).ConfigureAwait(false);
if (tokenCacheBytes == null)
{
return;
}

try
{
// Must call Deserialize, even if the L2 read operation returned nothing.
// Deserialize with null value will ensure that the cache in MSAL is properly initialized.
// This will also ensure that the cache in MSAL is cleared if the cache entry in L2 was empty.
args.TokenCache.DeserializeMsalV3(UnprotectBytes(tokenCacheBytes), shouldClearExistingCache: true);
}
catch (MsalClientException exception)
Expand All @@ -139,7 +137,10 @@ private async Task OnBeforeAccessAsync(TokenCacheNotificationArgs args)
}
}

private byte[] UnprotectBytes(byte[]? msalBytes)
// Tries to unprotect the bytes if protection is enabled and the cache is encrypted.
// If the cache is unencrypted, returns the same bytes.
// Returns null, if the bytes are null.
private byte[]? UnprotectBytes(byte[]? msalBytes)
{
if (msalBytes != null && _protector != null)
{
Expand All @@ -154,7 +155,7 @@ private byte[] UnprotectBytes(byte[]? msalBytes)
}
}

return msalBytes!;
return msalBytes;
}

/// <summary>
Expand Down
105 changes: 70 additions & 35 deletions tests/Microsoft.Identity.Web.Test/CacheExtensionsTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Threading.Tasks;
using Microsoft.Extensions.Caching.Distributed;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Identity.Client;
Expand All @@ -14,19 +15,14 @@ namespace Microsoft.Identity.Web.Test
{
public class CacheExtensionsTests
{
#pragma warning disable CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider declaring as nullable.
private IConfidentialClientApplication _confidentialApp;
// Non nullable needed for the Argument null exception tests
#pragma warning restore CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider declaring as nullable.

[Fact]
public void InMemoryCacheExtensionsTests()
{
CreateCca();
_confidentialApp.AddInMemoryTokenCache();
var confidentialApp = CreateCca();
confidentialApp.AddInMemoryTokenCache();

Assert.NotNull(_confidentialApp.UserTokenCache);
Assert.NotNull(_confidentialApp.AppTokenCache);
Assert.NotNull(confidentialApp.UserTokenCache);
Assert.NotNull(confidentialApp.AppTokenCache);
}

[Fact]
Expand All @@ -51,28 +47,30 @@ public async Task CacheExtensions_CcaAlreadyExists_TestsAsync()
[Fact]
public void InMemoryCacheExtensions_NoCca_ThrowsException_Tests()
{
var ex = Assert.Throws<ArgumentNullException>(() => _confidentialApp.AddInMemoryTokenCache());
IConfidentialClientApplication confidentialApp = null!;
var ex = Assert.Throws<ArgumentNullException>(() => confidentialApp.AddInMemoryTokenCache());

Assert.Equal("confidentialClientApp", ex.ParamName);
}

[Fact]
public void InMemoryCache_WithServices_ExtensionsTests()
{
CreateCca();
_confidentialApp.AddInMemoryTokenCache(services =>
var confidentialApp = CreateCca();
confidentialApp.AddInMemoryTokenCache(services =>
{
services.AddMemoryCache();
});

Assert.NotNull(_confidentialApp.UserTokenCache);
Assert.NotNull(_confidentialApp.AppTokenCache);
Assert.NotNull(confidentialApp.UserTokenCache);
Assert.NotNull(confidentialApp.AppTokenCache);
}

[Fact]
public void InMemoryCache_WithServices_NoCca_ThrowsException_Tests()
{
var ex = Assert.Throws<ArgumentNullException>(() => _confidentialApp.AddInMemoryTokenCache(services =>
{
IConfidentialClientApplication confidentialApp = null!;
var ex = Assert.Throws<ArgumentNullException>(() => confidentialApp.AddInMemoryTokenCache(services =>
{
services.AddMemoryCache();
}));
Expand All @@ -83,29 +81,30 @@ public void InMemoryCache_WithServices_NoCca_ThrowsException_Tests()
[Fact]
public void InMemoryCache_WithServices_NoService_ThrowsException_Tests()
{
CreateCca();
var ex = Assert.Throws<ArgumentNullException>(() => _confidentialApp.AddInMemoryTokenCache(null!));
var confidentialApp = CreateCca();
var ex = Assert.Throws<ArgumentNullException>(() => confidentialApp.AddInMemoryTokenCache(null!));

Assert.Equal("initializeMemoryCache", ex.ParamName);
}

[Fact]
public void DistributedCacheExtensionsTests()
{
CreateCca();
_confidentialApp.AddDistributedTokenCache(services =>
var confidentialApp = CreateCca();
confidentialApp.AddDistributedTokenCache(services =>
{
services.AddDistributedMemoryCache();
});

Assert.NotNull(_confidentialApp.UserTokenCache);
Assert.NotNull(_confidentialApp.AppTokenCache);
Assert.NotNull(confidentialApp.UserTokenCache);
Assert.NotNull(confidentialApp.AppTokenCache);
}

[Fact]
public void DistributedCacheExtensions_NoCca_ThrowsException_Tests()
{
var ex = Assert.Throws<ArgumentNullException>(() => _confidentialApp.AddDistributedTokenCache(services =>
{
IConfidentialClientApplication confidentialApp = null!;
var ex = Assert.Throws<ArgumentNullException>(() => confidentialApp.AddDistributedTokenCache(services =>
{
services.AddDistributedMemoryCache();
}));
Expand All @@ -116,12 +115,53 @@ public void DistributedCacheExtensions_NoCca_ThrowsException_Tests()
[Fact]
public void DistributedCacheExtensions_NoService_ThrowsException_Tests()
{
CreateCca();
var ex = Assert.Throws<ArgumentNullException>(() => _confidentialApp.AddDistributedTokenCache(null!));
var confidentialApp = CreateCca();
var ex = Assert.Throws<ArgumentNullException>(() => confidentialApp.AddDistributedTokenCache(null!));

Assert.Equal("initializeDistributedCache", ex.ParamName);
}

}

[Fact]
public async Task SingletonMsal_ResultsInCorrectCacheEntries_Test()
{
var tenantId1 = "tenant1";
var tenantId2 = "tenant2";
var cacheKey1 = $"{TestConstants.ClientId}_{tenantId1}_AppTokenCache";
var cacheKey2 = $"{TestConstants.ClientId}_{tenantId2}_AppTokenCache";

using MockHttpClientFactory mockHttpClient = new MockHttpClientFactory();
using (mockHttpClient.AddMockHandler(MockHttpCreator.CreateClientCredentialTokenHandler()))
using (mockHttpClient.AddMockHandler(MockHttpCreator.CreateClientCredentialTokenHandler()))
{
var confidentialApp = ConfidentialClientApplicationBuilder
.Create(TestConstants.ClientId)
.WithAuthority(TestConstants.AuthorityCommonTenant)
.WithHttpClientFactory(mockHttpClient)
.WithInstanceDiscovery(false)
.WithClientSecret(TestConstants.ClientSecret)
.Build();

var distributedCache = new TestDistributedCache();
confidentialApp.AddDistributedTokenCache(services =>
{
services.AddSingleton<IDistributedCache>(distributedCache);
});

// Different tenants used to created different cache entries
var result1 = await confidentialApp.AcquireTokenForClient(new[] { TestConstants.s_scopeForApp })
.WithTenantId(tenantId1)
.ExecuteAsync().ConfigureAwait(false);
var result2 = await confidentialApp.AcquireTokenForClient(new[] { TestConstants.s_scopeForApp })
.WithTenantId(tenantId2)
.ExecuteAsync().ConfigureAwait(false);

Assert.Equal(TokenSource.IdentityProvider, result1.AuthenticationResultMetadata.TokenSource);
Assert.Equal(TokenSource.IdentityProvider, result2.AuthenticationResultMetadata.TokenSource);
Assert.Equal(2, distributedCache._dict.Count);
Assert.Equal(distributedCache.Get(cacheKey1)!.Length, distributedCache.Get(cacheKey2)!.Length);
}
}

private enum CacheType
{
InMemory,
Expand Down Expand Up @@ -176,16 +216,11 @@ private static async Task<AuthenticationResult> CreateAppAndGetTokenAsync(
return result;
}

private void CreateCca()
{
if (_confidentialApp == null)
{
_confidentialApp = ConfidentialClientApplicationBuilder
private IConfidentialClientApplication CreateCca() =>
ConfidentialClientApplicationBuilder
.Create(TestConstants.ClientId)
.WithAuthority(TestConstants.AuthorityCommonTenant)
.WithClientSecret(TestConstants.ClientSecret)
.Build();
}
}
}
}
1 change: 1 addition & 0 deletions tests/Microsoft.Identity.Web.Test/Microsoft.Identity.Web.Test.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
<Compile Include="Certificates\DefaultCertificateLoaderTests.cs" />
<Compile Include="CacheExtensionsTests.cs" />
<Compile Include="CacheEncryptionTests.cs" />
<Compile Include="TestDistributedCache.cs" />
</ItemGroup>

<ItemGroup>
Expand Down

0 comments on commit ba21960

Please sign in to comment.