diff --git a/src/IdentityServer/Licensing/IdentityServerLicenseValidator.cs b/src/IdentityServer/Licensing/IdentityServerLicenseValidator.cs index ef8d1e985..657146c62 100644 --- a/src/IdentityServer/Licensing/IdentityServerLicenseValidator.cs +++ b/src/IdentityServer/Licensing/IdentityServerLicenseValidator.cs @@ -7,7 +7,6 @@ using Duende.IdentityServer.Configuration; using Microsoft.Extensions.Logging; using System; -using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; @@ -19,8 +18,6 @@ internal class IdentityServerLicenseValidator : LicenseValidator _clientIds = new ConcurrentDictionary(); - ConcurrentDictionary _issuers = new ConcurrentDictionary(); public void Initialize(ILoggerFactory loggerFactory, IdentityServerOptions options, bool isDevelopment = false) { @@ -56,7 +53,7 @@ protected override void ValidateProductFeatures(List errors) { throw new Exception("Invalid License: The BFF edition license is not valid for IdentityServer."); } - + if (_options.KeyManagement.Enabled && !License.KeyManagementFeature) { errors.Add("You have automatic key management enabled, but your license does not include that feature of Duende IdentityServer. This feature requires the Business or Enterprise Edition tier of license. Either upgrade your license or disable automatic key management by setting the KeyManagement.Enabled property to false on the IdentityServerOptions."); @@ -70,54 +67,97 @@ protected override void WarnForProductFeaturesWhenMissingLicense() } } - bool ValidateClientWarned = false; - public void ValidateClient(string clientId) + private void EnsureAdded(ref HashSet hashSet, object lockObject, string key) { - _clientIds.TryAdd(clientId, 1); + // Lock free test first. + if (!hashSet.Contains(key)) + { + lock (lockObject) + { + // Check again after lock, to quite early if another thread + // already did the job. + if (!hashSet.Contains(key)) + { + // The HashSet is not thread safe. And we don't want to lock for every single + // time we use it. Our access pattern should be a lot of reads and a few writes + // so better to create a new copy every time we need to add a value. + var newSet = new HashSet(hashSet) + { + key + }; - if (License != null) + // Reference assignment is atomic so non-locked readers will handle this. + hashSet = newSet; + } + } + } + } + + public void ValidateClient(string clientId) => ValidateClient(clientId, License); + + HashSet _clientIds = new(); + object _clientIdLock = new(); + bool _validateClientWarned = false; + // Internal method that takes license as parameter to allow testing + internal void ValidateClient(string clientId, IdentityServerLicense license) + { + if (license != null && !license.ClientLimit.HasValue) + { + return; + } + + EnsureAdded(ref _clientIds, _clientIdLock, clientId); + + if (license != null) { - if (License.ClientLimit.HasValue && _clientIds.Count > License.ClientLimit) + if (_clientIds.Count > license.ClientLimit) { ErrorLog.Invoke( "Your license for Duende IdentityServer only permits {clientLimit} number of clients. You have processed requests for {clientCount}. The clients used were: {clients}.", - new object[] { License.ClientLimit, _clientIds.Count, _clientIds.Keys.ToArray() }); + [license.ClientLimit, _clientIds.Count, _clientIds.ToArray()]); } } else { - if (_clientIds.Count > 5 && !ValidateClientWarned) + if (!_validateClientWarned && _clientIds.Count > 5) { - ValidateClientWarned = true; + _validateClientWarned = true; WarningLog?.Invoke( "You do not have a license, and you have processed requests for {clientCount} clients. This number requires a tier of license higher than Starter Edition. The clients used were: {clients}.", - new object[] { _clientIds.Count, _clientIds.Keys.ToArray() }); + [_clientIds.Count, _clientIds.ToArray()]); } } } - bool ValidateIssuerWarned = false; + HashSet _issuers = new(); + object _issuerLock = new(); + bool _validateIssuerWarned = false; public void ValidateIssuer(string iss) { - _issuers.TryAdd(iss, 1); + if (License != null && !License.IssuerLimit.HasValue) + { + return; + } + + EnsureAdded(ref _issuers, _issuerLock, iss); if (License != null) { - if (License.IssuerLimit.HasValue && _issuers.Count > License.IssuerLimit) + if (_issuers.Count > License.IssuerLimit) { ErrorLog.Invoke( "Your license for Duende IdentityServer only permits {issuerLimit} number of issuers. You have processed requests for {issuerCount}. The issuers used were: {issuers}. This might be due to your server being accessed via different URLs or a direct IP and/or you have reverse proxy or a gateway involved. This suggests a network infrastructure configuration problem, or you are deliberately hosting multiple URLs and require an upgraded license.", - new object[] { License.IssuerLimit, _issuers.Count, _issuers.Keys.ToArray() }); + [License.IssuerLimit, _issuers.Count, _issuers.ToArray()]); } } else { - if (_issuers.Count > 1 && !ValidateIssuerWarned) + if (!_validateIssuerWarned && _issuers.Count > 1) { - ValidateIssuerWarned = true; + _validateIssuerWarned = true; WarningLog?.Invoke( "You do not have a license, and you have processed requests for {issuerCount} issuers. If you are deliberately hosting multiple URLs then this number requires a license per issuer, or the Enterprise Edition tier of license. If not then this might be due to your server being accessed via different URLs or a direct IP and/or you have reverse proxy or a gateway involved, and this suggests a network infrastructure configuration problem. The issuers used were: {issuers}.", - new object[] { _issuers.Count, _issuers.Keys.ToArray() }); + [_issuers.Count, _issuers.ToArray()]); } } } @@ -179,9 +219,9 @@ public void ValidateResourceIndicators(string resourceIndicator) bool ValidateParWarned = false; public void ValidatePar() { - if(License != null) + if (License != null) { - if(!License.ParFeature) + if (!License.ParFeature) { throw new Exception("A request was made to the pushed authorization endpoint. Your license of Duende IdentityServer does not permit pushed authorization. This features requires the Business Edition or higher tier of license."); } diff --git a/test/IdentityServer.UnitTests/Licensing/IdentityServerLicenseValidatorTests.cs b/test/IdentityServer.UnitTests/Licensing/IdentityServerLicenseValidatorTests.cs index dc61e200f..852b76b0b 100644 --- a/test/IdentityServer.UnitTests/Licensing/IdentityServerLicenseValidatorTests.cs +++ b/test/IdentityServer.UnitTests/Licensing/IdentityServerLicenseValidatorTests.cs @@ -4,6 +4,7 @@ using System; using System.Security.Claims; +using Duende; using Duende.IdentityServer; using FluentAssertions; using Xunit; @@ -379,4 +380,54 @@ public void invalid_edition_should_fail() func.Should().Throw(); } } + + private class MockLicenseValidator : IdentityServerLicenseValidator + { + public MockLicenseValidator() + { + ErrorLog = (str, obj) => { ErrorLogCount++; }; + WarningLog = (str, obj) => { WarningLogCount++; }; + } + + public int ErrorLogCount { get; set; } + public int WarningLogCount { get; set; } + } + + [Theory] + [Trait("Category", Category)] + [InlineData(false, 5)] + [InlineData(true, 15)] + public void client_count_exceeded_should_warn(bool hasLicense, int allowedClients) + { + var license = hasLicense ? new IdentityServerLicense(new Claim("edition", "business")) : null; + var subject = new MockLicenseValidator(); + + for (int i = 0; i < allowedClients; i++) + { + subject.ValidateClient("client" + i, license); + } + + // Adding the allowed number of clients shouldn't log. + subject.ErrorLogCount.Should().Be(0); + subject.WarningLogCount.Should().Be(0); + + // Validating same client again shouldn't log. + subject.ValidateClient("client3", license); + subject.ErrorLogCount.Should().Be(0); + subject.WarningLogCount.Should().Be(0); + + subject.ValidateClient("extra1", license); + subject.ValidateClient("extra2", license); + + if (hasLicense) + { + subject.ErrorLogCount.Should().Be(2); + subject.WarningLogCount.Should().Be(0); + } + else + { + subject.ErrorLogCount.Should().Be(0); + subject.WarningLogCount.Should().Be(1); + } + } } \ No newline at end of file