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")); + } }