From 566648de3f896c227305937a54bdd2a20b15addc Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Fri, 18 Feb 2022 09:26:47 +0100 Subject: [PATCH 01/22] Add user group tests --- .../cypress/integration/Users/userGroups.js | 35 -------- .../cypress/integration/Users/userGroups.ts | 85 +++++++++++++++++++ .../package-lock.json | 6 +- .../Umbraco.Tests.AcceptanceTest/package.json | 2 +- 4 files changed, 89 insertions(+), 39 deletions(-) delete mode 100644 tests/Umbraco.Tests.AcceptanceTest/cypress/integration/Users/userGroups.js create mode 100644 tests/Umbraco.Tests.AcceptanceTest/cypress/integration/Users/userGroups.ts diff --git a/tests/Umbraco.Tests.AcceptanceTest/cypress/integration/Users/userGroups.js b/tests/Umbraco.Tests.AcceptanceTest/cypress/integration/Users/userGroups.js deleted file mode 100644 index ce2e366f2c..0000000000 --- a/tests/Umbraco.Tests.AcceptanceTest/cypress/integration/Users/userGroups.js +++ /dev/null @@ -1,35 +0,0 @@ -context('User Groups', () => { - - beforeEach(() => { - cy.umbracoLogin(Cypress.env('username'), Cypress.env('password')); - }); - - it('Create user group', () => { - const name = "Test Group"; - - cy.umbracoEnsureUserGroupNameNotExists(name); - - cy.umbracoSection('users'); - cy.get('[data-element="sub-view-userGroups"]').click(); - - cy.umbracoButtonByLabelKey("actions_createGroup").click(); - - //Type name - cy.umbracoEditorHeaderName(name); - - // Assign sections - cy.get('.umb-box:nth-child(1) .umb-property:nth-child(1) localize').click(); - cy.get('.umb-tree-item__inner').click({multiple:true, timeout: 10000}); - cy.get('.btn-success').last().click(); - - // Save - cy.get('.btn-success').click(); - - //Assert - cy.umbracoSuccessNotification().should('be.visible'); - - //Clean up - cy.umbracoEnsureUserGroupNameNotExists(name); - }); - -}); diff --git a/tests/Umbraco.Tests.AcceptanceTest/cypress/integration/Users/userGroups.ts b/tests/Umbraco.Tests.AcceptanceTest/cypress/integration/Users/userGroups.ts new file mode 100644 index 0000000000..cd4b022544 --- /dev/null +++ b/tests/Umbraco.Tests.AcceptanceTest/cypress/integration/Users/userGroups.ts @@ -0,0 +1,85 @@ +/// +import { UserGroupBuilder } from 'umbraco-cypress-testhelpers'; + +context('User Groups', () => { + + function navigateToUserGroups() { + cy.umbracoSection('users'); + cy.get('[data-element="sub-view-userGroups"]').click(); + } + + beforeEach(() => { + cy.umbracoLogin(Cypress.env('username'), Cypress.env('password')); + }); + + it('Create user group', () => { + const name = "Test Group"; + + cy.umbracoEnsureUserGroupNameNotExists(name); + + navigateToUserGroups(); + cy.umbracoButtonByLabelKey("actions_createGroup").click(); + + //Type name + cy.umbracoEditorHeaderName(name); + + // Assign sections + cy.get('.umb-box:nth-child(1) .umb-property:nth-child(1) localize').click(); + cy.get('.umb-tree-item__inner').click({multiple:true, timeout: 10000}); + cy.get('.btn-success').last().click(); + + // Save + cy.get('.btn-success').click(); + + //Assert + cy.umbracoSuccessNotification().should('be.visible'); + + //Clean up + cy.umbracoEnsureUserGroupNameNotExists(name); + }); + + it('Can delete user group', () => { + // Create user group + const groupName = "Delete user group test" + cy.umbracoEnsureUserGroupNameNotExists(groupName); + + const userGroup = new UserGroupBuilder() + .withName(groupName) + .build(); + + cy.saveUserGroup(userGroup); + navigateToUserGroups(); + + // Delete the user group + cy.get('.umb-table-body > :nth-child(2)').click(); + cy.umbracoButtonByLabelKey("general_delete").click(); + cy.get('umb-button[alias="overlaySubmit"]').click(); + + cy.umbracoSuccessNotification().should('be.visible'); + cy.get('.umb-table-body').contains(groupName).should('not.exist'); + + cy.umbracoEnsureUserGroupNameNotExists(groupName); + }); + + it('Cannot delete required groups', () => { + navigateToUserGroups(); + + // There's not really a good way to be 100% sure we'll get the admin group, it should be first, but who knows + // so double check that we actually got the correct one + const administrators = cy.get('.umb-table-body > :nth-child(1)'); + administrators.should('contain', 'Administrators'); + administrators.click({force: true}); + + const sensitive = cy.get('.umb-table-body > :nth-child(3)'); + sensitive.should('contain', 'Sensitive data'); + sensitive.click({force: true}); + + const translators = cy.get('.umb-table-body > :nth-child(4)'); + translators.should('contain', 'Translators'); + translators.click({force: true}); + + // Now that we've clicked all that we shouldn't be able to delete, ensure that the delete button does not show up + cy.get('.umb-editor-sub-header').should('not.contain', 'Delete'); + }); +}); + diff --git a/tests/Umbraco.Tests.AcceptanceTest/package-lock.json b/tests/Umbraco.Tests.AcceptanceTest/package-lock.json index 7ada1d9fb7..fb622bbffb 100644 --- a/tests/Umbraco.Tests.AcceptanceTest/package-lock.json +++ b/tests/Umbraco.Tests.AcceptanceTest/package-lock.json @@ -1628,9 +1628,9 @@ "integrity": "sha512-w6fIxVE/H1PkLKcCPsFqKE7Kv7QUwhU8qQY2MueZXWx5cPZdwFupLgKK3vntcK98BtNHZtAF4LA/yl2a7k8R6Q==" }, "umbraco-cypress-testhelpers": { - "version": "1.0.0-beta-63", - "resolved": "https://registry.npmjs.org/umbraco-cypress-testhelpers/-/umbraco-cypress-testhelpers-1.0.0-beta-63.tgz", - "integrity": "sha512-X+DHWktfB+WBb7YrxvpneVfS1PATx2zPYMdkeZTmtoQEeyGxXA9fW6P712/AUbyGAhRhH+46t4cAINdWJxItug==", + "version": "1.0.0-beta-66", + "resolved": "https://registry.npmjs.org/umbraco-cypress-testhelpers/-/umbraco-cypress-testhelpers-1.0.0-beta-66.tgz", + "integrity": "sha512-/Iq0P7rN9LfODO9snoLNqvbd8b432JIYtCVjYOdYZFceMAUIv0v2/6t7+N55Z7h8OpAQzcTLU3VCxfPzZp8wQw==", "dev": true, "requires": { "camelize": "^1.0.0", diff --git a/tests/Umbraco.Tests.AcceptanceTest/package.json b/tests/Umbraco.Tests.AcceptanceTest/package.json index a95a71020f..92141f4b1f 100644 --- a/tests/Umbraco.Tests.AcceptanceTest/package.json +++ b/tests/Umbraco.Tests.AcceptanceTest/package.json @@ -14,7 +14,7 @@ "del": "^6.0.0", "ncp": "^2.0.0", "prompt": "^1.2.0", - "umbraco-cypress-testhelpers": "^1.0.0-beta-63" + "umbraco-cypress-testhelpers": "^1.0.0-beta-65" }, "dependencies": { "typescript": "^3.9.2" From 4f0a837e208f1c5eebac653b3fbb88718770bd72 Mon Sep 17 00:00:00 2001 From: Mole Date: Tue, 22 Feb 2022 08:29:27 +0100 Subject: [PATCH 02/22] V9: Fix login timeout (#12029) * Turn SlidingExpiration off and only renew cookie of not RemainingSeconds request Also adds the TicketExpiresClaim before validating the the security stamp, otherwise the claim won't be merged and "dissappear", leading to the user being instantly logged out Also only EnsureValidSessionId if not RemainingSeconds request, otherwise the session will always be valid, since the remaining seconds request renews it. * Don't ignore SessionIdClaimType and Cookiepath when merging claims Besides what the comment used to state these claims are only issued when logging in, leading you to be logged out once the claims are merged, furthermore when we check the session ID we verify that you session has not expired. * Manually specify Issued and Expires when renewing token If we don't we lose 30 minutes of our ExpireTimeSpan every time the principal refreshes * Re-add ignored claims And use MergeAllClaims on refreshing principal instead. * EnsureValidSessionId before updating IssuedUtc * Fix comment * Update src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs Co-authored-by: nikolajlauridsen Co-authored-by: Bjarke Berg --- .../Extensions/ClaimsIdentityExtensions.cs | 2 +- .../Security/ClaimsIdentityExtensions.cs | 4 +- .../ConfigureBackOfficeCookieOptions.cs | 45 +++++++++++++++++-- .../Security/ConfigureSecurityStampOptions.cs | 3 +- 4 files changed, 47 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs b/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs index bceddf1fd6..0319be3297 100644 --- a/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs +++ b/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs @@ -132,7 +132,7 @@ namespace Umbraco.Extensions } } - verifiedIdentity = identity.AuthenticationType == Constants.Security.BackOfficeAuthenticationType ? identity : new ClaimsIdentity(identity.Claims, Constants.Security.BackOfficeAuthenticationType); + verifiedIdentity = identity.AuthenticationType == Constants.Security.BackOfficeAuthenticationType ? identity : new ClaimsIdentity(identity.Claims, Constants.Security.BackOfficeAuthenticationType); return true; } diff --git a/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs b/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs index de5b6206fc..9e11916223 100644 --- a/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs +++ b/src/Umbraco.Infrastructure/Security/ClaimsIdentityExtensions.cs @@ -1,5 +1,6 @@ // Copyright (c) Umbraco. // See LICENSE for more details. + using System.Linq; using System.Security.Claims; using Umbraco.Cms.Core; @@ -11,7 +12,8 @@ namespace Umbraco.Extensions { // Ignore these Claims when merging, these claims are dynamically added whenever the ticket // is re-issued and we don't want to merge old values of these. - private static readonly string[] s_ignoredClaims = new[] { ClaimTypes.CookiePath, Constants.Security.SessionIdClaimType }; + // We do however want to merge these when the SecurityStampValidator refreshes the principal since it's still the same login session + private static readonly string[] s_ignoredClaims = { ClaimTypes.CookiePath, Constants.Security.SessionIdClaimType }; public static void MergeAllClaims(this ClaimsIdentity destination, ClaimsIdentity source) { diff --git a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs index 58a6862300..916bfb17c0 100644 --- a/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs +++ b/src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs @@ -15,6 +15,7 @@ using Umbraco.Cms.Core.Net; using Umbraco.Cms.Core.Routing; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Web; +using Umbraco.Cms.Web.BackOffice.Controllers; using Umbraco.Extensions; namespace Umbraco.Cms.Web.BackOffice.Security @@ -92,7 +93,7 @@ namespace Umbraco.Cms.Web.BackOffice.Security /// public void Configure(CookieAuthenticationOptions options) { - options.SlidingExpiration = true; + options.SlidingExpiration = false; options.ExpireTimeSpan = _globalSettings.TimeOut; options.Cookie.Domain = _securitySettings.AuthCookieDomain; options.Cookie.Name = _securitySettings.AuthCookieName; @@ -150,8 +151,6 @@ namespace Umbraco.Cms.Web.BackOffice.Security // ensure the thread culture is set backOfficeIdentity.EnsureCulture(); - await EnsureValidSessionId(ctx); - await securityStampValidator.ValidateAsync(ctx); EnsureTicketRenewalIfKeepUserLoggedIn(ctx); // add or update a claim to track when the cookie expires, we use this to track time remaining @@ -163,6 +162,28 @@ namespace Umbraco.Cms.Web.BackOffice.Security Constants.Security.BackOfficeAuthenticationType, backOfficeIdentity)); + await securityStampValidator.ValidateAsync(ctx); + + // This might have been called from GetRemainingTimeoutSeconds, in this case we don't want to ensure valid session + // since that in it self will keep the session valid since we renew the lastVerified date. + // Similarly don't renew the token + if (IsRemainingSecondsRequest(ctx)) + { + return; + } + + // This relies on IssuedUtc, so call it before updating it. + await EnsureValidSessionId(ctx); + + // We have to manually specify Issued and Expires, + // because the SecurityStampValidator refreshes the principal every 30 minutes, + // When the principal is refreshed the Issued is update to time of refresh, however, the Expires remains unchanged + // When we then try and renew, the difference of issued and expires effectively becomes the new ExpireTimeSpan + // meaning we effectively lose 30 minutes of our ExpireTimeSpan for EVERY principal refresh if we don't + // https://github.com/dotnet/aspnetcore/blob/main/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs#L115 + ctx.Properties.IssuedUtc = _systemClock.UtcNow; + ctx.Properties.ExpiresUtc = _systemClock.UtcNow.Add(_globalSettings.TimeOut); + ctx.ShouldRenew = true; }, OnSigningIn = ctx => { @@ -226,7 +247,7 @@ namespace Umbraco.Cms.Web.BackOffice.Security } return Task.CompletedTask; - } + }, }; } @@ -276,5 +297,21 @@ namespace Umbraco.Cms.Web.BackOffice.Security } } } + + private bool IsRemainingSecondsRequest(CookieValidatePrincipalContext context) + { + var routeValues = context.HttpContext.Request.RouteValues; + if (routeValues.TryGetValue("controller", out var controllerName) && + routeValues.TryGetValue("action", out var action)) + { + if (controllerName?.ToString() == ControllerExtensions.GetControllerName() + && action?.ToString() == nameof(AuthenticationController.GetRemainingTimeoutSeconds)) + { + return true; + } + } + + return false; + } } } diff --git a/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs b/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs index 03bdf8f4dd..66cf97fd4c 100644 --- a/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs +++ b/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs @@ -30,7 +30,8 @@ namespace Umbraco.Cms.Web.Common.Security ClaimsIdentity newIdentity = refreshingPrincipal.NewPrincipal.Identities.First(); ClaimsIdentity currentIdentity = refreshingPrincipal.CurrentPrincipal.Identities.First(); - newIdentity.MergeClaimsFromCookieIdentity(currentIdentity); + // Since this is refreshing an existing principal, we want to merge all claims. + newIdentity.MergeAllClaims(currentIdentity); return Task.CompletedTask; }; From e67da0b1980e81bac8327b689d95b9a436600021 Mon Sep 17 00:00:00 2001 From: Paul Johnson Date: Tue, 22 Feb 2022 08:49:56 +0000 Subject: [PATCH 03/22] Ignore certificate errors for KeepAlive task. (#12019) --- src/Umbraco.Core/Constants-HttpClients.cs | 19 +++++++++++++++++++ .../HostedServices/KeepAlive.cs | 3 ++- .../UmbracoBuilderExtensions.cs | 6 ++++++ 3 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 src/Umbraco.Core/Constants-HttpClients.cs diff --git a/src/Umbraco.Core/Constants-HttpClients.cs b/src/Umbraco.Core/Constants-HttpClients.cs new file mode 100644 index 0000000000..474ec49a50 --- /dev/null +++ b/src/Umbraco.Core/Constants-HttpClients.cs @@ -0,0 +1,19 @@ +namespace Umbraco.Cms.Core +{ + /// + /// Defines constants. + /// + public static partial class Constants + { + /// + /// Defines constants for named http clients. + /// + public static class HttpClients + { + /// + /// Name for http client which ignores certificate errors. + /// + public const string IgnoreCertificateErrors = "Umbraco:HttpClients:IgnoreCertificateErrors"; + } + } +} diff --git a/src/Umbraco.Infrastructure/HostedServices/KeepAlive.cs b/src/Umbraco.Infrastructure/HostedServices/KeepAlive.cs index b564e7948d..22160b8f6e 100644 --- a/src/Umbraco.Infrastructure/HostedServices/KeepAlive.cs +++ b/src/Umbraco.Infrastructure/HostedServices/KeepAlive.cs @@ -7,6 +7,7 @@ using System.Net.Http; using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using Umbraco.Cms.Core; using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Hosting; using Umbraco.Cms.Core.Logging; @@ -99,7 +100,7 @@ namespace Umbraco.Cms.Infrastructure.HostedServices try { var request = new HttpRequestMessage(HttpMethod.Get, keepAlivePingUrl); - HttpClient httpClient = _httpClientFactory.CreateClient(); + HttpClient httpClient = _httpClientFactory.CreateClient(Constants.HttpClients.IgnoreCertificateErrors); _ = await httpClient.SendAsync(request); } catch (Exception ex) diff --git a/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs b/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs index 819076bbb8..d2cbf5bd14 100644 --- a/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs +++ b/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs @@ -3,6 +3,7 @@ using System.Data.Common; using System.Data.SqlClient; using System.IO; using System.Linq; +using System.Net.Http; using System.Reflection; using Dazinator.Extensions.FileProviders.GlobPatternFilter; using Microsoft.AspNetCore.Builder; @@ -191,6 +192,11 @@ namespace Umbraco.Extensions private static IUmbracoBuilder AddHttpClients(this IUmbracoBuilder builder) { builder.Services.AddHttpClient(); + builder.Services.AddHttpClient(Constants.HttpClients.IgnoreCertificateErrors) + .ConfigurePrimaryHttpMessageHandler(() => new HttpClientHandler + { + ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator + }); return builder; } From 860c8e8ae2c4790e0c16e8e5f0427a436352a338 Mon Sep 17 00:00:00 2001 From: Paul Johnson Date: Thu, 24 Feb 2022 10:17:34 +0000 Subject: [PATCH 04/22] Filesystem based MainDomLock & extract interface for MainDomKey generation (#12037) * Extract MainDomKey generation to its own class to ease customization. Also add discriminator config value to GlobalSettings for advanced users. Prevents a mandatory custom implementation, should be good enough for the vast majority of use cases. * Prevent duplicate runs of ScheduledPublishing during slot swap. * Add filesystem based MainDomLock --- .../Configuration/Models/GlobalSettings.cs | 8 ++ .../Persistence/Constants-Locks.cs | 5 + .../Runtime/IMainDomKeyGenerator.cs | 13 ++ src/Umbraco.Core/Runtime/MainDom.cs | 4 +- .../UmbracoBuilder.CoreServices.cs | 10 +- .../HostedServices/ScheduledPublishing.cs | 51 +++++-- .../Migrations/Install/DatabaseDataCreator.cs | 1 + .../Migrations/Upgrade/UmbracoPlan.cs | 3 + .../V_9_4_0/AddScheduledPublishingLock.cs | 15 ++ .../Runtime/DefaultMainDomKeyGenerator.cs | 34 +++++ .../Runtime/FileSystemMainDomLock.cs | 131 ++++++++++++++++++ .../Runtime/SqlMainDomLock.cs | 63 ++++++--- .../Runtime/FileSystemMainDomLockTests.cs | 97 +++++++++++++ .../ScheduledPublishingTests.cs | 11 +- .../DefaultMainDomKeyGeneratorTests.cs | 47 +++++++ 15 files changed, 457 insertions(+), 36 deletions(-) create mode 100644 src/Umbraco.Core/Runtime/IMainDomKeyGenerator.cs create mode 100644 src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_4_0/AddScheduledPublishingLock.cs create mode 100644 src/Umbraco.Infrastructure/Runtime/DefaultMainDomKeyGenerator.cs create mode 100644 src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs create mode 100644 tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Runtime/FileSystemMainDomLockTests.cs create mode 100644 tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Runtime/DefaultMainDomKeyGeneratorTests.cs diff --git a/src/Umbraco.Core/Configuration/Models/GlobalSettings.cs b/src/Umbraco.Core/Configuration/Models/GlobalSettings.cs index 7e3e1a2700..53584af1d1 100644 --- a/src/Umbraco.Core/Configuration/Models/GlobalSettings.cs +++ b/src/Umbraco.Core/Configuration/Models/GlobalSettings.cs @@ -137,6 +137,14 @@ namespace Umbraco.Cms.Core.Configuration.Models /// public string MainDomLock { get; set; } = string.Empty; + /// + /// Gets or sets a value to discriminate MainDom boundaries. + /// + /// Generally the default should suffice but useful for advanced scenarios e.g. azure deployment slot based zero downtime deployments. + /// + /// + public string MainDomKeyDiscriminator { get; set; } = string.Empty; + /// /// Gets or sets the telemetry ID. /// diff --git a/src/Umbraco.Core/Persistence/Constants-Locks.cs b/src/Umbraco.Core/Persistence/Constants-Locks.cs index 5312bf6886..3c0b2c4d28 100644 --- a/src/Umbraco.Core/Persistence/Constants-Locks.cs +++ b/src/Umbraco.Core/Persistence/Constants-Locks.cs @@ -65,6 +65,11 @@ namespace Umbraco.Cms.Core /// All languages. /// public const int Languages = -340; + + /// + /// ScheduledPublishing job. + /// + public const int ScheduledPublishing = -341; } } } diff --git a/src/Umbraco.Core/Runtime/IMainDomKeyGenerator.cs b/src/Umbraco.Core/Runtime/IMainDomKeyGenerator.cs new file mode 100644 index 0000000000..5b8fb819e6 --- /dev/null +++ b/src/Umbraco.Core/Runtime/IMainDomKeyGenerator.cs @@ -0,0 +1,13 @@ +namespace Umbraco.Cms.Core.Runtime +{ + /// + /// Defines a class which can generate a distinct key for a MainDom boundary. + /// + public interface IMainDomKeyGenerator + { + /// + /// Returns a key that signifies a MainDom boundary. + /// + string GenerateKey(); + } +} diff --git a/src/Umbraco.Core/Runtime/MainDom.cs b/src/Umbraco.Core/Runtime/MainDom.cs index 08d11db5cd..d22176d9cf 100644 --- a/src/Umbraco.Core/Runtime/MainDom.cs +++ b/src/Umbraco.Core/Runtime/MainDom.cs @@ -87,7 +87,7 @@ namespace Umbraco.Cms.Core.Runtime if (_isMainDom.HasValue == false) { - throw new InvalidOperationException("Register called when MainDom has not been acquired"); + throw new InvalidOperationException("Register called before IsMainDom has been established"); } else if (_isMainDom == false) { @@ -225,7 +225,7 @@ namespace Umbraco.Cms.Core.Runtime { if (!_isMainDom.HasValue) { - throw new InvalidOperationException("MainDom has not been acquired yet"); + throw new InvalidOperationException("IsMainDom has not been established yet"); } return _isMainDom.Value; } diff --git a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs index f4a4866beb..1c8b3ec0de 100644 --- a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs +++ b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs @@ -218,6 +218,7 @@ namespace Umbraco.Cms.Infrastructure.DependencyInjection private static IUmbracoBuilder AddMainDom(this IUmbracoBuilder builder) { + builder.Services.AddSingleton(); builder.Services.AddSingleton(factory => { var globalSettings = factory.GetRequiredService>(); @@ -229,15 +230,20 @@ namespace Umbraco.Cms.Infrastructure.DependencyInjection var isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); var loggerFactory = factory.GetRequiredService(); var npocoMappers = factory.GetRequiredService(); + var mainDomKeyGenerator = factory.GetRequiredService(); + + if (globalSettings.Value.MainDomLock == "FileSystemMainDomLock") + { + return new FileSystemMainDomLock(loggerFactory.CreateLogger(), mainDomKeyGenerator, hostingEnvironment); + } return globalSettings.Value.MainDomLock.Equals("SqlMainDomLock") || isWindows == false ? (IMainDomLock)new SqlMainDomLock( - loggerFactory.CreateLogger(), loggerFactory, globalSettings, connectionStrings, dbCreator, - hostingEnvironment, + mainDomKeyGenerator, databaseSchemaCreatorFactory, npocoMappers) : new MainDomSemaphoreLock(loggerFactory.CreateLogger(), hostingEnvironment); diff --git a/src/Umbraco.Infrastructure/HostedServices/ScheduledPublishing.cs b/src/Umbraco.Infrastructure/HostedServices/ScheduledPublishing.cs index d59ea4fad3..429389273f 100644 --- a/src/Umbraco.Infrastructure/HostedServices/ScheduledPublishing.cs +++ b/src/Umbraco.Infrastructure/HostedServices/ScheduledPublishing.cs @@ -5,13 +5,15 @@ using System; using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Runtime; -using Umbraco.Cms.Core.Security; +using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Sync; using Umbraco.Cms.Core.Web; +using Umbraco.Cms.Web.Common.DependencyInjection; namespace Umbraco.Cms.Infrastructure.HostedServices { @@ -27,20 +29,16 @@ namespace Umbraco.Cms.Infrastructure.HostedServices private readonly IMainDom _mainDom; private readonly IRuntimeState _runtimeState; private readonly IServerMessenger _serverMessenger; + private readonly IScopeProvider _scopeProvider; private readonly IServerRoleAccessor _serverRegistrar; private readonly IUmbracoContextFactory _umbracoContextFactory; /// /// Initializes a new instance of the class. /// - /// Representation of the state of the Umbraco runtime. - /// Representation of the main application domain. - /// Provider of server registrations to the distributed cache. - /// Service for handling content operations. - /// Service for creating and managing Umbraco context. - /// The typed logger. - /// Service broadcasting cache notifications to registered servers. - /// Creates and manages instances. + // Note: Ignoring the two version notice rule as this class should probably be internal. + // We don't expect anyone downstream to be instantiating a HostedService + [Obsolete("This constructor will be removed in version 10, please use an alternative constructor.")] public ScheduledPublishing( IRuntimeState runtimeState, IMainDom mainDom, @@ -49,6 +47,30 @@ namespace Umbraco.Cms.Infrastructure.HostedServices IUmbracoContextFactory umbracoContextFactory, ILogger logger, IServerMessenger serverMessenger) + : this( + runtimeState, + mainDom, + serverRegistrar, + contentService, + umbracoContextFactory, + logger, + serverMessenger, + StaticServiceProvider.Instance.GetRequiredService()) + { + } + + /// + /// Initializes a new instance of the class. + /// + public ScheduledPublishing( + IRuntimeState runtimeState, + IMainDom mainDom, + IServerRoleAccessor serverRegistrar, + IContentService contentService, + IUmbracoContextFactory umbracoContextFactory, + ILogger logger, + IServerMessenger serverMessenger, + IScopeProvider scopeProvider) : base(TimeSpan.FromMinutes(1), DefaultDelay) { _runtimeState = runtimeState; @@ -58,6 +80,7 @@ namespace Umbraco.Cms.Infrastructure.HostedServices _umbracoContextFactory = umbracoContextFactory; _logger = logger; _serverMessenger = serverMessenger; + _scopeProvider = scopeProvider; } public override Task PerformExecuteAsync(object state) @@ -93,8 +116,6 @@ namespace Umbraco.Cms.Infrastructure.HostedServices try { - // We don't need an explicit scope here because PerformScheduledPublish creates it's own scope - // so it's safe as it will create it's own ambient scope. // Ensure we run with an UmbracoContext, because this will run in a background task, // and developers may be using the UmbracoContext in the event handlers. @@ -105,6 +126,14 @@ namespace Umbraco.Cms.Infrastructure.HostedServices // - and we should definitively *not* have to flush it here (should be auto) using UmbracoContextReference contextReference = _umbracoContextFactory.EnsureUmbracoContext(); + using IScope scope = _scopeProvider.CreateScope(autoComplete: true); + + /* We used to assume that there will never be two instances running concurrently where (IsMainDom && ServerRole == SchedulingPublisher) + * However this is possible during an azure deployment slot swap for the SchedulingPublisher instance when trying to achieve zero downtime deployments. + * If we take a distributed write lock, we are certain that the multiple instances of the job will not run in parallel. + * It's possible that during the swapping process we may run this job more frequently than intended but this is not of great concern and it's + * only until the old SchedulingPublisher shuts down. */ + scope.EagerWriteLock(Constants.Locks.ScheduledPublishing); try { // Run diff --git a/src/Umbraco.Infrastructure/Migrations/Install/DatabaseDataCreator.cs b/src/Umbraco.Infrastructure/Migrations/Install/DatabaseDataCreator.cs index 4f2ef1f2e9..25dadb0c85 100644 --- a/src/Umbraco.Infrastructure/Migrations/Install/DatabaseDataCreator.cs +++ b/src/Umbraco.Infrastructure/Migrations/Install/DatabaseDataCreator.cs @@ -175,6 +175,7 @@ namespace Umbraco.Cms.Infrastructure.Migrations.Install _database.Insert(Cms.Core.Constants.DatabaseSchema.Tables.Lock, "id", false, new LockDto { Id = Cms.Core.Constants.Locks.Domains, Name = "Domains" }); _database.Insert(Cms.Core.Constants.DatabaseSchema.Tables.Lock, "id", false, new LockDto { Id = Cms.Core.Constants.Locks.KeyValues, Name = "KeyValues" }); _database.Insert(Cms.Core.Constants.DatabaseSchema.Tables.Lock, "id", false, new LockDto { Id = Cms.Core.Constants.Locks.Languages, Name = "Languages" }); + _database.Insert(Cms.Core.Constants.DatabaseSchema.Tables.Lock, "id", false, new LockDto { Id = Cms.Core.Constants.Locks.ScheduledPublishing, Name = "ScheduledPublishing" }); _database.Insert(Cms.Core.Constants.DatabaseSchema.Tables.Lock, "id", false, new LockDto { Id = Cms.Core.Constants.Locks.MainDom, Name = "MainDom" }); } diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs index 39d7d886b3..11de1209b3 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs @@ -16,6 +16,7 @@ using Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_9_0_0; using Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_9_1_0; using Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_9_2_0; using Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_9_3_0; +using Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_9_4_0; using Umbraco.Extensions; namespace Umbraco.Cms.Infrastructure.Migrations.Upgrade @@ -280,6 +281,8 @@ namespace Umbraco.Cms.Infrastructure.Migrations.Upgrade To("{CA7A1D9D-C9D4-4914-BC0A-459E7B9C3C8C}"); To("{0828F206-DCF7-4F73-ABBB-6792275532EB}"); + // TO 9.4.0 + To("{DBBA1EA0-25A1-4863-90FB-5D306FB6F1E1}"); } } } diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_4_0/AddScheduledPublishingLock.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_4_0/AddScheduledPublishingLock.cs new file mode 100644 index 0000000000..01cfb22a3d --- /dev/null +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_4_0/AddScheduledPublishingLock.cs @@ -0,0 +1,15 @@ +using Umbraco.Cms.Infrastructure.Persistence.Dtos; + +namespace Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_9_4_0 +{ + internal class AddScheduledPublishingLock : MigrationBase + { + public AddScheduledPublishingLock(IMigrationContext context) + : base(context) + { + } + + protected override void Migrate() => + Database.Insert(Cms.Core.Constants.DatabaseSchema.Tables.Lock, "id", false, new LockDto { Id = Cms.Core.Constants.Locks.ScheduledPublishing, Name = "ScheduledPublishing" }); + } +} diff --git a/src/Umbraco.Infrastructure/Runtime/DefaultMainDomKeyGenerator.cs b/src/Umbraco.Infrastructure/Runtime/DefaultMainDomKeyGenerator.cs new file mode 100644 index 0000000000..11944e776c --- /dev/null +++ b/src/Umbraco.Infrastructure/Runtime/DefaultMainDomKeyGenerator.cs @@ -0,0 +1,34 @@ +using System; +using System.Security.Cryptography; +using Microsoft.Extensions.Options; +using Umbraco.Cms.Core.Configuration.Models; +using Umbraco.Cms.Core.Hosting; +using Umbraco.Cms.Core.Runtime; +using Umbraco.Extensions; + +namespace Umbraco.Cms.Infrastructure.Runtime +{ + + internal class DefaultMainDomKeyGenerator : IMainDomKeyGenerator + { + private readonly IHostingEnvironment _hostingEnvironment; + private readonly IOptionsMonitor _globalSettings; + + public DefaultMainDomKeyGenerator(IHostingEnvironment hostingEnvironment, IOptionsMonitor globalSettings) + { + _hostingEnvironment = hostingEnvironment; + _globalSettings = globalSettings; + } + + public string GenerateKey() + { + var machineName = Environment.MachineName; + var mainDomId = MainDom.GetMainDomId(_hostingEnvironment); + var discriminator = _globalSettings.CurrentValue.MainDomKeyDiscriminator; + + var rawKey = $"{machineName}{mainDomId}{discriminator}"; + + return rawKey.GenerateHash(); + } + } +} diff --git a/src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs b/src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs new file mode 100644 index 0000000000..d18e4085a0 --- /dev/null +++ b/src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs @@ -0,0 +1,131 @@ +using System; +using System.Diagnostics; +using System.IO; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Umbraco.Cms.Core.Hosting; +using Umbraco.Cms.Core.Runtime; + +namespace Umbraco.Cms.Infrastructure.Runtime +{ + internal class FileSystemMainDomLock : IMainDomLock + { + private readonly ILogger _log; + + private readonly CancellationTokenSource _cancellationTokenSource = new(); + + private readonly string _lockFilePath; + private readonly string _releaseSignalFilePath; + + private FileStream _lockFileStream; + + public FileSystemMainDomLock( + ILogger log, + IMainDomKeyGenerator mainDomKeyGenerator, + IHostingEnvironment hostingEnvironment) + { + _log = log; + + var lockFileName = $"MainDom_{mainDomKeyGenerator.GenerateKey()}.lock"; + _lockFilePath = Path.Combine(hostingEnvironment.LocalTempPath, lockFileName); + _releaseSignalFilePath = $"{_lockFilePath}_release"; + } + + public Task AcquireLockAsync(int millisecondsTimeout) + { + var stopwatch = new Stopwatch(); + stopwatch.Start(); + + do + { + try + { + _log.LogDebug("Attempting to obtain MainDom lock file handle {lockFilePath}", _lockFilePath); + _lockFileStream = File.Open(_lockFilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None); + DeleteLockReleaseFile(); + return Task.FromResult(true); + } + catch (IOException) + { + _log.LogDebug("Couldn't obtain MainDom lock file handle, signalling for release of {lockFilePath}", _lockFilePath); + CreateLockReleaseFile(); + Thread.Sleep(500); + } + catch (Exception ex) + { + _log.LogError(ex, "Unexpected exception attempting to obtain MainDom lock file handle {lockFilePath}, giving up", _lockFilePath); + return Task.FromResult(false); + } + } + while (stopwatch.ElapsedMilliseconds < millisecondsTimeout); + + return Task.FromResult(false); + } + + // Create a long running task to poll to check if anyone has created a lock release file. + public Task ListenAsync() => + Task.Factory.StartNew( + ListeningLoop, + _cancellationTokenSource.Token, + TaskCreationOptions.LongRunning, + TaskScheduler.Default); + + public void Dispose() + { + _lockFileStream?.Close(); + _lockFileStream = null; + } + + private void CreateLockReleaseFile() + { + try + { + // Dispose immediately to release the file handle so it's easier to cleanup in any process. + using FileStream releaseFileStream = File.Open(_releaseSignalFilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.ReadWrite); + } + catch (Exception ex) + { + _log.LogError(ex, "Unexpected exception attempting to create lock release signal file {file}", _releaseSignalFilePath); + } + } + + private void DeleteLockReleaseFile() + { + while (File.Exists(_releaseSignalFilePath)) + { + try + { + File.Delete(_releaseSignalFilePath); + } + catch (Exception ex) + { + _log.LogError(ex, "Unexpected exception attempting to delete release signal file {file}", _releaseSignalFilePath); + Thread.Sleep(500); + } + } + } + + private void ListeningLoop() + { + while (true) + { + if (_cancellationTokenSource.IsCancellationRequested) + { + _log.LogDebug("ListenAsync Task canceled, exiting loop"); + return; + } + + if (File.Exists(_releaseSignalFilePath)) + { + _log.LogDebug("Found lock release signal file, releasing lock on {lockFilePath}", _lockFilePath); + _lockFileStream?.Close(); + _lockFileStream = null; + break; + } + + Thread.Sleep(2000); + } + } + } +} diff --git a/src/Umbraco.Infrastructure/Runtime/SqlMainDomLock.cs b/src/Umbraco.Infrastructure/Runtime/SqlMainDomLock.cs index 8d1c74b619..635b0b9c28 100644 --- a/src/Umbraco.Infrastructure/Runtime/SqlMainDomLock.cs +++ b/src/Umbraco.Infrastructure/Runtime/SqlMainDomLock.cs @@ -6,6 +6,7 @@ using System.Linq; using System.Security.Cryptography; using System.Threading; using System.Threading.Tasks; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using NPoco; @@ -18,6 +19,7 @@ using Umbraco.Cms.Infrastructure.Persistence; using Umbraco.Cms.Infrastructure.Persistence.Dtos; using Umbraco.Cms.Infrastructure.Persistence.Mappers; using Umbraco.Cms.Infrastructure.Persistence.SqlSyntax; +using Umbraco.Cms.Web.Common.DependencyInjection; using Umbraco.Extensions; using MapperCollection = Umbraco.Cms.Infrastructure.Persistence.Mappers.MapperCollection; @@ -30,7 +32,6 @@ namespace Umbraco.Cms.Infrastructure.Runtime private const string UpdatedSuffix = "_updated"; private readonly ILogger _logger; private readonly IOptions _globalSettings; - private readonly IHostingEnvironment _hostingEnvironment; private readonly IUmbracoDatabase _db; private readonly CancellationTokenSource _cancellationTokenSource = new CancellationTokenSource(); private SqlServerSyntaxProvider _sqlServerSyntax; @@ -41,6 +42,9 @@ namespace Umbraco.Cms.Infrastructure.Runtime private bool _hasTable = false; private bool _acquireWhenTablesNotAvailable = false; + // Note: Ignoring the two version notice rule as this class should probably be internal. + // We don't expect anyone downstream to be instantiating a SqlMainDomLock, only resolving IMainDomLock + [Obsolete("This constructor will be removed in version 10, please use an alternative constructor.")] public SqlMainDomLock( ILogger logger, ILoggerFactory loggerFactory, @@ -51,25 +55,20 @@ namespace Umbraco.Cms.Infrastructure.Runtime DatabaseSchemaCreatorFactory databaseSchemaCreatorFactory, NPocoMapperCollection npocoMappers, string connectionStringName) - { - // unique id for our appdomain, this is more unique than the appdomain id which is just an INT counter to its safer - _lockId = Guid.NewGuid().ToString(); - _logger = logger; - _globalSettings = globalSettings; - _sqlServerSyntax = new SqlServerSyntaxProvider(_globalSettings); - _hostingEnvironment = hostingEnvironment; - _dbFactory = new UmbracoDatabaseFactory( - loggerFactory.CreateLogger(), + : this( loggerFactory, - _globalSettings, - new MapperCollection(() => Enumerable.Empty()), + globalSettings, + connectionStrings, dbProviderFactoryCreator, + StaticServiceProvider.Instance.GetRequiredService(), databaseSchemaCreatorFactory, - npocoMappers, - connectionStringName); - MainDomKey = MainDomKeyPrefix + "-" + (Environment.MachineName + MainDom.GetMainDomId(_hostingEnvironment)).GenerateHash(); + npocoMappers) + { } + // Note: Ignoring the two version notice rule as this class should probably be internal. + // We don't expect anyone downstream to be instantiating a SqlMainDomLock, only resolving IMainDomLock + [Obsolete("This constructor will be removed in version 10, please use an alternative constructor.")] public SqlMainDomLock( ILogger logger, ILoggerFactory loggerFactory, @@ -80,18 +79,42 @@ namespace Umbraco.Cms.Infrastructure.Runtime DatabaseSchemaCreatorFactory databaseSchemaCreatorFactory, NPocoMapperCollection npocoMappers) : this( - logger, loggerFactory, globalSettings, connectionStrings, dbProviderFactoryCreator, - hostingEnvironment, + StaticServiceProvider.Instance.GetRequiredService(), databaseSchemaCreatorFactory, - npocoMappers, - connectionStrings.CurrentValue.UmbracoConnectionString.ConnectionString - ) + npocoMappers) { + } + public SqlMainDomLock( + ILoggerFactory loggerFactory, + IOptions globalSettings, + IOptionsMonitor connectionStrings, + IDbProviderFactoryCreator dbProviderFactoryCreator, + IMainDomKeyGenerator mainDomKeyGenerator, + DatabaseSchemaCreatorFactory databaseSchemaCreatorFactory, + NPocoMapperCollection npocoMappers) + { + // unique id for our appdomain, this is more unique than the appdomain id which is just an INT counter to its safer + _lockId = Guid.NewGuid().ToString(); + _logger = loggerFactory.CreateLogger(); + _globalSettings = globalSettings; + _sqlServerSyntax = new SqlServerSyntaxProvider(_globalSettings); + + _dbFactory = new UmbracoDatabaseFactory( + loggerFactory.CreateLogger(), + loggerFactory, + _globalSettings, + new MapperCollection(() => Enumerable.Empty()), + dbProviderFactoryCreator, + databaseSchemaCreatorFactory, + npocoMappers, + connectionStrings.CurrentValue.UmbracoConnectionString.ConnectionString); + + MainDomKey = MainDomKeyPrefix + "-" + mainDomKeyGenerator.GenerateKey(); } public async Task AcquireLockAsync(int millisecondsTimeout) diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Runtime/FileSystemMainDomLockTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Runtime/FileSystemMainDomLockTests.cs new file mode 100644 index 0000000000..59442a683a --- /dev/null +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Runtime/FileSystemMainDomLockTests.cs @@ -0,0 +1,97 @@ +using System.IO; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using NUnit.Framework; +using Umbraco.Cms.Core.Hosting; +using Umbraco.Cms.Core.Runtime; +using Umbraco.Cms.Infrastructure.Runtime; +using Umbraco.Cms.Tests.Integration.Testing; + +namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Runtime +{ + [TestFixture] + internal class FileSystemMainDomLockTests : UmbracoIntegrationTest + { + private IMainDomKeyGenerator MainDomKeyGenerator { get; set; } + + private IHostingEnvironment HostingEnvironment { get; set; } + + private FileSystemMainDomLock FileSystemMainDomLock { get; set; } + + private string LockFilePath { get; set; } + private string LockReleaseFilePath { get; set; } + + [SetUp] + public void SetUp() + { + MainDomKeyGenerator = GetRequiredService(); + HostingEnvironment = GetRequiredService(); + + var lockFileName = $"MainDom_{MainDomKeyGenerator.GenerateKey()}.lock"; + LockFilePath = Path.Combine(HostingEnvironment.LocalTempPath, lockFileName); + LockReleaseFilePath = LockFilePath + "_release"; + + var log = GetRequiredService>(); + FileSystemMainDomLock = new FileSystemMainDomLock(log, MainDomKeyGenerator, HostingEnvironment); + } + + [TearDown] + public void TearDown() + { + while (File.Exists(LockFilePath)) + { + File.Delete(LockFilePath); + } + while (File.Exists(LockReleaseFilePath)) + { + File.Delete(LockReleaseFilePath); + } + } + + [Test] + public async Task AcquireLockAsync_WhenNoOtherHoldsLockFileHandle_ReturnsTrue() + { + using var sut = FileSystemMainDomLock; + + var result = await sut.AcquireLockAsync(1000); + + Assert.True(result); + } + + [Test] + public async Task AcquireLockAsync_WhenTimeoutExceeded_ReturnsFalse() + { + await using var lockFile = File.Open(LockFilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None); + + using var sut = FileSystemMainDomLock; + + var result = await sut.AcquireLockAsync(1000); + + Assert.False(result); + } + + [Test] + public async Task ListenAsync_WhenLockReleaseSignalFileFound_DropsLockFileHandle() + { + using var sut = FileSystemMainDomLock; + + await sut.AcquireLockAsync(1000); + + var before = await sut.AcquireLockAsync(1000); + + await using (_ = File.Open(LockReleaseFilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.ReadWrite)) + { + } + + await sut.ListenAsync(); + + var after = await sut.AcquireLockAsync(1000); + + Assert.Multiple(() => + { + Assert.False(before); + Assert.True(after); + }); + } + } +} diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/HostedServices/ScheduledPublishingTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/HostedServices/ScheduledPublishingTests.cs index 2a8f006ab6..08999affe2 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/HostedServices/ScheduledPublishingTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/HostedServices/ScheduledPublishingTests.cs @@ -2,12 +2,15 @@ // See LICENSE for more details. using System; +using System.Data; using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Moq; using NUnit.Framework; using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Runtime; +using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Sync; @@ -108,6 +111,11 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.HostedServices var mockServerMessenger = new Mock(); + var mockScopeProvider = new Mock(); + mockScopeProvider + .Setup(x => x.CreateScope(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Returns(Mock.Of()); + return new ScheduledPublishing( mockRunTimeState.Object, mockMainDom.Object, @@ -115,7 +123,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.HostedServices _mockContentService.Object, mockUmbracoContextFactory.Object, _mockLogger.Object, - mockServerMessenger.Object); + mockServerMessenger.Object, + mockScopeProvider.Object); } private void VerifyScheduledPublishingNotPerformed() => VerifyScheduledPublishingPerformed(Times.Never()); diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Runtime/DefaultMainDomKeyGeneratorTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Runtime/DefaultMainDomKeyGeneratorTests.cs new file mode 100644 index 0000000000..9b013aa38f --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Runtime/DefaultMainDomKeyGeneratorTests.cs @@ -0,0 +1,47 @@ +using AutoFixture.NUnit3; +using Microsoft.Extensions.Options; +using NUnit.Framework; +using Umbraco.Cms.Core.Configuration.Models; +using Umbraco.Cms.Core.Hosting; +using Umbraco.Cms.Infrastructure.Runtime; +using Umbraco.Cms.Tests.UnitTests.AutoFixture; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Runtime +{ + [TestFixture] + internal class DefaultMainDomKeyGeneratorTests + { + [Test] + [AutoMoqData] + public void GenerateKey_WithConfiguredDiscriminatorValue_AltersHash( + [Frozen] IHostingEnvironment hostingEnvironment, + [Frozen] GlobalSettings globalSettings, + [Frozen] IOptionsMonitor globalSettingsMonitor, + DefaultMainDomKeyGenerator sut, + string aDiscriminator) + { + var withoutDiscriminator = sut.GenerateKey(); + globalSettings.MainDomKeyDiscriminator = aDiscriminator; + var withDiscriminator = sut.GenerateKey(); + + Assert.AreNotEqual(withoutDiscriminator, withDiscriminator); + } + + [Test] + [AutoMoqData] + public void GenerateKey_WithUnchangedDiscriminatorValue_ReturnsSameValue( + [Frozen] IHostingEnvironment hostingEnvironment, + [Frozen] GlobalSettings globalSettings, + [Frozen] IOptionsMonitor globalSettingsMonitor, + DefaultMainDomKeyGenerator sut, + string aDiscriminator) + { + globalSettings.MainDomKeyDiscriminator = aDiscriminator; + + var a = sut.GenerateKey(); + var b = sut.GenerateKey(); + + Assert.AreEqual(a, b); + } + } +} From 366d7c0fce203d92a9dbe79973cfb23a0debd087 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Thu, 24 Feb 2022 11:30:20 +0100 Subject: [PATCH 05/22] Update docfx dependency (#12046) * Update docfx * temp commit to test out azure pipeline * Rollback temp fix --- build/azure-pipelines.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build/azure-pipelines.yml b/build/azure-pipelines.yml index 7f231bd298..38d5e7673b 100644 --- a/build/azure-pipelines.yml +++ b/build/azure-pipelines.yml @@ -538,7 +538,7 @@ stages: inputs: targetType: inline script: | - choco install docfx --version=2.58.5 -y + choco install docfx --version=2.59.0 -y if ($lastexitcode -ne 0){ throw ("Error installing DocFX") } From 321d5b49bdaabbc5614b8f098416683cc8186a6e Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Thu, 24 Feb 2022 12:40:06 +0100 Subject: [PATCH 06/22] Fix docfx namespace (#12048) * Temp hack for building docs on pr * Fix namespace * revert hack to build docs on pr --- src/ApiDocs/umbracotemplate/partials/class.tmpl.partial | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ApiDocs/umbracotemplate/partials/class.tmpl.partial b/src/ApiDocs/umbracotemplate/partials/class.tmpl.partial index 9153a863a4..aa50d597ba 100644 --- a/src/ApiDocs/umbracotemplate/partials/class.tmpl.partial +++ b/src/ApiDocs/umbracotemplate/partials/class.tmpl.partial @@ -15,8 +15,8 @@
{{item.name.0.value}}
{{/inheritance.0}} -
{{__global.namespace}}:{{namespace}}
-
{{__global.assembly}}:{{assemblies.0}}.dll
+
{{__global.namespace}}: {{{namespace.specName.0.value}}}
+
{{__global.assembly}}: {{assemblies.0}}.dll
{{__global.syntax}}
{{syntax.content.0.value}}
From de4b3af28f091c4dec61df5eb17203eb98860528 Mon Sep 17 00:00:00 2001 From: Paul Johnson Date: Thu, 24 Feb 2022 14:38:33 +0000 Subject: [PATCH 07/22] Resolve various points related to deficiencies in FileSystemMainDomLock (#12052) * Resolve various points related to deficiencies in FileSystemMainDomLock See GH #12049 * Increasing backoff time for retry when deleting lock release signal file However reducing max tries, really hoping this never actually happens and if it does, failing to boot ASAP seems reasonable. --- .../Runtime/FileSystemMainDomLock.cs | 48 ++++++++++++------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs b/src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs index d18e4085a0..f77cb74c3f 100644 --- a/src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs +++ b/src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Diagnostics; using System.IO; using System.Threading; @@ -11,21 +12,22 @@ namespace Umbraco.Cms.Infrastructure.Runtime { internal class FileSystemMainDomLock : IMainDomLock { - private readonly ILogger _log; - + private readonly ILogger _logger; private readonly CancellationTokenSource _cancellationTokenSource = new(); - private readonly string _lockFilePath; private readonly string _releaseSignalFilePath; private FileStream _lockFileStream; + private Task _listenForReleaseSignalFileTask; + + private const int s_maxTriesRemovingLockReleaseSignalFile = 3; public FileSystemMainDomLock( - ILogger log, + ILogger logger, IMainDomKeyGenerator mainDomKeyGenerator, IHostingEnvironment hostingEnvironment) { - _log = log; + _logger = logger; var lockFileName = $"MainDom_{mainDomKeyGenerator.GenerateKey()}.lock"; _lockFilePath = Path.Combine(hostingEnvironment.LocalTempPath, lockFileName); @@ -41,20 +43,20 @@ namespace Umbraco.Cms.Infrastructure.Runtime { try { - _log.LogDebug("Attempting to obtain MainDom lock file handle {lockFilePath}", _lockFilePath); + _logger.LogDebug("Attempting to obtain MainDom lock file handle {lockFilePath}", _lockFilePath); _lockFileStream = File.Open(_lockFilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None); DeleteLockReleaseFile(); return Task.FromResult(true); } catch (IOException) { - _log.LogDebug("Couldn't obtain MainDom lock file handle, signalling for release of {lockFilePath}", _lockFilePath); + _logger.LogDebug("Couldn't obtain MainDom lock file handle, signalling for release of {lockFilePath}", _lockFilePath); CreateLockReleaseFile(); Thread.Sleep(500); } catch (Exception ex) { - _log.LogError(ex, "Unexpected exception attempting to obtain MainDom lock file handle {lockFilePath}, giving up", _lockFilePath); + _logger.LogError(ex, "Unexpected exception attempting to obtain MainDom lock file handle {lockFilePath}, giving up", _lockFilePath); return Task.FromResult(false); } } @@ -64,13 +66,22 @@ namespace Umbraco.Cms.Infrastructure.Runtime } // Create a long running task to poll to check if anyone has created a lock release file. - public Task ListenAsync() => - Task.Factory.StartNew( + public Task ListenAsync() + { + if (_listenForReleaseSignalFileTask != null) + { + return _listenForReleaseSignalFileTask; + } + + _listenForReleaseSignalFileTask = Task.Factory.StartNew( ListeningLoop, _cancellationTokenSource.Token, TaskCreationOptions.LongRunning, TaskScheduler.Default); + return _listenForReleaseSignalFileTask; + } + public void Dispose() { _lockFileStream?.Close(); @@ -86,24 +97,29 @@ namespace Umbraco.Cms.Infrastructure.Runtime } catch (Exception ex) { - _log.LogError(ex, "Unexpected exception attempting to create lock release signal file {file}", _releaseSignalFilePath); + _logger.LogError(ex, "Unexpected exception attempting to create lock release signal file {file}", _releaseSignalFilePath); } } private void DeleteLockReleaseFile() { - while (File.Exists(_releaseSignalFilePath)) + List encounteredExceptions = new(); + for (var i = 0; i < s_maxTriesRemovingLockReleaseSignalFile; i++) { try { File.Delete(_releaseSignalFilePath); + return; } catch (Exception ex) { - _log.LogError(ex, "Unexpected exception attempting to delete release signal file {file}", _releaseSignalFilePath); - Thread.Sleep(500); + _logger.LogError(ex, "Unexpected exception attempting to delete release signal file {file}", _releaseSignalFilePath); + encounteredExceptions.Add(ex); + Thread.Sleep(500 * (i + 1)); } } + + throw new ApplicationException($"Failed to remove lock release signal file {_releaseSignalFilePath}", new AggregateException(encounteredExceptions)); } private void ListeningLoop() @@ -112,13 +128,13 @@ namespace Umbraco.Cms.Infrastructure.Runtime { if (_cancellationTokenSource.IsCancellationRequested) { - _log.LogDebug("ListenAsync Task canceled, exiting loop"); + _logger.LogDebug("ListenAsync Task canceled, exiting loop"); return; } if (File.Exists(_releaseSignalFilePath)) { - _log.LogDebug("Found lock release signal file, releasing lock on {lockFilePath}", _lockFilePath); + _logger.LogDebug("Found lock release signal file, releasing lock on {lockFilePath}", _lockFilePath); _lockFileStream?.Close(); _lockFileStream = null; break; From 4351ce6ee4b544bb4575d15f9b70c61ffacb6b55 Mon Sep 17 00:00:00 2001 From: Paul Johnson Date: Fri, 25 Feb 2022 08:22:37 +0000 Subject: [PATCH 08/22] Further changes requested during review of #12049 (#12053) --- .../Configuration/Models/GlobalSettings.cs | 13 ++++ .../UmbracoBuilder.CoreServices.cs | 2 +- .../Runtime/FileSystemMainDomLock.cs | 70 ++++++------------- .../Runtime/SqlMainDomLock.cs | 2 +- .../Runtime/FileSystemMainDomLockTests.cs | 13 ++-- 5 files changed, 45 insertions(+), 55 deletions(-) diff --git a/src/Umbraco.Core/Configuration/Models/GlobalSettings.cs b/src/Umbraco.Core/Configuration/Models/GlobalSettings.cs index 53584af1d1..31068efd9f 100644 --- a/src/Umbraco.Core/Configuration/Models/GlobalSettings.cs +++ b/src/Umbraco.Core/Configuration/Models/GlobalSettings.cs @@ -29,6 +29,7 @@ namespace Umbraco.Cms.Core.Configuration.Models internal const string StaticNoNodesViewPath = "~/umbraco/UmbracoWebsite/NoNodes.cshtml"; internal const string StaticSqlWriteLockTimeOut = "00:00:05"; internal const bool StaticSanitizeTinyMce = false; + internal const int StaticMainDomReleaseSignalPollingInterval = 2000; /// /// Gets or sets a value for the reserved URLs (must end with a comma). @@ -145,6 +146,18 @@ namespace Umbraco.Cms.Core.Configuration.Models /// public string MainDomKeyDiscriminator { get; set; } = string.Empty; + /// + /// Gets or sets the duration (in milliseconds) for which the MainDomLock release signal polling task should sleep. + /// + /// + /// Doesn't apply to MainDomSemaphoreLock. + /// + /// The default value is 2000ms. + /// + /// + [DefaultValue(StaticMainDomReleaseSignalPollingInterval)] + public int MainDomReleaseSignalPollingInterval { get; set; } = StaticMainDomReleaseSignalPollingInterval; + /// /// Gets or sets the telemetry ID. /// diff --git a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs index 1c8b3ec0de..c4b9a6367c 100644 --- a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs +++ b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs @@ -234,7 +234,7 @@ namespace Umbraco.Cms.Infrastructure.DependencyInjection if (globalSettings.Value.MainDomLock == "FileSystemMainDomLock") { - return new FileSystemMainDomLock(loggerFactory.CreateLogger(), mainDomKeyGenerator, hostingEnvironment); + return new FileSystemMainDomLock(loggerFactory.CreateLogger(), mainDomKeyGenerator, hostingEnvironment, factory.GetRequiredService>()); } return globalSettings.Value.MainDomLock.Equals("SqlMainDomLock") || isWindows == false diff --git a/src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs b/src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs index f77cb74c3f..32b93513b4 100644 --- a/src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs +++ b/src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs @@ -1,10 +1,11 @@ using System; -using System.Collections.Generic; using System.Diagnostics; using System.IO; using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Hosting; using Umbraco.Cms.Core.Runtime; @@ -13,6 +14,7 @@ namespace Umbraco.Cms.Infrastructure.Runtime internal class FileSystemMainDomLock : IMainDomLock { private readonly ILogger _logger; + private readonly IOptionsMonitor _globalSettings; private readonly CancellationTokenSource _cancellationTokenSource = new(); private readonly string _lockFilePath; private readonly string _releaseSignalFilePath; @@ -20,14 +22,14 @@ namespace Umbraco.Cms.Infrastructure.Runtime private FileStream _lockFileStream; private Task _listenForReleaseSignalFileTask; - private const int s_maxTriesRemovingLockReleaseSignalFile = 3; - public FileSystemMainDomLock( ILogger logger, IMainDomKeyGenerator mainDomKeyGenerator, - IHostingEnvironment hostingEnvironment) + IHostingEnvironment hostingEnvironment, + IOptionsMonitor globalSettings) { _logger = logger; + _globalSettings = globalSettings; var lockFileName = $"MainDom_{mainDomKeyGenerator.GenerateKey()}.lock"; _lockFilePath = Path.Combine(hostingEnvironment.LocalTempPath, lockFileName); @@ -45,18 +47,18 @@ namespace Umbraco.Cms.Infrastructure.Runtime { _logger.LogDebug("Attempting to obtain MainDom lock file handle {lockFilePath}", _lockFilePath); _lockFileStream = File.Open(_lockFilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None); - DeleteLockReleaseFile(); + DeleteLockReleaseSignalFile(); return Task.FromResult(true); } catch (IOException) { _logger.LogDebug("Couldn't obtain MainDom lock file handle, signalling for release of {lockFilePath}", _lockFilePath); - CreateLockReleaseFile(); - Thread.Sleep(500); + CreateLockReleaseSignalFile(); } catch (Exception ex) { _logger.LogError(ex, "Unexpected exception attempting to obtain MainDom lock file handle {lockFilePath}, giving up", _lockFilePath); + _lockFileStream?.Close(); return Task.FromResult(false); } } @@ -65,6 +67,12 @@ namespace Umbraco.Cms.Infrastructure.Runtime return Task.FromResult(false); } + public void CreateLockReleaseSignalFile() => + _ = File.Open(_releaseSignalFilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.ReadWrite | FileShare.Delete); + + public void DeleteLockReleaseSignalFile() => + File.Delete(_releaseSignalFilePath); + // Create a long running task to poll to check if anyone has created a lock release file. public Task ListenAsync() { @@ -82,46 +90,6 @@ namespace Umbraco.Cms.Infrastructure.Runtime return _listenForReleaseSignalFileTask; } - public void Dispose() - { - _lockFileStream?.Close(); - _lockFileStream = null; - } - - private void CreateLockReleaseFile() - { - try - { - // Dispose immediately to release the file handle so it's easier to cleanup in any process. - using FileStream releaseFileStream = File.Open(_releaseSignalFilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.ReadWrite); - } - catch (Exception ex) - { - _logger.LogError(ex, "Unexpected exception attempting to create lock release signal file {file}", _releaseSignalFilePath); - } - } - - private void DeleteLockReleaseFile() - { - List encounteredExceptions = new(); - for (var i = 0; i < s_maxTriesRemovingLockReleaseSignalFile; i++) - { - try - { - File.Delete(_releaseSignalFilePath); - return; - } - catch (Exception ex) - { - _logger.LogError(ex, "Unexpected exception attempting to delete release signal file {file}", _releaseSignalFilePath); - encounteredExceptions.Add(ex); - Thread.Sleep(500 * (i + 1)); - } - } - - throw new ApplicationException($"Failed to remove lock release signal file {_releaseSignalFilePath}", new AggregateException(encounteredExceptions)); - } - private void ListeningLoop() { while (true) @@ -140,8 +108,14 @@ namespace Umbraco.Cms.Infrastructure.Runtime break; } - Thread.Sleep(2000); + Thread.Sleep(_globalSettings.CurrentValue.MainDomReleaseSignalPollingInterval); } } + + public void Dispose() + { + _lockFileStream?.Close(); + _lockFileStream = null; + } } } diff --git a/src/Umbraco.Infrastructure/Runtime/SqlMainDomLock.cs b/src/Umbraco.Infrastructure/Runtime/SqlMainDomLock.cs index 635b0b9c28..8a6698b92a 100644 --- a/src/Umbraco.Infrastructure/Runtime/SqlMainDomLock.cs +++ b/src/Umbraco.Infrastructure/Runtime/SqlMainDomLock.cs @@ -236,7 +236,7 @@ namespace Umbraco.Cms.Infrastructure.Runtime { // poll every couple of seconds // local testing shows the actual query to be executed from client/server is approx 300ms but would change depending on environment/IO - Thread.Sleep(2000); + Thread.Sleep(_globalSettings.Value.MainDomReleaseSignalPollingInterval); if (!_dbFactory.Configured) { diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Runtime/FileSystemMainDomLockTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Runtime/FileSystemMainDomLockTests.cs index 59442a683a..c709a756b8 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Runtime/FileSystemMainDomLockTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Runtime/FileSystemMainDomLockTests.cs @@ -1,7 +1,10 @@ using System.IO; using System.Threading.Tasks; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Moq; using NUnit.Framework; +using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Hosting; using Umbraco.Cms.Core.Runtime; using Umbraco.Cms.Infrastructure.Runtime; @@ -31,8 +34,11 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Runtime LockFilePath = Path.Combine(HostingEnvironment.LocalTempPath, lockFileName); LockReleaseFilePath = LockFilePath + "_release"; + var globalSettings = Mock.Of>(); + Mock.Get(globalSettings).Setup(x => x.CurrentValue).Returns(new GlobalSettings()); + var log = GetRequiredService>(); - FileSystemMainDomLock = new FileSystemMainDomLock(log, MainDomKeyGenerator, HostingEnvironment); + FileSystemMainDomLock = new FileSystemMainDomLock(log, MainDomKeyGenerator, HostingEnvironment, globalSettings); } [TearDown] @@ -79,10 +85,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Runtime var before = await sut.AcquireLockAsync(1000); - await using (_ = File.Open(LockReleaseFilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.ReadWrite)) - { - } - + sut.CreateLockReleaseSignalFile(); await sut.ListenAsync(); var after = await sut.AcquireLockAsync(1000); From 4a6c409a1f6b222646daa4a6796ce11d6a812fb5 Mon Sep 17 00:00:00 2001 From: Paul Johnson Date: Fri, 25 Feb 2022 10:56:45 +0000 Subject: [PATCH 09/22] Explicitly close release signal file. (#12057) --- .../Runtime/FileSystemMainDomLock.cs | 3 ++- .../Runtime/FileSystemMainDomLockTests.cs | 23 ++++++++++++++----- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs b/src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs index 32b93513b4..c4cbcef588 100644 --- a/src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs +++ b/src/Umbraco.Infrastructure/Runtime/FileSystemMainDomLock.cs @@ -68,7 +68,8 @@ namespace Umbraco.Cms.Infrastructure.Runtime } public void CreateLockReleaseSignalFile() => - _ = File.Open(_releaseSignalFilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.ReadWrite | FileShare.Delete); + File.Open(_releaseSignalFilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.ReadWrite | FileShare.Delete) + .Close(); public void DeleteLockReleaseSignalFile() => File.Delete(_releaseSignalFilePath); diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Runtime/FileSystemMainDomLockTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Runtime/FileSystemMainDomLockTests.cs index c709a756b8..ebb9b8a6a7 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Runtime/FileSystemMainDomLockTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Runtime/FileSystemMainDomLockTests.cs @@ -1,4 +1,5 @@ using System.IO; +using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -44,13 +45,23 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Runtime [TearDown] public void TearDown() { - while (File.Exists(LockFilePath)) + CleanupTestFile(LockFilePath); + CleanupTestFile(LockReleaseFilePath); + } + + private static void CleanupTestFile(string path) + { + for (var i = 0; i < 3; i++) { - File.Delete(LockFilePath); - } - while (File.Exists(LockReleaseFilePath)) - { - File.Delete(LockReleaseFilePath); + try + { + File.Delete(path); + return; + } + catch + { + Thread.Sleep(500 * (i + 1)); + } } } From 0c7ef060314440ef3d59e5e7f7bb1f5fd3b62be2 Mon Sep 17 00:00:00 2001 From: Mole Date: Mon, 28 Feb 2022 13:59:39 +0100 Subject: [PATCH 10/22] 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 Co-authored-by: Bjarke Berg --- .../DependencyInjection/UmbracoBuilder.cs | 1 + .../InstallSteps/TelemetryIdentifierStep.cs | 35 ++++---- .../Telemetry/ISiteIdentifierService.cs | 31 +++++++ .../Telemetry/SiteIdentifierService.cs | 81 +++++++++++++++++++ .../Telemetry/TelemetryService.cs | 32 ++------ .../UmbracoBuilder.Installer.cs | 10 ++- .../HostedServices/ReportSiteTask.cs | 3 - .../Controllers/DashboardController.cs | 38 ++++++++- .../Telemetry/SiteIdentifierServiceTests.cs | 77 ++++++++++++++++++ .../Telemetry/TelemetryServiceTests.cs | 59 +++++++------- 10 files changed, 287 insertions(+), 80 deletions(-) create mode 100644 src/Umbraco.Core/Telemetry/ISiteIdentifierService.cs create mode 100644 src/Umbraco.Core/Telemetry/SiteIdentifierService.cs create mode 100644 tests/Umbraco.Tests.UnitTests/Umbraco.Core/Telemetry/SiteIdentifierServiceTests.cs diff --git a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs index c4a95d45e5..235dc71252 100644 --- a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs +++ b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs @@ -262,6 +262,7 @@ namespace Umbraco.Cms.Core.DependencyInjection Services.AddSingleton(); // Register telemetry service used to gather data about installed packages + Services.AddUnique(); Services.AddUnique(); // Register a noop IHtmlSanitizer to be replaced diff --git a/src/Umbraco.Core/Install/InstallSteps/TelemetryIdentifierStep.cs b/src/Umbraco.Core/Install/InstallSteps/TelemetryIdentifierStep.cs index 37769afc53..d95fa6919d 100644 --- a/src/Umbraco.Core/Install/InstallSteps/TelemetryIdentifierStep.cs +++ b/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 { - private readonly ILogger _logger; private readonly IOptions _globalSettings; - private readonly IConfigManipulator _configManipulator; + private readonly ISiteIdentifierService _siteIdentifierService; - public TelemetryIdentifierStep(ILogger logger, IOptions globalSettings, IConfigManipulator configManipulator) + public TelemetryIdentifierStep( + IOptions globalSettings, + ISiteIdentifierService siteIdentifierService) { - _logger = logger; _globalSettings = globalSettings; - _configManipulator = configManipulator; + _siteIdentifierService = siteIdentifierService; + } + + [Obsolete("Use constructor that takes GlobalSettings and ISiteIdentifierService")] + public TelemetryIdentifierStep( + ILogger logger, + IOptions globalSettings, + IConfigManipulator configManipulator) + : this(globalSettings, StaticServiceProvider.Instance.GetRequiredService()) + { } public override Task 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(null); } diff --git a/src/Umbraco.Core/Telemetry/ISiteIdentifierService.cs b/src/Umbraco.Core/Telemetry/ISiteIdentifierService.cs new file mode 100644 index 0000000000..7fd0ee5a85 --- /dev/null +++ b/src/Umbraco.Core/Telemetry/ISiteIdentifierService.cs @@ -0,0 +1,31 @@ +using System; + +namespace Umbraco.Cms.Core.Telemetry +{ + /// + /// Used to get and create the site identifier + /// + public interface ISiteIdentifierService + { + + /// + /// Tries to get the site identifier + /// + /// True if success. + bool TryGetSiteIdentifier(out Guid siteIdentifier); + + /// + /// Creates the site identifier and writes it to config. + /// + /// asd. + /// True if success. + bool TryCreateSiteIdentifier(out Guid createdGuid); + + /// + /// Tries to get the site identifier or otherwise create it if it doesn't exist. + /// + /// The out parameter for the existing or create site identifier. + /// True if success. + bool TryGetOrCreateSiteIdentifier(out Guid siteIdentifier); + } +} diff --git a/src/Umbraco.Core/Telemetry/SiteIdentifierService.cs b/src/Umbraco.Core/Telemetry/SiteIdentifierService.cs new file mode 100644 index 0000000000..b6e40665c1 --- /dev/null +++ b/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 +{ + /// + internal class SiteIdentifierService : ISiteIdentifierService + { + private GlobalSettings _globalSettings; + private readonly IConfigManipulator _configManipulator; + private readonly ILogger _logger; + + public SiteIdentifierService( + IOptionsMonitor optionsMonitor, + IConfigManipulator configManipulator, + ILogger logger) + { + _globalSettings = optionsMonitor.CurrentValue; + optionsMonitor.OnChange(globalSettings => _globalSettings = globalSettings); + _configManipulator = configManipulator; + _logger = logger; + } + + /// + 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; + } + + /// + 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; + } + + /// + 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; + } + } +} diff --git a/src/Umbraco.Core/Telemetry/TelemetryService.cs b/src/Umbraco.Core/Telemetry/TelemetryService.cs index 63e4e1ff49..d5a3acac98 100644 --- a/src/Umbraco.Core/Telemetry/TelemetryService.cs +++ b/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 /// internal class TelemetryService : ITelemetryService { - private readonly IOptionsMonitor _globalSettings; private readonly IManifestParser _manifestParser; private readonly IUmbracoVersion _umbracoVersion; + private readonly ISiteIdentifierService _siteIdentifierService; /// /// Initializes a new instance of the class. /// public TelemetryService( - IOptionsMonitor globalSettings, IManifestParser manifestParser, - IUmbracoVersion umbracoVersion) + IUmbracoVersion umbracoVersion, + ISiteIdentifierService siteIdentifierService) { _manifestParser = manifestParser; _umbracoVersion = umbracoVersion; - _globalSettings = globalSettings; + _siteIdentifierService = siteIdentifierService; } /// 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 GetPackageTelemetry() { - List packages = new (); + List packages = new(); IEnumerable 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, }); } diff --git a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Installer.cs b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Installer.cs index e0958bfdb7..d750eb15e0 100644 --- a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Installer.cs +++ b/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 @@ namespace Umbraco.Cms.Infrastructure.DependencyInjection builder.Services.AddScoped(); builder.Services.AddScoped(); builder.Services.AddScoped(); - builder.Services.AddScoped(); + builder.Services.AddScoped(provider => + { + return new TelemetryIdentifierStep( + provider.GetRequiredService>(), + provider.GetRequiredService()); + }); builder.Services.AddScoped(); builder.Services.AddScoped(); builder.Services.AddScoped(); diff --git a/src/Umbraco.Infrastructure/HostedServices/ReportSiteTask.cs b/src/Umbraco.Infrastructure/HostedServices/ReportSiteTask.cs index 7591290bf4..cfce96281c 100644 --- a/src/Umbraco.Infrastructure/HostedServices/ReportSiteTask.cs +++ b/src/Umbraco.Infrastructure/HostedServices/ReportSiteTask.cs @@ -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/"); diff --git a/src/Umbraco.Web.BackOffice/Controllers/DashboardController.cs b/src/Umbraco.Web.BackOffice/Controllers/DashboardController.cs index 955081fa73..342686ceb3 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/DashboardController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/DashboardController.cs @@ -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; + /// /// Initializes a new instance of the with all its dependencies. /// + [ActivatorUtilitiesConstructor] public DashboardController( IBackOfficeSecurityAccessor backOfficeSecurityAccessor, AppCaches appCaches, @@ -54,7 +60,8 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers IDashboardService dashboardService, IUmbracoVersion umbracoVersion, IShortStringHelper shortStringHelper, - IOptions dashboardSettings) + IOptions 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 logger, + IDashboardService dashboardService, + IUmbracoVersion umbracoVersion, + IShortStringHelper shortStringHelper, + IOptions dashboardSettings) + : this( + backOfficeSecurityAccessor, + appCaches, + logger, + dashboardService, + umbracoVersion, + shortStringHelper, + dashboardSettings, + StaticServiceProvider.Instance.GetRequiredService()) + { + } + //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(key); diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Telemetry/SiteIdentifierServiceTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Telemetry/SiteIdentifierServiceTests.cs new file mode 100644 index 0000000000..81934cc1be --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Telemetry/SiteIdentifierServiceTests.cs @@ -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(), + Mock.Of>()); + + 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(); + + var sut = new SiteIdentifierService( + globalSettings, + configManipulatorMock.Object, + Mock.Of>()); + + var result = sut.TryGetOrCreateSiteIdentifier(out var identifier); + + if (shouldCreate) + { + configManipulatorMock.Verify(x => x.SetGlobalId(It.IsAny()), Times.Once); + Assert.AreNotEqual(Guid.Empty, identifier); + Assert.IsTrue(result); + } + else + { + configManipulatorMock.Verify(x => x.SetGlobalId(It.IsAny()), Times.Never()); + Assert.AreEqual(guidString.ToLower(), identifier.ToString()); + Assert.IsTrue(result); + } + } + + private IOptionsMonitor CreateGlobalSettings(string guidString) + { + var globalSettings = new GlobalSettings { Id = guidString }; + return Mock.Of>(x => x.CurrentValue == globalSettings); + } + } +} diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Telemetry/TelemetryServiceTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Telemetry/TelemetryServiceTests.cs index 1c92569695..910ca7c792 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Telemetry/TelemetryServiceTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Telemetry/TelemetryServiceTests.cs @@ -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(), version); + var version = CreateUmbracoVersion(9, 3, 1); + var siteIdentifierServiceMock = new Mock(); + var sut = new TelemetryService(Mock.Of(), 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(), 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(), version); + var sut = new TelemetryService(Mock.Of(), 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(x => x.SemanticVersion == version); } - private IOptionsMonitor 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>(x => x.CurrentValue == globalSettings); + var mock = new Mock(); + var siteIdentifier = shouldSucceed ? Guid.NewGuid() : Guid.Empty; + mock.Setup(x => x.TryGetOrCreateSiteIdentifier(out siteIdentifier)).Returns(shouldSucceed); + return mock.Object; } } } From dfbb182a9459266200e2b38dde77259326e7e801 Mon Sep 17 00:00:00 2001 From: nikolajlauridsen Date: Mon, 28 Feb 2022 17:29:11 +0100 Subject: [PATCH 11/22] Handle invariant culture in RedirectTrackingHandler GetRouteById and RedirectUrlService expects the culture to be null if it's invariant, however, Cultures in IPublished content uses empty string for invariant culture --- .../Routing/RedirectTrackingHandler.cs | 32 +++++++++++++------ 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs b/src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs index 7f99b32b02..f7c4a8ef0a 100644 --- a/src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs +++ b/src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs @@ -99,25 +99,33 @@ namespace Umbraco.Cms.Core.Routing { return; } - var contentCache = publishedSnapshot.Content; - var entityContent = contentCache?.GetById(entity.Id); + + IPublishedContentCache contentCache = publishedSnapshot.Content; + IPublishedContent entityContent = contentCache?.GetById(entity.Id); if (entityContent == null) + { return; + } // get the default affected cultures by going up the tree until we find the first culture variant entity (default to no cultures) var defaultCultures = entityContent.AncestorsOrSelf()?.FirstOrDefault(a => a.Cultures.Any())?.Cultures.Keys.ToArray() ?? new[] { (string)null }; - foreach (var x in entityContent.DescendantsOrSelf(_variationContextAccessor)) + + foreach (IPublishedContent publishedContent in entityContent.DescendantsOrSelf(_variationContextAccessor)) { // if this entity defines specific cultures, use those instead of the default ones - var cultures = x.Cultures.Any() ? x.Cultures.Keys : defaultCultures; + IEnumerable cultures = publishedContent.Cultures.Any() ? publishedContent.Cultures.Keys : defaultCultures; foreach (var culture in cultures) { - var route = contentCache.GetRouteById(x.Id, culture); + var checkedCulture = string.IsNullOrEmpty(culture) ? null : culture; + var route = contentCache.GetRouteById(publishedContent.Id, checkedCulture); if (IsNotRoute(route)) + { continue; - oldRoutes[new ContentIdAndCulture(x.Id, culture)] = new ContentKeyAndOldRoute(x.Key, route); + } + + oldRoutes[new ContentIdAndCulture(publishedContent.Id, culture)] = new ContentKeyAndOldRoute(publishedContent.Key, route); } } } @@ -135,14 +143,18 @@ namespace Umbraco.Cms.Core.Routing { _logger.LogWarning("Could not track redirects because there is no current published snapshot available."); return; - } + } - foreach (var oldRoute in oldRoutes) + foreach (KeyValuePair oldRoute in oldRoutes) { - var newRoute = contentCache.GetRouteById(oldRoute.Key.ContentId, oldRoute.Key.Culture); + var culture = string.IsNullOrWhiteSpace(oldRoute.Key.Culture) ? null : oldRoute.Key.Culture; + var newRoute = contentCache.GetRouteById(oldRoute.Key.ContentId, culture); if (IsNotRoute(newRoute) || oldRoute.Value.OldRoute == newRoute) + { continue; - _redirectUrlService.Register(oldRoute.Value.OldRoute, oldRoute.Value.ContentKey, oldRoute.Key.Culture); + } + + _redirectUrlService.Register(oldRoute.Value.OldRoute, oldRoute.Value.ContentKey, culture); } } From 993c582bd910c31af7ea913e6e4b1d94d825786d Mon Sep 17 00:00:00 2001 From: nikolajlauridsen Date: Tue, 1 Mar 2022 13:51:21 +0100 Subject: [PATCH 12/22] Handle empty string as invariant when generating cache key --- .../Routing/RedirectTrackingHandler.cs | 8 +++----- src/Umbraco.PublishedCache.NuCache/CacheKeys.cs | 4 +--- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs b/src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs index f7c4a8ef0a..556c548cae 100644 --- a/src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs +++ b/src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs @@ -118,8 +118,7 @@ namespace Umbraco.Cms.Core.Routing foreach (var culture in cultures) { - var checkedCulture = string.IsNullOrEmpty(culture) ? null : culture; - var route = contentCache.GetRouteById(publishedContent.Id, checkedCulture); + var route = contentCache.GetRouteById(publishedContent.Id, culture); if (IsNotRoute(route)) { continue; @@ -147,14 +146,13 @@ namespace Umbraco.Cms.Core.Routing foreach (KeyValuePair oldRoute in oldRoutes) { - var culture = string.IsNullOrWhiteSpace(oldRoute.Key.Culture) ? null : oldRoute.Key.Culture; - var newRoute = contentCache.GetRouteById(oldRoute.Key.ContentId, culture); + var newRoute = contentCache.GetRouteById(oldRoute.Key.ContentId, oldRoute.Key.Culture); if (IsNotRoute(newRoute) || oldRoute.Value.OldRoute == newRoute) { continue; } - _redirectUrlService.Register(oldRoute.Value.OldRoute, oldRoute.Value.ContentKey, culture); + _redirectUrlService.Register(oldRoute.Value.OldRoute, oldRoute.Value.ContentKey, oldRoute.Key.Culture); } } diff --git a/src/Umbraco.PublishedCache.NuCache/CacheKeys.cs b/src/Umbraco.PublishedCache.NuCache/CacheKeys.cs index 3d8f14afd3..0ec6f0b7cb 100644 --- a/src/Umbraco.PublishedCache.NuCache/CacheKeys.cs +++ b/src/Umbraco.PublishedCache.NuCache/CacheKeys.cs @@ -13,9 +13,7 @@ namespace Umbraco.Cms.Infrastructure.PublishedCache [MethodImpl(MethodImplOptions.AggressiveInlining)] private static string LangId(string culture) - { - return culture != null ? ("-L:" + culture) : string.Empty; - } + => string.IsNullOrEmpty(culture) ? string.Empty : ("-L:" + culture); public static string PublishedContentChildren(Guid contentUid, bool previewing) { From 95f1ed33ecc6e3390303f9bfc9f237531baac1c3 Mon Sep 17 00:00:00 2001 From: nikolajlauridsen Date: Tue, 1 Mar 2022 14:14:48 +0100 Subject: [PATCH 13/22] Make empty string invariant culture in DefaultUrlProvider --- src/Umbraco.Core/Routing/DefaultUrlProvider.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Core/Routing/DefaultUrlProvider.cs b/src/Umbraco.Core/Routing/DefaultUrlProvider.cs index 5c27760b2a..eea53aaa9c 100644 --- a/src/Umbraco.Core/Routing/DefaultUrlProvider.cs +++ b/src/Umbraco.Core/Routing/DefaultUrlProvider.cs @@ -159,7 +159,7 @@ namespace Umbraco.Cms.Core.Routing : DomainUtilities.DomainForNode(umbracoContext.PublishedSnapshot.Domains, _siteDomainMapper, int.Parse(route.Substring(0, pos), CultureInfo.InvariantCulture), current, culture); var defaultCulture = _localizationService.GetDefaultLanguageIsoCode(); - if (domainUri is not null || culture is null || culture.Equals(defaultCulture, StringComparison.InvariantCultureIgnoreCase)) + if (domainUri is not null || string.IsNullOrEmpty(culture) || culture.Equals(defaultCulture, StringComparison.InvariantCultureIgnoreCase)) { var url = AssembleUrl(domainUri, path, current, mode).ToString(); return UrlInfo.Url(url, culture); From 44ddf7acef2a13d47b282f32a59b8a44d17eae13 Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Wed, 2 Mar 2022 08:47:35 +0100 Subject: [PATCH 14/22] Add extra check (#12075) Co-authored-by: Nikolaj Geisle --- src/Umbraco.Web.Common/Extensions/ControllerExtensions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Web.Common/Extensions/ControllerExtensions.cs b/src/Umbraco.Web.Common/Extensions/ControllerExtensions.cs index 6ae94ab57f..911ecee8e5 100644 --- a/src/Umbraco.Web.Common/Extensions/ControllerExtensions.cs +++ b/src/Umbraco.Web.Common/Extensions/ControllerExtensions.cs @@ -24,7 +24,7 @@ namespace Umbraco.Extensions /// public static string GetControllerName(Type controllerType) { - if (!controllerType.Name.EndsWith("Controller")) + if (!controllerType.Name.EndsWith("Controller") && !controllerType.Name.EndsWith("Controller`1")) { throw new InvalidOperationException("The controller type " + controllerType + " does not follow conventions, MVC Controller class names must be suffixed with the term 'Controller'"); } From 009b36b49e59ee6ebe2c63a4ce9a96caffd4463c Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Wed, 2 Mar 2022 09:12:07 +0100 Subject: [PATCH 15/22] Send key from frontend to postsave (#12080) Co-authored-by: Nikolaj Geisle --- .../src/common/services/umbdataformatter.service.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Web.UI.Client/src/common/services/umbdataformatter.service.js b/src/Umbraco.Web.UI.Client/src/common/services/umbdataformatter.service.js index 7e4d7eaa4a..a8f62dca4b 100644 --- a/src/Umbraco.Web.UI.Client/src/common/services/umbdataformatter.service.js +++ b/src/Umbraco.Web.UI.Client/src/common/services/umbdataformatter.service.js @@ -154,7 +154,7 @@ formatUserPostData: function (displayModel) { //create the save model from the display model - var saveModel = _.pick(displayModel, 'id', 'parentId', 'name', 'username', 'culture', 'email', 'startContentIds', 'startMediaIds', 'userGroups', 'message'); + var saveModel = _.pick(displayModel, 'id', 'parentId', 'name', 'username', 'culture', 'email', 'startContentIds', 'startMediaIds', 'userGroups', 'message', 'key'); //make sure the userGroups are just a string array var currGroups = saveModel.userGroups; From 7e9def9df4515de780b859c30834611467b9ce75 Mon Sep 17 00:00:00 2001 From: nikolajlauridsen Date: Wed, 2 Mar 2022 10:11:33 +0100 Subject: [PATCH 16/22] Add tests --- .../Models/ContentTypeHistoryCleanupTests.cs | 105 ++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeHistoryCleanupTests.cs diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeHistoryCleanupTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeHistoryCleanupTests.cs new file mode 100644 index 0000000000..1a9d293527 --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeHistoryCleanupTests.cs @@ -0,0 +1,105 @@ +using System.Linq; +using NUnit.Framework; +using Umbraco.Cms.Core.Models.ContentEditing; +using Umbraco.Cms.Tests.Common.Builders; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Models +{ + [TestFixture] + public class ContentTypeHistoryCleanupTests + { + [Test] + public void Changing_Keep_all_Makes_ContentType_Dirty() + { + var contentType = ContentTypeBuilder.CreateBasicContentType(); + + Assert.IsFalse(contentType.IsDirty()); + + var newValue = 2; + contentType.HistoryCleanup.KeepAllVersionsNewerThanDays = newValue; + Assert.IsTrue(contentType.IsDirty()); + Assert.AreEqual(newValue, contentType.HistoryCleanup.KeepAllVersionsNewerThanDays); + } + + [Test] + public void Changing_Keep_latest_Makes_ContentType_Dirty() + { + var contentType = ContentTypeBuilder.CreateBasicContentType(); + + Assert.IsFalse(contentType.IsDirty()); + + var newValue = 2; + contentType.HistoryCleanup.KeepLatestVersionPerDayForDays = newValue; + Assert.IsTrue(contentType.IsDirty()); + Assert.AreEqual(newValue, contentType.HistoryCleanup.KeepLatestVersionPerDayForDays); + } + + [Test] + public void Changing_Prevent_Cleanup_Makes_ContentType_Dirty() + { + var contentType = ContentTypeBuilder.CreateBasicContentType(); + + Assert.IsFalse(contentType.IsDirty()); + + var newValue = true; + contentType.HistoryCleanup.PreventCleanup = newValue; + Assert.IsTrue(contentType.IsDirty()); + Assert.AreEqual(newValue, contentType.HistoryCleanup.PreventCleanup); + } + + [Test] + public void Replacing_History_Cleanup_Registers_As_Dirty() + { + var contentType = ContentTypeBuilder.CreateBasicContentType(); + Assert.IsFalse(contentType.IsDirty()); + + contentType.HistoryCleanup = new HistoryCleanup(); + + Assert.IsTrue(contentType.IsDirty()); + Assert.IsTrue(contentType.IsPropertyDirty(nameof(contentType.HistoryCleanup))); + } + + [Test] + public void Replacing_History_Cleanup_Removes_Old_Dirty_History_Properties() + { + var contentType = ContentTypeBuilder.CreateBasicContentType(); + + contentType.Alias = "NewValue"; + contentType.HistoryCleanup.KeepAllVersionsNewerThanDays = 2; + + contentType.PropertyChanged += (sender, args) => + { + // Ensure that property changed is only invoked for history cleanup + Assert.AreEqual(nameof(contentType.HistoryCleanup), args.PropertyName); + }; + + // Since we're replacing the entire HistoryCleanup the changed property is no longer dirty, the entire HistoryCleanup is + contentType.HistoryCleanup = new HistoryCleanup(); + + Assert.Multiple(() => + { + Assert.IsTrue(contentType.IsDirty()); + Assert.IsFalse(contentType.WasDirty()); + Assert.AreEqual(2, contentType.GetDirtyProperties().Count()); + Assert.IsTrue(contentType.IsPropertyDirty(nameof(contentType.HistoryCleanup))); + Assert.IsTrue(contentType.IsPropertyDirty(nameof(contentType.Alias))); + }); + } + + [Test] + public void Old_History_Cleanup_Reference_Doesnt_Make_Content_Type_Dirty() + { + var contentType = ContentTypeBuilder.CreateBasicContentType(); + var oldHistoryCleanup = contentType.HistoryCleanup; + + contentType.HistoryCleanup = new HistoryCleanup(); + contentType.ResetDirtyProperties(); + contentType.ResetWereDirtyProperties(); + + oldHistoryCleanup.KeepAllVersionsNewerThanDays = 2; + + Assert.IsFalse(contentType.IsDirty()); + Assert.IsFalse(contentType.WasDirty()); + } + } +} From 49e0e6c1c9cb7ba7bebdacdbd8cbcfe1b34050a6 Mon Sep 17 00:00:00 2001 From: nikolajlauridsen Date: Wed, 2 Mar 2022 10:11:48 +0100 Subject: [PATCH 17/22] Implement BeingDirtyBase on HistoryCleanup --- .../Models/ContentEditing/HistoryCleanup.cs | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Core/Models/ContentEditing/HistoryCleanup.cs b/src/Umbraco.Core/Models/ContentEditing/HistoryCleanup.cs index b7bfb32808..a0d9bbbcb3 100644 --- a/src/Umbraco.Core/Models/ContentEditing/HistoryCleanup.cs +++ b/src/Umbraco.Core/Models/ContentEditing/HistoryCleanup.cs @@ -1,17 +1,34 @@ using System.Runtime.Serialization; +using Umbraco.Cms.Core.Models.Entities; namespace Umbraco.Cms.Core.Models.ContentEditing { [DataContract(Name = "historyCleanup", Namespace = "")] - public class HistoryCleanup + public class HistoryCleanup : BeingDirtyBase { + private bool _preventCleanup; + private int? _keepAllVersionsNewerThanDays; + private int? _keepLatestVersionPerDayForDays; + [DataMember(Name = "preventCleanup")] - public bool PreventCleanup { get; set; } + public bool PreventCleanup + { + get => _preventCleanup; + set => SetPropertyValueAndDetectChanges(value, ref _preventCleanup, nameof(PreventCleanup)); + } [DataMember(Name = "keepAllVersionsNewerThanDays")] - public int? KeepAllVersionsNewerThanDays { get; set; } + public int? KeepAllVersionsNewerThanDays + { + get => _keepAllVersionsNewerThanDays; + set => SetPropertyValueAndDetectChanges(value, ref _keepAllVersionsNewerThanDays, nameof(KeepAllVersionsNewerThanDays)); + } [DataMember(Name = "keepLatestVersionPerDayForDays")] - public int? KeepLatestVersionPerDayForDays { get; set; } + public int? KeepLatestVersionPerDayForDays + { + get => _keepLatestVersionPerDayForDays; + set => SetPropertyValueAndDetectChanges(value, ref _keepLatestVersionPerDayForDays, nameof(KeepLatestVersionPerDayForDays)); + } } } From 1cdc6f0fd2d13cf57650a0dcbb7ec081d2d85156 Mon Sep 17 00:00:00 2001 From: nikolajlauridsen Date: Wed, 2 Mar 2022 10:15:50 +0100 Subject: [PATCH 18/22] Make HistoryCleanup register as dirty in ContentType --- src/Umbraco.Core/Models/ContentType.cs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Core/Models/ContentType.cs b/src/Umbraco.Core/Models/ContentType.cs index 6ff94f57f3..a252aa4723 100644 --- a/src/Umbraco.Core/Models/ContentType.cs +++ b/src/Umbraco.Core/Models/ContentType.cs @@ -96,7 +96,13 @@ namespace Umbraco.Cms.Core.Models } } - public HistoryCleanup HistoryCleanup { get; set; } + private HistoryCleanup _historyCleanup; + + public HistoryCleanup HistoryCleanup + { + get => _historyCleanup; + set => SetPropertyValueAndDetectChanges(value, ref _historyCleanup, nameof(HistoryCleanup)); + } /// /// Determines if AllowedTemplates contains templateId @@ -162,5 +168,8 @@ namespace Umbraco.Cms.Core.Models /// IContentType IContentType.DeepCloneWithResetIdentities(string newAlias) => (IContentType)DeepCloneWithResetIdentities(newAlias); + + /// + public override bool IsDirty() => base.IsDirty() || HistoryCleanup.IsDirty(); } } From ff477c3930d1ab638d5d4831f24b6522517ea956 Mon Sep 17 00:00:00 2001 From: Mole Date: Wed, 2 Mar 2022 10:26:23 +0100 Subject: [PATCH 19/22] Update src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> --- src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs b/src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs index 556c548cae..2ef2034d3d 100644 --- a/src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs +++ b/src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs @@ -102,7 +102,7 @@ namespace Umbraco.Cms.Core.Routing IPublishedContentCache contentCache = publishedSnapshot.Content; IPublishedContent entityContent = contentCache?.GetById(entity.Id); - if (entityContent == null) + if (entityContent is null) { return; } From 5ef013ca62f522f87fe50c57278b929b73d41344 Mon Sep 17 00:00:00 2001 From: nikolajlauridsen Date: Wed, 2 Mar 2022 10:49:06 +0100 Subject: [PATCH 20/22] Ensure ContentType is only marked as dirty when it's actually changed --- .../Mapping/ContentTypeMapDefinition.cs | 57 +++++++++++++++++-- 1 file changed, 52 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Core/Models/Mapping/ContentTypeMapDefinition.cs b/src/Umbraco.Core/Models/Mapping/ContentTypeMapDefinition.cs index 3625e90a14..4bb34017ec 100644 --- a/src/Umbraco.Core/Models/Mapping/ContentTypeMapDefinition.cs +++ b/src/Umbraco.Core/Models/Mapping/ContentTypeMapDefinition.cs @@ -133,7 +133,7 @@ namespace Umbraco.Cms.Core.Models.Mapping if (target is IContentTypeWithHistoryCleanup targetWithHistoryCleanup) { - targetWithHistoryCleanup.HistoryCleanup = source.HistoryCleanup; + MapHistoryCleanup(source, targetWithHistoryCleanup); } target.AllowedTemplates = source.AllowedTemplates @@ -147,6 +147,34 @@ namespace Umbraco.Cms.Core.Models.Mapping : _fileService.GetTemplate(source.DefaultTemplate)); } + private static void MapHistoryCleanup(DocumentTypeSave source, IContentTypeWithHistoryCleanup target) + { + // If source history cleanup is null we don't have to map all properties + if (source.HistoryCleanup is null) + { + target.HistoryCleanup = null; + return; + } + + // We need to reset the dirty properties, because it is otherwise true, just because the json serializer has set properties + target.HistoryCleanup.ResetDirtyProperties(false); + if (target.HistoryCleanup.PreventCleanup != source.HistoryCleanup.PreventCleanup) + { + target.HistoryCleanup.PreventCleanup = source.HistoryCleanup.PreventCleanup; + } + + if (target.HistoryCleanup.KeepAllVersionsNewerThanDays != source.HistoryCleanup.KeepAllVersionsNewerThanDays) + { + target.HistoryCleanup.KeepAllVersionsNewerThanDays = source.HistoryCleanup.KeepAllVersionsNewerThanDays; + } + + if (target.HistoryCleanup.KeepLatestVersionPerDayForDays != + source.HistoryCleanup.KeepLatestVersionPerDayForDays) + { + target.HistoryCleanup.KeepLatestVersionPerDayForDays = source.HistoryCleanup.KeepLatestVersionPerDayForDays; + } + } + // no MapAll - take care private void Map(MediaTypeSave source, IMediaType target, MapperContext context) { @@ -328,7 +356,10 @@ namespace Umbraco.Cms.Core.Models.Mapping if (source.GroupId > 0) { - target.PropertyGroupId = new Lazy(() => source.GroupId, false); + if (target.PropertyGroupId?.Value != source.GroupId) + { + target.PropertyGroupId = new Lazy(() => source.GroupId, false); + } } target.Alias = source.Alias; @@ -523,7 +554,15 @@ namespace Umbraco.Cms.Core.Models.Mapping target.Thumbnail = source.Thumbnail; target.AllowedAsRoot = source.AllowAsRoot; - target.AllowedContentTypes = source.AllowedContentTypes.Select((t, i) => new ContentTypeSort(t, i)); + + bool allowedContentTypesUnchanged = target.AllowedContentTypes.Select(x => x.Id.Value) + .SequenceEqual(source.AllowedContentTypes); + + if (allowedContentTypesUnchanged is false) + { + target.AllowedContentTypes = source.AllowedContentTypes.Select((t, i) => new ContentTypeSort(t, i)); + } + if (!(target is IMemberType)) { @@ -574,13 +613,21 @@ namespace Umbraco.Cms.Core.Models.Mapping // ensure no duplicate alias, then assign the group properties collection EnsureUniqueAliases(destProperties); - destGroup.PropertyTypes = new PropertyTypeCollection(isPublishing, destProperties); + if (destGroup.PropertyTypes.SupportsPublishing != isPublishing || destGroup.PropertyTypes.SequenceEqual(destProperties) is false) + { + destGroup.PropertyTypes = new PropertyTypeCollection(isPublishing, destProperties); + } + destGroups.Add(destGroup); } // ensure no duplicate name, then assign the groups collection EnsureUniqueAliases(destGroups); - target.PropertyGroups = new PropertyGroupCollection(destGroups); + + if (target.PropertyGroups.SequenceEqual(destGroups) is false) + { + target.PropertyGroups = new PropertyGroupCollection(destGroups); + } // because the property groups collection was rebuilt, there is no need to remove // the old groups - they are just gone and will be cleared by the repository From 44e880807971a9b5f7f3df85a0ee5a47a0cb973a Mon Sep 17 00:00:00 2001 From: Matt Brailsford Date: Thu, 3 Mar 2022 10:42:14 +0000 Subject: [PATCH 21/22] Check form and querystring when validating `ufprt` in `ValidateUmbracoFormRouteStringAttribute` (#11957) * Check form and querystring when validating ufprt Checks to see if the request has form data before validating the `ufprt` parameter, and if it doesn't assumes it must be on the querystring * Create GetUfprt extension method * Use GetUfprt extension * Update UmbracoRouteValueTransformer to use GetUfrpt() * Added missing using statement * Check for StringValues.Empty --- .../Extensions/HttpRequestExtensions.cs | 21 +++++++++++++++++++ ...ValidateUmbracoFormRouteStringAttribute.cs | 2 +- .../Routing/UmbracoRouteValueTransformer.cs | 18 ++++++++-------- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/src/Umbraco.Web.Common/Extensions/HttpRequestExtensions.cs b/src/Umbraco.Web.Common/Extensions/HttpRequestExtensions.cs index 2aeb2555eb..7d9ce136ef 100644 --- a/src/Umbraco.Web.Common/Extensions/HttpRequestExtensions.cs +++ b/src/Umbraco.Web.Common/Extensions/HttpRequestExtensions.cs @@ -6,6 +6,7 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Extensions; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Primitives; using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Routing; @@ -136,5 +137,25 @@ namespace Umbraco.Extensions return new Uri(routingSettings.UmbracoApplicationUrl); } + + /// + /// Gets the Umbraco `ufprt` encrypted string from the current request + /// + /// The current request + /// The extracted `ufprt` token. + public static string GetUfprt(this HttpRequest request) + { + if (request.HasFormContentType && request.Form.TryGetValue("ufprt", out StringValues formVal) && formVal != StringValues.Empty) + { + return formVal.ToString(); + } + + if (request.Query.TryGetValue("ufprt", out StringValues queryVal) && queryVal != StringValues.Empty) + { + return queryVal.ToString(); + } + + return null; + } } } diff --git a/src/Umbraco.Web.Common/Filters/ValidateUmbracoFormRouteStringAttribute.cs b/src/Umbraco.Web.Common/Filters/ValidateUmbracoFormRouteStringAttribute.cs index ed86d7c783..0c51edd4e1 100644 --- a/src/Umbraco.Web.Common/Filters/ValidateUmbracoFormRouteStringAttribute.cs +++ b/src/Umbraco.Web.Common/Filters/ValidateUmbracoFormRouteStringAttribute.cs @@ -42,7 +42,7 @@ namespace Umbraco.Cms.Web.Common.Filters { if (context == null) throw new ArgumentNullException(nameof(context)); - var ufprt = context.HttpContext.Request.Form["ufprt"]; + var ufprt = context.HttpContext.Request.GetUfprt(); if (context.ActionDescriptor is ControllerActionDescriptor controllerActionDescriptor) { diff --git a/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs b/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs index 9106c3ed09..60384de752 100644 --- a/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs +++ b/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs @@ -201,23 +201,23 @@ namespace Umbraco.Cms.Web.Website.Routing throw new ArgumentNullException(nameof(httpContext)); } - // if it is a POST/GET then a value must be in the request - if ((!httpContext.Request.HasFormContentType || !httpContext.Request.Form.TryGetValue("ufprt", out StringValues encodedVal)) - && !httpContext.Request.Query.TryGetValue("ufprt", out encodedVal)) + // if it is a POST/GET then a `ufprt` value must be in the request + var ufprt = httpContext.Request.GetUfprt(); + if (string.IsNullOrWhiteSpace(ufprt)) { return null; } if (!EncryptionHelper.DecryptAndValidateEncryptedRouteString( _dataProtectionProvider, - encodedVal, - out IDictionary decodedParts)) + ufprt, + out IDictionary decodedUfprt)) { return null; } // Get all route values that are not the default ones and add them separately so they eventually get to action parameters - foreach (KeyValuePair item in decodedParts.Where(x => ReservedAdditionalKeys.AllKeys.Contains(x.Key) == false)) + foreach (KeyValuePair item in decodedUfprt.Where(x => ReservedAdditionalKeys.AllKeys.Contains(x.Key) == false)) { values[item.Key] = item.Value; } @@ -225,9 +225,9 @@ namespace Umbraco.Cms.Web.Website.Routing // return the proxy info without the surface id... could be a local controller. return new PostedDataProxyInfo { - ControllerName = WebUtility.UrlDecode(decodedParts.First(x => x.Key == ReservedAdditionalKeys.Controller).Value), - ActionName = WebUtility.UrlDecode(decodedParts.First(x => x.Key == ReservedAdditionalKeys.Action).Value), - Area = WebUtility.UrlDecode(decodedParts.First(x => x.Key == ReservedAdditionalKeys.Area).Value), + ControllerName = WebUtility.UrlDecode(decodedUfprt.First(x => x.Key == ReservedAdditionalKeys.Controller).Value), + ActionName = WebUtility.UrlDecode(decodedUfprt.First(x => x.Key == ReservedAdditionalKeys.Action).Value), + Area = WebUtility.UrlDecode(decodedUfprt.First(x => x.Key == ReservedAdditionalKeys.Area).Value), }; } From ca5c85e656be20c95aa1fbbb85cc5d89c87970e2 Mon Sep 17 00:00:00 2001 From: Paul Johnson Date: Thu, 3 Mar 2022 17:18:39 +0000 Subject: [PATCH 22/22] v9 fix misc issues external member login (#12093) * Add missing override for SetTokenAsync * Fix mismatch between expected scheme prefix and exception message * Store tokens on member update --- .../Security/MemberUserStore.cs | 42 +++++++++++++++++++ .../Security/MemberAuthenticationBuilder.cs | 2 +- 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs index 420d66b0b4..345a404fcf 100644 --- a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs @@ -181,6 +181,7 @@ namespace Umbraco.Cms.Core.Security { // we have to remember whether Logins property is dirty, since the UpdateMemberProperties will reset it. var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(MemberIdentityUser.Logins)); + var isTokensPropertyDirty = user.IsPropertyDirty(nameof(MemberIdentityUser.LoginTokens)); MemberDataChangeType memberChangeType = UpdateMemberProperties(found, user); if (memberChangeType == MemberDataChangeType.FullSave) @@ -203,6 +204,16 @@ namespace Umbraco.Cms.Core.Security x.ProviderKey, x.UserData))); } + + if (isTokensPropertyDirty) + { + _externalLoginService.Save( + found.Key, + user.LoginTokens.Select(x => new ExternalLoginToken( + x.LoginProvider, + x.Name, + x.Value))); + } } return Task.FromResult(IdentityResult.Success); @@ -535,6 +546,37 @@ namespace Umbraco.Cms.Core.Security return found; } + /// + /// Overridden to support Umbraco's own data storage requirements + /// + /// + /// The base class's implementation of this calls into FindTokenAsync and AddUserTokenAsync, both methods will only work with ORMs that are change + /// tracking ORMs like EFCore. + /// + /// + public override Task SetTokenAsync(MemberIdentityUser user, string loginProvider, string name, string value, CancellationToken cancellationToken) + { + cancellationToken.ThrowIfCancellationRequested(); + ThrowIfDisposed(); + + if (user == null) + { + throw new ArgumentNullException(nameof(user)); + } + + IIdentityUserToken token = user.LoginTokens.FirstOrDefault(x => x.LoginProvider.InvariantEquals(loginProvider) && x.Name.InvariantEquals(name)); + if (token == null) + { + user.LoginTokens.Add(new IdentityUserToken(loginProvider, name, value, user.Id)); + } + else + { + token.Value = value; + } + + return Task.CompletedTask; + } + private MemberIdentityUser AssignLoginsCallback(MemberIdentityUser user) { if (user != null) diff --git a/src/Umbraco.Web.Website/Security/MemberAuthenticationBuilder.cs b/src/Umbraco.Web.Website/Security/MemberAuthenticationBuilder.cs index d58abfc871..8308abafff 100644 --- a/src/Umbraco.Web.Website/Security/MemberAuthenticationBuilder.cs +++ b/src/Umbraco.Web.Website/Security/MemberAuthenticationBuilder.cs @@ -41,7 +41,7 @@ namespace Umbraco.Cms.Web.Website.Security // Validate that the prefix is set if (!authenticationScheme.StartsWith(Constants.Security.MemberExternalAuthenticationTypePrefix)) { - throw new InvalidOperationException($"The {nameof(authenticationScheme)} is not prefixed with {Constants.Security.BackOfficeExternalAuthenticationTypePrefix}. The scheme must be created with a call to the method {nameof(SchemeForMembers)}"); + throw new InvalidOperationException($"The {nameof(authenticationScheme)} is not prefixed with {Constants.Security.MemberExternalAuthenticationTypePrefix}. The scheme must be created with a call to the method {nameof(SchemeForMembers)}"); } // add our login provider to the container along with a custom options configuration