V9: Fix missing site identifier (#12040)

* Add SiteIdentifierService

* Use SiteIdentifierService in TelemetryService

* Use SiteIdentifierService when installing

* Remove timeout

* Use TryGetOrCreateSiteIdentifier in TelemetryService

* Add site identifier to dashboard url

* Fix and add tests

* Don't accept empty guid as valid site identifier

* Fix dashboard controller

* Fix site id query parameter

* Use Optionsmonitor onchange

Co-authored-by: nikolajlauridsen <[email protected]>
Co-authored-by: Bjarke Berg <[email protected]>

Author: 3 people

src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs

@@ -262,6 +262,7 @@ private void AddCoreServices()
             Services.AddSingleton<IValueEditorCache, ValueEditorCache>();
 
             // Register telemetry service used to gather data about installed packages
+            Services.AddUnique<ISiteIdentifierService, SiteIdentifierService>();
             Services.AddUnique<ITelemetryService, TelemetryService>();
 
             // Register a noop IHtmlSanitizer to be replaced

src/Umbraco.Core/Install/InstallSteps/TelemetryIdentifierStep.cs

@@ -1,10 +1,13 @@
 using System;
 using System.Threading.Tasks;
+using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using Umbraco.Cms.Core.Configuration;
 using Umbraco.Cms.Core.Configuration.Models;
 using Umbraco.Cms.Core.Install.Models;
+using Umbraco.Cms.Core.Telemetry;
+using Umbraco.Cms.Web.Common.DependencyInjection;
 
 namespace Umbraco.Cms.Core.Install.InstallSteps
 {
@@ -13,31 +16,29 @@ namespace Umbraco.Cms.Core.Install.InstallSteps
         PerformsAppRestart = false)]
     public class TelemetryIdentifierStep : InstallSetupStep<object>
     {
-        private readonly ILogger<TelemetryIdentifierStep> _logger;
         private readonly IOptions<GlobalSettings> _globalSettings;
-        private readonly IConfigManipulator _configManipulator;
+        private readonly ISiteIdentifierService _siteIdentifierService;
 
-        public TelemetryIdentifierStep(ILogger<TelemetryIdentifierStep> logger, IOptions<GlobalSettings> globalSettings, IConfigManipulator configManipulator)
+        public TelemetryIdentifierStep(
+            IOptions<GlobalSettings> globalSettings,
+            ISiteIdentifierService siteIdentifierService)
         {
-            _logger = logger;
             _globalSettings = globalSettings;
-            _configManipulator = configManipulator;
+            _siteIdentifierService = siteIdentifierService;
         }
 
-        public override Task<InstallSetupResult> ExecuteAsync(object model)
+        [Obsolete("Use constructor that takes GlobalSettings and ISiteIdentifierService")]
+        public TelemetryIdentifierStep(
+            ILogger<TelemetryIdentifierStep> logger,
+            IOptions<GlobalSettings> globalSettings,
+            IConfigManipulator configManipulator)
+        : this(globalSettings, StaticServiceProvider.Instance.GetRequiredService<ISiteIdentifierService>())
         {
-            // Generate GUID
-            var telemetrySiteIdentifier = Guid.NewGuid();
-
-            try
-            {
-                _configManipulator.SetGlobalId(telemetrySiteIdentifier.ToString());
-            }
-            catch (Exception ex)
-            {
-                _logger.LogError(ex, "Couldn't update config files with a telemetry site identifier");
-            }
+        }
 
+        public override Task<InstallSetupResult> ExecuteAsync(object model)
+        {
+            _siteIdentifierService.TryCreateSiteIdentifier(out _);
             return Task.FromResult<InstallSetupResult>(null);
         }
 

src/Umbraco.Core/Telemetry/ISiteIdentifierService.cs

@@ -0,0 +1,31 @@
+using System;
+
+namespace Umbraco.Cms.Core.Telemetry
+{
+    /// <summary>
+    /// Used to get and create the site identifier
+    /// </summary>
+    public interface ISiteIdentifierService
+    {
+
+        /// <summary>
+        /// Tries to get the site identifier
+        /// </summary>
+        /// <returns>True if success.</returns>
+        bool TryGetSiteIdentifier(out Guid siteIdentifier);
+
+        /// <summary>
+        /// Creates the site identifier and writes it to config.
+        /// </summary>
+        /// <param name="createdGuid">asd.</param>
+        /// <returns>True if success.</returns>
+        bool TryCreateSiteIdentifier(out Guid createdGuid);
+
+        /// <summary>
+        /// Tries to get the site identifier or otherwise create it if it doesn't exist.
+        /// </summary>
+        /// <param name="siteIdentifier">The out parameter for the existing or create site identifier.</param>
+        /// <returns>True if success.</returns>
+        bool TryGetOrCreateSiteIdentifier(out Guid siteIdentifier);
+    }
+}

src/Umbraco.Core/Telemetry/SiteIdentifierService.cs

@@ -0,0 +1,81 @@
+using System;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Options;
+using Umbraco.Cms.Core.Configuration;
+using Umbraco.Cms.Core.Configuration.Models;
+
+namespace Umbraco.Cms.Core.Telemetry
+{
+    /// <inheritdoc />
+    internal class SiteIdentifierService : ISiteIdentifierService
+    {
+        private GlobalSettings _globalSettings;
+        private readonly IConfigManipulator _configManipulator;
+        private readonly ILogger<SiteIdentifierService> _logger;
+
+        public SiteIdentifierService(
+            IOptionsMonitor<GlobalSettings> optionsMonitor,
+            IConfigManipulator configManipulator,
+            ILogger<SiteIdentifierService> logger)
+        {
+            _globalSettings = optionsMonitor.CurrentValue;
+            optionsMonitor.OnChange(globalSettings => _globalSettings = globalSettings);
+            _configManipulator = configManipulator;
+            _logger = logger;
+        }
+
+        /// <inheritdoc/>
+        public bool TryGetSiteIdentifier(out Guid siteIdentifier)
+        {
+            // Parse telemetry string as a GUID & verify its a GUID and not some random string
+            // since users may have messed with or decided to empty the app setting or put in something random
+            if (Guid.TryParse(_globalSettings.Id, out var parsedTelemetryId) is false
+                || parsedTelemetryId == Guid.Empty)
+            {
+                siteIdentifier = Guid.Empty;
+                return false;
+            }
+
+            siteIdentifier = parsedTelemetryId;
+            return true;
+        }
+
+        /// <inheritdoc/>
+        public bool TryGetOrCreateSiteIdentifier(out Guid siteIdentifier)
+        {
+            if (TryGetSiteIdentifier(out Guid existingId))
+            {
+                siteIdentifier = existingId;
+                return true;
+            }
+
+            if (TryCreateSiteIdentifier(out Guid createdId))
+            {
+                siteIdentifier = createdId;
+                return true;
+            }
+
+            siteIdentifier = Guid.Empty;
+            return false;
+        }
+
+        /// <inheritdoc/>
+        public bool TryCreateSiteIdentifier(out Guid createdGuid)
+        {
+            createdGuid = Guid.NewGuid();
+
+            try
+            {
+                _configManipulator.SetGlobalId(createdGuid.ToString());
+            }
+            catch (Exception ex)
+            {
+                _logger.LogError(ex, "Couldn't update config files with a telemetry site identifier");
+                createdGuid = Guid.Empty;
+                return false;
+            }
+
+            return true;
+        }
+    }
+}

src/Umbraco.Core/Telemetry/TelemetryService.cs

@@ -1,8 +1,6 @@
 using System;
 using System.Collections.Generic;
-using Microsoft.Extensions.Options;
 using Umbraco.Cms.Core.Configuration;
-using Umbraco.Cms.Core.Configuration.Models;
 using Umbraco.Cms.Core.Manifest;
 using Umbraco.Cms.Core.Telemetry.Models;
 using Umbraco.Extensions;
@@ -12,27 +10,27 @@ namespace Umbraco.Cms.Core.Telemetry
     /// <inheritdoc/>
     internal class TelemetryService : ITelemetryService
     {
-        private readonly IOptionsMonitor<GlobalSettings> _globalSettings;
         private readonly IManifestParser _manifestParser;
         private readonly IUmbracoVersion _umbracoVersion;
+        private readonly ISiteIdentifierService _siteIdentifierService;
 
         /// <summary>
         /// Initializes a new instance of the <see cref="TelemetryService"/> class.
         /// </summary>
         public TelemetryService(
-            IOptionsMonitor<GlobalSettings> globalSettings,
             IManifestParser manifestParser,
-            IUmbracoVersion umbracoVersion)
+            IUmbracoVersion umbracoVersion,
+            ISiteIdentifierService siteIdentifierService)
         {
             _manifestParser = manifestParser;
             _umbracoVersion = umbracoVersion;
-            _globalSettings = globalSettings;
+            _siteIdentifierService = siteIdentifierService;
         }
 
         /// <inheritdoc/>
         public bool TryGetTelemetryReportData(out TelemetryReportData telemetryReportData)
         {
-            if (TryGetTelemetryId(out Guid telemetryId) is false)
+            if (_siteIdentifierService.TryGetOrCreateSiteIdentifier(out Guid telemetryId) is false)
             {
                 telemetryReportData = null;
                 return false;
@@ -42,28 +40,14 @@ public bool TryGetTelemetryReportData(out TelemetryReportData telemetryReportDat
             {
                 Id = telemetryId,
                 Version = _umbracoVersion.SemanticVersion.ToSemanticStringWithoutBuild(),
-                Packages = GetPackageTelemetry()
+                Packages = GetPackageTelemetry(),
             };
             return true;
         }
 
-        private bool TryGetTelemetryId(out Guid telemetryId)
-        {
-            // Parse telemetry string as a GUID & verify its a GUID and not some random string
-            // since users may have messed with or decided to empty the app setting or put in something random
-            if (Guid.TryParse(_globalSettings.CurrentValue.Id, out var parsedTelemetryId) is false)
-            {
-                telemetryId = Guid.Empty;
-                return false;
-            }
-
-            telemetryId = parsedTelemetryId;
-            return true;
-        }
-
         private IEnumerable<PackageTelemetry> GetPackageTelemetry()
         {
-            List<PackageTelemetry> packages = new ();
+            List<PackageTelemetry> packages = new();
             IEnumerable<PackageManifest> manifests = _manifestParser.GetManifests();
 
             foreach (PackageManifest manifest in manifests)
@@ -76,7 +60,7 @@ private IEnumerable<PackageTelemetry> GetPackageTelemetry()
                 packages.Add(new PackageTelemetry
                 {
                     Name = manifest.PackageName,
-                    Version = manifest.Version ?? string.Empty
+                    Version = manifest.Version ?? string.Empty,
                 });
             }
 

src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Installer.cs

@@ -1,7 +1,10 @@
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Options;
+using Umbraco.Cms.Core.Configuration.Models;
 using Umbraco.Cms.Core.DependencyInjection;
 using Umbraco.Cms.Core.Install.InstallSteps;
 using Umbraco.Cms.Core.Install.Models;
+using Umbraco.Cms.Core.Telemetry;
 using Umbraco.Cms.Infrastructure.Install;
 using Umbraco.Cms.Infrastructure.Install.InstallSteps;
 using Umbraco.Extensions;
@@ -19,7 +22,12 @@ internal static IUmbracoBuilder AddInstaller(this IUmbracoBuilder builder)
             builder.Services.AddScoped<InstallSetupStep, NewInstallStep>();
             builder.Services.AddScoped<InstallSetupStep, UpgradeStep>();
             builder.Services.AddScoped<InstallSetupStep, FilePermissionsStep>();
-            builder.Services.AddScoped<InstallSetupStep, TelemetryIdentifierStep>();
+            builder.Services.AddScoped<InstallSetupStep, TelemetryIdentifierStep>(provider =>
+            {
+                return new TelemetryIdentifierStep(
+                    provider.GetRequiredService<IOptions<GlobalSettings>>(),
+                    provider.GetRequiredService<ISiteIdentifierService>());
+            });
             builder.Services.AddScoped<InstallSetupStep, DatabaseConfigureStep>();
             builder.Services.AddScoped<InstallSetupStep, DatabaseInstallStep>();
             builder.Services.AddScoped<InstallSetupStep, DatabaseUpgradeStep>();

src/Umbraco.Infrastructure/HostedServices/ReportSiteTask.cs

@@ -59,9 +59,6 @@ public override async Task PerformExecuteAsync(object state)
                     // Send data to LIVE telemetry
                     s_httpClient.BaseAddress = new Uri("https://telemetry.umbraco.com/");
 
-                    // Set a low timeout - no need to use a larger default timeout for this POST request
-                    s_httpClient.Timeout = new TimeSpan(0, 0, 1);
-
 #if DEBUG
                     // Send data to DEBUG telemetry service
                     s_httpClient.BaseAddress = new Uri("https://telemetry.rainbowsrock.net/");