From ad5bce30211984776dda09b62d7b1df701efe3dd Mon Sep 17 00:00:00 2001 From: Alberto Spelta Date: Mon, 19 Oct 2026 20:43:35 +0200 Subject: [PATCH] refactor: read group policies through PolicyService PolicyService reads the registry once, on first access, and exposes an immutable PolicySnapshot. RegistryPolicyReader merges machine and user policies, with machine values taking precedence, and disposes the registry keys it opens. Consumers depend on IPolicyService instead of IPolicies. --- src/Host/BravoServiceCollectionExtensions.cs | 2 +- src/Infrastructure/AppWindow.cs | 8 +- src/Infrastructure/Policies/IPolicyReader.cs | 6 ++ src/Infrastructure/Policies/IPolicySource.cs | 22 ++--- .../Policies/IPolicySourceFactory.cs | 6 ++ src/Infrastructure/Policies/Policies.cs | 28 ------ src/Infrastructure/Policies/PolicyScope.cs | 7 ++ src/Infrastructure/Policies/PolicyService.cs | 25 ++++++ .../Policies/PolicyServiceFactory.cs | 12 +++ src/Infrastructure/Policies/PolicySnapshot.cs | 21 +++++ ...ciesFactory.cs => RegistryPolicyReader.cs} | 27 +++--- .../Policies/RegistryPolicySource.cs | 5 +- .../Policies/RegistryPolicySourceFactory.cs | 21 +++++ .../CloudAuthenticationClient.cs | 8 +- .../DaxTemplate/DaxTemplateManager.cs | 8 +- .../Telemetry/TelemetryService.cs | 6 +- src/Services/ManageDatesService.cs | 4 +- src/Services/TemplateDevelopmentService.cs | 10 +-- .../Infrastructure/Policies/PolicyFakes.cs | 45 ++++++++++ .../Policies/PolicyServiceTests.cs | 54 ++++++++++++ .../RegistryPolicyReaderLifetimeTests.cs | 83 ++++++++++++++++++ ...yTests.cs => RegistryPolicyReaderTests.cs} | 86 ++++++------------- .../Policies/RegistryPolicySourceTests.cs | 29 ++++--- 23 files changed, 368 insertions(+), 155 deletions(-) create mode 100644 src/Infrastructure/Policies/IPolicyReader.cs create mode 100644 src/Infrastructure/Policies/IPolicySourceFactory.cs delete mode 100644 src/Infrastructure/Policies/Policies.cs create mode 100644 src/Infrastructure/Policies/PolicyScope.cs create mode 100644 src/Infrastructure/Policies/PolicyService.cs create mode 100644 src/Infrastructure/Policies/PolicyServiceFactory.cs create mode 100644 src/Infrastructure/Policies/PolicySnapshot.cs rename src/Infrastructure/Policies/{PoliciesFactory.cs => RegistryPolicyReader.cs} (62%) create mode 100644 src/Infrastructure/Policies/RegistryPolicySourceFactory.cs create mode 100644 test/Bravo.Tests/Infrastructure/Policies/PolicyFakes.cs create mode 100644 test/Bravo.Tests/Infrastructure/Policies/PolicyServiceTests.cs create mode 100644 test/Bravo.Tests/Infrastructure/Policies/RegistryPolicyReaderLifetimeTests.cs rename test/Bravo.Tests/Infrastructure/Policies/{PoliciesFactoryTests.cs => RegistryPolicyReaderTests.cs} (65%) diff --git a/src/Host/BravoServiceCollectionExtensions.cs b/src/Host/BravoServiceCollectionExtensions.cs index 6f2b2f53..bda4a302 100644 --- a/src/Host/BravoServiceCollectionExtensions.cs +++ b/src/Host/BravoServiceCollectionExtensions.cs @@ -25,7 +25,7 @@ public static IServiceCollection AddBravoInitializationServices(this IServiceCol // redirection belong to the entry point and stay out of the container. services.AddSingleton(context.Instance); - services.AddSingleton(PoliciesFactory.Create()); + services.AddSingleton(new PolicyServiceFactory().Create()); services.AddSingleton(TelemetryService.Instance); return services; diff --git a/src/Infrastructure/AppWindow.cs b/src/Infrastructure/AppWindow.cs index 133fc2bd..448de2a2 100644 --- a/src/Infrastructure/AppWindow.cs +++ b/src/Infrastructure/AppWindow.cs @@ -46,18 +46,18 @@ internal partial class AppWindow : Form private readonly IOptions _startupSettingsOptionsAccessor; private readonly WebView2ProxyAuthHandler _proxyAuthHandler; private readonly Color _startupThemeColor; - private readonly IPolicies _policies; + private readonly IPolicyService _policyService; public AppWindow( IInstanceActivationEvents instanceActivationEvents, IServerAddressProvider serverAddressProvider, IOptions startupSettingsOptionsAccessor, - IPolicies policies) + IPolicyService policyService) { _instanceActivationEvents = instanceActivationEvents; _serverAddressProvider = serverAddressProvider; _startupSettingsOptionsAccessor = startupSettingsOptionsAccessor; - _policies = policies; + _policyService = policyService; _proxyAuthHandler = new WebView2ProxyAuthHandler(WebProxyWrapper.Current); _startupThemeColor = ThemeHelper.ShouldUseDarkMode(UserPreferences.Current.Theme) ? AppEnvironment.ThemeColorDark : AppEnvironment.ThemeColorLight; @@ -328,7 +328,7 @@ private MemoryStream GetConfigJs() token = AppEnvironment.ApiAuthenticationToken, version = AppVersion.SemanticVersion, options = BravoOptions.CreateFromUserPreferences(), - policies = _policies, + policies = _policyService.Current, culture = new { ietfLanguageTag = CultureInfo.CurrentCulture.IetfLanguageTag, diff --git a/src/Infrastructure/Policies/IPolicyReader.cs b/src/Infrastructure/Policies/IPolicyReader.cs new file mode 100644 index 00000000..c4be6520 --- /dev/null +++ b/src/Infrastructure/Policies/IPolicyReader.cs @@ -0,0 +1,6 @@ +namespace Sqlbi.Bravo.Infrastructure.Policies; + +internal interface IPolicyReader +{ + PolicySnapshot Read(); +} diff --git a/src/Infrastructure/Policies/IPolicySource.cs b/src/Infrastructure/Policies/IPolicySource.cs index c3241754..3418c5b8 100644 --- a/src/Infrastructure/Policies/IPolicySource.cs +++ b/src/Infrastructure/Policies/IPolicySource.cs @@ -1,24 +1,13 @@ using System; -using Microsoft.Win32; namespace Sqlbi.Bravo.Infrastructure.Policies; -/// -/// Abstraction over a single raw policy value store (e.g. a registry key), so that -/// ' parsing/precedence logic does not depend on -/// directly and can be unit tested against a fake, without touching the real registry. -/// -internal interface IPolicySource +internal interface IPolicySource : IDisposable { int? GetInt(string name); string? GetString(string name); } -/// -/// Typed reading conventions shared by every : a policy is a -/// DWORD (0/1 -> bool, or a defined enum member) or a string. Kept as extensions rather than -/// interface members so itself stays minimal (raw int/string only). -/// internal static class PolicySourceExtensions { private const int PolicyDisabledValue = 0; @@ -37,10 +26,13 @@ internal static class PolicySourceExtensions }; } - public T? GetEnum(string name) where T : struct, Enum + /// + /// TEnum must have int as its underlying type. + /// + public TEnum? GetEnum(string name) where TEnum : struct, Enum { - if (source.GetInt(name) is { } value && Enum.IsDefined(typeof(T), value)) - return (T)(object)value; + if (source.GetInt(name) is { } value && Enum.IsDefined(typeof(TEnum), value)) + return (TEnum)(object)value; return null; // Policy not set or invalid } diff --git a/src/Infrastructure/Policies/IPolicySourceFactory.cs b/src/Infrastructure/Policies/IPolicySourceFactory.cs new file mode 100644 index 00000000..9a9c8459 --- /dev/null +++ b/src/Infrastructure/Policies/IPolicySourceFactory.cs @@ -0,0 +1,6 @@ +namespace Sqlbi.Bravo.Infrastructure.Policies; + +internal interface IPolicySourceFactory +{ + IPolicySource Open(PolicyScope scope); +} diff --git a/src/Infrastructure/Policies/Policies.cs b/src/Infrastructure/Policies/Policies.cs deleted file mode 100644 index c0dc94aa..00000000 --- a/src/Infrastructure/Policies/Policies.cs +++ /dev/null @@ -1,28 +0,0 @@ -using Sqlbi.Bravo.Infrastructure.Configuration.Settings; - -namespace Sqlbi.Bravo.Infrastructure.Policies; - -internal interface IPolicies -{ - bool? TelemetryEnabled { get; } - UpdateChannelType? UpdateChannel { get; } - bool? UpdateCheckEnabled { get; } - bool? UseSystemBrowserForAuthentication { get; } - bool? BuiltInTemplatesEnabled { get; } - bool? CustomTemplatesEnabled { get; } - string? CustomTemplatesOrganizationRepositoryPath { get; } -} - -/// -/// Immutable snapshot of Bravo's effective policy values. Pure data - see -/// for how instances are read from the registry, parsed, -/// and merged with LocalMachine/CurrentUser precedence. -/// -internal sealed record Policies( - bool? TelemetryEnabled, - UpdateChannelType? UpdateChannel, - bool? UpdateCheckEnabled, - bool? UseSystemBrowserForAuthentication, - bool? BuiltInTemplatesEnabled, - bool? CustomTemplatesEnabled, - string? CustomTemplatesOrganizationRepositoryPath) : IPolicies; diff --git a/src/Infrastructure/Policies/PolicyScope.cs b/src/Infrastructure/Policies/PolicyScope.cs new file mode 100644 index 00000000..62abdec5 --- /dev/null +++ b/src/Infrastructure/Policies/PolicyScope.cs @@ -0,0 +1,7 @@ +namespace Sqlbi.Bravo.Infrastructure.Policies; + +internal enum PolicyScope +{ + Computer, + User, +} diff --git a/src/Infrastructure/Policies/PolicyService.cs b/src/Infrastructure/Policies/PolicyService.cs new file mode 100644 index 00000000..3129bea0 --- /dev/null +++ b/src/Infrastructure/Policies/PolicyService.cs @@ -0,0 +1,25 @@ +using System; + +namespace Sqlbi.Bravo.Infrastructure.Policies; + +public interface IPolicyService +{ + /// + /// Gets the policy snapshot initialized once for this service instance. + /// + PolicySnapshot Current { get; } +} + +internal sealed class PolicyService : IPolicyService +{ + private readonly IPolicyReader _reader; + private readonly Lazy _current; + + public PolicyService(IPolicyReader reader) + { + _reader = reader; + _current = new Lazy(_reader.Read); + } + + public PolicySnapshot Current => _current.Value; +} diff --git a/src/Infrastructure/Policies/PolicyServiceFactory.cs b/src/Infrastructure/Policies/PolicyServiceFactory.cs new file mode 100644 index 00000000..577be97e --- /dev/null +++ b/src/Infrastructure/Policies/PolicyServiceFactory.cs @@ -0,0 +1,12 @@ +namespace Sqlbi.Bravo.Infrastructure.Policies; + +internal interface IPolicyServiceFactory +{ + IPolicyService Create(); +} + +internal sealed class PolicyServiceFactory : IPolicyServiceFactory +{ + public IPolicyService Create() + => new PolicyService(new RegistryPolicyReader(new RegistryPolicySourceFactory())); +} diff --git a/src/Infrastructure/Policies/PolicySnapshot.cs b/src/Infrastructure/Policies/PolicySnapshot.cs new file mode 100644 index 00000000..923d8f7c --- /dev/null +++ b/src/Infrastructure/Policies/PolicySnapshot.cs @@ -0,0 +1,21 @@ +using Sqlbi.Bravo.Infrastructure.Configuration.Settings; + +namespace Sqlbi.Bravo.Infrastructure.Policies; + +/// +/// Contains the effective policy values from one read of the configured sources. +/// +/// +/// Null means not configured; false and zero are configured values. +/// +public sealed record PolicySnapshot( + bool? TelemetryEnabled, + UpdateChannelType? UpdateChannel, + bool? UpdateCheckEnabled, + bool? UseSystemBrowserForAuthentication, + bool? BuiltInTemplatesEnabled, + bool? CustomTemplatesEnabled, + string? CustomTemplatesOrganizationRepositoryPath) +{ + public static PolicySnapshot NotConfigured { get; } = new(null, null, null, null, null, null, null); +} diff --git a/src/Infrastructure/Policies/PoliciesFactory.cs b/src/Infrastructure/Policies/RegistryPolicyReader.cs similarity index 62% rename from src/Infrastructure/Policies/PoliciesFactory.cs rename to src/Infrastructure/Policies/RegistryPolicyReader.cs index 0daffa76..5a97375a 100644 --- a/src/Infrastructure/Policies/PoliciesFactory.cs +++ b/src/Infrastructure/Policies/RegistryPolicyReader.cs @@ -1,28 +1,23 @@ -using Microsoft.Win32; -using Sqlbi.Bravo.Infrastructure.Configuration.Settings; +using Sqlbi.Bravo.Infrastructure.Configuration.Settings; namespace Sqlbi.Bravo.Infrastructure.Policies; -/// -/// Builds instances: parses a single , -/// and composes the effective policy set from LocalMachine + CurrentUser with precedence. -/// -internal static class PoliciesFactory +internal sealed class RegistryPolicyReader(IPolicySourceFactory sourceFactory) : IPolicyReader { - private const string OptionSettingsSubKeyName = @"SOFTWARE\Policies\SQLBI\Bravo\OptionSettings"; + private readonly IPolicySourceFactory _sourceFactory = sourceFactory; - public static Policies Create() + public PolicySnapshot Read() { - using var machineKey = Registry.LocalMachine.OpenSubKey(OptionSettingsSubKeyName); - var machinePolicies = FromSource(new RegistryPolicySource(machineKey)); + using var machineSource = _sourceFactory.Open(PolicyScope.Computer); + var machinePolicies = FromSource(machineSource); - using var userKey = Registry.CurrentUser.OpenSubKey(OptionSettingsSubKeyName); - var userPolicies = FromSource(new RegistryPolicySource(userKey)); + using var userSource = _sourceFactory.Open(PolicyScope.User); + var userPolicies = FromSource(userSource); return Merge(machinePolicies, userPolicies); } - internal static Policies FromSource(IPolicySource source) => new( + internal static PolicySnapshot FromSource(IPolicySource source) => new( TelemetryEnabled: source.GetBool("TelemetryEnabled"), UpdateChannel: source.GetEnum("UpdateChannel"), UpdateCheckEnabled: source.GetBool("UpdateCheckEnabled"), @@ -31,10 +26,10 @@ public static Policies Create() CustomTemplatesEnabled: source.GetBool("CustomTemplatesEnabled"), CustomTemplatesOrganizationRepositoryPath: source.GetString("CustomTemplatesOrganizationRepositoryPath")); - internal static Policies Merge(Policies machinePolicies, Policies userPolicies) + internal static PolicySnapshot Merge(PolicySnapshot machinePolicies, PolicySnapshot userPolicies) { // LocalMachine takes precedence over CurrentUser when both are configured - return new Policies( + return new PolicySnapshot( TelemetryEnabled: machinePolicies.TelemetryEnabled ?? userPolicies.TelemetryEnabled, UpdateChannel: machinePolicies.UpdateChannel ?? userPolicies.UpdateChannel, UpdateCheckEnabled: machinePolicies.UpdateCheckEnabled ?? userPolicies.UpdateCheckEnabled, diff --git a/src/Infrastructure/Policies/RegistryPolicySource.cs b/src/Infrastructure/Policies/RegistryPolicySource.cs index bf598cd0..ce966475 100644 --- a/src/Infrastructure/Policies/RegistryPolicySource.cs +++ b/src/Infrastructure/Policies/RegistryPolicySource.cs @@ -3,8 +3,7 @@ namespace Sqlbi.Bravo.Infrastructure.Policies; /// -/// Adapter that bridges to a real . -/// Intentionally a thin pass-through with no logic of its own. +/// Owns a read-only registry key containing raw policy values. /// internal sealed class RegistryPolicySource(RegistryKey? key) : IPolicySource { @@ -15,4 +14,6 @@ internal sealed class RegistryPolicySource(RegistryKey? key) : IPolicySource public string? GetString(string name) => _key?.GetValue(name) as string; + + public void Dispose() => _key?.Dispose(); } diff --git a/src/Infrastructure/Policies/RegistryPolicySourceFactory.cs b/src/Infrastructure/Policies/RegistryPolicySourceFactory.cs new file mode 100644 index 00000000..b1d9864a --- /dev/null +++ b/src/Infrastructure/Policies/RegistryPolicySourceFactory.cs @@ -0,0 +1,21 @@ +using System; +using Microsoft.Win32; + +namespace Sqlbi.Bravo.Infrastructure.Policies; + +internal sealed class RegistryPolicySourceFactory : IPolicySourceFactory +{ + private const string OptionSettingsSubKeyName = @"SOFTWARE\Policies\SQLBI\Bravo\OptionSettings"; + + public IPolicySource Open(PolicyScope scope) + { + var hive = scope switch + { + PolicyScope.Computer => Registry.LocalMachine, + PolicyScope.User => Registry.CurrentUser, + _ => throw new ArgumentOutOfRangeException(nameof(scope)), + }; + + return new RegistryPolicySource(hive.OpenSubKey(OptionSettingsSubKeyName)); + } +} diff --git a/src/Infrastructure/PowerBI/Cloud/Authentication/CloudAuthenticationClient.cs b/src/Infrastructure/PowerBI/Cloud/Authentication/CloudAuthenticationClient.cs index c14d8d9c..d0ffd560 100644 --- a/src/Infrastructure/PowerBI/Cloud/Authentication/CloudAuthenticationClient.cs +++ b/src/Infrastructure/PowerBI/Cloud/Authentication/CloudAuthenticationClient.cs @@ -22,12 +22,12 @@ public interface ICloudAuthenticationClient /// /// Handles authentication with Microsoft Entra ID (Azure AD) using MSAL.NET, including token acquisition and cache management. /// -internal sealed class CloudAuthenticationClient(IPolicies policies) : ICloudAuthenticationClient +internal sealed class CloudAuthenticationClient(IPolicyService policyService) : ICloudAuthenticationClient { private const string SystemBrowserRedirectUri = "http://localhost"; private const string OrganizationalAccountsOnlyQueryParameter = "msafed=0"; // no Microsoft accounts (MSA) allowed - private readonly IPolicies _policies = policies; + private readonly IPolicyService _policyService = policyService; public async Task AcquireTokenAsync( CloudEnvironment environment, string email, CancellationToken cancellationToken) @@ -77,7 +77,7 @@ public async Task ClearTokenCacheAsync(CloudEnvironment environment) private async Task AcquireTokenInteractiveAsync( IPublicClientApplication client, string[] scopes, string email, string claims, CancellationToken cancellationToken) { - var useSystemBrowser = _policies.UseSystemBrowserForAuthentication ?? UserPreferences.Current.UseSystemBrowserForAuthentication; + var useSystemBrowser = _policyService.Current.UseSystemBrowserForAuthentication ?? UserPreferences.Current.UseSystemBrowserForAuthentication; var useEmbeddedBrowser = !useSystemBrowser; var extraQueryParameters = OrganizationalAccountsOnlyQueryParameter; var prompt = Prompt.SelectAccount; @@ -125,7 +125,7 @@ public async Task ClearTokenCacheAsync(CloudEnvironment environment) private IPublicClientApplication CreatePublicClient(CloudEnvironment environment) { - var useSystemBrowser = _policies.UseSystemBrowserForAuthentication ?? UserPreferences.Current.UseSystemBrowserForAuthentication; + var useSystemBrowser = _policyService.Current.UseSystemBrowserForAuthentication ?? UserPreferences.Current.UseSystemBrowserForAuthentication; var useEmbeddedBrowser = !useSystemBrowser; var redirectUri = (useEmbeddedBrowser ? environment.RedirectUri : SystemBrowserRedirectUri); diff --git a/src/Infrastructure/Services/DaxTemplate/DaxTemplateManager.cs b/src/Infrastructure/Services/DaxTemplate/DaxTemplateManager.cs index 3aa470bd..f7c1c5fb 100644 --- a/src/Infrastructure/Services/DaxTemplate/DaxTemplateManager.cs +++ b/src/Infrastructure/Services/DaxTemplate/DaxTemplateManager.cs @@ -31,11 +31,11 @@ internal class DaxTemplateManager internal static readonly string UserPath = Path.Combine(AppEnvironment.ApplicationDataPath, @"ManageDates\Templates"); private readonly object _cacheSyncLock = new(); - private readonly IPolicies _policies; + private readonly IPolicyService _policyService; - public DaxTemplateManager(IPolicies policies) + public DaxTemplateManager(IPolicyService policyService) { - _policies = policies; + _policyService = policyService; InitializeCache(); } @@ -46,7 +46,7 @@ public Package GetPackage(string path) public IEnumerable GetPackages() { - if (_policies.BuiltInTemplatesEnabled is false) + if (_policyService.Current.BuiltInTemplatesEnabled is false) { return Array.Empty(); } diff --git a/src/Infrastructure/Telemetry/TelemetryService.cs b/src/Infrastructure/Telemetry/TelemetryService.cs index 23d5031f..444a42dd 100644 --- a/src/Infrastructure/Telemetry/TelemetryService.cs +++ b/src/Infrastructure/Telemetry/TelemetryService.cs @@ -17,7 +17,7 @@ internal sealed class TelemetryService : ITelemetryService { private readonly TelemetryConfiguration _configuration; private readonly TelemetryClient _client; - private readonly IPolicies _policies; + private readonly PolicySnapshot _policies; private static readonly Lazy _instance = new(CreateInstance, isThreadSafe: true); @@ -37,7 +37,7 @@ private static TelemetryService CreateInstance() configuration.TelemetryInitializers.Add(new DefaultTelemetryInitializer()); configuration.ConnectionString = TelemetrySessionInfo.ConnectionString; - var policies = PoliciesFactory.Create(); + var policies = new PolicyServiceFactory().Create().Current; // Determine whether telemetry is enabled based on policies and user settings. var telemetryEnabled = policies.TelemetryEnabled ?? UserPreferences.Current.TelemetryEnabled; @@ -48,7 +48,7 @@ private static TelemetryService CreateInstance() return new TelemetryService(configuration, policies); } - private TelemetryService(TelemetryConfiguration configuration, IPolicies policies) + private TelemetryService(TelemetryConfiguration configuration, PolicySnapshot policies) { _configuration = configuration; _client = new TelemetryClient(configuration); diff --git a/src/Services/ManageDatesService.cs b/src/Services/ManageDatesService.cs index 8d62ba96..24260431 100644 --- a/src/Services/ManageDatesService.cs +++ b/src/Services/ManageDatesService.cs @@ -32,9 +32,9 @@ internal class ManageDatesService : IManageDatesService { private readonly DaxTemplateManager _templateManager; - public ManageDatesService(IPolicies policies) + public ManageDatesService(IPolicyService policyService) { - _templateManager = new DaxTemplateManager(policies); + _templateManager = new DaxTemplateManager(policyService); } public IEnumerable GetConfigurations(PBIDesktopReport report, CancellationToken cancellationToken) diff --git a/src/Services/TemplateDevelopmentService.cs b/src/Services/TemplateDevelopmentService.cs index 8c3f5da8..03680df5 100644 --- a/src/Services/TemplateDevelopmentService.cs +++ b/src/Services/TemplateDevelopmentService.cs @@ -52,13 +52,13 @@ internal class TemplateDevelopmentService : ITemplateDevelopmentService private readonly JsonSerializerOptions _serializerOptions; private readonly DaxTemplateManager _templateManager; private readonly IServerAddressProvider _serverAddressProvider; - private readonly IPolicies _policies; + private readonly IPolicyService _policyService; - public TemplateDevelopmentService(IServerAddressProvider serverAddressProvider, IPolicies policies) + public TemplateDevelopmentService(IServerAddressProvider serverAddressProvider, IPolicyService policyService) { _serverAddressProvider = serverAddressProvider; - _policies = policies; - _templateManager = new DaxTemplateManager(policies); + _policyService = policyService; + _templateManager = new DaxTemplateManager(policyService); _serializerOptions = new JsonSerializerOptions(AppEnvironment.DefaultJsonOptions) { WriteIndented = true }; } @@ -163,7 +163,7 @@ public IEnumerable GetOrganizationCustomPackages() { var customPackages = new List(); - var repositoryPath = _policies.CustomTemplatesOrganizationRepositoryPath; + var repositoryPath = _policyService.Current.CustomTemplatesOrganizationRepositoryPath; if (repositoryPath is not null && Directory.Exists(repositoryPath)) { var packagePaths = Directory.EnumerateFiles(repositoryPath, searchPattern: $"*{CustomPackageFileExtension}", new EnumerationOptions diff --git a/test/Bravo.Tests/Infrastructure/Policies/PolicyFakes.cs b/test/Bravo.Tests/Infrastructure/Policies/PolicyFakes.cs new file mode 100644 index 00000000..344a9d8b --- /dev/null +++ b/test/Bravo.Tests/Infrastructure/Policies/PolicyFakes.cs @@ -0,0 +1,45 @@ +using System; +using System.Collections.Generic; +using Sqlbi.Bravo.Infrastructure.Policies; + +namespace Bravo.Tests.Infrastructure.Policies; + +internal sealed class FakePolicySource : IPolicySource +{ + private readonly Dictionary _values = new(); + public int DisposeCount { get; private set; } + public Exception? ReadFailure { get; set; } + + public void Set(string name, object value) => _values[name] = value; + + public int? GetInt(string name) + { + if (ReadFailure is not null) + throw ReadFailure; + + return _values.TryGetValue(name, out var value) && value is int intValue ? intValue : null; + } + + public string? GetString(string name) + => _values.TryGetValue(name, out var value) ? value as string : null; + + public void Dispose() => DisposeCount++; +} + +internal sealed class FakePolicySourceFactory(Func open) : IPolicySourceFactory +{ + public IPolicySource Open(PolicyScope scope) => open(scope); +} + +internal sealed class FakePolicyReader : IPolicyReader +{ + public int ReadCount { get; private set; } + public PolicySnapshot Snapshot { get; set; } = PolicySnapshot.NotConfigured; + public Func? OnRead { get; set; } + + public PolicySnapshot Read() + { + ReadCount++; + return OnRead?.Invoke() ?? Snapshot; + } +} diff --git a/test/Bravo.Tests/Infrastructure/Policies/PolicyServiceTests.cs b/test/Bravo.Tests/Infrastructure/Policies/PolicyServiceTests.cs new file mode 100644 index 00000000..b81f9345 --- /dev/null +++ b/test/Bravo.Tests/Infrastructure/Policies/PolicyServiceTests.cs @@ -0,0 +1,54 @@ +using System; +using System.IO; +using System.Linq; +using System.Threading.Tasks; +using Sqlbi.Bravo.Infrastructure.Policies; +using Xunit; + +namespace Bravo.Tests.Infrastructure.Policies; + +public class PolicyServiceTests +{ + [Fact] + public void Current_ReadsOnceAndKeepsTheSameSnapshot() + { + var initial = PolicySnapshot.NotConfigured with { TelemetryEnabled = false }; + var reader = new FakePolicyReader { Snapshot = initial }; + var service = new PolicyService(reader); + + Assert.Equal(0, reader.ReadCount); + Assert.Same(initial, service.Current); + + reader.Snapshot = PolicySnapshot.NotConfigured with { TelemetryEnabled = true }; + + Assert.Same(initial, service.Current); + Assert.False(service.Current.TelemetryEnabled); + Assert.Equal(1, reader.ReadCount); + } + + [Fact] + public async Task Current_ConcurrentAccessInitializesOnce() + { + var reader = new FakePolicyReader(); + var service = new PolicyService(reader); + + var snapshots = await Task.WhenAll( + Enumerable.Range(0, 16).Select(_ => Task.Run(() => service.Current))) + .WaitAsync(TimeSpan.FromSeconds(10)); + + Assert.All(snapshots, snapshot => Assert.Same(service.Current, snapshot)); + Assert.Equal(1, reader.ReadCount); + } + + [Fact] + public void Current_ReadFailurePropagatesWithoutRetryingOrReturningDefaults() + { + var failure = new IOException("Registry unavailable"); + var reader = new FakePolicyReader { OnRead = () => throw failure }; + var service = new PolicyService(reader); + + Assert.Same(failure, Assert.Throws(() => service.Current)); + Assert.Same(failure, Assert.Throws(() => service.Current)); + Assert.Equal(1, reader.ReadCount); + } +} diff --git a/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicyReaderLifetimeTests.cs b/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicyReaderLifetimeTests.cs new file mode 100644 index 00000000..cf7e8cfd --- /dev/null +++ b/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicyReaderLifetimeTests.cs @@ -0,0 +1,83 @@ +using System; +using System.Collections.Generic; +using System.IO; +using Sqlbi.Bravo.Infrastructure.Policies; +using Xunit; + +namespace Bravo.Tests.Infrastructure.Policies; + +public class RegistryPolicyReaderLifetimeTests +{ + [Fact] + public void Read_CombinesBothScopesAndDisposesTheirSources() + { + var computer = new FakePolicySource(); + computer.Set("TelemetryEnabled", 0); + computer.Set("UpdateCheckEnabled", "invalid"); + var user = new FakePolicySource(); + user.Set("TelemetryEnabled", 1); + user.Set("UpdateCheckEnabled", 1); + var scopes = new List(); + var factory = new FakePolicySourceFactory(scope => + { + scopes.Add(scope); + return scope == PolicyScope.Computer ? computer : user; + }); + var reader = new RegistryPolicyReader(factory); + + var policies = reader.Read(); + + Assert.False(policies.TelemetryEnabled); + Assert.True(policies.UpdateCheckEnabled); + Assert.Equal([PolicyScope.Computer, PolicyScope.User], scopes); + Assert.All(new[] { computer, user }, source => Assert.Equal(1, source.DisposeCount)); + } + + [Fact] + public void Read_UserKeyOpenFailureDisposesComputerSourceAndPropagates() + { + var computer = new FakePolicySource(); + var failure = new UnauthorizedAccessException(); + var reader = new RegistryPolicyReader(new FakePolicySourceFactory(scope => + scope == PolicyScope.Computer ? computer : throw failure)); + + Assert.Same(failure, Assert.Throws(() => reader.Read())); + Assert.Equal(1, computer.DisposeCount); + } + + [Fact] + public void Read_ValueReadFailureDisposesEveryOpenedSourceAndPropagates() + { + var computer = new FakePolicySource(); + var failure = new IOException(); + var user = new FakePolicySource { ReadFailure = failure }; + var reader = new RegistryPolicyReader(new FakePolicySourceFactory(scope => + scope == PolicyScope.Computer ? computer : user)); + + Assert.Same(failure, Assert.Throws(() => reader.Read())); + Assert.Equal(1, computer.DisposeCount); + Assert.Equal(1, user.DisposeCount); + } + + [Fact] + public void Read_ComputerOpenFailureDoesNotFallBackToUser() + { + var scopes = new List(); + var reader = new RegistryPolicyReader(new FakePolicySourceFactory(scope => + { + scopes.Add(scope); + throw new UnauthorizedAccessException(); + })); + + Assert.Throws(() => reader.Read()); + Assert.Equal([PolicyScope.Computer], scopes); + } + + [Fact] + public void Read_MissingKeysAreAnEmptySnapshot() + { + var reader = new RegistryPolicyReader(new FakePolicySourceFactory(_ => new RegistryPolicySource(null))); + + Assert.Equal(PolicySnapshot.NotConfigured, reader.Read()); + } +} diff --git a/test/Bravo.Tests/Infrastructure/Policies/PoliciesFactoryTests.cs b/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicyReaderTests.cs similarity index 65% rename from test/Bravo.Tests/Infrastructure/Policies/PoliciesFactoryTests.cs rename to test/Bravo.Tests/Infrastructure/Policies/RegistryPolicyReaderTests.cs index 507d42c1..b85e9941 100644 --- a/test/Bravo.Tests/Infrastructure/Policies/PoliciesFactoryTests.cs +++ b/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicyReaderTests.cs @@ -5,46 +5,24 @@ namespace Bravo.Tests.Infrastructure.Policies; -/// -/// Exercises PoliciesFactory's parsing/precedence logic using an in-memory fake instead of the -/// real registry. Registry-adapter behavior itself is covered separately by -/// . -/// -public class PoliciesFactoryTests +public class RegistryPolicyReaderTests { - private sealed class FakePolicySource : IPolicySource - { - private readonly Dictionary _values = new(); - - public void Set(string name, object value) => _values[name] = value; - - public int? GetInt(string name) - => _values.TryGetValue(name, out var value) && value is int intValue ? intValue : null; - - public string? GetString(string name) - => _values.TryGetValue(name, out var value) ? value as string : null; - } - - // MemberData deliberately carries only the (public) property name rather than a - // Func selector: Policies/IPolicies are internal, and a public test - // method cannot declare a parameter of a less-accessible type (CS0051), even with - // InternalsVisibleTo. The property is read via reflection inside the test body instead. public static IEnumerable BoolPolicyNames() => new[] { - new object[] { nameof(IPolicies.TelemetryEnabled) }, - new object[] { nameof(IPolicies.UpdateCheckEnabled) }, - new object[] { nameof(IPolicies.UseSystemBrowserForAuthentication) }, - new object[] { nameof(IPolicies.BuiltInTemplatesEnabled) }, - new object[] { nameof(IPolicies.CustomTemplatesEnabled) }, + new object[] { nameof(PolicySnapshot.TelemetryEnabled) }, + new object[] { nameof(PolicySnapshot.UpdateCheckEnabled) }, + new object[] { nameof(PolicySnapshot.UseSystemBrowserForAuthentication) }, + new object[] { nameof(PolicySnapshot.BuiltInTemplatesEnabled) }, + new object[] { nameof(PolicySnapshot.CustomTemplatesEnabled) }, }; - private static bool? GetBoolProperty(Sqlbi.Bravo.Infrastructure.Policies.Policies policies, string propertyName) - => (bool?)typeof(Sqlbi.Bravo.Infrastructure.Policies.Policies).GetProperty(propertyName)!.GetValue(policies); + private static bool? GetBoolProperty(PolicySnapshot policies, string propertyName) + => (bool?)typeof(PolicySnapshot).GetProperty(propertyName)!.GetValue(policies); [Fact] public void FromSource_EmptySource_AllPropertiesAreNull() { - var policies = PoliciesFactory.FromSource(new FakePolicySource()); + var policies = RegistryPolicyReader.FromSource(new FakePolicySource()); Assert.Null(policies.TelemetryEnabled); Assert.Null(policies.UpdateChannel); @@ -62,7 +40,7 @@ public void FromSource_BoolPolicyValueOne_ReturnsTrue(string propertyName) var source = new FakePolicySource(); source.Set(propertyName, 1); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.True(GetBoolProperty(policies, propertyName)); } @@ -74,7 +52,7 @@ public void FromSource_BoolPolicyValueZero_ReturnsFalse(string propertyName) var source = new FakePolicySource(); source.Set(propertyName, 0); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.False(GetBoolProperty(policies, propertyName)); } @@ -86,7 +64,7 @@ public void FromSource_BoolPolicyOutOfRangeIntValue_ReturnsNull(string propertyN var source = new FakePolicySource(); source.Set(propertyName, 42); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.Null(GetBoolProperty(policies, propertyName)); } @@ -98,7 +76,7 @@ public void FromSource_BoolPolicyNonIntValue_ReturnsNull(string propertyName) var source = new FakePolicySource(); source.Set(propertyName, "not-a-number"); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.Null(GetBoolProperty(policies, propertyName)); } @@ -109,7 +87,7 @@ public void FromSource_UpdateChannelDefinedEnumValue_ReturnsParsedValue() var source = new FakePolicySource(); source.Set("UpdateChannel", (int)UpdateChannelType.Dev); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.Equal(UpdateChannelType.Dev, policies.UpdateChannel); } @@ -121,7 +99,7 @@ public void FromSource_UpdateChannelUndefinedEnumValue_ReturnsNull() var source = new FakePolicySource(); source.Set("UpdateChannel", 1); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.Null(policies.UpdateChannel); } @@ -132,7 +110,7 @@ public void FromSource_UpdateChannelNonIntValue_ReturnsNull() var source = new FakePolicySource(); source.Set("UpdateChannel", "Dev"); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.Null(policies.UpdateChannel); } @@ -143,7 +121,7 @@ public void FromSource_CustomTemplatesOrganizationRepositoryPathStringValueSet_R var source = new FakePolicySource(); source.Set("CustomTemplatesOrganizationRepositoryPath", @"C:\Templates\Org"); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.Equal(@"C:\Templates\Org", policies.CustomTemplatesOrganizationRepositoryPath); } @@ -154,24 +132,12 @@ public void FromSource_CustomTemplatesOrganizationRepositoryPathNonStringValue_R var source = new FakePolicySource(); source.Set("CustomTemplatesOrganizationRepositoryPath", 123); - var policies = PoliciesFactory.FromSource(source); + var policies = RegistryPolicyReader.FromSource(source); Assert.Null(policies.CustomTemplatesOrganizationRepositoryPath); } - [Fact] - public void Create_DoesNotThrow_AndReturnsAnInstance() - { - // Create() reads from the real HKLM/HKCU policy path, so its output depends on the - // machine's actual group-policy configuration and cannot be asserted deterministically here. - // This is a smoke test for the composition wiring (LocalMachine + CurrentUser -> Merge); - // the precedence rule itself is covered deterministically by the Merge tests below. - var policies = PoliciesFactory.Create(); - - Assert.NotNull(policies); - } - - private static readonly Sqlbi.Bravo.Infrastructure.Policies.Policies AllNull = new( + private static readonly PolicySnapshot AllNull = new( TelemetryEnabled: null, UpdateChannel: null, UpdateCheckEnabled: null, @@ -186,7 +152,7 @@ public void Merge_OnlyMachineValueSet_ReturnsMachineValue() var machine = AllNull with { TelemetryEnabled = true }; var user = AllNull; - var merged = PoliciesFactory.Merge(machine, user); + var merged = RegistryPolicyReader.Merge(machine, user); Assert.True(merged.TelemetryEnabled); } @@ -197,7 +163,7 @@ public void Merge_OnlyUserValueSet_ReturnsUserValue() var machine = AllNull; var user = AllNull with { TelemetryEnabled = false }; - var merged = PoliciesFactory.Merge(machine, user); + var merged = RegistryPolicyReader.Merge(machine, user); Assert.False(merged.TelemetryEnabled); } @@ -208,7 +174,7 @@ public void Merge_BothSet_MachineTakesPrecedenceOverUser() var machine = AllNull with { TelemetryEnabled = true, UpdateChannel = UpdateChannelType.Stable, UpdateCheckEnabled = false }; var user = AllNull with { TelemetryEnabled = false, UpdateChannel = UpdateChannelType.Dev, UpdateCheckEnabled = true }; - var merged = PoliciesFactory.Merge(machine, user); + var merged = RegistryPolicyReader.Merge(machine, user); Assert.True(merged.TelemetryEnabled); Assert.Equal(UpdateChannelType.Stable, merged.UpdateChannel); @@ -218,7 +184,7 @@ public void Merge_BothSet_MachineTakesPrecedenceOverUser() [Fact] public void Merge_NeitherSet_ReturnsNull() { - var merged = PoliciesFactory.Merge(AllNull, AllNull); + var merged = RegistryPolicyReader.Merge(AllNull, AllNull); Assert.Null(merged.TelemetryEnabled); } @@ -229,7 +195,7 @@ public void Merge_EachPropertyResolvedIndependently() // Guards against a copy-paste wiring mistake in Merge() (e.g. reading the wrong // property from machine/user) by exercising all 7 properties in a single assertion, // each with a distinct machine/user combination. - var machine = new Sqlbi.Bravo.Infrastructure.Policies.Policies( + var machine = new PolicySnapshot( TelemetryEnabled: true, UpdateChannel: null, UpdateCheckEnabled: null, @@ -238,7 +204,7 @@ public void Merge_EachPropertyResolvedIndependently() CustomTemplatesEnabled: null, CustomTemplatesOrganizationRepositoryPath: null); - var user = new Sqlbi.Bravo.Infrastructure.Policies.Policies( + var user = new PolicySnapshot( TelemetryEnabled: false, // machine wins UpdateChannel: UpdateChannelType.Dev, // machine unset -> user wins UpdateCheckEnabled: true, // machine unset -> user wins @@ -247,7 +213,7 @@ public void Merge_EachPropertyResolvedIndependently() CustomTemplatesEnabled: null, // neither set CustomTemplatesOrganizationRepositoryPath: @"C:\User\Path"); // machine unset -> user wins - var merged = PoliciesFactory.Merge(machine, user); + var merged = RegistryPolicyReader.Merge(machine, user); Assert.True(merged.TelemetryEnabled); Assert.Equal(UpdateChannelType.Dev, merged.UpdateChannel); diff --git a/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicySourceTests.cs b/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicySourceTests.cs index 68261a3e..dee23141 100644 --- a/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicySourceTests.cs +++ b/test/Bravo.Tests/Infrastructure/Policies/RegistryPolicySourceTests.cs @@ -6,10 +6,7 @@ namespace Bravo.Tests.Infrastructure.Policies; /// -/// Covers only the thin RegistryKey-to-IPolicySource bridging contract (missing key, missing -/// value, type mismatches). Uses a real, isolated key under HKEY_CURRENT_USER (writable without -/// admin rights) since RegistryKey is a sealed BCL type that cannot be faked. The -/// parsing/precedence logic that matters is covered by PoliciesTests against a fake source instead. +/// Verifies registry value types and handle ownership with an isolated key under HKEY_CURRENT_USER. /// public class RegistryPolicySourceTests : IDisposable { @@ -30,7 +27,7 @@ public void Dispose() [Fact] public void GetInt_NullKey_ReturnsNull() { - var source = new RegistryPolicySource(key: null); + using var source = new RegistryPolicySource(key: null); Assert.Null(source.GetInt("AnyValue")); } @@ -38,7 +35,7 @@ public void GetInt_NullKey_ReturnsNull() [Fact] public void GetInt_ValueNotSet_ReturnsNull() { - var source = new RegistryPolicySource(_testKey); + using var source = new RegistryPolicySource(_testKey); Assert.Null(source.GetInt("Missing")); } @@ -47,7 +44,7 @@ public void GetInt_ValueNotSet_ReturnsNull() public void GetInt_DWordValue_ReturnsInt() { _testKey.SetValue("TelemetryEnabled", 1, RegistryValueKind.DWord); - var source = new RegistryPolicySource(_testKey); + using var source = new RegistryPolicySource(_testKey); Assert.Equal(1, source.GetInt("TelemetryEnabled")); } @@ -56,7 +53,7 @@ public void GetInt_DWordValue_ReturnsInt() public void GetInt_StringValue_ReturnsNull() { _testKey.SetValue("TelemetryEnabled", "1", RegistryValueKind.String); - var source = new RegistryPolicySource(_testKey); + using var source = new RegistryPolicySource(_testKey); Assert.Null(source.GetInt("TelemetryEnabled")); } @@ -64,7 +61,7 @@ public void GetInt_StringValue_ReturnsNull() [Fact] public void GetString_NullKey_ReturnsNull() { - var source = new RegistryPolicySource(key: null); + using var source = new RegistryPolicySource(key: null); Assert.Null(source.GetString("AnyValue")); } @@ -73,7 +70,7 @@ public void GetString_NullKey_ReturnsNull() public void GetString_StringValue_ReturnsString() { _testKey.SetValue("CustomTemplatesOrganizationRepositoryPath", @"C:\Templates\Org", RegistryValueKind.String); - var source = new RegistryPolicySource(_testKey); + using var source = new RegistryPolicySource(_testKey); Assert.Equal(@"C:\Templates\Org", source.GetString("CustomTemplatesOrganizationRepositoryPath")); } @@ -82,8 +79,18 @@ public void GetString_StringValue_ReturnsString() public void GetString_DWordValue_ReturnsNull() { _testKey.SetValue("CustomTemplatesOrganizationRepositoryPath", 123, RegistryValueKind.DWord); - var source = new RegistryPolicySource(_testKey); + using var source = new RegistryPolicySource(_testKey); Assert.Null(source.GetString("CustomTemplatesOrganizationRepositoryPath")); } + + [Fact] + public void Dispose_ClosesTheOwnedRegistryKey() + { + var source = new RegistryPolicySource(_testKey); + + source.Dispose(); + + Assert.Throws(() => _testKey.GetValue("AnyValue")); + } }