Skip to content

Isolate configured token cache providers - #4014

Draft
Ignacio Inglese (iNinja) wants to merge 3 commits into
masterfrom
iinglese/token-cache-provider-isolation-minimal
Draft

Ignacio Inglese (iNinja) wants to merge 3 commits into
masterfrom
iinglese/token-cache-provider-isolation-minimal

Conversation

@iNinja

Copy link
Copy Markdown
Contributor

Summary

  • Use a distinct cache provider for each configured token-cache registration.
  • Preserve existing sharing for parameterless in-memory token-cache registration.

Release notes

Configured token-cache registrations no longer share an implicitly cached provider.
Parameterless in-memory token-cache registration continues to share its provider.

Testing

  • Focused tests passed on .NET 8 and .NET Framework 4.7.2.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Isolates configured token-cache registrations while preserving shared caching for parameterless in-memory registration.

Changes:

  • Builds a distinct service provider for each configured cache.
  • Retains a shared lazy provider for parameterless in-memory caches.
  • Adds tests covering provider isolation, backend isolation, options, and initialization.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/Microsoft.Identity.Web.TokenCache/TokenCacheExtensions.cs Implements configured-cache isolation and parameterless sharing.
tests/Microsoft.Identity.Web.Test/CacheExtensionsTests.cs Verifies the updated cache registration behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@bgavrilMS Bogdan Gavril (bgavrilMS) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still loking at this.

@bgavrilMS Bogdan Gavril (bgavrilMS) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As per my convo with GPT 6 Astra, this breaks the following scneario

async Task<string> GetTokenAsync()
{
    var app = ConfidentialClientApplicationBuilder
        .Create(clientId)
        .WithAuthority(authority)
        .WithClientSecret(clientSecret)
        .Build();

    app.AddInMemoryTokenCache(services =>
    {
        services.Configure<MemoryCacheOptions>(options =>
        {
            options.SizeLimit = 100 * 1024 * 1024;
        });
    });

    var result = await app
        .AcquireTokenForClient(new[] { "https://graph.microsoft.com/.default" })
        .ExecuteAsync();

    return result.AccessToken;
}

await GetTokenAsync();
await GetTokenAsync();  // expected: from cache actual: from STS

@bgavrilMS

Copy link
Copy Markdown
Member

The configuration bug is real, but this change also removes cross-CCA cache reuse for the configured standalone extensions. This is more than provider isolation.

For example, an application can repeatedly build a CCA and attach the same configured cache:

var app = ConfidentialClientApplicationBuilder
    .Create(clientId)
    .WithAuthority(authority)
    .WithClientSecret(clientSecret)
    .Build();

app.AddInMemoryTokenCache(services =>
{
    services.Configure<MemoryCacheOptions>(options =>
    {
        options.SizeLimit = 100 * 1024 * 1024;
    });
});

var result = await app.AcquireTokenForClient(scopes).ExecuteAsync();

Before this change, successive CCAs using that overload reused the backing memory cache. After this change, each registration builds a new service provider and therefore a new memory cache, even with identical configuration. Repeating this pattern loses cross-CCA token reuse and causes additional token-endpoint requests.

For AddDistributedTokenCache(configure), each new adapter constructs its own L1 MemoryCache. A shared Redis backend still allows L2 token reuse, but repeated CCA construction now produces repeated cold-L1 reads and independent memory budgets. With AddDistributedMemoryCache(), the default L2 storage is also separate.

Scope clarification: normal Id.Web DI registrations and internally created CCAs remain shared, as does parameterless AddInMemoryTokenCache(). The regression is in repeated use of the configured standalone extensions.

The previous tests exercised distributed L1 and L2 reuse across recreated CCAs; replacing those checks with isolation checks removes coverage of the behavior we need to preserve.

Please preserve reuse for repeated registrations of the same logical cache while fixing configuration isolation. An explicit cache identity or reusable registration could distinguish genuinely different caches without making CCA creation the cache-lifetime boundary. Arbitrary Action<IServiceCollection> callbacks cannot reliably be compared for configuration equivalence, so this needs an explicit ownership/configuration contract rather than method-based deduplication or unconditional isolation.

Regression coverage should include two CCAs with identical client/authority/scopes and the same logical cache: a second-call cache hit for configured in-memory caching, an L1 hit for distributed caching with L1 enabled, and an L2 hit with L1 disabled. Separate logical caches should independently honor their configurations/backends.

@iNinja
Ignacio Inglese (iNinja) marked this pull request as draft September 14, 2026 14:35

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants