From e4807c2430de0d47d271c57ec4bd924ff0237b17 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 11 Mar 2021 19:35:43 +1100 Subject: [PATCH 1/8] New table and service to store auth tokens Implements the auth token retrieval and storage for the identity implementation. This is now automatically done for providers and the back office user manager can be used to retreive and store all tokens. Fixes locking on the config writer. Removes the abstract NodeObjectTypeId from the base repo since it shouldn't be there. --- .../Models/Identity/ExternalLogin.cs | 1 + .../Models/Identity/ExternalLoginToken.cs | 27 +++ .../Models/Identity/IExternalLoginToken.cs | 23 +++ .../Models/Identity/IIdentityUserLogin.cs | 2 +- .../Models/Identity/IIdentityUserToken.cs | 30 ++++ .../Models/Identity/IdentityUserToken.cs | 44 +++++ .../Persistence/Constants-DatabaseSchema.cs | 3 +- .../Repositories/IExternalLoginRepository.cs | 15 +- .../Services/IExternalLoginService.cs | 25 ++- .../Configuration/JsonConfigManipulator.cs | 64 +++---- .../Install/DatabaseSchemaCreator.cs | 3 +- .../Migrations/Upgrade/UmbracoPlan.cs | 12 +- .../V_9_0_0/ExternalLoginTableIndexes.cs | 50 ++++++ .../V_9_0_0/ExternalLoginTokenTable.cs | 28 +++ .../Persistence/Dtos/ExternalLoginDto.cs | 19 +- .../Persistence/Dtos/ExternalLoginTokenDto.cs | 42 +++++ .../Factories/ExternalLoginFactory.cs | 23 +++ .../Mappers/ExternalLoginMapper.cs | 2 +- .../Mappers/ExternalLoginTokenMapper.cs | 25 +++ .../Mappers/MapperCollectionBuilder.cs | 3 +- .../Persistence/NPocoSqlExtensions.cs | 3 +- .../Implement/AuditEntryRepository.cs | 3 - .../Repositories/Implement/AuditRepository.cs | 5 - .../Implement/ConsentRepository.cs | 3 - .../Implement/ContentRepositoryBase.cs | 8 +- .../Implement/ContentTypeRepositoryBase.cs | 7 +- .../Implement/DataTypeRepository.cs | 4 +- .../Implement/DictionaryRepository.cs | 7 +- .../Implement/DocumentRepository.cs | 5 +- .../Implement/DomainRepository.cs | 7 +- .../Implement/EntityContainerRepository.cs | 17 +- .../Implement/EntityRepositoryBase.cs | 5 - .../Implement/ExternalLoginRepository.cs | 162 ++++++++++++++---- .../Implement/KeyValueRepository.cs | 4 +- .../Implement/LanguageRepository.cs | 4 +- .../Repositories/Implement/MacroRepository.cs | 4 +- .../Repositories/Implement/MediaRepository.cs | 15 +- .../Implement/MemberGroupRepository.cs | 4 +- .../Implement/PermissionRepository.cs | 4 +- .../Implement/PublicAccessRepository.cs | 4 +- .../Implement/RedirectUrlRepository.cs | 7 +- .../Implement/RelationRepository.cs | 7 +- .../Implement/RelationTypeRepository.cs | 7 +- .../Implement/ServerRegistrationRepository.cs | 4 +- .../Implement/SimpleGetRepository.cs | 4 +- .../Repositories/Implement/TagRepository.cs | 5 +- .../Implement/TemplateRepository.cs | 4 +- .../Implement/UserGroupRepository.cs | 6 +- .../Repositories/Implement/UserRepository.cs | 4 +- .../Security/BackOfficeUserStore.cs | 114 +++++++++++- .../Security/UmbracoIdentityUser.cs | 43 ++++- .../Implement/ExternalLoginService.cs | 27 ++- .../Services/ExternalLoginServiceTests.cs | 91 ++++++++-- .../Controllers/AuthenticationController.cs | 5 +- .../Controllers/BackOfficeController.cs | 26 +-- .../Security/IBackOfficeSignInManager.cs | 1 + src/Umbraco.Web.UI.NetCore/Program.cs | 19 +- src/Umbraco.Web.UI.NetCore/appsettings.json | 4 +- 58 files changed, 853 insertions(+), 241 deletions(-) create mode 100644 src/Umbraco.Core/Models/Identity/ExternalLoginToken.cs create mode 100644 src/Umbraco.Core/Models/Identity/IExternalLoginToken.cs create mode 100644 src/Umbraco.Core/Models/Identity/IIdentityUserToken.cs create mode 100644 src/Umbraco.Core/Models/Identity/IdentityUserToken.cs create mode 100644 src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_0_0/ExternalLoginTableIndexes.cs create mode 100644 src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_0_0/ExternalLoginTokenTable.cs create mode 100644 src/Umbraco.Infrastructure/Persistence/Dtos/ExternalLoginTokenDto.cs create mode 100644 src/Umbraco.Infrastructure/Persistence/Mappers/ExternalLoginTokenMapper.cs diff --git a/src/Umbraco.Core/Models/Identity/ExternalLogin.cs b/src/Umbraco.Core/Models/Identity/ExternalLogin.cs index 485ec66df4..c599dcab96 100644 --- a/src/Umbraco.Core/Models/Identity/ExternalLogin.cs +++ b/src/Umbraco.Core/Models/Identity/ExternalLogin.cs @@ -2,6 +2,7 @@ using System; namespace Umbraco.Cms.Core.Models.Identity { + /// public class ExternalLogin : IExternalLogin { diff --git a/src/Umbraco.Core/Models/Identity/ExternalLoginToken.cs b/src/Umbraco.Core/Models/Identity/ExternalLoginToken.cs new file mode 100644 index 0000000000..23f532fdfa --- /dev/null +++ b/src/Umbraco.Core/Models/Identity/ExternalLoginToken.cs @@ -0,0 +1,27 @@ +using System; + +namespace Umbraco.Cms.Core.Models.Identity +{ + /// + public class ExternalLoginToken : IExternalLoginToken + { + /// + /// Initializes a new instance of the class. + /// + public ExternalLoginToken(string loginProvider, string name, string value) + { + LoginProvider = loginProvider ?? throw new ArgumentNullException(nameof(loginProvider)); + Name = name ?? throw new ArgumentNullException(nameof(name)); + Value = value ?? throw new ArgumentNullException(nameof(value)); + } + + /// + public string LoginProvider { get; } + + /// + public string Name { get; } + + /// + public string Value { get; } + } +} diff --git a/src/Umbraco.Core/Models/Identity/IExternalLoginToken.cs b/src/Umbraco.Core/Models/Identity/IExternalLoginToken.cs new file mode 100644 index 0000000000..7cc3065e35 --- /dev/null +++ b/src/Umbraco.Core/Models/Identity/IExternalLoginToken.cs @@ -0,0 +1,23 @@ +namespace Umbraco.Cms.Core.Models.Identity +{ + /// + /// Used to persist an external login token for a user + /// + public interface IExternalLoginToken + { + /// + /// Gets the login provider + /// + string LoginProvider { get; } + + /// + /// Gets the name of the token + /// + string Name { get; } + + /// + /// Gets the value of the token + /// + string Value { get; } + } +} diff --git a/src/Umbraco.Core/Models/Identity/IIdentityUserLogin.cs b/src/Umbraco.Core/Models/Identity/IIdentityUserLogin.cs index 89ec823875..21d8f842af 100644 --- a/src/Umbraco.Core/Models/Identity/IIdentityUserLogin.cs +++ b/src/Umbraco.Core/Models/Identity/IIdentityUserLogin.cs @@ -24,7 +24,7 @@ namespace Umbraco.Cms.Core.Models.Identity string UserId { get; set; } // TODO: This should be able to be used by both users and members /// - /// Gets or sets any arbitrary data for the user and external provider - like user tokens returned from the provider + /// Gets or sets any arbitrary data for the user and external provider /// string UserData { get; set; } } diff --git a/src/Umbraco.Core/Models/Identity/IIdentityUserToken.cs b/src/Umbraco.Core/Models/Identity/IIdentityUserToken.cs new file mode 100644 index 0000000000..df0c97ff08 --- /dev/null +++ b/src/Umbraco.Core/Models/Identity/IIdentityUserToken.cs @@ -0,0 +1,30 @@ +using Umbraco.Cms.Core.Models.Entities; + +namespace Umbraco.Cms.Core.Models.Identity +{ + /// + /// An external login provider token + /// + public interface IIdentityUserToken : IEntity, IRememberBeingDirty + { + /// + /// Gets or sets user Id for the user who owns this token + /// + string UserId { get; set; } + + /// + /// Gets or sets the login provider for the login (i.e. Facebook, Google) + /// + string LoginProvider { get; set; } + + /// + /// Gets or sets the token name + /// + string Name { get; set; } + + /// + /// Gets or set the token value + /// + string Value { get; set; } + } +} diff --git a/src/Umbraco.Core/Models/Identity/IdentityUserToken.cs b/src/Umbraco.Core/Models/Identity/IdentityUserToken.cs new file mode 100644 index 0000000000..42e79a89dd --- /dev/null +++ b/src/Umbraco.Core/Models/Identity/IdentityUserToken.cs @@ -0,0 +1,44 @@ +using System; +using Umbraco.Cms.Core.Models.Entities; + +namespace Umbraco.Cms.Core.Models.Identity +{ + public class IdentityUserToken : EntityBase, IIdentityUserToken + { + /// + /// Initializes a new instance of the class. + /// + public IdentityUserToken(string loginProvider, string name, string value, string userId) + { + LoginProvider = loginProvider ?? throw new ArgumentNullException(nameof(loginProvider)); + Name = name ?? throw new ArgumentNullException(nameof(name)); + Value = value ?? throw new ArgumentNullException(nameof(value)); + UserId = userId; + } + + /// + /// Initializes a new instance of the class. + /// + public IdentityUserToken(int id, string loginProvider, string name, string value, string userId, DateTime createDate) + { + Id = id; + LoginProvider = loginProvider ?? throw new ArgumentNullException(nameof(loginProvider)); + Name = name ?? throw new ArgumentNullException(nameof(name)); + Value = value ?? throw new ArgumentNullException(nameof(value)); + UserId = userId; + CreateDate = createDate; + } + + /// + public string LoginProvider { get; set; } + + /// + public string Name { get; set; } + + /// + public string Value { get; set; } + + /// + public string UserId { get; set; } + } +} diff --git a/src/Umbraco.Core/Persistence/Constants-DatabaseSchema.cs b/src/Umbraco.Core/Persistence/Constants-DatabaseSchema.cs index 73004c491d..a98c72b393 100644 --- a/src/Umbraco.Core/Persistence/Constants-DatabaseSchema.cs +++ b/src/Umbraco.Core/Persistence/Constants-DatabaseSchema.cs @@ -1,4 +1,4 @@ -// ReSharper disable once CheckNamespace +// ReSharper disable once CheckNamespace namespace Umbraco.Cms.Core { public static partial class Constants @@ -52,6 +52,7 @@ namespace Umbraco.Cms.Core public const string UserGroup2App = TableNamePrefix + "UserGroup2App"; public const string UserGroup2NodePermission = TableNamePrefix + "UserGroup2NodePermission"; public const string ExternalLogin = TableNamePrefix + "ExternalLogin"; + public const string ExternalLoginToken = TableNamePrefix + "ExternalLoginToken"; public const string Macro = /*TableNamePrefix*/ "cms" + "Macro"; public const string MacroProperty = /*TableNamePrefix*/ "cms" + "MacroProperty"; diff --git a/src/Umbraco.Core/Persistence/Repositories/IExternalLoginRepository.cs b/src/Umbraco.Core/Persistence/Repositories/IExternalLoginRepository.cs index 8821c332c3..9f755c2256 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IExternalLoginRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IExternalLoginRepository.cs @@ -3,9 +3,22 @@ using Umbraco.Cms.Core.Models.Identity; namespace Umbraco.Cms.Core.Persistence.Repositories { - public interface IExternalLoginRepository : IReadWriteQueryRepository + public interface IExternalLoginRepository : IReadWriteQueryRepository, IQueryRepository { + /// + /// Replaces all external login providers for the user + /// + /// + /// void Save(int userId, IEnumerable logins); + + /// + /// Replaces all external login provider tokens for the providers specified for the user + /// + /// + /// + void Save(int userId, IEnumerable tokens); + void DeleteUserLogins(int memberId); } } diff --git a/src/Umbraco.Core/Services/IExternalLoginService.cs b/src/Umbraco.Core/Services/IExternalLoginService.cs index e19d30e293..b0c53c8f86 100644 --- a/src/Umbraco.Core/Services/IExternalLoginService.cs +++ b/src/Umbraco.Core/Services/IExternalLoginService.cs @@ -4,7 +4,7 @@ using Umbraco.Cms.Core.Models.Identity; namespace Umbraco.Cms.Core.Services { /// - /// Used to store the external login info, this can be replaced with your own implementation + /// Used to store the external login info /// public interface IExternalLoginService : IService { @@ -13,7 +13,14 @@ namespace Umbraco.Cms.Core.Services /// /// /// - IEnumerable GetAll(int userId); + IEnumerable GetExternalLogins(int userId); + + /// + /// Returns all user login tokens assigned + /// + /// + /// + IEnumerable GetExternalLoginTokens(int userId); /// /// Returns all logins matching the login info - generally there should only be one but in some cases @@ -36,11 +43,23 @@ namespace Umbraco.Cms.Core.Services /// void Save(int userId, IEnumerable logins); + /// + /// Saves the external login tokens associated with the user + /// + /// + /// The user associated with the tokens + /// + /// + /// + /// This will replace all external login tokens for the user + /// + void Save(int userId, IEnumerable tokens); + /// /// Save a single external login record /// /// - void Save(IIdentityUserLogin login); + void Save(IIdentityUserLogin login); // TODO: This is unused apart from tests, perhaps remove it? /// /// Deletes all user logins - normally used when a member is deleted diff --git a/src/Umbraco.Infrastructure/Configuration/JsonConfigManipulator.cs b/src/Umbraco.Infrastructure/Configuration/JsonConfigManipulator.cs index 6057ce8a82..78bb73f301 100644 --- a/src/Umbraco.Infrastructure/Configuration/JsonConfigManipulator.cs +++ b/src/Umbraco.Infrastructure/Configuration/JsonConfigManipulator.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.IO; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Configuration.Json; @@ -12,11 +12,9 @@ namespace Umbraco.Cms.Core.Configuration public class JsonConfigManipulator : IConfigManipulator { private readonly IConfiguration _configuration; + private readonly object _locker = new object(); - public JsonConfigManipulator(IConfiguration configuration) - { - _configuration = configuration; - } + public JsonConfigManipulator(IConfiguration configuration) => _configuration = configuration; public string UmbracoConnectionPath { get; } = $"ConnectionStrings:{ Cms.Core.Constants.System.UmbracoConnectionName}"; public void RemoveConnectionString() @@ -53,11 +51,13 @@ namespace Umbraco.Cms.Core.Configuration JToken token = json; foreach (var propertyName in key.Split(new[] { ':' })) { - if (token is null) break; + if (token is null) + break; token = CaseSelectPropertyValues(token, propertyName); } - if (token is null) return; + if (token is null) + return; var writer = new JTokenWriter(); writer.WriteValue(value); @@ -162,44 +162,46 @@ namespace Umbraco.Cms.Core.Configuration token?.Parent?.Remove(); } - private static void SaveJson(JsonConfigurationProvider provider, JObject json) + private void SaveJson(JsonConfigurationProvider provider, JObject json) { - if (provider.Source.FileProvider is PhysicalFileProvider physicalFileProvider) + lock (_locker) { - var jsonFilePath = Path.Combine(physicalFileProvider.Root, provider.Source.Path); + if (provider.Source.FileProvider is PhysicalFileProvider physicalFileProvider) + { + var jsonFilePath = Path.Combine(physicalFileProvider.Root, provider.Source.Path); - using (var sw = new StreamWriter(jsonFilePath, false)) - using (var jsonTextWriter = new JsonTextWriter(sw) - { - Formatting = Formatting.Indented, - }) - { - json?.WriteTo(jsonTextWriter); + using (var sw = new StreamWriter(jsonFilePath, false)) + using (var jsonTextWriter = new JsonTextWriter(sw) + { + Formatting = Formatting.Indented, + }) + { + json?.WriteTo(jsonTextWriter); + } } } - } - private static JObject GetJson(JsonConfigurationProvider provider) + private JObject GetJson(JsonConfigurationProvider provider) { - if (provider.Source.FileProvider is PhysicalFileProvider physicalFileProvider) + lock (_locker) { - var jsonFilePath = Path.Combine(physicalFileProvider.Root, provider.Source.Path); - - var serializer = new JsonSerializer(); - using (var sr = new StreamReader(jsonFilePath)) - using (var jsonTextReader = new JsonTextReader(sr)) + if (provider.Source.FileProvider is PhysicalFileProvider physicalFileProvider) { - return serializer.Deserialize(jsonTextReader); + var jsonFilePath = Path.Combine(physicalFileProvider.Root, provider.Source.Path); + + var serializer = new JsonSerializer(); + using (var sr = new StreamReader(jsonFilePath)) + using (var jsonTextReader = new JsonTextReader(sr)) + { + return serializer.Deserialize(jsonTextReader); + } } + + return null; } - - return null; } - - - private JsonConfigurationProvider GetJsonConfigurationProvider(string requiredKey = null) { if (_configuration is IConfigurationRoot configurationRoot) diff --git a/src/Umbraco.Infrastructure/Migrations/Install/DatabaseSchemaCreator.cs b/src/Umbraco.Infrastructure/Migrations/Install/DatabaseSchemaCreator.cs index d7db160b56..10564f7c98 100644 --- a/src/Umbraco.Infrastructure/Migrations/Install/DatabaseSchemaCreator.cs +++ b/src/Umbraco.Infrastructure/Migrations/Install/DatabaseSchemaCreator.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.Extensions.Logging; @@ -76,6 +76,7 @@ namespace Umbraco.Cms.Infrastructure.Migrations.Install typeof (AccessRuleDto), typeof (CacheInstructionDto), typeof (ExternalLoginDto), + typeof (ExternalLoginTokenDto), typeof (RedirectUrlDto), typeof (LockDto), typeof (UserGroupDto), diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs index 9aacab1740..da3325ef64 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs @@ -1,4 +1,4 @@ -using System; +using System; using Umbraco.Cms.Core.Configuration; using Umbraco.Cms.Core.Semver; using Umbraco.Cms.Infrastructure.Migrations.Upgrade.Common; @@ -9,6 +9,7 @@ using Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_8_10_0; using Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_8_6_0; using Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_8_7_0; using Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_8_9_0; +using Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_9_0_0; using Umbraco.Extensions; namespace Umbraco.Cms.Infrastructure.Migrations.Upgrade @@ -195,8 +196,15 @@ namespace Umbraco.Cms.Infrastructure.Migrations.Upgrade // to 8.9.0 To("{B5838FF5-1D22-4F6C-BCEB-F83ACB14B575}"); -// to 8.10.0 + + // to 8.10.0 To("{D6A8D863-38EC-44FB-91EC-ACD6A668BD18}"); + + // to 9.0.0 + + To("{50A43237-A6F4-49E2-A7A6-5DAD65C84669}"); + To("{3D8DADEF-0FDA-4377-A5F0-B52C2110E8F2}"); + //FINAL } } diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_0_0/ExternalLoginTableIndexes.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_0_0/ExternalLoginTableIndexes.cs new file mode 100644 index 0000000000..43d059b791 --- /dev/null +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_0_0/ExternalLoginTableIndexes.cs @@ -0,0 +1,50 @@ +using Umbraco.Cms.Infrastructure.Persistence.Dtos; + +namespace Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_9_0_0 +{ + public class ExternalLoginTableIndexes : MigrationBase + { + public ExternalLoginTableIndexes(IMigrationContext context) + : base(context) + { + } + + /// + /// Adds new indexes to the External Login table + /// + public override void Migrate() + { + // TODO: Before adding these indexes we need to remove duplicate data + + var indexName1 = "IX_" + ExternalLoginDto.TableName + "_LoginProvider"; + + if (!IndexExists(indexName1)) + { + Create + .Index(indexName1) + .OnTable(ExternalLoginDto.TableName) + .OnColumn("loginProvider") + .Ascending() + .WithOptions() + .Unique() + .WithOptions() + .NonClustered() + .Do(); + } + + var indexName2 = "IX_" + ExternalLoginDto.TableName + "_ProviderKey"; + + if (!IndexExists(indexName2)) + { + Create + .Index(indexName2) + .OnTable(ExternalLoginDto.TableName) + .OnColumn("loginProvider").Ascending() + .OnColumn("providerKey").Ascending() + .WithOptions() + .NonClustered() + .Do(); + } + } + } +} diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_0_0/ExternalLoginTokenTable.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_0_0/ExternalLoginTokenTable.cs new file mode 100644 index 0000000000..e098bbd0b9 --- /dev/null +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_0_0/ExternalLoginTokenTable.cs @@ -0,0 +1,28 @@ +using System.Collections.Generic; +using Umbraco.Cms.Infrastructure.Persistence.Dtos; +using Umbraco.Extensions; + +namespace Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_9_0_0 +{ + public class ExternalLoginTokenTable : MigrationBase + { + public ExternalLoginTokenTable(IMigrationContext context) + : base(context) + { + } + + /// + /// Adds new External Login token table + /// + public override void Migrate() + { + IEnumerable tables = SqlSyntax.GetTablesInSchema(Context.Database); + if (tables.InvariantContains(ExternalLoginTokenDto.TableName)) + { + return; + } + + Create.Table().Do(); + } + } +} diff --git a/src/Umbraco.Infrastructure/Persistence/Dtos/ExternalLoginDto.cs b/src/Umbraco.Infrastructure/Persistence/Dtos/ExternalLoginDto.cs index d7aa4ac173..3fc65b28a5 100644 --- a/src/Umbraco.Infrastructure/Persistence/Dtos/ExternalLoginDto.cs +++ b/src/Umbraco.Infrastructure/Persistence/Dtos/ExternalLoginDto.cs @@ -1,34 +1,39 @@ -using System; +using System; using NPoco; using Umbraco.Cms.Infrastructure.Persistence.DatabaseAnnotations; using Umbraco.Cms.Infrastructure.Persistence.DatabaseModelDefinitions; namespace Umbraco.Cms.Infrastructure.Persistence.Dtos { - [TableName(Cms.Core.Constants.DatabaseSchema.Tables.ExternalLogin)] + [TableName(TableName)] [ExplicitColumns] [PrimaryKey("Id")] internal class ExternalLoginDto { + public const string TableName = Cms.Core.Constants.DatabaseSchema.Tables.ExternalLogin; + [Column("id")] - [PrimaryKeyColumn(Name = "PK_umbracoExternalLogin")] + [PrimaryKeyColumn] public int Id { get; set; } - // TODO: This is completely missing a FK!!? + // TODO: This is completely missing a FK!!? ... IIRC that is because we want to change this to a GUID + // to support both members and users for external logins and that will not have any referential integrity + // This should be part of the members task for enabling external logins. [Column("userId")] + [Index(IndexTypes.NonClustered)] public int UserId { get; set; } - // TODO: There should be an index on both LoginProvider and ProviderKey - [Column("loginProvider")] - [Length(4000)] + [Length(4000)] // TODO: This value seems WAY too high, this is just a name [NullSetting(NullSetting = NullSettings.NotNull)] + [Index(IndexTypes.UniqueNonClustered, Name = "IX_" + TableName + "_LoginProvider")] public string LoginProvider { get; set; } [Column("providerKey")] [Length(4000)] [NullSetting(NullSetting = NullSettings.NotNull)] + [Index(IndexTypes.NonClustered, ForColumns = "loginProvider,providerKey", Name = "IX_" + TableName + "_ProviderKey")] public string ProviderKey { get; set; } [Column("createDate")] diff --git a/src/Umbraco.Infrastructure/Persistence/Dtos/ExternalLoginTokenDto.cs b/src/Umbraco.Infrastructure/Persistence/Dtos/ExternalLoginTokenDto.cs new file mode 100644 index 0000000000..c5f3b7c8ac --- /dev/null +++ b/src/Umbraco.Infrastructure/Persistence/Dtos/ExternalLoginTokenDto.cs @@ -0,0 +1,42 @@ +using System; +using NPoco; +using Umbraco.Cms.Infrastructure.Persistence.DatabaseAnnotations; +using Umbraco.Cms.Infrastructure.Persistence.DatabaseModelDefinitions; + +namespace Umbraco.Cms.Infrastructure.Persistence.Dtos +{ + [TableName(TableName)] + [ExplicitColumns] + [PrimaryKey("Id")] + internal class ExternalLoginTokenDto + { + public const string TableName = Cms.Core.Constants.DatabaseSchema.Tables.ExternalLoginToken; + + [Column("id")] + [PrimaryKeyColumn] + public int Id { get; set; } + + [Column("externalLoginId")] + [ForeignKey(typeof(ExternalLoginDto), Column = "id")] + public int ExternalLoginId { get; set; } + + [Column("name")] + [Length(255)] + [NullSetting(NullSetting = NullSettings.NotNull)] + [Index(IndexTypes.UniqueNonClustered, ForColumns = "externalLoginId,name", Name = "IX_" + TableName + "_Name")] + public string Name { get; set; } + + [Column("value")] + [SpecialDbType(SpecialDbTypes.NVARCHARMAX)] + [NullSetting(NullSetting = NullSettings.NotNull)] + public string Value { get; set; } + + [Column("createDate")] + [Constraint(Default = SystemMethods.CurrentDateTime)] + public DateTime CreateDate { get; set; } + + [ResultColumn] + [Reference(ReferenceType.OneToOne, ColumnName = "ExternalLoginId")] + public ExternalLoginDto ExternalLoginDto { get; set; } + } +} diff --git a/src/Umbraco.Infrastructure/Persistence/Factories/ExternalLoginFactory.cs b/src/Umbraco.Infrastructure/Persistence/Factories/ExternalLoginFactory.cs index f356274d04..772545e2a3 100644 --- a/src/Umbraco.Infrastructure/Persistence/Factories/ExternalLoginFactory.cs +++ b/src/Umbraco.Infrastructure/Persistence/Factories/ExternalLoginFactory.cs @@ -6,6 +6,15 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Factories { internal static class ExternalLoginFactory { + public static IIdentityUserToken BuildEntity(ExternalLoginTokenDto dto) + { + var entity = new IdentityUserToken(dto.Id, dto.ExternalLoginDto.LoginProvider, dto.Name, dto.Value, dto.ExternalLoginDto.UserId.ToString(), dto.CreateDate); + + // reset dirty initial properties (U4-1946) + entity.ResetDirtyProperties(false); + return entity; + } + public static IIdentityUserLogin BuildEntity(ExternalLoginDto dto) { var entity = new IdentityUserLogin(dto.Id, dto.LoginProvider, dto.ProviderKey, dto.UserId.ToString(), dto.CreateDate) @@ -47,5 +56,19 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Factories return dto; } + + public static ExternalLoginTokenDto BuildDto(int externalLoginId, IExternalLoginToken token, int? id = null) + { + var dto = new ExternalLoginTokenDto + { + Id = id ?? default, + ExternalLoginId = externalLoginId, + Name = token.Name, + Value = token.Value, + CreateDate = DateTime.Now + }; + + return dto; + } } } diff --git a/src/Umbraco.Infrastructure/Persistence/Mappers/ExternalLoginMapper.cs b/src/Umbraco.Infrastructure/Persistence/Mappers/ExternalLoginMapper.cs index 78f321fe4b..198554be0c 100644 --- a/src/Umbraco.Infrastructure/Persistence/Mappers/ExternalLoginMapper.cs +++ b/src/Umbraco.Infrastructure/Persistence/Mappers/ExternalLoginMapper.cs @@ -1,4 +1,4 @@ -using System; +using System; using Umbraco.Cms.Core.Models.Identity; using Umbraco.Cms.Infrastructure.Persistence.Dtos; diff --git a/src/Umbraco.Infrastructure/Persistence/Mappers/ExternalLoginTokenMapper.cs b/src/Umbraco.Infrastructure/Persistence/Mappers/ExternalLoginTokenMapper.cs new file mode 100644 index 0000000000..11cb60dc83 --- /dev/null +++ b/src/Umbraco.Infrastructure/Persistence/Mappers/ExternalLoginTokenMapper.cs @@ -0,0 +1,25 @@ +using System; +using Umbraco.Cms.Core.Models.Identity; +using Umbraco.Cms.Infrastructure.Persistence.Dtos; + +namespace Umbraco.Cms.Infrastructure.Persistence.Mappers +{ + [MapperFor(typeof(IIdentityUserToken))] + [MapperFor(typeof(IdentityUserToken))] + public sealed class ExternalLoginTokenMapper : BaseMapper + { + public ExternalLoginTokenMapper(Lazy sqlContext, MapperConfigurationStore maps) + : base(sqlContext, maps) + { } + + protected override void DefineMaps() + { + DefineMap(nameof(IdentityUserToken.Id), nameof(ExternalLoginTokenDto.Id)); + DefineMap(nameof(IdentityUserToken.CreateDate), nameof(ExternalLoginTokenDto.CreateDate)); + DefineMap(nameof(IdentityUserToken.Name), nameof(ExternalLoginTokenDto.Name)); + DefineMap(nameof(IdentityUserToken.Value), nameof(ExternalLoginTokenDto.Value)); + // separate table + DefineMap(nameof(IdentityUserToken.UserId), nameof(ExternalLoginDto.UserId)); + } + } +} diff --git a/src/Umbraco.Infrastructure/Persistence/Mappers/MapperCollectionBuilder.cs b/src/Umbraco.Infrastructure/Persistence/Mappers/MapperCollectionBuilder.cs index c5f0814469..d200b6fdc1 100644 --- a/src/Umbraco.Infrastructure/Persistence/Mappers/MapperCollectionBuilder.cs +++ b/src/Umbraco.Infrastructure/Persistence/Mappers/MapperCollectionBuilder.cs @@ -1,4 +1,4 @@ -using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection; using Umbraco.Cms.Core.Composing; namespace Umbraco.Cms.Infrastructure.Persistence.Mappers @@ -50,6 +50,7 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Mappers Add(); Add(); Add(); + Add(); Add(); Add(); Add(); diff --git a/src/Umbraco.Infrastructure/Persistence/NPocoSqlExtensions.cs b/src/Umbraco.Infrastructure/Persistence/NPocoSqlExtensions.cs index efe52b0271..05f15f7372 100644 --- a/src/Umbraco.Infrastructure/Persistence/NPocoSqlExtensions.cs +++ b/src/Umbraco.Infrastructure/Persistence/NPocoSqlExtensions.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections; using System.Collections.Generic; using System.Linq; @@ -1015,6 +1015,7 @@ namespace Umbraco.Extensions /// /// The Sql statement. /// The Sql statement. + /// NOTE: This method will not work for all queries, only simple ones! public static Sql ForUpdate(this Sql sql) { // go find the first FROM clause, and append the lock hint diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/AuditEntryRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/AuditEntryRepository.cs index 91408d0687..16c411c772 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/AuditEntryRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/AuditEntryRepository.cs @@ -27,9 +27,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement : base(scopeAccessor, cache, logger) { } - /// - protected override Guid NodeObjectTypeId => throw new NotSupportedException(); - /// protected override IAuditEntry PerformGet(int id) { diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/AuditRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/AuditRepository.cs index 1558266d55..d528f69dce 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/AuditRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/AuditRepository.cs @@ -117,11 +117,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement throw new NotImplementedException(); } - protected override Guid NodeObjectTypeId - { - get { throw new NotImplementedException(); } - } - public void CleanLogs(int maximumAgeOfLogsInMinutes) { var oldestPermittedLogEntry = DateTime.Now.Subtract(new TimeSpan(0, maximumAgeOfLogsInMinutes, 0)); diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ConsentRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ConsentRepository.cs index 0c1cc7376d..a64828ee54 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ConsentRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ConsentRepository.cs @@ -26,9 +26,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement : base(scopeAccessor, cache, logger) { } - /// - protected override Guid NodeObjectTypeId => throw new NotSupportedException(); - /// protected override IConsent PerformGet(int id) { diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentRepositoryBase.cs index da96e6dfc8..c6cf516283 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -27,10 +27,9 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement internal sealed class ContentRepositoryBase { /// - /// /// This is used for unit tests ONLY /// - public static bool ThrowOnWarning = false; + public static bool ThrowOnWarning { get; set; } = false; } public abstract class ContentRepositoryBase : EntityRepositoryBase, IContentRepository @@ -72,6 +71,11 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement protected abstract TRepository This { get; } + /// + /// Gets the node object type for the repository's entity + /// + protected abstract Guid NodeObjectTypeId { get; } + protected ILanguageRepository LanguageRepository { get; } protected IDataTypeService DataTypeService { get; } protected IRelationRepository RelationRepository { get; } diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs index 7c8d816ec8..e5f618878d 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Data; using System.Globalization; @@ -44,6 +44,11 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement protected ILanguageRepository LanguageRepository { get; } protected abstract bool SupportsPublishing { get; } + /// + /// Gets the node object type for the repository's entity + /// + protected abstract Guid NodeObjectTypeId { get; } + public IEnumerable> Move(TEntity moving, EntityContainer container) { var parentId = Cms.Core.Constants.System.Root; diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DataTypeRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DataTypeRepository.cs index 5647ae4d72..001f2db000 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DataTypeRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DataTypeRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Data; using System.Globalization; @@ -112,7 +112,7 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return Array.Empty(); } - protected override Guid NodeObjectTypeId => Cms.Core.Constants.ObjectTypes.DataType; + protected Guid NodeObjectTypeId => Cms.Core.Constants.ObjectTypes.DataType; #endregion diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs index 3cb9662a89..b953bc5b55 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.Extensions.Logging; @@ -120,11 +120,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return new List(); } - protected override Guid NodeObjectTypeId - { - get { throw new NotImplementedException(); } - } - #endregion #region Unit of Work Implementation diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DocumentRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DocumentRepository.cs index 3500f0458d..a0e651f1dd 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DocumentRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DocumentRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.Extensions.Logging; @@ -956,6 +956,7 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement // reading repository purely for looking up by GUID // TODO: ugly and to fix we need to decouple the IRepositoryQueryable -> IRepository -> IReadRepository which should all be separate things! + // This sub-repository pattern is super old and totally unecessary anymore, caching can be handled in much nicer ways without this private class ContentByGuidReadRepository : EntityRepositoryBase { private readonly DocumentRepository _outerRepo; @@ -966,8 +967,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement _outerRepo = outerRepo; } - protected override Guid NodeObjectTypeId => _outerRepo.NodeObjectTypeId; - protected override IContent PerformGet(Guid id) { var sql = _outerRepo.GetBaseQuery(QueryType.Single) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DomainRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DomainRepository.cs index fe9f74cd4a..4db9a3b0cc 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DomainRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DomainRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Data; using System.Linq; @@ -81,11 +81,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return list; } - protected override Guid NodeObjectTypeId - { - get { throw new NotImplementedException(); } - } - protected override void PersistNewItem(IDomain entity) { var exists = Database.ExecuteScalar("SELECT COUNT(*) FROM umbracoDomain WHERE domainName = @domainName", new { domainName = entity.DomainName }); diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/EntityContainerRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/EntityContainerRepository.cs index 9f6912c0fe..bc1b1b1881 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/EntityContainerRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/EntityContainerRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.Extensions.Logging; @@ -113,20 +113,11 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return sql; } - protected override string GetBaseWhereClause() - { - return "umbracoNode.id = @id and nodeObjectType = @NodeObjectType"; - } + protected override string GetBaseWhereClause() => "umbracoNode.id = @id and nodeObjectType = @NodeObjectType"; - protected override IEnumerable GetDeleteClauses() - { - throw new NotImplementedException(); - } + protected override IEnumerable GetDeleteClauses() => throw new NotImplementedException(); - protected override Guid NodeObjectTypeId - { - get { return _containerObjectType; } - } + protected Guid NodeObjectTypeId => _containerObjectType; protected override void PersistDeletedItem(EntityContainer entity) { diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/EntityRepositoryBase.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/EntityRepositoryBase.cs index 3b93e09867..25927213e5 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/EntityRepositoryBase.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/EntityRepositoryBase.cs @@ -78,11 +78,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return PerformCount(query); })); - /// - /// Gets the node object type for the repository's entity - /// - protected abstract Guid NodeObjectTypeId { get; } - /// /// Gets the repository cache policy /// diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs index ff64fb0d49..ab9f4b45a9 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs @@ -6,6 +6,7 @@ using NPoco; using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.Models.Entities; using Umbraco.Cms.Core.Models.Identity; +using Umbraco.Cms.Core.Persistence; using Umbraco.Cms.Core.Persistence.Querying; using Umbraco.Cms.Core.Persistence.Repositories; using Umbraco.Cms.Core.Scoping; @@ -24,10 +25,7 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement : base(scopeAccessor, cache, logger) { } - public void DeleteUserLogins(int memberId) - { - Database.Delete("WHERE userId=@userId", new { userId = memberId }); - } + public void DeleteUserLogins(int memberId) => Database.Delete("WHERE userId=@userId", new { userId = memberId }); public void Save(int userId, IEnumerable logins) { @@ -45,40 +43,36 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement var toInsert = new List(logins); var existingLogins = Database.Query(sql).OrderByDescending(x => x.CreateDate).ToList(); - // used to track duplicates so they can be removed - var keys = new HashSet<(string, string)>(); - + foreach (var existing in existingLogins) { - if (!keys.Add((existing.ProviderKey, existing.LoginProvider))) - { - // if it already exists we need to remove this one - toDelete.Add(existing.Id); - } - else - { - var found = logins.FirstOrDefault(x => + var found = logins.FirstOrDefault(x => x.LoginProvider.Equals(existing.LoginProvider, StringComparison.InvariantCultureIgnoreCase) && x.ProviderKey.Equals(existing.ProviderKey, StringComparison.InvariantCultureIgnoreCase)); - if (found != null) - { - toUpdate.Add(existing.Id, found); - // if it's an update then it's not an insert - toInsert.RemoveAll(x => x.ProviderKey == found.ProviderKey && x.LoginProvider == found.LoginProvider); - } - else - { - toDelete.Add(existing.Id); - } + if (found != null) + { + toUpdate.Add(existing.Id, found); + // if it's an update then it's not an insert + toInsert.RemoveAll(x => x.ProviderKey == found.ProviderKey && x.LoginProvider == found.LoginProvider); + } + else + { + toDelete.Add(existing.Id); } } // do the deletes, updates and inserts if (toDelete.Count > 0) + { Database.DeleteMany().Where(x => toDelete.Contains(x.Id)).Execute(); + } + foreach (var u in toUpdate) + { Database.Update(ExternalLoginFactory.BuildDto(userId, u.Value, u.Key)); + } + Database.InsertBulk(toInsert.Select(i => ExternalLoginFactory.BuildDto(userId, i))); } @@ -157,10 +151,7 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return sql; } - protected override string GetBaseWhereClause() - { - return "umbracoExternalLogin.id = @id"; - } + protected override string GetBaseWhereClause() => "umbracoExternalLogin.id = @id"; protected override IEnumerable GetDeleteClauses() { @@ -171,11 +162,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return list; } - protected override Guid NodeObjectTypeId - { - get { throw new NotImplementedException(); } - } - protected override void PersistNewItem(IIdentityUserLogin entity) { entity.AddingEntity(); @@ -198,5 +184,113 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement entity.ResetDirtyProperties(); } + + /// + /// Query for user tokens + /// + /// + /// + public IEnumerable Get(IQuery query) + { + Sql sqlClause = GetBaseTokenQuery(false); + + var translator = new SqlTranslator(sqlClause, query); + Sql sql = translator.Translate(); + + List dtos = Database.Fetch(sql); + + foreach (ExternalLoginTokenDto dto in dtos) + { + yield return ExternalLoginFactory.BuildEntity(dto); + } + } + + /// + /// Count for user tokens + /// + /// + /// + public int Count(IQuery query) + { + Sql sql = Sql().SelectCount().From(); + return Database.ExecuteScalar(sql); + } + + public void Save(int userId, IEnumerable tokens) + { + // get the existing logins (provider + id) + var existingUserLogins = Database + .Fetch(GetBaseQuery(false).Where(x => x.UserId == userId)) + .ToDictionary(x => x.LoginProvider, x => x.Id); + + // deduplicate the tokens + tokens = tokens.DistinctBy(x => x.LoginProvider + x.Name).ToList(); + + var providers = tokens.Select(x => x.LoginProvider).Distinct().ToList(); + + Sql sql = GetBaseTokenQuery(true) + .WhereIn(x => x.LoginProvider, providers) + .Where(x => x.UserId == userId); + + var toUpdate = new Dictionary(); + var toDelete = new List(); + var toInsert = new List(tokens); + + var existingTokens = Database.Query(sql).OrderByDescending(x => x.CreateDate).ToList(); + + foreach (ExternalLoginTokenDto existing in existingTokens) + { + IExternalLoginToken found = tokens.FirstOrDefault(x => + x.LoginProvider.InvariantEquals(existing.ExternalLoginDto.LoginProvider) + && x.Name.InvariantEquals(existing.Name)); + + if (found != null) + { + toUpdate.Add(existing.Id, (found, existing.ExternalLoginId)); + // if it's an update then it's not an insert + toInsert.RemoveAll(x => x.LoginProvider.InvariantEquals(found.LoginProvider) && x.Name.InvariantEquals(found.Name)); + } + else + { + toDelete.Add(existing.Id); + } + } + + // do the deletes, updates and inserts + if (toDelete.Count > 0) + { + Database.DeleteMany().Where(x => toDelete.Contains(x.Id)).Execute(); + } + + foreach (KeyValuePair u in toUpdate) + { + Database.Update(ExternalLoginFactory.BuildDto(u.Value.externalLoginId, u.Value.externalLoginToken, u.Key)); + } + + var insertDtos = new List(); + foreach(IExternalLoginToken t in toInsert) + { + if (!existingUserLogins.TryGetValue(t.LoginProvider, out int externalLoginId)) + { + throw new InvalidOperationException($"A token was attempted to be saved for login provider {t.LoginProvider} which is not assigned to this user"); + } + insertDtos.Add(ExternalLoginFactory.BuildDto(externalLoginId, t)); + } + Database.InsertBulk(insertDtos); + } + + private Sql GetBaseTokenQuery(bool forUpdate) + => forUpdate ? Sql() + .Select(r => r.Select(x => x.ExternalLoginDto)) + .From() + .Append(" WITH (UPDLOCK)") // ensure these table values are locked for updates, the ForUpdate ext method does not work here + .InnerJoin() + .On(x => x.ExternalLoginId, x => x.Id) + : Sql() + .Select() + .AndSelect(x => x.LoginProvider, x => x.UserId) + .From() + .InnerJoin() + .On(x => x.ExternalLoginId, x => x.Id); } } diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/KeyValueRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/KeyValueRepository.cs index 40dfb4cecf..74988d2026 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/KeyValueRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/KeyValueRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.Extensions.Logging; @@ -33,8 +33,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement #region Overrides of EntityRepositoryBase - protected override Guid NodeObjectTypeId => throw new NotSupportedException(); - protected override Sql GetBaseQuery(bool isCount) { var sql = SqlContext.Sql(); diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LanguageRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LanguageRepository.cs index 73c77e9e05..90d53df9ab 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LanguageRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LanguageRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.Extensions.Logging; @@ -124,8 +124,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return list; } - protected override Guid NodeObjectTypeId => throw new NotImplementedException(); - #endregion #region Unit of Work Implementation diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MacroRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MacroRepository.cs index d4ee347c98..0a9eb7b0ef 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MacroRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MacroRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.Extensions.Logging; @@ -136,8 +136,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return list; } - protected override Guid NodeObjectTypeId => throw new NotImplementedException(); - protected override void PersistNewItem(IMacro entity) { entity.AddingEntity(); diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MediaRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MediaRepository.cs index ba1fe05d0c..3efe35e4c4 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MediaRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MediaRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using System.Text.RegularExpressions; @@ -407,13 +407,10 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return _mediaByGuidReadRepository.Exists(id); } - /// - /// A reading repository purely for looking up by GUID - /// - /// - /// TODO: This is ugly and to fix we need to decouple the IRepositoryQueryable -> IRepository -> IReadRepository which should all be separate things! - /// Then we can do the same thing with repository instances and we wouldn't need to leave all these methods as not implemented because we wouldn't need to implement them - /// + + // A reading repository purely for looking up by GUID + // TODO: This is ugly and to fix we need to decouple the IRepositoryQueryable -> IRepository -> IReadRepository which should all be separate things! + // This sub-repository pattern is super old and totally unecessary anymore, caching can be handled in much nicer ways without this private class MediaByGuidReadRepository : EntityRepositoryBase { private readonly MediaRepository _outerRepo; @@ -424,8 +421,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement _outerRepo = outerRepo; } - protected override Guid NodeObjectTypeId => _outerRepo.NodeObjectTypeId; - protected override IMedia PerformGet(Guid id) { var sql = _outerRepo.GetBaseQuery(QueryType.Single) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberGroupRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberGroupRepository.cs index 84523777ee..abce17a331 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberGroupRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberGroupRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.Extensions.Logging; @@ -89,7 +89,7 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return list; } - protected override Guid NodeObjectTypeId => Cms.Core.Constants.ObjectTypes.MemberGroup; + protected Guid NodeObjectTypeId => Cms.Core.Constants.ObjectTypes.MemberGroup; protected override void PersistNewItem(IMemberGroup entity) { diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/PermissionRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/PermissionRepository.cs index 34fe764203..582120992b 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/PermissionRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/PermissionRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Globalization; using System.Linq; @@ -280,8 +280,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return new List(); } - protected override Guid NodeObjectTypeId => throw new InvalidOperationException("This property won't be implemented."); - protected override void PersistDeletedItem(ContentPermissionSet entity) { throw new InvalidOperationException("This method won't be implemented."); diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/PublicAccessRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/PublicAccessRepository.cs index c8bb10e7d9..a618e0e49f 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/PublicAccessRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/PublicAccessRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.Extensions.Logging; @@ -81,8 +81,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return list; } - protected override Guid NodeObjectTypeId => throw new NotImplementedException(); - protected override void PersistNewItem(PublicAccessEntry entity) { entity.AddingEntity(); diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RedirectUrlRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RedirectUrlRepository.cs index 5048812a3a..1536768bae 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RedirectUrlRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RedirectUrlRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using System.Security.Cryptography; @@ -79,11 +79,6 @@ JOIN umbracoNode ON umbracoRedirectUrl.contentKey=umbracoNode.uniqueID"); return list; } - protected override Guid NodeObjectTypeId - { - get { throw new NotImplementedException(); } - } - protected override void PersistNewItem(IRedirectUrl entity) { var dto = Map(entity); diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RelationRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RelationRepository.cs index 0927fe66af..f531c61037 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RelationRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RelationRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.Extensions.Logging; @@ -126,11 +126,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return list; } - protected override Guid NodeObjectTypeId - { - get { throw new NotImplementedException(); } - } - #endregion #region Unit of Work Implementation diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RelationTypeRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RelationTypeRepository.cs index 7ae651cc9e..f6f3454560 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RelationTypeRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RelationTypeRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.Extensions.Logging; @@ -121,11 +121,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return list; } - protected override Guid NodeObjectTypeId - { - get { throw new NotImplementedException(); } - } - #endregion #region Unit of Work Implementation diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ServerRegistrationRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ServerRegistrationRepository.cs index 44cdd445d1..66498b3f67 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ServerRegistrationRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ServerRegistrationRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.Extensions.Logging; @@ -94,8 +94,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return list; } - protected override Guid NodeObjectTypeId => throw new NotImplementedException(); - protected override void PersistNewItem(IServerRegistration entity) { entity.AddingEntity(); diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimpleGetRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimpleGetRepository.cs index 7fe013290b..6053d35085 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimpleGetRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimpleGetRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.Extensions.Logging; @@ -81,8 +81,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement throw new InvalidOperationException("This method won't be implemented."); } - protected sealed override Guid NodeObjectTypeId => throw new InvalidOperationException("This property won't be implemented."); - protected sealed override void PersistNewItem(TEntity entity) { throw new InvalidOperationException("This method won't be implemented."); diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TagRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TagRepository.cs index 9557383347..8fbc784576 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TagRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TagRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using System.Text; @@ -25,9 +25,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement #region Manage Tag Entities - /// - protected override Guid NodeObjectTypeId => throw new NotSupportedException(); - /// protected override ITag PerformGet(int id) { diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TemplateRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TemplateRepository.cs index e39a5eb7b7..ec54a6bef8 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TemplateRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TemplateRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.IO; using System.Linq; @@ -140,7 +140,7 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return list; } - protected override Guid NodeObjectTypeId => Cms.Core.Constants.ObjectTypes.Template; + protected Guid NodeObjectTypeId => Cms.Core.Constants.ObjectTypes.Template; protected override void PersistNewItem(ITemplate entity) { diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserGroupRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserGroupRepository.cs index d396fdcff7..ad5e36c0ab 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserGroupRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserGroupRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.Extensions.Logging; @@ -291,8 +291,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return list; } - protected override Guid NodeObjectTypeId => throw new InvalidOperationException("This property won't be implemented."); - protected override void PersistNewItem(IUserGroup entity) { entity.AddingEntity(); @@ -403,8 +401,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement throw new InvalidOperationException("This method won't be implemented."); } - protected override Guid NodeObjectTypeId => throw new InvalidOperationException("This property won't be implemented."); - #endregion protected override void PersistNewItem(UserGroupWithUsers entity) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs index 4e4f34bf54..93700549ac 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using System.Linq.Expressions; @@ -431,8 +431,6 @@ ORDER BY colName"; return list; } - protected override Guid NodeObjectTypeId => throw new NotImplementedException(); - protected override void PersistNewItem(IUser entity) { // the use may have no identity, ie ID is zero, and be v7 super diff --git a/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs b/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs index bd05ce0461..8205a380ef 100644 --- a/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs @@ -102,6 +102,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(BackOfficeIdentityUser.Logins)); + var isTokensPropertyDirty = user.IsPropertyDirty(nameof(BackOfficeIdentityUser.LoginTokens)); UpdateMemberProperties(userEntity, user); @@ -125,6 +126,16 @@ namespace Umbraco.Cms.Core.Security x.UserData))); } + if (isTokensPropertyDirty) + { + _externalLoginService.Save( + userEntity.Id, + user.LoginTokens.Select(x => new ExternalLoginToken( + x.LoginProvider, + x.Name, + x.Value))); + } + return Task.FromResult(IdentityResult.Success); } @@ -151,6 +162,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(BackOfficeIdentityUser.Logins)); + var isTokensPropertyDirty = user.IsPropertyDirty(nameof(BackOfficeIdentityUser.LoginTokens)); if (UpdateMemberProperties(found, user)) { @@ -166,6 +178,16 @@ namespace Umbraco.Cms.Core.Security x.ProviderKey, x.UserData))); } + + if (isTokensPropertyDirty) + { + _externalLoginService.Save( + found.Id, + user.LoginTokens.Select(x => new ExternalLoginToken( + x.LoginProvider, + x.Name, + x.Value))); + } } scope.Complete(); @@ -543,7 +565,9 @@ namespace Umbraco.Cms.Core.Security { if (user != null) { - user.SetLoginsCallback(new Lazy>(() => _externalLoginService.GetAll(UserIdToInt(user.Id)))); + var userId = UserIdToInt(user.Id); + user.SetLoginsCallback(new Lazy>(() => _externalLoginService.GetExternalLogins(userId))); + user.SetTokensCallback(new Lazy>(() => _externalLoginService.GetExternalLoginTokens(userId))); } return user; @@ -757,24 +781,102 @@ namespace Umbraco.Cms.Core.Security [EditorBrowsable(EditorBrowsableState.Never)] public override Task> GetUsersForClaimAsync(Claim claim, CancellationToken cancellationToken = default) => throw new NotImplementedException(); - // TODO: We should support these /// - /// Not supported in Umbraco + /// 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(BackOfficeIdentityUser 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; + } + + /// + /// Overridden to support Umbraco's own data storage requirements + /// + /// + /// The base class's implementation of this calls into FindTokenAsync, RemoveUserTokenAsync and AddUserTokenAsync, both methods will only work with ORMs that are change + /// tracking ORMs like EFCore. + /// + /// + public override Task RemoveTokenAsync(BackOfficeIdentityUser user, string loginProvider, string name, 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.Remove(token); + } + + return Task.CompletedTask; + } + + /// + /// Overridden to support Umbraco's own data storage requirements + /// + /// + /// The base class's implementation of this calls into FindTokenAsync, RemoveUserTokenAsync and AddUserTokenAsync, both methods will only work with ORMs that are change + /// tracking ORMs like EFCore. + /// + /// + public override Task GetTokenAsync(BackOfficeIdentityUser user, string loginProvider, string name, 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)); + + return Task.FromResult(token?.Value); + } + + /// + /// Not supported in Umbraco, see comments above on GetTokenAsync, RemoveTokenAsync, SetTokenAsync /// /// - [EditorBrowsable(EditorBrowsableState.Never)] protected override Task> FindTokenAsync(BackOfficeIdentityUser user, string loginProvider, string name, CancellationToken cancellationToken) => throw new NotImplementedException(); /// - /// Not supported in Umbraco + /// Not supported in Umbraco, see comments above on GetTokenAsync, RemoveTokenAsync, SetTokenAsync /// /// [EditorBrowsable(EditorBrowsableState.Never)] protected override Task AddUserTokenAsync(IdentityUserToken token) => throw new NotImplementedException(); /// - /// Not supported in Umbraco + /// Not supported in Umbraco, see comments above on GetTokenAsync, RemoveTokenAsync, SetTokenAsync /// /// [EditorBrowsable(EditorBrowsableState.Never)] diff --git a/src/Umbraco.Infrastructure/Security/UmbracoIdentityUser.cs b/src/Umbraco.Infrastructure/Security/UmbracoIdentityUser.cs index 9d3c1ad7c9..34ba0d2a12 100644 --- a/src/Umbraco.Infrastructure/Security/UmbracoIdentityUser.cs +++ b/src/Umbraco.Infrastructure/Security/UmbracoIdentityUser.cs @@ -37,7 +37,9 @@ namespace Umbraco.Cms.Core.Models.Identity private string _passwordHash; private DateTime? _lastPasswordChangeDateUtc; private ObservableCollection _logins; + private ObservableCollection _tokens; private Lazy> _getLogins; + private Lazy> _getTokens; private ObservableCollection> _roles; /// @@ -180,6 +182,37 @@ namespace Umbraco.Cms.Core.Models.Identity } } + /// + /// Gets the external login tokens collection + /// + public ICollection LoginTokens + { + get + { + // return if it exists + if (_tokens != null) + { + return _tokens; + } + + _tokens = new ObservableCollection(); + + // if the callback is there and hasn't been created yet then execute it and populate the logins + if (_getTokens != null && !_getTokens.IsValueCreated) + { + foreach (IIdentityUserToken l in _getTokens.Value) + { + _tokens.Add(l); + } + } + + // now assign events + _tokens.CollectionChanged += LoginTokens_CollectionChanged; + + return _tokens; + } + } + /// /// Gets or sets user ID (Primary Key) /// @@ -266,10 +299,18 @@ namespace Umbraco.Cms.Core.Models.Identity /// Used to set a lazy call back to populate the user's Login list /// /// The lazy value - public void SetLoginsCallback(Lazy> callback) => _getLogins = callback ?? throw new ArgumentNullException(nameof(callback)); + internal void SetLoginsCallback(Lazy> callback) => _getLogins = callback ?? throw new ArgumentNullException(nameof(callback)); + + /// + /// Used to set a lazy call back to populate the user's token list + /// + /// The lazy value + internal void SetTokensCallback(Lazy> callback) => _getTokens = callback ?? throw new ArgumentNullException(nameof(callback)); private void Logins_CollectionChanged(object sender, NotifyCollectionChangedEventArgs e) => BeingDirty.OnPropertyChanged(nameof(Logins)); + private void LoginTokens_CollectionChanged(object sender, NotifyCollectionChangedEventArgs e) => BeingDirty.OnPropertyChanged(nameof(LoginTokens)); + private void Roles_CollectionChanged(object sender, NotifyCollectionChangedEventArgs e) => BeingDirty.OnPropertyChanged(nameof(Roles)); } } diff --git a/src/Umbraco.Infrastructure/Services/Implement/ExternalLoginService.cs b/src/Umbraco.Infrastructure/Services/Implement/ExternalLoginService.cs index bbec75338b..03bd495fc2 100644 --- a/src/Umbraco.Infrastructure/Services/Implement/ExternalLoginService.cs +++ b/src/Umbraco.Infrastructure/Services/Implement/ExternalLoginService.cs @@ -20,16 +20,28 @@ namespace Umbraco.Cms.Core.Services.Implement } /// - public IEnumerable GetAll(int userId) + public IEnumerable GetExternalLogins(int userId) { using (var scope = ScopeProvider.CreateScope(autoComplete: true)) { - var asString = userId.ToString(); // TODO: This is temp until we update the external service to support guids for both users and members + // TODO: This is temp until we update the external service to support guids for both users and members + var asString = userId.ToString(); return _externalLoginRepository.Get(Query().Where(x => x.UserId == asString)) .ToList(); } } + public IEnumerable GetExternalLoginTokens(int userId) + { + using (var scope = ScopeProvider.CreateScope(autoComplete: true)) + { + // TODO: This is temp until we update the external service to support guids for both users and members + var asString = userId.ToString(); + return _externalLoginRepository.Get(Query().Where(x => x.UserId == asString)) + .ToList(); + } + } + /// public IEnumerable Find(string loginProvider, string providerKey) { @@ -51,6 +63,15 @@ namespace Umbraco.Cms.Core.Services.Implement } } + public void Save(int userId, IEnumerable tokens) + { + using (var scope = ScopeProvider.CreateScope()) + { + _externalLoginRepository.Save(userId, tokens); + scope.Complete(); + } + } + /// public void Save(IIdentityUserLogin login) { @@ -70,7 +91,5 @@ namespace Umbraco.Cms.Core.Services.Implement scope.Complete(); } } - - } } diff --git a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ExternalLoginServiceTests.cs b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ExternalLoginServiceTests.cs index 64eb16ff8a..b6a4e6bc4e 100644 --- a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ExternalLoginServiceTests.cs +++ b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ExternalLoginServiceTests.cs @@ -17,7 +17,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services { [TestFixture] [Apartment(ApartmentState.STA)] - [UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerFixture)] + [UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)] public class ExternalLoginServiceTests : UmbracoIntegrationTest { private IUserService UserService => GetRequiredService(); @@ -25,6 +25,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services private IExternalLoginService ExternalLoginService => GetRequiredService(); [Test] + [Ignore("We don't support duplicates anymore, this removing on save was a breaking change work around, this needs to be ported to a migration")] public void Removes_Existing_Duplicates_On_Save() { var user = new User(GlobalSettings, "Test", "test@test.com", "test", "helloworldtest"); @@ -63,7 +64,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services ExternalLoginService.Save(user.Id, externalLogins); - var logins = ExternalLoginService.GetAll(user.Id).ToList(); + var logins = ExternalLoginService.GetExternalLogins(user.Id).ToList(); // duplicates will be removed, keeping the latest entries Assert.AreEqual(2, logins.Count); @@ -87,7 +88,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services ExternalLoginService.Save(user.Id, externalLogins); - var logins = ExternalLoginService.GetAll(user.Id).ToList(); + var logins = ExternalLoginService.GetExternalLogins(user.Id).ToList(); Assert.AreEqual(1, logins.Count); } @@ -103,7 +104,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services }; ExternalLoginService.Save(extLogin); - IEnumerable found = ExternalLoginService.GetAll(user.Id); + IEnumerable found = ExternalLoginService.GetExternalLogins(user.Id); Assert.AreEqual(1, found.Count()); Assert.IsTrue(extLogin.HasIdentity); @@ -125,7 +126,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services extLogin.UserData = "world"; ExternalLoginService.Save(extLogin); - var found = ExternalLoginService.GetAll(user.Id).ToList(); + var found = ExternalLoginService.GetExternalLogins(user.Id).ToList(); Assert.AreEqual(1, found.Count); Assert.AreEqual("world", found[0].UserData); } @@ -152,7 +153,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services }; ExternalLoginService.Save(user.Id, extLogins); - var found = ExternalLoginService.GetAll(user.Id).OrderBy(x => x.LoginProvider).ToList(); + var found = ExternalLoginService.GetExternalLogins(user.Id).OrderBy(x => x.LoginProvider).ToList(); Assert.AreEqual(2, found.Count); Assert.AreEqual("123456", found[0].UserData); Assert.AreEqual("987654", found[1].UserData); @@ -193,7 +194,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services ExternalLoginService.Save(user.Id, externalLogins); - var logins = ExternalLoginService.GetAll(user.Id).OrderBy(x => x.LoginProvider).ToList(); + var logins = ExternalLoginService.GetExternalLogins(user.Id).OrderBy(x => x.LoginProvider).ToList(); Assert.AreEqual(2, logins.Count); for (int i = 0; i < logins.Count; i++) { @@ -202,6 +203,31 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services } } + [Test] + public void Add_Tokens() + { + var user = new User(GlobalSettings, "Test", "test@test.com", "test", "helloworldtest"); + UserService.Save(user); + + ExternalLogin[] externalLogins = new[] + { + new ExternalLogin("test1", Guid.NewGuid().ToString("N")) + }; + + ExternalLoginService.Save(user.Id, externalLogins); + + ExternalLoginToken[] externalTokens = new[] + { + new ExternalLoginToken(externalLogins[0].LoginProvider, "hello1", "world1"), + new ExternalLoginToken(externalLogins[0].LoginProvider, "hello2", "world2") + }; + + ExternalLoginService.Save(user.Id, externalTokens); + + var tokens = ExternalLoginService.GetExternalLoginTokens(user.Id).ToList(); + Assert.AreEqual(2, tokens.Count); + } + [Test] public void Add_Update_Delete_Logins() { @@ -218,19 +244,64 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services ExternalLoginService.Save(user.Id, externalLogins); - var logins = ExternalLoginService.GetAll(user.Id).OrderBy(x => x.LoginProvider).ToList(); + var logins = ExternalLoginService.GetExternalLogins(user.Id).OrderBy(x => x.LoginProvider).ToList(); logins.RemoveAt(0); // remove the first one logins.Add(new IdentityUserLogin("test5", Guid.NewGuid().ToString("N"), user.Id.ToString())); // add a new one + logins[0].ProviderKey = "abcd123"; // update // save new list ExternalLoginService.Save(user.Id, logins.Select(x => new ExternalLogin(x.LoginProvider, x.ProviderKey))); - var updatedLogins = ExternalLoginService.GetAll(user.Id).OrderBy(x => x.LoginProvider).ToList(); + var updatedLogins = ExternalLoginService.GetExternalLogins(user.Id).OrderBy(x => x.LoginProvider).ToList(); Assert.AreEqual(4, updatedLogins.Count); for (int i = 0; i < updatedLogins.Count; i++) { Assert.AreEqual(logins[i].LoginProvider, updatedLogins[i].LoginProvider); + Assert.AreEqual(logins[i].ProviderKey, updatedLogins[i].ProviderKey); + } + } + + [Test] + public void Add_Update_Delete_Tokens() + { + var user = new User(GlobalSettings, "Test", "test@test.com", "test", "helloworldtest"); + UserService.Save(user); + + ExternalLogin[] externalLogins = new[] + { + new ExternalLogin("test1", Guid.NewGuid().ToString("N")), + new ExternalLogin("test2", Guid.NewGuid().ToString("N")) + }; + + ExternalLoginService.Save(user.Id, externalLogins); + + ExternalLoginToken[] externalTokens = new[] + { + new ExternalLoginToken(externalLogins[0].LoginProvider, "hello1", "world1"), + new ExternalLoginToken(externalLogins[0].LoginProvider, "hello1a", "world1a"), + new ExternalLoginToken(externalLogins[1].LoginProvider, "hello2", "world2"), + new ExternalLoginToken(externalLogins[1].LoginProvider, "hello2a", "world2a") + }; + + ExternalLoginService.Save(user.Id, externalTokens); + + var tokens = ExternalLoginService.GetExternalLoginTokens(user.Id).OrderBy(x => x.LoginProvider).ToList(); + + tokens.RemoveAt(0); // remove the first one + tokens.Add(new IdentityUserToken(externalLogins[1].LoginProvider, "hello2b", "world2b", user.Id.ToString())); // add a new one + tokens[0].Value = "abcd123"; // update + + // save new list + ExternalLoginService.Save(user.Id, tokens.Select(x => new ExternalLoginToken(x.LoginProvider, x.Name, x.Value))); + + var updatedTokens = ExternalLoginService.GetExternalLoginTokens(user.Id).OrderBy(x => x.LoginProvider).ToList(); + Assert.AreEqual(4, updatedTokens.Count); + for (int i = 0; i < updatedTokens.Count; i++) + { + Assert.AreEqual(tokens[i].LoginProvider, updatedTokens[i].LoginProvider); + Assert.AreEqual(tokens[i].Name, updatedTokens[i].Name); + Assert.AreEqual(tokens[i].Value, updatedTokens[i].Value); } } @@ -247,7 +318,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services ExternalLoginService.Save(user.Id, externalLogins); - var logins = ExternalLoginService.GetAll(user.Id).ToList(); + var logins = ExternalLoginService.GetExternalLogins(user.Id).ToList(); Assert.AreEqual("hello world", logins[0].UserData); } diff --git a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs index 1ffc4ab996..5ad114880a 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs @@ -32,6 +32,7 @@ using Umbraco.Cms.Web.Common.Controllers; using Umbraco.Cms.Web.Common.Filters; using Umbraco.Extensions; using Constants = Umbraco.Cms.Core.Constants; +using SignInResult = Microsoft.AspNetCore.Identity.SignInResult; namespace Umbraco.Cms.Web.BackOffice.Controllers { @@ -314,7 +315,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers { // Sign the user in with username/password, this also gives a chance for developers to // custom verify the credentials and auto-link user accounts with a custom IBackOfficePasswordChecker - var result = await _signInManager.PasswordSignInAsync( + SignInResult result = await _signInManager.PasswordSignInAsync( loginModel.Username, loginModel.Password, isPersistent: true, lockoutOnFailure: true); if (result.Succeeded) @@ -331,7 +332,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return new ValidationErrorResult($"The registered {typeof(IBackOfficeTwoFactorOptions)} of type {_backOfficeTwoFactorOptions.GetType()} did not return a view for two factor auth "); } - var attemptedUser = _userService.GetByUsername(loginModel.Username); + IUser attemptedUser = _userService.GetByUsername(loginModel.Username); // create a with information to display a custom two factor send code view var verifyResponse = new ObjectResult(new diff --git a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs index 115c782af7..d97ae3e8ae 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs @@ -42,6 +42,9 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers [IsBackOffice] public class BackOfficeController : UmbracoController { + // See here for examples of what a lot of this is doing: https://github.com/dotnet/aspnetcore/blob/main/src/Identity/samples/IdentitySample.Mvc/Controllers/AccountController.cs + // along with our AuthenticationController + // NOTE: Each action must either be explicitly authorized or explicitly [AllowAnonymous], the latter is optional because // this controller itself doesn't require authz but it's more clear what the intention is. @@ -339,11 +342,15 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers /// /// Callback path when the user initiates a link login request from the back office to the external provider from the action /// + /// + /// An example of this is here https://github.com/dotnet/aspnetcore/blob/main/src/Identity/samples/IdentitySample.Mvc/Controllers/AccountController.cs#L155 + /// which this is based on + /// [Authorize(Policy = AuthorizationPolicies.BackOfficeAccess)] [HttpGet] public async Task ExternalLinkLoginCallback() { - var user = await _userManager.GetUserAsync(User); + BackOfficeIdentityUser user = await _userManager.GetUserAsync(User); if (user == null) { // ... this should really not happen @@ -351,22 +358,20 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return RedirectToLocal(Url.Action(nameof(Default), this.GetControllerName())); } - var loginInfo = await _signInManager.GetExternalLoginInfoAsync(await _userManager.GetUserIdAsync(user)); + ExternalLoginInfo info = await _signInManager.GetExternalLoginInfoAsync(await _userManager.GetUserIdAsync(user)); - if (loginInfo == null) + if (info == null) { //Add error and redirect for it to be displayed TempData[ViewDataExtensions.TokenExternalSignInError] = new[] { "An error occurred, could not get external login info" }; return RedirectToLocal(Url.Action(nameof(Default), this.GetControllerName())); } - var addLoginResult = await _userManager.AddLoginAsync(user, loginInfo); + IdentityResult addLoginResult = await _userManager.AddLoginAsync(user, info); if (addLoginResult.Succeeded) { - // Update any authentication tokens if login succeeded - // TODO: This is a new thing that we need to implement and because we can store data with the external login now, this is exactly - // what this is for but we'll need to peek under the code here to figure out exactly what goes on. - //await _signInManager.UpdateExternalAuthenticationTokensAsync(loginInfo); + // Update any authentication tokens if succeeded + await _signInManager.UpdateExternalAuthenticationTokensAsync(info); return RedirectToLocal(Url.Action(nameof(Default), this.GetControllerName())); } @@ -426,12 +431,13 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers // Sign in the user with this external login provider (which auto links, etc...) var result = await _signInManager.ExternalLoginSignInAsync(loginInfo, isPersistent: false); - + var errors = new List(); if (result == Microsoft.AspNetCore.Identity.SignInResult.Success) { - + // Update any authentication tokens if succeeded + await _signInManager.UpdateExternalAuthenticationTokensAsync(loginInfo); } else if (result == Microsoft.AspNetCore.Identity.SignInResult.TwoFactorRequired) { diff --git a/src/Umbraco.Web.BackOffice/Security/IBackOfficeSignInManager.cs b/src/Umbraco.Web.BackOffice/Security/IBackOfficeSignInManager.cs index 18e749bddb..38ed6ba74a 100644 --- a/src/Umbraco.Web.BackOffice/Security/IBackOfficeSignInManager.cs +++ b/src/Umbraco.Web.BackOffice/Security/IBackOfficeSignInManager.cs @@ -22,5 +22,6 @@ namespace Umbraco.Cms.Web.BackOffice.Security Task SignInAsync(BackOfficeIdentityUser user, bool isPersistent, string authenticationMethod = null); Task CreateUserPrincipalAsync(BackOfficeIdentityUser user); Task TwoFactorSignInAsync(string provider, string code, bool isPersistent, bool rememberClient); + Task UpdateExternalAuthenticationTokensAsync(ExternalLoginInfo externalLogin); } } diff --git a/src/Umbraco.Web.UI.NetCore/Program.cs b/src/Umbraco.Web.UI.NetCore/Program.cs index 89ce7d92bc..7a6ee4dcfd 100644 --- a/src/Umbraco.Web.UI.NetCore/Program.cs +++ b/src/Umbraco.Web.UI.NetCore/Program.cs @@ -1,4 +1,5 @@ using Microsoft.AspNetCore.Hosting; +using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; @@ -7,18 +8,20 @@ namespace Umbraco.Cms.Web.UI.NetCore public class Program { public static void Main(string[] args) - { - CreateHostBuilder(args) + => CreateHostBuilder(args) .Build() .Run(); - } public static IHostBuilder CreateHostBuilder(string[] args) => Host.CreateDefaultBuilder(args) - .ConfigureLogging(x => - { - x.ClearProviders(); - }) - .ConfigureWebHostDefaults(webBuilder => { webBuilder.UseStartup(); }); +#if DEBUG + .ConfigureAppConfiguration(config + => config.AddJsonFile( + "appsettings.Local.json", + optional: true, + reloadOnChange: true)) +#endif + .ConfigureLogging(x => x.ClearProviders()) + .ConfigureWebHostDefaults(webBuilder => webBuilder.UseStartup()); } } diff --git a/src/Umbraco.Web.UI.NetCore/appsettings.json b/src/Umbraco.Web.UI.NetCore/appsettings.json index a3e57978da..68bdfbc140 100644 --- a/src/Umbraco.Web.UI.NetCore/appsettings.json +++ b/src/Umbraco.Web.UI.NetCore/appsettings.json @@ -38,8 +38,8 @@ "ConvertUrlsToAscii": "try" }, "RuntimeMinification": { - "dataFolder": "App_Data/TEMP/Smidge", - "version": "637432008251409860" + "dataFolder": "Umbraco/Data/TEMP/Smidge", + "version": "637510451273675926" }, "Security": { "KeepUserLoggedIn": false, From 141af1fd4f842709e517b01579783f6396ec3dcd Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 12 Mar 2021 13:24:35 +1100 Subject: [PATCH 2/8] Adds logic to remove duplicates before adding the unique indexes. --- .../V_9_0_0/ExternalLoginTableIndexes.cs | 24 ++++++++++++++++++- .../Implement/ExternalLoginRepository.cs | 4 ++-- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_0_0/ExternalLoginTableIndexes.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_0_0/ExternalLoginTableIndexes.cs index 43d059b791..cf1124c4d9 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_0_0/ExternalLoginTableIndexes.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_9_0_0/ExternalLoginTableIndexes.cs @@ -1,3 +1,5 @@ +using System.Collections.Generic; +using System.Linq; using Umbraco.Cms.Infrastructure.Persistence.Dtos; namespace Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_9_0_0 @@ -14,7 +16,27 @@ namespace Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_9_0_0 /// public override void Migrate() { - // TODO: Before adding these indexes we need to remove duplicate data + // Before adding these indexes we need to remove duplicate data. + // Get all logins by latest + var logins = Database.Fetch() + .OrderByDescending(x => x.CreateDate) + .ToList(); + + var toDelete = new List(); + // used to track duplicates so they can be removed + var keys = new HashSet<(string, string)>(); + foreach(ExternalLoginDto login in logins) + { + if (!keys.Add((login.ProviderKey, login.LoginProvider))) + { + // if it already exists we need to remove this one + toDelete.Add(login.Id); + } + } + if (toDelete.Count > 0) + { + Database.DeleteMany().Where(x => toDelete.Contains(x.Id)).Execute(); + } var indexName1 = "IX_" + ExternalLoginDto.TableName + "_LoginProvider"; diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs index ab9f4b45a9..a262636bfc 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs @@ -42,7 +42,7 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement var toDelete = new List(); var toInsert = new List(logins); - var existingLogins = Database.Query(sql).OrderByDescending(x => x.CreateDate).ToList(); + var existingLogins = Database.Fetch(sql); foreach (var existing in existingLogins) { @@ -236,7 +236,7 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement var toDelete = new List(); var toInsert = new List(tokens); - var existingTokens = Database.Query(sql).OrderByDescending(x => x.CreateDate).ToList(); + var existingTokens = Database.Fetch(sql); foreach (ExternalLoginTokenDto existing in existingTokens) { From e20f73ba717fc821d5e555453250ff62f6b68e3f Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 12 Mar 2021 13:10:55 +0100 Subject: [PATCH 3/8] post merge fix --- .../Repositories/Implement/LogViewerQueryRepository.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LogViewerQueryRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LogViewerQueryRepository.cs index 35f0b8fdab..556aba8e22 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LogViewerQueryRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LogViewerQueryRepository.cs @@ -63,11 +63,6 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement return list; } - protected override Guid NodeObjectTypeId - { - get { throw new NotImplementedException(); } - } - protected override void PersistNewItem(ILogViewerQuery entity) { var exists = Database.ExecuteScalar($"SELECT COUNT(*) FROM {Core.Constants.DatabaseSchema.Tables.LogViewerQuery} WHERE name = @name", From 5c6b8fe2bc36a9348ac88b469fdb3e1ca8a2a42a Mon Sep 17 00:00:00 2001 From: Emma Garland Date: Mon, 15 Mar 2021 09:01:10 +0000 Subject: [PATCH 4/8] Added security logic previously added to PasswordChanger --- src/Umbraco.Web.BackOffice/Controllers/MemberController.cs | 1 + src/Umbraco.Web.BackOffice/Controllers/UsersController.cs | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs index 3c5adb8ebe..1ba7f41d97 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs @@ -477,6 +477,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers { return new ValidationErrorResult("The current user is not authorized"); } + var changingPasswordModel = new ChangingPasswordModel { Id = intId.Result, diff --git a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs index da19fa473a..ad81a32575 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.ComponentModel.DataAnnotations; using System.Globalization; using System.IO; using System.Linq; @@ -722,6 +723,11 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return new ValidationErrorResult("The current user is not authorized"); } + if (!currentUser.IsAdmin() && found.IsAdmin()) + { + return new ValidationErrorResult("The current user cannot change the password for the specified user"); + } + Attempt passwordChangeResult = await _passwordChanger.ChangePasswordWithIdentityAsync(changingPasswordModel, _userManager); if (passwordChangeResult.Success) From 1a573fcb7553d1451a4fc402d6a78bfe666472d4 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Tue, 16 Mar 2021 10:44:55 +0100 Subject: [PATCH 5/8] Review fixes https://github.com/umbraco/Umbraco-CMS/pull/9968 --- .../Models/Identity/IIdentityUserToken.cs | 2 +- .../Services/IExternalLoginService.cs | 6 -- .../Migrations/Install/DatabaseBuilder.cs | 2 +- .../Security/UmbracoIdentityUser.cs | 5 +- .../Implement/ExternalLoginService.cs | 10 --- .../Services/ExternalLoginServiceTests.cs | 62 ++++--------------- src/Umbraco.Web.UI.NetCore/appsettings.json | 2 +- 7 files changed, 17 insertions(+), 72 deletions(-) diff --git a/src/Umbraco.Core/Models/Identity/IIdentityUserToken.cs b/src/Umbraco.Core/Models/Identity/IIdentityUserToken.cs index df0c97ff08..c3a451f31d 100644 --- a/src/Umbraco.Core/Models/Identity/IIdentityUserToken.cs +++ b/src/Umbraco.Core/Models/Identity/IIdentityUserToken.cs @@ -5,7 +5,7 @@ namespace Umbraco.Cms.Core.Models.Identity /// /// An external login provider token /// - public interface IIdentityUserToken : IEntity, IRememberBeingDirty + public interface IIdentityUserToken : IEntity { /// /// Gets or sets user Id for the user who owns this token diff --git a/src/Umbraco.Core/Services/IExternalLoginService.cs b/src/Umbraco.Core/Services/IExternalLoginService.cs index b0c53c8f86..8834a4b33f 100644 --- a/src/Umbraco.Core/Services/IExternalLoginService.cs +++ b/src/Umbraco.Core/Services/IExternalLoginService.cs @@ -55,12 +55,6 @@ namespace Umbraco.Cms.Core.Services /// void Save(int userId, IEnumerable tokens); - /// - /// Save a single external login record - /// - /// - void Save(IIdentityUserLogin login); // TODO: This is unused apart from tests, perhaps remove it? - /// /// Deletes all user logins - normally used when a member is deleted /// diff --git a/src/Umbraco.Infrastructure/Migrations/Install/DatabaseBuilder.cs b/src/Umbraco.Infrastructure/Migrations/Install/DatabaseBuilder.cs index e06818a589..0da54785f2 100644 --- a/src/Umbraco.Infrastructure/Migrations/Install/DatabaseBuilder.cs +++ b/src/Umbraco.Infrastructure/Migrations/Install/DatabaseBuilder.cs @@ -156,7 +156,7 @@ namespace Umbraco.Cms.Infrastructure.Migrations.Install { _configManipulator.SaveConnectionString(EmbeddedDatabaseConnectionString, Constants.DbProviderNames.SqlCe); - var path = _hostingEnvironment.MapPathContentRoot(Path.Combine("App_Data", "Umbraco.sdf")); + var path = _hostingEnvironment.MapPathContentRoot(Path.Combine(Constants.SystemDirectories.Data, "Umbraco.sdf")); if (File.Exists(path) == false) { // this should probably be in a "using (new SqlCeEngine)" clause but not sure diff --git a/src/Umbraco.Infrastructure/Security/UmbracoIdentityUser.cs b/src/Umbraco.Infrastructure/Security/UmbracoIdentityUser.cs index 34ba0d2a12..525e7f839a 100644 --- a/src/Umbraco.Infrastructure/Security/UmbracoIdentityUser.cs +++ b/src/Umbraco.Infrastructure/Security/UmbracoIdentityUser.cs @@ -190,7 +190,7 @@ namespace Umbraco.Cms.Core.Models.Identity get { // return if it exists - if (_tokens != null) + if (_tokens is not null) { return _tokens; } @@ -198,7 +198,8 @@ namespace Umbraco.Cms.Core.Models.Identity _tokens = new ObservableCollection(); // if the callback is there and hasn't been created yet then execute it and populate the logins - if (_getTokens != null && !_getTokens.IsValueCreated) + // if (_getTokens != null && !_getTokens.IsValueCreated) + if (_getTokens?.IsValueCreated != true) { foreach (IIdentityUserToken l in _getTokens.Value) { diff --git a/src/Umbraco.Infrastructure/Services/Implement/ExternalLoginService.cs b/src/Umbraco.Infrastructure/Services/Implement/ExternalLoginService.cs index 03bd495fc2..79c827f641 100644 --- a/src/Umbraco.Infrastructure/Services/Implement/ExternalLoginService.cs +++ b/src/Umbraco.Infrastructure/Services/Implement/ExternalLoginService.cs @@ -72,16 +72,6 @@ namespace Umbraco.Cms.Core.Services.Implement } } - /// - public void Save(IIdentityUserLogin login) - { - using (var scope = ScopeProvider.CreateScope()) - { - _externalLoginRepository.Save(login); - scope.Complete(); - } - } - /// public void DeleteUserLogins(int userId) { diff --git a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ExternalLoginServiceTests.cs b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ExternalLoginServiceTests.cs index b6a4e6bc4e..e56891601c 100644 --- a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ExternalLoginServiceTests.cs +++ b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ExternalLoginServiceTests.cs @@ -2,14 +2,13 @@ // See LICENSE for more details. using System; -using System.Collections.Generic; using System.Linq; using System.Threading; using NUnit.Framework; using Umbraco.Cms.Core.Models.Identity; -using Umbraco.Cms.Core.Models.Membership; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Infrastructure.Persistence.Dtos; +using Umbraco.Cms.Tests.Common.Builders; using Umbraco.Cms.Tests.Common.Testing; using Umbraco.Cms.Tests.Integration.Testing; @@ -22,13 +21,13 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services { private IUserService UserService => GetRequiredService(); - private IExternalLoginService ExternalLoginService => GetRequiredService(); + private IExternalLoginService ExternalLoginService => (IExternalLoginService)GetRequiredService(); [Test] [Ignore("We don't support duplicates anymore, this removing on save was a breaking change work around, this needs to be ported to a migration")] public void Removes_Existing_Duplicates_On_Save() { - var user = new User(GlobalSettings, "Test", "test@test.com", "test", "helloworldtest"); + var user = new UserBuilder().Build(); UserService.Save(user); string providerKey = Guid.NewGuid().ToString("N"); @@ -76,7 +75,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services [Test] public void Does_Not_Persist_Duplicates() { - var user = new User(GlobalSettings, "Test", "test@test.com", "test", "helloworldtest"); + var user = new UserBuilder().Build(); UserService.Save(user); string providerKey = Guid.NewGuid().ToString("N"); @@ -92,49 +91,10 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services Assert.AreEqual(1, logins.Count); } - [Test] - public void Single_Create() - { - var user = new User(GlobalSettings, "Test", "test@test.com", "test", "helloworldtest"); - UserService.Save(user); - - var extLogin = new IdentityUserLogin("test1", Guid.NewGuid().ToString("N"), user.Id.ToString()) - { - UserData = "hello" - }; - ExternalLoginService.Save(extLogin); - - IEnumerable found = ExternalLoginService.GetExternalLogins(user.Id); - - Assert.AreEqual(1, found.Count()); - Assert.IsTrue(extLogin.HasIdentity); - Assert.IsTrue(extLogin.Id > 0); - } - - [Test] - public void Single_Update() - { - var user = new User(GlobalSettings, "Test", "test@test.com", "test", "helloworldtest"); - UserService.Save(user); - - var extLogin = new IdentityUserLogin("test1", Guid.NewGuid().ToString("N"), user.Id.ToString()) - { - UserData = "hello" - }; - ExternalLoginService.Save(extLogin); - - extLogin.UserData = "world"; - ExternalLoginService.Save(extLogin); - - var found = ExternalLoginService.GetExternalLogins(user.Id).ToList(); - Assert.AreEqual(1, found.Count); - Assert.AreEqual("world", found[0].UserData); - } - [Test] public void Multiple_Update() { - var user = new User(GlobalSettings, "Test", "test@test.com", "test", "helloworldtest"); + var user = new UserBuilder().Build(); UserService.Save(user); string providerKey1 = Guid.NewGuid().ToString("N"); @@ -162,7 +122,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services [Test] public void Can_Find_As_Extended_Type() { - var user = new User(GlobalSettings, "Test", "test@test.com", "test", "helloworldtest"); + var user = new UserBuilder().Build(); UserService.Save(user); string providerKey1 = Guid.NewGuid().ToString("N"); @@ -183,7 +143,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services [Test] public void Add_Logins() { - var user = new User(GlobalSettings, "Test", "test@test.com", "test", "helloworldtest"); + var user = new UserBuilder().Build(); UserService.Save(user); ExternalLogin[] externalLogins = new[] @@ -206,7 +166,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services [Test] public void Add_Tokens() { - var user = new User(GlobalSettings, "Test", "test@test.com", "test", "helloworldtest"); + var user = new UserBuilder().Build(); UserService.Save(user); ExternalLogin[] externalLogins = new[] @@ -231,7 +191,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services [Test] public void Add_Update_Delete_Logins() { - var user = new User(GlobalSettings, "Test", "test@test.com", "test", "helloworldtest"); + var user = new UserBuilder().Build(); UserService.Save(user); ExternalLogin[] externalLogins = new[] @@ -265,7 +225,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services [Test] public void Add_Update_Delete_Tokens() { - var user = new User(GlobalSettings, "Test", "test@test.com", "test", "helloworldtest"); + var user = new UserBuilder().Build(); UserService.Save(user); ExternalLogin[] externalLogins = new[] @@ -308,7 +268,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services [Test] public void Add_Retrieve_User_Data() { - var user = new User(GlobalSettings, "Test", "test@test.com", "test", "helloworldtest"); + var user = new UserBuilder().Build(); UserService.Save(user); ExternalLogin[] externalLogins = new[] diff --git a/src/Umbraco.Web.UI.NetCore/appsettings.json b/src/Umbraco.Web.UI.NetCore/appsettings.json index c7f192babb..0d0330b07b 100644 --- a/src/Umbraco.Web.UI.NetCore/appsettings.json +++ b/src/Umbraco.Web.UI.NetCore/appsettings.json @@ -38,7 +38,7 @@ "ConvertUrlsToAscii": "try" }, "RuntimeMinification": { - "dataFolder": "wmbraco/Data/TEMP/Smidge", + "dataFolder": "umbraco/Data/TEMP/Smidge", "version": "637510451273675926" }, "Security": { From 36f911b66f28120d1b9ff7a17027fd3f99867182 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 18 Mar 2021 15:57:53 +1100 Subject: [PATCH 6/8] Ensure IsApproved is persisted with MembersUserStore, Update member done in a trans and in specific steps, calls to SetLockoutEndDateAsync more safely with time before now. --- .../Security/MembersUserStore.cs | 6 + .../Controllers/MemberController.cs | 164 +++++++++++------- .../Controllers/UsersController.cs | 2 +- .../umbraco/UmbracoInstall/Index.cshtml | 4 +- 4 files changed, 106 insertions(+), 70 deletions(-) diff --git a/src/Umbraco.Infrastructure/Security/MembersUserStore.cs b/src/Umbraco.Infrastructure/Security/MembersUserStore.cs index 9405992ba8..6b4a735988 100644 --- a/src/Umbraco.Infrastructure/Security/MembersUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/MembersUserStore.cs @@ -595,6 +595,12 @@ namespace Umbraco.Cms.Core.Security } } + if (member.IsApproved != identityUserMember.IsApproved) + { + anythingChanged = true; + member.IsApproved = identityUserMember.IsApproved; + } + if (identityUserMember.IsPropertyDirty(nameof(MembersIdentityUser.UserName)) && member.Username != identityUserMember.UserName && identityUserMember.UserName.IsNullOrWhiteSpace() == false) { diff --git a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs index 1ba7f41d97..ad428d3b0f 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs @@ -20,6 +20,7 @@ using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.ContentEditing; using Umbraco.Cms.Core.Models.Membership; using Umbraco.Cms.Core.PropertyEditors; +using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Serialization; using Umbraco.Cms.Core.Services; @@ -58,6 +59,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers private readonly IJsonSerializer _jsonSerializer; private readonly IShortStringHelper _shortStringHelper; private readonly IPasswordChanger _passwordChanger; + private readonly IScopeProvider _scopeProvider; /// /// Initializes a new instance of the class. @@ -90,7 +92,8 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers IDataTypeService dataTypeService, IBackOfficeSecurityAccessor backOfficeSecurityAccessor, IJsonSerializer jsonSerializer, - IPasswordChanger passwordChanger) + IPasswordChanger passwordChanger, + IScopeProvider scopeProvider) : base(cultureDictionary, loggerFactory, shortStringHelper, eventMessages, localizedTextService, jsonSerializer) { _propertyEditors = propertyEditors; @@ -104,6 +107,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers _jsonSerializer = jsonSerializer; _shortStringHelper = shortStringHelper; _passwordChanger = passwordChanger; + _scopeProvider = scopeProvider; } /// @@ -260,9 +264,13 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return new ValidationErrorResult(forDisplay); } + // Create a scope here which will wrap all child data operations in a single transaction. + // We'll complete this at the end of this method if everything succeeeds, else + // all data operations will roll back. + using IScope scope = _scopeProvider.CreateScope(); + // Depending on the action we need to first do a create or update using the membership manager // this ensures that passwords are formatted correctly and also performs the validation on the provider itself. - switch (contentItem.Action) { case ContentSaveAction.Save: @@ -286,9 +294,6 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return NotFound(); } - // TODO: There's 3 things saved here and we should do this all in one transaction, which we can do here by wrapping in a scope - // but it would be nicer to have this taken care of within the Save method itself - // return the updated model MemberDisplay display = _umbracoMapper.Map(contentItem.PersistedContent); @@ -299,19 +304,20 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return new ValidationErrorResult(display, StatusCodes.Status403Forbidden); } - ILocalizedTextService localizedTextService = _localizedTextService; - // put the correct messages in switch (contentItem.Action) { case ContentSaveAction.Save: case ContentSaveAction.SaveNew: display.AddSuccessNotification( - localizedTextService.Localize("speechBubbles/editMemberSaved"), - localizedTextService.Localize("speechBubbles/editMemberSaved")); + _localizedTextService.Localize("speechBubbles/editMemberSaved"), + _localizedTextService.Localize("speechBubbles/editMemberSaved")); break; } + // Mark transaction to commit all changes + scope.Complete(); + return display; } @@ -328,7 +334,8 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers } // map the custom properties - this will already be set for new entities in our member binder - contentItem.PersistedContent.Email = contentItem.Email; + contentItem.PersistedContent.IsApproved = contentItem.IsApproved; + contentItem.PersistedContent.Email = contentItem.Email.Trim(); contentItem.PersistedContent.Username = contentItem.Username; // use the base method to map the rest of the properties @@ -394,15 +401,29 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers _memberService.Save(member); contentItem.PersistedContent = member; - await AddOrUpdateRoles(contentItem, identityMember); + ActionResult rolesChanged = await AddOrUpdateRoles(contentItem.Groups, identityMember); + if (!rolesChanged.Value && rolesChanged.Result != null) + { + return rolesChanged.Result; + } + return true; } /// - /// Update the member security data - /// If the password has been reset then this method will return the reset/generated password, otherwise will return null. + /// Update existing member data /// /// The member to save + /// + /// We need to use both IMemberService and ASP.NET Identity to do our updates because Identity is responsible for passwords/security. + /// When this method is called, the IMember will already have updated/mapped values from the http POST. + /// So then we do this in order: + /// 1. Deal with sensitive property values on IMember + /// 2. Use IMemberService to persist all changes + /// 3. Use ASP.NET and MemberUserManager to deal with lockouts + /// 4. Use ASP.NET, MemberUserManager and password changer to deal with passwords + /// 5. Deal with groups/roles + /// private async Task> UpdateMemberAsync(MemberSave contentItem) { contentItem.PersistedContent.WriterId = _backOfficeSecurityAccessor.BackOfficeSecurity.CurrentUser.Id; @@ -420,6 +441,9 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers foreach (IPropertyType sensitiveProperty in sensitiveProperties) { + // TODO: This logic seems to deviate from the logic that is in v8 where we are explitly checking + // against 3 properties: Comments, IsApproved, IsLockedOut, is the v8 version incorrect? + ContentPropertyBasic destProp = contentItem.Properties.FirstOrDefault(x => x.Alias == sensitiveProperty.Alias); if (destProp != null) { @@ -430,27 +454,37 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers } } - bool isLockedOut = contentItem.IsLockedOut; + // First save the IMember with mapped values before we start updating data with aspnet identity + _memberService.Save(contentItem.PersistedContent); - // if they were locked but now they are trying to be unlocked - if (contentItem.PersistedContent.IsLockedOut && isLockedOut == false) - { - contentItem.PersistedContent.IsLockedOut = false; - contentItem.PersistedContent.FailedPasswordAttempts = 0; - } - else if (!contentItem.PersistedContent.IsLockedOut && isLockedOut) - { - // NOTE: This should not ever happen unless someone is mucking around with the request data. - // An admin cannot simply lock a user, they get locked out by password attempts, but an admin can un-approve them - ModelState.AddModelError("custom", "An admin cannot lock a user"); - } + bool needsResync = false; MembersIdentityUser identityMember = await _memberManager.FindByIdAsync(contentItem.Id.ToString()); if (identityMember == null) { - return new ValidationErrorResult("Identity member was not found"); + return new ValidationErrorResult("Member was not found"); } + // Handle unlocking with the member manager (takes care of other nuances) + if (identityMember.IsLockedOut && contentItem.IsLockedOut == false) + { + IdentityResult unlockResult = await _memberManager.SetLockoutEndDateAsync(identityMember, DateTimeOffset.Now.AddMinutes(-1)); + if (unlockResult.Succeeded == false) + { + return new ValidationErrorResult( + $"Could not unlock for member {contentItem.Id} - error {unlockResult.Errors.ToErrorMessage()}"); + } + needsResync = true; + } + else if (identityMember.IsLockedOut == false && contentItem.IsLockedOut) + { + // NOTE: This should not ever happen unless someone is mucking around with the request data. + // An admin cannot simply lock a user, they get locked out by password attempts, but an admin can unlock them + return new ValidationErrorResult("An admin cannot lock a member"); + } + + // If we're changing the password... + // Handle changing with the member manager & password changer (takes care of other nuances) if (contentItem.Password != null) { IdentityResult validatePassword = await _memberManager.ValidatePasswordAsync(contentItem.Password.NewPassword); @@ -465,19 +499,6 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return new ValidationErrorResult("Member ID was not valid"); } - IMember foundMember = _memberService.GetById(intId.Result); - if (foundMember == null) - { - return new ValidationErrorResult("Member was not found"); - } - - IUser currentUser = _backOfficeSecurityAccessor.BackOfficeSecurity.CurrentUser; - // if the current user has access to reset/manually change the password - if (currentUser.HasSectionAccess(Constants.Applications.Members) == false) - { - return new ValidationErrorResult("The current user is not authorized"); - } - var changingPasswordModel = new ChangingPasswordModel { Id = intId.Result, @@ -485,9 +506,10 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers NewPassword = contentItem.Password.NewPassword, }; + // Change and persist the password Attempt passwordChangeResult = await _passwordChanger.ChangePasswordWithIdentityAsync(changingPasswordModel, _memberManager); - if (passwordChangeResult.Result.ChangeError?.MemberNames != null) + if (!passwordChangeResult.Success) { foreach (string memberName in passwordChangeResult.Result.ChangeError?.MemberNames) { @@ -496,33 +518,27 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return new ValidationErrorResult(new SimpleValidationModel(ModelState.ToErrorDictionary())); } - if (!passwordChangeResult.Success) - { - return new ValidationErrorResult("The password could not be changed"); - } - - // get the identity member now the password and dates have changed - identityMember = await _memberManager.FindByIdAsync(contentItem.Id.ToString()); - - //TODO: confirm this is correct - contentItem.PersistedContent.RawPasswordValue = identityMember.PasswordHash; - - if (identityMember.LastPasswordChangeDateUtc != null) - { - contentItem.PersistedContent.LastPasswordChangeDate = (DateTime)identityMember.LastPasswordChangeDateUtc; - } + needsResync = true; } - IdentityResult updatedResult = await _memberManager.UpdateAsync(identityMember); - - if (updatedResult.Succeeded == false) + // Update the roles and check for changes + ActionResult rolesChanged = await AddOrUpdateRoles(contentItem.Groups, identityMember); + if (!rolesChanged.Value && rolesChanged.Result != null) { - return new ValidationErrorResult(updatedResult.Errors.ToErrorMessage()); + return rolesChanged.Result; + } + else + { + needsResync = true; } - _memberService.Save(contentItem.PersistedContent); + // If there have been underlying changes made by ASP.NET Identity, then we need to resync the + // IMember on the PersistedContent with what is stored since it will be mapped to display. + if (needsResync) + { + contentItem.PersistedContent = _memberService.GetById(contentItem.PersistedContent.Id); + } - await AddOrUpdateRoles(contentItem, identityMember); return true; } @@ -585,10 +601,12 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers /// /// Add or update the identity roles /// - /// The member content item + /// The groups to updates /// The member as an identity user - private async Task AddOrUpdateRoles(MemberSave contentItem, MembersIdentityUser identityMember) + private async Task> AddOrUpdateRoles(IEnumerable groups, MembersIdentityUser identityMember) { + var hasChanges = false; + // We're gonna look up the current roles now because the below code can cause // events to be raised and developers could be manually adding roles to members in // their handlers. If we don't look this up now there's a chance we'll just end up @@ -597,22 +615,34 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers // find the ones to remove and remove them IEnumerable roles = currentRoles.ToList(); - string[] rolesToRemove = roles.Except(contentItem.Groups).ToArray(); + string[] rolesToRemove = roles.Except(groups).ToArray(); // Now let's do the role provider stuff - now that we've saved the content item (that is important since // if we are changing the username, it must be persisted before looking up the member roles). if (rolesToRemove.Any()) { - IdentityResult rolesIdentityResult = await _memberManager.RemoveFromRolesAsync(identityMember, rolesToRemove); + IdentityResult identityResult = await _memberManager.RemoveFromRolesAsync(identityMember, rolesToRemove); + if (!identityResult.Succeeded) + { + return ValidationErrorResult.CreateNotificationValidationErrorResult(identityResult.Errors.ToErrorMessage()); + } + hasChanges = true; } // find the ones to add and add them - string[] toAdd = contentItem.Groups.Except(roles).ToArray(); + string[] toAdd = groups.Except(roles).ToArray(); if (toAdd.Any()) { // add the ones submitted IdentityResult identityResult = await _memberManager.AddToRolesAsync(identityMember, toAdd); + if (!identityResult.Succeeded) + { + return ValidationErrorResult.CreateNotificationValidationErrorResult(identityResult.Errors.ToErrorMessage()); + } + hasChanges = true; } + + return hasChanges; } /// diff --git a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs index 54da40ac18..7ca83b00ff 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs @@ -789,7 +789,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers continue; } - var unlockResult = await _userManager.SetLockoutEndDateAsync(user, DateTimeOffset.Now); + var unlockResult = await _userManager.SetLockoutEndDateAsync(user, DateTimeOffset.Now.AddMinutes(-1)); if (unlockResult.Succeeded == false) { return new ValidationErrorResult( diff --git a/src/Umbraco.Web.UI.NetCore/umbraco/UmbracoInstall/Index.cshtml b/src/Umbraco.Web.UI.NetCore/umbraco/UmbracoInstall/Index.cshtml index 79cbe8305a..605e355894 100644 --- a/src/Umbraco.Web.UI.NetCore/umbraco/UmbracoInstall/Index.cshtml +++ b/src/Umbraco.Web.UI.NetCore/umbraco/UmbracoInstall/Index.cshtml @@ -1,4 +1,4 @@ -@using Umbraco.Extensions +@using Umbraco.Extensions @{ Layout = null; } @@ -74,6 +74,6 @@ }; - + From 3350611b16de19a85e436932226673c7b3afb2c9 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 18 Mar 2021 16:03:04 +1100 Subject: [PATCH 7/8] removes section check since it's already handled by authz policies --- src/Umbraco.Web.BackOffice/Controllers/UsersController.cs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs index 7ca83b00ff..5cd20d9f9a 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs @@ -686,12 +686,6 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers return new ValidationErrorResult("Password reset is not allowed"); } - // if the current user has access to reset/manually change the password - if (currentUser.HasSectionAccess(Constants.Applications.Users) == false) - { - return new ValidationErrorResult("The current user is not authorized"); - } - if (!currentUser.IsAdmin() && found.IsAdmin()) { return new ValidationErrorResult("The current user cannot change the password for the specified user"); From eccbb32f42e6ba82780115dfe12f38b45a2bf730 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 18 Mar 2021 16:36:23 +1100 Subject: [PATCH 8/8] Fixes build and tests --- .../Controllers/MemberControllerUnitTests.cs | 49 ++++++++++++------- .../Controllers/MemberController.cs | 2 +- 2 files changed, 32 insertions(+), 19 deletions(-) diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs index d447572861..63d587bb94 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Data; using System.Linq; using System.Threading.Tasks; using AngleSharp.Common; @@ -24,6 +25,7 @@ using Umbraco.Cms.Core.Models.ContentEditing; using Umbraco.Cms.Core.Models.Mapping; using Umbraco.Cms.Core.Models.Membership; using Umbraco.Cms.Core.PropertyEditors; +using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Serialization; using Umbraco.Cms.Core.Services; @@ -85,8 +87,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers .ReturnsAsync(() => IdentityResult.Success); var value = new MemberDisplay(); - string reason = "Validation failed"; - + // act ActionResult result = sut.PostSave(fakeMemberData).Result; var validation = result.Result as ValidationErrorResult; @@ -204,7 +205,6 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers .Setup(x => x.ValidatePasswordAsync(It.IsAny())) .ReturnsAsync(() => IdentityResult.Success); - string password = "fakepassword9aw89rnyco3938cyr^%&*()i8Y"; Mock.Get(umbracoMembersUserManager) .Setup(x => x.UpdateAsync(It.IsAny())) .ReturnsAsync(() => IdentityResult.Success); @@ -215,6 +215,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers SetupPasswordSuccess(umbracoMembersUserManager, passwordChanger); Mock.Get(memberService).Setup(x => x.GetByUsername(It.IsAny())).Returns(() => member); + Mock.Get(memberService).Setup(x => x.GetById(It.IsAny())).Returns(() => member); Mock.Get(memberService).SetupSequence( x => x.GetByEmail(It.IsAny())) .Returns(() => null) @@ -336,14 +337,16 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers Mock.Get(umbracoMembersUserManager) .Setup(x => x.ValidatePasswordAsync(It.IsAny())) .ReturnsAsync(() => IdentityResult.Success); + Mock.Get(umbracoMembersUserManager) + .Setup(x => x.AddToRolesAsync(It.IsAny(), It.IsAny>())) + .ReturnsAsync(() => IdentityResult.Success); Mock.Get(memberService).SetupSequence( x => x.GetByEmail(It.IsAny())) .Returns(() => member); MemberController sut = CreateSut(memberService, memberTypeService, memberGroupService, umbracoMembersUserManager, dataTypeService, backOfficeSecurityAccessor, passwordChanger, globalSettings, user); - string reason = "Validation failed"; - + // act ActionResult result = sut.PostSave(fakeMemberData).Result; var validation = result.Result as ValidationErrorResult; @@ -357,19 +360,18 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers [Test] [AutoMoqData] public async Task PostSaveMember_SaveExistingMember_WithNoRoles_Add1Role_ExpectSuccessResponse( - [Frozen] IMemberManager umbracoMembersUserManager, - IMemberService memberService, - IMemberTypeService memberTypeService, - IMemberGroupService memberGroupService, - IDataTypeService dataTypeService, - IBackOfficeSecurityAccessor backOfficeSecurityAccessor, - IBackOfficeSecurity backOfficeSecurity, - IPasswordChanger passwordChanger, - IOptions globalSettings, - IUser user) + [Frozen] IMemberManager umbracoMembersUserManager, + IMemberService memberService, + IMemberTypeService memberTypeService, + IMemberGroupService memberGroupService, + IDataTypeService dataTypeService, + IBackOfficeSecurityAccessor backOfficeSecurityAccessor, + IBackOfficeSecurity backOfficeSecurity, + IPasswordChanger passwordChanger, + IOptions globalSettings, + IUser user) { // arrange - string password = "fakepassword9aw89rnyco3938cyr^%&*()i8Y"; var roleName = "anyrole"; IMember member = SetupMemberTestData(out MemberSave fakeMemberData, out MemberDisplay memberDisplay, ContentSaveAction.Save); fakeMemberData.Groups = new List() @@ -386,9 +388,13 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers Mock.Get(umbracoMembersUserManager) .Setup(x => x.UpdateAsync(It.IsAny())) .ReturnsAsync(() => IdentityResult.Success); + Mock.Get(umbracoMembersUserManager) + .Setup(x => x.AddToRolesAsync(It.IsAny(), It.IsAny>())) + .ReturnsAsync(() => IdentityResult.Success); Mock.Get(memberTypeService).Setup(x => x.GetDefault()).Returns("fakeAlias"); Mock.Get(backOfficeSecurityAccessor).Setup(x => x.BackOfficeSecurity).Returns(backOfficeSecurity); Mock.Get(memberService).Setup(x => x.GetByUsername(It.IsAny())).Returns(() => member); + Mock.Get(memberService).Setup(x => x.GetById(It.IsAny())).Returns(() => member); SetupUserAccess(backOfficeSecurityAccessor, backOfficeSecurity, user); SetupPasswordSuccess(umbracoMembersUserManager, passwordChanger); @@ -465,7 +471,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers Mock.Get(dataEditor).Setup(x => x.GetValueEditor()).Returns(new TextOnlyValueEditor(Mock.Of(), Mock.Of(), new DataEditorAttribute(Constants.PropertyEditors.Aliases.TextBox, "Test Textbox", "textbox"), textService.Object, Mock.Of(), Mock.Of())); var propertyEditorCollection = new PropertyEditorCollection(new DataEditorCollection(new[] { dataEditor })); - + IMapDefinition memberMapDefinition = new MemberMapDefinition( commonMapper, new CommonTreeNodeMapper(Mock.Of()), @@ -517,7 +523,14 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.BackOffice.Controllers dataTypeService, backOfficeSecurityAccessor, new ConfigurationEditorJsonSerializer(), - passwordChanger); + passwordChanger, + Mock.Of(x => x.CreateScope( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny()) == Mock.Of())); } /// diff --git a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs index ad428d3b0f..be652860b5 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MemberController.cs @@ -511,7 +511,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers if (!passwordChangeResult.Success) { - foreach (string memberName in passwordChangeResult.Result.ChangeError?.MemberNames) + foreach (string memberName in passwordChangeResult.Result?.ChangeError?.MemberNames ?? Enumerable.Empty()) { ModelState.AddModelError(memberName, passwordChangeResult.Result.ChangeError?.ErrorMessage ?? string.Empty); }