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 <nel@umbraco.dk>
Co-authored-by: Bjarke Berg <mail@bergmania.dk>
This commit is contained in:
Mole
2022-02-28 13:59:39 +01:00
committed by GitHub
parent 4a6c409a1f
commit 0c7ef06031
10 changed files with 287 additions and 80 deletions

View File

@@ -262,6 +262,7 @@ namespace Umbraco.Cms.Core.DependencyInjection
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

View File

@@ -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;
}
[Obsolete("Use constructor that takes GlobalSettings and ISiteIdentifierService")]
public TelemetryIdentifierStep(
ILogger<TelemetryIdentifierStep> logger,
IOptions<GlobalSettings> globalSettings,
IConfigManipulator configManipulator)
: this(globalSettings, StaticServiceProvider.Instance.GetRequiredService<ISiteIdentifierService>())
{
}
public override Task<InstallSetupResult> ExecuteAsync(object model)
{
// 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");
}
_siteIdentifierService.TryCreateSiteIdentifier(out _);
return Task.FromResult<InstallSetupResult>(null);
}

View File

@@ -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);
}
}

View File

@@ -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;
}
}
}

View File

@@ -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 @@ namespace Umbraco.Cms.Core.Telemetry
{
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 @@ namespace Umbraco.Cms.Core.Telemetry
packages.Add(new PackageTelemetry
{
Name = manifest.PackageName,
Version = manifest.Version ?? string.Empty
Version = manifest.Version ?? string.Empty,
});
}

View File

@@ -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 @@ namespace Umbraco.Cms.Infrastructure.DependencyInjection
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>();

View File

@@ -59,9 +59,6 @@ namespace Umbraco.Cms.Infrastructure.HostedServices
// 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/");

View File

@@ -7,6 +7,7 @@ using System.Text;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Newtonsoft.Json;
@@ -19,10 +20,12 @@ using Umbraco.Cms.Core.Models.ContentEditing;
using Umbraco.Cms.Core.Security;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Strings;
using Umbraco.Cms.Core.Telemetry;
using Umbraco.Cms.Web.BackOffice.Filters;
using Umbraco.Cms.Web.Common.Attributes;
using Umbraco.Cms.Web.Common.Authorization;
using Umbraco.Cms.Web.Common.Controllers;
using Umbraco.Cms.Web.Common.DependencyInjection;
using Umbraco.Cms.Web.Common.Filters;
using Umbraco.Extensions;
using Constants = Umbraco.Cms.Core.Constants;
@@ -43,10 +46,13 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers
private readonly IDashboardService _dashboardService;
private readonly IUmbracoVersion _umbracoVersion;
private readonly IShortStringHelper _shortStringHelper;
private readonly ISiteIdentifierService _siteIdentifierService;
private readonly ContentDashboardSettings _dashboardSettings;
/// <summary>
/// Initializes a new instance of the <see cref="DashboardController"/> with all its dependencies.
/// </summary>
[ActivatorUtilitiesConstructor]
public DashboardController(
IBackOfficeSecurityAccessor backOfficeSecurityAccessor,
AppCaches appCaches,
@@ -54,7 +60,8 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers
IDashboardService dashboardService,
IUmbracoVersion umbracoVersion,
IShortStringHelper shortStringHelper,
IOptions<ContentDashboardSettings> dashboardSettings)
IOptions<ContentDashboardSettings> dashboardSettings,
ISiteIdentifierService siteIdentifierService)
{
_backOfficeSecurityAccessor = backOfficeSecurityAccessor;
@@ -63,9 +70,32 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers
_dashboardService = dashboardService;
_umbracoVersion = umbracoVersion;
_shortStringHelper = shortStringHelper;
_siteIdentifierService = siteIdentifierService;
_dashboardSettings = dashboardSettings.Value;
}
[Obsolete("Use the constructor that accepts ISiteIdentifierService")]
public DashboardController(
IBackOfficeSecurityAccessor backOfficeSecurityAccessor,
AppCaches appCaches,
ILogger<DashboardController> logger,
IDashboardService dashboardService,
IUmbracoVersion umbracoVersion,
IShortStringHelper shortStringHelper,
IOptions<ContentDashboardSettings> dashboardSettings)
: this(
backOfficeSecurityAccessor,
appCaches,
logger,
dashboardService,
umbracoVersion,
shortStringHelper,
dashboardSettings,
StaticServiceProvider.Instance.GetRequiredService<ISiteIdentifierService>())
{
}
//we have just one instance of HttpClient shared for the entire application
private static readonly HttpClient HttpClient = new HttpClient();
@@ -79,6 +109,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers
var language = user.Language;
var version = _umbracoVersion.SemanticVersion.ToSemanticStringWithoutBuild();
var isAdmin = user.IsAdmin();
_siteIdentifierService.TryGetOrCreateSiteIdentifier(out Guid siteIdentifier);
if (!IsAllowedUrl(baseUrl))
{
@@ -90,14 +121,15 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers
return JObject.Parse(errorJson);
}
var url = string.Format("{0}{1}?section={2}&allowed={3}&lang={4}&version={5}&admin={6}",
var url = string.Format("{0}{1}?section={2}&allowed={3}&lang={4}&version={5}&admin={6}&siteid={7}",
baseUrl,
_dashboardSettings.ContentDashboardPath,
section,
allowedSections,
language,
version,
isAdmin);
isAdmin,
siteIdentifier);
var key = "umbraco-dynamic-dashboard-" + language + allowedSections.Replace(",", "-") + section;
var content = _appCaches.RuntimeCache.GetCacheItem<JObject>(key);

View File

@@ -0,0 +1,77 @@
using System;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Moq;
using NUnit.Framework;
using Umbraco.Cms.Core.Configuration;
using Umbraco.Cms.Core.Configuration.Models;
using Umbraco.Cms.Core.Telemetry;
namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Telemetry
{
[TestFixture]
public class SiteIdentifierServiceTests
{
[TestCase("0F1785C5-7BA0-4C52-AB62-863BD2C8F3FE", true)]
[TestCase("This is not a guid", false)]
[TestCase("", false)]
[TestCase("00000000-0000-0000-0000-000000000000", false)] // Don't count empty GUID as valid
public void TryGetOnlyPassesIfValidId(string guidString, bool shouldSucceed)
{
var globalSettings = CreateGlobalSettings(guidString);
var sut = new SiteIdentifierService(
globalSettings,
Mock.Of<IConfigManipulator>(),
Mock.Of<ILogger<SiteIdentifierService>>());
var result = sut.TryGetSiteIdentifier(out var siteIdentifier);
Assert.AreEqual(shouldSucceed, result);
if (shouldSucceed)
{
// When toString is called on a GUID it will to lower, so do the same to our guidString
Assert.AreEqual(guidString.ToLower(), siteIdentifier.ToString());
}
else
{
Assert.AreEqual(Guid.Empty, siteIdentifier);
}
}
[TestCase("0F1785C5-7BA0-4C52-AB62-863BD2C8F3FE", false)]
[TestCase("This is not a guid", true)]
[TestCase("", true)]
[TestCase("00000000-0000-0000-0000-000000000000", true)] // Don't count empty GUID as valid
public void TryGetOrCreateOnlyCreatesNewGuidIfCurrentIsMissingOrInvalid(string guidString, bool shouldCreate)
{
var globalSettings = CreateGlobalSettings(guidString);
var configManipulatorMock = new Mock<IConfigManipulator>();
var sut = new SiteIdentifierService(
globalSettings,
configManipulatorMock.Object,
Mock.Of<ILogger<SiteIdentifierService>>());
var result = sut.TryGetOrCreateSiteIdentifier(out var identifier);
if (shouldCreate)
{
configManipulatorMock.Verify(x => x.SetGlobalId(It.IsAny<string>()), Times.Once);
Assert.AreNotEqual(Guid.Empty, identifier);
Assert.IsTrue(result);
}
else
{
configManipulatorMock.Verify(x => x.SetGlobalId(It.IsAny<string>()), Times.Never());
Assert.AreEqual(guidString.ToLower(), identifier.ToString());
Assert.IsTrue(result);
}
}
private IOptionsMonitor<GlobalSettings> CreateGlobalSettings(string guidString)
{
var globalSettings = new GlobalSettings { Id = guidString };
return Mock.Of<IOptionsMonitor<GlobalSettings>>(x => x.CurrentValue == globalSettings);
}
}
}

View File

@@ -15,36 +15,36 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Telemetry
[TestFixture]
public class TelemetryServiceTests
{
[TestCase("0F1785C5-7BA0-4C52-AB62-863BD2C8F3FE", true)]
[TestCase("This is not a guid", false)]
[TestCase("", false)]
public void OnlyParsesIfValidId(string guidString, bool shouldSucceed)
[Test]
public void UsesGetOrCreateSiteId()
{
var globalSettings = CreateGlobalSettings(guidString);
var version = CreateUmbracoVersion(9, 1, 1);
var sut = new TelemetryService(globalSettings, Mock.Of<IManifestParser>(), version);
var version = CreateUmbracoVersion(9, 3, 1);
var siteIdentifierServiceMock = new Mock<ISiteIdentifierService>();
var sut = new TelemetryService(Mock.Of<IManifestParser>(), version, siteIdentifierServiceMock.Object);
Guid guid;
var result = sut.TryGetTelemetryReportData(out var telemetryReportData);
siteIdentifierServiceMock.Verify(x => x.TryGetOrCreateSiteIdentifier(out guid), Times.Once);
}
[Test]
public void SkipsIfCantGetOrCreateId()
{
var version = CreateUmbracoVersion(9, 3, 1);
var sut = new TelemetryService(Mock.Of<IManifestParser>(), version, createSiteIdentifierService(false));
var result = sut.TryGetTelemetryReportData(out var telemetry);
Assert.AreEqual(shouldSucceed, result);
if (shouldSucceed)
{
// When toString is called on a GUID it will to lower, so do the same to our guidString
Assert.AreEqual(guidString.ToLower(), telemetry.Id.ToString());
}
else
{
Assert.IsNull(telemetry);
}
Assert.IsFalse(result);
Assert.IsNull(telemetry);
}
[Test]
public void ReturnsSemanticVersionWithoutBuild()
{
var globalSettings = CreateGlobalSettings();
var version = CreateUmbracoVersion(9, 1, 1, "-rc", "-ad2f4k2d");
var sut = new TelemetryService(globalSettings, Mock.Of<IManifestParser>(), version);
var sut = new TelemetryService(Mock.Of<IManifestParser>(), version, createSiteIdentifierService());
var result = sut.TryGetTelemetryReportData(out var telemetry);
@@ -55,7 +55,6 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Telemetry
[Test]
public void CanGatherPackageTelemetry()
{
var globalSettings = CreateGlobalSettings();
var version = CreateUmbracoVersion(9, 1, 1);
var versionPackageName = "VersionPackage";
var packageVersion = "1.0.0";
@@ -66,7 +65,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Telemetry
new () { PackageName = noVersionPackageName }
};
var manifestParser = CreateManifestParser(manifests);
var sut = new TelemetryService(globalSettings, manifestParser, version);
var sut = new TelemetryService(manifestParser, version, createSiteIdentifierService());
var success = sut.TryGetTelemetryReportData(out var telemetry);
@@ -87,15 +86,14 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Telemetry
[Test]
public void RespectsAllowPackageTelemetry()
{
var globalSettings = CreateGlobalSettings();
var version = CreateUmbracoVersion(9, 1, 1);
PackageManifest[] manifests =
{
new () { PackageName = "DoNotTrack", AllowPackageTelemetry = false },
new () { PackageName = "TrackingAllowed", AllowPackageTelemetry = true }
new () { PackageName = "TrackingAllowed", AllowPackageTelemetry = true },
};
var manifestParser = CreateManifestParser(manifests);
var sut = new TelemetryService(globalSettings, manifestParser, version);
var sut = new TelemetryService(manifestParser, version, createSiteIdentifierService());
var success = sut.TryGetTelemetryReportData(out var telemetry);
@@ -121,15 +119,12 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Telemetry
return Mock.Of<IUmbracoVersion>(x => x.SemanticVersion == version);
}
private IOptionsMonitor<GlobalSettings> CreateGlobalSettings(string guidString = null)
private ISiteIdentifierService createSiteIdentifierService(bool shouldSucceed = true)
{
if (guidString is null)
{
guidString = Guid.NewGuid().ToString();
}
var globalSettings = new GlobalSettings { Id = guidString };
return Mock.Of<IOptionsMonitor<GlobalSettings>>(x => x.CurrentValue == globalSettings);
var mock = new Mock<ISiteIdentifierService>();
var siteIdentifier = shouldSucceed ? Guid.NewGuid() : Guid.Empty;
mock.Setup(x => x.TryGetOrCreateSiteIdentifier(out siteIdentifier)).Returns(shouldSucceed);
return mock.Object;
}
}
}