From 769f24aaae00dca885ce64d459a64a2c2eac04f2 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 9 Sep 2021 13:15:11 -0600 Subject: [PATCH] Fixes reading connection strings after install The issue was discovered when testing Umbraco Id. If you have an existing install and there is a pending Umbraco migration (upgrade), but you clear out the connection string to force a new install screen, fill out the existing connection string details, it all works and redirects to the back office. This will throw exceptions because the runtime state is in an upgrade state (which is fixed in #11064) but if you then logout and try to log in with an active OAuth provider, it means there is a redirect outside of Umbraco and back again and you'll end up back on the installer screen - but it will not show you the upgrade screen, instead it shows you the normal install screen. This is because we are not using IOptionsMonitor for connection string settings which means it's already read the original empty connection string setting fom the very beginning and isn't reading the current/updated value. We need to review all IOptions usages. Most of them should be IOptionsMonitor unless its impossible to change the app behavior at runtime with a particular config option. --- .../UmbracoBuilder.CoreServices.cs | 2 +- .../Install/InstallHelper.cs | 14 +++++--------- .../InstallSteps/DatabaseConfigureStep.cs | 10 +++++----- .../InstallSteps/DatabaseUpgradeStep.cs | 8 ++++---- .../Install/InstallSteps/NewInstallStep.cs | 19 ++++++++++--------- .../Persistence/UmbracoDatabaseFactory.cs | 4 ++-- .../Runtime/SqlMainDomLock.cs | 2 +- .../TestUmbracoDatabaseFactoryProvider.cs | 4 ++-- .../Umbraco.Core/Components/ComponentTests.cs | 4 +--- 9 files changed, 31 insertions(+), 36 deletions(-) diff --git a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs index 7268d7c4db..1ad30c6e8e 100644 --- a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs +++ b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs @@ -219,7 +219,7 @@ namespace Umbraco.Cms.Infrastructure.DependencyInjection builder.Services.AddSingleton(factory => { var globalSettings = factory.GetRequiredService>(); - var connectionStrings = factory.GetRequiredService>(); + var connectionStrings = factory.GetRequiredService>(); var hostingEnvironment = factory.GetRequiredService(); var dbCreator = factory.GetRequiredService(); diff --git a/src/Umbraco.Infrastructure/Install/InstallHelper.cs b/src/Umbraco.Infrastructure/Install/InstallHelper.cs index 2b9c71f28d..944ebbb586 100644 --- a/src/Umbraco.Infrastructure/Install/InstallHelper.cs +++ b/src/Umbraco.Infrastructure/Install/InstallHelper.cs @@ -22,37 +22,33 @@ namespace Umbraco.Cms.Infrastructure.Install { public sealed class InstallHelper { - private static HttpClient _httpClient; private readonly DatabaseBuilder _databaseBuilder; private readonly ILogger _logger; private readonly IUmbracoVersion _umbracoVersion; - private readonly ConnectionStrings _connectionStrings; + private readonly IOptionsMonitor _connectionStrings; private readonly IInstallationService _installationService; private readonly ICookieManager _cookieManager; private readonly IUserAgentProvider _userAgentProvider; private readonly IUmbracoDatabaseFactory _umbracoDatabaseFactory; - private readonly IJsonSerializer _jsonSerializer; private InstallationType? _installationType; public InstallHelper(DatabaseBuilder databaseBuilder, ILogger logger, IUmbracoVersion umbracoVersion, - IOptions connectionStrings, + IOptionsMonitor connectionStrings, IInstallationService installationService, ICookieManager cookieManager, IUserAgentProvider userAgentProvider, - IUmbracoDatabaseFactory umbracoDatabaseFactory, - IJsonSerializer jsonSerializer) + IUmbracoDatabaseFactory umbracoDatabaseFactory) { _logger = logger; _umbracoVersion = umbracoVersion; _databaseBuilder = databaseBuilder; - _connectionStrings = connectionStrings.Value ?? throw new ArgumentNullException(nameof(connectionStrings)); + _connectionStrings = connectionStrings; _installationService = installationService; _cookieManager = cookieManager; _userAgentProvider = userAgentProvider; _umbracoDatabaseFactory = umbracoDatabaseFactory; - _jsonSerializer = jsonSerializer; //We need to initialize the type already, as we can't detect later, if the connection string is added on the fly. GetInstallationType(); @@ -118,7 +114,7 @@ namespace Umbraco.Cms.Infrastructure.Install { get { - var databaseSettings = _connectionStrings.UmbracoConnectionString; + var databaseSettings = _connectionStrings.CurrentValue.UmbracoConnectionString; if (databaseSettings.IsConnectionStringConfigured() == false) { //no version or conn string configured, must be a brand new install diff --git a/src/Umbraco.Infrastructure/Install/InstallSteps/DatabaseConfigureStep.cs b/src/Umbraco.Infrastructure/Install/InstallSteps/DatabaseConfigureStep.cs index 8e0c5601e6..7b02ea786e 100644 --- a/src/Umbraco.Infrastructure/Install/InstallSteps/DatabaseConfigureStep.cs +++ b/src/Umbraco.Infrastructure/Install/InstallSteps/DatabaseConfigureStep.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Threading.Tasks; using Microsoft.Extensions.Logging; @@ -18,12 +18,12 @@ namespace Umbraco.Cms.Infrastructure.Install.InstallSteps { private readonly DatabaseBuilder _databaseBuilder; private readonly ILogger _logger; - private readonly ConnectionStrings _connectionStrings; + private readonly IOptionsMonitor _connectionStrings; - public DatabaseConfigureStep(DatabaseBuilder databaseBuilder, IOptions connectionStrings, ILogger logger) + public DatabaseConfigureStep(DatabaseBuilder databaseBuilder, IOptionsMonitor connectionStrings, ILogger logger) { _databaseBuilder = databaseBuilder; - _connectionStrings = connectionStrings.Value ?? throw new ArgumentNullException(nameof(connectionStrings)); + _connectionStrings = connectionStrings; _logger = logger; } @@ -110,7 +110,7 @@ namespace Umbraco.Cms.Infrastructure.Install.InstallSteps private bool ShouldDisplayView() { //If the connection string is already present in web.config we don't need to show the settings page and we jump to installing/upgrading. - var databaseSettings = _connectionStrings.UmbracoConnectionString; + var databaseSettings = _connectionStrings.CurrentValue.UmbracoConnectionString; if (databaseSettings.IsConnectionStringConfigured()) { diff --git a/src/Umbraco.Infrastructure/Install/InstallSteps/DatabaseUpgradeStep.cs b/src/Umbraco.Infrastructure/Install/InstallSteps/DatabaseUpgradeStep.cs index aba2cdd9f4..929b448286 100644 --- a/src/Umbraco.Infrastructure/Install/InstallSteps/DatabaseUpgradeStep.cs +++ b/src/Umbraco.Infrastructure/Install/InstallSteps/DatabaseUpgradeStep.cs @@ -24,20 +24,20 @@ namespace Umbraco.Cms.Infrastructure.Install.InstallSteps private readonly IRuntimeState _runtime; private readonly ILogger _logger; private readonly IUmbracoVersion _umbracoVersion; - private readonly ConnectionStrings _connectionStrings; + private readonly IOptionsMonitor _connectionStrings; public DatabaseUpgradeStep( DatabaseBuilder databaseBuilder, IRuntimeState runtime, ILogger logger, IUmbracoVersion umbracoVersion, - IOptions connectionStrings) + IOptionsMonitor connectionStrings) { _databaseBuilder = databaseBuilder; _runtime = runtime; _logger = logger; _umbracoVersion = umbracoVersion; - _connectionStrings = connectionStrings.Value ?? throw new ArgumentNullException(nameof(connectionStrings)); + _connectionStrings = connectionStrings; } public override Task ExecuteAsync(object model) @@ -77,7 +77,7 @@ namespace Umbraco.Cms.Infrastructure.Install.InstallSteps return false; } - var databaseSettings = _connectionStrings.UmbracoConnectionString; + var databaseSettings = _connectionStrings.CurrentValue.UmbracoConnectionString; if (databaseSettings.IsConnectionStringConfigured()) { diff --git a/src/Umbraco.Infrastructure/Install/InstallSteps/NewInstallStep.cs b/src/Umbraco.Infrastructure/Install/InstallSteps/NewInstallStep.cs index bf8aaa54ac..ad288dcb3d 100644 --- a/src/Umbraco.Infrastructure/Install/InstallSteps/NewInstallStep.cs +++ b/src/Umbraco.Infrastructure/Install/InstallSteps/NewInstallStep.cs @@ -30,10 +30,10 @@ namespace Umbraco.Cms.Infrastructure.Install.InstallSteps { private readonly IUserService _userService; private readonly DatabaseBuilder _databaseBuilder; - private static HttpClient _httpClient; + private readonly IHttpClientFactory _httpClientFactory; private readonly UserPasswordConfigurationSettings _passwordConfiguration; private readonly SecuritySettings _securitySettings; - private readonly ConnectionStrings _connectionStrings; + private readonly IOptionsMonitor _connectionStrings; private readonly ICookieManager _cookieManager; private readonly IBackOfficeUserManager _userManager; private readonly IDbProviderFactoryCreator _dbProviderFactoryCreator; @@ -41,18 +41,20 @@ namespace Umbraco.Cms.Infrastructure.Install.InstallSteps public NewInstallStep( IUserService userService, DatabaseBuilder databaseBuilder, + IHttpClientFactory httpClientFactory, IOptions passwordConfiguration, IOptions securitySettings, - IOptions connectionStrings, + IOptionsMonitor connectionStrings, ICookieManager cookieManager, IBackOfficeUserManager userManager, IDbProviderFactoryCreator dbProviderFactoryCreator) { _userService = userService ?? throw new ArgumentNullException(nameof(userService)); _databaseBuilder = databaseBuilder ?? throw new ArgumentNullException(nameof(databaseBuilder)); + _httpClientFactory = httpClientFactory; _passwordConfiguration = passwordConfiguration.Value ?? throw new ArgumentNullException(nameof(passwordConfiguration)); _securitySettings = securitySettings.Value ?? throw new ArgumentNullException(nameof(securitySettings)); - _connectionStrings = connectionStrings.Value ?? throw new ArgumentNullException(nameof(connectionStrings)); + _connectionStrings = connectionStrings; _cookieManager = cookieManager; _userManager = userManager ?? throw new ArgumentNullException(nameof(userManager)); _dbProviderFactoryCreator = dbProviderFactoryCreator ?? throw new ArgumentNullException(nameof(dbProviderFactoryCreator)); @@ -89,15 +91,14 @@ namespace Umbraco.Cms.Infrastructure.Install.InstallSteps if (user.SubscribeToNewsLetter) { - if (_httpClient == null) - _httpClient = new HttpClient(); - var values = new NameValueCollection { { "name", admin.Name }, { "email", admin.Email } }; var content = new StringContent(JsonConvert.SerializeObject(values), Encoding.UTF8, "application/json"); + HttpClient httpClient = _httpClientFactory.CreateClient(); + try { - var response = _httpClient.PostAsync("https://shop.umbraco.com/base/Ecom/SubmitEmail/installer.aspx", content).Result; + var response = httpClient.PostAsync("https://shop.umbraco.com/base/Ecom/SubmitEmail/installer.aspx", content).Result; } catch { /* fail in silence */ } } @@ -140,7 +141,7 @@ namespace Umbraco.Cms.Infrastructure.Install.InstallSteps // TODO: we need to do a null check here since this could be entirely missing and we end up with a null ref // exception in the installer. - var databaseSettings = _connectionStrings.UmbracoConnectionString; + var databaseSettings = _connectionStrings.CurrentValue.UmbracoConnectionString; var hasConnString = databaseSettings != null && _databaseBuilder.IsDatabaseConfigured; if (hasConnString) diff --git a/src/Umbraco.Infrastructure/Persistence/UmbracoDatabaseFactory.cs b/src/Umbraco.Infrastructure/Persistence/UmbracoDatabaseFactory.cs index 5c04fbf010..6dfe2ada6b 100644 --- a/src/Umbraco.Infrastructure/Persistence/UmbracoDatabaseFactory.cs +++ b/src/Umbraco.Infrastructure/Persistence/UmbracoDatabaseFactory.cs @@ -80,7 +80,7 @@ namespace Umbraco.Cms.Infrastructure.Persistence ILogger logger, ILoggerFactory loggerFactory, IOptions globalSettings, - IOptions connectionStrings, + IOptionsMonitor connectionStrings, IMapperCollection mappers, IDbProviderFactoryCreator dbProviderFactoryCreator, DatabaseSchemaCreatorFactory databaseSchemaCreatorFactory, @@ -95,7 +95,7 @@ namespace Umbraco.Cms.Infrastructure.Persistence _logger = logger ?? throw new ArgumentNullException(nameof(logger)); _loggerFactory = loggerFactory; - var settings = connectionStrings.Value.UmbracoConnectionString; + var settings = connectionStrings.CurrentValue.UmbracoConnectionString; if (settings == null) { diff --git a/src/Umbraco.Infrastructure/Runtime/SqlMainDomLock.cs b/src/Umbraco.Infrastructure/Runtime/SqlMainDomLock.cs index f206e063a3..5e4597903a 100644 --- a/src/Umbraco.Infrastructure/Runtime/SqlMainDomLock.cs +++ b/src/Umbraco.Infrastructure/Runtime/SqlMainDomLock.cs @@ -45,7 +45,7 @@ namespace Umbraco.Cms.Infrastructure.Runtime ILogger logger, ILoggerFactory loggerFactory, IOptions globalSettings, - IOptions connectionStrings, + IOptionsMonitor connectionStrings, IDbProviderFactoryCreator dbProviderFactoryCreator, IHostingEnvironment hostingEnvironment, DatabaseSchemaCreatorFactory databaseSchemaCreatorFactory, diff --git a/src/Umbraco.Tests.Integration/Testing/TestUmbracoDatabaseFactoryProvider.cs b/src/Umbraco.Tests.Integration/Testing/TestUmbracoDatabaseFactoryProvider.cs index 8d702e82d8..a844e83610 100644 --- a/src/Umbraco.Tests.Integration/Testing/TestUmbracoDatabaseFactoryProvider.cs +++ b/src/Umbraco.Tests.Integration/Testing/TestUmbracoDatabaseFactoryProvider.cs @@ -19,7 +19,7 @@ namespace Umbraco.Cms.Tests.Integration.Testing { private readonly ILoggerFactory _loggerFactory; private readonly IOptions _globalSettings; - private readonly IOptions _connectionStrings; + private readonly IOptionsMonitor _connectionStrings; private readonly IMapperCollection _mappers; private readonly IDbProviderFactoryCreator _dbProviderFactoryCreator; private readonly DatabaseSchemaCreatorFactory _databaseSchemaCreatorFactory; @@ -28,7 +28,7 @@ namespace Umbraco.Cms.Tests.Integration.Testing public TestUmbracoDatabaseFactoryProvider( ILoggerFactory loggerFactory, IOptions globalSettings, - IOptions connectionStrings, + IOptionsMonitor connectionStrings, IMapperCollection mappers, IDbProviderFactoryCreator dbProviderFactoryCreator, DatabaseSchemaCreatorFactory databaseSchemaCreatorFactory, diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Components/ComponentTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Components/ComponentTests.cs index 5a02fbee2b..4173417582 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Components/ComponentTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Components/ComponentTests.cs @@ -22,8 +22,6 @@ using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Hosting; using Umbraco.Cms.Core.IO; using Umbraco.Cms.Core.Logging; -using Umbraco.Cms.Core.Models; -using Umbraco.Cms.Core.PropertyEditors; using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Strings; using Umbraco.Cms.Infrastructure.Migrations.Install; @@ -54,7 +52,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Components loggerFactory.CreateLogger(), loggerFactory, Options.Create(globalSettings), - Options.Create(connectionStrings), + Mock.Of>(x => x.CurrentValue == connectionStrings), new MapperCollection(() => Enumerable.Empty()), TestHelper.DbProviderFactoryCreator, new DatabaseSchemaCreatorFactory(loggerFactory.CreateLogger(), loggerFactory, new UmbracoVersion(), Mock.Of()),