From ebf4b96789237a1b83d2fd54008f37af9d222873 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 09:17:52 -0400 Subject: [PATCH] fix(notification-service): evict stale OAuth2 token cache entries after credential rotation (review 2026-10-21 06/MEM-3) --- .../Component-NotificationService.md | 2 +- .../OAuth2TokenService.cs | 60 +++++++++++++++++- .../OAuth2TokenServiceTests.cs | 61 +++++++++++++++++++ 3 files changed, 119 insertions(+), 4 deletions(-) diff --git a/docs/requirements/Component-NotificationService.md b/docs/requirements/Component-NotificationService.md index b3a138e5..80ca9178 100644 --- a/docs/requirements/Component-NotificationService.md +++ b/docs/requirements/Component-NotificationService.md @@ -45,7 +45,7 @@ The SMTP configuration is defined centrally and used by the central Email delive - **Port**: SMTP port (e.g., 587 for StartTLS, 465 for SSL). - **Authentication mode**: One of: - **Basic Auth**: Username and password. For on-prem SMTP relays or servers that support basic authentication. - - **OAuth2 Client Credentials**: Tenant ID, Client ID, and Client Secret. For Microsoft 365 and other modern SMTP providers that require OAuth2. The Email adapter handles the token lifecycle internally (fetch, cache, refresh on expiry). + - **OAuth2 Client Credentials**: Tenant ID, Client ID, and Client Secret. For Microsoft 365 and other modern SMTP providers that require OAuth2. The Email adapter handles the token lifecycle internally (fetch, cache, refresh on expiry). The token cache (`OAuth2TokenService`, a singleton) is keyed per credential/authority/scope; an entry idle for more than an hour whose token has expired (or was never acquired) is evicted on the next token request, so a credential rotation does not leave the old credential cached for the life of the process. - **OAuth2 authority** (optional): The token-endpoint URL requested during the client-credentials grant. When omitted it defaults to the Microsoft 365 endpoint `https://login.microsoftonline.com/{tenant}/oauth2/v2.0/token`, with the `{tenant}` placeholder substituted from the credential's tenant id; set it explicitly to target another identity provider (e.g. a non-M365 OAuth2 SMTP provider). - **OAuth2 scope** (optional): The scope requested from the token endpoint. When omitted it defaults to the Microsoft 365 scope `https://outlook.office365.com/.default`; set it explicitly for another provider. Both fields are managed by Admin-role users via the CLI (`notification smtp update --oauth2-authority`/`--oauth2-scope`) and the Central UI `/notifications/smtp` page (the two inputs appear only for the OAuth2 auth type); a blank value is stored as null so the M365 default applies. Together they make "and other modern SMTP providers" true — a non-Microsoft OAuth2 relay is now configurable without a code change. - **TLS mode**: None, StartTLS, or SSL. diff --git a/src/ZB.MOM.WW.ScadaBridge.NotificationService/OAuth2TokenService.cs b/src/ZB.MOM.WW.ScadaBridge.NotificationService/OAuth2TokenService.cs index d9976333..ebf0c1ed 100644 --- a/src/ZB.MOM.WW.ScadaBridge.NotificationService/OAuth2TokenService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.NotificationService/OAuth2TokenService.cs @@ -15,8 +15,17 @@ namespace ZB.MOM.WW.ScadaBridge.NotificationService; /// public class OAuth2TokenService { + /// + /// How long an entry must have gone unused — with its token expired or never acquired — + /// before the next token request evicts it. A credential rotation otherwise leaves the + /// old credential's entry (and its semaphore) cached for the life of the process + /// (review 2026-10-21 06/MEM-3). + /// + internal static readonly TimeSpan StaleEntryGrace = TimeSpan.FromHours(1); + private readonly IHttpClientFactory _httpClientFactory; private readonly ILogger _logger; + private readonly TimeProvider _timeProvider; // Cache keyed by a hash of the credential string. Each distinct // tenant/client/secret triple gets its own cached token and its own lock. @@ -28,11 +37,27 @@ public class OAuth2TokenService public OAuth2TokenService( IHttpClientFactory httpClientFactory, ILogger logger) + : this(httpClientFactory, logger, TimeProvider.System) + { + } + + /// Test seam: initializes the service with an explicit clock. + /// Factory used to create HTTP clients for token endpoint requests. + /// Logger instance. + /// Clock used for token expiry and stale-entry eviction. + internal OAuth2TokenService( + IHttpClientFactory httpClientFactory, + ILogger logger, + TimeProvider timeProvider) { _httpClientFactory = httpClientFactory; _logger = logger; + _timeProvider = timeProvider; } + /// The number of credential entries currently cached (test observability). + internal int CachedEntryCount => _cache.Count; + /// /// Gets a valid access token, refreshing if expired. /// Credentials format: "tenantId:clientId:clientSecret" @@ -54,12 +79,15 @@ public class OAuth2TokenService // Cache key incorporates authority + scope so two configurations with the same // credentials but different endpoints/scopes never share a token. var key = CredentialKey($"{credentials}|{authority}|{scope}"); + var now = _timeProvider.GetUtcNow(); + EvictStaleEntries(key, now); var entry = _cache.GetOrAdd(key, _ => new CacheEntry()); + entry.LastUsedTicks = now.UtcTicks; // Read the token/expiry pair through a single volatile reference so a lock-free // reader can never observe a torn token/expiry combination (arch-review S8). var current = entry.Current; - if (current != null && DateTimeOffset.UtcNow < current.Expiry) + if (current != null && _timeProvider.GetUtcNow() < current.Expiry) { return current.Token; } @@ -69,7 +97,7 @@ public class OAuth2TokenService { // Double-check after acquiring the per-credential lock. current = entry.Current; - if (current != null && DateTimeOffset.UtcNow < current.Expiry) + if (current != null && _timeProvider.GetUtcNow() < current.Expiry) { return current.Token; } @@ -116,7 +144,7 @@ public class OAuth2TokenService // Publish the token/expiry pair as a single atomic reference assignment // (Refresh 60s before expiry). entry.Current = new TokenValue( - token, DateTimeOffset.UtcNow.AddSeconds(expiresIn - 60)); + token, _timeProvider.GetUtcNow().AddSeconds(expiresIn - 60)); // The token endpoint identity is logged by tenant only — never // the client secret or the access token itself. @@ -130,6 +158,29 @@ public class OAuth2TokenService } } + /// + /// Drops every other entry that has been idle past and + /// holds no live token. An entry whose lock is held (a refresh in flight) is skipped. + /// Losing a race with a caller that just fetched the entry is harmless: that caller + /// completes against the orphaned entry and the next request simply re-fetches. + /// The semaphore is deliberately not disposed — a racing waiter may still hold it, + /// and an un-awaited owns no unmanaged handle. + /// + private void EvictStaleEntries(string keepKey, DateTimeOffset now) + { + var idleCutoff = now.UtcTicks - StaleEntryGrace.Ticks; + foreach (var pair in _cache) + { + if (pair.Key == keepKey) continue; + var entry = pair.Value; + if (entry.LastUsedTicks > idleCutoff) continue; + var current = entry.Current; + if (current != null && now < current.Expiry) continue; + if (entry.Lock.CurrentCount == 0) continue; + _cache.TryRemove(pair); + } + } + /// /// A stable, non-reversible key for the credential string so the cache /// is partitioned by credential identity without holding the secret as a key. @@ -149,5 +200,8 @@ public class OAuth2TokenService // always sees a self-consistent token/expiry pair (or null for "no token yet"). public volatile TokenValue? Current; public readonly SemaphoreSlim Lock = new(1, 1); + + // UTC ticks of the last GetTokenAsync touch; drives stale-entry eviction. + public long LastUsedTicks; } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests/OAuth2TokenServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests/OAuth2TokenServiceTests.cs index 238cf0b4..3757af32 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests/OAuth2TokenServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests/OAuth2TokenServiceTests.cs @@ -231,6 +231,67 @@ public class OAuth2TokenServiceTests Assert.Equal("https://outlook.office365.com/.default", handler.LastForm!["scope"]); } + /// + /// Verifies that a credential rotation does not leave the old credential's cache entry + /// behind forever: once the old token has expired and the entry has been idle past the + /// stale grace, the next token request evicts it (review 2026-10-21 06/MEM-3). + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task GetTokenAsync_RotatedCredential_StaleEntryIsEvicted() + { + var handler = new CountingHttpMessageHandler(HttpStatusCode.OK, TokenJson("t", expiresIn: 3600)); + var factory = CreateMockFactory(new HttpClient(handler)); + var clock = new ManualTimeProvider(DateTimeOffset.Parse("2026-10-22T00:00:00Z")); + var service = new OAuth2TokenService(factory, NullLogger.Instance, clock); + + await service.GetTokenAsync("tenant:client:old-secret"); + Assert.Equal(1, service.CachedEntryCount); + + // Rotate: the old entry is still fresh, so it survives the first request with the new secret. + await service.GetTokenAsync("tenant:client:new-secret"); + Assert.Equal(2, service.CachedEntryCount); + + // Well past the old token's expiry plus the stale grace — the next request sweeps it. + clock.Advance(TimeSpan.FromHours(3)); + await service.GetTokenAsync("tenant:client:new-secret"); + Assert.Equal(1, service.CachedEntryCount); + } + + /// + /// Verifies that an entry whose token was never acquired (bad credentials) is also evicted + /// once idle past the stale grace, rather than lingering with a null token forever. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task GetTokenAsync_FailedCredential_IdleEntryIsEvicted() + { + var handler = new CountingHttpMessageHandler(HttpStatusCode.OK, TokenJson("t", expiresIn: 3600)); + var factory = CreateMockFactory(new HttpClient(handler)); + var clock = new ManualTimeProvider(DateTimeOffset.Parse("2026-10-22T00:00:00Z")); + var service = new OAuth2TokenService(factory, NullLogger.Instance, clock); + + await Assert.ThrowsAsync(() => service.GetTokenAsync("not-a-triple")); + Assert.Equal(1, service.CachedEntryCount); + + clock.Advance(TimeSpan.FromHours(2)); + await service.GetTokenAsync("tenant:client:secret"); + Assert.Equal(1, service.CachedEntryCount); + } + + /// A hand-advanced clock for the cache-eviction tests. + private sealed class ManualTimeProvider(DateTimeOffset start) : TimeProvider + { + private DateTimeOffset _now = start; + + /// Moves the clock forward. + /// The amount to advance. + public void Advance(TimeSpan by) => _now += by; + + /// + public override DateTimeOffset GetUtcNow() => _now; + } + private static string TokenJson(string accessToken, int expiresIn) => JsonSerializer.Serialize(new {