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

Fix L2 logging. Fix L2 empty read. #2339

Merged
merged 6 commits into from
Jul 28, 2023
Merged

Fix L2 logging. Fix L2 empty read. #2339

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a3d3443
Fix L2 logging. Fix L2 empty read.
pmaytak Jul 22, 2023
9978dbd
Add test.
pmaytak Jul 27, 2023
099646a
Merge remote-tracking branch 'origin/master' into pmaytak/l2-cache-fix
pmaytak Jul 27, 2023
1a58d60
Add fille to netFx TFM.
pmaytak Jul 27, 2023
822cd23
Fix test.
pmaytak Jul 27, 2023
de77791
Minor fixes, PR feedback.
pmaytak Jul 28, 2023
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
6 changes: 4 additions & 2 deletions src/Microsoft.Identity.Web.TokenCache/Distributed/MsalDistributedTokenCacheAdapter.cs
View file
Open in desktop
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
9 changes: 2 additions & 7 deletions src/Microsoft.Identity.Web.TokenCache/MsalAbstractTokenCacheProvider.cs
View file
Open in desktop
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,10 +108,6 @@ 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
{
pmaytak marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -139,7 +134,7 @@ private async Task OnBeforeAccessAsync(TokenCacheNotificationArgs args)
}
}

private byte[] UnprotectBytes(byte[]? msalBytes)
private byte[]? UnprotectBytes(byte[]? msalBytes)
pmaytak marked this conversation as resolved.
Show resolved Hide resolved
{
if (msalBytes != null && _protector != null)
{
Expand All @@ -154,7 +149,7 @@ private byte[] UnprotectBytes(byte[]? msalBytes)
}
}

return msalBytes!;
return msalBytes;
}

/// <summary>
Expand Down
100 changes: 65 additions & 35 deletions tests/Microsoft.Identity.Web.Test/CacheExtensionsTests.cs
View file
Open in desktop
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,48 @@ 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()
{
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("tenant1")
pmaytak marked this conversation as resolved.
Show resolved Hide resolved
.ExecuteAsync().ConfigureAwait(false);
var result2 = await confidentialApp.AcquireTokenForClient(new[] { TestConstants.s_scopeForApp })
.WithTenantId("tenant2")
.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($"{TestConstants.ClientId}_tenant1_AppTokenCache")!.Length, distributedCache.Get($"{TestConstants.ClientId}_tenant2_AppTokenCache")!.Length);
pmaytak marked this conversation as resolved.
Show resolved Hide resolved
}
}

private enum CacheType
{
InMemory,
Expand Down Expand Up @@ -176,16 +211,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
Open in desktop
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
Loading